possible circular locking dependency backtrace seen when enabling PREEMPT_RT

possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Guenter Roeck]

Hi,

when enabling PREEMPT_RT together with lock debugging, I see the attached
"possible circular locking dependency" warning when instantiating the pcnet32
Ethernet adapter with qemu.

Note: This is with

lockdep: Remove disable_irq_lockdep().
lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()

applied on the top of the mainline kernel.

Thanks,
Guenter

---
Build reference: v6.14-rc2-3-g59b1a66735ce
Compiler version: x86_64-linux-gcc (GCC) 13.3.0
Qemu version: 9.1.2 (v9.1.2-51-g94825e22f3)
...
Starting network: [   26.638045] pcnet32 0000:00:02.0 eth0: link up
[   26.639386] 
[   26.639429] ======================================================
[   26.639435] WARNING: possible circular locking dependency detected
[   26.639595] 6.14.0-rc2-00003-g59b1a66735ce #1 Tainted: G                 N
[   26.639609] ------------------------------------------------------
[   26.639616] irq/22-eth0/2680 is trying to acquire lock:
[   26.639648] ffff9cb0438dae30 (&lp->lock){+.+.}-{3:3}, at: pcnet32_interrupt+0x2e/0x1e0
[   26.639853] 
[   26.639853] but task is already holding lock:
[   26.639858] ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x94/0x180
[   26.639877] 
[   26.639877] which lock already depends on the new lock.
[   26.639877] 
[   26.639882] 
[   26.639882] the existing dependency chain (in reverse order) is:
[   26.639898] 
[   26.639898] -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
[   26.639950]        rt_spin_lock+0x39/0x170
[   26.639985]        __local_bh_disable_ip+0x94/0x180
[   26.639991]        __netdev_alloc_skb+0x139/0x1c0
[   26.639998]        pcnet32_init_ring+0x18b/0x310
[   26.640003]        pcnet32_open+0x477/0x820
[   26.640008]        __dev_open+0x100/0x200
[   26.640014]        __dev_change_flags+0x217/0x2b0
[   26.640018]        dev_change_flags+0x21/0x60
[   26.640022]        do_setlink.isra.0+0x31b/0x1250
[   26.640027]        rtnl_newlink+0x7eb/0xb10
[   26.640031]        rtnetlink_rcv_msg+0x414/0x670
[   26.640036]        netlink_rcv_skb+0x56/0x100
[   26.640041]        netlink_unicast+0x1a3/0x290
[   26.640046]        netlink_sendmsg+0x203/0x450
[   26.640050]        ____sys_sendmsg+0x301/0x330
[   26.640060]        ___sys_sendmsg+0x95/0xe0
[   26.640066]        __sys_sendmsg+0x88/0xe0
[   26.640070]        do_syscall_64+0xbb/0x1d0
[   26.640076]        entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   26.640094] 
[   26.640094] -> #0 (&lp->lock){+.+.}-{3:3}:
[   26.640102]        __lock_acquire+0x11ef/0x1940
[   26.640109]        lock_acquire.part.0+0x9a/0x220
[   26.640114]        rt_spin_lock+0x39/0x170
[   26.640118]        pcnet32_interrupt+0x2e/0x1e0
[   26.640123]        irq_thread_fn+0x1e/0x60
[   26.640129]        irq_forced_thread_fn+0x2d/0x50
[   26.640134]        irq_thread+0x15d/0x220
[   26.640138]        kthread+0xfe/0x220
[   26.640144]        ret_from_fork+0x2f/0x50
[   26.640151]        ret_from_fork_asm+0x1a/0x30
[   26.640162] 
[   26.640162] other info that might help us debug this:
[   26.640162] 
[   26.640188]  Possible unsafe locking scenario:
[   26.640188] 
[   26.640192]        CPU0                    CPU1
[   26.640196]        ----                    ----
[   26.640203]   lock((softirq_ctrl.lock));
[   26.640217]                                lock(&lp->lock);
[   26.640232]                                lock((softirq_ctrl.lock));
[   26.640251]   lock(&lp->lock);
[   26.640264] 
[   26.640264]  *** DEADLOCK ***
[   26.640264] 
[   26.640285] 3 locks held by irq/22-eth0/2680:
[   26.640302]  #0: ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x94/0x180
[   26.640327]  #1: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xdf/0x170
[   26.640351]  #2: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip+0xab/0x180
[   26.640370] 
[   26.640370] stack backtrace:
[   26.640464] CPU: 0 UID: 0 PID: 2680 Comm: irq/22-eth0 Tainted: G                 N 6.14.0-rc2-00003-g59b1a66735ce #1
[   26.640507] Tainted: [N]=TEST
[   26.640516] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[   26.640599] Call Trace:
[   26.640640]  <TASK>
[   26.640691]  dump_stack_lvl+0x78/0xe0
[   26.640743]  print_circular_bug+0x26e/0x350
[   26.640752]  check_noncircular+0x169/0x180
[   26.640764]  __lock_acquire+0x11ef/0x1940
[   26.640776]  lock_acquire.part.0+0x9a/0x220
[   26.640787]  ? pcnet32_interrupt+0x2e/0x1e0
[   26.640795]  ? trace_lock_acquire+0x83/0xb0
[   26.640800]  ? lock_acquire+0x33/0x80
[   26.640805]  ? pcnet32_interrupt+0x2e/0x1e0
[   26.640813]  rt_spin_lock+0x39/0x170
[   26.640818]  ? pcnet32_interrupt+0x2e/0x1e0
[   26.640823]  ? trace_lock_acquire+0x83/0xb0
[   26.640829]  pcnet32_interrupt+0x2e/0x1e0
[   26.640835]  ? irq_forced_thread_fn+0x1a/0x50
[   26.640841]  irq_thread_fn+0x1e/0x60
[   26.640846]  ? irq_forced_thread_fn+0x1a/0x50
[   26.640850]  irq_forced_thread_fn+0x2d/0x50
[   26.640855]  ? irq_thread+0xad/0x220
[   26.640859]  irq_thread+0x15d/0x220
[   26.640864]  ? __pfx_irq_forced_thread_fn+0x10/0x10
[   26.640869]  ? __pfx_irq_thread_dtor+0x10/0x10
[   26.640876]  ? __pfx_irq_thread+0x10/0x10
[   26.640881]  kthread+0xfe/0x220
[   26.640889]  ? __pfx_kthread+0x10/0x10
[   26.640896]  ret_from_fork+0x2f/0x50
[   26.640902]  ? __pfx_kthread+0x10/0x10
[   26.640908]  ret_from_fork_asm+0x1a/0x30
[   26.640935]  </TASK>

Stack decode:

