[PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Rajat Jain]

Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
unconditionally. Let us try to help the users to identify the removable
root hubs, by checking the device on which the root hub sits. If the base
(parent) device on which the root hub sits, is removable (e.g. on
thunderbolt docks), then the roothub is also marked as removable.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/usb/core/hub.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 86658a81d284..45d1c81b121d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2440,8 +2440,16 @@ static void set_usb_port_removable(struct usb_device *udev)
 
 	dev_set_removable(&udev->dev, DEVICE_REMOVABLE_UNKNOWN);
 
-	if (!hdev)
+	if (!hdev) {
+		/*
+		 * If the root hub sits on a removable device, mark the root hub
+		 * removable as well. This helps with the USB root hubs sitting
+		 * on the thunderbolt docks.
+		 */
+		if (udev->dev.parent && dev_is_removable(udev->dev.parent))
+			dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
 		return;
+	}
 
 	hub = usb_hub_to_struct_hub(udev->parent);
 
-- 
2.33.0.685.g46640cef36-goog

[PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Rajat Jain]

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(+)

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);
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Greg Kroah-Hartman]

On Wed, Sep 29, 2021 at 03:48:22PM -0700, Rajat Jain wrote:
> Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> unconditionally. Let us try to help the users to identify the removable
> root hubs, by checking the device on which the root hub sits. If the base
> (parent) device on which the root hub sits, is removable (e.g. on
> thunderbolt docks), then the roothub is also marked as removable.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/usb/core/hub.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 86658a81d284..45d1c81b121d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2440,8 +2440,16 @@ static void set_usb_port_removable(struct usb_device *udev)
>  
>  	dev_set_removable(&udev->dev, DEVICE_REMOVABLE_UNKNOWN);
>  
> -	if (!hdev)
> +	if (!hdev) {
> +		/*
> +		 * If the root hub sits on a removable device, mark the root hub
> +		 * removable as well. This helps with the USB root hubs sitting
> +		 * on the thunderbolt docks.
> +		 */
> +		if (udev->dev.parent && dev_is_removable(udev->dev.parent))
> +			dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
>  		return;
> +	}

How far "up the chain" are you going to go here?  What if the
thunderbolt device is on a PCI device that can be removed?  What if that
PCI device is on a PCI bus that can be removed?

Is a USB controller on a docking device really "removable"?

The goal here is to say if this device itself is removable in the sense
of "I can yank this out", and a roothub is not that type of device.

What problem are you trying to solve with this series?

thanks,

greg k-h

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Greg Kroah-Hartman]

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

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Oliver Neukum]

On 30.09.21 00:48, Rajat Jain wrote:
> Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> unconditionally. Let us try to help the users to identify the removable
> root hubs, by checking the device on which the root hub sits. If the base
> (parent) device on which the root hub sits, is removable (e.g. on
> thunderbolt docks), then the roothub is also marked as removable.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>

Hi,

frankly, why? You are needlessly throwing away information about where
in the tree
removal can happen. This looks like a worsening, not an improvement to me.

    Regards
        Oliver

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Rajat Jain]

Hello,

On Wed, Sep 29, 2021 at 10:30 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 29, 2021 at 03:48:22PM -0700, Rajat Jain wrote:
> > Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> > unconditionally. Let us try to help the users to identify the removable
> > root hubs, by checking the device on which the root hub sits. If the base
> > (parent) device on which the root hub sits, is removable (e.g. on
> > thunderbolt docks), then the roothub is also marked as removable.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/usb/core/hub.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 86658a81d284..45d1c81b121d 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2440,8 +2440,16 @@ static void set_usb_port_removable(struct usb_device *udev)
> >
> >       dev_set_removable(&udev->dev, DEVICE_REMOVABLE_UNKNOWN);
> >
> > -     if (!hdev)
> > +     if (!hdev) {
> > +             /*
> > +              * If the root hub sits on a removable device, mark the root hub
> > +              * removable as well. This helps with the USB root hubs sitting
> > +              * on the thunderbolt docks.
> > +              */
> > +             if (udev->dev.parent && dev_is_removable(udev->dev.parent))
> > +                     dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
> >               return;
> > +     }
>
> How far "up the chain" are you going to go here?  What if the
> thunderbolt device is on a PCI device that can be removed?  What if that
> PCI device is on a PCI bus that can be removed?

PCI already takes care of that.

>
> Is a USB controller on a docking device really "removable"?
>
> The goal here is to say if this device itself is removable in the sense
> of "I can yank this out", and a roothub is not that type of device.
>
> What problem are you trying to solve with this series?

