Re: Problematic Set TR Deq error handling series in xhci-next

["Neronin, Niklas" <niklas.neronin@linux.intel.com>]

> I noticed that xhci/for-usb-next now includes a series which tries
> to handle Set TR Deq errors. It strikes me as a solution looking for
> a problem, and frankly creating new problems where none existed ;)

Hi,

The purpose of this series is to add some error handling and improve
warning messages. Fixing the root cause is another task altogether.

Before, the only difference between a Set TR Deq command failure
and success was that, in the case of failure, a warning was printed,
and the xhci driver ring dequeue pointer was not moved. Otherwise,
a Set TR Deq command failure behaved as if it were successful.

In my opinion, instead of ignoring the Set TR Deq errors it would be
better to retry the command if the error is due to something easily
fixed, such as wrong EP state or Slot state.
In the worst-case scenario, the xhci driver will still fail, but with
a more specific warning message.

> I am aware of three cases leading to errors being handled here, and
> none of them is addressed. One is harmless and easy to fix properly,
> but this series appears to turn it into a "never give back the URB"
> disaster. Tests pending, I hope to find some time this weekend.
> > ...
> 
> The case of "running" (or "halted", which "running" can morth into)
> is possibly more useful, because we assume it's caused by illegal
> state changes without driver's knowledge. But these are supposed to
> be detected and fixed by handle_cmd_stop_ep(), because they cause
> other problems too. It's unclear if retrying Stop Endpoint and Set
> TR Deq again will solve any case not solved yet, but risk exists of
> infinite retries on broken HW. (And HW is broken if we are here).
The infinite retry loop is a good point, did not consider this.
But wouldn't the Stop EP command fail first, otherwise the state is
correct for the Set TR Deq?

Do you still think these changes might result in more negative impacts
than positive ones?

Thank you for the review and comments.

Best Regards,
Niklas

> 
> Queuing a Soft Retry and then doing Set TR Deq out of the halted TD
> instead of restarting the ring is a weird thing to do and IDK if HW
> will get it right. IIRC, some ASMedia had issues in similar cases:
> it would retry the TD anyway, despite Set TR Deq.
> 
> ...
> 
> Regards,
> Michal