[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-usb
Subject: Re: [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Bastien Nocera <hadess () hadess ! net>
Date: 2023-02-28 16:23:28
Message-ID: cd0cabef9b434fdbf640a37c357878cdac80358b.camel () hadess ! net
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic