From: Stefan Sperling Subject: Re: Third batch of qwz commits To: tech@openbsd.org Date: Tue, 29 Sep 2026 15:50:40 +0200 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? > } > } > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(sc->hal.rdpmem), 0, > + QWZ_DMA_LEN(sc->hal.rdpmem), BUS_DMASYNC_PREREAD); > #ifdef notyet > srng->timestamp = jiffies; > #endif > @@ -19444,11 +19460,16 @@ qwz_hal_alloc_cont_rdp(struct qwz_softc *sc) > return ENOMEM; > > } > - } else > + } else { > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(hal->rdpmem), 0, > + QWZ_DMA_LEN(hal->rdpmem), BUS_DMASYNC_POSTREAD); > memset(QWZ_DMA_KVA(hal->rdpmem), 0, size); > + } > > hal->rdp.vaddr = QWZ_DMA_KVA(hal->rdpmem); > hal->rdp.paddr = QWZ_DMA_DVA(hal->rdpmem); > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(hal->rdpmem), 0, > + QWZ_DMA_LEN(hal->rdpmem), BUS_DMASYNC_PREREAD); > return 0; > } > > @@ -19481,11 +19502,16 @@ qwz_hal_alloc_cont_wrp(struct qwz_softc *sc) > return ENOMEM; > > } > - } else > + } else { > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(hal->wrpmem), 0, > + QWZ_DMA_LEN(hal->wrpmem), BUS_DMASYNC_POSTWRITE); > memset(QWZ_DMA_KVA(hal->wrpmem), 0, size); > + } > > hal->wrp.vaddr = QWZ_DMA_KVA(hal->wrpmem); > hal->wrp.paddr = QWZ_DMA_DVA(hal->wrpmem); > + bus_dmamap_sync(sc->sc_dmat, QWZ_DMA_MAP(hal->wrpmem), 0, > + QWZ_DMA_LEN(hal->wrpmem), BUS_DMASYNC_PREWRITE); > return 0; > }