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
