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

[PATCH net v5 0/4] 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 v5:
- Fixed typo in commit messages.
- Changed the commit message to be more accurate.
- Fixed the wrong data type used.
- Link to v4: https://lore.kernel.org/r/20260720-fix-race-condition-and-crash-v4-0-8273e2f38a1f@onsemi.com

Changes in v4:
  - As disable_traffic means device is uselss unless
    driver re-loaded, all tx queues are turned off.
  - Process all the received chunks on buffer overflow,
    as long as data chunks doesn't have any error bits set
    in their footer.
  - Added spin lock protection in every place wait_tx_skb
    is used.
  - Carrier is not turned off on disable_traffic.

- Link to v3: https://lore.kernel.org/r/20260705-fix-race-condition-and-crash-v3-0-3e51841e4d08@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 (4):
      net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances
      net: ethernet: oa_tc6: Improve the error recovery
      net: ethernet: oa_tc6: Disable tx queues on fatal error
      net: ethernet: oa_tc6: Fix for the wrong data type

 drivers/net/ethernet/oa_tc6.c | 238 +++++++++++++++++++++++++++++++-----------
 1 file changed, 176 insertions(+), 62 deletions(-)
---
base-commit: 54ad7ea45d63146a8e3c57375f8a269d4cf7ecea
change-id: 20260621-fix-race-condition-and-crash-94d055a665c4

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

[PATCH net v5 1/4] 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 transmit 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 v5
  - Fixed the typo in commit message
changes in v4
  - No change
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 v5 2/4] net: ethernet: oa_tc6: Improve the error recovery

