| Message ID | 20220603115222.2151283-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: hub: port: add sysfs entry to switch port power | expand |
On Fri, Jun 03, 2022 at 01:52:22PM +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. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- This is looking a lot better. I have only a few small comments. > 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 > > v2 -> v3: > - renamed sysfs file to disable instead of port_power > - added disable_show function to read out the current port state > - moved usb_lock/unlock_device near put/get_interface > - removed unnecessary usb_set_configuration of port_dev->child before disconnect > - removed unnecessary usb_autosuspend of port_dev->child before disconnect > - moved clearing of port_feature flags to be done after usb_hub_set_port_power > - checking for hub->disconnected after locking hdev > - updated the ABI documentation > > Documentation/ABI/testing/sysfs-bus-usb | 11 +++++ > drivers/usb/core/port.c | 61 +++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > index 7efe31ed3a25c7..d907123ac5d0f9 100644 > --- a/Documentation/ABI/testing/sysfs-bus-usb > +++ b/Documentation/ABI/testing/sysfs-bus-usb > @@ -253,6 +253,17 @@ 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>/disable > +Date: June 2022 > +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> > +Description: > + This file controls the state to USB ports, including s/to USB ports/of a USB port/ > + Vbus power output (but only on hubs that support > + power switching -- most hubs don't support it). When > + turning a port off, the port is unusable: Devices s/When turning a port off/If a port is disabled/ > + 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..8343590c3800f8 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -17,6 +17,66 @@ static int usb_port_block_power_off; > > static const struct attribute_group *port_dev_group[]; > > +static ssize_t disable_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + 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); > + int port1 = port_dev->portnum; > + bool state = test_bit(port1, hub->power_bits); > + > + return sprintf(buf, "%s\n", state ? "0" : "1"); Maybe "false" and "true" instead of "0" and "1"? > +} > + > +static ssize_t disable_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; Is this test needed? If it is then it should be present in disable_show() as well, and the line above that calls to_usb_interface() should not run until after the test has been done. However, I suspect the test isn't needed. > + > + rc = strtobool(buf, &set); > + if (rc) > + return rc; > + > + rc = usb_autopm_get_interface(intf); > + if (rc < 0) > + return rc; > + > + usb_lock_device(hdev); > + if (unlikely(hub->disconnected)) Add: rc = -ENODEV; > + goto out_hdev_lock; > + > + if (set && port_dev->child) > + usb_disconnect(&port_dev->child); I think the logic will be easier to understand if you rename "set" to "disabled". > + > + rc = usb_hub_set_port_power(hdev, hub, port1, !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); > + } > + > + if (!rc) > + rc = count; > + > +out_hdev_lock: > + usb_unlock_device(hdev); > + usb_autopm_put_interface(intf); > + > + return rc; > +} > +static DEVICE_ATTR_RW(disable); > + > static ssize_t location_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -153,6 +213,7 @@ static struct attribute *port_dev_attrs[] = { > &dev_attr_location.attr, > &dev_attr_quirks.attr, > &dev_attr_over_current_count.attr, > + &dev_attr_disable.attr, > NULL, > }; Alan Stern
On Fri, Jun 03, 2022 at 11:58:50AM -0400, Alan Stern wrote: >On Fri, Jun 03, 2022 at 01:52:22PM +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. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- > >This is looking a lot better. I have only a few small comments. > >> 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 >> >> v2 -> v3: >> - renamed sysfs file to disable instead of port_power >> - added disable_show function to read out the current port state >> - moved usb_lock/unlock_device near put/get_interface >> - removed unnecessary usb_set_configuration of port_dev->child before disconnect >> - removed unnecessary usb_autosuspend of port_dev->child before disconnect >> - moved clearing of port_feature flags to be done after usb_hub_set_port_power >> - checking for hub->disconnected after locking hdev >> - updated the ABI documentation >> >> Documentation/ABI/testing/sysfs-bus-usb | 11 +++++ >> drivers/usb/core/port.c | 61 +++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb >> index 7efe31ed3a25c7..d907123ac5d0f9 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-usb >> +++ b/Documentation/ABI/testing/sysfs-bus-usb >> @@ -253,6 +253,17 @@ 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>/disable >> +Date: June 2022 >> +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> >> +Description: >> + This file controls the state to USB ports, including > >s/to USB ports/of a USB port/ Okay >> + Vbus power output (but only on hubs that support >> + power switching -- most hubs don't support it). When >> + turning a port off, the port is unusable: Devices > >s/When turning a port off/If a port is disabled/ Okay >> + 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..8343590c3800f8 100644 >> --- a/drivers/usb/core/port.c >> +++ b/drivers/usb/core/port.c >> @@ -17,6 +17,66 @@ static int usb_port_block_power_off; >> >> static const struct attribute_group *port_dev_group[]; >> >> +static ssize_t disable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + 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); >> + int port1 = port_dev->portnum; >> + bool state = test_bit(port1, hub->power_bits); >> + >> + return sprintf(buf, "%s\n", state ? "0" : "1"); > >Maybe "false" and "true" instead of "0" and "1"? I prefer 0 and 1 since we also feed this to the file. Also, I just found out that just parsing power_bits is not enough. E.g. when we use other tools to set clear PORT_POWER on the hub like uhubctl to disable a port. The value does not represent the real state of the port. I think it is better to use hub_port_status and port_is_power_on from hub.c to test the real state by sending a GET_STATUS command. For this, the functions need to be unset static and exported via hub.h. I will add this in v4. >> +} >> + >> +static ssize_t disable_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; > >Is this test needed? If it is then it should be present in >disable_show() as well, and the line above that calls to_usb_interface() >should not run until after the test has been done. > >However, I suspect the test isn't needed. Okay, I will remove it. >> + >> + rc = strtobool(buf, &set); >> + if (rc) >> + return rc; >> + >> + rc = usb_autopm_get_interface(intf); >> + if (rc < 0) >> + return rc; >> + >> + usb_lock_device(hdev); >> + if (unlikely(hub->disconnected)) > >Add: rc = -ENODEV; > Right. >> + goto out_hdev_lock; >> + >> + if (set && port_dev->child) >> + usb_disconnect(&port_dev->child); > >I think the logic will be easier to understand if you rename "set" to >"disabled". Much better! >> + >> + rc = usb_hub_set_port_power(hdev, hub, port1, !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); >> + } >> + >> + if (!rc) >> + rc = count; >> + >> +out_hdev_lock: >> + usb_unlock_device(hdev); >> + usb_autopm_put_interface(intf); >> + >> + return rc; >> +} >> +static DEVICE_ATTR_RW(disable); >> + >> static ssize_t location_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -153,6 +213,7 @@ static struct attribute *port_dev_attrs[] = { >> &dev_attr_location.attr, >> &dev_attr_quirks.attr, >> &dev_attr_over_current_count.attr, >> + &dev_attr_disable.attr, >> NULL, >> }; > >Alan Stern Thanks, Michael
On Fri, Jun 03, 2022 at 07:22:09PM +0200, Michael Grzeschik wrote: > On Fri, Jun 03, 2022 at 11:58:50AM -0400, Alan Stern wrote: > > On Fri, Jun 03, 2022 at 01:52:22PM +0200, Michael Grzeschik wrote: > > > +static ssize_t disable_show(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + 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); > > > + int port1 = port_dev->portnum; > > > + bool state = test_bit(port1, hub->power_bits); > > > + > > > + return sprintf(buf, "%s\n", state ? "0" : "1"); > > > > Maybe "false" and "true" instead of "0" and "1"? > > I prefer 0 and 1 since we also feed this to the file. Oops -- I was wrong about "true" or "false" (which is a little odd; you'd think a parser for boolean values would certainly recognize those words). However, strtobool() does also recognize "yes", "no", "on", and "off". Using "on" and "off" would lead to confusion, since "on" means "disable is turned on", which means the port is off! "Yes" and "no" would be okay, though. > Also, I just found out that just parsing power_bits is not enough. > > E.g. when we use other tools to set clear PORT_POWER on the hub like > uhubctl to disable a port. The value does not represent the real state > of the port. The value in power_bits is always supposed to match the real state of the port. How does uhubctl manage to get them to disagree? > I think it is better to use hub_port_status and port_is_power_on from > hub.c to test the real state by sending a GET_STATUS command. Maybe. But if power_bits is working properly, this should not be needed. It would be better to fix the value stored in power_bits. Alan Stern > For this, the functions need to be unset static and exported via hub.h. > > I will add this in v4.
On Fri, Jun 03, 2022 at 03:37:44PM -0400, Alan Stern wrote: >On Fri, Jun 03, 2022 at 07:22:09PM +0200, Michael Grzeschik wrote: >> On Fri, Jun 03, 2022 at 11:58:50AM -0400, Alan Stern wrote: >> > On Fri, Jun 03, 2022 at 01:52:22PM +0200, Michael Grzeschik wrote: > >> > > +static ssize_t disable_show(struct device *dev, >> > > + struct device_attribute *attr, char *buf) >> > > +{ >> > > + 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); >> > > + int port1 = port_dev->portnum; >> > > + bool state = test_bit(port1, hub->power_bits); >> > > + >> > > + return sprintf(buf, "%s\n", state ? "0" : "1"); >> > >> > Maybe "false" and "true" instead of "0" and "1"? >> >> I prefer 0 and 1 since we also feed this to the file. > >Oops -- I was wrong about "true" or "false" (which is a little odd; >you'd think a parser for boolean values would certainly recognize those >words). However, strtobool() does also recognize "yes", "no", "on", and >"off". > >Using "on" and "off" would lead to confusion, since "on" means >"disable is turned on", which means the port is off! "Yes" and "no" >would be okay, though. I still prefer 1 an 0. >> Also, I just found out that just parsing power_bits is not enough. >> >> E.g. when we use other tools to set clear PORT_POWER on the hub like >> uhubctl to disable a port. The value does not represent the real state >> of the port. > >The value in power_bits is always supposed to match the real state of >the port. How does uhubctl manage to get them to disagree? https://github.com/mvp/uhubctl/blob/master/uhubctl.c#L1082 It just calls a direct control transfer with rather CLEAR or SET_FEATURE set. So this will be transfered completely passing the kernel usb core layer. I actually would expect that the hubs interrupt worker would trigger. I will have to check if this is the case. >> I think it is better to use hub_port_status and port_is_power_on from >> hub.c to test the real state by sending a GET_STATUS command. > >Maybe. But if power_bits is working properly, this should not be >needed. It would be better to fix the value stored in power_bits. I don't know if this is trivial. If it is not, I would prefer to trigger the GET_STATUS in disable_show for now. Regards, Michael
On Fri, Jun 03, 2022 at 10:29:58PM +0200, Michael Grzeschik wrote: > On Fri, Jun 03, 2022 at 03:37:44PM -0400, Alan Stern wrote: > > On Fri, Jun 03, 2022 at 07:22:09PM +0200, Michael Grzeschik wrote: > > > Also, I just found out that just parsing power_bits is not enough. > > > > > > E.g. when we use other tools to set clear PORT_POWER on the hub like > > > uhubctl to disable a port. The value does not represent the real state > > > of the port. > > > > The value in power_bits is always supposed to match the real state of > > the port. How does uhubctl manage to get them to disagree? > > https://github.com/mvp/uhubctl/blob/master/uhubctl.c#L1082 I see. It relies on the fact that the hub-specific requests have their recipient field set to Device or Other, not Interface (which in my opinion it should be). This means we can't intercept these requests easily. > It just calls a direct control transfer with rather CLEAR or SET_FEATURE > set. So this will be transfered completely passing the kernel usb core > layer. I actually would expect that the hubs interrupt worker would > trigger. I will have to check if this is the case. Regardless, the hub driver won't be aware that the port's power state has changed, so it won't update the power_bits value. > > > I think it is better to use hub_port_status and port_is_power_on from > > > hub.c to test the real state by sending a GET_STATUS command. > > > > Maybe. But if power_bits is working properly, this should not be > > needed. It would be better to fix the value stored in power_bits. > > I don't know if this is trivial. If it is not, I would prefer to > trigger the GET_STATUS in disable_show for now. Yes, I think there is no choice. Alan Stern
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb index 7efe31ed3a25c7..d907123ac5d0f9 100644 --- a/Documentation/ABI/testing/sysfs-bus-usb +++ b/Documentation/ABI/testing/sysfs-bus-usb @@ -253,6 +253,17 @@ 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>/disable +Date: June 2022 +Contact: Michael Grzeschik <m.grzeschik@pengutronix.de> +Description: + This file controls the state to USB ports, including + Vbus power output (but only on hubs that support + power switching -- most hubs don't support it). When + turning a port 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..8343590c3800f8 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -17,6 +17,66 @@ static int usb_port_block_power_off; static const struct attribute_group *port_dev_group[]; +static ssize_t disable_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + 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); + int port1 = port_dev->portnum; + bool state = test_bit(port1, hub->power_bits); + + return sprintf(buf, "%s\n", state ? "0" : "1"); +} + +static ssize_t disable_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; + + usb_lock_device(hdev); + if (unlikely(hub->disconnected)) + goto out_hdev_lock; + + if (set && port_dev->child) + usb_disconnect(&port_dev->child); + + rc = usb_hub_set_port_power(hdev, hub, port1, !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); + } + + if (!rc) + rc = count; + +out_hdev_lock: + usb_unlock_device(hdev); + usb_autopm_put_interface(intf); + + return rc; +} +static DEVICE_ATTR_RW(disable); + static ssize_t location_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -153,6 +213,7 @@ static struct attribute *port_dev_attrs[] = { &dev_attr_location.attr, &dev_attr_quirks.attr, &dev_attr_over_current_count.attr, + &dev_attr_disable.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 v2 -> v3: - renamed sysfs file to disable instead of port_power - added disable_show function to read out the current port state - moved usb_lock/unlock_device near put/get_interface - removed unnecessary usb_set_configuration of port_dev->child before disconnect - removed unnecessary usb_autosuspend of port_dev->child before disconnect - moved clearing of port_feature flags to be done after usb_hub_set_port_power - checking for hub->disconnected after locking hdev - updated the ABI documentation Documentation/ABI/testing/sysfs-bus-usb | 11 +++++ drivers/usb/core/port.c | 61 +++++++++++++++++++++++++ 2 files changed, 72 insertions(+)