| Message ID | 20220602012731.2942309-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: hub: port: add sysfs entry to switch port power | expand |
On Thu, Jun 02, 2022 at 03:27:31AM +0200, Michael Grzeschik wrote: > In some cases the port of an hub needs to be disabled or switched off > and on again. E.g. when the connected device needs to be re-enumerated. > Or it needs to be explicitly disabled while the rest of the usb tree > stays working. > > For this purpose this patch adds an sysfs switch to enable/disable the > port on any hub. In the case the hub is supporting power switching, the > power line will be disabled to the connected device. > > When the port gets disabled, the associated device gets disconnected and > removed from the logical usb tree. No further device will be enumerated > on that port until the port gets enabled again. A lot better than the description in the earlier patch version! > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > v1 -> v2: > - improved patch description > - moved usb_hub_set_port_power to end of function > - renamed value to set > - removed udev variable > - added usb_set_configuration set to -1 before removing device > - calling autosuspend of udev before usb_disconnect, ensuring hub_suspend succeeds > - removed port_dev->child = NULL assignment > - directly returning count on no failure success > - removed test for hub->in_reset > - using usb_autopm_get_interface/usb_autopm_put_interface around hub handling > - locking usb_disconnect call > - using &port_dev->child instead of local udev pointer > - added Documentation/ABI > > Documentation/ABI/testing/sysfs-bus-usb | 13 +++++++ > drivers/usb/core/port.c | 49 +++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > index 7efe31ed3a25c7..9c87ca50bcab79 100644 > --- a/Documentation/ABI/testing/sysfs-bus-usb > +++ b/Documentation/ABI/testing/sysfs-bus-usb > @@ -253,6 +253,19 @@ Description: > only if the system firmware is capable of describing the > connection between a port and its connector. > > +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/port_power > +Date: June 2022 > +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> > +Description: > + To disable or enable a hub port the sysfs file port_power exists > + for each hub port. When disabling the hub port it is unusable anymore, > + which means no enumeration will take place on this port until enabled again. > + > + When disabling the port set (<hubdev-portX>/port_power to 0) the > + USB_PORT_FEAT_C_CONNECTION, USB_PORT_FEAT_POWER and (for high speed hubs) the > + USB_PORT_FEAT_C_ENABLE port features are cleared. It all gets reversed when the > + port will be enabled again (set <hubdev-portX>/port_power to 1). This description is rather disorganized. I'd prefer something like this: This file controls Vbus power to USB ports (but only on hubs that support power switching -- most hubs don't support it). When power to a port is turned off, the port is unusable: Devices attached to the port will not be detected, initialized, or enumerated. > + > What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout > Date: May 2013 > Contact: Mathias Nyman <mathias.nyman@linux.intel.com> > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index d5bc36ca5b1f77..3e707db88291e9 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -17,6 +17,54 @@ static int usb_port_block_power_off; > > static const struct attribute_group *port_dev_group[]; > > +static ssize_t port_power_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct usb_port *port_dev = to_usb_port(dev); > + struct usb_device *hdev = to_usb_device(dev->parent->parent); > + struct usb_hub *hub = usb_hub_to_struct_hub(hdev); > + struct usb_interface *intf = to_usb_interface(hub->intfdev); > + int port1 = port_dev->portnum; > + bool set; > + int rc; > + > + if (!hub) > + return -EINVAL; > + > + rc = strtobool(buf, &set); > + if (rc) > + return rc; > + > + rc = usb_autopm_get_interface(intf); > + if (rc < 0) > + return rc; You should call usb_lock_device(hdev) here, not later. And after the hub has been locked, you have to check whether hub->disconnected has been set. > + > + if (!set) { > + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION); > + if (!port_dev->is_superspeed) > + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); These should be cleared after the port power is disabled, not before. > + if (port_dev->child) { > + usb_set_configuration(port_dev->child, -1); > + usb_autosuspend_device(port_dev->child); I don't see why either of these is needed. (In fact, some devices malfunction when you try to unconfigure them.) > + usb_lock_device(hdev); > + usb_disconnect(&port_dev->child); > + usb_unlock_device(hdev); > + } > + } > + > + rc = usb_hub_set_port_power(hdev, hub, port1, set); And call usb_unlock_device(hdev) here, after the port feature flags have been cleared. > + if (rc) { > + usb_autopm_put_interface(intf); > + return rc; > + } > + > + usb_autopm_put_interface(intf); > + > + return count; Simpler: if (rc == 0) rc = count; usb_autopm_put_interface(intf); return rc; > +} > +static DEVICE_ATTR_WO(port_power); > + > static ssize_t location_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -153,6 +201,7 @@ static struct attribute *port_dev_attrs[] = { > &dev_attr_location.attr, > &dev_attr_quirks.attr, > &dev_attr_over_current_count.attr, > + &dev_attr_port_power.attr, > NULL, > }; You might want to disable the new sysfs file (don't create it or have it return -EOPNOTSUPP) if the hub doesn't support per-port power switching. Finally, you should add a test to port_event() in hub.c, probably where the pm_runtime_active() check is. If the port is powered off, return before doing any of the warm_reset or connect_change processing. Alan Stern
On Thu, Jun 02, 2022 at 10:39:54AM -0400, Alan Stern wrote: >On Thu, Jun 02, 2022 at 03:27:31AM +0200, Michael Grzeschik wrote: >> In some cases the port of an hub needs to be disabled or switched off >> and on again. E.g. when the connected device needs to be re-enumerated. >> Or it needs to be explicitly disabled while the rest of the usb tree >> stays working. >> >> For this purpose this patch adds an sysfs switch to enable/disable the >> port on any hub. In the case the hub is supporting power switching, the >> power line will be disabled to the connected device. >> >> When the port gets disabled, the associated device gets disconnected and >> removed from the logical usb tree. No further device will be enumerated >> on that port until the port gets enabled again. > >A lot better than the description in the earlier patch version! > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> v1 -> v2: >> - improved patch description >> - moved usb_hub_set_port_power to end of function >> - renamed value to set >> - removed udev variable >> - added usb_set_configuration set to -1 before removing device >> - calling autosuspend of udev before usb_disconnect, ensuring hub_suspend succeeds >> - removed port_dev->child = NULL assignment >> - directly returning count on no failure success >> - removed test for hub->in_reset >> - using usb_autopm_get_interface/usb_autopm_put_interface around hub handling >> - locking usb_disconnect call >> - using &port_dev->child instead of local udev pointer >> - added Documentation/ABI >> >> Documentation/ABI/testing/sysfs-bus-usb | 13 +++++++ >> drivers/usb/core/port.c | 49 +++++++++++++++++++++++++ >> 2 files changed, 62 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb >> index 7efe31ed3a25c7..9c87ca50bcab79 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-usb >> +++ b/Documentation/ABI/testing/sysfs-bus-usb >> @@ -253,6 +253,19 @@ Description: >> only if the system firmware is capable of describing the >> connection between a port and its connector. >> >> +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/port_power >> +Date: June 2022 >> +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> >> +Description: >> + To disable or enable a hub port the sysfs file port_power exists >> + for each hub port. When disabling the hub port it is unusable anymore, >> + which means no enumeration will take place on this port until enabled again. >> + >> + When disabling the port set (<hubdev-portX>/port_power to 0) the >> + USB_PORT_FEAT_C_CONNECTION, USB_PORT_FEAT_POWER and (for high speed hubs) the >> + USB_PORT_FEAT_C_ENABLE port features are cleared. It all gets reversed when the >> + port will be enabled again (set <hubdev-portX>/port_power to 1). > >This description is rather disorganized. I'd prefer something like this: > > This file controls Vbus power to USB ports (but only on hubs > that support power switching -- most hubs don't support it). > When power to a port is turned off, the port is unusable: > Devices attached to the port will not be detected, > initialized, or enumerated. Okay, I will keep yours. >> + >> What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout >> Date: May 2013 >> Contact: Mathias Nyman <mathias.nyman@linux.intel.com> >> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c >> index d5bc36ca5b1f77..3e707db88291e9 100644 >> --- a/drivers/usb/core/port.c >> +++ b/drivers/usb/core/port.c >> @@ -17,6 +17,54 @@ static int usb_port_block_power_off; >> >> static const struct attribute_group *port_dev_group[]; >> >> +static ssize_t port_power_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct usb_port *port_dev = to_usb_port(dev); >> + struct usb_device *hdev = to_usb_device(dev->parent->parent); >> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev); >> + struct usb_interface *intf = to_usb_interface(hub->intfdev); >> + int port1 = port_dev->portnum; >> + bool set; >> + int rc; >> + >> + if (!hub) >> + return -EINVAL; >> + >> + rc = strtobool(buf, &set); >> + if (rc) >> + return rc; >> + >> + rc = usb_autopm_get_interface(intf); >> + if (rc < 0) >> + return rc; > >You should call usb_lock_device(hdev) here, not later. And after the hub >has been locked, you have to check whether hub->disconnected has been set. Will add in v3. >> + >> + if (!set) { >> + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION); >> + if (!port_dev->is_superspeed) >> + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); > >These should be cleared after the port power is disabled, not before. Okay, I will add this to the code. >> + if (port_dev->child) { >> + usb_set_configuration(port_dev->child, -1); >> + usb_autosuspend_device(port_dev->child); > >I don't see why either of these is needed. (In fact, some devices >malfunction when you try to unconfigure them.) I will test it with that code removed. >> + usb_lock_device(hdev); >> + usb_disconnect(&port_dev->child); >> + usb_unlock_device(hdev); >> + } >> + } >> + >> + rc = usb_hub_set_port_power(hdev, hub, port1, set); > >And call usb_unlock_device(hdev) here, after the port feature flags have >been cleared. Will add in v3. >> + if (rc) { >> + usb_autopm_put_interface(intf); >> + return rc; >> + } >> + >> + usb_autopm_put_interface(intf); >> + >> + return count; > > >Simpler: > > if (rc == 0) > rc = count; > > usb_autopm_put_interface(intf); > return rc; Good Idea, I will change this. >> +} >> +static DEVICE_ATTR_WO(port_power); >> + >> static ssize_t location_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -153,6 +201,7 @@ static struct attribute *port_dev_attrs[] = { >> &dev_attr_location.attr, >> &dev_attr_quirks.attr, >> &dev_attr_over_current_count.attr, >> + &dev_attr_port_power.attr, >> NULL, >> }; > >You might want to disable the new sysfs file (don't create it or have it >return -EOPNOTSUPP) if the hub doesn't support per-port power switching. Is it possible to read out if this feature is not working by the hub? The most hubs, that I was working with, did not really toggle the vbus, because the physical logic to switch a regulator was completely missing in the hardware. But with removing the other PORT_FEATURES the hub behaved like the port is just not powered any more. Because of that; I am currently curious if we just should rename that property to something more generic like "enable" or "disable". So that as the real vbus power switching is missing, the hubs port switching does still function like intended. >Finally, you should add a test to port_event() in hub.c, probably where >the pm_runtime_active() check is. If the port is powered off, return >before doing any of the warm_reset or connect_change processing. I don't understand jet why this is needed. In all my tests, the hubs port was just not functioning any more. Regardless if the hub was capable of real vbus switching or not. Just as described above. Is it possible that this is already handled correctly because of the other cleared port_features I mentioned? In v3 I will also add port_power_show to make it possible for the userspace to read out the current port status but returning the value of test_bit(port1, hub->power_bits);. Regards, Michael
On Thu, Jun 02, 2022 at 04:59:18PM +0200, Michael Grzeschik wrote: > On Thu, Jun 02, 2022 at 10:39:54AM -0400, Alan Stern wrote: > > You might want to disable the new sysfs file (don't create it or have it > > return -EOPNOTSUPP) if the hub doesn't support per-port power switching. > > Is it possible to read out if this feature is not working by the hub? Actually, I don't think so. You can get some information about ganged power switching, and there's the hub_is_port_power_switchable() test, but that's all. The situation is discussed in section 11.11 (Hub Port Power Control) of the USB-2.0 spec. > The most hubs, that I was working with, did not really toggle the vbus, > because the physical logic to switch a regulator was completely missing > in the hardware. But with removing the other PORT_FEATURES the hub > behaved like the port is just not powered any more. Yes, that's how most hubs work. There are a few, however, which really do switch port Vbus power on and off. > Because of that; I am currently curious if we just should rename that > property to something more generic like "enable" or "disable". So that > as the real vbus power switching is missing, the hubs port switching > does still function like intended. That makes sense. But the question arises, does this patch really do what you want? The patch description talks about the need to disable devices or re-enumerate them. You can disable a device right now by writing -1 to the bConfigurationValue sysfs file, and you can force a device to be re-enumerated by resetting it (using the USBDEVFS_RESET usbfs ioctl). About the only thing you can't currently do is actually turn off power to the port. This patch will allow users to do that, but only if the hub supports power switching. (Okay, there's one other thing: The patch also allows users to disable a port, so that devices plugged into that port get ignored. Maybe that's what you really had in mind...?) > > Finally, you should add a test to port_event() in hub.c, probably where > > the pm_runtime_active() check is. If the port is powered off, return > > before doing any of the warm_reset or connect_change processing. > > I don't understand jet why this is needed. In all my tests, the hubs > port was just not functioning any more. Regardless if the hub > was capable of real vbus switching or not. Just as described above. > Is it possible that this is already handled correctly because of the > other cleared port_features I mentioned? The USB spec does say that hubs should ignore connections to ports whose port_power feature flag is off. The test I suggested was meant as a "just in case" sort of thing, for hubs that don't comply with the spec's requirement. In the end it may not be necessary, and it can be done in a separate patch. > In v3 I will also add port_power_show to make it possible for the > userspace to read out the current port status but returning the > value of test_bit(port1, hub->power_bits);. Good idea; I should have thought of it. Alan Stern
On Thu, Jun 02, 2022 at 03:16:27PM -0400, Alan Stern wrote: >On Thu, Jun 02, 2022 at 04:59:18PM +0200, Michael Grzeschik wrote: >> On Thu, Jun 02, 2022 at 10:39:54AM -0400, Alan Stern wrote: >> > You might want to disable the new sysfs file (don't create it or have it >> > return -EOPNOTSUPP) if the hub doesn't support per-port power switching. >> >> Is it possible to read out if this feature is not working by the hub? > >Actually, I don't think so. You can get some information about ganged >power switching, and there's the hub_is_port_power_switchable() test, but >that's all. The situation is discussed in section 11.11 (Hub Port Power >Control) of the USB-2.0 spec. I think we can skip this check then, since this sysfs file could still work with hubs, that do not support port power switching by at least disabling the port connection. >> The most hubs, that I was working with, did not really toggle the vbus, >> because the physical logic to switch a regulator was completely missing >> in the hardware. But with removing the other PORT_FEATURES the hub >> behaved like the port is just not powered any more. > >Yes, that's how most hubs work. There are a few, however, which really do >switch port Vbus power on and off. > >> Because of that; I am currently curious if we just should rename that >> property to something more generic like "enable" or "disable". So that >> as the real vbus power switching is missing, the hubs port switching >> does still function like intended. > >That makes sense. But the question arises, does this patch really do what >you want? > >The patch description talks about the need to disable devices or >re-enumerate them. You can disable a device right now by writing -1 to >the bConfigurationValue sysfs file, and you can force a device to be >re-enumerated by resetting it (using the USBDEVFS_RESET usbfs ioctl). > >About the only thing you can't currently do is actually turn off power to >the port. This patch will allow users to do that, but only if the hub >supports power switching. > >(Okay, there's one other thing: The patch also allows users to disable a >port, so that devices plugged into that port get ignored. Maybe that's >what you really had in mind...?) Yes, that is what I had in mind. If you agree, I would still keep the name "port_power" since it is the main function, but skip the hub_is_port_power_switchable check. >> > Finally, you should add a test to port_event() in hub.c, probably where >> > the pm_runtime_active() check is. If the port is powered off, return >> > before doing any of the warm_reset or connect_change processing. >> >> I don't understand jet why this is needed. In all my tests, the hubs >> port was just not functioning any more. Regardless if the hub >> was capable of real vbus switching or not. Just as described above. >> Is it possible that this is already handled correctly because of the >> other cleared port_features I mentioned? > >The USB spec does say that hubs should ignore connections to ports whose >port_power feature flag is off. The test I suggested was meant as a "just >in case" sort of thing, for hubs that don't comply with the spec's >requirement. In the end it may not be necessary, and it can be done in a >separate patch. Agreed. >> In v3 I will also add port_power_show to make it possible for the >> userspace to read out the current port status but returning the >> value of test_bit(port1, hub->power_bits);. > >Good idea; I should have thought of it. :) Thanks, Michael
On Thu, Jun 02, 2022 at 11:34:54PM +0200, Michael Grzeschik wrote: > On Thu, Jun 02, 2022 at 03:16:27PM -0400, Alan Stern wrote: > > On Thu, Jun 02, 2022 at 04:59:18PM +0200, Michael Grzeschik wrote: > > > Because of that; I am currently curious if we just should rename that > > > property to something more generic like "enable" or "disable". So that > > > as the real vbus power switching is missing, the hubs port switching > > > does still function like intended. > > > > That makes sense. But the question arises, does this patch really do what > > you want? > > > > The patch description talks about the need to disable devices or > > re-enumerate them. You can disable a device right now by writing -1 to > > the bConfigurationValue sysfs file, and you can force a device to be > > re-enumerated by resetting it (using the USBDEVFS_RESET usbfs ioctl). > > > > About the only thing you can't currently do is actually turn off power to > > the port. This patch will allow users to do that, but only if the hub > > supports power switching. > > > > (Okay, there's one other thing: The patch also allows users to disable a > > port, so that devices plugged into that port get ignored. Maybe that's > > what you really had in mind...?) > > Yes, that is what I had in mind. If you agree, I would still keep the > name "port_power" since it is the main function, but skip the > hub_is_port_power_switchable check. I favor the more generic name. "disable" will be more understandable for users than "port_power", if the file doesn't actually control the bus power. Alan Stern
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb index 7efe31ed3a25c7..9c87ca50bcab79 100644 --- a/Documentation/ABI/testing/sysfs-bus-usb +++ b/Documentation/ABI/testing/sysfs-bus-usb @@ -253,6 +253,19 @@ Description: only if the system firmware is capable of describing the connection between a port and its connector. +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/port_power +Date: June 2022 +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> +Description: + To disable or enable a hub port the sysfs file port_power exists + for each hub port. When disabling the hub port it is unusable anymore, + which means no enumeration will take place on this port until enabled again. + + When disabling the port set (<hubdev-portX>/port_power to 0) the + USB_PORT_FEAT_C_CONNECTION, USB_PORT_FEAT_POWER and (for high speed hubs) the + USB_PORT_FEAT_C_ENABLE port features are cleared. It all gets reversed when the + port will be enabled again (set <hubdev-portX>/port_power to 1). + What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout Date: May 2013 Contact: Mathias Nyman <mathias.nyman@linux.intel.com> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index d5bc36ca5b1f77..3e707db88291e9 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -17,6 +17,54 @@ static int usb_port_block_power_off; static const struct attribute_group *port_dev_group[]; +static ssize_t port_power_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct usb_port *port_dev = to_usb_port(dev); + struct usb_device *hdev = to_usb_device(dev->parent->parent); + struct usb_hub *hub = usb_hub_to_struct_hub(hdev); + struct usb_interface *intf = to_usb_interface(hub->intfdev); + int port1 = port_dev->portnum; + bool set; + int rc; + + if (!hub) + return -EINVAL; + + rc = strtobool(buf, &set); + if (rc) + return rc; + + rc = usb_autopm_get_interface(intf); + if (rc < 0) + return rc; + + if (!set) { + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION); + if (!port_dev->is_superspeed) + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); + + if (port_dev->child) { + usb_set_configuration(port_dev->child, -1); + usb_autosuspend_device(port_dev->child); + usb_lock_device(hdev); + usb_disconnect(&port_dev->child); + usb_unlock_device(hdev); + } + } + + rc = usb_hub_set_port_power(hdev, hub, port1, set); + if (rc) { + usb_autopm_put_interface(intf); + return rc; + } + + usb_autopm_put_interface(intf); + + return count; +} +static DEVICE_ATTR_WO(port_power); + static ssize_t location_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -153,6 +201,7 @@ static struct attribute *port_dev_attrs[] = { &dev_attr_location.attr, &dev_attr_quirks.attr, &dev_attr_over_current_count.attr, + &dev_attr_port_power.attr, NULL, };
In some cases the port of an hub needs to be disabled or switched off and on again. E.g. when the connected device needs to be re-enumerated. Or it needs to be explicitly disabled while the rest of the usb tree stays working. For this purpose this patch adds an sysfs switch to enable/disable the port on any hub. In the case the hub is supporting power switching, the power line will be disabled to the connected device. When the port gets disabled, the associated device gets disconnected and removed from the logical usb tree. No further device will be enumerated on that port until the port gets enabled again. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1 -> v2: - improved patch description - moved usb_hub_set_port_power to end of function - renamed value to set - removed udev variable - added usb_set_configuration set to -1 before removing device - calling autosuspend of udev before usb_disconnect, ensuring hub_suspend succeeds - removed port_dev->child = NULL assignment - directly returning count on no failure success - removed test for hub->in_reset - using usb_autopm_get_interface/usb_autopm_put_interface around hub handling - locking usb_disconnect call - using &port_dev->child instead of local udev pointer - added Documentation/ABI Documentation/ABI/testing/sysfs-bus-usb | 13 +++++++ drivers/usb/core/port.c | 49 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+)