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

[Michael Grzeschik <mgr@pengutronix.de>]

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

Hi!

On Wed, May 11, 2022 at 10:09:26AM -0400, Alan Stern wrote:
>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.

Good point. This goes together with the missing ABI documentation.
I will fix it in the v2.

>> 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.

I could remove it for the sake of simplicity in the first version.
If this will matter in the future we can add it then.

>> +
>> +	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.

I found this function was called when plugging in a device into an
suspended hub. So it seemed to be the right thing to do.

>Furthermore, nothing prevents the hub from going back into runtime
>suspend the moment this function completes.

OK. That needs to be locked than.

Is it with usb_autopm_get_interface and usb_autopm_put_interface?

>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.

In case the hub had only one device attached to one one port. When I
called this sysfs function on that port, the hub would suspend
afterwards. Which is probably a correct way to go for a hub with
no active devices attached.

When replacing usb_remote_wakeup with usb_autoresume_device this
works exactly the same. So I will replace it in v2.

The extra usb_autosuspend_device for the hub is probably not necessary.

>> +
>> +	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.

Yes. I will do that in v2.

>> +
>> +	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.

Here are two things that are in play.

First I have to set port_dev->child = NULL before calling
usb_disconnect. Otherwise the following automatic hub_suspend call (in
case this was the last operational port of the hub) will print the
message "device x-y not suspended yet" and will fail the hub to
suspend.

When calling usb_autoresume_device(udev) instead and before calling
usb_disconnect, this is no longer the case. The hub will be succesfully
suspended.

The second thing is the assignment. I still have to explicitly assign NULL to
port_dev->child. Otherwise a following enable of this port via this sysfs will
run into an hub_event with the usb_disonnect for the device on that port. But
this will spit out a ugly traceback leading with the following error:

[   21.718574] usb 2-1.1: USB disconnect, device number -1

[   21.719100] Unable to handle kernel paging request at virtual address 96d628cc24e2e078
[   21.719807] Mem abort info:
[   21.720065]   ESR = 0x96000044
[   21.720348]   EC = 0x25: DABT (current EL), IL = 32 bits
[   21.720827]   SET = 0, FnV = 0
[   21.721109]   EA = 0, S1PTW = 0
[   21.721447]   FSC = 0x04: level 0 translation fault
[   21.721891] Data abort info:
[   21.722157]   ISV = 0, ISS = 0x00000044
[   21.722505]   CM = 0, WnR = 1
[   21.722779] [96d628cc24e2e078] address between user and kernel address ranges
[   21.723429] Internal error: Oops: 96000044 [#1] PREEMPT SMP
[   21.723927] Modules linked in: uio_pdrv_genirq fuse
[   21.724380] CPU: 0 PID: 58 Comm: kworker/0:3 Not tainted 5.18.0-rc6+ #93
[   21.724977] Hardware name: Radxa ROCK3 Model A (DT)
[   21.725412] Workqueue: usb_hub_wq hub_event
[   21.725802] pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   21.726419] pc : usb_disable_endpoint+0x7c/0xdc
[   21.726832] lr : usb_disable_device_endpoints+0xbc/0xe0
[   21.727301] sp : ffffffc009b33b30
[   21.727597] x29: ffffffc009b33b30 x28: ffffff8003ad4ed8 x27: 0000000000000001
[   21.728239] x26: ffffff8004344928 x25: 0000000000000000 x24: ffffffc0096eb8e0
[   21.728885] x23: ffffff80043448a8 x22: ffffff8003961800 x21: 0000000000000001
[   21.729527] x20: 96d628cc24e2e034 x19: ffffff8004344800 x18: ffffffffffffffff
[   21.730168] x17: 0000000000000001 x16: 0000000000000001 x15: ffffffc089b33857
[   21.730810] x14: 0000000000000000 x13: 312d207265626d75 x12: 6e20656369766564
[   21.731452] x11: 00000000fffff7ff x10: 00000000fffff7ff x9 : ffffffc00871b8dc
[   21.732093] x8 : 000000000000bfe8 x7 : ffffffc009b33a38 x6 : 0000000000000001
[   21.732733] x5 : ffffffc009569000 x4 : ffffffc009569050 x3 : ffffff8004344878
[   21.733374] x2 : 0000000000000001 x1 : 000000000000008f x0 : 0000000000000001
[   21.734015] Call trace:
[   21.734235]  usb_disable_endpoint+0x7c/0xdc
[   21.734616]  usb_disable_device_endpoints+0xbc/0xe0
[   21.735054]  usb_disable_device+0x1c0/0x260
[   21.735432]  usb_disconnect+0x108/0x300
[   21.735778]  hub_event+0x1378/0x19c0
[   21.736102]  process_one_work+0x220/0x49c
[   21.736469]  worker_thread+0x154/0x450
[   21.736810]  kthread+0xfc/0x110
[   21.737096]  ret_from_fork+0x10/0x20
[   21.737429] Code: f941c474 340001e0 f901c47f b4ffff14 (b900469f)
[   21.737970] ---[ end trace 0000000000000000 ]---

Did I miss something?

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

Thanks,
Michael Grzeschik

-- 
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 --]