[U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop

[Hans de Goede]

Hi,

On 06/30/2015 04:58 PM, Simon Glass wrote:
> Hi Hans,
>
> On 30 June 2015 at 06:54, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>> On 29-06-15 05:45, Simon Glass wrote:
>>>
>>> Hi Hans,
>>>
>>> On 17 June 2015 at 13:33, Hans de Goede <hdegoede at redhat.com> wrote:
>>>>
>>>> On an usb stop instead of leaving orphan usb devices behind simply remove
>>>
>>>
>>> On a usb_stop()
>>>
>>> or
>>>
>>> On a 'usb stop' command  ?
>>
>>
>> My intention was for both, since I was under the assumption that "usb stop"
>> on the cmdline, was the only caller of usb_stop(), but a quick grep to the
>> sources show that I'm wrong ...
>>
>>>> them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build
>>>> usb_stop() when that is set.
>>>
>>>
>>> This seems a little unfortunate. I can see the reasoning, but do you
>>> think this is necessary? I suspect people chasing code size may remove
>>> that option and still want to use USB properly.
>>
>>
>> This was mostly a result of my thinking that usb_stop() is only used
>> on "usb stop" at the cmdline, which I know realize is wrong.
>>
>> However my quick grep has learned that we do really need
>> CONFIG_DM_DEVICE_REMOVE
>> to properly implement usb_stop():
>>
>>  From common/bootm.c :
>>
>> #if defined(CONFIG_CMD_USB)
>>          /*
>>           * turn off USB to prevent the host controller from writing to the
>>           * SDRAM while Linux is booting. This could happen (at least for
>> OHCI
>>           * controller), because the HCCA (Host Controller Communication
>> Area)
>>           * lies within the SDRAM and the host controller writes continously
>> to
>>           * this area (as busmaster!). The HccaFrameNumber is for example
>>           * updated every 1 ms within the HCCA structure in SDRAM! For more
>>           * details see the OpenHCI specification.
>>           */
>>          usb_stop();
>> #endif
>>
>> And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove
>> callback and thus do not properly stop the usb controller.
>>
>> So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists
>> before this patch. If you want I can split out the adding of the #ifdef
>> in a separate commit, spelling out why usb_stop() MUST have
>> CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all
>> to
>> Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE?
>>
>
> I don't think that is necessary, it feels a bit too inflexible. But
> perhaps you could add a comment to the Kconfig help for
> CONFIG_DM_DEVICE_REMOVE?

Ok will do.

> It is remove() that is needed, not unbind(). Actually I think it is
> quite unfortunate to make usb_stop() call unbind. It is a waste of
> time to do this just before booting the kernel - the current design
> leaves all devices bound (but I hope we can remove() them at some
> point).
>
> Instead, I wonder if we can remove the children when we probe the bus?

That should work, but I do not really see any advantage in that,
removing the children is not that expensive and it feels like a
kludge.

> Also, what happens to children that are in the device tree - i.e.
> static USB devices like WiFi? The device tree might have parameters
> for them. Still, that might not matter as I'm not sure that case is
> handled correctly today.

AFAIK there is no such thing as usb devices in devicetree, which
makes sense as usb is a fully discoverable bus.



>
>>
>>
>>>
>>>>
>>>> The result of this commit is best seen in the output of "dm tree" after
>>>> plugging out an usb hub with 2 devices plugges in and plugging in a keyb.
>>>> instead, before this commit the output would be:
>>>>
>>>>    usb         [ + ]    `-- sunxi-musb
>>>>    usb_hub     [   ]        |-- usb_hub
>>>>    usb_mass_st [   ]        |   |-- usb_mass_storage
>>>>    usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
>>>>    usb_dev_gen [ + ]        `-- generic_bus_0_dev_1
>>>>
>>>> Notice the non active usb_hub child and its 2 non active children. The
>>>> first child being non-active as in this example also causes
>>>> usb_get_dev_index
>>>> to return NULL when probing the first child, which results in the usb kbd
>>>> code not binding to the keyboard.
>>>
>>>
>>> Although I suspect that could be fixed.
>>
>>
>> Right, but just removing the children is a much cleaner solution, and also
>> makes the output of "dm tree" properly reflect reality.
>
> True, although you also have 'usb tree' for that. Another option would
> be to mark devices that were found and remove the others after the
> scan.

That seems like needless complexity. I believe that simply removing + unbinding
the children on usb_stop is the right thing to do, and it also is the KISS
solution.

Regards,

Hans