[PATCH net v3 0/3] Fix to possible skb leak due to race condtion in tx path

[PATCH net v3 0/3] Fix to possible skb leak due to race condtion in tx path

[Selvamani Rajagopal via B4 Relay]

Now the traffic is handled in threaded IRQ, and the
disable_traffic flag is checked before handling the
data, new race condition is exposed, in which
buffer may leak, if threaded IRQ interrupts the
trasmit path midway.

With this change, disable_traffic and waiting_tx_skb
pointer are protected by spin lock/unlock pair.

This is highlighted in Sashiko review
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260611-level-trigger-v5-0-4533a9e85ce2%40onsemi.com

Also on buffer overrun condition, probably due to loss of
SPI data chunks, receive path doesn't see the expected
data chunk with end_valid bit set. As a result, driver
keeps adding data chunks to the skb before running out
of space and kernel panic is seen.

With this change, before adding data to the skb, if there
is no space, skb is freed and driver starts looking for
new frame by looking for a data chunk with start_valid
bit set.

[  705.405490] skbuff: skb_over_panic: text:ffffffd2eb72a264 len:1600 put:64 head:ffffff804e5cdc40 data:ffffff804e5cdc80 tail:0x680 end:0x640 dev:eth1
[  705.405569] ------------[ cut here ]------------
[  705.405575] kernel BUG at net/core/skbuff.c:214!
[  705.405589] Internal error: Oops - BUG: 00000000f2000800 [#1]  SMP

[ 6703.427690] Call trace:
[  705.925157]  skb_panic+0x58/0x68 (P)
[  705.928726]  skb_put+0x74/0x80
[  705.931772]  oa_tc6_update_rx_skb+0x44/0x98 [oa_tc6_mod]
[  705.937084]  oa_tc6_macphy_threaded_irq+0x3f4/0x900 [oa_tc6_mod]
[  705.943084]  irq_thread_fn+0x34/0xb8
[  705.946654]  irq_thread+0x1a0/0x300
[  705.950134]  kthread+0x138/0x150
[  705.953356]  ret_from_fork+0x10/0x20

Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
---
Changes in v3:
- Cover all the instances of disable_traffic flag with
  spin lock to serialize the access
- Disabling the tx queue and mark the carrier off when
  disable_traffic is set.
- Continue processing received chunks on buffer overflow
  error and "out of skb" error.
- Link to v2: https://lore.kernel.org/r/20260626-fix-race-condition-and-crash-v2-0-b6c5c10e604f@onsemi.com

Changes in v2:
- Improvment to how error -EAGAIN is handled. Took care of
  couple of use cases where start_bit and end_bit may be missing or
  repeated due to lost data chunks.
- Protected handling of waiting_tx_skb pointer with spin lock
- Link to v1: https://lore.kernel.org/r/20260621-fix-race-condition-and-crash-v1-0-87e290d9357f@onsemi.com

---
Selvamani Rajagopal (3):
      net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances
      net: ethernet: oa_tc6: Improvement in buffer overflow handling
      net: ethernet: oa_tc6: Carrier off when disable_traffic is set

 drivers/net/ethernet/oa_tc6.c | 219 ++++++++++++++++++++++++++++++------------
 1 file changed, 158 insertions(+), 61 deletions(-)
---
base-commit: d7a8d500d7e42837bd8dce40cb52c97c6e8706a9
change-id: 20260621-fix-race-condition-and-crash-94d055a665c4

Best regards,
-- 
Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

[PATCH net v3 1/3] net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances

[Selvamani Rajagopal via B4 Relay]

From: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Threaded IRQ uses waiting_tx_skb. Transmit path also uses
this pointer without any mutual exclusion protection. As a
result, it might leak skb buffer, particularly threaded IRQ
runs in the middle of tranmsmit path, near skb_linearize.

Fixes: b542d13fab0f ("net: ethernet: oa_tc6: Interrupt is active low, level triggered.")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

---
changes in v3
  - Added the missed out spin lock protection for
    waiting_tx_skb and disable_traffic flag
changes in v2
  - added the missing prefix to the title
---
 drivers/net/ethernet/oa_tc6.c | 100 +++++++++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 0727d53345a3..5b24cce4f9b5 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -652,6 +652,26 @@ static int oa_tc6_enable_data_transfer(struct oa_tc6 *tc6)
 	return oa_tc6_write_register(tc6, OA_TC6_REG_CONFIG0, value);
 }
 
