[U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi,

On 01-05-15 09:21, Hans de Goede wrote:
> Hi,
>
> On 01-05-15 06:11, Simon Glass wrote:
>> Hi Hans,
>>
>> On 30 April 2015 at 08:35, Hans de Goede <hdegoede at redhat.com> wrote:
>>> Currently we copy over a number of usb_device values stored in the on stack
>>> struct usb_device probed in usb_scan_device() to the final driver-model managed
>>> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
>>> then call usb_select_config() to fill in the rest.
>>>
>>> There are 2 problems with this approach:
>>>
>>> 1) It does not fill in enough fields before calling usb_select_config(),
>>> specifically it does not fill in ep0's maxpacketsize causing a div by zero
>>> exception in the ehci driver.
>>
>> Didn't you have another patch that fixes that?
>
> Yes, but as explained in the coverletter of that patch I believe that
> this version is better. It would seem we disagree on that though :)
>
>>> 2) It unnecessarily redoes a number of usb requests making usb probing slower,
>>> and potentially upsetting some devices.
>>
>> Does it actually upset anything?
>
> Not to my knowledge, but I'm afraid that with some devices it may.
>
>> The extra requests are in the second call to usb_select_config().
>
> Correct.
>
>> Do you think we could put the things we want to copy in a struct, and
>> copy just those?
>
> We would end up pretty much duplicating usb_device AFAICT, since we need
> the descriptors, the max packet sizes per endpoint parsed from them, etc.
>
> Anyways since you clearly dislike this patch I'll drop it for v2, replacing
> it with my original fix for the ep0 maxpacket not being set issue, assuming
> that doing so does not cause any regressions during my testing.

Ok, so my first test, which is hooking up a sunxi device to my dvi/usb kvm switch
fails immediately when I use the maxpacketsize fix instead of my fix to avoid
calling get_config twice. This is actually a pretty though test-case as the
kvm presents itself + the keyboard and mouse as a complex hub hierarchy with
both usb-2 and usb-1 hubs in there, which is what makes it a great test-case.

Now I could spend a couple of hours debugging this and maybe find a different
fix, but this really shows that there is a reason why all usb stacks do the
device probing / descriptor reading all in the exact same sequence, because
usb devices are cheap and there qa consists of plug it into $random windows
version running machine, does it work? Yes -> ship it.

So I really believe that my original fix for this is best. As for trying to
pass all the bits we need through platdata rather then passing the struct
usb_device itself. I can see value in that as part of a patch-set to get rid
of usb_device, iow as part of a larger patchset but until then it just feels
like make work to me.

So I'm going to stick with my original approach for v1 of this patch.

Regards,

Hans