Devicetree
 help / color / mirror / Atom feed
* [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt
@ 2026-06-09 19:50 Selvamani Rajagopal via B4 Relay
  2026-06-09 19:50 ` [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered Selvamani Rajagopal via B4 Relay
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Selvamani Rajagopal via B4 Relay @ 2026-06-09 19:50 UTC (permalink / raw)
  To: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Parthiban Veerasooran, Andrew Lunn, netdev, linux-kernel,
	Conor Dooley, devicetree, Piergiorgio Beruto, Selvamani Rajagopal

According to OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface
specification, MAC-PHY interrupt is "active low, level triggered".
The specification mentions about the conditions in which the IRQ
is asserted and deasserted.

Bug is inadvertently introduced by treating the IRQ in the OA TC6
framework driver and in dt-binding YAML file as edge triggered.

Changes are done in two files
 - OA TC6 framework Ethernet driver
 - YAML file for the vendor that already uses OA TC6 framework.

Maintainer for this driver is already informed and aware of these
changes. Testing for these changes was done in onsemi's setup and
found to be working.

To: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: Piergiorgio Beruto <pier.beruto@onsemi.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Conor Dooley <conor.dooley@microchip.com>
Cc: devicetree@vger.kernel.org
Cc: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Changes in v4:

- IRQ handler is changed to interrupt handler + wake up thread
  to interrupt handler + threaded irq. Threaded irq mechanism
  is better suited for level triggered interrupt. Because it can
  keep the interrupt disabled until interrupting conditions are 
  handled by a handler thread.
- SPI data handling function is called again on EAGAIN error code
  as it indicates RX buffer overflow error, which requires draining
  the bad data chunks.
 
  - Changed wakeup thread to threaded IRQ 
  - RX buffer overflow is handled before threaded irq returns

- Link to v3: https://lore.kernel.org/r/20260601-level-trigger-v3-0-da73e7010532@onsemi.com

---
Selvamani Rajagopal (4):
      net: ethernet: oa_tc6: Interrupt is active low, level triggered.
      net: ethernet: oa_tc6: No return on rx buffer overflow error
      net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
      dt-bindings: net: updated interrupt type to be active low, level triggered

 .../devicetree/bindings/net/microchip,lan8650.yaml |  2 +-
 drivers/net/ethernet/oa_tc6.c                      | 66 +++++++---------------
 2 files changed, 21 insertions(+), 47 deletions(-)
---
base-commit: 0aa05daef7848a5ac11158949dc73cd741995dc1
change-id: 20260531-level-trigger-8cb1a83af034

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
  2026-06-09 19:50 [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt Selvamani Rajagopal via B4 Relay
@ 2026-06-09 19:50 ` Selvamani Rajagopal via B4 Relay
  2026-06-10 19:50   ` sashiko-bot
  2026-06-09 19:50 ` [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error Selvamani Rajagopal via B4 Relay
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Selvamani Rajagopal via B4 Relay @ 2026-06-09 19:50 UTC (permalink / raw)
  To: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Parthiban Veerasooran, Andrew Lunn, netdev, linux-kernel,
	Conor Dooley, devicetree, Piergiorgio Beruto, Selvamani Rajagopal

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

According OPEN Alliance 10BASET1x MAC-PHY Serial Interface
specification, interrupt is active low, level triggered.

Fixes: 2c6ce5354453 ("net: ethernet: oa_tc6: implement mac-phy interrupt")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Changes in v4:
  Interrupt handling mechanism changed to threaded irq because it is
  best suited to handle level triggered interrupt. Threaded irq
  automatically blocks further interrupts until interrupting
  conditions are cleared by its handler.

Changes in v3:
 interrupt registered as level triggerred
---
 drivers/net/ethernet/oa_tc6.c | 61 ++++++++++++-------------------------------
 1 file changed, 17 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 91a906a7918a..e2c0843207f2 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/iopoll.h>
+#include <linux/interrupt.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
 #include <linux/oa_tc6.h>
@@ -121,8 +122,6 @@ struct oa_tc6 {
 	struct sk_buff *ongoing_tx_skb;
 	struct sk_buff *waiting_tx_skb;
 	struct sk_buff *rx_skb;
-	struct task_struct *spi_thread;
-	wait_queue_head_t spi_wq;
 	u16 tx_skb_offset;
 	u16 spi_data_tx_buf_offset;
 	u16 tx_credits;
@@ -1105,29 +1104,18 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
 	return 0;
 }
 
-static int oa_tc6_spi_thread_handler(void *data)
+static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
 {
 	struct oa_tc6 *tc6 = data;
-	int ret;
-
-	while (likely(!kthread_should_stop())) {
-		/* This kthread will be waken up if there is a tx skb or mac-phy
-		 * interrupt to perform spi transfer with tx chunks.
-		 */
-		wait_event_interruptible(tc6->spi_wq, tc6->int_flag ||
-					 (tc6->waiting_tx_skb &&
-					 tc6->tx_credits) ||
-					 kthread_should_stop());
-
-		if (kthread_should_stop())
-			break;
+	int ret = 0;
 
+	while (tc6->int_flag || (tc6->waiting_tx_skb && tc6->tx_credits)) {
 		ret = oa_tc6_try_spi_transfer(tc6);
 		if (ret)
-			return ret;
+			break;
 	}
 
-	return 0;
+	return IRQ_HANDLED;
 }
 
 static int oa_tc6_update_buffer_status_from_register(struct oa_tc6 *tc6)
@@ -1162,10 +1150,8 @@ static irqreturn_t oa_tc6_macphy_isr(int irq, void *data)
 	 * - extended status event not reported in the previous rx footer.
 	 */
 	tc6->int_flag = true;
-	/* Wake spi kthread to perform spi transfer */
-	wake_up_interruptible(&tc6->spi_wq);
-
-	return IRQ_HANDLED;
+	/* Wake IRQ thread to perform spi transfer */
+	return IRQ_WAKE_THREAD;
 }
 
 /**
@@ -1217,8 +1203,8 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
 	tc6->waiting_tx_skb = skb;
 	spin_unlock_bh(&tc6->tx_skb_lock);
 
-	/* Wake spi kthread to perform spi transfer */
-	wake_up_interruptible(&tc6->spi_wq);
+	/* Wake the threaded IRQ to perform spi transfer. */
+	irq_wake_thread(tc6->spi->irq, tc6);
 
 	return NETDEV_TX_OK;
 }
@@ -1311,24 +1297,15 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
 		goto phy_exit;
 	}
 