[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.

SK buffer allocation error is considered as recoverable error.

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 v5
  - Changed the summary to match the convention
  - Added more clarity to why the return code is not checked for
    the call to oa_tc6_prcs_rx_frame_end
changes in v4
  - rx_buf_overflow flag cleared, when end of frame and start of
    frame are handled in the same data chunk.
  - Added more comments to answer some of the AI review questions.
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 | 132 ++++++++++++++++++++++++++++++++----------
 1 file changed, 101 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 5b24cce4f9b5..421f72e6f71e 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
@@ -946,6 +980,15 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
 		if (tc6->rx_skb) {
 			size = end_byte_offset + 1;
 			oa_tc6_prcs_rx_frame_end(tc6, data, size);
+
+			/* Return value from oa_tc6_prcs_rx_frame_end is not
+			 * checked. If it returned an error, it is to make
+			 * the code to look for new frame. At this stage,
+			 * code below is going to process a new frame. So,
+			 * error condition is set to false, in case it is
+			 * set before proceeding.
+			 */
+			tc6->rx_buf_overflow = false;
 		}
 		size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
 		return oa_tc6_prcs_rx_frame_start(tc6,
@@ -954,9 +997,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 +1013,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 +1024,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 +1040,35 @@ 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;
+	/* Not bailing out on recoverable error codes, -EAGAIN and
+	 * -ENOMEM. If subsequent loop iterations, if any, succeeds,
+	 * error code would be overwritten. retry flag helps to
+	 * make the caller to continue and retry. Since recovery
+	 * action for -ENOMEM and -EAGAIN are same, we are returning
+	 * one of the error codes, that is -EAGAIN.
+	 *
+	 * Successful recovery depends on how small the frames are,
+	 * how many chunks, among the received chunks triggered the
+	 * error, whether data is intact even with error conditions.
+	 * As a result, there is no single, best method to recover
+	 * most data when error conditions hit. We do our best by
+	 * processing all the chunks with good "footer header" and
+	 * "data valid" bit set.
+	 */
+	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 +1230,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

[PATCH net v5 3/4] net: ethernet: oa_tc6: Disable tx queues on fatal error

[Selvamani Rajagopal via B4 Relay]

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

Previously, TX queue interface was stopped when
disable_traffic flag was set, which would indicate fatal
error. It is more appropriate to disable the queue as,
unless driver is unloaded and reloaded, there is no recovery
after disable_traffic is set.

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

---
changes in v5
  - Changed the commit message to accurately reflect the
    changes
changes in v4
  - Reverted the change that turned carrier off on disable_traffic,
    as it may have side effects
changes in v3
  - New patch. Carrier marked off once disable_traffic is set
---
 drivers/net/ethernet/oa_tc6.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 421f72e6f71e..bb644509c32b 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -730,6 +730,10 @@ 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. TX queues are disabled.
+	 */
+	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

[PATCH net v5 4/4] net: ethernet: oa_tc6: Fix for the wrong data type

[Selvamani Rajagopal via B4 Relay]

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

Inadvertently bool data type is used where int is supposed to
be used. This might turn a negative error code into true or
false and sign of the return code would be lost.

Fixes: 8f9bf857e43b ("net: ethernet: oa_tc6: implement internal PHY initialization")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

---
changes in v5
  - New patch. Fixed the wrong data type used.
---
 drivers/net/ethernet/oa_tc6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index bb644509c32b..2ec22ba3ccba 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -423,7 +423,7 @@ static int oa_tc6_mdiobus_read(struct mii_bus *bus, int addr, int regnum)
 {
 	struct oa_tc6 *tc6 = bus->priv;
 	u32 regval;
-	bool ret;
+	int ret;
 
 	ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
 				   (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),

-- 
2.43.0

Re: [PATCH net v5 1/4] net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

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/
---
--- Patch [1]: [PATCH net v5 1/4] net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances ---
commit e4a0f5cf54cb40242b52216e184b30bf2c9fff57
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances

This patch puts tc6->waiting_tx_skb accesses under the existing
tx_skb_lock, adds the helpers oa_tc6_drop_tx_skb() and
oa_tc6_detach_waiting_tx_skb(), and also brings tc6->disable_traffic
under the same lock in oa_tc6_disable_traffic(), oa_tc6_exit() and the
threaded IRQ handler.  A few questions below.

>     net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances
>
>     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 transmit path, near skb_linearize.

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
Should the changelog mention the other behavioural changes that come with
this fix?  The message only describes adding mutual exclusion for
waiting_tx_skb, but the patch also:

 - changes oa_tc6_start_xmit() so the disable_traffic case drops the frame
   and returns NETDEV_TX_OK instead of netif_stop_queue() plus
   NETDEV_TX_BUSY,
 - moves skb_linearize() ahead of the busy/disabled checks,
 - brings disable_traffic under tx_skb_lock in oa_tc6_exit() and in
   oa_tc6_macphy_threaded_irq(),
 - converts the tx skb free from kfree_skb() to dev_kfree_skb_any() in the
   new oa_tc6_drop_tx_skb().

The exact interleaving that leaks the skb is also not spelled out.  Since
this carries a Fixes: tag and will be picked up for stable, could the
message describe the leaking sequence and the return-code change?  The
disable_traffic locking is only mentioned in the v3 changelog below the
cut, which is dropped when the patch is applied.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 0727d53345a33..5b24cce4f9b56 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) {

[ ... ]

> @@ -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);

[ ... ]

> @@ -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);

[Severity: Low]
[Finding: fable-5-5-19]
[Sources: fable-5, opus-5]
Is tx_skb_lock the right lock for tc6->disable_traffic?

The writers in oa_tc6_disable_traffic() and oa_tc6_exit() and this reader
now take a spin_lock_bh() lock, but the primary hard IRQ handler still
reads the same bool with a plain load and cannot take a BH lock:

drivers/net/ethernet/oa_tc6.c:oa_tc6_macphy_isr() {
	...
	if (tc6->disable_traffic)
		disable_irq_nosync(tc6->spi->irq);
	else
		tc6->int_flag = true;
	...
}

In oa_tc6_exit() the store happens under the lock and disable_irq() only
runs afterwards:

	spin_lock_bh(&tc6->tx_skb_lock);
	tc6->disable_traffic = true;
	spin_unlock_bh(&tc6->tx_skb_lock);
	disable_irq(tc6->spi->irq);

Can a level triggered interrupt in that window make oa_tc6_macphy_isr()
observe a stale false, set int_flag and wake the thread, which then takes
the early return above without doing the data chunk transfer that
deasserts INT?

Also, is taking the lock here worth it when it is dropped again before the
value is used?  Would READ_ONCE()/WRITE_ONCE() on the flag (or an
IRQ-safe lock) express the actual access rules more accurately than a BH
lock that the hardirq reader can never hold?

>  
> +	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;
> +	}

