Download raw body.
Third batch of qwz commits
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;
> }
Third batch of qwz commits