RE: [PATCH 1/2] xhci: make sure TRB is fully written before giving it to the controller

From: David Laight <David.Laight@ACULAB.COM>
To: 'Sergei Shtylyov' <sergei.shtylyov@gmail.com>,
	Mathias Nyman <mathias.nyman@linux.intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Ross Zwisler <zwisler@google.com>
Subject: RE: [PATCH 1/2] xhci: make sure TRB is fully written before giving it to the controller
Date: Fri, 15 Jan 2021 16:50:01 +0000	[thread overview]
Message-ID: <b70e0bb512d44f00ac5f8380ba450ba6@AcuMS.aculab.com> (raw)
In-Reply-To: <42c6632e-28f1-9aae-e1a6-3525bb493c58@gmail.com>

From: Sergei Shtylyov
> Sent: 15 January 2021 16:40
> 
> On 1/15/21 7:19 PM, Mathias Nyman wrote:
> 
> > Once the command ring doorbell is rung the xHC controller will parse all
> > command TRBs on the command ring that have the cycle bit set properly.
> >
> > If the driver just started writing the next command TRB to the ring when
> > hardware finished the previous TRB, then HW might fetch an incomplete TRB
> > as long as its cycle bit set correctly.
> >
> > A command TRB is 16 bytes (128 bits) long.
> > Driver writes the command TRB in four 32 bit chunks, with the chunk
> > containing the cycle bit last. This does however not guarantee that
> > chunks actually get written in that order.
> >
> > This was detected in stress testing when canceling URBs with several
> > connected USB devices.
> > Two consecutive "Set TR Dequeue pointer" commands got queued right
> > after each other, and the second one was only partially written when
> > the controller parsed it, causing the dequeue pointer to be set
> > to bogus values. This was seen as error messages:
> >
> > "Mismatch between completed Set TR Deq Ptr command & xHCI internal state"
> >
> > Solution is to add a write memory barrier before writing the cycle bit.
> >
> > Cc: <stable@vger.kernel.org>
> > Tested-by: Ross Zwisler <zwisler@google.com>
> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > ---
> >  drivers/usb/host/xhci-ring.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 5677b81c0915..cf0c93a90200 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2931,6 +2931,8 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> >  	trb->field[0] = cpu_to_le32(field1);
> >  	trb->field[1] = cpu_to_le32(field2);
> >  	trb->field[2] = cpu_to_le32(field3);
> > +	/* make sure TRB is fully written before giving it to the controller */
> > +	wmb();
> 
>    Have you tried the lighter barrier, dma_wmb()? IIRC, it exists for these exact cases...

Isn't dma_wmb() needed between the last memory write and the io_write to the doorbell?
Here we need to ensure the two memory writes aren't re-ordered.
Apart from alpha isn't a barrier() likely to be enough for that.
It is worth checking that the failing compiles didn't have the writes reordered.

	David

> 
> >  	trb->field[3] = cpu_to_le32(field4);
> >
> >  	trace_xhci_queue_trb(ring, trb);
> 
> MBR, Sergei

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)