-	init_waitqueue_head(&tc6->spi_wq);
-
-	tc6->spi_thread = kthread_run(oa_tc6_spi_thread_handler, tc6,
-				      "oa-tc6-spi-thread");
-	if (IS_ERR(tc6->spi_thread)) {
-		dev_err(&tc6->spi->dev, "Failed to create SPI thread\n");
-		goto phy_exit;
-	}
-
-	sched_set_fifo(tc6->spi_thread);
-
-	ret = devm_request_irq(&tc6->spi->dev, tc6->spi->irq, oa_tc6_macphy_isr,
-			       IRQF_TRIGGER_FALLING, dev_name(&tc6->spi->dev),
-			       tc6);
+	ret = devm_request_threaded_irq(&tc6->spi->dev, tc6->spi->irq,
+					oa_tc6_macphy_isr,
+					oa_tc6_macphy_threaded_irq,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					dev_name(&tc6->spi->dev), tc6);
 	if (ret) {
 		dev_err(&tc6->spi->dev, "Failed to request macphy isr %d\n",
 			ret);
-		goto kthread_stop;
+		goto phy_exit;
 	}
 
 	/* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
@@ -1338,12 +1315,9 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
 	 * 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
 	 */
 	tc6->int_flag = true;
-	wake_up_interruptible(&tc6->spi_wq);
+	irq_wake_thread(tc6->spi->irq, tc6);
 
 	return tc6;
-
-kthread_stop:
-	kthread_stop(tc6->spi_thread);
 phy_exit:
 	oa_tc6_phy_exit(tc6);
 	return NULL;
