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

[Simon Glass]

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.

>
>
>> 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