Re: [RFC PATCH v1] usb: core: add sysfs entry for usb device state

[Roy Luo]

On Thu, May 25, 2023 at 5:50 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, May 25, 2023 at 01:31:17PM -0700, Roy Luo wrote:
> > On Thu, May 25, 2023 at 12:10 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, May 25, 2023 at 11:46:23AM -0700, Roy Luo wrote:
> > > > Alan, thanks for the quick response!
> > > > Yes, port_dev->state is indeed the same as port_dev->child->state. However,
> > > > I still add port_dev->state because port_dev->child won't be assigned until
> > > > the corresponding usb_device is in ADDRESS state.
> > > > I wish I can assign get port_dev->child assigned earlier, but I think
> > > > the current design - assign port_dev->child and device_add() after ADDRESS
> > > > state - also makes sense because there are many ways that the enumeration
> > > > could fail in the early stage. By adding port_dev->state, I can link
> > > > usb_device->state to usb_port as soon as the usb_device is created to get
> > > > around the limitation of port_dev->child.
> > > > I would be very happy to hear other ideas.
> > >
> > > Is there any real reason not to set port_dev->child as soon as the
> > > usb_device structure is created?  If enumeration fails, the pointer can
> > > be cleared.
> > >
> > > Alan Stern
> >
> > Currently the usb core assumes the usb_device that port_dev->child points
> > to is enumerated and port_dev->child->dev is registered when
> > port_dev->child is present. Setting port_dev->child early would break this
> > fundamental assumption, hence I'm a bit reluctant to go this way.
>
> Well, you could remove that assumption by adding a "child_is_registered"
> flag and explicitly checking it.
>
> Alan Stern

Agree that's doable, with the following overheads:
1. We can no longer safely access port_dev->child without checking
    "child_is_registered", and there are plenty of places in the usb core that
    touch port_dev->child. The implicit assumption could also hurt code
    maintainability.
2. In the worst case where enumeration keeps failing, the retry loop in
    hub_port_connect() would frequently hold device_state_lock in order
    to link/unlink the usb_device to port_dev->child.
This would definitely make sense if more places need port_dev->child early.
However, we only need port_dev->child->state at this point, so it does not
seem like a good deal to me.

On Fri, May 26, 2023 at 12:42 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, May 25, 2023 at 05:38:18PM +0000, Roy Luo wrote:
> > + * @work: workqueue for sysfs_notify()
>
> Do you really need this?  This should be possible to call in any context
> as kernfs_notify() says that it is safe to do that, right?
>

Thanks for pointing this out!
Yes, kernfs_notify() or sysfs_notify_dirent() should work, will take
that into the next patch.

> Also, what userspace code is now calling poll() on your new sysfs file?
>
> thanks,
>
> greg k-h

We are looking at adding the code to the generic userspace components
if not hardware abstraction layer in the userspace.

Thanks,
Roy