Message ID | 20240703160053.9892-1-rwahl@gmx.de (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | ks8851: Fix deadlock with the SPI chip variant | expand |
On Wed, 3 Jul 2024 18:00:53 +0200 Ronald Wahl wrote: > + bool need_wake_queue; > > netif_dbg(ks, intr, ks->netdev, > "%s: txspace %d\n", __func__, tx_space); > > spin_lock(&ks->statelock); > ks->tx_space = tx_space; > - if (netif_queue_stopped(ks->netdev)) > - netif_wake_queue(ks->netdev); > + need_wake_queue = netif_queue_stopped(ks->netdev); > spin_unlock(&ks->statelock); > + if (need_wake_queue) > + netif_wake_queue(ks->netdev); xmit runs in BH, this is just one way you can hit this deadlock better fix would be to make sure statelock is always taken using spin_lock_bh()
Thanks, I made a v2. I now also found another potential TX stall issue caused by improper locking. In ks8851_tx_work we need to move last = skb_queue_empty(&ks->txq); under the lock or otherwise risk a TX stall because in case the queue was empty and has meanwhile being completely filled while we were waiting for the lock. I need to double check this scenario first. If it is indeed an issue then I will provide a separate patch later. On 04.07.24 16:44, Jakub Kicinski wrote: > On Wed, 3 Jul 2024 18:00:53 +0200 Ronald Wahl wrote: >> + bool need_wake_queue; >> >> netif_dbg(ks, intr, ks->netdev, >> "%s: txspace %d\n", __func__, tx_space); >> >> spin_lock(&ks->statelock); >> ks->tx_space = tx_space; >> - if (netif_queue_stopped(ks->netdev)) >> - netif_wake_queue(ks->netdev); >> + need_wake_queue = netif_queue_stopped(ks->netdev); >> spin_unlock(&ks->statelock); >> + if (need_wake_queue) >> + netif_wake_queue(ks->netdev); > > xmit runs in BH, this is just one way you can hit this deadlock > better fix would be to make sure statelock is always taken > using spin_lock_bh()
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c index 6453c92f0fa7..60b959126b26 100644 --- a/drivers/net/ethernet/micrel/ks8851_common.c +++ b/drivers/net/ethernet/micrel/ks8851_common.c @@ -348,15 +348,17 @@ static irqreturn_t ks8851_irq(int irq, void *_ks) if (status & IRQ_TXI) { unsigned short tx_space = ks8851_rdreg16(ks, KS_TXMIR); + bool need_wake_queue; netif_dbg(ks, intr, ks->netdev, "%s: txspace %d\n", __func__, tx_space); spin_lock(&ks->statelock); ks->tx_space = tx_space; - if (netif_queue_stopped(ks->netdev)) - netif_wake_queue(ks->netdev); + need_wake_queue = netif_queue_stopped(ks->netdev); spin_unlock(&ks->statelock); + if (need_wake_queue) + netif_wake_queue(ks->netdev); } if (status & IRQ_SPIBEI) {