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

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

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.

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

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

Alan Stern

> For this, the functions need to be unset static and exported via hub.h.
> 
> I will add this in v4.