Re: xhci problem -> general protection fault

[Ross Zwisler <zwisler@google.com>]

On Thu, Jan 07, 2021 at 10:57:19AM +0200, Mathias Nyman wrote:
> On 6.1.2021 20.52, Ross Zwisler wrote:
> > On Wed, Dec 30, 2020 at 5:31 AM Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> > <>
> >> I was able to reproduce the issue with two USB devices on a different system.
> >>
> >> I saw that the new incorrect dequeue pointer sometimes had the higher 32 bits
> >> incorrect while the lower bits were correct.
> >> So this looks like a memory access order issue.
> >>
> >> The command TRB is 16 bytes. The xhci driver writes it in four 4 byte chunks.
> >> Even if the driver writes the chunk that sets the cycle bit last, handing the TRB
> >> over to the controller, it appears that the actual write order can be different.
> >> The controller ends up reading a command TRB with updated cycle bit but old bogus
> >> values in the "new dequeue pointer" field.
> >>
> >> Adding a write memory barrier before writing the cycle bit solved the issue in my case.
> >>
> >> Whole patch series is updated, added write memory barrier, and rebased on 5.10.
> >> It can be found force-updated in the same rewrite_halt_stop_handling branch:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git rewrite_halt_stop_handling
> >> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=rewrite_halt_stop_handling
> >>
> >> Does this work for you?
> > 
> > Yes, it does!!  I verified that I'm able to reproduce the issue in less than
> > 10 seconds with this commit which is the HEAD~1 of your series:
> > 
> > a7d053d207121 xhci: handle halting transfer event properly after endpoint stop and halt raced.
> > 
> > With this commit (the final commit in your series, adding a single patch): 
> > 
> > 96887d191a88c xhci: make sure TRB is fully written before giving it to the controller
> > 
> > I ran cleanly for over 20 minutes and haven't been able to reproduce the
> > issue.  It looks like the wmb() added in that patch makes all the difference!
> > 
> > Thank you for the fix, and you can add this tag to the series:
> > 
> > Tested-by: Ross Zwisler <zwisler@google.com>
> > 
> 
> Great, thanks for testing.
> 
> I think it makes sense to cherry pick that one-liner wmb() patch first to 5.11 and stable,
> then add rest of the rewrite later.
> 
> -Mathias

Makes sense to me.  FWIW I've verified that v5.10 reproduces the issue easily,
and v5.10 with just the wmb() patch does not.

Thanks again for the fix!