[PATCH v2 net] net: enetc: safely reinitialize TX BD ring when it has unsent frames

[PATCH v2 net] net: enetc: safely reinitialize TX BD ring when it has unsent frames

[Wei Fang]

Currently the driver does not reset the producer index register (PIR) and
consumer index register (CIR) when initializing a TX BD ring. The driver
only reads the PIR and CIR and initializes the software indexes. If the
TX BD ring is reinitialized when it still contains unsent frames, its PIR
and CIR will not be equal after the reinitialization. However, the BDs
between CIR and PIR have been freed and become invalid and this can lead
to a hardware malfunction, causing the TX BD ring will not work perperly.

Since the PIR and CIR are sofeware-configurable on ENETC v4. Therefore,
the driver must reset them if they are not equal when reinitializing
the TX BD ring.

However, resetting the PIR and CIR alone is insufficient, it cannot
completely solve the problem. When a link-down event occurs while the
the TX BD ring is transmitting frames, subsequent reinitialization of
the TX BD ring may cause it to malfunction. For example, the following
steps may reproduce the problem (not 100% reproducible).

1. Unplug the cable when the TX BD ring is busy transmitting frames.
2. Disable the network interface (ifconfig eth0 down).
3. Re-enable the network interface (ifconfig eth0 up).
4. Plug in the cable, the TX BD ring may fail to transmit packets.

When the link-down event occurs, enetc4_pl_mac_link_down() only clears
PMa_COMMAND_CONFIG[TX_EN] to disable MAC transmit data path. It doesn't
set PORT[TXDIS] to 1 to flush the TX BD ring. Therefore, reinitializing
the TX BD ring at this point is unsafe. To safely reinitialize the TX BD
ring after a link-down event, we checked with the NETC IP team, a proper
Ethernet MAC graceful stop is necessary. Therefore, add the Ethernet MAC
graceful stop to the link-down event handler enetc4_pl_mac_link_down().

Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
v2:
1. Remove unused register macros (ENETC_SISR and SISR_TX_BUSY)
2. Remove spurious semicolon from enetc4_mac_wait_rx_empty()
---
 drivers/net/ethernet/freescale/enetc/enetc.c  |   9 ++
 .../net/ethernet/freescale/enetc/enetc4_hw.h  |  11 ++
 .../net/ethernet/freescale/enetc/enetc4_pf.c  | 126 ++++++++++++++++--
 3 files changed, 132 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 70768392912c..825a5d1f2965 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -2578,6 +2578,7 @@ EXPORT_SYMBOL_GPL(enetc_free_si_resources);
 
 static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
 {
+	struct enetc_si *si = container_of(hw, struct enetc_si, hw);
 	int idx = tx_ring->index;
 	u32 tbmr;
 
@@ -2595,6 +2596,14 @@ static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
 	tx_ring->next_to_use = enetc_txbdr_rd(hw, idx, ENETC_TBPIR);
 	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR);
 
+	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
+	    !is_enetc_rev1(si)) {
+		tx_ring->next_to_use = 0;
+		tx_ring->next_to_clean = 0;
+		enetc_txbdr_wr(hw, idx, ENETC_TBPIR, 0);
+		enetc_txbdr_wr(hw, idx, ENETC_TBCIR, 0);
+	}
+
 	/* enable Tx ints by setting pkt thr to 1 */
 	enetc_txbdr_wr(hw, idx, ENETC_TBICR0, ENETC_TBICR0_ICEN | 0x1);
 
diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
index 3ed0f7a02767..1ce6551e186c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
@@ -134,6 +134,12 @@
 
 /* Port operational register */
 #define ENETC4_POR			0x4100
+#define  POR_TXDIS			BIT(0)
+#define  POR_RXDIS			BIT(1)
+
+/* Port status register */
+#define ENETC4_PSR			0x4100
+#define  PSR_RX_BUSY			BIT(1)
 
 /* Port traffic class a transmit maximum SDU register */
 #define ENETC4_PTCTMSDUR(a)		((a) * 0x20 + 0x4208)
@@ -173,6 +179,11 @@
 /* Port internal MDIO base address, use to access PCS */
 #define ENETC4_PM_IMDIO_BASE		0x5030
 
