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

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Fri, Jun 03, 2022 at 03:37:44PM -0400, Alan Stern wrote:
>On Fri, Jun 03, 2022 at 07:22:09PM +0200, Michael Grzeschik wrote:
>> 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:
>
>> > > +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.
>
>Oops -- I was wrong about "true" or "false" (which is a little odd;
>you'd think a parser for boolean values would certainly recognize those
>words).  However, strtobool() does also recognize "yes", "no", "on", and
>"off".
>
>Using "on" and "off" would lead to confusion, since "on" means
>"disable is turned on", which means the port is off!  "Yes" and "no"
>would be okay, though.

I still prefer 1 an 0.

>> 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.
>
>The value in power_bits is always supposed to match the real state of
>the port.  How does uhubctl manage to get them to disagree?

https://github.com/mvp/uhubctl/blob/master/uhubctl.c#L1082

It just calls a direct control transfer with rather CLEAR or SET_FEATURE
set. So this will be transfered completely passing the kernel usb core
layer. I actually would expect that the hubs interrupt worker would
trigger. I will have to check if this is the case.

>> 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.
>
>Maybe.  But if power_bits is working properly, this should not be
>needed.  It would be better to fix the value stored in power_bits.

I don't know if this is trivial. If it is not, I would prefer to
trigger the GET_STATUS in disable_show for now.

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