| Message ID | 1447051688-24936-19-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
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@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. > 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. > /* if we were not able to find at least one working bus, bail out */ > if (!count) > printf("No controllers found\n"); > Regards, Hans
Hi Hans, On 9 November 2015 at 00:22, Hans de Goede <hdegoede@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@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
Hi Hans, On 9 November 2015 at 12:25, Simon Glass <sjg@chromium.org> wrote: > Hi Hans, > > On 9 November 2015 at 00:22, Hans de Goede <hdegoede@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@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
Hi Hans, On 11 November 2015 at 10:02, Hans de Goede <j.w.r.degoede@gmail.com> wrote: > > Hi, > > > On 11-11-15 00:30, Simon Glass wrote: >> >> Hi Hans, >> >> On 9 November 2015 at 12:25, Simon Glass <sjg@chromium.org> wrote: >>> >>> Hi Hans, >>> >>> On 9 November 2015 at 00:22, Hans de Goede <hdegoede@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@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. > > > 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. > > 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? 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. 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? > > ### > > 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. > > Regards, > > Hans > > > > > >> >>> >>>> >>>> >>>>> 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 Regards, Simon
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
Hi Hans, On 12 November 2015 at 07:08, Hans de Goede <hdegoede@redhat.com> wrote: > 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. The helper function is basically the same as the one you have - it removes devices that are not activated, rather than all devices. I don't think the difference is large. It doesn't actually walk the list of devices twice. It walks the list of USB controllers once, then the list of USB hubs once. We need to check both, as hubs may be need to be unbound. > > 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 I'm not sure that is a bonus. It means that sandbox is no-longer using the same code path as other boards. That's actually one of the objectives of sandbox. > > 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. It does have the virtue of allowing sandbox USB to work properly. At present 'usb reset' is broken, as I mentioned. Willy-nilly unbinding of devices is a bad idea IMO. Yes I understand that the code to find the child is needed, but it's not large: struct udevice *dev; *devp = NULL; for (device_find_first_child(parent, &dev); dev; device_find_next_child(&dev)) { struct usb_dev_platdata *plat = dev_get_parent_platdata(dev); /* If this device is already in use, skip it */ if (device_active(dev)) continue; debug(" %s: name='%s', plat=%d, desc=%d\n", __func__, dev->name, plat->id.bDeviceClass, desc->bDeviceClass); if (usb_match_one_id(desc, iface, &plat->id)) { *devp = dev; return 0; } } and usb_match_one_id() is needed anyway to support drivers. I suppose we could avoid this and always create a new device, which would degenerate to your solution of removing all the old devices. So if we want to provide that option, it would be easy to do. > >> 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. Yes, although in most cases the list of devices will not change. We are really talking about an unusual case. Most of the time when U-Boot starts it runs 'usb start' once. The case where it runs multiple times is IMO not an important case to optimise, unless we are trying to use knowledge of previous devices to speed up detection of the new bus state. > >> 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. OK > >>> ### >>> >>> 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 ? I suppose I could do that. I would like to make sure that any USB fish hooks are removed, and that is part of the solution. I'd really like to resolve this and move things forward. If you still have have strong objections please let me know. We did discuss this at the time and I was clear that I did not want to keep the solution as it was - I merged it because it fixed a bug, as you pointed out at the time. Regards, Simon
Hi, On 11/13/2015 10:58 PM, Simon Glass wrote: > Hi Hans, <snip> >>>> 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 ? > > I suppose I could do that. I would like to make sure that any USB fish > hooks are removed, and that is part of the solution. > > I'd really like to resolve this and move things forward. If you still > have have strong objections please let me know. There are no objections from my side to move forward with your current solution. > We did discuss this at > the time and I was clear that I did not want to keep the solution as > it was - I merged it because it fixed a bug, as you pointed out at the > time. Ack. Regards, Hans
Hi Hans, On 15 November 2015 at 12:35, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 11/13/2015 10:58 PM, Simon Glass wrote: >> >> Hi Hans, > > > <snip> > >>>>> 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 ? >> >> >> I suppose I could do that. I would like to make sure that any USB fish >> hooks are removed, and that is part of the solution. >> >> I'd really like to resolve this and move things forward. If you still >> have have strong objections please let me know. > > > There are no objections from my side to move forward with your current > solution. > >> We did discuss this at >> the time and I was clear that I did not want to keep the solution as >> it was - I merged it because it fixed a bug, as you pointed out at the >> time. > > > Ack. OK thanks. I'll see if I can get some more comments on the other patches. Regards, Simon
Applied to u-boot-dm.
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); + /* if we were not able to find at least one working bus, bail out */ if (!count) printf("No controllers found\n");
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@chromium.org> --- Changes in v2: None drivers/usb/host/usb-uclass.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)