Essentially we're trying to collect some statistics (and perhaps
implement some policies) that require differentiating between internal
USB devices and external USB devices. Today, a root hub (whether
internal or external) is always shown up as "unknown". Since the
roothubs on a thunderbolt dock are removable from the system, IMHO it
is appropriate to indicate the same in sysfs.

Thanks,

Rajat

>
> thanks,
>
> greg k-h

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Rajat Jain]

+Dmitry Torokhov

On Thu, Sep 30, 2021 at 1:02 AM Oliver Neukum <oneukum@suse.com> wrote:
>
>
> On 30.09.21 00:48, Rajat Jain wrote:
> > Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> > unconditionally. Let us try to help the users to identify the removable
> > root hubs, by checking the device on which the root hub sits. If the base
> > (parent) device on which the root hub sits, is removable (e.g. on
> > thunderbolt docks), then the roothub is also marked as removable.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
>
> Hi,
>
> frankly, why? You are needlessly throwing away information about where
> in the tree
> removal can happen.

I think this comment was meant for the *next* patch
(https://lkml.org/lkml/2021/9/29/1023) and perhaps we can discuss this
there? Let me copy your comment there and respond.

*This* patch does not throw away any information as currently the root
hubs are always and unconditionally marked as "unknown". This patch
aims to provide information where we can, for roothubs only.

Thanks,

Rajat


This looks like a worsening, not an improvement to me.
>
>     Regards
>         Oliver
>

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Rajat Jain]

+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

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Greg Kroah-Hartman]

On Wed, Sep 29, 2021 at 03:48:22PM -0700, Rajat Jain wrote:
> Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> unconditionally. Let us try to help the users to identify the removable
> root hubs, by checking the device on which the root hub sits. If the base
> (parent) device on which the root hub sits, is removable (e.g. on
> thunderbolt docks), then the roothub is also marked as removable.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/usb/core/hub.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 86658a81d284..45d1c81b121d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2440,8 +2440,16 @@ static void set_usb_port_removable(struct usb_device *udev)
>  
>  	dev_set_removable(&udev->dev, DEVICE_REMOVABLE_UNKNOWN);
>  
> -	if (!hdev)
> +	if (!hdev) {
> +		/*
> +		 * If the root hub sits on a removable device, mark the root hub
> +		 * removable as well. This helps with the USB root hubs sitting
> +		 * on the thunderbolt docks.
> +		 */
> +		if (udev->dev.parent && dev_is_removable(udev->dev.parent))

How can a roothub device not have a parent?

I still don't know about this.  What userspace tool is going to do
anything with this information?  What is broken today that this fixes?

thanks,

greg k-h

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Alan Stern]

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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Dmitry Torokhov]

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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Alan Stern]

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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Rajat Jain]

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

Re: [PATCH 1/2] usb: hub: Mark root hubs on removable devices, as removable.

[Rajat Jain]

On Tue, Oct 5, 2021 at 4:19 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 29, 2021 at 03:48:22PM -0700, Rajat Jain wrote:
> > Currently all usb root hubs are always marked as DEVICE_REMOVABLE_UNKNOWN
> > unconditionally. Let us try to help the users to identify the removable
> > root hubs, by checking the device on which the root hub sits. If the base
> > (parent) device on which the root hub sits, is removable (e.g. on
> > thunderbolt docks), then the roothub is also marked as removable.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/usb/core/hub.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 86658a81d284..45d1c81b121d 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2440,8 +2440,16 @@ static void set_usb_port_removable(struct usb_device *udev)
> >
> >       dev_set_removable(&udev->dev, DEVICE_REMOVABLE_UNKNOWN);
> >
> > -     if (!hdev)
> > +     if (!hdev) {
> > +             /*
> > +              * If the root hub sits on a removable device, mark the root hub
> > +              * removable as well. This helps with the USB root hubs sitting
> > +              * on the thunderbolt docks.
> > +              */
> > +             if (udev->dev.parent && dev_is_removable(udev->dev.parent))
>
> How can a roothub device not have a parent?

It was a sanity check. I can remove that if we know that it can never happen.

>
> I still don't know about this.  What userspace tool is going to do
> anything with this information?  What is broken today that this fixes?

With these 2 patches, Chromeos intends to collect statistics about
external USB device usage in the system. The tool will be an internal
chrome os statistics collection tool. Today, any thunderbolt docks and
its devices appear as "unknown". There is also a VM case that is
helped by these patches, but I think Dmitry on Benson on this thread
can elaborate more about that.

Thanks & Best Regards,

Rajat


>
> thanks,
>
> greg k-h

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Andrew Lunn]

> 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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Oliver Neukum]

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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Alan Stern]

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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[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

Re: [PATCH 2/2] usb: hub: Mark devices downstream a removable hub, as removable

[Oliver Neukum]

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