+/* Called when a frame that is meant to be transmitted, is dropped. */
+static void oa_tc6_drop_tx_skb(struct oa_tc6 *tc6, struct sk_buff *skb)
+{
+	if (skb) {
+		tc6->netdev->stats.tx_dropped++;
+		dev_kfree_skb_any(skb);
+	}
+}
+
+static struct sk_buff *oa_tc6_detach_waiting_tx_skb(struct oa_tc6 *tc6)
+{
+	struct sk_buff *skb;
+
+	lockdep_assert_held(&tc6->tx_skb_lock);
+	skb = tc6->waiting_tx_skb;
+	tc6->waiting_tx_skb = NULL;
+
+	return skb;
+}
+
 static void oa_tc6_cleanup_ongoing_rx_skb(struct oa_tc6 *tc6)
 {
 	if (tc6->rx_skb) {
@@ -663,26 +683,30 @@ static void oa_tc6_cleanup_ongoing_rx_skb(struct oa_tc6 *tc6)
 
 static void oa_tc6_cleanup_ongoing_tx_skb(struct oa_tc6 *tc6)
 {
-	if (tc6->ongoing_tx_skb) {
-		tc6->netdev->stats.tx_dropped++;
-		kfree_skb(tc6->ongoing_tx_skb);
-		tc6->ongoing_tx_skb = NULL;
-	}
+	oa_tc6_drop_tx_skb(tc6, tc6->ongoing_tx_skb);
+	tc6->ongoing_tx_skb = NULL;
 }
 
 static void oa_tc6_cleanup_waiting_tx_skb(struct oa_tc6 *tc6)
 {
-	if (tc6->waiting_tx_skb) {
-		tc6->netdev->stats.tx_dropped++;
-		kfree_skb(tc6->waiting_tx_skb);
-		tc6->waiting_tx_skb = NULL;
-	}
+	struct sk_buff *skb;
+
+	spin_lock_bh(&tc6->tx_skb_lock);
+	skb = oa_tc6_detach_waiting_tx_skb(tc6);
+	spin_unlock_bh(&tc6->tx_skb_lock);
+
+	oa_tc6_drop_tx_skb(tc6, skb);
 }
 
-static void oa_tc6_free_pending_skbs(struct oa_tc6 *tc6)
+static void oa_tc6_free_ongoing_skbs(struct oa_tc6 *tc6)
 {
 	oa_tc6_cleanup_ongoing_tx_skb(tc6);
 	oa_tc6_cleanup_ongoing_rx_skb(tc6);
+}
+
+static void oa_tc6_free_pending_skbs(struct oa_tc6 *tc6)
+{
+	oa_tc6_free_ongoing_skbs(tc6);
 	oa_tc6_cleanup_waiting_tx_skb(tc6);
 }
 
@@ -693,9 +717,15 @@ static void oa_tc6_free_pending_skbs(struct oa_tc6 *tc6)
 static void oa_tc6_disable_traffic(struct oa_tc6 *tc6)
 {
 	u32 regval = INT_MASK0_ALL_INTERRUPTS;
+	struct sk_buff *skb;
 
+	spin_lock_bh(&tc6->tx_skb_lock);
 	tc6->disable_traffic = true;
-	oa_tc6_free_pending_skbs(tc6);
+	skb = oa_tc6_detach_waiting_tx_skb(tc6);
+	spin_unlock_bh(&tc6->tx_skb_lock);
+
+	oa_tc6_drop_tx_skb(tc6, skb);
+	oa_tc6_free_ongoing_skbs(tc6);
 	oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
 	oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &regval);
 	oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
@@ -1136,8 +1166,7 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
 			if (ret == -EAGAIN)
 				continue;
 
-			oa_tc6_cleanup_ongoing_tx_skb(tc6);
-			oa_tc6_cleanup_ongoing_rx_skb(tc6);
+			oa_tc6_free_ongoing_skbs(tc6);
 			netdev_err(tc6->netdev, "Device error: %d\n", ret);
 			return ret;
 		}
@@ -1159,15 +1188,20 @@ static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
 	 * no need to attempt spi transfer, once it fails. Pending skbs
 	 * are already freed.
 	 */
-	if (!tc6->disable_traffic) {
-		while (tc6->int_flag ||
-		       (tc6->waiting_tx_skb && tc6->tx_credits)) {
-			ret = oa_tc6_try_spi_transfer(tc6);
-			if (ret) {
-				disable_irq_nosync(tc6->spi->irq);
-				oa_tc6_disable_traffic(tc6);
-				break;
-			}
+	spin_lock_bh(&tc6->tx_skb_lock);
+	if (tc6->disable_traffic) {
+		spin_unlock_bh(&tc6->tx_skb_lock);
+		return IRQ_HANDLED;
+	}
+	spin_unlock_bh(&tc6->tx_skb_lock);
+
+	while (tc6->int_flag ||
+	       (tc6->waiting_tx_skb && tc6->tx_credits)) {
+		ret = oa_tc6_try_spi_transfer(tc6);
+		if (ret) {
+			disable_irq_nosync(tc6->spi->irq);
+			oa_tc6_disable_traffic(tc6);
+			break;
 		}
 	}
 
@@ -1250,18 +1284,22 @@ EXPORT_SYMBOL_GPL(oa_tc6_zero_align_receive_frame_enable);
  */
 netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
 {
-	if (tc6->disable_traffic || tc6->waiting_tx_skb) {
-		netif_stop_queue(tc6->netdev);
-		return NETDEV_TX_BUSY;
-	}
-
 	if (skb_linearize(skb)) {
-		dev_kfree_skb_any(skb);
-		tc6->netdev->stats.tx_dropped++;
+		oa_tc6_drop_tx_skb(tc6, skb);
 		return NETDEV_TX_OK;
 	}
 
 	spin_lock_bh(&tc6->tx_skb_lock);
+	if (tc6->waiting_tx_skb) {
+		netif_stop_queue(tc6->netdev);
+		spin_unlock_bh(&tc6->tx_skb_lock);
+		return NETDEV_TX_BUSY;
+	}
+	if (tc6->disable_traffic) {
+		spin_unlock_bh(&tc6->tx_skb_lock);
+		oa_tc6_drop_tx_skb(tc6, skb);
+		return NETDEV_TX_OK;
+	}
 	tc6->waiting_tx_skb = skb;
 	spin_unlock_bh(&tc6->tx_skb_lock);
 
@@ -1393,7 +1431,9 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
  */
 void oa_tc6_exit(struct oa_tc6 *tc6)
 {
+	spin_lock_bh(&tc6->tx_skb_lock);
 	tc6->disable_traffic = true;
+	spin_unlock_bh(&tc6->tx_skb_lock);
 	disable_irq(tc6->spi->irq);
 	oa_tc6_phy_exit(tc6);
 	oa_tc6_free_pending_skbs(tc6);

-- 
2.43.0

[PATCH net v3 2/3] net: ethernet: oa_tc6: Improvement in buffer overflow handling

[Selvamani Rajagopal via B4 Relay]

From: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

When oversubscribed traffic causes lot of buffer overflow errors,
probably due to loss of data chunks, driver fails to find a
data chunk with end_valid bit set, before it runs out of sk buffer
space. As a result, assert is seen during skb_put.

Now check is made if tail + len > end, driver abandons the current
data and starts look for a data chunk with start_valid bit,
that is a new frame.

Fixes: d70a0d8f2f2d ("net: ethernet: oa_tc6: implement receive path to receive rx ethernet frames")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

---
changes in v3
  - Continue processing more chunks on error code -EAGAIN. Previously
    we were bailing out.
changes in v2
  - Check rx_skb pointer before new allocation and NULL before use.
---
 drivers/net/ethernet/oa_tc6.c | 113 ++++++++++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 5b24cce4f9b5..a6b8762f6052 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -710,6 +710,12 @@ static void oa_tc6_free_pending_skbs(struct oa_tc6 *tc6)
 	oa_tc6_cleanup_waiting_tx_skb(tc6);
 }
 
+static void oa_tc6_look_for_new_frame(struct oa_tc6 *tc6)
+{
+	tc6->rx_buf_overflow = true;
+	oa_tc6_cleanup_ongoing_rx_skb(tc6);
+}
+
 /* If the failure is at SPI interface level, masking and clearing
  * the interrupt of the device won't work. Since SPI interrupt is
  * disabled, it should stop the repeated interrupts.
@@ -753,8 +759,7 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
 	}
 
 	if (FIELD_GET(STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
-		tc6->rx_buf_overflow = true;
-		oa_tc6_cleanup_ongoing_rx_skb(tc6);
+		oa_tc6_look_for_new_frame(tc6);
 		net_err_ratelimited("%s: Receive buffer overflow error\n",
 				    tc6->netdev->name);
 		return -EAGAIN;
@@ -780,6 +785,8 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
 
 static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
 {
+	int ret = 0;
+
 	/* Process rx chunk footer for the following,
 	 * 1. tx credits
 	 * 2. errors if any from MAC-PHY
@@ -790,9 +797,11 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
 					     footer);
 
 	if (FIELD_GET(OA_TC6_DATA_FOOTER_EXTENDED_STS, footer)) {
-		int ret = oa_tc6_process_extended_status(tc6);
-
-		if (ret)
+		ret = oa_tc6_process_extended_status(tc6);
+		/* EAGAIN error is recoverable. Move on to check
+		 * HEADER and SYNC errors before returning.
+		 */
+		if (ret && ret != -EAGAIN)
 			return ret;
 	}
 
@@ -810,7 +819,7 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
 		return -ENODEV;
 	}
 
-	return 0;
+	return ret;
 }
 
 static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
@@ -835,13 +844,35 @@ static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
 	tc6->rx_skb = NULL;
 }
 
-static void oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u8 length)
+/* On oversubscribed traffic condition, particularly with overwhelming rx
+ * buffer overflow errors, there could be data chunk loss. If tail + length
+ * goes beyond end pointer, that is an indication that the data chunk with
+ * end_valid bit is lost. Time to look for a data chunk with start_valid bit.
+ *
+ * If rx_skb is NULL, it is time to start looking for data chunk with
+ * start_bit.
+ */
+static int oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u8 length)
 {
+	if (!tc6->rx_skb ||
+	    (tc6->rx_skb->tail + length) > tc6->rx_skb->end) {
+		oa_tc6_look_for_new_frame(tc6);
+		return -EAGAIN;
+	}
+
 	memcpy(skb_put(tc6->rx_skb, length), payload, length);
+	return 0;
 }
 
+/* On overwhelming rx buffer overflow errors, due to data chunk loss, it is
+ * possible that we get two data chunks with start_valid bit set, without
+ * end_valid bit set in between. In this case, rx_skb would have a valid
+ * buffer pointer. We should release, if a valid pointer is found before
+ * allocating a new one.
+ */
 static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
 {
+	oa_tc6_cleanup_ongoing_rx_skb(tc6);
 	tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu +
 						ETH_HLEN + ETH_FCS_LEN);
 	if (!tc6->rx_skb) {
@@ -861,7 +892,9 @@ static int oa_tc6_prcs_complete_rx_frame(struct oa_tc6 *tc6, u8 *payload,
 	if (ret)
 		return ret;
 
-	oa_tc6_update_rx_skb(tc6, payload, size);
+	ret = oa_tc6_update_rx_skb(tc6, payload, size);
+	if (ret)
+		return ret;
 
 	oa_tc6_submit_rx_skb(tc6);
 
@@ -876,22 +909,24 @@ static int oa_tc6_prcs_rx_frame_start(struct oa_tc6 *tc6, u8 *payload, u16 size)
 	if (ret)
 		return ret;
 
-	oa_tc6_update_rx_skb(tc6, payload, size);
-
-	return 0;
+	return oa_tc6_update_rx_skb(tc6, payload, size);
 }
 
-static void oa_tc6_prcs_rx_frame_end(struct oa_tc6 *tc6, u8 *payload, u16 size)
+static int oa_tc6_prcs_rx_frame_end(struct oa_tc6 *tc6, u8 *payload, u16 size)
 {
-	oa_tc6_update_rx_skb(tc6, payload, size);
+	int ret;
 
-	oa_tc6_submit_rx_skb(tc6);
+	ret = oa_tc6_update_rx_skb(tc6, payload, size);
+	if (!ret)
+		oa_tc6_submit_rx_skb(tc6);
+	return ret;
 }
 
-static void oa_tc6_prcs_ongoing_rx_frame(struct oa_tc6 *tc6, u8 *payload,
-					 u32 footer)
+static int oa_tc6_prcs_ongoing_rx_frame(struct oa_tc6 *tc6, u8 *payload,
+					u32 footer)
 {
-	oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);
+	return oa_tc6_update_rx_skb(tc6, payload,
+				    OA_TC6_CHUNK_PAYLOAD_SIZE);
 }
 
 static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
