Re: [PATCH v4] usb: xhci: fix lack of short packet event trb handling
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.
Re: [PATCH v4] usb: xhci: fix lack of short packet event trb handling
Hi Marek, On Thu, Dec 3, 2020 at 6:52 AM Marek Vasut 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
Re: [PATCH v4] usb: xhci: fix lack of short packet event trb handling
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.
Re: [PATCH v4] usb: xhci: fix lack of short packet event trb handling
Hi Ran, On Mon, Nov 30, 2020 at 9:42 AM Ran Wang 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 > > Reviewed-by: Bin Meng > > --- > > 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
RE: [PATCH v4] usb: xhci: fix lack of short packet event trb handling
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
> Reviewed-by: Bin Meng
> ---
> 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
[PATCH v4] usb: xhci: fix lack of short packet event trb handling
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
Reviewed-by: Bin Meng
---
Change in v4:
- Update commit message: 'for case of short packet' => 'for the case of short
packet'
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
