Re: [PATCH v2] usbcore: Add quirk for 255-bytes initial config read

[Alan Stern]

On Wed, Jun 24, 2026 at 02:44:07AM +0530, Nikhil Solanke wrote:
> > Moving this delay up here changes the behavior when the quirk flag isn't
> > set.  While it agrees with the intention of the USB_QUIRK_DELAY_INIT
> > flag, such a change should be mentioned in the patch description.
> 
> How should I mention it then? Nothing comes to mind besides the
> obvious: "Also move the USB_QUIRK_DELAY_INIT sleep to before the
> initial descriptor read, so the delay applies consistently regardless
> of whether USB_QUIRK_CONFIG_SIZE is set.". Or should i revert it back
> to original position?

Actually, the best approach here would be to put this single change into 
a separate patch that comes before the current one.  That removes issues 
of making more than one functional change in one patch and improves 
bisectability.

But to answer your question: In general, a patch's description should 
explain the reasons for the changes that the patch makes.  Especially 
when a particular change doesn't appear, at first glance, to be related 
to the patch's primary purpose.  (On the other hand, it doesn't need to 
explain in detail what the patch does; we can see that for ourselves 
just by reading the patch's contents.)

> > > +             /*
> > > +              * Grab just the first descriptor so we know how long the whole
> > > +              * configuration is. In case of quirky firmware, try to grab the
> > > +              * whole thing in one go by asking for a 255-bytes sized buffer
> > > +              * mirroring Windows behavior.
> > > +              */
> >
> > This needs to be rewritten, as it is self-contradictory.  When the quirk
> > flag is set we issue a 255-byte request to mimic the Windows behavior,
> > and only when the flag isn't set do we grab just the first descriptor.
> 
> I am sorry I didn't understand how it is self contradictory. The
> comment does say, "in case of quirky firmware..."? Am i missing
> something?

Literally, what the comment says is: Grab just the first descriptor, 
and if the quirk flag is set, get all the descriptors.  That's a 
contradiction -- you can get just the first, or you can get all of 
them, but you can't do both at the same time!

> > >               result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
> > > -                 desc, USB_DT_CONFIG_SIZE);
> > > +                                             desc, usb_config_req_size);
> >
> > Don't make extraneous changes to the existing indentation (or whitespace
> > in general), here and below.
> 
> Well the linux coding style guidelines mention that those descendants
> should preferably be aligned with the function open parenthesis. Since
> i did "touch" that line/part of code I though might as well indent it
> a bit accordingly. Should i revert the indent then (in this and the
> other place)?

The style used in this file is to indent continuation lines by 4 spaces, 
because some of the continued statements are extremely long.  If you 
want to align new continuation lines with an open paren, you can -- but 
you didn't even do that in the example above; you aligned it with the 
space following the first comma.

And while you did change some nearby code, you did not change the code 
in this line.  So reformatting it is not justified.

> > >                       if (result != -EPIPE)
> > >                               goto err;
> > >                       dev_notice(ddev, "chopping to %d config(s)\n", cfgno);
> > > @@ -957,13 +976,25 @@ int usb_get_configuration(struct usb_device *dev)
> > >                       break;
> > >               } else if (result < 4) {
> > >                       dev_err(ddev, "config index %d descriptor too short "
> > > -                         "(expected %i, got %i)\n", cfgno,
> > > -                         USB_DT_CONFIG_SIZE, result);
> > > +                             "(asked for %zu, got %i, expected at least %i)\n",
> > > +                             cfgno, usb_config_req_size, result, 4);
> > >                       result = -EINVAL;
> > >                       goto err;
> > >               }
> > > +
> > >               length = max_t(int, le16_to_cpu(desc->wTotalLength),
> > > -                 USB_DT_CONFIG_SIZE);
> > > +                             USB_DT_CONFIG_SIZE);
> >
> > This is another example of a change that has nothing to do with the
> > purpose of the patch.
> 
> Isn't that what you told me to change? So the logs are accurate? I
> made that change because you suggested it. :')

My comment referred to the two lines directly above it, and I did not 
suggest leaving the code exactly the same except for indenting it 
farther.  Or inserting an extra blank line just before the assignment to 
length.

Alan Stern