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@xxxxxxxxxx>
> > ---
> > 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