+/* Port MAC 0/1 Interrupt Event Register */
+#define ENETC4_PM_IEVENT(mac)		(0x5040 + (mac) * 0x400)
+#define  PM_IEVENT_TX_EMPTY		BIT(5)
+#define  PM_IEVENT_RX_EMPTY		BIT(6)
+
 /* Port MAC 0/1 Pause Quanta Register */
 #define ENETC4_PM_PAUSE_QUANTA(mac)	(0x5054 + (mac) * 0x400)
 
diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
index 689b9f13c5eb..6103f1535d43 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
@@ -444,20 +444,11 @@ static void enetc4_set_trx_frame_size(struct enetc_pf *pf)
 	enetc4_pf_reset_tc_msdu(&si->hw);
 }
 
-static void enetc4_enable_trx(struct enetc_pf *pf)
-{
-	struct enetc_hw *hw = &pf->si->hw;
-
-	/* Enable port transmit/receive */
-	enetc_port_wr(hw, ENETC4_POR, 0);
-}
-
 static void enetc4_configure_port(struct enetc_pf *pf)
 {
 	enetc4_configure_port_si(pf);
 	enetc4_set_trx_frame_size(pf);
 	enetc_set_default_rss_key(pf);
-	enetc4_enable_trx(pf);
 }
 
 static int enetc4_init_ntmp_user(struct enetc_si *si)
@@ -801,15 +792,120 @@ static void enetc4_set_tx_pause(struct enetc_pf *pf, int num_rxbdr, bool tx_paus
 	enetc_port_wr(hw, ENETC4_PPAUOFFTR, pause_off_thresh);
 }
 
-static void enetc4_enable_mac(struct enetc_pf *pf, bool en)
+static void enetc4_mac_wait_tx_empty(struct enetc_si *si, int mac)
+{
+	u32 timeout = 0;
+
+	while (!(enetc_port_rd(&si->hw, ENETC4_PM_IEVENT(mac)) &
+		 PM_IEVENT_TX_EMPTY)) {
+		if (timeout >= 100) {
+			dev_warn(&si->pdev->dev,
+				 "MAC %d TX is not empty\n", mac);
+			break;
+		}
+
+		usleep_range(100, 110);
+		timeout++;
+	}
+}
+
+static void enetc4_mac_tx_graceful_stop(struct enetc_pf *pf)
 {
+	struct enetc_hw *hw = &pf->si->hw;
 	struct enetc_si *si = pf->si;
 	u32 val;
 
+	val = enetc_port_rd(hw, ENETC4_POR);
+	val |= POR_TXDIS;
+	enetc_port_wr(hw, ENETC4_POR, val);
+
+	enetc4_mac_wait_tx_empty(si, 0);
+	if (si->hw_features & ENETC_SI_F_QBU)
+		enetc4_mac_wait_tx_empty(si, 1);
+
 	val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
-	val &= ~(PM_CMD_CFG_TX_EN | PM_CMD_CFG_RX_EN);
-	val |= en ? (PM_CMD_CFG_TX_EN | PM_CMD_CFG_RX_EN) : 0;
+	val &= ~PM_CMD_CFG_TX_EN;
+	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
+}
+
+static void enetc4_mac_tx_enable(struct enetc_pf *pf)
+{
+	struct enetc_hw *hw = &pf->si->hw;
+	struct enetc_si *si = pf->si;
+	u32 val;
 
+	val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
+	val |= PM_CMD_CFG_TX_EN;
+	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
+
+	val = enetc_port_rd(hw, ENETC4_POR);
+	val &= ~POR_TXDIS;
+	enetc_port_wr(hw, ENETC4_POR, val);
+}
+
+static void enetc4_mac_wait_rx_empty(struct enetc_si *si, int mac)
+{
+	u32 timeout = 0;
+
+	while (!(enetc_port_rd(&si->hw, ENETC4_PM_IEVENT(mac)) &
+		 PM_IEVENT_RX_EMPTY)) {
+		if (timeout >= 100) {
+			dev_warn(&si->pdev->dev,
+				 "MAC %d RX is not empty\n", mac);
+			break;
+		}
+
+		usleep_range(100, 110);
+		timeout++;
+	}
+}
+
+static void enetc4_mac_rx_graceful_stop(struct enetc_pf *pf)
+{
+	struct enetc_hw *hw = &pf->si->hw;
+	struct enetc_si *si = pf->si;
+	u32 timeout = 0;
+	u32 val;
+
+	if (si->hw_features & ENETC_SI_F_QBU) {
+		val = enetc_port_rd(hw, ENETC4_PM_CMD_CFG(1));
+		val &= ~PM_CMD_CFG_RX_EN;
+		enetc_port_wr(hw, ENETC4_PM_CMD_CFG(1), val);
+		enetc4_mac_wait_rx_empty(si, 1);
+	}
+
+	val = enetc_port_rd(hw, ENETC4_PM_CMD_CFG(0));
+	val &= ~PM_CMD_CFG_RX_EN;
+	enetc_port_wr(hw, ENETC4_PM_CMD_CFG(0), val);
+	enetc4_mac_wait_rx_empty(si, 0);
+
+	while (enetc_port_rd(hw, ENETC4_PSR) & PSR_RX_BUSY) {
+		if (timeout >= 100) {
+			dev_warn(&si->pdev->dev, "Port RX busy\n");
+			break;
+		}
+
+		usleep_range(100, 110);
+		timeout++;
+	}
+
+	val = enetc_port_rd(hw, ENETC4_POR);
+	val |= POR_RXDIS;
+	enetc_port_wr(hw, ENETC4_POR, val);
+}
+
+static void enetc4_mac_rx_enable(struct enetc_pf *pf)
+{
+	struct enetc_hw *hw = &pf->si->hw;
+	struct enetc_si *si = pf->si;
+	u32 val;
+
+	val = enetc_port_rd(hw, ENETC4_POR);
+	val &= ~POR_RXDIS;
+	enetc_port_wr(hw, ENETC4_POR, val);
+
+	val = enetc_port_mac_rd(si, ENETC4_PM_CMD_CFG(0));
+	val |= PM_CMD_CFG_RX_EN;
 	enetc_port_mac_wr(si, ENETC4_PM_CMD_CFG(0), val);
 }
 
