Re: [PATCH v3] usb: hub: port: add sysfs entry to switch port power

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7421 bytes --]

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

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]