From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Tue, 30 Jun 2015 14:54:21 +0200 Subject: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop In-Reply-To: References: <1434569645-30322-1-git-send-email-hdegoede@redhat.com> <1434569645-30322-9-git-send-email-hdegoede@redhat.com> Message-ID: <5592917D.9040000@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 29-06-15 05:45, Simon Glass wrote: > Hi Hans, > > On 17 June 2015 at 13:33, Hans de Goede 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? > >> >> 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. > >> >> With this commit in place the output after swapping and "usb reset" is: >> >> usb [ + ] `-- sunxi-musb >> usb_dev_gen [ + ] `-- generic_bus_0_dev_1 >> >> As expected, and usb_get_dev_index works properly and the keyboard works. >> >> After this commit usb_find_child() is only necessary for emulated usb >> devices, so make its body #ifdef CONFIG_USB_EMUL. >> >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/host/usb-uclass.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c >> index bce6cec..8f26e35 100644 >> --- a/drivers/usb/host/usb-uclass.c >> +++ b/drivers/usb/host/usb-uclass.c >> @@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev) >> return ops->alloc_device(bus, udev); >> } >> >> +#ifdef CONFIG_DM_DEVICE_REMOVE >> int usb_stop(void) >> { >> struct udevice *bus; >> @@ -143,6 +144,12 @@ int usb_stop(void) >> uc_priv = uc->priv; >> >> uclass_foreach_dev(bus, uc) { >> + ret = device_chld_remove(bus); >> + if (ret && !err) >> + err = ret; >> + ret = device_chld_unbind(bus); >> + if (ret && !err) >> + err = ret; >> ret = device_remove(bus); >> if (ret && !err) >> err = ret; >> @@ -166,6 +173,7 @@ int usb_stop(void) >> >> return err; >> } >> +#endif >> >> static void usb_scan_bus(struct udevice *bus, bool recurse) >> { >> @@ -491,6 +499,7 @@ static int usb_find_child(struct udevice *parent, >> struct usb_interface_descriptor *iface, >> struct udevice **devp) >> { >> +#ifdef CONFIG_USB_EMUL /* Emulated devices are explictily bound */ > > explicitly Ack. > Can you add a comment about this? It seems that we should rename this > function to usb_find_emul_child() and have it present only when > CONFIG_USB_EMUL is around? Renaming it to usb_find_emul_child() and only defining the function when CONFIG_USB_EMUL works for me I will do that for v2. > Also, why bother with the #ifdef if this function is never called > outside sandbox? Because its call site does not have a #ifdef, I'll add an #ifdef at its call site to make it more clear that this is only used for emulated devices. > >> struct udevice *dev; >> >> *devp = NULL; >> @@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent, >> return 0; >> } >> } >> +#endif >> >> return -ENOENT; >> } >> -- >> 2.4.3 >> > > Regards, > Simon > Regards, Hans