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

[Nikhil Solanke]

> You don't need Suggested-by here.  It's redundant; we always assume that
> people are responsible for authorship of the patches they write and
> submit, unless they say otherwise.
>
> > Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Suggested-by: Michal Pecio <michal.pecio@xxxxxxxxx>
> > Closes: https://lore.kernel.org/linux-usb/CAFgddh+JWdT4LLwMc5qjM8q_pBu-fRo2qADR5ovAKoGHWMQrRw@xxxxxxxxxxxxxx/
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Nikhil Solanke <nikhilsolanke5@xxxxxxxxx>
> > ---
> >  drivers/usb/core/config.c  | 56 +++++++++++++++++++++++++++-----------
> >  drivers/usb/core/quirks.c  |  3 ++
> >  include/linux/usb/quirks.h |  4 +++
> >  3 files changed, 47 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> > index 45e20c6d76c0..623425cef085 100644
> > --- a/drivers/usb/core/config.c
> > +++ b/drivers/usb/core/config.c
> > @@ -912,6 +912,8 @@ int usb_get_configuration(struct usb_device *dev)
> >       unsigned char *bigbuffer;
> >       struct usb_config_descriptor *desc;
> >       int result;
> > +     size_t usb_dt_config_size = (dev->quirks & USB_QUIRK_CONFIG_SIZE)
> > +             ? USB_DT_CONFIG_SIZE_QUIRK : USB_DT_CONFIG_SIZE;
>
> I wouldn't call the variable usb_dt_config_size.  It isn't always the
> size of a USB configuration descriptor; it isn't even always the size
> you expect for the response.  Rather, it is the size you intend to ask
> for.
>
> >
> >       if (ncfg > USB_MAXCONFIG) {
> >               dev_notice(ddev, "too many configurations: %d, "
> > @@ -938,7 +940,8 @@ int usb_get_configuration(struct usb_device *dev)
> >       if (!dev->rawdescriptors)
> >               return -ENOMEM;
> >
> > -     desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
> > +     desc = kmalloc(usb_dt_config_size, GFP_KERNEL);
> > +
> >       if (!desc)
> >               return -ENOMEM;
> >
> > @@ -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.

>
> >               result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
> > -                 desc, USB_DT_CONFIG_SIZE);
> > +                 desc, usb_dt_config_size);
> >               if (result < 0) {
> >                       dev_err(ddev, "unable to read config index %d "
> >                           "descriptor/%s: %d\n", cfgno, "start", result);
> > @@ -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.

>
> > +                         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.

> 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. 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. 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).

> The "due to above errors" part isn't needed, since the errors will be
> obvious in the kernel log.  In fact, it probably would be better not to
> put this information here at all but instead modify the error message in
> usb_enumerate_device() (the caller).

Alright

> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index 87810eff974e..92219684a604 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -142,6 +142,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp)
> >                               break;
> >                       case 'q':
> >                               flags |= USB_QUIRK_FORCE_ONE_CONFIG;
> > +                             break;
> > +                     case 'r':
> > +                             flags |= USB_QUIRK_CONFIG_SIZE;
>
> For good style, there should be a "break" statement here.

Yea I do agree. Then again I tried to keep my changes in respect to what
was like originally. I will add the missing break then.

> Also, you need to document the new flag under the usbcore.quirks entry
> in Documentation/admin-guide/kernel-parameters.txt.

Oh sorry, totally missed it. Working on it.

> 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).

Nikhil Solanke