| Message ID | 20240513143922.1330122-1-rwahl@gmx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ks8851: Fix another TX stall caused by wrong ISR flag handling | expand |
On Mon, May 13, 2024 at 04:39:22PM +0200, Ronald Wahl wrote: > From: Ronald Wahl <ronald.wahl@raritan.com> > > Under some circumstances it may happen that the ks8851 Ethernet driver > stops sending data. > > Currently the interrupt handler resets the interrupt status flags in the > hardware after handling TX. With this approach we may lose interrupts in > the time window between handling the TX interrupt and resetting the TX > interrupt status bit. > > When all of the three following conditions are true then transmitting > data stops: > > - TX queue is stopped to wait for room in the hardware TX buffer > - no queued SKBs in the driver (txq) that wait for being written to hw > - hardware TX buffer is empty and the last TX interrupt was lost > > This is because reenabling the TX queue happens when handling the TX > interrupt status but if the TX status bit has already been cleared then > this interrupt will never come. > > With this commit the interrupt status flags will be cleared before they > are handled. That way we stop losing interrupts. > > The wrong handling of the ISR flags was there from the beginning but > with commit 3dc5d4454545 ("net: ks8851: Fix TX stall caused by TX > buffer overrun") the issue becomes apparent. > > Fixes: 3dc5d4454545 ("net: ks8851: Fix TX stall caused by TX buffer overrun") > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Simon Horman <horms@kernel.org> > Cc: netdev@vger.kernel.org > Cc: stable@vger.kernel.org # 5.10+ > Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com> Reviewed-by: Simon Horman <horms@kernel.org>
On Mon, 13 May 2024 16:39:22 +0200 Ronald Wahl wrote: > Under some circumstances it may happen that the ks8851 Ethernet driver > stops sending data. > > Currently the interrupt handler resets the interrupt status flags in the > hardware after handling TX. With this approach we may lose interrupts in > the time window between handling the TX interrupt and resetting the TX > interrupt status bit. Thanks! This is commit 317a215d4932 ("net: ks8851: Fix another TX stall caused by wrong ISR flag handling") in net. Looks like bot didn't respond.
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c index 502518cdb461..6453c92f0fa7 100644 --- a/drivers/net/ethernet/micrel/ks8851_common.c +++ b/drivers/net/ethernet/micrel/ks8851_common.c @@ -328,7 +328,6 @@ static irqreturn_t ks8851_irq(int irq, void *_ks) { struct ks8851_net *ks = _ks; struct sk_buff_head rxq; - unsigned handled = 0; unsigned long flags; unsigned int status; struct sk_buff *skb; @@ -336,24 +335,17 @@ static irqreturn_t ks8851_irq(int irq, void *_ks) ks8851_lock(ks, &flags); status = ks8851_rdreg16(ks, KS_ISR); + ks8851_wrreg16(ks, KS_ISR, status); netif_dbg(ks, intr, ks->netdev, "%s: status 0x%04x\n", __func__, status); - if (status & IRQ_LCI) - handled |= IRQ_LCI; - if (status & IRQ_LDI) { u16 pmecr = ks8851_rdreg16(ks, KS_PMECR); pmecr &= ~PMECR_WKEVT_MASK; ks8851_wrreg16(ks, KS_PMECR, pmecr | PMECR_WKEVT_LINK); - - handled |= IRQ_LDI; } - if (status & IRQ_RXPSI) - handled |= IRQ_RXPSI; - if (status & IRQ_TXI) { unsigned short tx_space = ks8851_rdreg16(ks, KS_TXMIR); @@ -365,20 +357,12 @@ static irqreturn_t ks8851_irq(int irq, void *_ks) if (netif_queue_stopped(ks->netdev)) netif_wake_queue(ks->netdev); spin_unlock(&ks->statelock); - - handled |= IRQ_TXI; } - if (status & IRQ_RXI) - handled |= IRQ_RXI; - if (status & IRQ_SPIBEI) { netdev_err(ks->netdev, "%s: spi bus error\n", __func__); - handled |= IRQ_SPIBEI; } - ks8851_wrreg16(ks, KS_ISR, handled); - if (status & IRQ_RXI) { /* the datasheet says to disable the rx interrupt during * packet read-out, however we're masking the interrupt