| Message ID | 20230223132452.37958-4-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] HID: logitech-hidpp: Add support for ADC measurement feature | expand |
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@hadess.net> > --- > 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() */ > +} Why is this in a workqueue? Why can't it just happen when asked? I do not see any justification for that in your changelog or code :( thanks, greg k-h
On Thu, 2023-02-23 at 14:52 +0100, Greg Kroah-Hartman 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@hadess.net> > > --- > > 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() */ > > +} > > Why is this in a workqueue? Why can't it just happen when asked? > > I do not see any justification for that in your changelog or code :( Because, in many cases, updates would be triggered from USB events, which happen in a softirq. Is that explanation good enough for the commit message, or do you prefer it in the code? Looks something like this if you don't use a workqueue: Call Trace: <IRQ> dump_stack_lvl+0x5b/0x77 mark_lock.cold+0x48/0xe1 ? lock_is_held_type+0xe8/0x140 ? lock_is_held_type+0xe8/0x140 __lock_acquire+0x800/0x1ef0 ? update_load_avg+0x762/0x890 lock_acquire+0xce/0x2d0 ? __kmem_cache_alloc_node+0x31/0x3b0 ? lock_is_held_type+0xe8/0x140 ? find_held_lock+0x32/0x90 fs_reclaim_acquire+0xa5/0xe0 ? __kmem_cache_alloc_node+0x31/0x3b0 __kmem_cache_alloc_node+0x31/0x3b0 ? usb_set_wireless_status+0x29/0x120 kmalloc_trace+0x26/0x60 usb_set_wireless_status+0x29/0x120 hidpp_raw_hidpp_event.cold+0x107/0x119 [hid_logitech_hidpp] ? lock_release+0x14f/0x460 hidpp_raw_event+0xf2/0x610 [hid_logitech_hidpp] ? _raw_spin_unlock_irqrestore+0x40/0x60 hid_input_report+0x142/0x160 hid_irq_in+0x177/0x1a0 __usb_hcd_giveback_urb+0x9a/0x110 usb_giveback_urb_bh+0x97/0x110 tasklet_action_common.constprop.0+0xe3/0x110 __do_softirq+0xec/0x590 __irq_exit_rcu+0xf9/0x170 irq_exit_rcu+0xa/0x30 common_interrupt+0xb9/0xd0 </IRQ> <TASK> asm_common_interrupt+0x22/0x40 RIP: 0010:cpuidle_enter_state+0xeb/0x320 Code: 4b 99 75 ff 45 84 ff 74 16 9c 58 0f 1f 40 00 f6 c4 02 0f 85 05 02 00 00 31 ff e8 80 b6 7d ff e8 1b 9c 85 ff fb 0f 1f 44 00 00 <45> 85 ed 0f 88 e2 00 00 00 49 63 cd 4c 89 f2 48 8d 04 49 48 8d 04 RSP: 0018:ffffaa07c017be98 EFLAGS: 00000206 RAX: 00000000000a8327 RBX: ffffca07be8036b8 RCX: 0000000000000001 RDX: 0000000000000000 RSI: ffffffffb46ba489 RDI: ffffffffb463dd36 RBP: 0000000000000008 R08: 0000000000000001 R09: 0000000000000001 R10: 0000000000000001 R11: 0000000000000000 R12: ffffffffb50d5e00 R13: 0000000000000008 R14: 0000001da3b692b2 R15: 0000000000000000 ? cpuidle_enter_state+0xe5/0x320 cpuidle_enter+0x29/0x40 do_idle+0x21d/0x2b0 cpu_startup_entry+0x19/0x20 start_secondary+0x113/0x140 secondary_startup_64_no_verify+0xe5/0xeb </TASK> BUG: sleeping function called from invalid context at include/linux/sched/mm.h:274 in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/5 preempt_count: 101, expected: 0
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@hadess.net> > --- > 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? > /* > * usb_set_configuration - Makes a particular device setting be current > @@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration) > intf->dev.type = &usb_if_device_type; > intf->dev.groups = usb_interface_groups; > INIT_WORK(&intf->reset_ws, __usb_queue_reset_device); > + INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf); > intf->minor = -1; > device_initialize(&intf->dev); > pm_runtime_no_callbacks(&intf->dev); > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 11b15d7b357a..5f42c5b9d209 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev) > } > EXPORT_SYMBOL_GPL(usb_get_current_frame_number); > > +/** > + * usb_set_wireless_status - sets the wireless_status struct member > + * @dev: the device to modify > + * @status: the new wireless status > + * > + * Set the wireless_status struct member to the new value, and emit > + * sysfs changes as necessary. > + * > + * Returns: 0 on success, -EALREADY if already set. > + */ > +int usb_set_wireless_status(struct usb_interface *iface, > + enum usb_wireless_status status) > +{ > + if (iface->wireless_status == status) > + return -EALREADY; > + > + usb_get_intf(iface); > + iface->wireless_status = status; > + schedule_work(&iface->wireless_status_work); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_set_wireless_status); This routine belongs in message.c, next to __usb_wireless_status_intf(). Alan Stern
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@hadess.net> > > --- > > 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. > > /* > > * usb_set_configuration - Makes a particular device setting be > > current > > @@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device > > *dev, int configuration) > > intf->dev.type = &usb_if_device_type; > > intf->dev.groups = usb_interface_groups; > > INIT_WORK(&intf->reset_ws, > > __usb_queue_reset_device); > > + INIT_WORK(&intf->wireless_status_work, > > __usb_wireless_status_intf); > > intf->minor = -1; > > device_initialize(&intf->dev); > > pm_runtime_no_callbacks(&intf->dev); > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 11b15d7b357a..5f42c5b9d209 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct > > usb_device *dev) > > } > > EXPORT_SYMBOL_GPL(usb_get_current_frame_number); > > > > +/** > > + * usb_set_wireless_status - sets the wireless_status struct > > member > > + * @dev: the device to modify > > + * @status: the new wireless status > > + * > > + * Set the wireless_status struct member to the new value, and > > emit > > + * sysfs changes as necessary. > > + * > > + * Returns: 0 on success, -EALREADY if already set. > > + */ > > +int usb_set_wireless_status(struct usb_interface *iface, > > + enum usb_wireless_status status) > > +{ > > + if (iface->wireless_status == status) > > + return -EALREADY; > > + > > + usb_get_intf(iface); > > + iface->wireless_status = status; > > + schedule_work(&iface->wireless_status_work); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(usb_set_wireless_status); > > This routine belongs in message.c, next to > __usb_wireless_status_intf(). Sure, done locally.
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@hadess.net> > > > --- > > > 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? Alan Stern
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@hadess.net> > > > > --- > > > > 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?
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@hadess.net> > > > > > --- > > > > > 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
On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote: > 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@hadess.net> > > > > > > --- > > > > > > 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. 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. Cheers
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
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
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() */ +} /* * usb_set_configuration - Makes a particular device setting be current @@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration) intf->dev.type = &usb_if_device_type; intf->dev.groups = usb_interface_groups; INIT_WORK(&intf->reset_ws, __usb_queue_reset_device); + INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf); intf->minor = -1; device_initialize(&intf->dev); pm_runtime_no_callbacks(&intf->dev); diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 11b15d7b357a..5f42c5b9d209 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev) } EXPORT_SYMBOL_GPL(usb_get_current_frame_number); +/** + * usb_set_wireless_status - sets the wireless_status struct member + * @dev: the device to modify + * @status: the new wireless status + * + * Set the wireless_status struct member to the new value, and emit + * sysfs changes as necessary. + * + * Returns: 0 on success, -EALREADY if already set. + */ +int usb_set_wireless_status(struct usb_interface *iface, + enum usb_wireless_status status) +{ + if (iface->wireless_status == status) + return -EALREADY; + + usb_get_intf(iface); + iface->wireless_status = status; + schedule_work(&iface->wireless_status_work); + + return 0; +} +EXPORT_SYMBOL_GPL(usb_set_wireless_status); + /*-------------------------------------------------------------------*/ /* * __usb_get_extra_descriptor() finds a descriptor of specific type in the diff --git a/include/linux/usb.h b/include/linux/usb.h index 517ae4b4e333..a48eeec62a66 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -262,6 +262,7 @@ struct usb_interface { unsigned resetting_device:1; /* true: bandwidth alloc after reset */ unsigned authorized:1; /* used for interface authorization */ enum usb_wireless_status wireless_status; + struct work_struct wireless_status_work; struct device dev; /* interface specific device info */ struct device *usb_dev; @@ -897,6 +898,9 @@ static inline int usb_interface_claimed(struct usb_interface *iface) extern void usb_driver_release_interface(struct usb_driver *driver, struct usb_interface *iface); +int usb_set_wireless_status(struct usb_interface *iface, + enum usb_wireless_status status); + const struct usb_device_id *usb_match_id(struct usb_interface *interface, const struct usb_device_id *id); extern int usb_match_one_id(struct usb_interface *interface,
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@hadess.net> --- drivers/usb/core/message.c | 13 +++++++++++++ drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++ include/linux/usb.h | 4 ++++ 3 files changed, 41 insertions(+)