From: Kevin Lo <kevlo@openbsd.org>
Subject: Re: rge(4): fixup rge_encap() DMA syncs
To: David Gwynne <david@gwynne.id.au>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, Patrick Wildt <patrick@blueri.se>, dlg@openbsd.org, tech@openbsd.org
Date: Mon, 12 Aug 2024 10:34:33 +0800

On Sun, Aug 11, 2024 at 10:03:38AM +1000, David Gwynne wrote:
> 
> On Sat, Aug 10, 2024 at 11:38:19PM +0200, Mark Kettenis wrote:
> > > 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@
> 
> i think the free space checks on the ring are wrong too. the way i read
> the current code, we could fill the ring too much and overwrite some
> descriptors from another packet. you would have to be extremely unlucky
> for this to happen though.
> 
> ok?

Sure. ok kevlo@

> Index: pci/if_rge.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_rge.c,v
> diff -u -p -r1.28 if_rge.c
> --- pci/if_rge.c	10 Aug 2024 21:53:06 -0000	1.28
> +++ pci/if_rge.c	10 Aug 2024 23:59:46 -0000
> @@ -581,7 +581,7 @@ rge_start(struct ifqueue *ifq)
>  	free -= idx;
>  
>  	for (;;) {
> -		if (RGE_TX_NSEGS >= free + 2) {
> +		if (free < RGE_TX_NSEGS + 2) {
>  			ifq_set_oactive(&ifp->if_snd);
>  			break;
>  		}
> Index: ic/re.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/re.c,v
> diff -u -p -r1.217 re.c
> --- ic/re.c	19 Jan 2024 03:46:14 -0000	1.217
> +++ ic/re.c	10 Aug 2024 23:59:46 -0000
> @@ -1834,7 +1834,7 @@ re_start(struct ifqueue *ifq)
>  	free -= idx;
>  
>  	for (;;) {
> -		if (sc->rl_ldata.rl_tx_ndescs >= free + 2) {
> +		if (free < sc->rl_ldata.rl_tx_ndescs + 2) {
>  			ifq_set_oactive(ifq);
>  			break;
>  		}