Re: [net,PATCH v2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-12 10:51:25 [-0700], Jakub Kicinski wrote:
> > 
> >    rt_spin_lock from ks8851_start_xmit_par+0x68/0x1a0
> >    ks8851_start_xmit_par from netdev_start_xmit+0x1c/0x40 <---- this 
> > tries to grab the same PAR spinlock, and deadlocks
> >    netdev_start_xmit from dev_hard_start_xmit+0xec/0x1b0
> >    dev_hard_start_xmit from sch_direct_xmit+0xb8/0x25c
> >    sch_direct_xmit from __qdisc_run+0x20c/0x4fc
> >    __qdisc_run from qdisc_run+0x1c/0x28
> >    qdisc_run from net_tx_action+0x1f4/0x244
> >    net_tx_action from handle_softirqs+0x1c0/0x29c
> >    handle_softirqs from __local_bh_enable_ip+0xdc/0xf4
> >    __local_bh_enable_ip from __netdev_alloc_skb+0x140/0x194
> >    __netdev_alloc_skb from ks8851_irq+0x348/0x4d8 <---- this is called 
> > from ks8851_rx_pkts() via netdev_alloc_skb_ip_align()
> >    ks8851_irq from irq_thread_fn+0x24/0x64 <-------- this here runs with 
> > the PAR spinlock held
> > 
> > > The patch looks way to "advanced" for a driver. Something is going
> > > very wrong here. Or the commit message must be updated to explain
> > > it better to people like me. Or both.  
> > 
> > Does the backtrace make the problem clearer, with the annotation above ?
> 
> Sebastian, do you have any recommendation here? tl;dr is that the driver does
> 
> 	spin_lock_irqsave()
> 	__netdev_alloc_skb()
> 	spin_unlock_irqrestore()

So that is what happens in the backtrace. But not as of v7.0 if I look
at ks8851_irq():

|         if (status & IRQ_TXI) {
|                 unsigned short tx_space = ks8851_rdreg16(ks, KS_TXMIR);
|
|                 netif_dbg(ks, intr, ks->netdev,
|                           "%s: txspace %d\n", __func__, tx_space);
|
|                 spin_lock_bh(&ks->statelock);
disables bh

|                 ks->tx_space = tx_space;
|                 if (netif_queue_stopped(ks->netdev))
|                         netif_wake_queue(ks->netdev);
wakes queue, raise softirq, net-tx which does the qdisc_run() as seen in
the backtrace

|                 spin_unlock_bh(&ks->statelock);
enables bh and runs it
|         }

So this I understand and it would lead to a similar backtrace.
However this shouldn't occur from __netdev_alloc_skb(). 

> And __netdev_alloc_skb() does:
> 
> 	if (in_hardirq() || irqs_disabled()) {
> 		nc = this_cpu_ptr(&netdev_alloc_cache);
> 		data = page_frag_alloc(nc, len, gfp_mask);
> 		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
> 	} else {
> 		local_bh_disable();
> 		local_lock_nested_bh(&napi_alloc_cache.bh_lock);
> 
> 		nc = this_cpu_ptr(&napi_alloc_cache.page);
> 		data = page_frag_alloc(nc, len, gfp_mask);
> 		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
> 
> 		local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
> 		local_bh_enable();
> 	}
> 
> the local_bh_enable() seems to kick in BH processing inline,
> and BH processing takes the same spin lock the driver is already
> holding.

Yes, it does. But there is nothing between local_bh_disable() and
local_bh_enable() that raises the softirq. Looking at v6.9 there is the
following instead:

|                 spin_lock(&ks->statelock);
|                 ks->tx_space = tx_space;
|                 if (netif_queue_stopped(ks->netdev))
|                         netif_wake_queue(ks->netdev);
|                 spin_unlock(&ks->statelock);

So no _bh() here. So here netif_wake_queue() woke ksoftirqd to
handle it. _Later_ there is this alloc_skb which does
local_bh_disable()/ enable() and the latter will look at pending
softirqs. They are still set from before because ksoftirqd had no chance
processing them. And now you see the deadlock from within
__netdev_alloc_skb().

I *think* lockdep will yell here on RT.
Looking at current kernel from !RT perspective, this isn't good either.
We have:

| ks8851_irq
| {
|    ks8851_lock()
|       -> spin_lock_irqsave()
irqs are off

|    if (status & IRQ_TXI) {
|       spin_lock_bh(&ks->statelock);
|       if (netif_queue_stopped(ks->netdev))
|          netif_wake_queue(ks->netdev);
raise softirq
|       spin_unlock_bh(&ks->statelock);
bh enable with disabled interrupts. And __local_bh_enable_ip() has this
gem:

|void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
| {
|         WARN_ON_ONCE(in_hardirq());
|         lockdep_assert_irqs_enabled();
| #ifdef CONFIG_TRACE_IRQFLAGS
|         local_irq_disable();
| #endif

so lockep will yell if interrupts are disabled. And handle_softirqs()
will enable interrupts before handling softirqs and restore them later
on. But CONFIG_TRACE_IRQFLAGS will keep them enabled. Since the lock is
not acquired in hardirq, it has no other deadlock problem.

What I don't understand is why this is limited to PREEMPT_RT. !RT is
also affected by this:
- ks8851_irq() acquires the lock, disables interrupts
- netif_wake_queue() raises the softirq
- spin_unlock_bh(&ks->statelock) enables BH and handles softirqs, and
  goes to ks8851_start_xmit()

This is only possible in newer kernels due to  0913ec336a6c0 ("net:
ks8851: Fix deadlock with the SPI chip variant") because of the
irq_disabled() check in skb allocation.

So. Using _bh instead _irq remains my recommendation. Lockdep should
already yell on !RT here. 

Sebastian