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...@gmail.com> wrote: > > > > Hi Ran, > > > > On Tue, Sep 22, 2020 at 1:02 PM Ran Wang <ran.wan...@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 > > 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. " 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)? > > > 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.wan...@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