Re: [RFT PATCH] xhci: Handle spurious events on Etron host isoc enpoints

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 3.3.2025 12.34, Michał Pecio wrote:
> 
> Works here too, modulo the obvious build problem.

Thanks for testing on both Etron and Renesas hosts.

> 
> Etron with errors:
> [ 1064.865311] xhci_hcd 0000:06:00.0: Transfer error for slot 1 ep 2 on endpoint
> [ 1064.865322] xhci_hcd 0000:06:00.0: Successful completion on short TX for slot 1 ep 2 with last td comp code 4
> [ 1064.865326] xhci_hcd 0000:06:00.0: Spurious event dma 0x00000000ffef88c0, comp_code 13 after 4
> 
> Renesas with short packets:
> [ 1365.299218] xhci_hcd 0000:08:00.0: Successful completion on short TX for slot 1 ep 2 with last td comp code 13
> [ 1365.299223] xhci_hcd 0000:08:00.0: Spurious event dma 0x00000000ffbbf870, comp_code 13 after 13
> 
> BTW, it says "comp_code 13 after something" because of this crazy
> TRUST_TX_LENGTH hack, which changes trb_comp_code if it's success
> but the residual is nonzero. If I remove the hack,
> 
> Etron:
> [ 2713.630443] xhci_hcd 0000:06:00.0: Spurious event dma 0x00000000ff3b6130, comp_code 1 after 4
> 
> Renesas:
> [ 4033.652300] xhci_hcd 0000:08:00.0: Spurious event dma 0x00000000ffcd1b80, comp_code 1 after 13
> 
> 
> The hack could almost be removed now, but if there really are HCs
> which report Success on the first event, this won't work for them:

This looks better, and I agree that the hack/quirk is annoying, but in fear of regression I
don't want to touch that in this patch yet.

> 
>> +static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
>> +					   struct xhci_ring *ring)
>> +{
>> +	switch (ring->old_trb_comp_code) {
>> +	case COMP_SHORT_PACKET:
>> +		return xhci->quirks & XHCI_SPURIOUS_SUCCESS;
> 
> Could it work without relying on fictional COMP_SHORT_PACKET events?
> 
>> +			if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
>> +				xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
>> +					 &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
>> +				ep_ring->old_trb_comp_code = trb_comp_code;
> 
> This part will (quite arbitrarily IMO) not execute if td_list is empty.

Yes, if td_list is empty we don't take this path, and we don't print any
"ERROR Transfer event TRB DMA ptr not part of current TD..." message either,
just like before.

> 
> I had this idea that "empty td_list" and "no matching TD on td_list"
> are practically identical cases, and their code could be merged.'

Possibly, but not in this patch.

Thanks
Mathias