[Severity: Low]
[Finding: fable-5-1-5]
[Sources: fable-5, opus-5]
Does moving skb_linearize() in front of the waiting_tx_skb check change the
back-pressure policy?

Previously an xmit that hit the busy case returned NETDEV_TX_BUSY without
touching the skb.  Now every xmit linearizes first, so a transient
skb_linearize() -ENOMEM during a busy period frees the frame and returns
NETDEV_TX_OK, where before it would have been requeued and retried.

The kernel-doc above oa_tc6_start_xmit() still says:

 * Return: NETDEV_TX_OK if the transmit ethernet frame skb added in the tx_skb_q
 * otherwise returns NETDEV_TX_BUSY.

which no longer covers the drop cases.  Should that be refreshed too?

> +	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);
-- 
This is an AI-generated review.

Re: [PATCH net v5 2/4] net: ethernet: oa_tc6: Improve the error recovery

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

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/
---
--- Patch [2]: [PATCH net v5 2/4] net: ethernet: oa_tc6: Improve the error recovery ---
commit f8f47e53b81fcb2cf25c3e00de04be99437717cf
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Improve the error recovery

This patch adds a tailroom check before skb_put() in the oa_tc6 receive
path, turns the rx skb update helpers into functions returning an error
code, and makes oa_tc6_process_spi_data_rx_buf() continue over
recoverable (-EAGAIN/-ENOMEM) errors instead of bailing out on the first
one.  A new helper oa_tc6_look_for_new_frame() bundles setting
rx_buf_overflow with dropping the in-progress rx skb.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 5b24cce4f9b56..421f72e6f71e3 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);
> +}
> +

[Severity: Low]
[Finding: main-1-2]
[Sources: opus-5]
Should the rx_buf_overflow field keep that name after this change?

The flag is now also set for conditions that are not MAC-PHY receive
buffer overflows: exhausted skb tailroom and rx_skb == NULL in
oa_tc6_update_rx_skb(), plus the blanket per-buffer retry at the end of
oa_tc6_process_spi_data_rx_buf(), which also covers -ENOMEM from
netdev_alloc_skb_ip_align().

Would a rename (something like rx_resync or skip_until_frame_start), or a
second flag, keep the two conditions distinguishable when debugging?

[ ... ]

> @@ -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;
>  }

[Severity: Low]
[Finding: main-1-1]
[Sources: opus-5]
This isn't a bug, but could this use the documented accessor instead of
reaching into the sk_buff layout fields?

	if (!tc6->rx_skb || skb_tailroom(tc6->rx_skb) < length)

The arithmetic here does match the overflow test inside skb_put() for both
representations selected by NET_SKBUFF_DATA_USES_OFFSET, but skb->tail and
skb->end are internal fields, and skb_tailroom() in
include/linux/skbuff.h additionally returns 0 for non-linear skbs.

[ ... ]

> @@ -946,6 +980,15 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
>  		if (tc6->rx_skb) {
>  			size = end_byte_offset + 1;
>  			oa_tc6_prcs_rx_frame_end(tc6, data, size);
> +
> +			/* Return value from oa_tc6_prcs_rx_frame_end is not
> +			 * checked. If it returned an error, it is to make
> +			 * the code to look for new frame. At this stage,
> +			 * code below is going to process a new frame. So,
> +			 * error condition is set to false, in case it is
> +			 * set before proceeding.
> +			 */
> +			tc6->rx_buf_overflow = false;
>  		}
>  		size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
>  		return oa_tc6_prcs_rx_frame_start(tc6,

[ ... ]

> @@ -972,8 +1013,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 +1024,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;
> +		}

[Severity: Medium]
[Finding: main-2-5]
[Sources: opus-5]
Since the loop no longer returns on the first -EAGAIN, how many STATUS0
control transfers does one buffer now cost?

Every remaining footer in the same buffer that has
OA_TC6_DATA_FOOTER_EXTENDED_STS set re-enters
oa_tc6_process_extended_status(), which does a blocking
oa_tc6_read_register(OA_TC6_REG_STATUS0) plus an
oa_tc6_write_register() write-back, each taking spi_ctrl_lock, even
though the first invocation already latched and cleared the error.

