| Message ID | 20210929224823.556943-2-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] usb: hub: Mark root hubs on removable devices, as removable. | expand |
On Wed, Sep 29, 2021 at 03:48:23PM -0700, Rajat Jain wrote: > If a usb device sits below a removable hub, mark the device also as > removable. This helps with devices inserted on a standard removable hub or > also thunderbold docks, to be shown as removable. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > drivers/usb/core/hub.c | 7 +++++++ > 1 file changed, 7 insertions(+) Combined with the previous patch, you are now marking all devices that happen to be attached to a root hub that is on a thunderbolt controller as removable. So all USB devices inside of a docking station are now removable? What type of devices did you test this series out with? And again, what problem are you trying to solve? thanks, greg k-h
+Dmitry Torokhov Hi Greg, Oliver, Thanks for taking a look. On Wed, Sep 29, 2021 at 10:31 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 29, 2021 at 03:48:23PM -0700, Rajat Jain wrote: > > If a usb device sits below a removable hub, mark the device also as > > removable. This helps with devices inserted on a standard removable hub or > > also thunderbold docks, to be shown as removable. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > drivers/usb/core/hub.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > Combined with the previous patch, you are now marking all devices that > happen to be attached to a root hub that is on a thunderbolt controller > as removable. So all USB devices inside of a docking station are now > removable? With this patch, yes that was my intent. I think what we are debating here is should the "removable" attribute imply possibility of removal from "the system" or just the "local immediate box" (e.g. thunderbolt dock). In my mind, the removable property was analogous to imply an "external device", i.e a device that may be removed from the system, perhaps as a result of its parent devices being removed from the system. I guess this definition doesn't match what you believe it should be? [Oliver says] > frankly, why? You are needlessly throwing away information about where > in the tree > removal can happen. I believe you are referring to multi level USB hubs and feel that "removable" should be set only for devices that hang off a port, and not for children of such device. I wouldn't necessarily disagree, pending the discussion above (although I think it applies to this patch only, I think the previous patch still provides value without throwing away any info). As a data point, I notice that with my USB hub, the USB device representing the hub is correctly marked as "removable", however a USB device I insert into the USB hub, is shown as "unknown". I don't know if this is the behavior with all USB hubs or just because my USB hub has a bug. But my patch helps solve this issue and makes the device show up as "removable". Thanks Rajat > > What type of devices did you test this series out with? And again, what > problem are you trying to solve? > > thanks, > > greg k-h
On Mon, Oct 04, 2021 at 03:42:46PM -0700, Rajat Jain wrote: > +Dmitry Torokhov > > Hi Greg, Oliver, > > Thanks for taking a look. > > On Wed, Sep 29, 2021 at 10:31 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Sep 29, 2021 at 03:48:23PM -0700, Rajat Jain wrote: > > > If a usb device sits below a removable hub, mark the device also as > > > removable. This helps with devices inserted on a standard removable hub or > > > also thunderbold docks, to be shown as removable. > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > --- > > > drivers/usb/core/hub.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > Combined with the previous patch, you are now marking all devices that > > happen to be attached to a root hub that is on a thunderbolt controller > > as removable. So all USB devices inside of a docking station are now > > removable? > > With this patch, yes that was my intent. I think what we are debating > here is should the "removable" attribute imply possibility of removal > from "the system" or just the "local immediate box" (e.g. thunderbolt > dock). In my mind, the removable property was analogous to imply an > "external device", i.e a device that may be removed from the system, > perhaps as a result of its parent devices being removed from the > system. I guess this definition doesn't match what you believe it > should be? As I understand it, the "removable" property refers specifically to the device's upstream link, not to whether _any_ of the links leading from the device to the computer could be removed. This is probably what Oliver meant when he complained about losing information. With the knowledge of whether each individual link is removable, you can easily tell whether there's some way to remove a device from the system. But if you only know whether the device is removable from the system overall, you generally can't tell whether the link to the device's parent is removable. Alan Stern
Hi Alan, On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Oct 04, 2021 at 03:42:46PM -0700, Rajat Jain wrote: > > +Dmitry Torokhov > > > > Hi Greg, Oliver, > > > > Thanks for taking a look. > > > > On Wed, Sep 29, 2021 at 10:31 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Sep 29, 2021 at 03:48:23PM -0700, Rajat Jain wrote: > > > > If a usb device sits below a removable hub, mark the device also as > > > > removable. This helps with devices inserted on a standard removable hub or > > > > also thunderbold docks, to be shown as removable. > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > --- > > > > drivers/usb/core/hub.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > Combined with the previous patch, you are now marking all devices that > > > happen to be attached to a root hub that is on a thunderbolt controller > > > as removable. So all USB devices inside of a docking station are now > > > removable? > > > > With this patch, yes that was my intent. I think what we are debating > > here is should the "removable" attribute imply possibility of removal > > from "the system" or just the "local immediate box" (e.g. thunderbolt > > dock). In my mind, the removable property was analogous to imply an > > "external device", i.e a device that may be removed from the system, > > perhaps as a result of its parent devices being removed from the > > system. I guess this definition doesn't match what you believe it > > should be? > > As I understand it, the "removable" property refers specifically to > the device's upstream link, not to whether _any_ of the links leading > from the device to the computer could be removed. No, that is not what it means. I'll cite our sysfs ABI: What: /sys/devices/.../removable Date: May 2021 Contact: Rajat Jain <rajatxjain@gmail.com> Description: Information about whether a given device can be removed from the platform by the user. This is determined by its subsystem in a bus / platform-specific way. This attribute is only present for devices that can support determining such information: "removable": device can be removed from the platform by the user "fixed": device is fixed to the platform / cannot be removed by the user. "unknown": The information is unavailable / cannot be deduced. Currently this is only supported by USB (which infers the information from a combination of hub descriptor bits and platform-specific data such as ACPI) and PCI (which gets this from ACPI / device tree). It specifically talks about _platform_, not about properties of some peripheral attached to a system. Note that the wording is very similar to what we had for USB devices that originally implemented "removable" attribute: > > This is probably what Oliver meant when he complained about losing > information. With the knowledge of whether each individual link is > removable, you can easily tell whether there's some way to remove a > device from the system. But if you only know whether the device is > removable from the system overall, you generally can't tell whether > the link to the device's parent is removable. If we need this data then we need to establish some new attribute to convey this info. Thanks, Dmitry
On Tue, Oct 05, 2021 at 09:51:02AM -0700, Dmitry Torokhov wrote: > Hi Alan, > > On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > As I understand it, the "removable" property refers specifically to > > the device's upstream link, not to whether _any_ of the links leading > > from the device to the computer could be removed. > > No, that is not what it means. I'll cite our sysfs ABI: > > What: /sys/devices/.../removable > Date: May 2021 > Contact: Rajat Jain <rajatxjain@gmail.com> > Description: > Information about whether a given device can be removed from the > platform by the user. This is determined by its subsystem in a > bus / platform-specific way. This attribute is only present for > devices that can support determining such information: > > "removable": device can be removed from the platform by the user > "fixed": device is fixed to the platform / cannot be removed > by the user. > "unknown": The information is unavailable / cannot be deduced. > > Currently this is only supported by USB (which infers the > information from a combination of hub descriptor bits and > platform-specific data such as ACPI) and PCI (which gets this > from ACPI / device tree). > > It specifically talks about _platform_, not about properties of some > peripheral attached to a system. Note that the wording is very similar > to what we had for USB devices that originally implemented "removable" > attribute: In that case, shouldn't Rajat's patch change go into the driver core rather than the hub driver? _Every_ device downstream from a removable link should count as removable, yes? Not just the USB devices. And to say that the attribute is supported only by USB and PCI is misleading, since it applies to every device downstream from a removable link. > > This is probably what Oliver meant when he complained about losing > > information. With the knowledge of whether each individual link is > > removable, you can easily tell whether there's some way to remove a > > device from the system. But if you only know whether the device is > > removable from the system overall, you generally can't tell whether > > the link to the device's parent is removable. > > If we need this data then we need to establish some new attribute to > convey this info. I don't know if we need it, but such an attribute seems like a good idea. Alan Stern
Hello, On Tue, Oct 5, 2021 at 12:59 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, Oct 05, 2021 at 09:51:02AM -0700, Dmitry Torokhov wrote: > > Hi Alan, > > > > On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > As I understand it, the "removable" property refers specifically to > > > the device's upstream link, not to whether _any_ of the links leading > > > from the device to the computer could be removed. > > > > No, that is not what it means. I'll cite our sysfs ABI: > > > > What: /sys/devices/.../removable > > Date: May 2021 > > Contact: Rajat Jain <rajatxjain@gmail.com> > > Description: > > Information about whether a given device can be removed from the > > platform by the user. This is determined by its subsystem in a > > bus / platform-specific way. This attribute is only present for > > devices that can support determining such information: > > > > "removable": device can be removed from the platform by the user > > "fixed": device is fixed to the platform / cannot be removed > > by the user. > > "unknown": The information is unavailable / cannot be deduced. > > > > Currently this is only supported by USB (which infers the > > information from a combination of hub descriptor bits and > > platform-specific data such as ACPI) and PCI (which gets this > > from ACPI / device tree). > > > > It specifically talks about _platform_, not about properties of some > > peripheral attached to a system. Note that the wording is very similar > > to what we had for USB devices that originally implemented "removable" > > attribute: > > In that case, shouldn't Rajat's patch change go into the driver core > rather than the hub driver? _Every_ device downstream from a > removable link should count as removable, yes? Not just the USB > devices. I have no preference either way, and can do that if that is more acceptable. > > And to say that the attribute is supported only by USB and PCI is > misleading, since it applies to every device downstream from a > removable link. However I do think it makes sense to have the bus control whether this attribute applies to it or not. Determining the first point in a hierarchy of devices, where a device can be removed is highly bus specific (set_usb_port_removable()). AFAIK, the primary reason / use of this attribute was to distinguish devices that can be removed by the user, and really all such devices (at least the ones that matter to user) today sit either on PCI or USB bus. We intend to use this attribute to segregate internal devices from external devices, and collect some statistics about usb device usage this way. There is also a VM case that I think Dmitry or Benson on this thread can elaborate more about. There seem to be hundreds of other bus types and I'm not sure if we want to unnecessarily flood the sysfs with a removable attribute under each device. Thanks & Best Regards, Rajat > > > > This is probably what Oliver meant when he complained about losing > > > information. With the knowledge of whether each individual link is > > > removable, you can easily tell whether there's some way to remove a > > > device from the system. But if you only know whether the device is > > > removable from the system overall, you generally can't tell whether > > > the link to the device's parent is removable. > > > > If we need this data then we need to establish some new attribute to > > convey this info. > > I don't know if we need it, but such an attribute seems like a good > idea. > > Alan Stern
> AFAIK, the primary reason / use of this attribute was to distinguish > devices that can be removed by the user, and really all such devices > (at least the ones that matter to user) today sit either on PCI or USB > bus. Hard disk on SATA? You can hot plug them. SFP modules on i2c? I'm sure there are others, which are not PCI or USB. Andrew
On 05.10.21 21:59, Alan Stern wrote: > On Tue, Oct 05, 2021 at 09:51:02AM -0700, Dmitry Torokhov wrote: >> Hi Alan, >> >> On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: >>> As I understand it, the "removable" property refers specifically to >>> the device's upstream link, not to whether _any_ of the links leading >>> from the device to the computer could be removed. >> No, that is not what it means. I'll cite our sysfs ABI: >> >> What: /sys/devices/.../removable >> Date: May 2021 >> Contact: Rajat Jain <rajatxjain@gmail.com> >> Description: >> Information about whether a given device can be removed from the >> platform by the user. This is determined by its subsystem in a >> bus / platform-specific way. This attribute is only present for >> devices that can support determining such information: >> >> "removable": device can be removed from the platform by the user >> "fixed": device is fixed to the platform / cannot be removed >> by the user. >> "unknown": The information is unavailable / cannot be deduced. >> >> Currently this is only supported by USB (which infers the >> information from a combination of hub descriptor bits and >> platform-specific data such as ACPI) and PCI (which gets this >> from ACPI / device tree). >> >> It specifically talks about _platform_, not about properties of some >> peripheral attached to a system. Note that the wording is very similar >> to what we had for USB devices that originally implemented "removable" >> attribute: > In that case, shouldn't Rajat's patch change go into the driver core > rather than the hub driver? _Every_ device downstream from a > removable link should count as removable, yes? Not just the USB > devices. In theory yes. If your HC is removable by that logic every device is. That renders the information content of 'removable' to zero. Everything is removable. > And to say that the attribute is supported only by USB and PCI is > misleading, since it applies to every device downstream from a > removable link. Exactly and it is a difference. If you know that a device is removable you must not disable hotplug detection on that port if you want full functionality. While if you know that a device is not removable you may straight up cut power, even if the _parent_ is still removable. The device tree is a tree and if you want to know whether hotplugging is possible (let's ignore hibernation), you need to walk the tree top to bottom. Regards Oliver
On Tue, Oct 05, 2021 at 04:43:33PM -0700, Rajat Jain wrote: > Hello, > > On Tue, Oct 5, 2021 at 12:59 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Tue, Oct 05, 2021 at 09:51:02AM -0700, Dmitry Torokhov wrote: > > > Hi Alan, > > > > > > On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > As I understand it, the "removable" property refers specifically to > > > > the device's upstream link, not to whether _any_ of the links leading > > > > from the device to the computer could be removed. > > > > > > No, that is not what it means. I'll cite our sysfs ABI: > > > > > > What: /sys/devices/.../removable > > > Date: May 2021 > > > Contact: Rajat Jain <rajatxjain@gmail.com> > > > Description: > > > Information about whether a given device can be removed from the > > > platform by the user. This is determined by its subsystem in a > > > bus / platform-specific way. This attribute is only present for > > > devices that can support determining such information: > > > > > > "removable": device can be removed from the platform by the user > > > "fixed": device is fixed to the platform / cannot be removed > > > by the user. > > > "unknown": The information is unavailable / cannot be deduced. > > > > > > Currently this is only supported by USB (which infers the > > > information from a combination of hub descriptor bits and > > > platform-specific data such as ACPI) and PCI (which gets this > > > from ACPI / device tree). > > > > > > It specifically talks about _platform_, not about properties of some > > > peripheral attached to a system. Note that the wording is very similar > > > to what we had for USB devices that originally implemented "removable" > > > attribute: > > > > In that case, shouldn't Rajat's patch change go into the driver core > > rather than the hub driver? _Every_ device downstream from a > > removable link should count as removable, yes? Not just the USB > > devices. > > I have no preference either way, and can do that if that is more acceptable. > > > > > And to say that the attribute is supported only by USB and PCI is > > misleading, since it applies to every device downstream from a > > removable link. > > However I do think it makes sense to have the bus control whether this > attribute applies to it or not. The sysfs ABI quoted by Dmitry above is a little vague. It seems to say that only certain buses can determine whether a device is removable, but this simply isn't true, because any device downstream from something removable will itself be removable, no matter what kind of bus it's on. > Determining the first point in a > hierarchy of devices, where a device can be removed is highly bus > specific (set_usb_port_removable()). Yes, the bus must be at least partially responsible for _determining_ the value of the removable attribute. But the attribute should _apply_ to all devices, regardless of what bus they are on. To be more precise, the bus can determine whether a device's upstream link (the first link in the chain leading from the device back to the CPU) can be hot-unplugged. The device is removable if any of the links in that chain are hot-unpluggable. > AFAIK, the primary reason / use of this attribute was to distinguish > devices that can be removed by the user, and really all such devices > (at least the ones that matter to user) today sit either on PCI or USB > bus. We intend to use this attribute to segregate internal devices > from external devices, and collect some statistics about usb device > usage this way. There is also a VM case that I think Dmitry or Benson > on this thread can elaborate more about. There seem to be hundreds of > other bus types and I'm not sure if we want to unnecessarily flood the > sysfs with a removable attribute under each device. sysfs already contains a lot of mostly unused information. I don't think adding one or two more will hurt much. Alan Stern
On Wed, Oct 06, 2021 at 11:37:58AM +0200, Oliver Neukum wrote: > > On 05.10.21 21:59, Alan Stern wrote: > > On Tue, Oct 05, 2021 at 09:51:02AM -0700, Dmitry Torokhov wrote: > >> Hi Alan, > >> > >> On Tue, Oct 5, 2021 at 7:56 AM Alan Stern <stern@rowland.harvard.edu> wrote: > >>> As I understand it, the "removable" property refers specifically to > >>> the device's upstream link, not to whether _any_ of the links leading > >>> from the device to the computer could be removed. > >> No, that is not what it means. I'll cite our sysfs ABI: > >> > >> What: /sys/devices/.../removable > >> Date: May 2021 > >> Contact: Rajat Jain <rajatxjain@gmail.com> > >> Description: > >> Information about whether a given device can be removed from the > >> platform by the user. This is determined by its subsystem in a > >> bus / platform-specific way. This attribute is only present for > >> devices that can support determining such information: > >> > >> "removable": device can be removed from the platform by the user > >> "fixed": device is fixed to the platform / cannot be removed > >> by the user. > >> "unknown": The information is unavailable / cannot be deduced. > >> > >> Currently this is only supported by USB (which infers the > >> information from a combination of hub descriptor bits and > >> platform-specific data such as ACPI) and PCI (which gets this > >> from ACPI / device tree). > >> > >> It specifically talks about _platform_, not about properties of some > >> peripheral attached to a system. Note that the wording is very similar > >> to what we had for USB devices that originally implemented "removable" > >> attribute: > > In that case, shouldn't Rajat's patch change go into the driver core > > rather than the hub driver? _Every_ device downstream from a > > removable link should count as removable, yes? Not just the USB > > devices. > In theory yes. If your HC is removable by that logic every device is. > That renders the information content of 'removable' to zero. Everything > is removable. So we should add a new attribute. Call it "unpluggable", perhaps. It will say whether the device's immediate upstream link is hot-unpluggable. Then the device is removable if its parent is removable or if it is unpluggable. > > And to say that the attribute is supported only by USB and PCI is > > misleading, since it applies to every device downstream from a > > removable link. > Exactly and it is a difference. If you know that a device is removable > you must not disable hotplug detection on that port if you want full > functionality. While if you know that a device is not removable you may > straight up cut power, even if the _parent_ is still removable. > > The device tree is a tree and if you want to know whether hotplugging > is possible (let's ignore hibernation), you need to walk the tree top to > bottom. Adding the "unpluggable" attribute should take care of this, right? Alan Stern
On 06.10.21 18:10, Alan Stern wrote: > On Wed, Oct 06, 2021 at 11:37:58AM +0200, Oliver Neukum wrote: >> >> In theory yes. If your HC is removable by that logic every device is. >> That renders the information content of 'removable' to zero. Everything >> is removable. > So we should add a new attribute. Call it "unpluggable", perhaps. It > will say whether the device's immediate upstream link is > hot-unpluggable. Then the device is removable if its parent is > removable or if it is unpluggable. Hi, yes that would solve the issue. We are basically trying two press two attributes into one and that does not work. Regards ' Oliver
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 45d1c81b121d..901d74bcdbd9 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2449,6 +2449,13 @@ static void set_usb_port_removable(struct usb_device *udev) if (udev->dev.parent && dev_is_removable(udev->dev.parent)) dev_set_removable(&udev->dev, DEVICE_REMOVABLE); return; + } else if (dev_is_removable(&hdev->dev)) { + /* + * If this USB device sits downstream a removable hub, then mark + * this device also as removable. + */ + dev_set_removable(&udev->dev, DEVICE_REMOVABLE); + return; } hub = usb_hub_to_struct_hub(udev->parent);
If a usb device sits below a removable hub, mark the device also as removable. This helps with devices inserted on a standard removable hub or also thunderbold docks, to be shown as removable. Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/usb/core/hub.c | 7 +++++++ 1 file changed, 7 insertions(+)