[   26.639435] WARNING: possible circular locking dependency detected
[   26.639595] 6.14.0-rc2-00003-g59b1a66735ce #1 Tainted: G                 N
[   26.639609] ------------------------------------------------------
[   26.639616] irq/22-eth0/2680 is trying to acquire lock:
[   26.639648] ffff9cb0438dae30 (&lp->lock){+.+.}-{3:3}, at: pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.639853]
[   26.639853] but task is already holding lock:
[   26.639858] ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
[   26.639877]
[   26.639877] which lock already depends on the new lock.
[   26.639877]
[   26.639882]
[   26.639882] the existing dependency chain (in reverse order) is:
[   26.639898]
[   26.639898] -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
[   26.639950] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
[   26.639985] __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
[   26.639991] __netdev_alloc_skb (net/core/skbuff.c:759) 
[   26.639998] pcnet32_init_ring (drivers/net/ethernet/amd/pcnet32.c:2376 (discriminator 1)) 
[   26.640003] pcnet32_open (drivers/net/ethernet/amd/pcnet32.c:2269) 
[   26.640008] __dev_open (net/core/dev.c:1606) 
[   26.640014] __dev_change_flags (net/core/dev.c:9172) 
[   26.640018] dev_change_flags (net/core/dev.c:9244) 
[   26.640022] do_setlink.isra.0 (net/core/rtnetlink.c:3118 (discriminator 1)) 
[   26.640027] rtnl_newlink (net/core/rtnetlink.c:3732 net/core/rtnetlink.c:3884 net/core/rtnetlink.c:4021) 
[   26.640031] rtnetlink_rcv_msg (net/core/rtnetlink.c:6911) 
[   26.640036] netlink_rcv_skb (net/netlink/af_netlink.c:2543) 
[   26.640041] netlink_unicast (net/netlink/af_netlink.c:1323 net/netlink/af_netlink.c:1348) 
[   26.640046] netlink_sendmsg (net/netlink/af_netlink.c:1892) 
[   26.640050] ____sys_sendmsg (net/socket.c:718 (discriminator 1) net/socket.c:733 (discriminator 1) net/socket.c:2573 (discriminator 1)) 
[   26.640060] ___sys_sendmsg (net/socket.c:2629) 
[   26.640066] __sys_sendmsg (net/socket.c:2659 (discriminator 1)) 
[   26.640070] do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1)) 
[   26.640076] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
[   26.640094]
[   26.640094] -> #0 (&lp->lock){+.+.}-{3:3}:
[   26.640102] __lock_acquire (kernel/locking/lockdep.c:3164 (discriminator 1) kernel/locking/lockdep.c:3282 (discriminator 1) kernel/locking/lockdep.c:3906 (discriminator 1) kernel/locking/lockdep.c:5228 (discriminator 1)) 
[   26.640109] lock_acquire.part.0 (kernel/locking/lockdep.c:469 (discriminator 4) kernel/locking/lockdep.c:5853 (discriminator 4)) 
[   26.640114] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
[   26.640118] pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.640123] irq_thread_fn (kernel/irq/manage.c:1191) 
[   26.640129] irq_forced_thread_fn (include/linux/bottom_half.h:33 (discriminator 1) kernel/irq/manage.c:1216 (discriminator 1)) 
[   26.640134] irq_thread (kernel/irq/manage.c:1318) 
[   26.640138] kthread (kernel/kthread.c:464) 
[   26.640144] ret_from_fork (arch/x86/kernel/process.c:154) 
[   26.640151] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
[   26.640162]
[   26.640162] other info that might help us debug this:
[   26.640162]
[   26.640188]  Possible unsafe locking scenario:
[   26.640188]
[   26.640192]        CPU0                    CPU1
[   26.640196]        ----                    ----
[   26.640203]   lock((softirq_ctrl.lock));
[   26.640217]                                lock(&lp->lock);
[   26.640232]                                lock((softirq_ctrl.lock));
[   26.640251]   lock(&lp->lock);
[   26.640264]
[   26.640264]  *** DEADLOCK ***
[   26.640264]
[   26.640285] 3 locks held by irq/22-eth0/2680:
[   26.640302] #0: ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
[   26.640327] #1: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate.h:849 (discriminator 1) kernel/locking/spinlock_rt.c:50 (discriminator 1) kernel/locking/spinlock_rt.c:57 (discriminator 1)) 
[   26.640351] #2: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate.h:849 (discriminator 1) kernel/softirq.c:156 (discriminator 1)) 
[   26.640370]
[   26.640370] stack backtrace:
[   26.640464] CPU: 0 UID: 0 PID: 2680 Comm: irq/22-eth0 Tainted: G                 N 6.14.0-rc2-00003-g59b1a66735ce #1
[   26.640507] Tainted: [N]=TEST
[   26.640516] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[   26.640599] Call Trace:
[   26.640640]  <TASK>
[   26.640691] dump_stack_lvl (lib/dump_stack.c:123) 
[   26.640743] print_circular_bug (kernel/locking/lockdep.c:2079) 
[   26.640752] check_noncircular (kernel/locking/lockdep.c:2211) 
[   26.640764] __lock_acquire (kernel/locking/lockdep.c:3164 (discriminator 1) kernel/locking/lockdep.c:3282 (discriminator 1) kernel/locking/lockdep.c:3906 (discriminator 1) kernel/locking/lockdep.c:5228 (discriminator 1)) 
[   26.640776] lock_acquire.part.0 (kernel/locking/lockdep.c:469 (discriminator 4) kernel/locking/lockdep.c:5853 (discriminator 4)) 
[   26.640787] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.640795] ? trace_lock_acquire (include/trace/events/lock.h:24 (discriminator 2)) 
[   26.640800] ? lock_acquire (kernel/locking/lockdep.c:5824) 
[   26.640805] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.640813] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
[   26.640818] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.640823] ? trace_lock_acquire (include/trace/events/lock.h:24 (discriminator 2)) 
[   26.640829] pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
[   26.640835] ? irq_forced_thread_fn (include/linux/bottom_half.h:20 (discriminator 1) kernel/irq/manage.c:1210 (discriminator 1)) 
[   26.640841] irq_thread_fn (kernel/irq/manage.c:1191) 
[   26.640846] ? irq_forced_thread_fn (include/linux/bottom_half.h:20 (discriminator 1) kernel/irq/manage.c:1210 (discriminator 1)) 
[   26.640850] irq_forced_thread_fn (include/linux/bottom_half.h:33 (discriminator 1) kernel/irq/manage.c:1216 (discriminator 1)) 
[   26.640855] ? irq_thread (kernel/irq/manage.c:1307) 
[   26.640859] irq_thread (kernel/irq/manage.c:1318) 
[   26.640864] ? __pfx_irq_forced_thread_fn (kernel/irq/manage.c:1207) 
[   26.640869] ? __pfx_irq_thread_dtor (kernel/irq/manage.c:1227) 
[   26.640876] ? __pfx_irq_thread (kernel/irq/manage.c:1294) 
[   26.640881] kthread (kernel/kthread.c:464) 
[   26.640889] ? __pfx_kthread (kernel/kthread.c:413) 
[   26.640896] ret_from_fork (arch/x86/kernel/process.c:154) 
[   26.640902] ? __pfx_kthread (kernel/kthread.c:413) 
[   26.640908] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
[   26.640935]  </TASK>

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 07:56:44 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
> 
> when enabling PREEMPT_RT together with lock debugging, I see the attached
> "possible circular locking dependency" warning when instantiating the pcnet32
> Ethernet adapter with qemu.
> 
> Note: This is with
> 
> lockdep: Remove disable_irq_lockdep().
> lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()
> 
> applied on the top of the mainline kernel.

Thanks for the report.

> 
> Thanks,
> Guenter
> 
> ---
> Build reference: v6.14-rc2-3-g59b1a66735ce
> Compiler version: x86_64-linux-gcc (GCC) 13.3.0
> Qemu version: 9.1.2 (v9.1.2-51-g94825e22f3)
> ...
> Starting network: [   26.638045] pcnet32 0000:00:02.0 eth0: link up
> [   26.639386] 
> [   26.639429] ======================================================
> [   26.639435] WARNING: possible circular locking dependency detected
> [   26.639595] 6.14.0-rc2-00003-g59b1a66735ce #1 Tainted: G                 N
> [   26.639609] ------------------------------------------------------
> [   26.639616] irq/22-eth0/2680 is trying to acquire lock:
> [   26.639648] ffff9cb0438dae30 (&lp->lock){+.+.}-{3:3}, at: pcnet32_interrupt+0x2e/0x1e0

So the spin_lock(lp->lock) is being taken from the interrupt handler (now a
thread), while it already holds the softirq_ctrl.lock from the
local_bh_disable() in the irq_thread_fn().

> [   26.639853] 
> [   26.639853] but task is already holding lock:
> [   26.639858] ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x94/0x180
> [   26.639877] 
> [   26.639877] which lock already depends on the new lock.
> [   26.639877] 
> [   26.639882] 
> [   26.639882] the existing dependency chain (in reverse order) is:
> [   26.639898] 
> [   26.639898] -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
> [   26.639950]        rt_spin_lock+0x39/0x170
> [   26.639985]        __local_bh_disable_ip+0x94/0x180
> [   26.639991]        __netdev_alloc_skb+0x139/0x1c0

The __netdev_alloc_skb() calls local_bh_disable() (which takes the
softirq_ctrl.lock in PREEMPT_RT).

> [   26.639998]        pcnet32_init_ring+0x18b/0x310
> [   26.640003]        pcnet32_open+0x477/0x820

But here it already acquired the lp->lock via a spin_lock_irqsave().

Now, local_bh_disable() in non PREEMPT_RT disables preemption as well as
softirqs. But in PREEMPT_RT it does not disable preemption and has the
following code from __local_bh_disable_ip() in softirq.c:

	/* First entry of a task into a BH disabled section? */
	if (!current->softirq_disable_cnt) {
		if (preemptible()) {
			local_lock(&softirq_ctrl.lock);
			/* Required to meet the RCU bottomhalf requirements. */
			rcu_read_lock();
		} else {
			DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt));
		}
	}

Where that local_lock() disables preemption and prevents softirqs from
executing on this CPU.

But in non PREEMPT_RT, spin_lock_irqsave() (as well as spin_lock_irq())
disables preemption and because it also disables interrupts will disable
softirqs too.

Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?

Like this patch (untested):

diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
index f6499c37157d..50ad0fab66b6 100644
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -90,6 +90,7 @@ static __always_inline void spin_lock_bh(spinlock_t *lock)
 
 static __always_inline void spin_lock_irq(spinlock_t *lock)
 {
+	local_bh_disable();
 	rt_spin_lock(lock);
 }
 
@@ -97,12 +98,14 @@ static __always_inline void spin_lock_irq(spinlock_t *lock)
 	do {						 \
 		typecheck(unsigned long, flags);	 \
 		flags = 0;				 \
+		local_bh_disable();			 \
 		spin_lock(lock);			 \
 	} while (0)
 
 static __always_inline void spin_unlock(spinlock_t *lock)
 {
 	rt_spin_unlock(lock);
+	local_bh_enable();
 }
 
 static __always_inline void spin_unlock_bh(spinlock_t *lock)
@@ -114,6 +117,7 @@ static __always_inline void spin_unlock_bh(spinlock_t *lock)
 static __always_inline void spin_unlock_irq(spinlock_t *lock)
 {
 	rt_spin_unlock(lock);
+	local_bh_enable();
 }
 
 static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,


We could probably consolidate the spin_lock_bh() with spin_lock_irq()
variants.

Guenter,

Can you test that patch to see if it stops this lockdep splat?

Thanks,

-- Steve