@@ -931,8 +966,7 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
 	/* Process the chunk with only rx frame end */
 	if (end_valid && !start_valid) {
 		size = end_byte_offset + 1;
-		oa_tc6_prcs_rx_frame_end(tc6, data, size);
-		return 0;
+		return oa_tc6_prcs_rx_frame_end(tc6, data, size);
 	}
 
 	/* Process the chunk with previous rx frame end and next rx frame
@@ -954,9 +988,7 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
 	}
 
 	/* Process the chunk with ongoing rx frame data */
-	oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);
-
-	return 0;
+	return oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);
 }
 
 static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
@@ -972,8 +1004,9 @@ static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
 static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
 {
 	u16 no_of_rx_chunks = length / OA_TC6_CHUNK_SIZE;
+	bool retry = false;
+	int ret = 0;
 	u32 footer;
-	int ret;
 
 	/* All the rx chunks in the receive SPI data buffer are examined here */
 	for (int i = 0; i < no_of_rx_chunks; i++) {
@@ -982,8 +1015,11 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
 						    OA_TC6_CHUNK_PAYLOAD_SIZE);
 
 		ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
-		if (ret)
-			return ret;
+		if (ret) {
+			if (ret != -EAGAIN)
+				return ret;
+			retry = true;
+		}
 
 		/* If there is a data valid chunks then process it for the
 		 * information needed to determine the validity and the location
@@ -995,12 +1031,25 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
 
 			ret = oa_tc6_prcs_rx_chunk_payload(tc6, payload,
 							   footer);
-			if (ret)
-				return ret;
+			if (ret) {
+				if (ret != -ENOMEM && ret != -EAGAIN)
+					return ret;
+				retry = true;
+			}
 		}
 	}
 
-	return 0;
+	/* As a recoverable error, not bailing out on error code
+	 * -EAGAIN. If subsequent loop iterations, if any, succeeds,
+	 * error code would be overwritten. retry flag helps to
+	 * make the caller to continue and retry.
+	 */
+	if (retry) {
+		ret = -EAGAIN;
+		oa_tc6_look_for_new_frame(tc6);
+	}
+
+	return ret;
 }
 
 static __be32 oa_tc6_prepare_data_header(bool data_valid, bool start_valid,
@@ -1162,10 +1211,12 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
 		}
 
 		ret = oa_tc6_process_spi_data_rx_buf(tc6, spi_len);
