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

[Nikhil Solanke]

> 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?

> > +
> > +             /*
> > +              * 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?

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

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

> > +
> > +             /*
> > +              * If the device returns the full length configuration
> > +              * descriptor, skip the second read. Otherwise, send a second
>
> Strictly speaking, the configuration descriptor is only 9 bytes long.
> What you mean here is the entire configuration descriptor set.

Alright i'll reword it.

> > +              * request asking for the full length.
> > +              */
> > +             if (result >= le16_to_cpu(desc->wTotalLength)) {
>
> Shouldn't this be: result >= length?  No point in repeating the
> le16_to_cpu calculation.

Yess. initially the length assignment was happening afterwards in my
patch. then i decided to move it before the "if" statement since the
outcome of length was going to be similar in any case (within if and
after if). but then i forgot to modify the if too. Will fix it.

> Like above, this string should all be on one line.

Will fix all the strings as well

Nikhil Solanke