linux-kernel - Re: [PATCH 0/3] usb: gadget: uvc: allocate requests based on frame interval length and buffersize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240522182348.4jyitatwtdqeo2gf@synopsys.com>
Date: Wed, 22 May 2024 18:23:58 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Michael Grzeschik <mgr@...gutronix.de>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Avichal Rakesh <arakesh@...gle.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Daniel Scally <dan.scally@...asonboard.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jayant Chowdhary <jchowdhary@...gle.com>,
        "etalvala@...gle.com" <etalvala@...gle.com>,
        Michael Riesch <michael.riesch@...fvision.net>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] usb: gadget: uvc: allocate requests based on frame
 interval length and buffersize

On Wed, May 22, 2024, Michael Grzeschik wrote:
> On Wed, May 22, 2024 at 05:17:02PM +0000, Thinh Nguyen wrote:
> > On Wed, May 22, 2024, Alan Stern wrote:
> > > On Wed, May 22, 2024 at 01:41:42AM +0000, Thinh Nguyen wrote:
> > > > On Wed, May 22, 2024, Michael Grzeschik wrote:
> > > > > On Fri, May 17, 2024 at 01:44:05AM +0000, Thinh Nguyen wrote:
> > > > > > For isoc endpoint IN, yes. If the host requests for isoc data IN while
> > > > > > no TRB is prepared, then the controller will automatically send 0-length
> > > > > > packet respond.
> > > > >
> > > > > Perfect! This will help a lot and will make active queueing of own
> > > > > zero-length requests run unnecessary.
> > > >
> > > > Yes, if we rely on the current start/stop isoc transfer scheme for UVC,
> > > > then this will work.
> > > 
> > > You shouldn't rely on this behavior.  Other device controllers might not
> > > behave this way; they might send no packet at all to the host (causing a
> > > USB protocol error) instead of sending a zero-length packet.
> > 
> > I agree. The dwc3 driver has this workaround to somewhat work with the
> > UVC. This behavior is not something the controller expected, and this
> > workaround should not be a common behavior for different function
> > driver/protocol. Since this behavior was added a long time ago, it will
> > remain the default behavior in dwc3 to avoid regression with UVC (at
> > least until the UVC is changed). However, it would be nice for UVC to
> > not rely on this.
> 
> With "this" you mean exactly the following commit, right?
> 
> (f5e46aa4 usb: dwc3: gadget: when the started list is empty stop the active xfer)

I believe that was the case. However, there were changes prior to that
that restarts the isoc on missed isoc, which shouldn't be the default
behavior.

> 
> When we start questioning this, then lets dig deeper here.
> 
> With the fast datarate of at least usb superspeed shouldn't they not all
> completely work asynchronous with their in flight trbs?

I'm not sure what you mean by asynchronous here.

> 
> In my understanding this validates that, with at least superspeed we are
> unlikely to react fast enough to maintain a steady isoc dataflow, since
> the driver above has to react to errors in the processing context.

The point is that we don't stop the isoc transfers unless there's a
change to the interface. The dwc3 driver should not need to do anything
else except reporting the missed request to the function driver. The
application/function driver should keep up with the continous data base
on the established periodic service interval. This is not bulk transfer.

> 
> This runs the above patch (f5e46aa4) a gadget independent solution
> which has nothing to do with uvc in particular IMHO.

If that's not the case, I stand corrected since I thought you sent
that patch in particular for UVC. Regardless, my other points still
stand.

> 
> How do other controllers and their drivers work?

If we stop and start the transfer because the function driver can't keep
up, then the data will go out in the wrong interval. The UVC host seems
to work base on the video frame rather than at the USB microframe
interval that it uses. It may not be the case for other protocols. If
isoc endpoint is used, then we expect timeliness matter.

BR,
Thinh

> 
> > Side note, when the dwc3 driver reschedules/starts isoc transfer again,
> > the first transfer will be scheduled go out at some future interval and
> > not the next immediate microframe. For UVC, it probably won't be a
> > problem since it doesn't seem to need data going out every interval.
> 
> It should not make a difference. [TM]
> 
> > > 
> > > On the other hand, it may not make any difference.  The host's UVC
> > > driver most likely won't care about the difference between no packet and
> > > a 0-length packet.  :-)
> > > 
> > > Alan Stern
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |