Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: rge(4): fixup rge_encap() DMA syncs
To:
Patrick Wildt <patrick@blueri.se>
Cc:
kevlo@openbsd.org, dlg@openbsd.org, tech@openbsd.org
Date:
Sat, 10 Aug 2024 23:38:19 +0200

Download raw body.

Thread
> Date: Sat, 10 Aug 2024 23:11:35 +0200
> From: Patrick Wildt <patrick@blueri.se>
> 
> Hi,
> 
> a friend of mine has been complaining about watchdog timeouts on RK3588.
> I compared the driver with other ones that are stable, like dwqe(4), and
> found a few differences in behaviour.  One thing I noticed is that the
> DMA syncs on rge_encap() most certainly don't work correctly, because it
> only syncs the *last* descriptor in the chain.
> 
> I think this diff should make sure all TX descs are synced.

Looks good to me; ok kettenis@

> diff --git a/sys/dev/pci/if_rge.c b/sys/dev/pci/if_rge.c
> index b9d42eedfda..0b6c5e14fab 100644
> --- a/sys/dev/pci/if_rge.c
> +++ b/sys/dev/pci/if_rge.c
> @@ -480,24 +480,27 @@ rge_encap(struct rge_queues *q, struct mbuf *m, int idx)
>  
>  		if (cur == RGE_TX_LIST_CNT - 1)
>  			cmdsts |= RGE_TDCMDSTS_EOR;
> +		if (i == (txmap->dm_nsegs - 1))
> +			cmdsts |= RGE_TDCMDSTS_EOF;
>  
>  		d->rge_cmdsts = htole32(cmdsts);
>  
> +		bus_dmamap_sync(sc->sc_dmat, q->q_tx.rge_tx_list_map,
> +		    cur * sizeof(struct rge_tx_desc), sizeof(struct rge_tx_desc),
> +		    BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
> +
>  		last = cur;
>  		cmdsts = RGE_TDCMDSTS_OWN;
>  		cur = RGE_NEXT_TX_DESC(cur);
>  	}
>  
> -	/* Set EOF on the last descriptor. */
> -	d->rge_cmdsts |= htole32(RGE_TDCMDSTS_EOF);
> -
>  	/* Transfer ownership of packet to the chip. */
>  	d = &q->q_tx.rge_tx_list[idx];
>  
>  	d->rge_cmdsts |= htole32(RGE_TDCMDSTS_OWN);
>  
>  	bus_dmamap_sync(sc->sc_dmat, q->q_tx.rge_tx_list_map,
> -	    cur * sizeof(struct rge_tx_desc), sizeof(struct rge_tx_desc),
> +	    idx * sizeof(struct rge_tx_desc), sizeof(struct rge_tx_desc),
>  	    BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
>  
>  	/* Update info of TX queue and descriptors. */
> 
>