[BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Frank Mori Hess]

When there is interrupt latency, the dwc2 driver can fail to issue its
"complete split" transactions during the microframes they were
scheduled for.  When this happens, the result from the transaction is
silently lost without producing any error.

The reason the complete splits need to happen on time, is the
"transaction translator" on the hub discards responses from its
complete split pipeline after keeping them only 1 microframe, as
specified by the usb standard. So if the complete split transactions
are late, there is no way to recover.  An error must be returned so
higher level code can deal with the lost transaction (in my case, the
USB HID driver needs to re-sync with my keyboard after losing an
interrupt urb).

Instead, the current code silently continues by issuing a new "start
split".  This next split transaction may succeed, but it is not enough
since the devices thinks it has already successfully sent its
interrupt transaction (it has in fact succeeded in sending the
transaction to the hub, buy the hub then discarded it).

My hack to fix this was to change dwc2_hc_nyet_intr() so it sets
qtd->error_count=3 when past_end is true.

Also I should mention I am using an old kernel 5.4.13, but based on
inspection of the mainline master branch, this problem still exists in
current kernels.

-- 
Frank

Re: [BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Greg KH]

On Fri, Aug 12, 2022 at 09:37:29PM -0400, Frank Mori Hess wrote:
> Also I should mention I am using an old kernel 5.4.13, but based on
> inspection of the mainline master branch, this problem still exists in
> current kernels.

Loads of things have changed since 2019, please try the 5.19 kernel to
see if it still has the same issue.

thanks,

greg k-h

Re: [BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Frank Mori Hess]

I'm using a linux-socfpga altera (intel) patched kernel, and they
don't have a 5.19 kernel available.  In any case, nothing outside of
the dwc2 driver could possibly fix this problem.  Arguably, it is a
bug (or at least major deficiency) in the synopsis hardware, in that
it does not support reliably scheduling the complete splits.  Or you
could view it as a deficiency in the usb standard, in that it does not
consider error handling in the case where complete splits are late.

On Sat, Aug 13, 2022 at 12:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Aug 12, 2022 at 09:37:29PM -0400, Frank Mori Hess wrote:
> > Also I should mention I am using an old kernel 5.4.13, but based on
> > inspection of the mainline master branch, this problem still exists in
> > current kernels.
>
> Loads of things have changed since 2019, please try the 5.19 kernel to
> see if it still has the same issue.
>
> thanks,
>
> greg k-h



-- 
Frank

Re: [BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sat, Aug 13, 2022 at 07:52:05AM -0700, Frank Mori Hess wrote:
> I'm using a linux-socfpga altera (intel) patched kernel, and they
> don't have a 5.19 kernel available.

Then please work with your vendor as only they can provide the needed
support that you are already paying for.  Take advantage of that,
nothing we can do about odd vendor kernels, sorry.

good luck!

greg k-h

Re: [BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Frank Mori Hess]

On Sat, Aug 13, 2022 at 8:08 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> Then please work with your vendor as only they can provide the needed
> support that you are already paying for.  Take advantage of that,
> nothing we can do about odd vendor kernels, sorry.

I'm not looking for support, I've already fixed the bug for myself.
I'm just trying to provide some useful information to the dwc2
maintainer in case they care about fixing the bug.  The bug is
obviously still present in the current mainline kernel.  From the
dwc2_hc_nyet_intr function, they #ifdef out incrementing the
error_count (as I tried to explain earlier, even incrementing the
error_count is not enough, there needs to be a hard error):

#if 0
/*
* Todo: Fix system performance so this can
* be treated as an error. Right now complete
* splits cannot be scheduled precisely enough
* due to other system activity, so this error
* occurs regularly in Slave mode.
*/
qtd->error_count++;
#endif



-- 
Frank

Re: [BUG] usb: dwc2: split interrupt in transactions silently dropped due to driver latency

[Greg KH]

On Sun, Aug 14, 2022 at 05:33:09AM -0700, Frank Mori Hess wrote:
> On Sat, Aug 13, 2022 at 8:08 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Then please work with your vendor as only they can provide the needed
> > support that you are already paying for.  Take advantage of that,
> > nothing we can do about odd vendor kernels, sorry.
> 
> I'm not looking for support, I've already fixed the bug for myself.
> I'm just trying to provide some useful information to the dwc2
> maintainer in case they care about fixing the bug.  The bug is
> obviously still present in the current mainline kernel.  From the
> dwc2_hc_nyet_intr function, they #ifdef out incrementing the
> error_count (as I tried to explain earlier, even incrementing the
> error_count is not enough, there needs to be a hard error):
> 
> #if 0
> /*
> * Todo: Fix system performance so this can
> * be treated as an error. Right now complete
> * splits cannot be scheduled precisely enough
> * due to other system activity, so this error
> * occurs regularly in Slave mode.
> */
> qtd->error_count++;
> #endif

If you have a fix, please, send it as a normal patch submission so we
can review and accept it like normal.  No need to do anything else here
out-of-the-ordinary.

thanks,

greg k-h