Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi,
On 29-06-15 05:45, Simon Glass wrote:
Hi Hans,
On 17 June 2015 at 13:33, Hans de Goede hdego...@redhat.com 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 hdego...@redhat.com
---
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
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi, On 06/30/2015 04:58 PM, Simon Glass wrote: Hi Hans, On 30 June 2015 at 06:54, Hans de Goede hdego...@redhat.com wrote: Hi, On 29-06-15 05:45, Simon Glass wrote: Hi Hans, On 17 June 2015 at 13:33, Hans de Goede hdego...@redhat.com 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? I don't think that is necessary, it feels a bit too inflexible. But perhaps you could add a comment to the Kconfig help for CONFIG_DM_DEVICE_REMOVE? Ok will do. It is remove() that is needed, not unbind(). Actually I think it is quite unfortunate to make usb_stop() call unbind. It is a waste of time to do this just before booting the kernel - the current design leaves all devices bound (but I hope we can remove() them at some point). Instead, I wonder if we can remove the children when we probe the bus? That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge. Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today. AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus. 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. True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan. That seems like needless complexity. I believe that simply removing + unbinding the children on usb_stop is the right thing to do, and it also is the KISS solution. Regards, Hans ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi Hans, On 30 June 2015 at 09:54, Hans de Goede hdego...@redhat.com wrote: Hi, On 06/30/2015 04:58 PM, Simon Glass wrote: Hi Hans, On 30 June 2015 at 06:54, Hans de Goede hdego...@redhat.com wrote: Hi, On 29-06-15 05:45, Simon Glass wrote: Hi Hans, On 17 June 2015 at 13:33, Hans de Goede hdego...@redhat.com 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? I don't think that is necessary, it feels a bit too inflexible. But perhaps you could add a comment to the Kconfig help for CONFIG_DM_DEVICE_REMOVE? Ok will do. It is remove() that is needed, not unbind(). Actually I think it is quite unfortunate to make usb_stop() call unbind. It is a waste of time to do this just before booting the kernel - the current design leaves all devices bound (but I hope we can remove() them at some point). Instead, I wonder if we can remove the children when we probe the bus? That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge. That's how it currently works, from what I can see in the code. But since there is a 'usb_started' boolean this is irrelevant. Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today. AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus. Sort-of. But as with PCI it is useful to be able to add settings for the devices in some cases. You can match them using vendor/device or interface IDs. Then the driver can access its settings. That's why I'm suggesting we unbind the devices that are no-longer present. 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. True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan. That seems like needless complexity. I believe that simply removing + unbinding the children on usb_stop is the right thing to do, and it also is the KISS solution. I'm good with the
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi Hans,
On 30 June 2015 at 06:54, Hans de Goede hdego...@redhat.com wrote:
Hi,
On 29-06-15 05:45, Simon Glass wrote:
Hi Hans,
On 17 June 2015 at 13:33, Hans de Goede hdego...@redhat.com 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?
I don't think that is necessary, it feels a bit too inflexible. But
perhaps you could add a comment to the Kconfig help for
CONFIG_DM_DEVICE_REMOVE?
It is remove() that is needed, not unbind(). Actually I think it is
quite unfortunate to make usb_stop() call unbind. It is a waste of
time to do this just before booting the kernel - the current design
leaves all devices bound (but I hope we can remove() them at some
point).
Instead, I wonder if we can remove the children when we probe the bus?
Also, what happens to children that are in the device tree - i.e.
static USB devices like WiFi? The device tree might have parameters
for them. Still, that might not matter as I'm not sure that case is
handled correctly today.
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.
True, although you also have 'usb tree' for that. Another option would
be to mark devices that were found and remove the others after the
scan.
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 hdego...@redhat.com
---
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);
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi, On 06/30/2015 06:07 PM, Simon Glass wrote: snip Instead, I wonder if we can remove the children when we probe the bus? That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge. That's how it currently works, from what I can see in the code. But since there is a 'usb_started' boolean this is irrelevant. Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today. AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus. Sort-of. But as with PCI it is useful to be able to add settings for the devices in some cases. You can match them using vendor/device or interface IDs. Then the driver can access its settings. AFAIK there is not a single example of having settings in devicetree for an usb device, since usb-devices are always 100% self describing since usb is a bus designed for hot(un)plug from the outset. That's why I'm suggesting we unbind the devices that are no-longer present. You're asking to make the code more complicated here using a what if reasoning with a what if which is likely to never happen. 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. True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan. That seems like needless complexity. I believe that simply removing + unbinding the children on usb_stop is the right thing to do, and it also is the KISS solution. I'm good with the remove(), but less sure about the unbind(). The unbind is necessary for usb_get_dev_index() to work properly, which is necessary for proper output of usb tree and for driver binding to work properly, without the unbind usb-keyboards will e.g. not work in certain circumstances. The state of 'dm tree' does not bother me, The state if dm tree is what usb_get_dev_index() works from, so if it is not in a good state, then usb_get_dev_index() will not work. and I worry that we then limit our ability to use usb_find_child() to locate a device's parameters (i.e. support for more complex devices which need settings might be harder). Again this is a what if reasoning for a hypothetical future problem which will likely never happen, where as the broken state of the dm tree after a usb reset is causing real problems. For now, can we just leave this alone? I really don't want to re-visit this later. Nope we cannot leave this alone, without unbinding usb devices which no longer exist, the dm tree will be broken and with it usb_get_dev_index() and through usb_get_dev_index() the keyboard driver. Regards, Hans ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
HI Hans, On 30 June 2015 at 14:20, Hans de Goede hdego...@redhat.com wrote: Hi, On 06/30/2015 06:07 PM, Simon Glass wrote: snip Instead, I wonder if we can remove the children when we probe the bus? That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge. That's how it currently works, from what I can see in the code. But since there is a 'usb_started' boolean this is irrelevant. Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today. AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus. Sort-of. But as with PCI it is useful to be able to add settings for the devices in some cases. You can match them using vendor/device or interface IDs. Then the driver can access its settings. AFAIK there is not a single example of having settings in devicetree for an usb device, since usb-devices are always 100% self describing since usb is a bus designed for hot(un)plug from the outset. That's why I'm suggesting we unbind the devices that are no-longer present. You're asking to make the code more complicated here using a what if reasoning with a what if which is likely to never happen. 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. True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan. That seems like needless complexity. I believe that simply removing + unbinding the children on usb_stop is the right thing to do, and it also is the KISS solution. I'm good with the remove(), but less sure about the unbind(). The unbind is necessary for usb_get_dev_index() to work properly, which is necessary for proper output of usb tree and for driver binding to work properly, without the unbind usb-keyboards will e.g. not work in certain circumstances. The state of 'dm tree' does not bother me, The state if dm tree is what usb_get_dev_index() works from, so if it is not in a good state, then usb_get_dev_index() will not work. and I worry that we then limit our ability to use usb_find_child() to locate a device's parameters (i.e. support for more complex devices which need settings might be harder). Again this is a what if reasoning for a hypothetical future problem which will likely never happen, where as the broken state of the dm tree after a usb reset is causing real problems. For now, can we just leave this alone? I really don't want to re-visit this later. Nope we cannot leave this alone, without unbinding usb devices which no longer exist, the dm tree will be broken and with it usb_get_dev_index() and through usb_get_dev_index() the keyboard driver. We really should not be calling usb_get_dev_index() from the keyboard driver - that function is an interim port-helper. If it were ported fully to driver model it could use USB_DEVICE() like mass storage. We could fairly easily change the devnum to 0 for all devices instead of deleting them, and I suspect that would solve the problem. But we would have to port usb ethernet which uses the same technique. Anyway I think you've persuaded me that I might be worrying too much about what is to come. USB is not really the same as PCI in the sense that settings don't seem to be added to the device like PCI. Let's go with your approach. We still have the USB emulation stuff working, and that's the only real use case today. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
Hi Hans,
On 17 June 2015 at 13:33, Hans de Goede hdego...@redhat.com wrote:
On an usb stop instead of leaving orphan usb devices behind simply remove
On a usb_stop()
or
On a 'usb stop' command
?
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.
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.
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 hdego...@redhat.com
---
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
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?
Also, why bother with the #ifdef if this function is never called
outside sandbox?
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop
On an usb stop instead of leaving orphan usb devices behind simply remove
them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build
usb_stop() when that is set.
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.
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 hdego...@redhat.com
---
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 */
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
