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@xxxxxxxxxxxxxx> --- 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@xxxxxxxxxxxxxx> +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@xxxxxxxxxxxxxxx> 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 -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
