Re: [PATCH 4/5] USB: core: Add API to change the wireless_status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 05:51:48PM +0100, Bastien Nocera wrote:
> > On Thu, 2023-02-23 at 11:25 -0500, Alan Stern wrote:
> > > On Thu, Feb 23, 2023 at 05:17:13PM +0100, Bastien Nocera wrote:
> > > > On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> > > > > On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera
> > > > > wrote:
> > > > > > Allow device specific drivers to change the wireless status
> > > > > > of
> > > > > > a
> > > > > > device.
> > > > > > This will allow user-space to know whether the device is
> > > > > > available,
> > > > > > whether or not specific USB interfaces can detect it.
> > > > > > 
> > > > > > This can be used by wireless headsets with USB receivers to
> > > > > > propagate to
> > > > > > user-space whether or not the headset is turned on, so as
> > > > > > to
> > > > > > consider it
> > > > > > as unavailable, and not switch to it just because the
> > > > > > receiver
> > > > > > is
> > > > > > plugged in.
> > > > > > 
> > > > > > Signed-off-by: Bastien Nocera <hadess@xxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/usb/core/message.c | 13 +++++++++++++
> > > > > >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> > > > > >  include/linux/usb.h        |  4 ++++
> > > > > >  3 files changed, 41 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/usb/core/message.c
> > > > > > b/drivers/usb/core/message.c
> > > > > > index 127fac1af676..d5c7749d515e 100644
> > > > > > --- a/drivers/usb/core/message.c
> > > > > > +++ b/drivers/usb/core/message.c
> > > > > > @@ -1908,6 +1908,18 @@ static void
> > > > > > __usb_queue_reset_device(struct
> > > > > > work_struct *ws)
> > > > > >         usb_put_intf(iface);    /* Undo _get_ in
> > > > > > usb_queue_reset_device() */
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * Internal function to set the wireless_status sysfs
> > > > > > attribute
> > > > > > + * See usb_set_wireless_status() for more details
> > > > > > + */
> > > > > > +static void __usb_wireless_status_intf(struct work_struct
> > > > > > *ws)
> > > > > > +{
> > > > > > +       struct usb_interface *iface =
> > > > > > +               container_of(ws, struct usb_interface,
> > > > > > wireless_status_work);
> > > > > > +
> > > > > > +       usb_update_wireless_status_attr(iface);
> > > > > > +       usb_put_intf(iface);    /* Undo _get_ in
> > > > > > usb_set_wireless_status() */
> > > > > > +}
> > > > > 
> > > > > Have you thought about what will happen if this routine ends
> > > > > up
> > > > > running 
> > > > > after the interface has been deleted?
> > > > 
> > > > I believe that usb_release_interface() will only be called once
> > > > the
> > > > last reference to the interface is dropped, so bar any
> > > > refcounting
> > > > bugs, the interface should always exist when this function is
> > > > called.
> > > 
> > > Yes, but what about the calls made by
> > > usb_update_wireless_status_attr(): 
> > > sysfs_update_group(), sysfs_notify(), and kobject_uevent()?  Will
> > > they 
> > > work properly when called for an object that has been
> > > unregistered
> > > from 
> > > sysfs?
> > 
> > Those calls are made before the last reference to the interface can
> > be
> > dropped by our own call to usb_put_intf(). So, in effect, the
> > interface
> > is kept alive until we're done with our work so we can't ever be
> > poking
> > at objects that disappeared.
> > 
> > Am I missing something about the object model, or the refcounting
> > here?
> 
> Yes, you are.
> 
> There's a difference between object _existence_ and object 
> _registration_.  An object (kobject, struct device, whatever) exists
> for 
> as long as its memory is still allocated and in use.  This is what 
> refcounting protects.
> 
> An object is registered in sysfs during the time between calls to
> device_add() and device_del().  During this time it shows up as a 
> directory under /sys, along with its various attribute files. 
> Calling 
> device_del() gets rid of all that -- James Bottomley calls it
> "removing 
> an object from visibility".  The object still exists, but it isn't
> part 
> of sysfs any more.
> 
> The refcounting in your patch guarantees that when the work function 
> runs, the interface structure will still exist.  But refcounting does
> not guarantee that the interface will still be registered in sysfs,
> and 
> this can actually happen if the work is scheduled immediately before
> the 
> interface is unregistered.
> 
> So my question is: What will happen when sysfs_update_group(), 
> sysfs_notify(), and kobject_uevent() are called after the interface
> has 
> been unregistered from sysfs?  Maybe they will work okay -- I simply 
> don't know, and I wanted to find out whether you had considered the 
> issue.

A long week-end started for me a couple of hours ago, but I wanted to
dump my thoughts before either I forgot, or it took over my whole week-
end ;)

I had thought about the problem, and didn't think that sysfs files
would get removed before the interface got released/unref'ed and
usb_remove_sysfs_intf_files() got called.

If the device gets removed from the device bus before it's released,
then this patch should fix it:
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1917,7 +1917,8 @@ static void __usb_wireless_status_intf(struct work_struct *ws)
        struct usb_interface *iface =
                container_of(ws, struct usb_interface, wireless_status_work);
 
-       usb_update_wireless_status_attr(iface);
+       if (intf->sysfs_files_created)
+               usb_update_wireless_status_attr(iface);
        usb_put_intf(iface);    /* Undo _get_ in usb_set_wireless_status() */

The callback would be a no-op if the device's sysfs is already
unregistered, just unref'ing the reference it held.

What do you think? I'll amend that into my patchset on Monday.

Cheers



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux