Re: xhci: zero length multi-TRB inbound xfer does not work

2020-06-24 Thread sc . dying
On 2020/06/24 11:57, Patrick Wildt wrote:
> On Tue, Jun 16, 2020 at 06:55:27AM +, sc.dy...@gmail.com wrote:
>> hi,
>>
>> The function xhci_event_xfer_isoc() of sys/dev/usb/xhci.c at line 954
>> does not work with zero length multi-TRB inbound transfer.
>>
>>949   /*
>>950* If we queued two TRBs for a frame and this is the 
>> second TRB,
>>951* check if the first TRB needs accounting since it 
>> might not have
>>952* raised an interrupt in case of full data received.
>>953*/
>>954   if ((letoh32(xp->ring.trbs[trb_idx].trb_flags) & 
>> XHCI_TRB_TYPE_MASK) ==
>>955   XHCI_TRB_TYPE_NORMAL) {
>>956   frame_idx--;
>>957   if (trb_idx == 0)
>>958   trb0_idx = xp->ring.ntrb - 2;
>>959   else
>>960   trb0_idx = trb_idx - 1;
>>961   if (xfer->frlengths[frame_idx] == 0) {
>>962   xfer->frlengths[frame_idx] = 
>> XHCI_TRB_LEN(letoh32(
>>963   
>> xp->ring.trbs[trb0_idx].trb_status));
>>964   }
>>965   }
>>966   
>>967   xfer->frlengths[frame_idx] +=
>>968   
>> XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>>969   xfer->actlen += xfer->frlengths[frame_idx];
>>
>> When a multi-TRB inbound transfer TD completes with transfer length = 0,
>> the HC should generate two events: 1st event for ISOCH TRB /w ISP|CHAIN
>> and 2nd event for NORMAL TRB w/ ISP|IOC.
>> Transfer Length field (it's remain length, actually) of each event is
>> same as requested length is, i.e., transferred length is 0.
>> So when the first event raises the frlengths is set to 0 at line 967.
>> It's correct.
>> On second event, as the comment describes, xhci.c tries to calculate
>> the 1st TRB xfer length at lines 954-965. The requested length of
>> 1st TRB is stored into frlengths -- even though the xfer len is 0.
>>
>> If frlengths = 0, we cannot distinguish the case the first event is
>> not raised from the case the transferred length is 0.
>> The frlengths is already 0 so the requested length of 1st TRB is stored.
> 
> That's a really good find!  I actually do wonder if we could have the
> same issue with the non-isoc transfers, when the first TRB throws a
> short and then we get another event for the last TRB.

It may occur on non-isoc pipes and should be fixed.
I have not observed problems on non-isoc pipes, but it's because
the devices I saw did not do zero-length xfer on non-isoc pipes,
I guess.

> 
> Maybe it would make sense to record the idx of the last TRB that we have
> received an event for?  Then we could check if we already processed that
> TRB.

At first I recorded such an index and compared with current idx, but
I noticed it would be expressed as a bit.
Do you have any plan to use the software's dequeue pointer for other
purposes?

> 
> Patrick
> 
>> For example, I applied debug printf [*1], and run
>> mplayer tv:// for my webcam.
>> I see...
>>
>> #25 remain 1024 type 5 origlen 1024 frlengths[25] 0
>> #25 (omitted) frlen[25] 1024
>> #26 remain 2048 type 1 origlen 2048 frlengths[25] 1024
>>
>> These console logs show a 3072 bytes frame is splitted into
>> two TRBs and got 0 bytes. The first TRB transfers 0 bytes and
>> the second TRB transfers 0, too, but it results 1024 bytes.
>>
>> My proposal patch [*2] adds a flag to xhci_xfer that indicates the
>> TRB processed by xhci.c previously has CHAIN bit, and updates the
>> frlengths only when that flag is not set.
>>
>>
>>
>> [*1]
>> debug printf.
>> It shows only splitted isochronous TDs.
>>
>> --- sys/dev/usb/xhci.c.orig  Sun Apr  5 10:12:37 2020
>> +++ sys/dev/usb/xhci.c   Fri May 29 04:13:36 2020
>> @@ -961,12 +961,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
>>  if (xfer->frlengths[frame_idx] == 0) {
>>  xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
>>  xp->ring.trbs[trb0_idx].trb_status));
>> +printf("#%d (omitted) frlen[%d] %u\n",
>> +trb0_idx, frame_idx, xfer->frlengths[frame_idx]);
>>  }
>>  }
>>  
>>  xfer->frlengths[frame_idx] +=
>>  XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>>  xfer->actlen += xfer->frlengths[frame_idx];
>> +uint32_t trb_flags = letoh32(xp->ring.trbs[trb_idx].trb_flags);
>> +if ((trb_flags & XHCI_TRB_CHAIN) ||
>> +(trb_flags & XHCI_TRB_TYPE_MASK) == XHCI_TRB_TYPE_NORMAL) {
>> +printf("#%d remain %u type %u origlen %u frlengths[%d] %hu\n",
>> +trb_idx, remain,
>> +XHCI_TRB_TYPE(trb_flags),
>> +

Re: xhci: zero length multi-TRB inbound xfer does not work

2020-06-24 Thread Patrick Wildt
On Tue, Jun 16, 2020 at 06:55:27AM +, sc.dy...@gmail.com wrote:
> hi,
> 
> The function xhci_event_xfer_isoc() of sys/dev/usb/xhci.c at line 954
> does not work with zero length multi-TRB inbound transfer.
> 
>949/*
>950 * If we queued two TRBs for a frame and this is the 
> second TRB,
>951 * check if the first TRB needs accounting since it 
> might not have
>952 * raised an interrupt in case of full data received.
>953 */
>954if ((letoh32(xp->ring.trbs[trb_idx].trb_flags) & 
> XHCI_TRB_TYPE_MASK) ==
>955XHCI_TRB_TYPE_NORMAL) {
>956frame_idx--;
>957if (trb_idx == 0)
>958trb0_idx = xp->ring.ntrb - 2;
>959else
>960trb0_idx = trb_idx - 1;
>961if (xfer->frlengths[frame_idx] == 0) {
>962xfer->frlengths[frame_idx] = 
> XHCI_TRB_LEN(letoh32(
>963
> xp->ring.trbs[trb0_idx].trb_status));
>964}
>965}
>966
>967xfer->frlengths[frame_idx] +=
>968
> XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>969xfer->actlen += xfer->frlengths[frame_idx];
> 
> When a multi-TRB inbound transfer TD completes with transfer length = 0,
> the HC should generate two events: 1st event for ISOCH TRB /w ISP|CHAIN
> and 2nd event for NORMAL TRB w/ ISP|IOC.
> Transfer Length field (it's remain length, actually) of each event is
> same as requested length is, i.e., transferred length is 0.
> So when the first event raises the frlengths is set to 0 at line 967.
> It's correct.
> On second event, as the comment describes, xhci.c tries to calculate
> the 1st TRB xfer length at lines 954-965. The requested length of
> 1st TRB is stored into frlengths -- even though the xfer len is 0.
> 
> If frlengths = 0, we cannot distinguish the case the first event is
> not raised from the case the transferred length is 0.
> The frlengths is already 0 so the requested length of 1st TRB is stored.

That's a really good find!  I actually do wonder if we could have the
same issue with the non-isoc transfers, when the first TRB throws a
short and then we get another event for the last TRB.

Maybe it would make sense to record the idx of the last TRB that we have
received an event for?  Then we could check if we already processed that
TRB.

Patrick

> For example, I applied debug printf [*1], and run
> mplayer tv:// for my webcam.
> I see...
> 
> #25 remain 1024 type 5 origlen 1024 frlengths[25] 0
> #25 (omitted) frlen[25] 1024
> #26 remain 2048 type 1 origlen 2048 frlengths[25] 1024
> 
> These console logs show a 3072 bytes frame is splitted into
> two TRBs and got 0 bytes. The first TRB transfers 0 bytes and
> the second TRB transfers 0, too, but it results 1024 bytes.
> 
> My proposal patch [*2] adds a flag to xhci_xfer that indicates the
> TRB processed by xhci.c previously has CHAIN bit, and updates the
> frlengths only when that flag is not set.
> 
> 
> 
> [*1]
> debug printf.
> It shows only splitted isochronous TDs.
> 
> --- sys/dev/usb/xhci.c.orig   Sun Apr  5 10:12:37 2020
> +++ sys/dev/usb/xhci.cFri May 29 04:13:36 2020
> @@ -961,12 +961,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
>   if (xfer->frlengths[frame_idx] == 0) {
>   xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
>   xp->ring.trbs[trb0_idx].trb_status));
> + printf("#%d (omitted) frlen[%d] %u\n",
> + trb0_idx, frame_idx, xfer->frlengths[frame_idx]);
>   }
>   }
>  
>   xfer->frlengths[frame_idx] +=
>   XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>   xfer->actlen += xfer->frlengths[frame_idx];
> + uint32_t trb_flags = letoh32(xp->ring.trbs[trb_idx].trb_flags);
> + if ((trb_flags & XHCI_TRB_CHAIN) ||
> + (trb_flags & XHCI_TRB_TYPE_MASK) == XHCI_TRB_TYPE_NORMAL) {
> + printf("#%d remain %u type %u origlen %u frlengths[%d] %hu\n",
> + trb_idx, remain,
> + XHCI_TRB_TYPE(trb_flags),
> + XHCI_TRB_LEN(le32toh(xp->ring.trbs[trb_idx].trb_status)),
> + frame_idx, xfer->frlengths[frame_idx]);
> + }
>  
>   if (xx->index != trb_idx)
>   return (1);
> 
> [*2]
> patch
> 
> --- sys/dev/usb/xhcivar.h.origSun Oct  6 21:19:28 2019
> +++ sys/dev/usb/xhcivar.h Fri May 22 04:19:57 2020
> @@ -40,6 +40,7 @@ struct xhci_xfer {
>   struct usbd_xfer xfer;
>   int  index; /* Index of the last TRB */
>   size_t   

xhci: zero length multi-TRB inbound xfer does not work

2020-06-16 Thread sc . dying
hi,

The function xhci_event_xfer_isoc() of sys/dev/usb/xhci.c at line 954
does not work with zero length multi-TRB inbound transfer.

   949  /*
   950   * If we queued two TRBs for a frame and this is the second TRB,
   951   * check if the first TRB needs accounting since it might not 
have
   952   * raised an interrupt in case of full data received.
   953   */
   954  if ((letoh32(xp->ring.trbs[trb_idx].trb_flags) & 
XHCI_TRB_TYPE_MASK) ==
   955  XHCI_TRB_TYPE_NORMAL) {
   956  frame_idx--;
   957  if (trb_idx == 0)
   958  trb0_idx = xp->ring.ntrb - 2;
   959  else
   960  trb0_idx = trb_idx - 1;
   961  if (xfer->frlengths[frame_idx] == 0) {
   962  xfer->frlengths[frame_idx] = 
XHCI_TRB_LEN(letoh32(
   963  xp->ring.trbs[trb0_idx].trb_status));
   964  }
   965  }
   966  
   967  xfer->frlengths[frame_idx] +=
   968  XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - 
remain;
   969  xfer->actlen += xfer->frlengths[frame_idx];

When a multi-TRB inbound transfer TD completes with transfer length = 0,
the HC should generate two events: 1st event for ISOCH TRB /w ISP|CHAIN
and 2nd event for NORMAL TRB w/ ISP|IOC.
Transfer Length field (it's remain length, actually) of each event is
same as requested length is, i.e., transferred length is 0.
So when the first event raises the frlengths is set to 0 at line 967.
It's correct.
On second event, as the comment describes, xhci.c tries to calculate
the 1st TRB xfer length at lines 954-965. The requested length of
1st TRB is stored into frlengths -- even though the xfer len is 0.

If frlengths = 0, we cannot distinguish the case the first event is
not raised from the case the transferred length is 0.
The frlengths is already 0 so the requested length of 1st TRB is stored.

For example, I applied debug printf [*1], and run
mplayer tv:// for my webcam.
I see...

#25 remain 1024 type 5 origlen 1024 frlengths[25] 0
#25 (omitted) frlen[25] 1024
#26 remain 2048 type 1 origlen 2048 frlengths[25] 1024

These console logs show a 3072 bytes frame is splitted into
two TRBs and got 0 bytes. The first TRB transfers 0 bytes and
the second TRB transfers 0, too, but it results 1024 bytes.

My proposal patch [*2] adds a flag to xhci_xfer that indicates the
TRB processed by xhci.c previously has CHAIN bit, and updates the
frlengths only when that flag is not set.



[*1]
debug printf.
It shows only splitted isochronous TDs.

--- sys/dev/usb/xhci.c.orig Sun Apr  5 10:12:37 2020
+++ sys/dev/usb/xhci.c  Fri May 29 04:13:36 2020
@@ -961,12 +961,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
if (xfer->frlengths[frame_idx] == 0) {
xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
xp->ring.trbs[trb0_idx].trb_status));
+   printf("#%d (omitted) frlen[%d] %u\n",
+   trb0_idx, frame_idx, xfer->frlengths[frame_idx]);
}
}
 