> [   26.640008]        __dev_open+0x100/0x200
> [   26.640014]        __dev_change_flags+0x217/0x2b0
> [   26.640018]        dev_change_flags+0x21/0x60
> [   26.640022]        do_setlink.isra.0+0x31b/0x1250
> [   26.640027]        rtnl_newlink+0x7eb/0xb10
> [   26.640031]        rtnetlink_rcv_msg+0x414/0x670
> [   26.640036]        netlink_rcv_skb+0x56/0x100
> [   26.640041]        netlink_unicast+0x1a3/0x290
> [   26.640046]        netlink_sendmsg+0x203/0x450
> [   26.640050]        ____sys_sendmsg+0x301/0x330
> [   26.640060]        ___sys_sendmsg+0x95/0xe0
> [   26.640066]        __sys_sendmsg+0x88/0xe0
> [   26.640070]        do_syscall_64+0xbb/0x1d0
> [   26.640076]        entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   26.640094] 
> [   26.640094] -> #0 (&lp->lock){+.+.}-{3:3}:
> [   26.640102]        __lock_acquire+0x11ef/0x1940
> [   26.640109]        lock_acquire.part.0+0x9a/0x220
> [   26.640114]        rt_spin_lock+0x39/0x170
> [   26.640118]        pcnet32_interrupt+0x2e/0x1e0
> [   26.640123]        irq_thread_fn+0x1e/0x60
> [   26.640129]        irq_forced_thread_fn+0x2d/0x50
> [   26.640134]        irq_thread+0x15d/0x220
> [   26.640138]        kthread+0xfe/0x220
> [   26.640144]        ret_from_fork+0x2f/0x50
> [   26.640151]        ret_from_fork_asm+0x1a/0x30
> [   26.640162] 
> [   26.640162] other info that might help us debug this:
> [   26.640162] 
> [   26.640188]  Possible unsafe locking scenario:
> [   26.640188] 
> [   26.640192]        CPU0                    CPU1
> [   26.640196]        ----                    ----
> [   26.640203]   lock((softirq_ctrl.lock));
> [   26.640217]                                lock(&lp->lock);
> [   26.640232]                                lock((softirq_ctrl.lock));
> [   26.640251]   lock(&lp->lock);
> [   26.640264] 
> [   26.640264]  *** DEADLOCK ***
> [   26.640264] 
> [   26.640285] 3 locks held by irq/22-eth0/2680:
> [   26.640302]  #0: ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x94/0x180
> [   26.640327]  #1: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xdf/0x170
> [   26.640351]  #2: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip+0xab/0x180
> [   26.640370] 
> [   26.640370] stack backtrace:
> [   26.640464] CPU: 0 UID: 0 PID: 2680 Comm: irq/22-eth0 Tainted: G                 N 6.14.0-rc2-00003-g59b1a66735ce #1
> [   26.640507] Tainted: [N]=TEST
> [   26.640516] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [   26.640599] Call Trace:
> [   26.640640]  <TASK>
> [   26.640691]  dump_stack_lvl+0x78/0xe0
> [   26.640743]  print_circular_bug+0x26e/0x350
> [   26.640752]  check_noncircular+0x169/0x180
> [   26.640764]  __lock_acquire+0x11ef/0x1940
> [   26.640776]  lock_acquire.part.0+0x9a/0x220
> [   26.640787]  ? pcnet32_interrupt+0x2e/0x1e0
> [   26.640795]  ? trace_lock_acquire+0x83/0xb0
> [   26.640800]  ? lock_acquire+0x33/0x80
> [   26.640805]  ? pcnet32_interrupt+0x2e/0x1e0
> [   26.640813]  rt_spin_lock+0x39/0x170
> [   26.640818]  ? pcnet32_interrupt+0x2e/0x1e0
> [   26.640823]  ? trace_lock_acquire+0x83/0xb0
> [   26.640829]  pcnet32_interrupt+0x2e/0x1e0
> [   26.640835]  ? irq_forced_thread_fn+0x1a/0x50
> [   26.640841]  irq_thread_fn+0x1e/0x60
> [   26.640846]  ? irq_forced_thread_fn+0x1a/0x50
> [   26.640850]  irq_forced_thread_fn+0x2d/0x50
> [   26.640855]  ? irq_thread+0xad/0x220
> [   26.640859]  irq_thread+0x15d/0x220
> [   26.640864]  ? __pfx_irq_forced_thread_fn+0x10/0x10
> [   26.640869]  ? __pfx_irq_thread_dtor+0x10/0x10
> [   26.640876]  ? __pfx_irq_thread+0x10/0x10
> [   26.640881]  kthread+0xfe/0x220
> [   26.640889]  ? __pfx_kthread+0x10/0x10
> [   26.640896]  ret_from_fork+0x2f/0x50
> [   26.640902]  ? __pfx_kthread+0x10/0x10
> [   26.640908]  ret_from_fork_asm+0x1a/0x30
> [   26.640935]  </TASK>
> 
> Stack decode:
> 
> [   26.639435] WARNING: possible circular locking dependency detected
> [   26.639595] 6.14.0-rc2-00003-g59b1a66735ce #1 Tainted: G                 N
> [   26.639609] ------------------------------------------------------
> [   26.639616] irq/22-eth0/2680 is trying to acquire lock:
> [   26.639648] ffff9cb0438dae30 (&lp->lock){+.+.}-{3:3}, at: pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.639853]
> [   26.639853] but task is already holding lock:
> [   26.639858] ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
> [   26.639877]
> [   26.639877] which lock already depends on the new lock.
> [   26.639877]
> [   26.639882]
> [   26.639882] the existing dependency chain (in reverse order) is:
> [   26.639898]
> [   26.639898] -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
> [   26.639950] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
> [   26.639985] __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
> [   26.639991] __netdev_alloc_skb (net/core/skbuff.c:759) 
> [   26.639998] pcnet32_init_ring (drivers/net/ethernet/amd/pcnet32.c:2376 (discriminator 1)) 
> [   26.640003] pcnet32_open (drivers/net/ethernet/amd/pcnet32.c:2269) 
> [   26.640008] __dev_open (net/core/dev.c:1606) 
> [   26.640014] __dev_change_flags (net/core/dev.c:9172) 
> [   26.640018] dev_change_flags (net/core/dev.c:9244) 
> [   26.640022] do_setlink.isra.0 (net/core/rtnetlink.c:3118 (discriminator 1)) 
> [   26.640027] rtnl_newlink (net/core/rtnetlink.c:3732 net/core/rtnetlink.c:3884 net/core/rtnetlink.c:4021) 
> [   26.640031] rtnetlink_rcv_msg (net/core/rtnetlink.c:6911) 
> [   26.640036] netlink_rcv_skb (net/netlink/af_netlink.c:2543) 
> [   26.640041] netlink_unicast (net/netlink/af_netlink.c:1323 net/netlink/af_netlink.c:1348) 
> [   26.640046] netlink_sendmsg (net/netlink/af_netlink.c:1892) 
> [   26.640050] ____sys_sendmsg (net/socket.c:718 (discriminator 1) net/socket.c:733 (discriminator 1) net/socket.c:2573 (discriminator 1)) 
> [   26.640060] ___sys_sendmsg (net/socket.c:2629) 
> [   26.640066] __sys_sendmsg (net/socket.c:2659 (discriminator 1)) 
> [   26.640070] do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1)) 
> [   26.640076] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
> [   26.640094]
> [   26.640094] -> #0 (&lp->lock){+.+.}-{3:3}:
> [   26.640102] __lock_acquire (kernel/locking/lockdep.c:3164 (discriminator 1) kernel/locking/lockdep.c:3282 (discriminator 1) kernel/locking/lockdep.c:3906 (discriminator 1) kernel/locking/lockdep.c:5228 (discriminator 1)) 
> [   26.640109] lock_acquire.part.0 (kernel/locking/lockdep.c:469 (discriminator 4) kernel/locking/lockdep.c:5853 (discriminator 4)) 
> [   26.640114] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
> [   26.640118] pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.640123] irq_thread_fn (kernel/irq/manage.c:1191) 
> [   26.640129] irq_forced_thread_fn (include/linux/bottom_half.h:33 (discriminator 1) kernel/irq/manage.c:1216 (discriminator 1)) 
> [   26.640134] irq_thread (kernel/irq/manage.c:1318) 
> [   26.640138] kthread (kernel/kthread.c:464) 
> [   26.640144] ret_from_fork (arch/x86/kernel/process.c:154) 
> [   26.640151] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
> [   26.640162]
> [   26.640162] other info that might help us debug this:
> [   26.640162]
> [   26.640188]  Possible unsafe locking scenario:
> [   26.640188]
> [   26.640192]        CPU0                    CPU1
> [   26.640196]        ----                    ----
> [   26.640203]   lock((softirq_ctrl.lock));
> [   26.640217]                                lock(&lp->lock);
> [   26.640232]                                lock((softirq_ctrl.lock));
> [   26.640251]   lock(&lp->lock);
> [   26.640264]
> [   26.640264]  *** DEADLOCK ***
> [   26.640264]
> [   26.640285] 3 locks held by irq/22-eth0/2680:
> [   26.640302] #0: ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:847 kernel/softirq.c:156) 
> [   26.640327] #1: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate.h:849 (discriminator 1) kernel/locking/spinlock_rt.c:50 (discriminator 1) kernel/locking/spinlock_rt.c:57 (discriminator 1)) 
> [   26.640351] #2: ffffffff97406f20 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip (include/linux/rcupdate.h:337 (discriminator 1) include/linux/rcupdate.h:849 (discriminator 1) kernel/softirq.c:156 (discriminator 1)) 
> [   26.640370]
> [   26.640370] stack backtrace:
> [   26.640464] CPU: 0 UID: 0 PID: 2680 Comm: irq/22-eth0 Tainted: G                 N 6.14.0-rc2-00003-g59b1a66735ce #1
> [   26.640507] Tainted: [N]=TEST
> [   26.640516] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [   26.640599] Call Trace:
> [   26.640640]  <TASK>
> [   26.640691] dump_stack_lvl (lib/dump_stack.c:123) 
> [   26.640743] print_circular_bug (kernel/locking/lockdep.c:2079) 
> [   26.640752] check_noncircular (kernel/locking/lockdep.c:2211) 
> [   26.640764] __lock_acquire (kernel/locking/lockdep.c:3164 (discriminator 1) kernel/locking/lockdep.c:3282 (discriminator 1) kernel/locking/lockdep.c:3906 (discriminator 1) kernel/locking/lockdep.c:5228 (discriminator 1)) 
> [   26.640776] lock_acquire.part.0 (kernel/locking/lockdep.c:469 (discriminator 4) kernel/locking/lockdep.c:5853 (discriminator 4)) 
> [   26.640787] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.640795] ? trace_lock_acquire (include/trace/events/lock.h:24 (discriminator 2)) 
> [   26.640800] ? lock_acquire (kernel/locking/lockdep.c:5824) 
> [   26.640805] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.640813] rt_spin_lock (arch/x86/include/asm/current.h:47 kernel/locking/spinlock_rt.c:48 kernel/locking/spinlock_rt.c:57) 
> [   26.640818] ? pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.640823] ? trace_lock_acquire (include/trace/events/lock.h:24 (discriminator 2)) 
> [   26.640829] pcnet32_interrupt (drivers/net/ethernet/amd/pcnet32.c:2572) 
> [   26.640835] ? irq_forced_thread_fn (include/linux/bottom_half.h:20 (discriminator 1) kernel/irq/manage.c:1210 (discriminator 1)) 
> [   26.640841] irq_thread_fn (kernel/irq/manage.c:1191) 
> [   26.640846] ? irq_forced_thread_fn (include/linux/bottom_half.h:20 (discriminator 1) kernel/irq/manage.c:1210 (discriminator 1)) 
> [   26.640850] irq_forced_thread_fn (include/linux/bottom_half.h:33 (discriminator 1) kernel/irq/manage.c:1216 (discriminator 1)) 
> [   26.640855] ? irq_thread (kernel/irq/manage.c:1307) 
> [   26.640859] irq_thread (kernel/irq/manage.c:1318) 
> [   26.640864] ? __pfx_irq_forced_thread_fn (kernel/irq/manage.c:1207) 
> [   26.640869] ? __pfx_irq_thread_dtor (kernel/irq/manage.c:1227) 
> [   26.640876] ? __pfx_irq_thread (kernel/irq/manage.c:1294) 
> [   26.640881] kthread (kernel/kthread.c:464) 
> [   26.640889] ? __pfx_kthread (kernel/kthread.c:413) 
> [   26.640896] ret_from_fork (arch/x86/kernel/process.c:154) 
> [   26.640902] ? __pfx_kthread (kernel/kthread.c:413) 
> [   26.640908] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
> [   26.640935]  </TASK>

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 07:56:44 [-0800], Guenter Roeck wrote:
> Hi,
Hi,

