RE: [PATCH v5 net 2/3] net: enetc: add graceful stop to safely reinitialize the TX Ring

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@xxxxxxx>
> Sent: Friday, March 13, 2026 11:47 AM
[...]
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> Subject: [PATCH v5 net 2/3] net: enetc: add graceful stop to safely reinitialize
> the TX Ring
> 
> For ENETC v4, the PIR and CIR will be reset if they are not equal when
> reinitializing the TX BD ring. However, resetting the PIR and CIR alone
> is insufficient. When a link-down event occurs while the TX BD ring is
> transmitting frames, subsequent reinitialization of the TX BD ring may
> cause it to malfunction. For example, the below steps can reproduce the
> problem.
> 
> 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@xxxxxxx>
> ---
>  .../net/ethernet/freescale/enetc/enetc4_hw.h  |  11 ++
>  .../net/ethernet/freescale/enetc/enetc4_pf.c  | 111 +++++++++++++++---
>  2 files changed, 108 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
> index 3ed0f7a02767..719c88ceb801 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			0x4104
> +#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..53cecbb23a97 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,105 @@ 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 val;
> +
> +	if (read_poll_timeout(enetc_port_rd, val,
> +			      val & PM_IEVENT_TX_EMPTY,
> +			      100, 10000, false, &si->hw,
> +			      ENETC4_PM_IEVENT(mac)))
> +		dev_warn(&si->pdev->dev,
> +			 "MAC %d TX is not empty\n", mac);
> +}
> +
> +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;
> +	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 | 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);
> +
> +	val = enetc_port_rd(hw, ENETC4_POR);
> +	val &= ~POR_TXDIS;
> +	enetc_port_wr(hw, ENETC4_POR, val);

Nice, taking care that the MAC->Port enabling order on Tx is opposite to the order
on Rx (Port->MAC).

> +}
> +
> +static void enetc4_mac_wait_rx_empty(struct enetc_si *si, int mac)
> +{
> +	u32 val;
> +
> +	if (read_poll_timeout(enetc_port_rd, val,
> +			      val & PM_IEVENT_RX_EMPTY,
> +			      100, 10000, false, &si->hw,
> +			      ENETC4_PM_IEVENT(mac)))
> +		dev_warn(&si->pdev->dev,
> +			 "MAC %d RX is not empty\n", mac);
> +}
> +
> +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 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);
> +
> +	if (read_poll_timeout(enetc_port_rd, val,
> +			      !(val & PSR_RX_BUSY),
> +			      100, 10000, false, hw,
> +			      ENETC4_PSR))
> +		dev_warn(&si->pdev->dev, "Port RX busy\n");
> +
> +	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 +934,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);
>  }
> 

Nothing for me to object on so far in this patch.

Reviewed-by: Claudiu Manoil <claudiu.manoil@xxxxxxx>