Re: [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD

[Eli Billauer <eli.billauer@gmail.com>]

Hello,

I've taken the liberty to add parentheses for sake of clarity, so I 
tested the following patch (against kernel 5.3.0) with my Resesas USB 
controller. And as expected, it does the job: The warnings are not printed.

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9741cdeea9d7..b062e3a19e95 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2378,7 +2378,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
      case COMP_SUCCESS:
          if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
              break;
-        if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
+        if ((xhci->quirks & XHCI_TRUST_TX_LENGTH) ||
+            ep_ring->last_td_was_short)
              trb_comp_code = COMP_SHORT_PACKET;
          else
              xhci_warn_ratelimited(xhci,

So it solves the problem for me, and hopefully for future Renesas 
controllers with the same issue.

Regards,
    Eli

On 18/11/19 18:08, Mathias Nyman wrote:
> On 13.11.2019 15.06, eli.billauer@gmail.com wrote:
>> From: Eli Billauer <eli.billauer@gmail.com>
>>
>> When an IN transfer ends with a short packet, the xHCI controller is
>> required to submit an event TRB with Completion Code COMP_SHORT_PACKET
>> against the data TRB that was in effect when the short packet 
>> arrived, as
>> well as any event TRBs it submits on behalf of this transfer.
>>
>> Alas, some controllers (e.g. Renesas) mark the subsequent events TRBs 
>> (if
>> any) with COMP_SUCCESS. As these subsequent event TRBs are useless, they
>> are ignored on the basis that they have no matching TD queued (it was
>> dequeued in response to the first COMP_SHORT_PACKET event TRB).
>>
>> Accordingly, the quirk handling and kernel log warning is moved to after
>> the TD match check, in particular in order to avoid unnecessary warnings
>> messages.
>>
>> Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 19 +++++++++++--------
>>   1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 9741cdeea9d7..96680eb71a45 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2376,14 +2376,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>        * transfer type
>>        */
>>       case COMP_SUCCESS:
>> -        if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
>> -            break;
>> -        if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
>> -            trb_comp_code = COMP_SHORT_PACKET;
>> -        else
>> -            xhci_warn_ratelimited(xhci,
>> -                          "WARN Successful completion on short TX 
>> for slot %u ep %u: needs XHCI_TRUST_TX_LENGTH quirk?\n",
>> -                          slot_id, ep_index);
>>       case COMP_SHORT_PACKET:
>>           break;
>>       /* Completion codes for endpoint stopped state */
>> @@ -2586,6 +2578,17 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>               skip_isoc_td(xhci, td, event, ep, &status);
>>               goto cleanup;
>>           }
>> +
>> +        if ((trb_comp_code == COMP_SUCCESS) &&
>> +            (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0)) {
>> +            if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
>> +                trb_comp_code = COMP_SHORT_PACKET;
>> +            else
>> +                xhci_warn_ratelimited(xhci,
>> +                              "WARN Successful completion on short 
>> TX for slot %u ep %u: needs XHCI_TRUST_TX_LENGTH quirk?\n",
>> +                              slot_id, ep_index);
>> +        }
>> +
>>           if (trb_comp_code == COMP_SHORT_PACKET)
>>               ep_ring->last_td_was_short = true;
>>           else
>>
>
> I'd hate to rip out the success case from the switch statement where 
> the other
> completion codes are handled. We're still only making a choice about a 
> warning
> message
>
> How about handling all COMP_SUCCESS cases with remaining data after a 
> short
> transfer as COMP_SHORT_PACKET by default?
>
> The code below won't behave exactly the same as in your patch, but should
> do the trick in your Renesas case as well. Can you try it out?
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 9ebaa8e132a9..d23f7408c81f 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2381,7 +2381,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>         case COMP_SUCCESS:
>                 if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) == 0)
>                         break;
> -               if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
> +               if (xhci->quirks & XHCI_TRUST_TX_LENGTH ||
> +                   ep_ring->last_td_was_short)
>                         trb_comp_code = COMP_SHORT_PACKET;
>                 else
>                         xhci_warn_ratelimited(xhci,
>
>