[PATCH] usb: xhci: fix event trb handling missed

[Bin Meng]

Hi Ran,

On Tue, Nov 10, 2020 at 3:36 PM Ran Wang <ran.wang_1 at nxp.com> wrote:
>
> Hi Bin,
>
> On Tuesday, November 10, 2020 1:43 PM Bin Meng wrote:
> >
> > Hi Ran,
> >
> > On Tue, Nov 10, 2020 at 1:30 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Ran,
> > >
> > > On Tue, Sep 22, 2020 at 1:02 PM Ran Wang <ran.wang_1 at nxp.com> wrote:
> > > >
> > > > In functiion xhci_bulk_tx(), when buffer cross 64KB boundary, it
> > > > will
> > >
> > > typo: function
>
> Got it.
>
> > > > send request in more than 1 Transfer TRB by chaining them, but then
> > > > handle only 1 event TRB to mark request completed.
> > > >
> > > > However, on Layerscape platforms (LS1028A, LS1088A, etc), we observe
> > > > xhci controller will generated more than 1 event TRB sometimes, this
> > > > cause that
> > >
> > > I am not sure if it's fair to say this is Layerscape specific
> > > behavior. Based on the xHCI spec, the spec indicates 1 event trb
> > > should be generated when IOC/ISP flag is set to 1 or an error occurs.
> >
> > Ah, ISP flag is set if the pipe is from an IN endpoint. Currently we have:
> >
> >         /* Only set interrupt on short packet for IN endpoints */
> >         if (usb_pipein(pipe))
> >             field |= TRB_ISP;
> >
> > Can you verify that if removing the above codes, and without your changes in
> > this patch, the original issue can be resolved on LS1028?
>
> Bingo, removing above codes can resolve my issue, too

Thank you for your testing.

>
> > > I will see if I can reproduce your issue on an x86 board.
> > >
> >
> > Note this patch does not apply on top of u-boot/master. Please rebase.
>
> Sure, I can rebase my patch, but I am nor sure my solution is still worth:
> xHCI says "The detection of a USB Short Packet (i.e. the actual number of
> bytes received was less than the expected number of bytes defined by the
> Transfer TRB) during a transfer does not necessarily generate an Event. "
>

Yes, that's what the spec says. So in your case, can you add some logs
to verify that there is a transfer event trb generated by the xHC and
the completion code is 13 (short packet)? You can add some debug
output in record_transfer_result().

> So does it mean above workaround you suggest would not violate spec, either
> (although current Linux kernel driver still set ISP for this case, but may have
> a more robust mechanism for event TRB handling)?

I need to do more testing to see if this is a LS1028 behavior or
generic xHCI behavior, ie: on an x86 board. Do you happen to know if
there is any errata document for LS1028 that is related to this?

>
>
> > > > function mishandle event TRB in next round call, then system hang
> > > > due to
> > > > BUG() checking.
> > > >
> > > > This patch adds a loop to make sure the event TRB for last Transfer
> > > > TRB has to be handled in time.
> > > >
> > > > Signed-off-by: Ran Wang <ran.wang_1 at nxp.com>
> > > > ---
> > > >  drivers/usb/host/xhci-ring.c | 17 ++++++++++++++---
> > > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-ring.c
> > > > b/drivers/usb/host/xhci-ring.c index 092ed6e..d77e058 100644
> > > > --- a/drivers/usb/host/xhci-ring.c
> > > > +++ b/drivers/usb/host/xhci-ring.c
> > > > @@ -578,10 +578,13 @@ int xhci_bulk_tx(struct usb_device *udev,
> > unsigned long pipe,
> > > >         int ret;
> > > >         u32 trb_fields[4];
> > > >         u64 val_64 = virt_to_phys(buffer);
> > > > +       void *last_transfer_trb_addr;
> > > > +       int available_length;
> > > >
> > > >         debug("dev=%p, pipe=%lx, buffer=%p, length=%d\n",
> > > >                 udev, pipe, buffer, length);
> > > >
> > > > +       available_length = length;
> > > >         ep_index = usb_pipe_ep_index(pipe);
> > > >         virt_dev = ctrl->devs[slot_id];
> > > >
> > > > @@ -701,7 +704,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned
> > long pipe,
> > > >                 trb_fields[2] = length_field;
> > > >                 trb_fields[3] = field | (TRB_NORMAL <<
> > > > TRB_TYPE_SHIFT);
> > > >
> > > > -               queue_trb(ctrl, ring, (num_trbs > 1), trb_fields);
> > > > +               last_transfer_trb_addr = queue_trb(ctrl, ring,
> > > > + (num_trbs > 1), trb_fields);
> > > >
> > > >                 --num_trbs;
> > > >
> > > > @@ -714,6 +717,7 @@ int xhci_bulk_tx(struct usb_device *udev,
> > > > unsigned long pipe,
> > > >
> > > >         giveback_first_trb(udev, ep_index, start_cycle, start_trb);
> > > >
> > > > +again:
> > > >         event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
> > > >         if (!event) {
> > > >                 debug("XHCI bulk transfer timed out,
> > > > aborting...\n"); @@ -722,14 +726,21 @@ int xhci_bulk_tx(struct
> > usb_device *udev, unsigned long pipe,
> > > >                 udev->act_len = 0;
> > > >                 return -ETIMEDOUT;
> > > >         }
> > > > -       field = le32_to_cpu(event->trans_event.flags);
> > > >
> > > > +       if ((void *)event->trans_event.buffer !=
> > > > + last_transfer_trb_addr) {
> > >
> > > This should be:
> > >
> > > if ((void *)le64_to_cpu(event->trans_event.buffer) !=
> > > last_transfer_trb_addr)
>
> Got it, will update this in next version (if you think my solution is still acceptable for this issue).
>
> Thanks & regards,
> Ran
>
> > > > +               available_length -=
> > > > +
> > (int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
> > > > +               xhci_acknowledge_event(ctrl);
> > > > +               goto again;
> > > > +       }
> > > > +
> > > > +       field = le32_to_cpu(event->trans_event.flags);
> > > >         BUG_ON(TRB_TO_SLOT_ID(field) != slot_id);
> > > >         BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
> > > >         BUG_ON(*(void
> > **)(uintptr_t)le64_to_cpu(event->trans_event.buffer) -
> > > >                 buffer > (size_t)length);
> > > >
> > > > -       record_transfer_result(udev, event, length);
> > > > +       record_transfer_result(udev, event, available_length);
> > > >         xhci_acknowledge_event(ctrl);
> > > >         xhci_inval_cache((uintptr_t)buffer, length);
> > > >

Regards,
Bin