[U-Boot] [PATCH v2 18/26] dm: usb: Remove inactive children after a bus scan

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11-11-15 19:15, Simon Glass wrote:
> Hi Hans,
>
> On 11 November 2015 at 10:02, Hans de Goede <j.w.r.degoede@gmail.com> wrote:

<snip>

>> Ok, I've ran a whole battery of tests on your u-boot-dm/usb-working branch.
>
> Thanks!
>
>>
>> There are 2 issues:
>>
>> 1) You need to add these change to the commit introducing usb-keyb dm
>> support (or one of the related commits), otherwise usb-keyb support
>> breaks:
>>
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -513,6 +513,7 @@ config ARCH_SUNXI
>>          select DM
>>          select DM_GPIO
>>          select DM_ETH
>> +       select DM_KEYBOARD
>>          select DM_SERIAL
>>          select DM_USB
>>          select OF_CONTROL
>>
>> The breakage is that without this usb_scan_device() returns -96
>> (EPFNOSUPPORT) for usb keyboards.
>
> OK. I am a bit concerned this might affect other boards too. Still, if
> we get this series applied soon there is plenty of time for testing.
> I'll look a bit closer.

Ok.

>> 2) With that branch there still is the purely cosmetical issue,
>> that if one plugs a usb-stick + keyb into the same hub, then swaps
>> their place and do "usb reset" and then "usb tree" looks like this:
>>
>> USB device tree:
>>    1  Hub (480 Mb/s, 100mA)
>>    |   USB2.0 Hub
>>    |
>>    +-3  Human Interface (1.5 Mb/s, 100mA)
>>    |    SINO WEALTH USB Composite Device
>>    |
>>    +-2  Mass Storage (480 Mb/s, 100mA)
>>         USB Flash Disk 4C0E960F
>>
>> Notice the order of devices listed: 1 - 3 - 2, which is a bit weird,
>> as said this is purely cosmetical.
>>
>> Note you can easily fix this by only reverting the:
>>
>> "dm: usb: Rename usb_find_child to usb_find_emul_child"
>>
>> commit, and keep the other 2 you revert and dropping this patch ?
>>
>> As I already suggested before, this is both more KISS and as you
>> can see it solves some actually (admittedly very minor) issues.
>>
>> I'm not really buying your arguments for your more complex solution,
>> as shown above re-using the devices actually causes issues.
>>
>> Your other argument of wanting to attach device-tree properties
>> I also do not find a strong argument, for one there has never been
>> a need to do so sofar, and if we ever need this we need a way
>> to specify which usb-device the properties belong to based on
>> the topology under the host / root-hub anyway, and match things
>> up when first scanning the bus. And if we can match things up
>> on the first scan we can also match them up on subsequent scans
>> and attach the same of-node again.
>>
>> Anyways I'm fine with doing things your way, but I still have
>> a preference for the more KISS and IMHO robust solution of
>> simple unbinding all devices on usb-stop.
>
> I wonder if it really is more complex. My preferred algorithm is to
> remove any devices that have not been probed after a scan. Yours is to
> remove and unbind any USB devices before a scan. What is the
> difference?

If you unbind everything before (re)scanning you always start with
a clean slate, making things more reproducible and simpler.

As for it being more complex, your approach needs a whole new
helper function and needs to walk the list of devices twice, where
as mine is just a single extra function call + error checking.

As an added bonus my approach allows adding an ifdef to usb_find_child
turning it into a nop like this

#if defined CONFIG_SANDBOX || !defined CONFIG_DM_DEVICE_REMOVE
    ...
    ... complex code to find child
    ...
#else
    return -ENODEV;
#endif

So besides not needing the extra helper function your solution
needs, my version also removes a bunch of extra code (from the
resulting binary) in almost all cases.

So your solution is definitely more complex, needlessly so IMHO.

> I much prefer not unbinding and rebinding devices. To my mind, devices
> should be bound once if possible, and most of the time it is possible.

Well in case of a re-scan we are discovering all devices from scratch,
not just looking for changed devices, simply starting with a clean
device list reflects this.

> This is different from the Linux approach of combining 'bind' and
> 'probe'. At present sandbox cannot support 'usb reset'. I would like
> sandbox to provide full support so that we can use tests to verify
> functionality rather than manual testing.
>
> Re the ordering above, I suppose I could fix that command easily
> enough. This is not a common problem though. Does it really matter?

Nope, as said this is a cosmetic issue, and for the record
I'm fine with your approach of always re-using existing devices
on a re-scan. I prefer the start with a clean slate approach but
either is fine with me.

>> ###
>>
>> Talking about usb-stop, there is still one (BIG!) problem after
>> this patch set when building usb-support with DM_DEVICE_REMOVE
>> not set. This means that the controllers dma engines will not
>> be stopped when booting the actual OS and they will still be
>> accessing parts of the main memory while the actual OS is
>> booting, which is BAD. So I suggest adding something like
>> this to all host drivers which use dma in this way:
>>
>> #if defined CONFIG_DM_USB && !defined CONFIG_DM_DEVICE_REMOVE
>> #error The EHCI driver cannot be used without CONFIG_DM_DEVICE_REMOVE otherwise DMA stays active while booting the OS
>> #endif
>
> Sounds good. I am not suggesting that we encourage people to build USB
> with out DM_DEVICE_REMOVE. I just want to make sure that we don't
> create unnecessary dependencies such that DM_DEVICE_REMOVE becomes
> impossible to use.
>
> We have a note in the Kconfig for that, but I agree we need to make it
> more explicit. With the #if approach it makes the pitfall much more
> obvious.

Ack, so we need to have patches for this for at least ohci, ehci, xhci
and I guess also uhci ? Who is going to write these patches ?

Regards,

Hans