> when enabling PREEMPT_RT together with lock debugging, I see the attached
> "possible circular locking dependency" warning when instantiating the pcnet32
> Ethernet adapter with qemu.

I may buy you a virtio adapter :)

What about this in the meantime:

diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
index c6bd803f5b0c1..c7648045917a6 100644
--- a/drivers/net/ethernet/amd/pcnet32.c
+++ b/drivers/net/ethernet/amd/pcnet32.c
@@ -754,6 +754,7 @@ static int pcnet32_set_link_ksettings(struct net_device *dev,
 	int r = -EOPNOTSUPP;
 	int suspended, bcr2, bcr9, csr15;
 
+	local_bh_disable();
 	spin_lock_irqsave(&lp->lock, flags);
 	if (lp->mii) {
 		r = mii_ethtool_set_link_ksettings(&lp->mii_if, cmd);
@@ -789,6 +790,7 @@ static int pcnet32_set_link_ksettings(struct net_device *dev,
 		r = 0;
 	}
 	spin_unlock_irqrestore(&lp->lock, flags);
+	local_bh_enable();
 	return r;
 }
 
@@ -890,6 +892,7 @@ static int pcnet32_set_ringparam(struct net_device *dev,
 		pcnet32_netif_stop(dev);
 
 	netdev_lock(dev);
+	local_bh_disable();
 	spin_lock_irqsave(&lp->lock, flags);
 	lp->a->write_csr(ioaddr, CSR0, CSR0_STOP);	/* stop the chip */
 
@@ -921,6 +924,7 @@ static int pcnet32_set_ringparam(struct net_device *dev,
 	}
 
 	spin_unlock_irqrestore(&lp->lock, flags);
+	local_bh_enable();
 	netdev_unlock(dev);
 
 	netif_info(lp, drv, dev, "Ring Param Settings: RX: %d, TX: %d\n",
@@ -988,6 +992,7 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
 		pcnet32_netif_stop(dev);
 
 	netdev_lock(dev);
+	local_bh_disable();
 	spin_lock_irqsave(&lp->lock, flags);
 	lp->a->write_csr(ioaddr, CSR0, CSR0_STOP);	/* stop the chip */
 
@@ -1065,7 +1070,9 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
 		rmb();
 		while ((lp->rx_ring[x].status & teststatus) && (ticks < 200)) {
 			spin_unlock_irqrestore(&lp->lock, flags);
+			local_bh_enable();
 			msleep(1);
+			local_bh_disable();
 			spin_lock_irqsave(&lp->lock, flags);
 			rmb();
 			ticks++;
@@ -1125,6 +1132,7 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
 		lp->a->write_bcr(ioaddr, 20, 4);	/* return to 16bit mode */
 	}
 	spin_unlock_irqrestore(&lp->lock, flags);
+	local_bh_enable();
 	netdev_unlock(dev);
 
 	return rc;
@@ -2106,6 +2114,7 @@ static int pcnet32_open(struct net_device *dev)
 	}
 
 	netdev_lock(dev);
+	local_bh_disable();
 	spin_lock_irqsave(&lp->lock, flags);
 	/* Check for a valid station address */
 	if (!is_valid_ether_addr(dev->dev_addr)) {
@@ -2305,6 +2314,7 @@ static int pcnet32_open(struct net_device *dev)
 		     lp->a->read_csr(ioaddr, CSR0));
 
 	spin_unlock_irqrestore(&lp->lock, flags);
+	local_bh_enable();
 	netdev_unlock(dev);
 
 	return 0;		/* Always succeed */
@@ -2321,6 +2331,7 @@ static int pcnet32_open(struct net_device *dev)
 
 err_free_irq:
 	spin_unlock_irqrestore(&lp->lock, flags);
+	local_bh_enable();
 	netdev_unlock(dev);
 	free_irq(dev->irq, dev);
 	return rc;

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 11:45:31 [-0500], Steven Rostedt wrote:
> 
> Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?

Better not. The consequences sure wouldn't be funny.

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 17:59:08 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2025-02-12 11:45:31 [-0500], Steven Rostedt wrote:
> > 
> > Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?  
> 
> Better not. The consequences sure wouldn't be funny.
> 

Why not? It keeps the same paradigm as non PREEMPT_RT. Do we really want to
litter local_bh_disable around all these cases that spin_lock_irq*() is
used to disable bh?

-- Steve

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 12:03:28 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 12 Feb 2025 17:59:08 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > On 2025-02-12 11:45:31 [-0500], Steven Rostedt wrote:  
> > > 
> > > Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?    
> > 
> > Better not. The consequences sure wouldn't be funny.
> >   
> 
> Why not? It keeps the same paradigm as non PREEMPT_RT. Do we really want to
> litter local_bh_disable around all these cases that spin_lock_irq*() is
> used to disable bh?
> 

Expanding on my argument. Why does it work for non-PREEMPT_RT but not for
PREEMPT_RT?

-- Steve

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 12:03:28 [-0500], Steven Rostedt wrote:
> On Wed, 12 Feb 2025 17:59:08 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > On 2025-02-12 11:45:31 [-0500], Steven Rostedt wrote:
> > > 
> > > Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?  
> > 
> > Better not. The consequences sure wouldn't be funny.
> > 
> 
> Why not? It keeps the same paradigm as non PREEMPT_RT. Do we really want to
> litter local_bh_disable around all these cases that spin_lock_irq*() is
> used to disable bh?

Yes.

> -- Steve

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 18:09:55 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> > Why not? It keeps the same paradigm as non PREEMPT_RT. Do we really want to
> > litter local_bh_disable around all these cases that spin_lock_irq*() is
> > used to disable bh?  
> 
> Yes.

OK, after testing that patch I see the issue. The local_bh_disable() makes
it look like we are in a softirq, and that triggers a bunch of
BUG(in_interrupt()) that are around the kernel.

Couldn't we make the spin_lock_irq*() also disable softirqs without making
softirq_count() return true?

That would keep the same paradigm between non-PREEMPT_RT as well as
PREEMPT_RT without having to litter these all over the place.

-- Steve

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 12:07:06 [-0500], Steven Rostedt wrote:
> 
> Expanding on my argument. Why does it work for non-PREEMPT_RT but not for
> PREEMPT_RT?

The semantics are different. It is not a race on !RT but *might* happen
on RT.
The other alternative would be to add additional locking into
pcnet32_access() and avoid having the lock in the ISR.
But this driver/ hardware is old and probably hardly used outside of
qemu. We did not have any reports about this recently. We had a few
reports within the net stack but this is gone now.

So. Adding your suggestion into the core code just to satisfy one driver
is not something I had in mind.

> -- Steve

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 12:18:37 [-0500], Steven Rostedt wrote:
> On Wed, 12 Feb 2025 18:09:55 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > > Why not? It keeps the same paradigm as non PREEMPT_RT. Do we really want to
> > > litter local_bh_disable around all these cases that spin_lock_irq*() is
> > > used to disable bh?  
> > 
> > Yes.
> 
> OK, after testing that patch I see the issue. The local_bh_disable() makes
> it look like we are in a softirq, and that triggers a bunch of
> BUG(in_interrupt()) that are around the kernel.
> 
> Couldn't we make the spin_lock_irq*() also disable softirqs without making
> softirq_count() return true?

Maybe. But I don't see the advantage. See my other reply.

> That would keep the same paradigm between non-PREEMPT_RT as well as
> PREEMPT_RT without having to litter these all over the place.

I am working on getting rid of the lock in local_bh_disable(). Guess
what happens then in terms of "same paradigm".

> -- Steve

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 18:24:17 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> > That would keep the same paradigm between non-PREEMPT_RT as well as
> > PREEMPT_RT without having to litter these all over the place.  
> 
> I am working on getting rid of the lock in local_bh_disable(). Guess
> what happens then in terms of "same paradigm".

No idea.

When you disable interrupts in non PREEMPT_RT, it also disables the softirqs for
that CPU. Thus a spin_lock_irq*() by definition will disable the softirqs too.

I guess it also comes down to how many places this happens. If it's not
that common, then perhaps this is the simplest solution. I just don't want
this to become the new cond_resched(), where we have to sprinkle
local_bh_disable() around to solve issues that are reported here and there.

-- Steve

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Guenter Roeck]

On 2/12/25 08:45, Steven Rostedt wrote:
> On Wed, 12 Feb 2025 07:56:44 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> Hi,
>>
>> when enabling PREEMPT_RT together with lock debugging, I see the attached
>> "possible circular locking dependency" warning when instantiating the pcnet32
>> Ethernet adapter with qemu.
>>
>> Note: This is with
>>
>> lockdep: Remove disable_irq_lockdep().
>> lockdep: Don't disable interrupts on RT in disable_irq_nosync_lockdep.*()
>>
>> applied on the top of the mainline kernel.
> 
> Thanks for the report.
> 
>>
>> Thanks,
>> Guenter
>>
>> ---
>> Build reference: v6.14-rc2-3-g59b1a66735ce
>> Compiler version: x86_64-linux-gcc (GCC) 13.3.0
>> Qemu version: 9.1.2 (v9.1.2-51-g94825e22f3)
>> ...
>> Starting network: [   26.638045] pcnet32 0000:00:02.0 eth0: link up
>> [   26.639386]
>> [   26.639429] ======================================================
>> [   26.639435] WARNING: possible circular locking dependency detected
>> [   26.639595] 6.14.0-rc2-00003-g59b1a66735ce #1 Tainted: G                 N
>> [   26.639609] ------------------------------------------------------
>> [   26.639616] irq/22-eth0/2680 is trying to acquire lock:
>> [   26.639648] ffff9cb0438dae30 (&lp->lock){+.+.}-{3:3}, at: pcnet32_interrupt+0x2e/0x1e0
> 
> So the spin_lock(lp->lock) is being taken from the interrupt handler (now a
> thread), while it already holds the softirq_ctrl.lock from the
> local_bh_disable() in the irq_thread_fn().
> 
>> [   26.639853]
>> [   26.639853] but task is already holding lock:
>> [   26.639858] ffff9cb0bdc1c818 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x94/0x180
>> [   26.639877]
>> [   26.639877] which lock already depends on the new lock.
>> [   26.639877]
>> [   26.639882]
>> [   26.639882] the existing dependency chain (in reverse order) is:
>> [   26.639898]
>> [   26.639898] -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
>> [   26.639950]        rt_spin_lock+0x39/0x170
>> [   26.639985]        __local_bh_disable_ip+0x94/0x180
>> [   26.639991]        __netdev_alloc_skb+0x139/0x1c0
> 
> The __netdev_alloc_skb() calls local_bh_disable() (which takes the
> softirq_ctrl.lock in PREEMPT_RT).
> 
>> [   26.639998]        pcnet32_init_ring+0x18b/0x310
>> [   26.640003]        pcnet32_open+0x477/0x820
> 
> But here it already acquired the lp->lock via a spin_lock_irqsave().
> 
> Now, local_bh_disable() in non PREEMPT_RT disables preemption as well as
> softirqs. But in PREEMPT_RT it does not disable preemption and has the
> following code from __local_bh_disable_ip() in softirq.c:
> 
> 	/* First entry of a task into a BH disabled section? */
> 	if (!current->softirq_disable_cnt) {
> 		if (preemptible()) {
> 			local_lock(&softirq_ctrl.lock);
> 			/* Required to meet the RCU bottomhalf requirements. */
> 			rcu_read_lock();
> 		} else {
> 			DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt));
> 		}
> 	}
> 
> Where that local_lock() disables preemption and prevents softirqs from
> executing on this CPU.
> 
> But in non PREEMPT_RT, spin_lock_irqsave() (as well as spin_lock_irq())
> disables preemption and because it also disables interrupts will disable
> softirqs too.
> 
> Shouldn't the spin_lock_irq*() variants for PREEMPT_RT also disable local_bh?
> 
> Like this patch (untested):
> 
> diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
> index f6499c37157d..50ad0fab66b6 100644
> --- a/include/linux/spinlock_rt.h
> +++ b/include/linux/spinlock_rt.h
> @@ -90,6 +90,7 @@ static __always_inline void spin_lock_bh(spinlock_t *lock)
>   
>   static __always_inline void spin_lock_irq(spinlock_t *lock)
>   {
> +	local_bh_disable();
>   	rt_spin_lock(lock);
>   }
>   
> @@ -97,12 +98,14 @@ static __always_inline void spin_lock_irq(spinlock_t *lock)
>   	do {						 \
>   		typecheck(unsigned long, flags);	 \
>   		flags = 0;				 \
> +		local_bh_disable();			 \
>   		spin_lock(lock);			 \
>   	} while (0)
>   
>   static __always_inline void spin_unlock(spinlock_t *lock)
>   {
>   	rt_spin_unlock(lock);
> +	local_bh_enable();
>   }
>   
>   static __always_inline void spin_unlock_bh(spinlock_t *lock)
> @@ -114,6 +117,7 @@ static __always_inline void spin_unlock_bh(spinlock_t *lock)
>   static __always_inline void spin_unlock_irq(spinlock_t *lock)
>   {
>   	rt_spin_unlock(lock);
> +	local_bh_enable();
>   }
>   
>   static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,
> 
> 
> We could probably consolidate the spin_lock_bh() with spin_lock_irq()
> variants.
> 
> Guenter,
> 
> Can you test that patch to see if it stops this lockdep splat?
> 