-		if (ret) {
-			if (ret == -EAGAIN)
-				continue;
 
+		/* Not continuing with the next iteration to give
+		 * waiting_tx_skb a chance to get drained, if
+		 * needed.
+		 */
+		if (ret && ret != -EAGAIN) {
 			oa_tc6_free_ongoing_skbs(tc6);
 			netdev_err(tc6->netdev, "Device error: %d\n", ret);
 			return ret;

-- 
2.43.0

Re: [PATCH net v3 2/3] net: ethernet: oa_tc6: Improvement in buffer overflow handling

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: ethernet: oa_tc6: Improvement in buffer overflow handling

This patch teaches the oa_tc6 rx path to detect when tail + length would
exceed skb->end, abandon the current rx_skb, and resume on the next
data chunk carrying a start_valid bit. It also propagates -EAGAIN as a
recoverable error through the chunk/footer processing and the outer
SPI transfer loop.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 5b24cce4f9b5..a6b8762f6052 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -931,8 +966,7 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
>  	/* Process the chunk with only rx frame end */
>  	if (end_valid && !start_valid) {
>  		size = end_byte_offset + 1;
> -		oa_tc6_prcs_rx_frame_end(tc6, data, size);
> -		return 0;
> +		return oa_tc6_prcs_rx_frame_end(tc6, data, size);
>  	}
>  
>  	/* Process the chunk with previous rx frame end and next rx frame

[High]
Now that oa_tc6_prcs_rx_frame_end() returns int (and can return -EAGAIN
via oa_tc6_update_rx_skb() when tail + size > end), should the
"previous rx frame end + next rx frame start" branch also observe the
return value?  That branch is not shown in the diff, but in the
resulting file it reads:

    if (start_valid && end_valid && start_byte_offset > end_byte_offset) {
        if (tc6->rx_skb) {
            size = end_byte_offset + 1;
            oa_tc6_prcs_rx_frame_end(tc6, data, size);
        }
        size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
        return oa_tc6_prcs_rx_frame_start(tc6,
                                          &data[start_byte_offset],
                                          size);
    }

If oa_tc6_prcs_rx_frame_end() here returns -EAGAIN, oa_tc6_look_for_new_frame()
has already set tc6->rx_buf_overflow = true and cleared rx_skb.  Execution
then falls through to oa_tc6_prcs_rx_frame_start(), which allocates a
fresh rx_skb and copies the start bytes, but tc6->rx_buf_overflow stays
true because the guard

    if (start_valid && tc6->rx_buf_overflow)
        tc6->rx_buf_overflow = false;

is only evaluated on chunk entry.  Subsequent ongoing chunks (no
start_valid) then hit the earlier

    if (tc6->rx_buf_overflow)
        return 0;

path and are silently dropped, and the partial rx_skb is later thrown
away by the next allocate_rx_skb() -> cleanup_ongoing_rx_skb(), which
also increments rx_dropped.  Is this the intended behaviour for the
oversubscribed/chunk-loss scenario the patch is targeting?

[ ... ]

> @@ -972,8 +1004,9 @@ static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
>  static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  {
>  	u16 no_of_rx_chunks = length / OA_TC6_CHUNK_SIZE;
> +	bool retry = false;
> +	int ret = 0;
>  	u32 footer;
> -	int ret;
>  
>  	/* All the rx chunks in the receive SPI data buffer are examined here */
>  	for (int i = 0; i < no_of_rx_chunks; i++) {
> @@ -982,8 +1015,11 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  						    OA_TC6_CHUNK_PAYLOAD_SIZE);
>  
>  		ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			if (ret != -EAGAIN)
> +				return ret;
> +			retry = true;
> +		}
>  
>  		/* If there is a data valid chunks then process it for the
>  		 * information needed to determine the validity and the location
> @@ -995,12 +1031,25 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  
>  			ret = oa_tc6_prcs_rx_chunk_payload(tc6, payload,
>  							   footer);
> -			if (ret)
> -				return ret;
> +			if (ret) {
> +				if (ret != -ENOMEM && ret != -EAGAIN)
> +					return ret;
> +				retry = true;
> +			}
>  		}
>  	}

[Medium]
Is it intentional that a real -ENOMEM from oa_tc6_prcs_rx_chunk_payload()
(originating in oa_tc6_allocate_rx_skb() -> netdev_alloc_skb_ip_align())
is now folded into the retry path and converted to -EAGAIN at the end
of this function?

Before this patch, -ENOMEM propagated up through oa_tc6_try_spi_transfer()
and produced a netdev_err("Device error: %d", ret) plus
oa_tc6_free_ongoing_skbs() cleanup.  With this change, the outer while
loop treats -EAGAIN as non-fatal, so sustained memory exhaustion is
reported only as increments to stats.rx_dropped from
oa_tc6_allocate_rx_skb().

The commit message describes the change as being about buffer overflow
errors; is the change in visibility for allocation failures intended?

>  
> -	return 0;
> +	/* As a recoverable error, not bailing out on error code
> +	 * -EAGAIN. If subsequent loop iterations, if any, succeeds,
> +	 * error code would be overwritten. retry flag helps to
> +	 * make the caller to continue and retry.
> +	 */
> +	if (retry) {
> +		ret = -EAGAIN;
> +		oa_tc6_look_for_new_frame(tc6);
> +	}
> +
> +	return ret;
>  }

