From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Mon, 9 Nov 2015 12:25:01 -0800 Subject: [U-Boot] [PATCH v2 18/26] dm: usb: Remove inactive children after a bus scan In-Reply-To: <564057DC.1000809@redhat.com> References: <1447051688-24936-1-git-send-email-sjg@chromium.org> <1447051688-24936-19-git-send-email-sjg@chromium.org> <564057DC.1000809@redhat.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Hans, On 9 November 2015 at 00:22, Hans de Goede 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 > > > 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