All footers of one buffer are sampled on the wire before any of them is
processed, so a single overflow event tends to leave the extended status
bit set in most footers of that buffer.  With OA_TC6_MAX_TX_CHUNKS = 48
that turns one read/write pair into up to 48 pairs of control transfers
in the SPI IRQ thread, before the received frames are handed to the
stack.

Does that delay the next data transfer that drains the MAC-PHY receive
buffer, and could it aggravate the overflow condition being recovered
from?  The commit message does not mention this side effect.

>  
>  		/* If there is a data valid chunks then process it for the
>  		 * information needed to determine the validity and the location
> @@ -995,12 +1040,35 @@ 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;
> +	/* Not bailing out on recoverable error codes, -EAGAIN and
> +	 * -ENOMEM. If subsequent loop iterations, if any, succeeds,
> +	 * error code would be overwritten. retry flag helps to
> +	 * make the caller to continue and retry. Since recovery
> +	 * action for -ENOMEM and -EAGAIN are same, we are returning
> +	 * one of the error codes, that is -EAGAIN.
> +	 *
> +	 * Successful recovery depends on how small the frames are,
> +	 * how many chunks, among the received chunks triggered the
> +	 * error, whether data is intact even with error conditions.
> +	 * As a result, there is no single, best method to recover
> +	 * most data when error conditions hit. We do our best by
> +	 * processing all the chunks with good "footer header" and
> +	 * "data valid" bit set.
> +	 */
> +	if (retry) {
> +		ret = -EAGAIN;
> +		oa_tc6_look_for_new_frame(tc6);
> +	}
> +
> +	return ret;
>  }

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
Can this drop a frame that was already successfully re-synchronized
inside the same buffer?

The retry flag is sticky, so oa_tc6_look_for_new_frame() runs at loop exit
even when a later chunk recovered.  Sequence:

  chunk i:  extended status set, STATUS0 reports rx buffer overflow
            oa_tc6_process_extended_status()
               oa_tc6_look_for_new_frame()   /* rx_skb freed,
                                                rx_buf_overflow = true */
               return -EAGAIN
            loop sets retry = true and keeps going

  chunk j:  start_valid, so oa_tc6_prcs_rx_chunk_payload() does
               if (start_valid && tc6->rx_buf_overflow)
                       tc6->rx_buf_overflow = false;
            oa_tc6_prcs_rx_frame_start() -> oa_tc6_allocate_rx_skb()
            allocates a fresh rx_skb, later "ongoing" chunks append fine

  loop exit: retry is still true
               ret = -EAGAIN;
               oa_tc6_look_for_new_frame(tc6);   /* frees the good
                                                    rx_skb, rx_dropped++,
                                                    rx_buf_overflow = true */

On the next SPI transfer the continuation and end chunks of that frame are
then discarded by:

	if (tc6->rx_buf_overflow)
		return 0;

until another start_valid chunk arrives.  rx_chunks_available is 5 bits
(max 31) while a 1514-byte frame needs roughly 24 chunks of 64 payload
bytes, so a frame still in progress at buffer end is a normal occurrence.

This also appears to undo the tc6->rx_buf_overflow = false added in the
end-plus-start-in-one-chunk branch of oa_tc6_prcs_rx_chunk_payload().

Is the trailing call needed for state consistency at all?  The overflow
path already sets the flag in oa_tc6_process_extended_status(), the
tailroom and NULL paths set it in oa_tc6_update_rx_skb(), and the only
remaining case, -ENOMEM from oa_tc6_allocate_rx_skb(), leaves rx_skb NULL
so the next mid-frame chunk hits the !tc6->rx_skb branch and sets the
flag anyway.

[ ... ]

> @@ -1162,10 +1230,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;

[Severity: Low]
[Finding: main-3-7]
[Sources: opus-5]
Does this comment match the resulting control flow?  The while (true) loop
does proceed to the next iteration on -EAGAIN.

What changed is that the continue was removed, so the tail of the loop
body is now reached on the recoverable-error path:

	if (!tc6->waiting_tx_skb && netif_queue_stopped(tc6->netdev))
		netif_wake_queue(tc6->netdev);