[High]
Can this unconditional oa_tc6_look_for_new_frame() discard a frame that
was successfully started later in the same buffer?

Consider a buffer where iteration N returns -EAGAIN from
oa_tc6_process_rx_chunk_footer() due to STATUS0 rx overflow (which
already sets rx_buf_overflow and clears rx_skb), and a later iteration
N+K carries a start_valid chunk that clears rx_buf_overflow via

    if (start_valid && tc6->rx_buf_overflow)
        tc6->rx_buf_overflow = false;

allocates a fresh rx_skb in oa_tc6_prcs_rx_frame_start(), and writes
the start bytes without further error.  retry is still true from
iteration N, so on loop exit the terminal oa_tc6_look_for_new_frame()
frees that newly-started rx_skb (incrementing rx_dropped) and re-sets
rx_buf_overflow = true.

On the next SPI transfer, the ongoing chunks of that frame are then
dropped by the top-of-function

    if (tc6->rx_buf_overflow)
        return 0;

guard in oa_tc6_prcs_rx_chunk_payload() until yet another start_valid
arrives.  Would it be safer to only call oa_tc6_look_for_new_frame()
here if the last iteration was still in an errored state, e.g. by
tracking the outcome of the most recent chunk rather than an aggregate
retry flag?

[ ... ]

