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

[Simon Glass]

Hi Hans,

On 9 November 2015 at 12:25, Simon Glass <sjg at chromium.org> wrote:
> Hi Hans,
>
> On 9 November 2015 at 00:22, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>> On 09-11-15 07:48, Simon Glass wrote:
>>>
>>> Each scan of the USB bus may return different results. Existing
>>> driver-model
>>> devices are reused when found, but if a device no longer exists it will
>>> stay
>>> around, de-activated, but bound.
>>>
>>> Detect these devices and remove them after the scan completes.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>
>>
>> I wonder how this is better then my original:
>> "dm: usb: Use device_unbind_children to clean up usb devs on stop"
>>
>> Patch, the end result of both patches is the same and both are
>> a NOP when DM_DEVICE_REMOVE is not set. Where as my code seems
>> to be a much more KISS approach to the problem (my approach is
>> just 3 lines vs 23 lines for yours).
>>
>> I know we will need usb_find_child in the DM_DEVICE_REMOVE not
>> set case, but why not only revert the:
>>
>> "dm: usb: Rename usb_find_child to usb_find_emul_child"
>>
>> commit, keep the other 2 you revert and drop this patch ?
>>
>> This drops 3 patches from your patch-set and the end result is
>> more clean IMHO.
>
> I would like to avoid binding/unbinding things when nothing changes if
> possible. Also I'd like to support attaching device tree
> nodes/properties to USB devices as necessary, as we do with PCI, and
> removing things breaks that.
>
> I still have to figure out one more test case, so I'll do that before
> commenting further.

I've added the test case and pushed a new tree. However it turns out
that this doesn't behave differently.

So please can you go ahead and run your original (manual) test case?
I'd like to make sure that my automated tests are correct and catch
the bug you reported and fixed.

>
>>
>>
>>> Changes in v2: None
>>>
>>>   drivers/usb/host/usb-uclass.c | 23 +++++++++++++++++++++++
>>>   1 file changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>>> index 4aa92f8..50538e0 100644
>>> --- a/drivers/usb/host/usb-uclass.c
>>> +++ b/drivers/usb/host/usb-uclass.c
>>> @@ -202,6 +202,20 @@ static void usb_scan_bus(struct udevice *bus, bool
>>> recurse)
>>>                 printf("%d USB Device(s) found\n", priv->next_addr);
>>>   }
>>>
>>> +static void remove_inactive_children(struct uclass *uc, struct udevice
>>> *bus)
>>> +{
>>> +       uclass_foreach_dev(bus, uc) {
>>> +               struct udevice *dev, *next;
>>> +
>>> +               if (!device_active(bus))
>>> +                       continue;
>>> +               device_foreach_child_safe(dev, next, bus) {
>>> +                       if (!device_active(dev))
>>> +                               device_unbind(dev);
>>> +               }
>>> +       }
>>> +}
>>> +
>>>   int usb_init(void)
>>>   {
>>>         int controllers_initialized = 0;
>>> @@ -270,6 +284,15 @@ int usb_init(void)
>>>         }
>>>
>>>         debug("scan end\n");
>>> +
>>> +       /* Remove any devices that were not found on this scan */
>>> +       remove_inactive_children(uc, bus);
>>> +
>>> +       ret = uclass_get(UCLASS_USB_HUB, &uc);
>>> +       if (ret)
>>> +               return ret;
>>> +       remove_inactive_children(uc, bus);
>>> +
>>
>>
>> Why do you need to call remove_inactive_children twice here? This seems
>> worthy of a comment explaining why this is necessary.
>
> One is removing the children of USB controllers, one is removing the
> children of USB hubs. I'll add a comment.
>
>>
>>>         /* if we were not able to find at least one working bus, bail out
>>> */
>>>         if (!count)
>>>                 printf("No controllers found\n");
>
> Regards,
> Simon