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:
signature.asc
Description: PGP signature
