[net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[net,PATCH v3 1/2] 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 critical sections, including the
IRQ handler, thus preventing the net_tx_action() softirq from triggering
during these critical sections. The net_tx_action() softirq is 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+0x548/0x904
 rtlock_slowlock_locked from rt_spin_lock+0x60/0x9c
 rt_spin_lock from ks8851_start_xmit_par+0x74/0x1a8
 ks8851_start_xmit_par from netdev_start_xmit+0x20/0x44
 netdev_start_xmit from dev_hard_start_xmit+0xd0/0x188
 dev_hard_start_xmit from sch_direct_xmit+0xb8/0x25c
 sch_direct_xmit from __qdisc_run+0x1f8/0x4ec
 __qdisc_run from qdisc_run+0x1c/0x28
 qdisc_run from net_tx_action+0x1f0/0x268
 net_tx_action from handle_softirqs+0x1a4/0x270
 handle_softirqs from __local_bh_enable_ip+0xcc/0xe0
 __local_bh_enable_ip from __alloc_skb+0xd8/0x128
 __alloc_skb from __netdev_alloc_skb+0x3c/0x19c
 __netdev_alloc_skb from ks8851_irq+0x388/0x4d4
 ks8851_irq from irq_thread_fn+0x24/0x64
 irq_thread_fn from irq_thread+0x178/0x28c
 irq_thread from kthread+0x12c/0x138
 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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
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.
V3: Switch to spin_lock_bh(), update commit message
---
 drivers/net/ethernet/micrel/ks8851.h        |  6 +-
 drivers/net/ethernet/micrel/ks8851_common.c | 64 +++++++++------------
 drivers/net/ethernet/micrel/ks8851_par.c    | 15 ++---
 drivers/net/ethernet/micrel/ks8851_spi.c    | 11 ++--
 4 files changed, 38 insertions(+), 58 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 31f75b4a67fd7..b795a3a605711 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -408,10 +408,8 @@ struct ks8851_net {
 	struct gpio_desc	*gpio;
 	struct mii_bus		*mii_bus;
 
-	void			(*lock)(struct ks8851_net *ks,
-					unsigned long *flags);
-	void			(*unlock)(struct ks8851_net *ks,
-					  unsigned long *flags);
+	void			(*lock)(struct ks8851_net *ks);
+	void			(*unlock)(struct ks8851_net *ks);
 	unsigned int		(*rdreg16)(struct ks8851_net *ks,
 					   unsigned int reg);
 	void			(*wrreg16)(struct ks8851_net *ks,
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 8048770958d60..6c375647b24de 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -28,25 +28,23 @@
 /**
  * ks8851_lock - register access lock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Claim chip register access lock
  */
-static void ks8851_lock(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_lock(struct ks8851_net *ks)
 {
-	ks->lock(ks, flags);
+	ks->lock(ks);
 }
 
 /**
  * ks8851_unlock - register access unlock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Release chip register access lock
  */
-static void ks8851_unlock(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_unlock(struct ks8851_net *ks)
 {
-	ks->unlock(ks, flags);
+	ks->unlock(ks);
 }
 
 /**
@@ -129,11 +127,10 @@ static void ks8851_set_powermode(struct ks8851_net *ks, unsigned pwrmode)
 static int ks8851_write_mac_addr(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 	u16 val;
 	int i;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	/*
 	 * Wake up chip in case it was powered off when stopped; otherwise,
@@ -149,7 +146,7 @@ static int ks8851_write_mac_addr(struct net_device *dev)
 	if (!netif_running(dev))
 		ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
 
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	return 0;
 }
@@ -163,12 +160,11 @@ static int ks8851_write_mac_addr(struct net_device *dev)
 static void ks8851_read_mac_addr(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 	u8 addr[ETH_ALEN];
 	u16 reg;
 	int i;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	for (i = 0; i < ETH_ALEN; i += 2) {
 		reg = ks8851_rdreg16(ks, KS_MAR(i));
@@ -177,7 +173,7 @@ static void ks8851_read_mac_addr(struct net_device *dev)
 	}
 	eth_hw_addr_set(dev, addr);
 
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 }
 
 /**
@@ -312,11 +308,10 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 {
 	struct ks8851_net *ks = _ks;
 	struct sk_buff_head rxq;
-	unsigned long flags;
 	unsigned int status;
 	struct sk_buff *skb;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	status = ks8851_rdreg16(ks, KS_ISR);
 	ks8851_wrreg16(ks, KS_ISR, status);
@@ -373,7 +368,7 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 		ks8851_wrreg16(ks, KS_RXCR1, rxc->rxcr1);
 	}
 
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	if (status & IRQ_LCI)
 		mii_check_link(&ks->mii);
@@ -405,7 +400,6 @@ static void ks8851_flush_tx_work(struct ks8851_net *ks)
 static int ks8851_net_open(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 	int ret;
 
 	ret = request_threaded_irq(dev->irq, NULL, ks8851_irq,
@@ -418,7 +412,7 @@ static int ks8851_net_open(struct net_device *dev)
 
 	/* lock the card, even if we may not actually be doing anything
 	 * else at the moment */
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	netif_dbg(ks, ifup, ks->netdev, "opening\n");
 
@@ -471,7 +465,7 @@ static int ks8851_net_open(struct net_device *dev)
 
 	netif_dbg(ks, ifup, ks->netdev, "network device up\n");
 
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 	mii_check_link(&ks->mii);
 	return 0;
 }
@@ -487,23 +481,22 @@ static int ks8851_net_open(struct net_device *dev)
 static int ks8851_net_stop(struct net_device *dev)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 
 	netif_info(ks, ifdown, dev, "shutting down\n");
 
 	netif_stop_queue(dev);
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 	/* turn off the IRQs and ack any outstanding */
 	ks8851_wrreg16(ks, KS_IER, 0x0000);
 	ks8851_wrreg16(ks, KS_ISR, 0xffff);
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	/* stop any outstanding work */
 	ks8851_flush_tx_work(ks);
 	flush_work(&ks->rxctrl_work);
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 	/* shutdown RX process */
 	ks8851_wrreg16(ks, KS_RXCR1, 0x0000);
 
@@ -512,7 +505,7 @@ static int ks8851_net_stop(struct net_device *dev)
 
 	/* set powermode to soft power down to save power */
 	ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN);
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	/* ensure any queued tx buffers are dumped */
 	while (!skb_queue_empty(&ks->txq)) {
@@ -566,14 +559,13 @@ static netdev_tx_t ks8851_start_xmit(struct sk_buff *skb,
 static void ks8851_rxctrl_work(struct work_struct *work)
 {
 	struct ks8851_net *ks = container_of(work, struct ks8851_net, rxctrl_work);
-	unsigned long flags;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	/* need to shutdown RXQ before modifying filter parameters */
 	ks8851_wrreg16(ks, KS_RXCR1, 0x00);
 
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 }
 
 static void ks8851_set_rx_mode(struct net_device *dev)
@@ -780,7 +772,6 @@ static int ks8851_set_eeprom(struct net_device *dev,
 {
 	struct ks8851_net *ks = netdev_priv(dev);
 	int offset = ee->offset;
-	unsigned long flags;
 	int len = ee->len;
 	u16 tmp;
 
@@ -794,7 +785,7 @@ static int ks8851_set_eeprom(struct net_device *dev,
 	if (!(ks->rc_ccr & CCR_EEPROM))
 		return -ENOENT;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	ks8851_eeprom_claim(ks);
 
@@ -817,7 +808,7 @@ static int ks8851_set_eeprom(struct net_device *dev,
 	eeprom_93cx6_wren(&ks->eeprom, false);
 
 	ks8851_eeprom_release(ks);
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	return 0;
 }
@@ -827,7 +818,6 @@ static int ks8851_get_eeprom(struct net_device *dev,
 {
 	struct ks8851_net *ks = netdev_priv(dev);
 	int offset = ee->offset;
-	unsigned long flags;
 	int len = ee->len;
 
 	/* must be 2 byte aligned */
@@ -837,7 +827,7 @@ static int ks8851_get_eeprom(struct net_device *dev,
 	if (!(ks->rc_ccr & CCR_EEPROM))
 		return -ENOENT;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 
 	ks8851_eeprom_claim(ks);
 
@@ -845,7 +835,7 @@ static int ks8851_get_eeprom(struct net_device *dev,
 
 	eeprom_93cx6_multiread(&ks->eeprom, offset/2, (__le16 *)data, len/2);
 	ks8851_eeprom_release(ks);
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	return 0;
 }
@@ -904,7 +894,6 @@ static int ks8851_phy_reg(int reg)
 static int ks8851_phy_read_common(struct net_device *dev, int phy_addr, int reg)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 	int result;
 	int ksreg;
 
@@ -912,9 +901,9 @@ static int ks8851_phy_read_common(struct net_device *dev, int phy_addr, int reg)
 	if (ksreg < 0)
 		return ksreg;
 
-	ks8851_lock(ks, &flags);
+	ks8851_lock(ks);
 	result = ks8851_rdreg16(ks, ksreg);
-	ks8851_unlock(ks, &flags);
+	ks8851_unlock(ks);
 
 	return result;
 }
@@ -949,14 +938,13 @@ static void ks8851_phy_write(struct net_device *dev,
 			     int phy, int reg, int value)
 {
 	struct ks8851_net *ks = netdev_priv(dev);
-	unsigned long flags;
 	int ksreg;
 
 	ksreg = ks8851_phy_reg(reg);
 	if (ksreg >= 0) {
-		ks8851_lock(ks, &flags);
+		ks8851_lock(ks);
 		ks8851_wrreg16(ks, ksreg, value);
-		ks8851_unlock(ks, &flags);
+		ks8851_unlock(ks);
 	}
 }
 
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
index 78695be2570bf..9f1c33f6ddec0 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -55,29 +55,27 @@ struct ks8851_net_par {
 /**
  * ks8851_lock_par - register access lock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Claim chip register access lock
  */
-static void ks8851_lock_par(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_lock_par(struct ks8851_net *ks)
 {
 	struct ks8851_net_par *ksp = to_ks8851_par(ks);
 
-	spin_lock_irqsave(&ksp->lock, *flags);
+	spin_lock_bh(&ksp->lock);
 }
 
 /**
  * ks8851_unlock_par - register access unlock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Release chip register access lock
  */
-static void ks8851_unlock_par(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_unlock_par(struct ks8851_net *ks)
 {
 	struct ks8851_net_par *ksp = to_ks8851_par(ks);
 
-	spin_unlock_irqrestore(&ksp->lock, *flags);
+	spin_unlock_bh(&ksp->lock);
 }
 
 /**
@@ -233,7 +231,6 @@ static netdev_tx_t ks8851_start_xmit_par(struct sk_buff *skb,
 {
 	struct ks8851_net *ks = netdev_priv(dev);
 	netdev_tx_t ret = NETDEV_TX_OK;
-	unsigned long flags;
 	unsigned int txqcr;
 	u16 txmir;
 	int err;
@@ -241,7 +238,7 @@ static netdev_tx_t ks8851_start_xmit_par(struct sk_buff *skb,
 	netif_dbg(ks, tx_queued, ks->netdev,
 		  "%s: skb %p, %d@%p\n", __func__, skb, skb->len, skb->data);
 
-	ks8851_lock_par(ks, &flags);
+	ks8851_lock_par(ks);
 
 	txmir = ks8851_rdreg16_par(ks, KS_TXMIR) & 0x1fff;
 
@@ -262,7 +259,7 @@ static netdev_tx_t ks8851_start_xmit_par(struct sk_buff *skb,
 		ret = NETDEV_TX_BUSY;
 	}
 
-	ks8851_unlock_par(ks, &flags);
+	ks8851_unlock_par(ks);
 
 	return ret;
 }
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
index a161ae45743ab..b9e68520278d0 100644
--- a/drivers/net/ethernet/micrel/ks8851_spi.c
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -71,11 +71,10 @@ struct ks8851_net_spi {
 /**
  * ks8851_lock_spi - register access lock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Claim chip register access lock
  */
-static void ks8851_lock_spi(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_lock_spi(struct ks8851_net *ks)
 {
 	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 
@@ -85,11 +84,10 @@ static void ks8851_lock_spi(struct ks8851_net *ks, unsigned long *flags)
 /**
  * ks8851_unlock_spi - register access unlock
  * @ks: The chip state
- * @flags: Spinlock flags
  *
  * Release chip register access lock
  */
-static void ks8851_unlock_spi(struct ks8851_net *ks, unsigned long *flags)
+static void ks8851_unlock_spi(struct ks8851_net *ks)
 {
 	struct ks8851_net_spi *kss = to_ks8851_spi(ks);
 
@@ -309,7 +307,6 @@ static void ks8851_tx_work(struct work_struct *work)
 	struct ks8851_net_spi *kss;
 	unsigned short tx_space;
 	struct ks8851_net *ks;
-	unsigned long flags;
 	struct sk_buff *txb;
 	bool last;
 
@@ -317,7 +314,7 @@ static void ks8851_tx_work(struct work_struct *work)
 	ks = &kss->ks8851;
 	last = skb_queue_empty(&ks->txq);
 
-	ks8851_lock_spi(ks, &flags);
+	ks8851_lock_spi(ks);
 
 	while (!last) {
 		txb = skb_dequeue(&ks->txq);
@@ -343,7 +340,7 @@ static void ks8851_tx_work(struct work_struct *work)
 	ks->tx_space = tx_space;
 	spin_unlock_bh(&ks->statelock);
 
-	ks8851_unlock_spi(ks, &flags);
+	ks8851_unlock_spi(ks);
 }
 
 /**
-- 
2.53.0

[net,PATCH v3 2/2] net: ks8851: Avoid excess softirq scheduling

[Marek Vasut]

The code injects a packet into netif_rx() repeatedly, which will add
it to its internal NAPI and schedule a softirq, and process it. It is
more efficient to queue multiple packets and process them all at the
local_bh_enable() time.

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: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Yicong Hui <yiconghui@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V3: New patch
---
 drivers/net/ethernet/micrel/ks8851_common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 6c375647b24de..4afbb40bc0e4a 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -373,9 +373,12 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 	if (status & IRQ_LCI)
 		mii_check_link(&ks->mii);
 
-	if (status & IRQ_RXI)
+	if (status & IRQ_RXI) {
+		local_bh_disable();
 		while ((skb = __skb_dequeue(&rxq)))
 			netif_rx(skb);
+		local_bh_enable();
+	}
 
 	return IRQ_HANDLED;
 }
-- 
2.53.0

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-14 12:32:52 [+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.

#1 [received RX packet and a] TX packet has been sent
#2 Driver enables TX queue via netif_wake_queue() which schedules TX
   softirq to queue packets for this device.
#2 After spin_unlock_bh(&ks->statelock) the pending softirqs will be
   processed
#3 This deadlocks because of recursive locking via ks8851_net::lock in
   ks8851_irq() and ks8851_start_xmit_par().

This is what happens since commit 0913ec336a6c0 ("net: ks8851: Fix
deadlock with the SPI chip variant"). Before that commit the softirq
execution will be picked up by netdev_alloc_skb_ip_align() and requires
PREEMPT_RT and a RX packet in #1 to trigger the deadlock.

> Fix the problem by disabling BH around critical sections, including the
> IRQ handler, thus preventing the net_tx_action() softirq from triggering
> during these critical sections. The net_tx_action() softirq is 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+0x548/0x904
>  rtlock_slowlock_locked from rt_spin_lock+0x60/0x9c
>  rt_spin_lock from ks8851_start_xmit_par+0x74/0x1a8
>  ks8851_start_xmit_par from netdev_start_xmit+0x20/0x44
>  netdev_start_xmit from dev_hard_start_xmit+0xd0/0x188
>  dev_hard_start_xmit from sch_direct_xmit+0xb8/0x25c
>  sch_direct_xmit from __qdisc_run+0x1f8/0x4ec
>  __qdisc_run from qdisc_run+0x1c/0x28
>  qdisc_run from net_tx_action+0x1f0/0x268
>  net_tx_action from handle_softirqs+0x1a4/0x270
>  handle_softirqs from __local_bh_enable_ip+0xcc/0xe0
>  __local_bh_enable_ip from __alloc_skb+0xd8/0x128
>  __alloc_skb from __netdev_alloc_skb+0x3c/0x19c
>  __netdev_alloc_skb from ks8851_irq+0x388/0x4d4
>  ks8851_irq from irq_thread_fn+0x24/0x64
>  irq_thread_fn from irq_thread+0x178/0x28c
>  irq_thread from kthread+0x12c/0x138
>  kthread from ret_from_fork+0x14/0x28

The backtrace here and the description is based on an older kernel.
However

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> 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>

Sebastian

Re: [net,PATCH v3 2/2] net: ks8851: Avoid excess softirq scheduling

[Sebastian Andrzej Siewior]

On 2026-04-14 12:32:53 [+0200], Marek Vasut wrote:
> The code injects a packet into netif_rx() repeatedly, which will add
> it to its internal NAPI and schedule a softirq, and process it. It is
> more efficient to queue multiple packets and process them all at the
> local_bh_enable() time.
> 
> 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>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Marek Vasut]

On 4/14/26 2:57 PM, Sebastian Andrzej Siewior wrote:
> On 2026-04-14 12:32:52 [+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.
> 
> #1 [received RX packet and a] TX packet has been sent
> #2 Driver enables TX queue via netif_wake_queue() which schedules TX
>     softirq to queue packets for this device.
> #2 After spin_unlock_bh(&ks->statelock) the pending softirqs will be
>     processed
> #3 This deadlocks because of recursive locking via ks8851_net::lock in
>     ks8851_irq() and ks8851_start_xmit_par().
> 
> This is what happens since commit 0913ec336a6c0 ("net: ks8851: Fix
> deadlock with the SPI chip variant"). Before that commit the softirq
> execution will be picked up by netdev_alloc_skb_ip_align() and requires
> PREEMPT_RT and a RX packet in #1 to trigger the deadlock.

Do you want me to add this into the V4 commit message ?

>> Fix the problem by disabling BH around critical sections, including the
>> IRQ handler, thus preventing the net_tx_action() softirq from triggering
>> during these critical sections. The net_tx_action() softirq is 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+0x548/0x904
>>   rtlock_slowlock_locked from rt_spin_lock+0x60/0x9c
>>   rt_spin_lock from ks8851_start_xmit_par+0x74/0x1a8
>>   ks8851_start_xmit_par from netdev_start_xmit+0x20/0x44
>>   netdev_start_xmit from dev_hard_start_xmit+0xd0/0x188
>>   dev_hard_start_xmit from sch_direct_xmit+0xb8/0x25c
>>   sch_direct_xmit from __qdisc_run+0x1f8/0x4ec
>>   __qdisc_run from qdisc_run+0x1c/0x28
>>   qdisc_run from net_tx_action+0x1f0/0x268
>>   net_tx_action from handle_softirqs+0x1a4/0x270
>>   handle_softirqs from __local_bh_enable_ip+0xcc/0xe0
>>   __local_bh_enable_ip from __alloc_skb+0xd8/0x128
>>   __alloc_skb from __netdev_alloc_skb+0x3c/0x19c
>>   __netdev_alloc_skb from ks8851_irq+0x388/0x4d4
>>   ks8851_irq from irq_thread_fn+0x24/0x64
>>   irq_thread_fn from irq_thread+0x178/0x28c
>>   irq_thread from kthread+0x12c/0x138
>>   kthread from ret_from_fork+0x14/0x28
> 
> The backtrace here and the description is based on an older kernel.
> However
I actually did update the backtrace in V3 with the one from current next 
20260413 .

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-14 16:20:46 [+0200], Marek Vasut wrote:
> > This is what happens since commit 0913ec336a6c0 ("net: ks8851: Fix
> > deadlock with the SPI chip variant"). Before that commit the softirq
> > execution will be picked up by netdev_alloc_skb_ip_align() and requires
> > PREEMPT_RT and a RX packet in #1 to trigger the deadlock.
> 
> Do you want me to add this into the V4 commit message ?

The description does not match the code since the commit mentioned
above.

> > > Fix the problem by disabling BH around critical sections, including the
> > > IRQ handler, thus preventing the net_tx_action() softirq from triggering
> > > during these critical sections. The net_tx_action() softirq is 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+0x548/0x904
> > >   rtlock_slowlock_locked from rt_spin_lock+0x60/0x9c
> > >   rt_spin_lock from ks8851_start_xmit_par+0x74/0x1a8
> > >   ks8851_start_xmit_par from netdev_start_xmit+0x20/0x44
> > >   netdev_start_xmit from dev_hard_start_xmit+0xd0/0x188
> > >   dev_hard_start_xmit from sch_direct_xmit+0xb8/0x25c
> > >   sch_direct_xmit from __qdisc_run+0x1f8/0x4ec
> > >   __qdisc_run from qdisc_run+0x1c/0x28
> > >   qdisc_run from net_tx_action+0x1f0/0x268
> > >   net_tx_action from handle_softirqs+0x1a4/0x270
> > >   handle_softirqs from __local_bh_enable_ip+0xcc/0xe0
> > >   __local_bh_enable_ip from __alloc_skb+0xd8/0x128
> > >   __alloc_skb from __netdev_alloc_skb+0x3c/0x19c
> > >   __netdev_alloc_skb from ks8851_irq+0x388/0x4d4
> > >   ks8851_irq from irq_thread_fn+0x24/0x64
> > >   irq_thread_fn from irq_thread+0x178/0x28c
> > >   irq_thread from kthread+0x12c/0x138
> > >   kthread from ret_from_fork+0x14/0x28
> > 
> > The backtrace here and the description is based on an older kernel.
> > However
> I actually did update the backtrace in V3 with the one from current next
> 20260413 .

That would be from yesterday and the change is merged since v6.10. But
why is the softirq starting from __netdev_alloc_skb() instead of
spin_unlock_bh(&ks->statelock)? After that unlock, the softirq must be
processed and __netdev_alloc_skb() _could_ observe pending softirqs but
not from ks8851.

Sebastian

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Jakub Kicinski]

On Tue, 14 Apr 2026 14:57:53 +0200 Sebastian Andrzej Siewior wrote:
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Maybe I'm not being forceful enough.

Putting workarounds in the drivers is unacceptable.
__netdev_alloc_skb() must be legal to call under an _irq spin lock.

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Jakub Kicinski]

On Tue, 14 Apr 2026 08:09:31 -0700 Jakub Kicinski wrote:
> On Tue, 14 Apr 2026 14:57:53 +0200 Sebastian Andrzej Siewior wrote:
> > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>  
> 
> Maybe I'm not being forceful enough.
> 
> Putting workarounds in the drivers is unacceptable.
> __netdev_alloc_skb() must be legal to call under an _irq spin lock.

My bad, only read your reply to the old thread now.

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Marek Vasut]

On 4/14/26 4:52 PM, Sebastian Andrzej Siewior wrote:
> On 2026-04-14 16:20:46 [+0200], Marek Vasut wrote:
>>> This is what happens since commit 0913ec336a6c0 ("net: ks8851: Fix
>>> deadlock with the SPI chip variant"). Before that commit the softirq
>>> execution will be picked up by netdev_alloc_skb_ip_align() and requires
>>> PREEMPT_RT and a RX packet in #1 to trigger the deadlock.
>>
>> Do you want me to add this into the V4 commit message ?
> 
> The description does not match the code since the commit mentioned
> above.

I hope the V4 commit message is a bit better.

>>> The backtrace here and the description is based on an older kernel.
>>> However
>> I actually did update the backtrace in V3 with the one from current next
>> 20260413 .
> 
> That would be from yesterday and the change is merged since v6.10. But
> why is the softirq starting from __netdev_alloc_skb() instead of
> spin_unlock_bh(&ks->statelock)? After that unlock, the softirq must be
> processed and __netdev_alloc_skb() _could_ observe pending softirqs but
> not from ks8851.
Because __netdev_alloc_skb() also enables/disables BH , see the "else" 
branch:

  759 struct sk_buff *__netdev_alloc_skb(struct net_device *dev, 
unsigned int len,
  760                                    gfp_t gfp_mask)
  761 {
...
  786         if (in_hardirq() || irqs_disabled()) {
  787                 nc = this_cpu_ptr(&netdev_alloc_cache);
  788                 data = page_frag_alloc(nc, len, gfp_mask);
  789                 pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
  790         } else {
  791                 local_bh_disable();
  792                 local_lock_nested_bh(&napi_alloc_cache.bh_lock);
...
  798                 local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
  799                 local_bh_enable();
...

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-16 01:14:35 [+0200], Marek Vasut wrote:
> > spin_unlock_bh(&ks->statelock)? After that unlock, the softirq must be
> > processed and __netdev_alloc_skb() _could_ observe pending softirqs but
> > not from ks8851.
> Because __netdev_alloc_skb() also enables/disables BH , see the "else"

Yes. But there is no softirq raised in that part. That softirq is raised
by netif_wake_queue() within a bh disabled section. Therefore upon the
unlock the softirq must be invoked.
After that, rhe allocation later on may invoke softirqs which were
raised but I don't see how ks8851 can be part of it.
Before commit 0913ec336a6c0 ("net: ks8851: Fix deadlock with the SPI
chip variant") there was no _bh around it meaning the softirq was raised
but not invoked immediately. This happened on the bh unlock during
memory allocation. Therefore I am saying this backtrace is from an older
kernel.

If there is a flaw in my the theory please explain _how_ you managed
that get that backtrace. I am sure it must have from an older kernel and
_now_ this lockup also happens on !RT kernels (except for the SPI
platform).

Sebastian

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Marek Vasut]

On 4/16/26 8:21 AM, Sebastian Andrzej Siewior wrote:
> On 2026-04-16 01:14:35 [+0200], Marek Vasut wrote:
>>> spin_unlock_bh(&ks->statelock)? After that unlock, the softirq must be
>>> processed and __netdev_alloc_skb() _could_ observe pending softirqs but
>>> not from ks8851.
>> Because __netdev_alloc_skb() also enables/disables BH , see the "else"
> 
> Yes. But there is no softirq raised in that part. That softirq is raised
> by netif_wake_queue() within a bh disabled section. Therefore upon the
> unlock the softirq must be invoked.
> After that, rhe allocation later on may invoke softirqs which were
> raised but I don't see how ks8851 can be part of it.
> Before commit 0913ec336a6c0 ("net: ks8851: Fix deadlock with the SPI
> chip variant") there was no _bh around it meaning the softirq was raised
> but not invoked immediately. This happened on the bh unlock during
> memory allocation. Therefore I am saying this backtrace is from an older
> kernel.

I actually did update the backtrace in V3 with the one from next 
20260413 that contained b44596ffe1b4 ("ARM: Allow to enable RT") from 
stable-rt/v6.12-rt-rebase branch [1] .

I think I misunderstood the usage of "softirq is raised" vs. "softirq is 
invoked" above . Is it possible that there was an already raised softirq 
before the threaded IRQ handler was invoked, and __netdev_alloc_skb() is 
what invoked that softirq ?

> If there is a flaw in my the theory please explain _how_ you managed
> that get that backtrace. I am sure it must have from an older kernel and
> _now_ this lockup also happens on !RT kernels (except for the SPI
> platform).
I used [1] , with PREEMPT_RT enabled , on stm32mp157c SoC . I ran iperf3 
-s on the stm32 side, iperf3 -c 192.168.1.2 -t 0 --bidir on the hostpc 
side. The backtrace happened shortly after.

Re: [net,PATCH v3 1/2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-16 11:26:00 [+0200], Marek Vasut wrote:
> > memory allocation. Therefore I am saying this backtrace is from an older
> > kernel.
> 
> I actually did update the backtrace in V3 with the one from next 20260413
> that contained b44596ffe1b4 ("ARM: Allow to enable RT") from
> stable-rt/v6.12-rt-rebase branch [1] .
> 
> I think I misunderstood the usage of "softirq is raised" vs. "softirq is
> invoked" above . Is it possible that there was an already raised softirq
> before the threaded IRQ handler was invoked, and __netdev_alloc_skb() is
> what invoked that softirq ?

It is not impossible. Something needs to netif_wake_queue() and
ks8851_irq() must only report IRQ_RXI (not IRQ_TXI). Then it can happen.
But usually the driver "stops" the queue if it can't process any new
packets and resumes it once a packet has been sent so it has room again.

> > If there is a flaw in my the theory please explain _how_ you managed
> > that get that backtrace. I am sure it must have from an older kernel and
> > _now_ this lockup also happens on !RT kernels (except for the SPI
> > platform).
> I used [1] , with PREEMPT_RT enabled , on stm32mp157c SoC . I ran iperf3 -s
> on the stm32 side, iperf3 -c 192.168.1.2 -t 0 --bidir on the hostpc side.
> The backtrace happened shortly after.

Hmm. Let me accept it then.

Sebastian