| Message ID | 20210929224823.556943-1-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: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
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
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
+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 >
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
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
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);
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(-)