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

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

[Marek Vasut]

If CONFIG_PREEMPT_RT=y is set AND the driver executes ks8851_irq() AND
KSZ_ISR register bit IRQ_RXI is set AND ks8851_rx_pkts() detects that
there are packets in the RX FIFO, then netdev_alloc_skb_ip_align() is
called to allocate SKBs. If netdev_alloc_skb_ip_align() is called with
BH enabled, local_bh_enable() at the end of netdev_alloc_skb_ip_align()
will call __local_bh_enable_ip(), which will call __do_softirq(), which
may trigger net_tx_action() softirq, which may ultimately call the xmit
callback ks8851_start_xmit_par(). The ks8851_start_xmit_par() will try
to lock struct ks8851_net_par .lock spinlock, which is already locked
by ks8851_irq() from which ks8851_start_xmit_par() was called. This
leads to a deadlock, which is reported by the kernel, including a trace
listed below.

Fix the problem by disabling BH around the IRQ handler, thus preventing
the net_tx_action() softirq from triggering during the IRQ handler. The
net_tx_action() softirq is now triggered at the end of the IRQ handler,
once all the other IRQ handler actions have been completed.

  __schedule from schedule_rtlock+0x1c/0x34
  schedule_rtlock from rtlock_slowlock_locked+0x538/0x894
  rtlock_slowlock_locked from rt_spin_lock+0x44/0x5c
  rt_spin_lock from ks8851_start_xmit_par+0x68/0x1a0
  ks8851_start_xmit_par from netdev_start_xmit+0x1c/0x40
  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
  ks8851_irq from irq_thread_fn+0x24/0x64
  irq_thread_fn from irq_thread+0x110/0x1dc
  irq_thread from kthread+0x104/0x10c
  kthread from ret_from_fork+0x14/0x28

Fixes: e0863634bf9f ("net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs")
Cc: stable@vger.kernel.org
Signed-off-by: Marek Vasut <marex@nabladev.com>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Nicolai Buchwitz <nb@tipi-net.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: Yicong Hui <yiconghui@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: Register dedicated IRQ handler wrapper which disables BH for the
    parallel variant of the MAC, the variant which uses spinlocks as
    the locking primitive. Use stock IRQ handler with BH unchanged
    for the SPI variant, which uses mutexes as locking primitive.
---
 drivers/net/ethernet/micrel/ks8851.h        |  2 ++
 drivers/net/ethernet/micrel/ks8851_common.c | 22 ++++++++++++++++++++-
 drivers/net/ethernet/micrel/ks8851_par.c    |  1 +
 drivers/net/ethernet/micrel/ks8851_spi.c    |  1 +
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 31f75b4a67fd7..71213b44b0d04 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -394,6 +394,8 @@ struct ks8851_net {
 	u16			rc_rxqcr;
 	u16			rc_ccr;
 
+	bool			no_bh_in_irq_handler;
+
 	struct mii_if_info	mii;
 	struct ks8851_rxctrl	rxctrl;
 
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 8048770958d60..0d7c548d18356 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -385,6 +385,24 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 	return IRQ_HANDLED;
 }
 
+/**
+ * ks8851_irq_nobh - IRQ handler with BH disabled
+ * @irq: IRQ number
+ * @_ks: cookie
+ *
+ * Wrapper which calls ks8851_irq() with BH disabled.
+ */
+static irqreturn_t ks8851_irq_nobh(int irq, void *_ks)
+{
+	irqreturn_t ret;
+
+	local_bh_disable();
+	ret = ks8851_irq(irq, _ks);
+	local_bh_enable();
+
+	return ret;
+}
+
 /**
  * ks8851_flush_tx_work - flush outstanding TX work
  * @ks: The device state
@@ -408,7 +426,9 @@ static int ks8851_net_open(struct net_device *dev)
 	unsigned long flags;
 	int ret;
 
-	ret = request_threaded_irq(dev->irq, NULL, ks8851_irq,
+	ret = request_threaded_irq(dev->irq, NULL,
+				   ks->no_bh_in_irq_handler ?
+				   ks8851_irq_nobh : ks8851_irq,
 				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 				   dev->name, ks);
 	if (ret < 0) {
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
index 78695be2570bf..0fe3e40291a24 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -288,6 +288,7 @@ static int ks8851_probe_par(struct platform_device *pdev)
 	ks->rdfifo = ks8851_rdfifo_par;
 	ks->wrfifo = ks8851_wrfifo_par;
 	ks->start_xmit = ks8851_start_xmit_par;
+	ks->no_bh_in_irq_handler = true;
 
 #define STD_IRQ (IRQ_LCI |	/* Link Change */	\
 		 IRQ_RXI |	/* RX done */		\
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
index a161ae45743ab..9afdb7eb7953f 100644
--- a/drivers/net/ethernet/micrel/ks8851_spi.c
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -424,6 +424,7 @@ static int ks8851_probe_spi(struct spi_device *spi)
 	ks->wrfifo = ks8851_wrfifo_spi;
 	ks->start_xmit = ks8851_start_xmit_spi;
 	ks->flush_tx_work = ks8851_flush_tx_work_spi;