That code wakes the tx queue, which lets the stack queue new skbs, rather
than draining waiting_tx_skb.  Could the comment be reworded to describe
the wake-queue check being reached?
-- 
This is an AI-generated review.

Re: [PATCH net v5 3/4] net: ethernet: oa_tc6: Disable tx queues on fatal error

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

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/
---
--- Patch [3]: [PATCH net v5 3/4] net: ethernet: oa_tc6: Disable tx queues on fatal error ---
commit e2e6581c7bb33d3713f9dc8fab5b2d869c2c7d89
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Disable tx queues on fatal error

This adds a netif_tx_disable() call to oa_tc6_disable_traffic() so that
the TX queues are stopped when the disable_traffic flag is set, on the
grounds that there is no recovery from that state short of a driver
reload.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 421f72e6f71e3..bb644509c32b3 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -730,6 +730,10 @@ 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. TX queues are disabled.
> +	 */
> +	netif_tx_disable(tc6->netdev);

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
The comment describes this as "a point of no return to working state", but
netif_tx_disable() only sets the reversible per-queue __QUEUE_STATE_DRV_XOFF
bit.  Is that enough to make the state stick?

The only in-tree consumer clears it again on every open, without looking at
tc6->disable_traffic:

drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_net_open() {
	ret = lan865x_hw_enable(priv);
	...
	phy_start(netdev->phydev);

	netif_start_queue(netdev);
}

So after oa_tc6_disable_traffic() runs, an ordinary

	ip link set dev X down
	ip link set dev X up

re-enables the queues, while tc6->disable_traffic stays true (it is only
ever set, never cleared), the SPI interrupt stays disabled by
disable_irq_nosync() in the threaded IRQ handler and INT_MASK0 masks all
device interrupts.  Frames then land in the pre-existing drop branch:

drivers/net/ethernet/oa_tc6.c:oa_tc6_start_xmit() {
	if (tc6->disable_traffic) {
		spin_unlock_bh(&tc6->tx_skb_lock);
		oa_tc6_drop_tx_skb(tc6, skb);
		return NETDEV_TX_OK;
	}
}

Is an interface that looks operational but silently swallows every frame the
intended end state here?

There is also an ordering aspect: oa_tc6_init() arms the threaded IRQ with
devm_request_threaded_irq() and then calls irq_wake_thread(), which happens
before the consumer's register_netdev() in lan865x_probe().  If a fatal
error is detected during that window, the queue disable is applied to a
netdev that has not been registered yet and is then undone by the first
open.  Would netif_device_detach(), netif_carrier_off(), or having ndo_open
consult the tc6 fatal state express this more durably, or should the comment
and commit message be reworded to match what the code actually does?

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
A second question about the same call, regarding what userspace can observe.
The v4 changelog notes the carrier-off change was reverted, so the carrier
stays up while every txq has DRV_XOFF set.  With the queues stopped,
netif_xmit_stopped() is true and ndo_start_xmit is never called again, so the
per-frame accounting in the disable_traffic branch of oa_tc6_start_xmit():

	oa_tc6_drop_tx_skb(tc6, skb);	/* netdev->stats.tx_dropped++ */

becomes unreachable.  Packets are instead dropped at the qdisc, and qdisc
drops are not folded into rtnl_link_stats64.tx_dropped (only dev->stats and
dev->core_stats are; the enqueue-drop path only does qdisc_qstats_drop()).
Does that mean ip -s link reports zero errors and zero drops for a device
that is permanently dead?

lan865x_netdev_ops also registers no .ndo_tx_timeout:

drivers/net/ethernet/microchip/lan865x/lan865x.c:
static const struct net_device_ops lan865x_netdev_ops = {
	.ndo_open		= lan865x_net_open,
	.ndo_stop		= lan865x_net_close,
	.ndo_start_xmit		= lan865x_send_packet,
	...
};

so __netdev_watchdog_up() never arms dev_watchdog() and there is no
"transmit queue timed out" message either.  Link monitors and
carrier-driven aggregators such as bonding, team and bridge would still see
UP/LOWER_UP with no errors, and senders stall rather than getting drops.

Could the commit message document this loss of device-level drop accounting,
or could the dead state be made observable with netif_device_detach() /
netif_carrier_off() or another persistent error indication?
-- 
This is an AI-generated review.