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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+zupgzTe7pVhByLPVr4tn-94kCMZbzNHxf_iVyTa3qi6bzvAQ@mail.gmail.com>
Date:   Fri, 26 May 2023 11:34:44 -0700
From:   Roy Luo <royluo@...gle.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     raychi@...gle.com, badhri@...gle.com,
        Alan Stern <stern@...land.harvard.edu>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Michael Grzeschik <m.grzeschik@...gutronix.de>,
        Bastien Nocera <hadess@...ess.net>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Douglas Anderson <dianders@...omium.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH v1] usb: core: add sysfs entry for usb device state

On Thu, May 25, 2023 at 5:50 PM Alan Stern <stern@...land.harvard.edu> 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@...land.harvard.edu> 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@...uxfoundation.org> 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