* [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD
@ 2019-11-13 13:06 eli.billauer
2019-11-18 16:08 ` Mathias Nyman
0 siblings, 1 reply; 3+ messages in thread
From: eli.billauer @ 2019-11-13 13:06 UTC (permalink / raw)
To: mathias.nyman; +Cc: mathias.nyman, linux-usb, Eli Billauer
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
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD
2019-11-13 13:06 [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD eli.billauer
@ 2019-11-18 16:08 ` Mathias Nyman
2019-11-19 11:48 ` Eli Billauer
0 siblings, 1 reply; 3+ messages in thread
From: Mathias Nyman @ 2019-11-18 16:08 UTC (permalink / raw)
To: eli.billauer; +Cc: mathias.nyman, linux-usb
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,
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD
2019-11-18 16:08 ` Mathias Nyman
@ 2019-11-19 11:48 ` Eli Billauer
0 siblings, 0 replies; 3+ messages in thread
From: Eli Billauer @ 2019-11-19 11:48 UTC (permalink / raw)
To: Mathias Nyman; +Cc: mathias.nyman, linux-usb
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,
>
>
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-19 11:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 13:06 [PATCH] xhci: No XHCI_TRUST_TX_LENGTH check in the absence of matching TD eli.billauer
2019-11-18 16:08 ` Mathias Nyman
2019-11-19 11:48 ` Eli Billauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).