@@ -1357,7 +1331,6 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
 void oa_tc6_exit(struct oa_tc6 *tc6)
 {
 	oa_tc6_phy_exit(tc6);
-	kthread_stop(tc6->spi_thread);
 	dev_kfree_skb_any(tc6->ongoing_tx_skb);
 	dev_kfree_skb_any(tc6->waiting_tx_skb);
 	dev_kfree_skb_any(tc6->rx_skb);

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error
  2026-06-09 19:50 [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt Selvamani Rajagopal via B4 Relay
  2026-06-09 19:50 ` [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered Selvamani Rajagopal via B4 Relay
@ 2026-06-09 19:50 ` Selvamani Rajagopal via B4 Relay
  2026-06-10 19:50   ` sashiko-bot
  2026-06-09 19:50 ` [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL Selvamani Rajagopal via B4 Relay
  2026-06-09 19:50 ` [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered Selvamani Rajagopal via B4 Relay
  3 siblings, 1 reply; 10+ messages in thread
From: Selvamani Rajagopal via B4 Relay @ 2026-06-09 19:50 UTC (permalink / raw)
  To: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Parthiban Veerasooran, Andrew Lunn, netdev, linux-kernel,
	Conor Dooley, devicetree, Piergiorgio Beruto, Selvamani Rajagopal

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

On RX buffer overflow error, the bad data chunks must
be thrown away and code should look for a data chunk with
"start valid" bit set, which would indicate next valid MAC
frame. In this change, oa_tc6_try_spi_transfer is called
again if it returns error code EAGAIN. This helps to
recover from the error gracefully.

Fixes: 2c6ce5354453 ("net: ethernet: oa_tc6: implement mac-phy interrupt")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Changes in v4:
   new patch
---
 drivers/net/ethernet/oa_tc6.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index e2c0843207f2..20c9e98d1d60 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -1111,6 +1111,8 @@ static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
 
 	while (tc6->int_flag || (tc6->waiting_tx_skb && tc6->tx_credits)) {
 		ret = oa_tc6_try_spi_transfer(tc6);
+		if (ret == -EAGAIN)
+			continue;
 		if (ret)
 			break;
 	}

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
  2026-06-09 19:50 [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt Selvamani Rajagopal via B4 Relay
  2026-06-09 19:50 ` [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered Selvamani Rajagopal via B4 Relay
  2026-06-09 19:50 ` [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error Selvamani Rajagopal via B4 Relay
@ 2026-06-09 19:50 ` Selvamani Rajagopal via B4 Relay
  2026-06-10 19:50   ` sashiko-bot
  2026-06-09 19:50 ` [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered Selvamani Rajagopal via B4 Relay
  3 siblings, 1 reply; 10+ messages in thread
From: Selvamani Rajagopal via B4 Relay @ 2026-06-09 19:50 UTC (permalink / raw)
  To: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Parthiban Veerasooran, Andrew Lunn, netdev, linux-kernel,
	Conor Dooley, devicetree, Piergiorgio Beruto, Selvamani Rajagopal

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

As "dev" pointer in oa_tc6 structure is never initialized,
mbiosub->parent was initialized with NULL.  This change
fixes the issue.

Fixes: 2c6ce5354453 ("net: ethernet: oa_tc6: implement mac-phy interrupt")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

Changes in v4:
   new patch
---
 drivers/net/ethernet/oa_tc6.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 20c9e98d1d60..0710a5a8dcde 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -108,7 +108,6 @@
 
 /* Internal structure for MAC-PHY drivers */
 struct oa_tc6 {
-	struct device *dev;
 	struct net_device *netdev;
 	struct phy_device *phydev;
 	struct mii_bus *mdiobus;
@@ -517,7 +516,7 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
 	tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
 	tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;
 	tc6->mdiobus->name = "oa-tc6-mdiobus";
-	tc6->mdiobus->parent = tc6->dev;
+	tc6->mdiobus->parent = &tc6->spi->dev;
 
 	snprintf(tc6->mdiobus->id, ARRAY_SIZE(tc6->mdiobus->id), "%s",
 		 dev_name(&tc6->spi->dev));

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered
  2026-06-09 19:50 [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt Selvamani Rajagopal via B4 Relay
                   ` (2 preceding siblings ...)
  2026-06-09 19:50 ` [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL Selvamani Rajagopal via B4 Relay
@ 2026-06-09 19:50 ` Selvamani Rajagopal via B4 Relay
  2026-06-10  7:22   ` Krzysztof Kozlowski
  3 siblings, 1 reply; 10+ messages in thread
From: Selvamani Rajagopal via B4 Relay @ 2026-06-09 19:50 UTC (permalink / raw)
  To: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Parthiban Veerasooran, Andrew Lunn, netdev, linux-kernel,
	Conor Dooley, devicetree, Piergiorgio Beruto, Selvamani Rajagopal

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

According to OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6)
specification, interrupt type is active low, level triggered interrupt.

Fixes: ac49b950bea9 ("dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY")
Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

changes in v4:
  no change
changes in v3
  interrupts entry changed to level triggered from edge triggered
---
 Documentation/devicetree/bindings/net/microchip,lan8650.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/microchip,lan8650.yaml b/Documentation/devicetree/bindings/net/microchip,lan8650.yaml
index 61e11d4a07c4..766ff58147ae 100644
--- a/Documentation/devicetree/bindings/net/microchip,lan8650.yaml
+++ b/Documentation/devicetree/bindings/net/microchip,lan8650.yaml
@@ -67,7 +67,7 @@ examples:
         pinctrl-names = "default";
         pinctrl-0 = <&eth0_pins>;
         interrupt-parent = <&gpio>;
-        interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+        interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
         local-mac-address = [04 05 06 01 02 03];
         spi-max-frequency = <15000000>;
       };

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered
  2026-06-09 19:50 ` [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered Selvamani Rajagopal via B4 Relay
@ 2026-06-10  7:22   ` Krzysztof Kozlowski
  2026-06-10 15:09     ` Selvamani Rajagopal
  0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2026-06-10  7:22 UTC (permalink / raw)
  To: Selvamani Rajagopal
  Cc: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Andrew Lunn, netdev, linux-kernel, Conor Dooley,
	devicetree, Piergiorgio Beruto

On Tue, Jun 09, 2026 at 12:50:27PM -0700, Selvamani Rajagopal wrote:
> According to OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6)
> specification, interrupt type is active low, level triggered interrupt.
> 
> Fixes: ac49b950bea9 ("dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY")

This to be a fix, you would need to describe the issue, e.g. impact. I
already asked last time and commit msg did not improve.

> Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
> 

Messed patch - missing separator.

> changes in v4:
>   no change
> changes in v3
>   interrupts entry changed to level triggered from edge triggered

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered
  2026-06-10  7:22   ` Krzysztof Kozlowski
@ 2026-06-10 15:09     ` Selvamani Rajagopal
  0 siblings, 0 replies; 10+ messages in thread
From: Selvamani Rajagopal @ 2026-06-10 15:09 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Andrew Lunn, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Conor Dooley,
	devicetree@vger.kernel.org, Piergiorgio Beruto

> Subject: Re: [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low,
> level triggered
> 
> On Tue, Jun 09, 2026 at 12:50:27PM -0700, Selvamani Rajagopal wrote:
> > According to OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6)
> > specification, interrupt type is active low, level triggered interrupt.
> >
> > Fixes: ac49b950bea9 ("dt-bindings: net: add Microchip's LAN865X 10BASE-T1S
> MACPHY")
> 
> This to be a fix, you would need to describe the issue, e.g. impact. I
> already asked last time and commit msg did not improve.


Have added some details in the source change. I should have added here too. I will add here in the next submission. 


> 
> > Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
> >
> 
> Messed patch - missing separator.


I didn't know, therefore didn't realize. Will add in the next submission.


> 
> > changes in v4:
> > no change
> > changes in v3
> > interrupts entry changed to level triggered from edge triggered
> 
> Best regards,
> Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error
  2026-06-09 19:50 ` [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error Selvamani Rajagopal via B4 Relay
@ 2026-06-10 19:50   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 19:50 UTC (permalink / raw)
  To: Selvamani Rajagopal via B4 Relay; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The newly added check for `-EAGAIN` in the threaded IRQ loop is dead code for its intended purpose (RX overflow recovery) and introduces a dangerous tight busy-loop if the underlying SPI driver fails.

Pre-existing issues:
- [Critical] Missing tailroom check before calling `skb_put()` on the receive path allows a malformed or malicious MAC-PHY stream to cause an `skb_over_panic()`.
- [Critical] Out-of-bounds memory access due to unvalidated hardware inputs for TX credits and RX chunks.
- [High] The device IRQ is not disabled before freeing SKB pointers in `oa_tc6_exit()`, leading to a race condition and use-after-free.
--

commit bd14c34a7b1c3dad290ba34e28faf15014377d8e
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: No return on rx buffer overflow error

This patch modifies the threaded IRQ handler to retry the SPI transfer if
oa_tc6_try_spi_transfer() returns -EAGAIN. This is intended to gracefully
recover from RX buffer overflow errors by discarding bad data chunks.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -1111,6 +1111,8 @@ static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
>  
>  	while (tc6->int_flag || (tc6->waiting_tx_skb && tc6->tx_credits)) {
>  		ret = oa_tc6_try_spi_transfer(tc6);
> +		if (ret == -EAGAIN)
> +			continue;

[Severity: High]
Can oa_tc6_try_spi_transfer() actually return -EAGAIN for an RX overflow
condition here?

Looking at oa_tc6_try_spi_transfer(), it catches the -EAGAIN internally:

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

Since it loops internally, it seems it won't return -EAGAIN to the IRQ
handler for an RX overflow.

The only way oa_tc6_try_spi_transfer() returns -EAGAIN to the caller
is if the underlying SPI transfer fails with -EAGAIN.

If that happens, wouldn't this new continue statement create a tight infinite
busy-loop in the threaded IRQ handler context without sleeping?

>  		if (ret)
>  			break;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does oa_tc6_update_rx_skb() verify that
the skb has sufficient tailroom before calling skb_put()?

In the receive path, data is copied into the skb:

static void oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u16 length)
{
        ...
        memcpy(skb_put(tc6->rx_skb, length), payload, length);
}

This is called repeatedly for ongoing frames by oa_tc6_prcs_ongoing_rx_frame()
which appends 64-byte payload chunks to the skb.

If a faulty MAC-PHY or bit-flip streams chunks without setting the end_valid
bit, could the unconditional calls to skb_put() overrun the SKB's tailroom
and trigger a kernel panic via skb_over_panic()?

[Severity: High]
This is a pre-existing issue, but is the device IRQ properly disabled
before oa_tc6_exit() frees the SKB pointers?

oa_tc6_exit() does the following:

static void oa_tc6_exit(struct oa_tc6 *tc6)
{
        ...
        dev_kfree_skb_any(tc6->ongoing_tx_skb);
        dev_kfree_skb_any(tc6->waiting_tx_skb);
        dev_kfree_skb_any(tc6->rx_skb);
        ...
}

Since the driver uses devm_request_threaded_irq(), the IRQ remains active
until the driver unbinds, which happens after oa_tc6_exit() returns.

If an interrupt fires concurrently while oa_tc6_exit() calls
dev_kfree_skb_any(), could the still-active oa_tc6_macphy_threaded_irq()
handler attempt to access these freed pointers?

[Severity: Critical]
This is a pre-existing issue, but do we validate the chunk counts read
from the hardware?

The driver updates the chunk counts from hardware registers:

static void oa_tc6_update_buffer_status_from_register(struct oa_tc6 *tc6,
                                                      u32 value)
{
        tc6->rx_chunks_available = FIELD_GET(BUFFER_STATUS_RX_CHUNKS_AVAILABLE,
                                             value);
        ...
}

These bitfields are 8 bits wide, allowing values up to 255. However, the
SPI data buffers are statically sized to handle a maximum of
OA_TC6_MAX_TX_CHUNKS (48).

If the hardware reports a value larger than 48, could loops like
oa_tc6_add_empty_chunks_to_spi_buf() write past the bounds of
spi_data_tx_buf and cause memory corruption during the SPI transfer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-level-trigger-v4-0-6f389abdd192@onsemi.com?part=2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
  2026-06-09 19:50 ` [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered Selvamani Rajagopal via B4 Relay
@ 2026-06-10 19:50   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 19:50 UTC (permalink / raw)
  To: Selvamani Rajagopal via B4 Relay; +Cc: conor+dt, devicetree, robh

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing IRQ teardown in `oa_tc6_exit()` allows the threaded IRQ to run concurrently with or after driver teardown, causing Use-After-Free of SKBs.
- [High] Unmasking a level-triggered interrupt without clearing the hardware condition causes an interrupt storm on SPI/protocol errors.
--

commit 5c9cbbecd5c1eb1d5efb41b338ac224d42d08b34
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
net: ethernet: oa_tc6: Interrupt is active low, level triggered.

This patch modifies the interrupt handling mechanism for the oa_tc6 driver.
It replaces the kthread-based SPI transfer approach with a threaded IRQ, and
configures the interrupt trigger type from edge-falling to level-low.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 91a906a7918a2..e2c0843207f24 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -1105,29 +1104,18 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
>  	return 0;
>  }
>  
> -static int oa_tc6_spi_thread_handler(void *data)
> +static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
>  {
>  	struct oa_tc6 *tc6 = data;
> -	int ret;
> -
> -	while (likely(!kthread_should_stop())) {
> -		/* This kthread will be waken up if there is a tx skb or mac-phy
> -		 * interrupt to perform spi transfer with tx chunks.
> -		 */
> -		wait_event_interruptible(tc6->spi_wq, tc6->int_flag ||
> -					 (tc6->waiting_tx_skb &&
> -					 tc6->tx_credits) ||
> -					 kthread_should_stop());
> -
> -		if (kthread_should_stop())
> -			break;
>  
> +	while (tc6->int_flag || (tc6->waiting_tx_skb && tc6->tx_credits)) {
>  		ret = oa_tc6_try_spi_transfer(tc6);
>  		if (ret)
> -			return ret;
> +			break;
>  	}
>  
> -	return 0;
> +	return IRQ_HANDLED;
>  }

[Severity: High]
If oa_tc6_try_spi_transfer() fails (e.g., due to an SPI transfer failure)
and returns an error, the loop breaks and returns IRQ_HANDLED.

Since the IRQ is now configured as level-triggered, returning IRQ_HANDLED
will cause the genirq core to unmask the interrupt. If the hardware condition
was not successfully cleared due to the transfer failure, won't this result
in an immediate interrupt storm that locks up the CPU?

Should the IRQ be explicitly disabled (e.g., via disable_irq_nosync()) when
aborting on an unrecoverable error?

[ ... ]

> @@ -1357,7 +1331,6 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
>  void oa_tc6_exit(struct oa_tc6 *tc6)
>  {
>  	oa_tc6_phy_exit(tc6);
> -	kthread_stop(tc6->spi_thread);
>  	dev_kfree_skb_any(tc6->ongoing_tx_skb);
>  	dev_kfree_skb_any(tc6->waiting_tx_skb);
>  	dev_kfree_skb_any(tc6->rx_skb);
>  }

[Severity: High]
Does this code introduce a use-after-free window?

The patch removes kthread_stop(), but it doesn't explicitly disable or free
the newly added threaded IRQ here. Since the IRQ was requested with
devm_request_threaded_irq(), it will remain fully active until the devres
cleanup phase, which occurs after the driver's remove path finishes.

If the threaded IRQ runs concurrently with or after oa_tc6_exit(), can it
access tc6->ongoing_tx_skb, tc6->waiting_tx_skb, or tc6->rx_skb while they
are being unconditionally freed by dev_kfree_skb_any()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-level-trigger-v4-0-6f389abdd192@onsemi.com?part=1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
  2026-06-09 19:50 ` [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL Selvamani Rajagopal via B4 Relay
@ 2026-06-10 19:50   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 19:50 UTC (permalink / raw)
  To: Selvamani Rajagopal via B4 Relay; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free during driver removal due to missing devm teardown synchronization.
- [High] Hardware-induced kernel panic (DoS) due to unchecked `skb_put()` extending beyond allocated buffer bounds.
- [High] Race condition freezing the TX queue due to lockless `waiting_tx_skb` check.
--

commit 2c34943e07dcd7c39f87708d303b4633c83360bd
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
    
As "dev" pointer in oa_tc6 structure is never initialized,
mbiosub->parent was initialized with NULL.  This change
fixes the issue.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 20c9e98d1d607..0710a5a8dcdec 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -108,7 +108,6 @@
>  
>  /* Internal structure for MAC-PHY drivers */
>  struct oa_tc6 {
> -	struct device *dev;
>  	struct net_device *netdev;
>  	struct phy_device *phydev;
>  	struct mii_bus *mdiobus;

[ ... ]

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free crash?

In drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_remove():

    unregister_netdev(priv->netdev);
    oa_tc6_exit(priv->tc6);
    free_netdev(priv->netdev);

The driver registers a threaded IRQ using devres. This means the IRQ is freed
after the remove() function completes.

Could the IRQ thread run concurrently with or after oa_tc6_exit() and
free_netdev(), accessing the freed SKBs and netdev structure since the active
background thread is not canceled before freeing its resources?

[Severity: High]
This isn't a bug introduced by this patch, but does this code properly
validate the buffer boundaries before appending data?

In drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_ongoing_rx_frame():

    oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);

Which appends data unconditionally in oa_tc6_update_rx_skb():

    memcpy(skb_put(tc6->rx_skb, length), payload, length);

If a faulty or malicious MAC-PHY continuously sends data chunks without
asserting the end_valid flag, wouldn't skb_put() hit the end of the buffer
and trigger skb_over_panic() since there is no check against skb_tailroom()?

[Severity: High]
This is a pre-existing issue, but could this lockless check cause the TX
queue to freeze permanently?

In drivers/net/ethernet/oa_tc6.c:oa_tc6_start_xmit():

    if (tc6->waiting_tx_skb) {
        netif_stop_queue(tc6->netdev);
        return NETDEV_TX_BUSY;
    }

And in oa_tc6_try_spi_transfer() on the worker thread:

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

If the worker thread takes the lock, clears waiting_tx_skb, and evaluates
netif_queue_stopped() before start_xmit() actually stops the queue, the
worker thread skips waking the queue.

When start_xmit() resumes, it stops the queue and returns NETDEV_TX_BUSY.
Since waiting_tx_skb is now NULL and the worker missed the wake-up, is there
anything left to wake the queue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-level-trigger-v4-0-6f389abdd192@onsemi.com?part=3

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-06-10 19:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-09 19:50 [PATCH net v4 0/4] MAC-PHY interrupt changed to level triggered interrupt Selvamani Rajagopal via B4 Relay
2026-06-09 19:50 ` [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered Selvamani Rajagopal via B4 Relay
2026-06-10 19:50   ` sashiko-bot
2026-06-09 19:50 ` [PATCH net v4 2/4] net: ethernet: oa_tc6: No return on rx buffer overflow error Selvamani Rajagopal via B4 Relay
2026-06-10 19:50   ` sashiko-bot
2026-06-09 19:50 ` [PATCH net v4 3/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL Selvamani Rajagopal via B4 Relay
2026-06-10 19:50   ` sashiko-bot
2026-06-09 19:50 ` [PATCH net v4 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered Selvamani Rajagopal via B4 Relay
2026-06-10  7:22   ` Krzysztof Kozlowski
2026-06-10 15:09     ` Selvamani Rajagopal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox