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 21:34 -0500, Alan Stern wrote:
> On Fri, Feb 24, 2023 at 12:04:12AM +0100, Bastien Nocera wrote:
> > On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote:
> > > 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.
> 
> That's a good way to do it, but it does race with 
> usb_remove_sysfs_intf_files().  To prevent this race, you can protect
> the test and function call with device_lock(iface->dev.parent) (that
> is, 
> lock the interface's parent usb_device).

Perfect, I've done that locally.

I'll send an updated patchset once I've been able to test it against
the hardware I have.

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