RE: [PATCH net v3 2/3] net: ethernet: oa_tc6: Improvement in buffer overflow handling

[Selvamani Rajagopal]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, July 10, 2026 7:20 AM
> To: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
> Cc: Simon Horman <horms@kernel.org>; parthiban.veerasooran@microchip.com;

> 
> [High]
> Now that oa_tc6_prcs_rx_frame_end() returns int (and can return -EAGAIN
> via oa_tc6_update_rx_skb() when tail + size > end), should the
> "previous rx frame end + next rx frame start" branch also observe the
> return value? That branch is not shown in the diff, but in the
> resulting file it reads:


No need to check the return value. My comment below gives the reason.

> 
> If oa_tc6_prcs_rx_frame_end() here returns -EAGAIN, oa_tc6_look_for_new_frame()
> has already set tc6->rx_buf_overflow = true and cleared rx_skb. Execution
> then falls through to oa_tc6_prcs_rx_frame_start(), which allocates a
> fresh rx_skb and copies the start bytes, but tc6->rx_buf_overflow stays
> true because the guard

Yes. This is an issue. I will clear the rx_buf_overflow to false as the condition
for clearing is to see a start_valid bit (a.k.a new frame). And we have a new frame 
to process at this stage.


> 
> [Medium]
> Is it intentional that a real -ENOMEM from oa_tc6_prcs_rx_chunk_payload()
> (originating in oa_tc6_allocate_rx_skb() -> netdev_alloc_skb_ip_align())
> is now folded into the retry path and converted to -EAGAIN at the end
> of this function?


