Download raw body.
Third batch of qwz commits
On Tue, 29 Sep 2026 15:50:40 +0200,
Stefan Sperling <stsp@stsp.name> 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
Third batch of qwz commits