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