That results in a variety of interesting warning backtraces and crashes :-).

[    0.013997] WARNING: CPU: 0 PID: 0 at kernel/softirq.c:212 __local_bh_enable_ip+0xe2/0x160
...
[    0.014890] Call Trace:
[    0.014975]  <TASK>
[    0.015082]  ? __warn+0x8c/0x190
[    0.015115]  ? __local_bh_enable_ip+0xe2/0x160
[    0.015127]  ? report_bug+0x164/0x190
[    0.015144]  ? early_fixup_exception+0xa4/0xc0
[    0.015158]  ? early_idt_handler_common+0x2f/0x40
[    0.015184]  ? update_page_count+0x3c/0x50
[    0.015192]  ? __local_bh_enable_ip+0xe2/0x160
[    0.015206]  ? phys_pte_init+0xaa/0x1c0
[    0.015225]  ? phys_pmd_init+0x242/0x3d0
[    0.015248]  ? phys_pud_init+0xb4/0x3d0
[    0.015261]  ? phys_pud_init+0x52/0x3d0
[    0.015276]  ? __kernel_physical_mapping_init+0x171/0x380
[    0.015298]  ? init_memory_mapping+0x14e/0x3d0
[    0.015335]  ? init_mem_mapping+0xed/0x450
[    0.015350]  ? setup_arch+0x757/0xd60
[    0.015371]  ? start_kernel+0x6b/0x810
[    0.015391]  ? x86_64_start_reservations+0x18/0x30
[    0.015397]  ? x86_64_start_kernel+0xc5/0xe0
[    0.015406]  ? common_startup_64+0x12c/0x138
[    0.015447]  </TASK>
...
[    0.246710] ------------[ cut here ]------------
[    0.246720] kernel BUG at mm/vmalloc.c:3118!
[    0.247070] Oops: invalid opcode: 0000 [#1] PREEMPT_RT SMP PTI
[    0.247152] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G        W          6.14.0-rc2-00004-g7c65c4480e19 #1
[    0.247175] Tainted: [W]=WARN
[    0.247180] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[    0.247185] RIP: 0010:__get_vm_area_node+0x124/0x140
[    0.247204] Code: ff ff ff 48 0f bd ca ba 0c 00 00 00 83 c1 01 39 d1 0f 4c ca ba 1e 00 00 00 39 d1 0f 4f ca 48 d3 e0 49 89 c6 e9 4a ff ff f
f 90 <0f> 0b 48 89 ef e8 e2 5f 01 00 31 ed eb b0 66 66 2e 0f 1f 84 00 00
[    0.247210] RSP: 0000:ffffffffbdc03df8 EFLAGS: 00010006
[    0.247221] RAX: 000000000000a200 RBX: 0000000000000004 RCX: 0000000000000004
[    0.247225] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000004000
[    0.247227] RBP: 0000000000004000 R08: ffffac1540000000 R09: ffffcc153fffffff
[    0.247230] R10: ffff9459010577c0 R11: 0000000000000000 R12: 0000000000000004
[    0.247233] R13: ffffffffbdc03e88 R14: 0000000000000001 R15: 0000000000014770
[    0.247236] FS:  0000000000000000(0000) GS:ffff94590f800000(0000) knlGS:0000000000000000
[    0.247240] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.247243] CR2: ffff94590f001000 CR3: 000000000d2f6000 CR4: 00000000000000b0
[    0.247247] Call Trace:
[    0.247278]  <TASK>
[    0.247295]  ? die+0x31/0x80
[    0.247304]  ? do_trap+0xd8/0x100
[    0.247312]  ? __get_vm_area_node+0x124/0x140
[    0.247320]  ? __get_vm_area_node+0x124/0x140
[    0.247326]  ? do_error_trap+0x78/0x100
[    0.247333]  ? __get_vm_area_node+0x124/0x140
[    0.247342]  ? exc_invalid_op+0x51/0x70
[    0.247348]  ? __get_vm_area_node+0x124/0x140
[    0.247354]  ? asm_exc_invalid_op+0x1a/0x20
[    0.247374]  ? __get_vm_area_node+0x124/0x140
[    0.247389]  get_vm_area_caller+0x39/0x50
[    0.247406]  ? irq_init_percpu_irqstack+0xe8/0x120
[    0.247414]  vmap+0x64/0xe0
[    0.247423]  irq_init_percpu_irqstack+0xe8/0x120
[    0.247435]  init_IRQ+0x56/0x70
[    0.247446]  start_kernel+0x366/0x810
[    0.247456]  x86_64_start_reservations+0x18/0x30
[    0.247463]  x86_64_start_kernel+0xc5/0xe0
[    0.247470]  common_startup_64+0x12c/0x138
[    0.247489]  </TASK>

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Steven Rostedt]

