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 10:29:58PM +0200, Michael Grzeschik wrote:
> 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:
> > > 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

I see.  It relies on the fact that the hub-specific requests have their
recipient field set to Device or Other, not Interface (which in my opinion 
it should be).  This means we can't intercept these requests easily.

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

Regardless, the hub driver won't be aware that the port's power state has 
changed, so it won't update the power_bits value.

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

Yes, I think there is no choice.

Alan Stern