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

[Nikhil Solanke]

> And here it's USB_QUIRK_WINDOWS_CONFIG_REQ_SIZE, but in the commit
> message it was USB_QUIRK_CONFIG_SIZE.

Yes, I have fixed it already.

> Honestly, I would suggest a third option: something with "255" instead
> of "Windows", because not everybody knows how windows queries
> descriptors, but everybody knows what 255 is.

Well naming ain't my strong suit (if I name something, it will
definitely get called out). Do you have a specific name in mind? if
your concern lies more in people not knowing what Windows does, should
i just make it apparent in a comment where it is set?

> That's a lot of capital letters, USBCONFIG_WINDOWS_REQ_SIZE never
> appears outside this function and personally I would just spell it out
> as 255 here with appropriate comment.

I wanted to avoid the plausible "avoid magic numbers" criticism. If
everybody else is fine with bare 255, I will just do that then.


> > + bigbuffer = (unsigned char *) desc;
> > + desc = NULL;
>
> What happens in the next iteration of the loop?

Disaster to say the least. A memcpy in bigbuffer and memset(0,..) on
desc would be the correct approach here, wouldn't it?

On that note, I discovered that usb_get_descriptor just blindly trusts
the caller with the allocation of buf, it never checks if buf is null
or not. There is only a check for the size. and then there is a
memset(buf, 0, size). This results in a segfault if buf is NULL and
size > 0. Perhaps it's time for a new patch to fix this?

> I wonder if it wouldn't make sense to split announce_device() so that
> the first line is printed as soon as usb_new_device() starts, before
> enumeration is attempted and possibly fails.

Do you want something like announce_device_ids() where it would only
print out the first "New Usb device found..." line? and then
announce_device_strings()?

Perhaps even another macro announce_device that does both? But its
only used in one place, so I doubt it would be necessary.

> That would be a separate patch, of course.

If i have to make this change, should this be a completely separate
patch or is it fine to include it in this patchseries?

Thanks,
Nikhil Solanke