On Wed, 12 Feb 2025 09:54:20 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> > Can you test that patch to see if it stops this lockdep splat?
> >   
> 
> That results in a variety of interesting warning backtraces and crashes :-).

I said I didn't test it ;-) It also had a bug where I put the
local_bh_enable() in the wrong function. But that doesn't matter, there's
other issues with it.

Could you try Sebastian's version instead.

Thanks,

-- Steve

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Guenter Roeck]

On 2/12/25 08:56, Sebastian Andrzej Siewior wrote:
> On 2025-02-12 07:56:44 [-0800], Guenter Roeck wrote:
>> Hi,
> Hi,
> 
>> when enabling PREEMPT_RT together with lock debugging, I see the attached
>> "possible circular locking dependency" warning when instantiating the pcnet32
>> Ethernet adapter with qemu.
> 
> I may buy you a virtio adapter :)
> 

The whole point of my testing is to execute as much code as possible.
I _do_ test various virtio devices, but to limit testing to virtio
would defeat the purpose.

> What about this in the meantime:

Yes, that passes my tests (with and without rt enabled).

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> 
> diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
> index c6bd803f5b0c1..c7648045917a6 100644
> --- a/drivers/net/ethernet/amd/pcnet32.c
> +++ b/drivers/net/ethernet/amd/pcnet32.c
> @@ -754,6 +754,7 @@ static int pcnet32_set_link_ksettings(struct net_device *dev,
>   	int r = -EOPNOTSUPP;
>   	int suspended, bcr2, bcr9, csr15;
>   
> +	local_bh_disable();
>   	spin_lock_irqsave(&lp->lock, flags);
>   	if (lp->mii) {
>   		r = mii_ethtool_set_link_ksettings(&lp->mii_if, cmd);
> @@ -789,6 +790,7 @@ static int pcnet32_set_link_ksettings(struct net_device *dev,
>   		r = 0;
>   	}
>   	spin_unlock_irqrestore(&lp->lock, flags);
> +	local_bh_enable();
>   	return r;
>   }
>   
> @@ -890,6 +892,7 @@ static int pcnet32_set_ringparam(struct net_device *dev,
>   		pcnet32_netif_stop(dev);
>   
>   	netdev_lock(dev);
> +	local_bh_disable();
>   	spin_lock_irqsave(&lp->lock, flags);
>   	lp->a->write_csr(ioaddr, CSR0, CSR0_STOP);	/* stop the chip */
>   
> @@ -921,6 +924,7 @@ static int pcnet32_set_ringparam(struct net_device *dev,
>   	}
>   
>   	spin_unlock_irqrestore(&lp->lock, flags);
> +	local_bh_enable();
>   	netdev_unlock(dev);
>   
>   	netif_info(lp, drv, dev, "Ring Param Settings: RX: %d, TX: %d\n",
> @@ -988,6 +992,7 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
>   		pcnet32_netif_stop(dev);
>   
>   	netdev_lock(dev);
> +	local_bh_disable();
>   	spin_lock_irqsave(&lp->lock, flags);
>   	lp->a->write_csr(ioaddr, CSR0, CSR0_STOP);	/* stop the chip */
>   
> @@ -1065,7 +1070,9 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
>   		rmb();
>   		while ((lp->rx_ring[x].status & teststatus) && (ticks < 200)) {
>   			spin_unlock_irqrestore(&lp->lock, flags);
> +			local_bh_enable();
>   			msleep(1);
> +			local_bh_disable();
>   			spin_lock_irqsave(&lp->lock, flags);
>   			rmb();
>   			ticks++;
> @@ -1125,6 +1132,7 @@ static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1)
>   		lp->a->write_bcr(ioaddr, 20, 4);	/* return to 16bit mode */
>   	}
>   	spin_unlock_irqrestore(&lp->lock, flags);
> +	local_bh_enable();
>   	netdev_unlock(dev);
>   
>   	return rc;
> @@ -2106,6 +2114,7 @@ static int pcnet32_open(struct net_device *dev)
>   	}
>   
>   	netdev_lock(dev);
> +	local_bh_disable();
>   	spin_lock_irqsave(&lp->lock, flags);
>   	/* Check for a valid station address */
>   	if (!is_valid_ether_addr(dev->dev_addr)) {
> @@ -2305,6 +2314,7 @@ static int pcnet32_open(struct net_device *dev)
>   		     lp->a->read_csr(ioaddr, CSR0));
>   
>   	spin_unlock_irqrestore(&lp->lock, flags);
> +	local_bh_enable();
>   	netdev_unlock(dev);
>   
>   	return 0;		/* Always succeed */
> @@ -2321,6 +2331,7 @@ static int pcnet32_open(struct net_device *dev)
>   
>   err_free_irq:
>   	spin_unlock_irqrestore(&lp->lock, flags);
> +	local_bh_enable();
>   	netdev_unlock(dev);
>   	free_irq(dev->irq, dev);
>   	return rc;
> 
> Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 10:36:06 [-0800], Guenter Roeck wrote:
> > I may buy you a virtio adapter :)
> 
> The whole point of my testing is to execute as much code as possible.
> I _do_ test various virtio devices, but to limit testing to virtio
> would defeat the purpose.

I understand.

> > What about this in the meantime:
> 
> Yes, that passes my tests (with and without rt enabled).
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Thank you. Let me think what I do here…

> Thanks,
> Guenter

Sebastian

Re: possible circular locking dependency backtrace seen when enabling PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-02-12 12:33:02 [-0500], Steven Rostedt wrote:
> I guess it also comes down to how many places this happens. If it's not
> that common, then perhaps this is the simplest solution. I just don't want
> this to become the new cond_resched(), where we have to sprinkle
> local_bh_disable() around to solve issues that are reported here and there.

Don't worry about that. Both, the ne2k and the pcnet driver are _old_. I
had the ne2k as ISA a while ago. Maybe even a PCI later on.
Converting the ne2k to proper NAPI would avoid the problem.
Adding a bus-lock to the pcnet driver would avoid the problem by in the
driver by not having to disable interrupts for the entire NAPI routine.

> -- Steve

Sebastian