From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C7968C64ED6 for ; Fri, 24 Feb 2023 02:34:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229587AbjBXCe0 (ORCPT ); Thu, 23 Feb 2023 21:34:26 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49420 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229441AbjBXCeZ (ORCPT ); Thu, 23 Feb 2023 21:34:25 -0500 Received: from netrider.rowland.org (netrider.rowland.org [192.131.102.5]) by lindbergh.monkeyblade.net (Postfix) with SMTP id 1CC851B330 for ; Thu, 23 Feb 2023 18:34:24 -0800 (PST) Received: (qmail 22418 invoked by uid 1000); 23 Feb 2023 21:34:23 -0500 Date: Thu, 23 Feb 2023 21:34:22 -0500 From: Alan Stern To: Bastien Nocera Cc: linux-usb@vger.kernel.org, linux-input@vger.kernel.org, Greg Kroah-Hartman , Benjamin Tissoires , Filipe =?iso-8859-1?Q?La=EDns?= , Nestor Lopez Casado Subject: Re: [PATCH 4/5] USB: core: Add API to change the wireless_status Message-ID: References: <20230223132452.37958-1-hadess@hadess.net> <20230223132452.37958-4-hadess@hadess.net> <16aaaa1a6207e7da07faa932ecac0dcc9e5f10e3.camel@hadess.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org 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). Alan Stern