Yes. It is intentional. Code changes attempt to recover from either ENOMEM or 
EAGAIN. Recovery actions on both errors, are same. I will add some more comment 
to make it clear.


> 
> Before this patch, -ENOMEM propagated up through oa_tc6_try_spi_transfer()
> and produced a netdev_err("Device error: %d", ret) plus
> oa_tc6_free_ongoing_skbs() cleanup. With this change, the outer while
> loop treats -EAGAIN as non-fatal, so sustained memory exhaustion is
> reported only as increments to stats.rx_dropped from
> oa_tc6_allocate_rx_skb().
> 
> The commit message describes the change as being about buffer overflow
> errors; is the change in visibility for allocation failures intended?


Maybe I will change the commit message as "improvement in error recovery"


> > }
> 
> [High]
> Can this unconditional oa_tc6_look_for_new_frame() discard a frame that
> was successfully started later in the same buffer?
> 
> guard in oa_tc6_prcs_rx_chunk_payload() until yet another start_valid
> arrives. Would it be safer to only call oa_tc6_look_for_new_frame()
> here if the last iteration was still in an errored state, e.g. by
> tracking the outcome of the most recent chunk rather than an aggregate
> retry flag?

I don't think there is no "best method" to solve this issue. But I am open to 
suggestions. In my view, we may have many data chunks with buffer overflow 
set, not just one or two. It is hard to tell whether every chunk with overflow error 
has good (or bad) data. 

Code treats the data chunk as good if there is no "bad header" bit set, and "data valid"
bit is set, which I think is fair.

Here are the choices I see

1) Original code: Discarded all the received chunks once we see overflow error.
2) Current change: Let the loop recover as many frames as possible from
    the data chunks received. At the end of the loop, we trash what is left. Yes. It is 
    possible that we may trash a last one that may be a good one.
3) What Sashiko says: Ignore chunk errors in the middle or beginning of the loop.
    Just worry about the recent chunk with error. 

I don't think we can accurately tell whether 2 is better or 3. It may entirely depend on
how many chunks are set with overflow error it at any given time. 

I can change the Implementation if someone thinks #3 is better.

> 
> [ ... ]

[PATCH net v3 3/3] net: ethernet: oa_tc6: Carrier off when disable_traffic is set

[Selvamani Rajagopal via B4 Relay]

From: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Previously, TX queue interface was stopped when
disable_traffic flag was set. It is more appropriate to
disable the queue as there is no recovery, once
disable_traffic is set. Carrier is also marked off

Fixes: b542d13fab0f ("net: ethernet: oa_tc6: Interrupt is active low, level triggered.")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

changes in v3
  - New patch. Carrier marked off once disable_traffic is set
---
 drivers/net/ethernet/oa_tc6.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index a6b8762f6052..1f8564779025 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -730,6 +730,12 @@ static void oa_tc6_disable_traffic(struct oa_tc6 *tc6)
 	skb = oa_tc6_detach_waiting_tx_skb(tc6);
 	spin_unlock_bh(&tc6->tx_skb_lock);
 
+	/* disable_traffic, when set, is a point of no
+	 * return to working state. Better to mark
+	 * the carrier off.
+	 */
+	netif_carrier_off(tc6->netdev);
+	netif_tx_disable(tc6->netdev);
 	oa_tc6_drop_tx_skb(tc6, skb);
 	oa_tc6_free_ongoing_skbs(tc6);
 	oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);

-- 
2.43.0