Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

["Michał Pecio" <michal.pecio@gmail.com>]

Hi,

> > This used to get the empty list warning, but now it's mere
> > xhci_dbg(). Throwing out all queued TDs is not the common case and
> > it may easily be a bug. Indeed, I can readily name two cases when
> > it is a bug today:
> > 
> > 1. Force Stopped Event on a NOP or Link following the missed TD.
> > Then trb_in_td() doesn't match subsequent TD and the rest is
> > trashed.
> > 
> > Actually, this is a v6.11 regression since d56b0b2ab1429. Although
> > past behavior was bad and broken too, it was broken differently.
> > 
> > 2. Ring Underrun/Overrun if new TDs were queued before we handled
> > it. If ep_trb_dma is NULL, nothing ever matches and everything goes
> > out.
> > 
> > Arguably, these are rare events and I haven't observed them yet.
> > And one more problem that I don't think currently exists, but:
> > 
> > 3. If you ever find yourself doing it on an ordinary event (Success,
> > Transaction Error, Babble, etc.) then, seriously, WTF?
> > 
> > Bottom line, empty list is a very suspicious thing to see here. I
> > can only think of two legitimate cases:
> > 
> > 1. Ring X-run, only if nothing new was queued since it occurred.
> > 2. FSE outside transfer TDs, if no transfer TDs existed after it.  
> 
> I can change it from a debug to a warning. Then the edge case should
> be more visible.

Actually, I'm no longer sure. If that ever happens, the user will next
see "WARN list empty" or "ERROR TRB not part of current TD" and this
will show there is a problem, so dyndbg will be enabled to investigate.

I would still like to see more comp_codes printed in those messages,
because on isoc there are some "weird" codes like Missed Service or
Underrun, which have a very different meaning from the usual ones and
are a source of bugs. But I would have to think about which cases it
is useful for, particularly after this change.

Lastly, I'm not sure if this change is worthwile. Over the weekend,
I have produced a fairly simple but dramatic simplification of this
whole code. The 'empty list' branch that your patch deals with can
be completely removed, because it duplicates functionality of other
code. The skipping loop is reduced down to 8 lines of code, as it
should have always been in the first place.

I will try to clean those patches up and send them out today. They
received varying degree of testing (some are two weeks old) and have
not caused any regression so far. They are simple and revieable IMO.

Regards,
Michal