xfer->frlengths[frame_idx] +=
XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
xfer->actlen += xfer->frlengths[frame_idx];
+   uint32_t trb_flags = letoh32(xp->ring.trbs[trb_idx].trb_flags);
+   if ((trb_flags & XHCI_TRB_CHAIN) ||
+   (trb_flags & XHCI_TRB_TYPE_MASK) == XHCI_TRB_TYPE_NORMAL) {
+   printf("#%d remain %u type %u origlen %u frlengths[%d] %hu\n",
+   trb_idx, remain,
+   XHCI_TRB_TYPE(trb_flags),
+   XHCI_TRB_LEN(le32toh(xp->ring.trbs[trb_idx].trb_status)),
+   frame_idx, xfer->frlengths[frame_idx]);
+   }
 
if (xx->index != trb_idx)
return (1);

[*2]
patch

--- sys/dev/usb/xhcivar.h.orig  Sun Oct  6 21:19:28 2019
+++ sys/dev/usb/xhcivar.h   Fri May 22 04:19:57 2020
@@ -40,6 +40,7 @@ struct xhci_xfer {
struct usbd_xfer xfer;
int  index; /* Index of the last TRB */
size_t   ntrb;  /* Number of associated TRBs */
+   bool trb_chained;
 };
 
 struct xhci_ring {
--- sys/dev/usb/xhci.c.orig Sun Apr  5 10:12:37 2020
+++ sys/dev/usb/xhci.c  Thu Jun  4 05:39:03 2020
@@ -958,15 +958,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
trb0_idx = xp->ring.ntrb - 2;
else
trb0_idx = trb_idx - 1;
-   if (xfer->frlengths[frame_idx] == 0) {
+   if (xfer->frlengths[frame_idx] == 0 && !xx->trb_chained) {
xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
xp->ring.trbs[trb0_idx].trb_status));
}