FAILED: patch "[PATCH] net: ks8851: Fix TX stall caused by TX buffer overrun" failed to apply to 4.19-stable tree

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-4.19.y
git checkout FETCH_HEAD
git cherry-pick -x 3dc5d44545453de1de9c53cc529cc960a85933da
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2023122818-clapper-repaint-6f52@gregkh' --subject-prefix 'PATCH 4.19.y' HEAD^..

Possible dependencies:

3dc5d4454545 ("net: ks8851: Fix TX stall caused by TX buffer overrun") b07f987a8d77 ("net: ks8851: Separate SPI operations into separate file") 7a552c850c45 ("net: ks8851: Implement register, FIFO, lock accessor callbacks") d2a1c643a00e ("net: ks8851: Permit overridding interrupt enable register") 144ad36c3d3b ("net: ks8851: Factor out TX work flush function") 24be72632c68 ("net: ks8851: Split out SPI specific code from probe() and remove()") d48b7634c692 ("net: ks8851: Split out SPI specific entries in struct ks8851_net") 18a3df730932 ("net: ks8851: Factor out SKB receive function") 22726020050b ("net: ks8851: Factor out bus lock handling") aa39bf6730b7 ("net: ks8851: Use 16-bit read of RXFC register") 88cfedd0d7ab ("net: ks8851: Use 16-bit writes to program MAC address") 806f66495e79 ("net: ks8851: Remove ks8851_rdreg32()")
2c5b0a86ac54 ("net: ks8851: Use dev_{get,set}_drvdata()")
b6948e1b7b09 ("net: ks8851: Use devm_alloc_etherdev()")
bfd1e0eb08f6 ("net: ks8851: Rename ndev to netdev in probe") d320692d9f85 ("net: ks8851: Factor out spi->dev in probe()/remove()") aae079aa76d0 ("net: ks8851: Deduplicate register macros")
9624bafa5f64 ("net: ks8851: Set initial carrier state to down") d268f3155279 ("net: ks8851: Delay requesting IRQ until opened") 761cfa979a0c ("net: ks8851: Reassert reset pin if chip ID check fails")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 3dc5d44545453de1de9c53cc529cc960a85933da Mon Sep 17 00:00:00 2001
From: Ronald Wahl <[email protected]>
Date: Thu, 14 Dec 2023 19:11:12 +0100
Subject: [PATCH] net: ks8851: Fix TX stall caused by TX buffer overrun

There is a bug in the ks8851 Ethernet driver that more data is written to the hardware TX buffer than actually available. This is caused by wrong accounting of the free TX buffer space.

The driver maintains a tx_space variable that represents the TX buffer space that is deemed to be free. The ks8851_start_xmit_spi() function adds an SKB to a queue if tx_space is large enough and reduces tx_space by the amount of buffer space it will later need in the TX buffer and then schedules a work item. If there is not enough space then the TX queue is stopped.

The worker function ks8851_tx_work() dequeues all the SKBs and writes the data into the hardware TX buffer. The last packet will trigger an interrupt after it was send. Here it is assumed that all data fits into the TX buffer.

In the interrupt routine (which runs asynchronously because it is a threaded interrupt) tx_space is updated with the current value from the hardware. Also the TX queue is woken up again.

Now it could happen that after data was sent to the hardware and before handling the TX interrupt new data is queued in ks8851_start_xmit_spi() when the TX buffer space had still some space left. When the interrupt is actually handled tx_space is updated from the hardware but now we already have new SKBs queued that have not been written to the hardware TX buffer yet. Since tx_space has been overwritten by the value from the hardware the space is not accounted for.

Now we have more data queued then buffer space available in the hardware and ks8851_tx_work() will potentially overrun the hardware TX buffer. In many cases it will still work because often the buffer is written out fast enough so that no overrun occurs but for example if the peer throttles us via flow control then an overrun may happen.

This can be fixed in different ways. The most simple way would be to set tx_space to 0 before writing data to the hardware TX buffer preventing the queuing of more SKBs until the TX interrupt has been handled. I have chosen a slightly more efficient (and still rather simple) way and track the amount of data that is already queued and not yet written to the hardware. When new SKBs are to be queued the already queued amount of data is honoured when checking free TX buffer space.

I tested this with a setup of two linked KS8851 running iperf3 between the two in bidirectional mode. Before the fix I got a stall after some minutes. With the fix I saw now issues anymore after hours.

index fecd43754cea..e5ec0a363aff 100644
@@ -350,6 +350,8 @@ union ks8851_tx_hdr {
  * @rxd: Space for receiving SPI data, in DMA-able space.
  * @txd: Space for transmitting SPI data, in DMA-able space.
  * @msg_enable: The message flags controlling driver output (see ethtool).
+ * @tx_space: Free space in the hardware TX buffer (cached copy of KS_TXMIR).
+ * @queued_len: Space required in hardware TX buffer for queued packets in txq.
  * @fid: Incrementing frame id tag.
  * @rc_ier: Cached copy of KS_IER.
  * @rc_ccr: Cached copy of KS_CCR.

@@ -399,6 +401,7 @@ struct ks8851_net {
 	struct work_struct	rxctrl_work;
 
 	struct sk_buff_head	txq;
+	unsigned int		queued_len;
 
 	struct eeprom_93cx6	eeprom;
 	struct regulator	*vdd_reg;

index cfbc900d4aeb..0bf13b38b8f5 100644
@@ -362,16 +362,18 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 		handled |= IRQ_RXPSI;
 
 	if (status & IRQ_TXI) {
-		handled |= IRQ_TXI;
+		unsigned short tx_space = ks8851_rdreg16(ks, KS_TXMIR);
 
-		/* no lock here, tx queue should have been stopped */
+		netif_dbg(ks, intr, ks->netdev,
+			  "%s: txspace %d\n", __func__, tx_space);
 
-		/* update our idea of how much tx space is available to the
-		 * system */
-		ks->tx_space = ks8851_rdreg16(ks, KS_TXMIR);
+		spin_lock(&ks->statelock);
+		ks->tx_space = tx_space;
+		if (netif_queue_stopped(ks->netdev))
+			netif_wake_queue(ks->netdev);
+		spin_unlock(&ks->statelock);
 
-		netif_dbg(ks, intr, ks->netdev,
-			  "%s: txspace %d\n", __func__, ks->tx_space);
+		handled |= IRQ_TXI;
 	}
 
 	if (status & IRQ_RXI)

@@ -414,9 +416,6 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 	if (status & IRQ_LCI)
 		mii_check_link(&ks->mii);
 
-	if (status & IRQ_TXI)
-		netif_wake_queue(ks->netdev);
-
 	return IRQ_HANDLED;
 }
 

@@ -500,6 +499,7 @@ static int ks8851_net_open(struct net_device *dev)
 	ks8851_wrreg16(ks, KS_ISR, ks->rc_ier);
 	ks8851_wrreg16(ks, KS_IER, ks->rc_ier);
 
+	ks->queued_len = 0;
 	netif_start_queue(ks->netdev);
 
 	netif_dbg(ks, ifup, ks->netdev, "network device up\n");