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)
next prev parent reply other threads:[~2021-01-15 16:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-15 16:19 [PATCH 0/2] xhci fixes for usb-linus Mathias Nyman
2021-01-15 16:19 ` [PATCH 1/2] xhci: make sure TRB is fully written before giving it to the controller Mathias Nyman
2021-01-15 16:40 ` Sergei Shtylyov
2021-01-15 16:50 ` David Laight [this message]
2021-01-15 17:21 ` Sergei Shtylyov
2021-01-18 12:07 ` Mathias Nyman
2021-01-19 23:25 ` Ross Zwisler
2021-01-15 16:19 ` [PATCH 2/2] xhci: tegra: Delay for disabling LFPS detector Mathias Nyman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b70e0bb512d44f00ac5f8380ba450ba6@AcuMS.aculab.com \
--to=david.laight@aculab.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@linux.intel.com \
--cc=sergei.shtylyov@gmail.com \
--cc=stable@vger.kernel.org \
--cc=zwisler@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).