Re: [Intel-wired-lan] [PATCH iwl-net] idpf: convert control queue mutex to a spinlock

[Simon Horman <horms@kernel.org>]

On Tue, Jun 03, 2025 at 11:37:30AM -0600, Ahmed Zaki wrote:
> 
> 
> On 2025-05-28 3:55 a.m., Simon Horman wrote:
> > On Fri, May 23, 2025 at 02:55:37PM -0600, Ahmed Zaki wrote:
> > > With VIRTCHNL2_CAP_MACFILTER enabled, the following warning is generated
> > > on module load:
> > > 
> > > [  324.701677] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:578
> > > [  324.701684] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1582, name: NetworkManager
> > > [  324.701689] preempt_count: 201, expected: 0
> > > [  324.701693] RCU nest depth: 0, expected: 0
> > > [  324.701697] 2 locks held by NetworkManager/1582:
> > > [  324.701702]  #0: ffffffff9f7be770 (rtnl_mutex){....}-{3:3}, at: rtnl_newlink+0x791/0x21e0
> > > [  324.701730]  #1: ff1100216c380368 (_xmit_ETHER){....}-{2:2}, at: __dev_open+0x3f0/0x870
> > > [  324.701749] Preemption disabled at:
> > > [  324.701752] [<ffffffff9cd23b9d>] __dev_open+0x3dd/0x870
> > > [  324.701765] CPU: 30 UID: 0 PID: 1582 Comm: NetworkManager Not tainted 6.15.0-rc5+ #2 PREEMPT(voluntary)
> > > [  324.701771] Hardware name: Intel Corporation M50FCP2SBSTD/M50FCP2SBSTD, BIOS SE5C741.86B.01.01.0001.2211140926 11/14/2022
> > > [  324.701774] Call Trace:
> > > [  324.701777]  <TASK>
> > > [  324.701779]  dump_stack_lvl+0x5d/0x80
> > > [  324.701788]  ? __dev_open+0x3dd/0x870
> > > [  324.701793]  __might_resched.cold+0x1ef/0x23d
> > > <..>
> > > [  324.701818]  __mutex_lock+0x113/0x1b80
> > > <..>
> > > [  324.701917]  idpf_ctlq_clean_sq+0xad/0x4b0 [idpf]
> > > [  324.701935]  ? kasan_save_track+0x14/0x30
> > > [  324.701941]  idpf_mb_clean+0x143/0x380 [idpf]
> > > <..>
> > > [  324.701991]  idpf_send_mb_msg+0x111/0x720 [idpf]
> > > [  324.702009]  idpf_vc_xn_exec+0x4cc/0x990 [idpf]
> > > [  324.702021]  ? rcu_is_watching+0x12/0xc0
> > > [  324.702035]  idpf_add_del_mac_filters+0x3ed/0xb50 [idpf]
> > > <..>
> > > [  324.702122]  __hw_addr_sync_dev+0x1cf/0x300
> > > [  324.702126]  ? find_held_lock+0x32/0x90
> > > [  324.702134]  idpf_set_rx_mode+0x317/0x390 [idpf]
> > > [  324.702152]  __dev_open+0x3f8/0x870
> > > [  324.702159]  ? __pfx___dev_open+0x10/0x10
> > > [  324.702174]  __dev_change_flags+0x443/0x650
> > > <..>
> > > [  324.702208]  netif_change_flags+0x80/0x160
> > > [  324.702218]  do_setlink.isra.0+0x16a0/0x3960
> > > <..>
> > > [  324.702349]  rtnl_newlink+0x12fd/0x21e0
> > > 
> > > The sequence is as follows:
> > > 	rtnl_newlink()->
> > > 	__dev_change_flags()->
> > > 	__dev_open()->
> > > 	dev_set_rx_mode() - >  # disables BH and grabs "dev->addr_list_lock"
> > > 	idpf_set_rx_mode() ->  # proceed only if VIRTCHNL2_CAP_MACFILTER is ON
> > > 	__dev_uc_sync() ->
> > > 	idpf_add_mac_filter ->
> > > 	idpf_add_del_mac_filters ->
> > > 	idpf_send_mb_msg() ->
> > > 	idpf_mb_clean() ->
> > > 	idpf_ctlq_clean_sq()   # mutex_lock(cq_lock)
> > > 
> > > Fix by converting cq_lock to a spinlock. All operations under the new
> > > lock are safe except freeing the DMA memory, which may use vunmap(). Fix
> > > by requesting a contiguous physical memory for the DMA mapping.
> > 
> > Hi Ahmed,
> 
> Hi Simon, Sorry for the late reply, I was off last week.
> 
> > 
> > If I understand things correctly, then by safe you mean won't sleep.  But
> 
> correct, that is what I meant.
> 
> > if so my question is if the path that frees DMA memory which is updated by
> > this patch is run in a context where sleeping is not allowed.
> 
> I am not sure I understand the question, but the current freeing path runs
> in process context and sleeping is allowed (hence the previous use of
> mutex).
> 
> With the new spinlock, we need to make sure all code in-between the new spin
> lock/unlock cannot sleep. All was safe except DMA buffer freeing which
> called vunmap(). That is avoided in this patch by requesting contiguous DMA
> memory via DMA_ATTR_FORCE_CONTIGUOUS.

Thanks for the clarification. And I agree that this is a good approach.
And sorry for my somewhat nonsensical question earlier, my mind had
gone off on a tangent.

Reviewed-by: Simon Horman <horms@kernel.org>