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

Sebastian Andrzej Siewior
2026-04-14T10:48:22Z

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