net: ks8851: Reinstate disabling of BHs around IRQ handler

If the driver executes ks8851_irq() AND a TX packet has been sent, then
the driver enables TX queue via netif_wake_queue() which schedules TX
softirq to queue packets for this device.

If CONFIG_PREEMPT_RT=y is set AND a packet has also been received by
the MAC, then ks8851_rx_pkts() calls netdev_alloc_skb_ip_align() to
allocate SKBs for the received packets. If netdev_alloc_skb_ip_align()
is called with BH enabled, then local_bh_enable() at the end of
netdev_alloc_skb_ip_align() will trigger the pending softirq processing,
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.

If CONFIG_PREEMPT_RT is not set, then since commit 0913ec336a6c0
("net: ks8851: Fix deadlock with the SPI chip variant") the deadlock
can also be triggered without received packet in the RX FIFO. The
pending softirqs will be processed on return from
spin_unlock_bh(&ks->statelock) in ks8851_irq(), which triggers the
deadlock as well.

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
once BH are re-enabled and 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

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>
Link: https://patch.msgid.link/20260415231020.455298-1-marex@nabladev.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 31f75b4a67fd79eb1c7c08096a5a11a163a96b73..b795a3a605711892704fb9969645b8d943c1d6aa 100644 (file)
--- 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 8048770958d60e81440bc1ccf28be4945b02f8db..6c375647b24de6fb1a18720d50a70a2221397457 100644 (file)
--- 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 78695be2570bfbc525c5f3a54ae54518e19e4f66..9f1c33f6ddec01cce6277771983c744ad5fae4c5 100644 (file)
--- 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 a161ae45743ab588889f24ab0af334085db81be9..b9e68520278d0801e6eb08c0e648ab31f4a07136 100644 (file)
--- 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);
 }
 
 /**