@@ -853,7 +949,8 @@ static void enetc4_pl_mac_link_up(struct phylink_config *config,
 	enetc4_set_hd_flow_control(pf, hd_fc);
 	enetc4_set_tx_pause(pf, priv->num_rx_rings, tx_pause);
 	enetc4_set_rx_pause(pf, rx_pause);
-	enetc4_enable_mac(pf, true);
+	enetc4_mac_tx_enable(pf);
+	enetc4_mac_rx_enable(pf);
 }
 
 static void enetc4_pl_mac_link_down(struct phylink_config *config,
@@ -862,7 +959,8 @@ static void enetc4_pl_mac_link_down(struct phylink_config *config,
 {
 	struct enetc_pf *pf = phylink_to_enetc_pf(config);
 
-	enetc4_enable_mac(pf, false);
+	enetc4_mac_rx_graceful_stop(pf);
+	enetc4_mac_tx_graceful_stop(pf);
 }
 
 static const struct phylink_mac_ops enetc_pl_mac_ops = {
-- 
2.34.1

Re: [PATCH v2 net] net: enetc: safely reinitialize TX BD ring when it has unsent frames

[Simon Horman]

On Mon, Mar 09, 2026 at 11:04:12AM +0800, Wei Fang wrote:
> Currently the driver does not reset the producer index register (PIR) and
> consumer index register (CIR) when initializing a TX BD ring. The driver
> only reads the PIR and CIR and initializes the software indexes. If the
> TX BD ring is reinitialized when it still contains unsent frames, its PIR
> and CIR will not be equal after the reinitialization. However, the BDs
> between CIR and PIR have been freed and become invalid and this can lead
> to a hardware malfunction, causing the TX BD ring will not work perperly.
> 
> Since the PIR and CIR are sofeware-configurable on ENETC v4. Therefore,
> the driver must reset them if they are not equal when reinitializing
> the TX BD ring.
> 
> However, resetting the PIR and CIR alone is insufficient, it cannot
> completely solve the problem. When a link-down event occurs while the
> the TX BD ring is transmitting frames, subsequent reinitialization of
> the TX BD ring may cause it to malfunction. For example, the following
> steps may reproduce the problem (not 100% reproducible).
> 
> 1. Unplug the cable when the TX BD ring is busy transmitting frames.
> 2. Disable the network interface (ifconfig eth0 down).
> 3. Re-enable the network interface (ifconfig eth0 up).
> 4. Plug in the cable, the TX BD ring may fail to transmit packets.
> 
> When the link-down event occurs, enetc4_pl_mac_link_down() only clears
> PMa_COMMAND_CONFIG[TX_EN] to disable MAC transmit data path. It doesn't
> set PORT[TXDIS] to 1 to flush the TX BD ring. Therefore, reinitializing
> the TX BD ring at this point is unsafe. To safely reinitialize the TX BD
> ring after a link-down event, we checked with the NETC IP team, a proper
> Ethernet MAC graceful stop is necessary. Therefore, add the Ethernet MAC
> graceful stop to the link-down event handler enetc4_pl_mac_link_down().
> 
> Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
> v2:
> 1. Remove unused register macros (ENETC_SISR and SISR_TX_BUSY)
> 2. Remove spurious semicolon from enetc4_mac_wait_rx_empty()
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c  |   9 ++
>  .../net/ethernet/freescale/enetc/enetc4_hw.h  |  11 ++
>  .../net/ethernet/freescale/enetc/enetc4_pf.c  | 126 ++++++++++++++++--
>  3 files changed, 132 insertions(+), 14 deletions(-)

Hi,

This is a not a small patch for a fix.
Could you consider splitting it up a bit?
Say one patch for the ENETC_TBPIR/ENETC_TBCIR
portion and another for the graceful stop?

> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 70768392912c..825a5d1f2965 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -2578,6 +2578,7 @@ EXPORT_SYMBOL_GPL(enetc_free_si_resources);
>  
>  static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
>  {
> +	struct enetc_si *si = container_of(hw, struct enetc_si, hw);
>  	int idx = tx_ring->index;
>  	u32 tbmr;
>  
> @@ -2595,6 +2596,14 @@ static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)

The line above this hunk is:

	/* clearing PI/CI registers for Tx not supported, adjust sw indexes */

And the AI generated review reports:

  This isn't a bug, but the comment now contradicts the code. The comment
  says "clearing PI/CI registers for Tx not supported" but the code
  immediately below clears TBPIR and TBCIR for ENETC v4 hardware.

  Should the comment be updated to clarify that clearing is only
  unsupported on ENETC rev1?

>  	tx_ring->next_to_use = enetc_txbdr_rd(hw, idx, ENETC_TBPIR);
>  	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR);
>  
> +	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
> +	    !is_enetc_rev1(si)) {
> +		tx_ring->next_to_use = 0;
> +		tx_ring->next_to_clean = 0;
> +		enetc_txbdr_wr(hw, idx, ENETC_TBPIR, 0);
> +		enetc_txbdr_wr(hw, idx, ENETC_TBCIR, 0);
> +	}
> +
>  	/* enable Tx ints by setting pkt thr to 1 */
>  	enetc_txbdr_wr(hw, idx, ENETC_TBICR0, ENETC_TBICR0_ICEN | 0x1);
>  

...

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

...

> @@ -801,15 +792,120 @@ static void enetc4_set_tx_pause(struct enetc_pf *pf, int num_rxbdr, bool tx_paus
>  	enetc_port_wr(hw, ENETC4_PPAUOFFTR, pause_off_thresh);
>  }
>  
> -static void enetc4_enable_mac(struct enetc_pf *pf, bool en)
> +static void enetc4_mac_wait_tx_empty(struct enetc_si *si, int mac)
> +{
> +	u32 timeout = 0;
> +
> +	while (!(enetc_port_rd(&si->hw, ENETC4_PM_IEVENT(mac)) &
> +		 PM_IEVENT_TX_EMPTY)) {
> +		if (timeout >= 100) {
> +			dev_warn(&si->pdev->dev,
> +				 "MAC %d TX is not empty\n", mac);
> +			break;
> +		}
> +
> +		usleep_range(100, 110);
> +		timeout++;
> +	}
> +}

Can this be implemented using poll_timeout_us() ?

...

> +static void enetc4_mac_wait_rx_empty(struct enetc_si *si, int mac)
> +{
> +	u32 timeout = 0;
> +
> +	while (!(enetc_port_rd(&si->hw, ENETC4_PM_IEVENT(mac)) &
> +		 PM_IEVENT_RX_EMPTY)) {
> +		if (timeout >= 100) {
> +			dev_warn(&si->pdev->dev,
> +				 "MAC %d RX is not empty\n", mac);
> +			break;
> +		}
> +
> +		usleep_range(100, 110);
> +		timeout++;
> +	}
> +}

Likewise, here.

...

RE: [PATCH v2 net] net: enetc: safely reinitialize TX BD ring when it has unsent frames

[Wei Fang]

> This is a not a small patch for a fix.
> Could you consider splitting it up a bit?
> Say one patch for the ENETC_TBPIR/ENETC_TBCIR
> portion and another for the graceful stop?

Sure, I will, thanks.

> 
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 70768392912c..825a5d1f2965 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -2578,6 +2578,7 @@ EXPORT_SYMBOL_GPL(enetc_free_si_resources);
> >
> >  static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr
> *tx_ring)
> >  {
> > +	struct enetc_si *si = container_of(hw, struct enetc_si, hw);
> >  	int idx = tx_ring->index;
> >  	u32 tbmr;
> >
> > @@ -2595,6 +2596,14 @@ static void enetc_setup_txbdr(struct enetc_hw
> *hw, struct enetc_bdr *tx_ring)
> 
> The line above this hunk is:
> 
> 	/* clearing PI/CI registers for Tx not supported, adjust sw indexes */
> 
> And the AI generated review reports:
> 
>   This isn't a bug, but the comment now contradicts the code. The comment
>   says "clearing PI/CI registers for Tx not supported" but the code
>   immediately below clears TBPIR and TBCIR for ENETC v4 hardware.
> 
>   Should the comment be updated to clarify that clearing is only
>   unsupported on ENETC rev1?

I will update the comments, only ENETC v1 does not support setting
PIR and CIR.

> 
> >  	tx_ring->next_to_use = enetc_txbdr_rd(hw, idx, ENETC_TBPIR);
> >  	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR);
> >
> > +	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
> > +	    !is_enetc_rev1(si)) {
> > +		tx_ring->next_to_use = 0;
> > +		tx_ring->next_to_clean = 0;
> > +		enetc_txbdr_wr(hw, idx, ENETC_TBPIR, 0);
> > +		enetc_txbdr_wr(hw, idx, ENETC_TBCIR, 0);
> > +	}
> > +
> >  	/* enable Tx ints by setting pkt thr to 1 */
> >  	enetc_txbdr_wr(hw, idx, ENETC_TBICR0, ENETC_TBICR0_ICEN | 0x1);
> >
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> 
> ...
> 
> > @@ -801,15 +792,120 @@ static void enetc4_set_tx_pause(struct enetc_pf
> *pf, int num_rxbdr, bool tx_paus
> >  	enetc_port_wr(hw, ENETC4_PPAUOFFTR, pause_off_thresh);
> >  }
> >
> > -static void enetc4_enable_mac(struct enetc_pf *pf, bool en)
> > +static void enetc4_mac_wait_tx_empty(struct enetc_si *si, int mac)
> > +{
> > +	u32 timeout = 0;
> > +
> > +	while (!(enetc_port_rd(&si->hw, ENETC4_PM_IEVENT(mac)) &
> > +		 PM_IEVENT_TX_EMPTY)) {
> > +		if (timeout >= 100) {
> > +			dev_warn(&si->pdev->dev,
> > +				 "MAC %d TX is not empty\n", mac);
> > +			break;
> > +		}
> > +
> > +		usleep_range(100, 110);
> > +		timeout++;
> > +	}
> > +}
> 
> Can this be implemented using poll_timeout_us() ?

Sure, thanks.