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

[Alan Stern]

On Sun, Jun 28, 2026 at 07:02:01PM +0200, Michal Pecio wrote:
> On Sun, 28 Jun 2026 11:48:36 -0400, Alan Stern wrote:
> > > How about "keep unrelated changes out of a stable patch", i.e. always
> > > do the delay (if any) after the first request, regardless of size?  
> > 
> > This is not an unrelated change.  Rather, it's deciding on how to behave 
> > in an entirely new control pathway -- the one where the 255-byte quirk 
> > flag is set.  The old pathway is completely unaffected.
> > 
> > I suspect no devices will have both this quirk flag and the DELAY_INIT 
> > flag set, which means the location of any delays in the new pathway 
> > won't matter at all since they will never be used.
> 
> If no devices will have both quirks then new delay added before the
> first configuration request will never execute.

Correct.

> If such devices will exist, then it probably won't matter whether the
> delay comes after or before the first request. Purpose isn't known,
> but it appears to be rate limiting configuration descriptor requests
> or delaying other requests after this function returns.

In fact, the commit that talks about the Logitech webcams does describe 
their buggy behavior to some extent.  It says that they seem to reply 
with stale video data instead of the real config information, and from 
there it's a short guess that adding a delay gives time for the video 
pipeline to drain or time out.

In addition, the fact that the delay is needed after the first request 
but before the second suggests that the data corruption only affects 
transfers longer than 9 bytes -- which the new first request would be.  
Therefore it would be appropriate to have the delay before the new first 
request.  Whether another delay would be needed before the second 
request (if there is one) is unknown.

> Either way, no known need exists to add another delay before the first
> request or alter the existing delay (or its conditions) in any way.

See the reasoning above.

> In general, I always object to code which serves no purpose because
> such code is easy to add but very hard to remove when it gets in the
> way. There are no known users, no test cases, only paranoia.
> 
> So I would keep the delay code completely unchaged.

At this point it's entirely theoretical anyway, since the devices that 
would get the new 255-byte quirk flag don't also have the DELAY_INIT 
quirk.

> And skip other random changes like error string nitpicking. Reliable
> and up to date information about how many bytes are requested,
> "expected" (what does it even mean, to somebody reading dmesg?),
> received or verified to exist can be gained from source and usbmon.

Good point.  But I dislike messages that actively produce wrong 
information.  Nikhil could get rid of the parts of the log messages you 
don't like, but he shouldn't leave them as they are.  He could even do 
that in a second patch, separate from this one.

> A stable patch is supposedly supposed to be 100 lines with context ;)

Nonsense.  An excellent stable patch is 7 lines, including context.  :-)

> I really think it could (and should) be a simple patch.
> 
> This is what I wrote a few weeks ago. It's an unconditional change
> for all devices, but it would be easy to turn it into a quirk.
> 
> 
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -938,15 +938,14 @@ int usb_get_configuration(struct usb_device *dev)
>  	if (!dev->rawdescriptors)
>  		return -ENOMEM;
>  
> -	desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
> +	desc = kmalloc(255, GFP_KERNEL);
>  	if (!desc)
>  		return -ENOMEM;
>  
>  	for (cfgno = 0; cfgno < ncfg; cfgno++) {
> -		/* We grab just the first descriptor so we know how long
> -		 * the whole configuration is */
> +		/* Try 255 bytes first because that's what Windows does */
>  		result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
> -		    desc, USB_DT_CONFIG_SIZE);
> +		    desc, 255);
>  		if (result < 0) {
>  			dev_err(ddev, "unable to read config index %d "
>  			    "descriptor/%s: %d\n", cfgno, "start", result);
> @@ -975,8 +974,12 @@ int usb_get_configuration(struct usb_device *dev)
>  		if (dev->quirks & USB_QUIRK_DELAY_INIT)
>  			msleep(200);
>  
> -		result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
> -		    bigbuffer, length);
> +		/* Don't bother if we already have it all */
> +		if (length <= result)
> +			memcpy(bigbuffer, desc, length);
> +		else
> +			result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
> +					bigbuffer, length);
>  		if (result < 0) {
>  			dev_err(ddev, "unable to read config index %d "
>  			    "descriptor/%s\n", cfgno, "all");

Making this depend on a quirk flag instead of being unconditional would 
yield a patch very similar to what Nikhil has already posted.

Alan Stern