Re: git: 38cbdae33b7c - main - dwc: Rewrite barrier part

On Tue, 10 Oct 2023 11:21:21 -0700
John Baldwin <jhb@FreeBSD.org> wrote:

> On 10/6/23 1:06 PM, Emmanuel Vadot wrote:
> > The branch main has been updated by manu:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=38cbdae33b7c3f772845c06f52b86c0ddeab6a17
> > 
> > commit 38cbdae33b7c3f772845c06f52b86c0ddeab6a17
> > Author:     Emmanuel Vadot <manu@FreeBSD.org>
> > AuthorDate: 2023-10-06 16:04:49 +0000
> > Commit:     Emmanuel Vadot <manu@FreeBSD.org>
> > CommitDate: 2023-10-06 17:05:45 +0000
> > 
> >      dwc: Rewrite barrier part
> >      
> >      We only need a barrier after writing the OWN bit so everything is
> >      coherent for the DMA engine.
> > ---
> >   sys/dev/dwc/dwc1000_dma.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/sys/dev/dwc/dwc1000_dma.c b/sys/dev/dwc/dwc1000_dma.c
> > index f6baa0bd3e45..4f8ae088305a 100644
> > --- a/sys/dev/dwc/dwc1000_dma.c
> > +++ b/sys/dev/dwc/dwc1000_dma.c
> > @@ -216,7 +216,6 @@ txdesc_setup(struct dwc_softc *sc, int idx, bus_addr_t paddr,
> >   	sc->txdesc_ring[idx].addr1 = (uint32_t)(paddr);
> >   	sc->txdesc_ring[idx].desc0 = desc0;
> >   	sc->txdesc_ring[idx].desc1 = desc1;
> > -	wmb();
> >   	sc->txdesc_ring[idx].desc0 |= TDESC0_OWN;
> >   	wmb();
> >   }
> > @@ -237,7 +236,6 @@ rxdesc_setup(struct dwc_softc *sc, int idx, bus_addr_t paddr)
> >   		sc->rxdesc_ring[idx].desc1 = ERDESC1_RCH |
> >   		    MIN(MCLBYTES, ERDESC1_RBS1_MASK);
> >   
> > -	wmb();
> >   	sc->rxdesc_ring[idx].desc0 = RDESC0_OWN;
> >   	wmb();
> >   	return (nidx);
> > @@ -524,7 +522,6 @@ dma1000_rxfinish_locked(struct dwc_softc *sc)
> >   
> >   		m = dwc_rxfinish_one(sc, desc, sc->rxbuf_map + idx);
> >   		if (m == NULL) {
> > -			wmb();
> >   			desc->desc0 = RDESC0_OWN;
> >   			wmb();
> >   		} else {
> 
> Hmm, are you sure?  I suspect you removed the wrong barrier here.  You want the
> other writes to update other fields in the desc to post before the write that
> sets TDESC0_OWN.  The barrier after TDESC0_OWN is useless, but the one you removed
> is probably the one that matters.  In particular you don't want TDESC0_OWN to be
> set when other writes (e.g. setting desc1) are not yet visible.
> 
> -- 
> John Baldwin
> 

 Yeah what you say make sense, the thing is on all the platform I have
I've never needed the barrier so it was hard to test for me what was
really needed, but you explanation make sense and I'll change the code.
 Thanks.

 Cheers,

-- 
Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>