Re: [PATCH 4/5] USB: core: Add API to change the wireless_status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux