[PATCH v3] usb: xhci: fix lack of short packet event trb handling

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] usb: xhci: fix lack of short packet event trb handling

Ran Wang
For bulk IN transfer, the codes will set ISP flag to request event TRB
being generated by xHC for case of short packet. So in
buffer-cross-64K-boundary case (which we will divide payload and enqueuqe
more than 1 transfer TRB), if 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 gegerated for this transfer.

However, current codes will only handle the first transfer event TRB
then mark current transfer completed, which 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 <[hidden email]>
---
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 | 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 13065d7..e0ccf01 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,19 @@ 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 *)le64_to_cpu(event->trans_event.buffer) != 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

Reply | Threaded
Open this post in threaded view
|

RE: [PATCH v3] usb: xhci: fix lack of short packet event trb handling

Ran Wang
Hi Bin,

On Wednesday, November 18, 2020 11:06 AM Ran Wang wrote:

>
> For bulk IN transfer, the codes will set ISP flag to request event TRB being
> generated by xHC for case of short packet. So in buffer-cross-64K-boundary
> case (which we will divide payload and enqueuqe more than 1 transfer TRB), if
> 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
> gegerated for this transfer.
>
> However, current codes will only handle the first transfer event TRB then mark
> current transfer completed, which 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 <[hidden email]>
> ---
> 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) {
>        |       ^
>  ...

For this version, I newly added a fix for above issue, please help check if it's OK for you.

Thanks & Regards
Ran

> 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 | 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
> 13065d7..e0ccf01 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,19 @@ 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 *)le64_to_cpu(event->trans_event.buffer) !=
> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] usb: xhci: fix lack of short packet event trb handling

Bin Meng-2
Hi Ran,

On Wed, Nov 18, 2020 at 11:02 AM Ran Wang <[hidden email]> wrote:

>
> Hi Bin,
>
> On Wednesday, November 18, 2020 11:06 AM Ran Wang wrote:
> >
> > For bulk IN transfer, the codes will set ISP flag to request event TRB being
> > generated by xHC for case of short packet. So in buffer-cross-64K-boundary
> > case (which we will divide payload and enqueuqe more than 1 transfer TRB), if
> > 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
> > gegerated for this transfer.
> >
> > However, current codes will only handle the first transfer event TRB then mark
> > current transfer completed, which 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 <[hidden email]>
> > ---
> > 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) {
> >        |       ^
> >  ...
>
> For this version, I newly added a fix for above issue, please help check if it's OK for you.
>

Are you sure you sent the correct patch? Because I see neither the
commit message was fixed, nor the fix you mentioned "Replace (void *)
with (uintptr_t)"

Regards,
Bin
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH v3] usb: xhci: fix lack of short packet event trb handling

Ran Wang
Hi Bin,

On Wednesday, November 18, 2020 11:20 AM Bin Meng wrote:

>
> Hi Ran,
>
> On Wed, Nov 18, 2020 at 11:02 AM Ran Wang <[hidden email]>
> wrote:
> >
> > Hi Bin,
> >
> > On Wednesday, November 18, 2020 11:06 AM Ran Wang wrote:
> > >
> > > For bulk IN transfer, the codes will set ISP flag to request event
> > > TRB being generated by xHC for case of short packet. So in
> > > buffer-cross-64K-boundary case (which we will divide payload and
> > > enqueuqe more than 1 transfer TRB), if 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 gegerated for this
> transfer.
> > >
> > > However, current codes will only handle the first transfer event TRB
> > > then mark current transfer completed, which 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 <[hidden email]>
> > > ---
> > > 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) {
> > >        |       ^
> > >  ...
> >
> > For this version, I newly added a fix for above issue, please help check if it's
> OK for you.
> >
>
> Are you sure you sent the correct patch? Because I see neither the commit
> message was fixed, nor the fix you mentioned "Replace (void *) with
> (uintptr_t)"

My bad, will send the correct patch soon, sorry.

Regards,
Ran

> Regards,
> Bin