Re: Fix a bug in sfcc_cache_wbinv_range()

[Visa Hankala]

On Sat, Jan 15, 2022 at 01:34:35PM +0100, Mark Kettenis wrote:
> > Date: Sat, 15 Jan 2022 12:21:59 +0000
> > From: Visa Hankala <v...@hankala.org>
> > 
> > If the ending address in sfcc_cache_wbinv_range(), pa + len, is not
> > cache-line-aligned, the function spins because len wraps around.
> > The following patch fixes this.
> > 
> > There still is a subtle detail. If len is zero but pa is non-aligned,
> > the function is not a no-op. However, shouldn't upper layers handle
> > things so that zero length does not reach this deep?
> 
> Probably.  But one could also argue that even when the length is zero,
> this should be a barrier.  And the barrier doesn't really hurt.  So I
> don't think we need to worry about this.
> 
> I noticed that len has the type paddr_t.  That is clealy wrong and
> probably should be psize_t.  Can you fix that as well?

Sure, I will fix it.

It occurred to me that my patch handles incorrectly the upper end of
the paddr_t range. At least in theory, pa + len may wrap to zero.
I have adjusted the code in an attempt to fix this. Now `end' is
rounded up and equality comparison is used to cover wrapping.

OK?

Index: arch/riscv64/dev/sfcc.c
===================================================================
RCS file: src/sys/arch/riscv64/dev/sfcc.c,v
retrieving revision 1.2
diff -u -p -r1.2 sfcc.c
--- arch/riscv64/dev/sfcc.c     22 May 2021 17:07:28 -0000      1.2
+++ arch/riscv64/dev/sfcc.c     15 Jan 2022 17:38:03 -0000
@@ -93,18 +93,19 @@ sfcc_attach(struct device *parent, struc
 }
 
 void
-sfcc_cache_wbinv_range(paddr_t pa, paddr_t len)
+sfcc_cache_wbinv_range(paddr_t pa, psize_t len)
 {
        struct sfcc_softc *sc = sfcc_sc;
+       paddr_t end, mask;
 
-       len += pa & (sc->sc_line_size - 1);
-       pa &= ~((paddr_t)sc->sc_line_size - 1);
+       mask = sc->sc_line_size - 1;
+       end = (pa + len + mask) & ~mask;
+       pa &= ~mask;
 
        __asm volatile ("fence iorw,iorw" ::: "memory");
-       while (len > 0) {
+       while (pa != end) {
                bus_space_write_8(sc->sc_iot, sc->sc_ioh, SFCC_FLUSH64, pa);
                __asm volatile ("fence iorw,iorw" ::: "memory");
                pa += sc->sc_line_size;
-               len -= sc->sc_line_size;
        }
 }