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

[Alan Stern]

On Sat, Jun 20, 2026 at 12:38:00PM +0530, Nikhil Solanke wrote:
> > > @@ -946,7 +949,7 @@ int usb_get_configuration(struct usb_device *dev)
> > >               /* We grab just the first descriptor so we know how long
> > >                * the whole configuration is */
> >
> > This comment is now out of date.  It should be rewritten to explain why
> > the quirk does and why.
> 
> I will explain the quirk above where the variable is defined and reference
> it here. A reader would probably question about the quirk there.

Okay, so long as you don't keep a comment that is now incorrect.

> > > @@ -957,26 +960,39 @@ 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);
> > > +                         "(expected %zu, got %i)\n", cfgno,
> >
> > Likewise, "expected" here is wrong.  It should be "asked for" or
> > something like that.
> 
> For this branch tho, we are expecting atleast 9 bytes. if we don't get
> those we simply bail out. expected is the right word here. But this was the
> originial implementation. With the introduction of the quirk, the wording
> of it does fall apart. Instead of adding more branches just to rename a log
> message, let's just keep it as "expected"? In a similar branch later on,
> when we ask for the bigbuffer, it does make sense to use "asked for" like
> you suggested. I will make the change there.

Again, I don't care too much about the exact wording; I just want to 
make sure that the log message isn't actually wrong.  Saying "expected 
255" would definitely be wrong, but saying "expected at least 9" would 
be okay.

> > > +                         usb_dt_config_size, result);
> > >                       result = -EINVAL;
> > >                       goto err;
> > >               }
> > > -             length = max_t(int, le16_to_cpu(desc->wTotalLength),
> > > -                 USB_DT_CONFIG_SIZE);
> > > +             /* If the device does returns the full length configuration
> > > +              * descriptor, skip the second read. Fallback to default
> > > +              * behavior otherwise.
> > > +              */
> >
> > New multiline comments (or ones that are rewritten) should use the same
> > format as the rest of the USB stack:
> >
> >         /*
> >          * Blah, blah, blah
> >          * Blah, blah, blah
> >          */
> >
> 
> Alright. About the comments formatting, all other comments were not
> following a consistent format in the same file. so I didn't bother to fix
> such small style changes. But I will still fix them in the way you (and
> kernel code style guidelines) say.

Yeah, don't bother to fix up comments that the patch doesn't otherwise 
touch -- that would be mere pointless churn and would make reviewing the 
patch more difficult for no good reason.  But comments that you _do_ 
alter are fair game for reformatting.

> > Whether the quirk flag is set doesn't matter.  All you care about is
> > whether the information received earlier contains the entire descriptor
> > set.  The first and third tests here should be removed.
> >
> > There is some question about what to do if wTotalLength < result.  My
> > advice is to use the smaller value in this case, but not smaller than
> > USB_DT_CONFIG_SIZE.
> 
> In the case where wTotalLength < result, wouldn't it be better to consider
> the result value as the truth? Or are there scenarios where the device or
> the buffer will contain gibberish just to fill it, which is why you
> suggested a smaller value.

Sort of.  Software that parses the descriptors later on will ignore the 
extra stuff in any case, limiting itself to just the first wTotalLength 
bytes.  So there's not really any need to keep the excess.

>  I did understand the part that it should be
> atleast USB_DT_CONFIG_SIZE because its the header, but isn't that part
> already handled above with result < 4? It does ensure all the critical
> fields are actually present.

The critical fields are not just the first 4 bytes, but the first 9 
bytes.  If you want, you can change that test against 4 above, making it 
against USB_DT_CONFIG_SIZE.  The reason the existing code only tests 
against 4 is because it knows that it will issue another request for the 
full length, but that won't always be true after your changes.

>  And with just the second test, the code will
> naturally jump to the else branch for any cases like you mentioned. I will
> change the max_t back to use USB_DT_CONFIG_SIZE and everything seems to be
> covered now? Also looking at the 3rd test now, it is actually redundant.
> Thanks for pointing that out.
> 
> 
> > >
> > > -             /* Now that we know the length, get the whole thing */
> > > -             bigbuffer = kmalloc(length, GFP_KERNEL);
> > > -             if (!bigbuffer) {
> > > -                     result = -ENOMEM;
> > > -                     goto err;
> > > -             }
> > > +                     bigbuffer = (unsigned char *) desc;
> > > +                     desc = NULL;
> > > +                     length = result;
> >
> > Don't keep the entire 255-byte buffer.  Use krealloc() to shrink the
> > buffer down to the right size.
> 
> I did intially though of using krealloc(), but when looked at existing
> implementation, bigbuffer is alloced with wTotalLength while ensuring its
> atleast USB_DT_CONFIG_SIZE (9) bytes. Then when we receive the result, the
> bigbuffer isn't realloced as per the size we received. So I tried to mirror
> this exising behavior in fear that I might mess up something else while
> trying to be smart. (Although yea, it is waste of memory).

You're right that the existing code could have reallocated the buffer if 
it received less than it asked for.  I believe it doesn't bother to do 
this because that possibility is considered to be very unlikely.

In fact, you could arrange to do the check and reallocation after the 
two code paths merge back together.  That would be the best solution.

> > Again, I don't like this name.  It's not a quirk in the size of the
> > configuration descriptor type, which is what "USB_DT_CONFIG_SIZE" stands
> > for; it's a quirk in the way the kernel asks for config descriptors.
> > (Or in what size request the device will accept, if you prefer.)
> >
> > And the 255 value doesn't belong in this header file anyway.  It should
> > be defined in config.c since that's the only place it gets used.
> 
> So what about USB_CONFIG_REQ_SIZE? it's what you had suggested before in
> earlier conversations (without the QUIRK word in between naming our magic
> number 255).

Yes, that seems like a better choice for the name.  Or maybe 
USB_CONFIG_BIG_REQ_SIZE or USB_CONFIG_WINDOWS_REQ_SIZE, to indicate this 
isn't the value we normally use but copies the behavior of Windows.

Alan Stern