| Message ID | 20201118074902.9410-1-ran.wang_1@nxp.com |
|---|---|
| State | Accepted |
| Commit | 621ed49d3a2ea3c45be1cf774bef48439bd566f3 |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [v4] usb: xhci: fix lack of short packet event trb handling | expand |
Hi Marek, Bin, On Wednesday, November 18, 2020 3:49 PM, Ran Wang wrote: > > For bulk IN transfer, the codes will set ISP flag to request event TRB being > generated by xHC for the case of short packet. So when encountering > buffer-cross-64K-boundary (which we will divide payload and enqueuqe more > than 1 transfer TRB), and the first TRB ends up with a short packet condition it > will trigger an short packet code transfer event per that flag and cause more > than 1 event TRB generated for this transfer. > > However, current codes will only handle the first transfer event TRB then mark > current transfer completed, causing next transfer failure due to event TRB > mis-match. > > Such issue has been observed on some Layerscape platforms (LS1028A, > LS1088A, etc) with USB ethernet device. > > This patch adds a loop to make sure the event TRB for last transfer TRB has > been handled in time. > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > --- > Change in v4: > - Update commit message: 'for case of short packet' => 'for the case of short > packet' Has this v4 patch been accepted? Thanks & Regards, Ran > Change in v3: > - Update commit message according to v2 feedback . > - Replace (void *) with (uintptr_t) to fix below armv7 compile warning: > drivers/usb/host/xhci-ring.c: In function 'xhci_bulk_tx': > drivers/usb/host/xhci-ring.c:726:6: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > 726 | if ((void *)le64_to_cpu(event->trans_event.buffer) != > last_transfer_trb_addr) { > | ^ > > Change in v2: > - Re-write commit message to describe context more clearly. > - Add prefix 'le64_to_cpu' for 'event->trans_event.buffer'. > > drivers/usb/host/xhci-ring.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index > 13065d7..d708fc9 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -580,10 +580,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]; > > @@ -697,7 +700,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned > long pipe, > trb_fields[2] = length_field; > trb_fields[3] = field | TRB_TYPE(TRB_NORMAL); > > - queue_trb(ctrl, ring, (num_trbs > 1), trb_fields); > + last_transfer_trb_addr = queue_trb(ctrl, ring, (num_trbs > 1), > +trb_fields); > > --num_trbs; > > @@ -710,6 +713,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"); @@ -718,12 > +722,20 @@ 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 ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) > + != (uintptr_t)last_transfer_trb_addr) { > + 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); > > - record_transfer_result(udev, event, length); > + record_transfer_result(udev, event, available_length); > xhci_acknowledge_event(ctrl); > xhci_inval_cache((uintptr_t)buffer, length); > > -- > 2.7.4
Hi Ran, On Mon, Nov 30, 2020 at 9:42 AM Ran Wang <ran.wang_1@nxp.com> wrote: > > Hi Marek, Bin, > > > On Wednesday, November 18, 2020 3:49 PM, Ran Wang wrote: > > > > For bulk IN transfer, the codes will set ISP flag to request event TRB being > > generated by xHC for the case of short packet. So when encountering > > buffer-cross-64K-boundary (which we will divide payload and enqueuqe more > > than 1 transfer TRB), and the first TRB ends up with a short packet condition it > > will trigger an short packet code transfer event per that flag and cause more > > than 1 event TRB generated for this transfer. > > > > However, current codes will only handle the first transfer event TRB then mark > > current transfer completed, causing next transfer failure due to event TRB > > mis-match. > > > > Such issue has been observed on some Layerscape platforms (LS1028A, > > LS1088A, etc) with USB ethernet device. > > > > This patch adds a loop to make sure the event TRB for last transfer TRB has > > been handled in time. > > > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > Change in v4: > > - Update commit message: 'for case of short packet' => 'for the case of short > > packet' > > Has this v4 patch been accepted? I believe Marek will apply this patch. Regards, Bin
On 11/18/20 8:49 AM, Ran Wang wrote: > For bulk IN transfer, the codes will set ISP flag to request event TRB > being generated by xHC for the case of short packet. So when encountering > buffer-cross-64K-boundary (which we will divide payload and enqueuqe > more than 1 transfer TRB), and the first TRB ends up with a short packet > condition it will trigger an short packet code transfer event per that > flag and cause more than 1 event TRB generated for this transfer. > > However, current codes will only handle the first transfer event TRB > then mark current transfer completed, causing next transfer > failure due to event TRB mis-match. > > Such issue has been observed on some Layerscape platforms (LS1028A, > LS1088A, etc) with USB ethernet device. > > This patch adds a loop to make sure the event TRB for last transfer TRB > has been handled in time. Applied, thanks.
Hi Marek, On Thu, Dec 3, 2020 at 6:52 AM Marek Vasut <marex@denx.de> wrote: > > On 11/18/20 8:49 AM, Ran Wang wrote: > > For bulk IN transfer, the codes will set ISP flag to request event TRB > > being generated by xHC for the case of short packet. So when encountering > > buffer-cross-64K-boundary (which we will divide payload and enqueuqe > > more than 1 transfer TRB), and the first TRB ends up with a short packet > > condition it will trigger an short packet code transfer event per that > > flag and cause more than 1 event TRB generated for this transfer. > > > > However, current codes will only handle the first transfer event TRB > > then mark current transfer completed, causing next transfer > > failure due to event TRB mis-match. > > > > Such issue has been observed on some Layerscape platforms (LS1028A, > > LS1088A, etc) with USB ethernet device. > > > > This patch adds a loop to make sure the event TRB for last transfer TRB > > has been handled in time. > > Applied, thanks. I see in patchwork this was assigned to me and I don't see this patch got its way to mainline. Let me know if I need to apply this and send PR. Regards, Bin
On 12/16/20 4:02 AM, Bin Meng wrote: Hi [...] >>> This patch adds a loop to make sure the event TRB for last transfer TRB >>> has been handled in time. >> >> Applied, thanks. > > I see in patchwork this was assigned to me and I don't see this patch > got its way to mainline. Let me know if I need to apply this and send > PR. It is in u-boot-usb, I just need to send out PR.
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 13065d7..d708fc9 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -580,10 +580,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]; @@ -697,7 +700,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, trb_fields[2] = length_field; trb_fields[3] = field | TRB_TYPE(TRB_NORMAL); - queue_trb(ctrl, ring, (num_trbs > 1), trb_fields); + last_transfer_trb_addr = queue_trb(ctrl, ring, (num_trbs > 1), trb_fields); --num_trbs; @@ -710,6 +713,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"); @@ -718,12 +722,20 @@ 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 ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) + != (uintptr_t)last_transfer_trb_addr) { + 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); - record_transfer_result(udev, event, length); + record_transfer_result(udev, event, available_length); xhci_acknowledge_event(ctrl); xhci_inval_cache((uintptr_t)buffer, length);