From: Kirill A. Korinsky Subject: Re: Third batch of qwz commits To: tech@openbsd.org Date: Tue, 29 Sep 2026 19:56:01 +0200 On Tue, 29 Sep 2026 15:50:40 +0200, Stefan Sperling wrote: > > On Tue, Sep 29, 2026 at 01:57:06PM +0200, Kirill A. Korinsky wrote: > > Stefan, > > > > the 2nd batch without one commit in question was commited, let keep for > > after release. > > > > So, here the third batch of qwz commits. Here 11 commits 7 of them almost > > just of backports, and 4 had required some working around. > > > > Ok? > > Ok, though qwx.c r1.140 was not adapted as-is. Your version of the patch > is adding a lot more DMA syncs than I did. See my questions below: > > > Based on sys/dev/ic/qwx.c,v 1.140 and sys/dev/ic/qwx.c,v 1.114 > > > > Pair DMA synchronization around shared pointer accesses in SRNG > > begin and end; prpeare new or reused pointer buffers for DMA and retain > > volatile reads of device updated pointers. > > --- > > sys/dev/ic/qwz.c | 36 +++++++++++++++++++++++++++++++----- > > 1 file changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/sys/dev/ic/qwz.c b/sys/dev/ic/qwz.c > > index 4c9447c9fbf..a4325b80550 100644 > > --- a/sys/dev/ic/qwz.c > > +++ b/sys/dev/ic/qwz.c > > @@ -7322,6 +7322,9 @@ qwz_hal_srng_access_begin(struct qwz_softc *sc, struct hal_srng *srng) > > #ifdef notyet > > lockdep_assert_held(&srng->lock); > > #endif > > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.rdpmem), 0, > > + QWZ_DMA_LEN(sc->hal.rdpmem), BUS_DMASYNC_POSTREAD); > > In qwz, the above sync is done elsewhere, more precisely below ... > > > + > > if (srng->ring_dir == HAL_SRNG_DIR_SRC) { > > srng->u.src_ring.cached_tp = > > *(volatile uint32_t *)srng->u.src_ring.tp_addr; > > @@ -7338,6 +7341,8 @@ qwz_hal_srng_access_begin(struct qwz_softc *sc, struct hal_srng *srng) > > srng->u.dst_ring.cached_hp = > > *(volatile uint32_t *)srng->u.dst_ring.hp_addr; > > ... this line. > > > } > > And this sync call doesn't exist in qwx: > > > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.rdpmem), 0, > > + QWZ_DMA_LEN(sc->hal.rdpmem), BUS_DMASYNC_PREREAD); > > } > > > > void > > @@ -7346,8 +7351,13 @@ qwz_hal_srng_access_end(struct qwz_softc *sc, struct hal_srng *srng) > > #ifdef notyet > > lockdep_assert_held(&srng->lock); > > #endif > > - /* TODO: See if we need a write memory barrier here */ > > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.rdpmem), 0, > > + QWZ_DMA_LEN(sc->hal.rdpmem), BUS_DMASYNC_POSTREAD); > > And the above POSTREAD sync isn't present in qwx. > > The idea I followed with my change is that we want a POSTWRITE sync > after the driver has written to descriptors to shared memory (host->device) > and will be moving the ring pointer which the hardware will read. > > And want a POSTREAD sync before reading descriptors which the hardware > has written for us (device->memory), before we read the updated ring > pointer set by hardware. > > What are the extra DMA syncs trying to achieve on top of this? > > > + > > if (srng->flags & HAL_SRNG_FLAGS_LMAC_RING) { > > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.wrpmem), 0, > > + QWZ_DMA_LEN(sc->hal.wrpmem), BUS_DMASYNC_POSTWRITE); > > + > > /* For LMAC rings, ring pointer updates are done through FW and > > * hence written to a shared memory location that is read by FW > > */ > > @@ -7356,9 +7366,12 @@ qwz_hal_srng_access_end(struct qwz_softc *sc, struct hal_srng *srng) > > *(volatile uint32_t *)srng->u.src_ring.tp_addr; > > *srng->u.src_ring.hp_addr = srng->u.src_ring.hp; > > } else { > > - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr; > > + srng->u.dst_ring.last_hp = > > + *(volatile uint32_t *)srng->u.dst_ring.hp_addr; > > *srng->u.dst_ring.tp_addr = srng->u.dst_ring.tp; > > } > > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.wrpmem), 0, > > + QWZ_DMA_LEN(sc->hal.wrpmem), BUS_DMASYNC_PREWRITE); > > } else { > > if (srng->ring_dir == HAL_SRNG_DIR_SRC) { > > srng->u.src_ring.last_tp = > > @@ -7367,12 +7380,15 @@ qwz_hal_srng_access_end(struct qwz_softc *sc, struct hal_srng *srng) > > (unsigned long)srng->u.src_ring.hp_addr - > > (unsigned long)sc->mem, srng->u.src_ring.hp); > > } else { > > - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr; > > + srng->u.dst_ring.last_hp = > > + *(volatile uint32_t *)srng->u.dst_ring.hp_addr; > > sc->ops.write32(sc, > > (unsigned long)srng->u.dst_ring.tp_addr - > > (unsigned long)sc->mem, srng->u.dst_ring.tp); > > I didn't add extra syncs here because these cases are updating ring > pointers by writing data across the PCI bus, not shared DMA memory. > The assumption would be that an update via a PCI transaction would not > result in an inconsistent view of DMA memory vs. ring pointer for the > device or for the host. Maybe my assumption is wrong. I'm neither a > PCI nor a memory coherency expert. > > I suppose adding all these extra DMA syncs won't hurt. But I would like > to understand why they are needed in all these extra places. > Without improving my own understanding of this stuff I will not be able > put DMA syncs in correct places. This stuff is still mostly voodoo to me. > In the past I've usually been asking either patrick@ or kettenis@ to > confirm my DMA sync choices, and I often got them wrong. > > Can you explain the choices you've made here? > Frankly? The comment trigger me to think about this and because as far as I understand it is harmless (am I wrong?) and I'm a bit paranoid... so I decided that it was sane to add sync also here when I was here, because tracing that kind of issues is insane. -- wbr, Kirill