+	ks->no_bh_in_irq_handler = false;
 
 #define STD_IRQ (IRQ_LCI |	/* Link Change */	\
 		 IRQ_TXI |	/* TX done */		\
-- 
2.53.0

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

[Nicolai Buchwitz]

Hi Marek

On 8.4.2026 18:24, Marek Vasut wrote:

> [...]

> +	bool			no_bh_in_irq_handler;

> [...]

> +/**
> + * ks8851_irq_nobh - IRQ handler with BH disabled
> + * @irq: IRQ number
> + * @_ks: cookie
> + *
> + * Wrapper which calls ks8851_irq() with BH disabled.
> + */
> +static irqreturn_t ks8851_irq_nobh(int irq, void *_ks)
> +{
> +	irqreturn_t ret;
> +
> +	local_bh_disable();
> +	ret = ks8851_irq(irq, _ks);
> +	local_bh_enable();
> +
> +	return ret;
> +}
> +
>  /**
>   * ks8851_flush_tx_work - flush outstanding TX work
>   * @ks: The device state
> @@ -408,7 +426,9 @@ static int ks8851_net_open(struct net_device *dev)
>  	unsigned long flags;
>  	int ret;
> 
> -	ret = request_threaded_irq(dev->irq, NULL, ks8851_irq,
> +	ret = request_threaded_irq(dev->irq, NULL,
> +				   ks->no_bh_in_irq_handler ?
> +				   ks8851_irq_nobh : ks8851_irq,

This works, but wouldn't it be simpler to put the BH disable
into the PAR lock/unlock directly?

   static void ks8851_lock_par(...)
   {
       local_bh_disable();
       spin_lock_irqsave(&ksp->lock, *flags);
   }

   static void ks8851_unlock_par(...)
   {
       spin_unlock_irqrestore(&ksp->lock, *flags);
       local_bh_enable();
   }

No flag, no wrapper, no conditional in request_threaded_irq.
And it protects all PAR lock/unlock callsites, not just the
IRQ handler.

> [...]

Regards
Nicolai

Tested-by: Nicolai Buchwitz <nb@tipi-net.de>  # KS8851 SPI, non-RT

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

[Marek Vasut]

On 4/9/26 8:52 AM, Nicolai Buchwitz wrote:

Hello Nicolai,

>> @@ -408,7 +426,9 @@ static int ks8851_net_open(struct net_device *dev)
>>      unsigned long flags;
>>      int ret;
>>
>> -    ret = request_threaded_irq(dev->irq, NULL, ks8851_irq,
>> +    ret = request_threaded_irq(dev->irq, NULL,
>> +                   ks->no_bh_in_irq_handler ?
>> +                   ks8851_irq_nobh : ks8851_irq,
> 
> This works, but wouldn't it be simpler to put the BH disable
> into the PAR lock/unlock directly?
> 
>    static void ks8851_lock_par(...)
>    {
>        local_bh_disable();
>        spin_lock_irqsave(&ksp->lock, *flags);
>    }
> 
>    static void ks8851_unlock_par(...)
>    {
>        spin_unlock_irqrestore(&ksp->lock, *flags);
>        local_bh_enable();
>    }
> 
> No flag, no wrapper, no conditional in request_threaded_irq.
> And it protects all PAR lock/unlock callsites, not just the
> IRQ handler.
That is exactly why I wrapped the IRQ handler, because the BH should be 
disabled ONLY around the IRQ handler, not around the other call sites.

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

[Nicolai Buchwitz]

On 9.4.2026 17:26, Marek Vasut wrote:
> On 4/9/26 8:52 AM, Nicolai Buchwitz wrote:
> 
> Hello Nicolai,
> 
>>> @@ -408,7 +426,9 @@ static int ks8851_net_open(struct net_device 
>>> *dev)
>>>      unsigned long flags;
>>>      int ret;
>>> 
>>> -    ret = request_threaded_irq(dev->irq, NULL, ks8851_irq,
>>> +    ret = request_threaded_irq(dev->irq, NULL,
>>> +                   ks->no_bh_in_irq_handler ?
>>> +                   ks8851_irq_nobh : ks8851_irq,
>> 
>> This works, but wouldn't it be simpler to put the BH disable
>> into the PAR lock/unlock directly?
>> 
>>    static void ks8851_lock_par(...)
>>    {
>>        local_bh_disable();
>>        spin_lock_irqsave(&ksp->lock, *flags);
>>    }
>> 
>>    static void ks8851_unlock_par(...)
>>    {
>>        spin_unlock_irqrestore(&ksp->lock, *flags);
>>        local_bh_enable();
>>    }
>> 
>> No flag, no wrapper, no conditional in request_threaded_irq.
>> And it protects all PAR lock/unlock callsites, not just the
>> IRQ handler.
> That is exactly why I wrapped the IRQ handler, because the BH should be 
> disabled ONLY around the IRQ handler, not around the other call sites.

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

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

[Jakub Kicinski]

On Wed,  8 Apr 2026 18:24:58 +0200 Marek Vasut wrote:
> If CONFIG_PREEMPT_RT=y is set AND the driver executes ks8851_irq() AND
> KSZ_ISR register bit IRQ_RXI is set AND ks8851_rx_pkts() detects that
> there are packets in the RX FIFO, then netdev_alloc_skb_ip_align() is
> called to allocate SKBs. If netdev_alloc_skb_ip_align() is called with
> BH enabled, local_bh_enable() at the end of netdev_alloc_skb_ip_align()
> will call __local_bh_enable_ip(), which will call __do_softirq(), which
> may trigger net_tx_action() softirq, which may ultimately call the xmit
> callback ks8851_start_xmit_par(). The ks8851_start_xmit_par() will try
> to lock struct ks8851_net_par .lock spinlock, which is already locked
> by ks8851_irq() from which ks8851_start_xmit_par() was called. This
> leads to a deadlock, which is reported by the kernel, including a trace
> listed below.

lock_par is a spinlock, and AFAIU softirqs run in their on thread on RT.
I'm not following.

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.
-- 
pw-bot: cr

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

[Marek Vasut]

On 4/12/26 6:01 PM, Jakub Kicinski wrote:
> On Wed,  8 Apr 2026 18:24:58 +0200 Marek Vasut wrote:
>> If CONFIG_PREEMPT_RT=y is set AND the driver executes ks8851_irq() AND
>> KSZ_ISR register bit IRQ_RXI is set AND ks8851_rx_pkts() detects that
>> there are packets in the RX FIFO, then netdev_alloc_skb_ip_align() is
>> called to allocate SKBs. If netdev_alloc_skb_ip_align() is called with
>> BH enabled, local_bh_enable() at the end of netdev_alloc_skb_ip_align()
>> will call __local_bh_enable_ip(), which will call __do_softirq(), which
>> may trigger net_tx_action() softirq, which may ultimately call the xmit
>> callback ks8851_start_xmit_par(). The ks8851_start_xmit_par() will try
>> to lock struct ks8851_net_par .lock spinlock, which is already locked
>> by ks8851_irq() from which ks8851_start_xmit_par() was called. This
>> leads to a deadlock, which is reported by the kernel, including a trace
>> listed below.
> 
> lock_par is a spinlock, and AFAIU softirqs run in their on thread on RT.
> I'm not following.

Please look at the backtrace in the commit message, this part, please 
read from bottom to top to observe the failure in chronological order. 
It does not seem the handle_softirqs() is running in its own thread, 
separate from the IRQ thread ?

   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 ?

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

[Jakub Kicinski]

On Sun, 12 Apr 2026 18:27:28 +0200 Marek Vasut wrote:
> On 4/12/26 6:01 PM, Jakub Kicinski wrote:
> > On Wed,  8 Apr 2026 18:24:58 +0200 Marek Vasut wrote:  
> >> If CONFIG_PREEMPT_RT=y is set AND the driver executes ks8851_irq() AND
> >> KSZ_ISR register bit IRQ_RXI is set AND ks8851_rx_pkts() detects that
> >> there are packets in the RX FIFO, then netdev_alloc_skb_ip_align() is
> >> called to allocate SKBs. If netdev_alloc_skb_ip_align() is called with
> >> BH enabled, local_bh_enable() at the end of netdev_alloc_skb_ip_align()
> >> will call __local_bh_enable_ip(), which will call __do_softirq(), which
> >> may trigger net_tx_action() softirq, which may ultimately call the xmit
> >> callback ks8851_start_xmit_par(). The ks8851_start_xmit_par() will try
> >> to lock struct ks8851_net_par .lock spinlock, which is already locked
> >> by ks8851_irq() from which ks8851_start_xmit_par() was called. This
> >> leads to a deadlock, which is reported by the kernel, including a trace
> >> listed below.  
> > 
> > lock_par is a spinlock, and AFAIU softirqs run in their on thread on RT.
> > I'm not following.  
> 
> Please look at the backtrace in the commit message, this part, please 
> read from bottom to top to observe the failure in chronological order. 
> It does not seem the handle_softirqs() is running in its own thread, 
> separate from the IRQ thread ?
> 
>    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()

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.