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

[Alan Stern <stern@rowland.harvard.edu>]

On Wed, May 11, 2022 at 01:13:17AM +0200, Michael Grzeschik wrote:
> This patch adds an sysfs switch to enable/disable a port on an power
> switchable hub. It also ensures that the associated device gets
> disconnected from the logical usb tree.

This says what the patch does.  It does not explain why the patch was 
written or why anybody would want to switch the power on a hub's port.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/core/port.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index d5bc36ca5b1f77..abc618d87888f3 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -17,6 +17,52 @@ 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 *udev = port_dev->child;
> +	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 value;
> +	int rc = 0;
> +
> +	if (!hub)
> +		return -EINVAL;
> +
> +	if (hub->in_reset)
> +		return -EBUSY;

What point is there in doing this test?  The value of hub->in_reset may 
change an instant later.  Unless you acquire the hub's lock first.
For that matter, you should be holding the hub's lock while you call 
usb_hub_to_struct_hub() -- unless you don't care if the hub gets 
disconnected while this routine is running.  Or if udev does.

> +
> +	rc = strtobool(buf, &value);
> +	if (rc)
> +		return rc;
> +
> +	if (value)
> +		usb_remote_wakeup(hdev);

Why call usb_remote_wakeup()?  The function was not intended to be used 
this way; it was meant to be used when a device sends a wakeup request.  
Furthermore, nothing prevents the hub from going back into runtime 
suspend the moment this function completes.

If you want to bring a USB device out of runtime suspend, call 
usb_autoresume_device().  And then don't forget to call 
usb_autosuspend_device() when you're done with it.

> +
> +	rc = usb_hub_set_port_power(hdev, hub, port1, value);
> +	if (rc)
> +		return rc;

You probably should acquire the port's lock before doing this.  
Otherwise some other thread might be doing something else to the port at 
the same time.

> +
> +	if (!value) {
> +		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 (udev) {
> +			port_dev->child = NULL;

That assignment is not necessary; usb_disconnect() will take care of it.

> +			usb_disconnect(&udev);
> +		}
> +	}

Alan Stern