Re: [PATCH v3 6/6] usb: gadget: uvc: add configfs option for sg support

[Alan Stern <stern@rowland.harvard.edu>]

On Tue, Oct 18, 2022 at 10:14:54AM -0500, Dan Vacura wrote:
> Hi Alan,
> 
> On Tue, Oct 18, 2022 at 10:32:33AM -0400, Alan Stern wrote:
> > On Tue, Oct 18, 2022 at 02:27:13PM +0100, Dan Scally wrote:
> > > Hi Dan

> > > > --- a/Documentation/usb/gadget-testing.rst
> > > > +++ b/Documentation/usb/gadget-testing.rst
> > > > @@ -796,6 +796,8 @@ The uvc function provides these attributes in its function directory:
> > > >   	function_name       name of the interface
> > > >   	req_int_skip_div    divisor of total requests to aid in calculating
> > > >   			    interrupt frequency, 0 indicates all interrupt
> > > > +	sg_supported        allow for scatter gather to be used if the UDC
> > > > +			    hw supports it
> > 
> > Why is a configuration option needed for this?  Why not always use SG 
> > when the UDC supports it?  Or at least, make the decision automatically 
> > (say, based on the amount of data to be transferred) with no need for 
> > any user input?
> 
> Patches for a fix and to select to use SG depending on amount of data
> are already submitted and under review. I agree, ideally we don't need
> this patch, but there have been several regressions uncovered with
> enabling this support and it takes time to root cause these issues.

Please put this information into the patch description, and maybe also 
into the documentation file.  For your readers' and reviewers' sake it's 
important -- probably _more_ important -- to explain why you're making a 
change than what that change is.

Alan Stern

> In my specific environment, Android GKI 2.0, changes need to get
> upstreamed first here before they're pulled into Android device
> software. Having this logic in place gives us the ability to turn off
> this functionality without going through this process. A revert was also
> considered until all the bugs are resolved, but the code is quite
> entrenched now to take out, plus others seem to benefit from it being
> enabled. Thus the configurability.