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

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Thu, Jun 02, 2022 at 03:16:27PM -0400, Alan Stern wrote:
>On Thu, Jun 02, 2022 at 04:59:18PM +0200, Michael Grzeschik wrote:
>> On Thu, Jun 02, 2022 at 10:39:54AM -0400, Alan Stern wrote:
>> > 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?
>
>Actually, I don't think so.  You can get some information about ganged
>power switching, and there's the hub_is_port_power_switchable() test, but
>that's all.  The situation is discussed in section 11.11 (Hub Port Power
>Control) of the USB-2.0 spec.

I think we can skip this check then, since this sysfs file could still
work with hubs, that do not support port power switching by at least
disabling the port connection.

>> 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.
>
>Yes, that's how most hubs work.  There are a few, however, which really do
>switch port Vbus power on and off.
>
>> 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.
>
>That makes sense.  But the question arises, does this patch really do what
>you want?
>
>The patch description talks about the need to disable devices or
>re-enumerate them.  You can disable a device right now by writing -1 to
>the bConfigurationValue sysfs file, and you can force a device to be
>re-enumerated by resetting it (using the USBDEVFS_RESET usbfs ioctl).
>
>About the only thing you can't currently do is actually turn off power to
>the port.  This patch will allow users to do that, but only if the hub
>supports power switching.
>
>(Okay, there's one other thing: The patch also allows users to disable a
>port, so that devices plugged into that port get ignored.  Maybe that's
>what you really had in mind...?)

Yes, that is what I had in mind. If you agree, I would still keep the
name "port_power" since it is the main function, but skip the
hub_is_port_power_switchable check.

>> > 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?
>
>The USB spec does say that hubs should ignore connections to ports whose
>port_power feature flag is off.  The test I suggested was meant as a "just
>in case" sort of thing, for hubs that don't comply with the spec's
>requirement.  In the end it may not be necessary, and it can be done in a
>separate patch.

Agreed.

>> 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);.
>
>Good idea; I should have thought of it.

:)

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