Re: [PATCH net-next v4 2/3] net: dsa: qca8k: serialize CPU MAC pause during MTU changes

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Sep 29, 2026 at 12:08:10AM +0200, Yongzhao Chen wrote:
> The global maximum frame size must be updated with CPU MACs disabled.
> The previous logic only paused ports 0 and 6, leaving an internal PHY
> CPU port enabled while modifying the register.
> 
> Include enabled internal CPU ports in the pause sequence. Use the
> existing reg_mutex to serialize the MTU update against port enable, port
> disable, and phylink link-up and link-down transitions. Read and restore
> each port's original TXMAC and RXMAC bits, ensuring ports that were down
> remain down and preserving LINK_AUTO. Retain existing handling for ports
> 0 and 6.
> 
> Abort before updating the frame size if reading port status or pausing
> the MAC fails. Attempt to restore all ports already modified, and report
> any restoration failures even if an earlier error occurred.
> 
> The standalone qca8k MDIO error-propagation fix is a prerequisite for
> this series; that error-handling bug predates this locking change.
> 
> Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
> Assisted-by: LLM
> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c   |  2 +
>  drivers/net/dsa/qca/qca8k-common.c | 79 ++++++++++++++++++++++++------
>  2 files changed, 66 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 89113d22d5d..7bd9d9abcef 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1495,7 +1495,9 @@ qca8k_phylink_mac_link_up(struct phylink_config *config,
>  
>  	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
>  
> +	mutex_lock(&priv->reg_mutex);
>  	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
> +	mutex_unlock(&priv->reg_mutex);
>  }

I'm not entirely sure we need to use the reg mutex here since each port
have their own register and is independent... a dedicated mutex should be
considered for the task...

>  
>  static struct qca8k_pcs *pcs_to_qca8k_pcs(struct phylink_pcs *pcs)
> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> index 13005f10edb..6b32bdd75ea 100644
> --- a/drivers/net/dsa/qca/qca8k-common.c
> +++ b/drivers/net/dsa/qca/qca8k-common.c
> @@ -463,7 +463,8 @@ int qca8k_mib_init(struct qca8k_priv *priv)
>  	return ret;
>  }
>  
> -void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
> +static void qca8k_port_set_status_locked(struct qca8k_priv *priv, int port,
> +					 int enable)

Personal taste but I always feel this might be confusing...
_locked may imply that the function will lock, not that you should lock
before calling... I know it's already pattern in the kernel, your choice to
use this or __ variant.
I would use __qca8k_port_set.. and add tag to enforce that the mutex should be
locked here.

>  {
>  	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
>  
> @@ -477,6 +478,13 @@ void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
>  		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
>  }
>  
> +void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
> +{
> +	mutex_lock(&priv->reg_mutex);
> +	qca8k_port_set_status_locked(priv, port, enable);
> +	mutex_unlock(&priv->reg_mutex);
> +}
> +
>  void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset,
>  		       uint8_t *data)
>  {
> @@ -751,8 +759,10 @@ int qca8k_port_enable(struct dsa_switch *ds, int port,
>  {
>  	struct qca8k_priv *priv = ds->priv;
>  
> -	qca8k_port_set_status(priv, port, 1);
> +	mutex_lock(&priv->reg_mutex);
> +	qca8k_port_set_status_locked(priv, port, 1);
>  	priv->port_enabled_map |= BIT(port);
> +	mutex_unlock(&priv->reg_mutex);
>  

Can port be enabled concurrently and corrupt the port enable map? Can you
check with AI if this case is possible? If yes then this might be a good
idea to make a separate prereq patch introducing a dedicated mutex for
port status and protect it accordingly. (might also be worth for net)

>  	if (dsa_is_user_port(ds, port))
>  		phy_support_asym_pause(phy);
> @@ -764,14 +774,20 @@ void qca8k_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct qca8k_priv *priv = ds->priv;
>  
> -	qca8k_port_set_status(priv, port, 0);
> +	mutex_lock(&priv->reg_mutex);
> +	qca8k_port_set_status_locked(priv, port, 0);
>  	priv->port_enabled_map &= ~BIT(port);
> +	mutex_unlock(&priv->reg_mutex);
>  }

ditto.

>  
>  int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
> +	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
>  	struct qca8k_priv *priv = ds->priv;
> -	int ret;
> +	u32 status[QCA8K_NUM_PORTS] = { 0 };

nit. Reverse tree.

> +	int ret, restore_ret, i;
> +	u32 stopped = 0;
> +	u32 ports;
>  
>  	/* We have only have a general MTU setting.
>  	 * DSA always set the CPU port's MTU to the largest MTU of the user
> @@ -784,25 +800,58 @@ int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  
>  	/* To change the MAX_FRAME_SIZE the cpu ports must be off or
>  	 * the switch panics.
> -	 * Turn off both cpu ports before applying the new value to prevent
> -	 * this.
> +	 * Include internal PHY CPU ports as well as the two MAC-only ports.
> +	 * Toggle only MAC enables, preserving the phylink link-control mode.
>  	 */
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 0);
> +	ports = BIT(0) | BIT(6);
> +	for (i = 1; i < 6; i++)
> +		if (dsa_is_cpu_port(ds, i))
> +			ports |= BIT(i);

In the context of internal PHY CPU port port 0 and port 6 won't be
connected... Should we check that and create a mask of the cpu port right
from the start?

>  
> -	if (priv->port_enabled_map & BIT(6))
> -		qca8k_port_set_status(priv, 6, 0);
> +	mutex_lock(&priv->reg_mutex);
> +	ports &= priv->port_enabled_map;
> +
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {

for_each_set_bit might be better?

> +		if (!(ports & BIT(i)))
> +			continue;
> +
> +		ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +				  &status[i]);
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {

ditto.

> +		if (!(ports & BIT(i)) || !(status[i] & mask))
> +			continue;
> +
> +		stopped |= BIT(i);
> +		ret = regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +					mask);
> +		if (ret)
> +			goto restore;
> +	}
>  
>  	/* Include L2 header / FCS length */
>  	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu +
>  			  ETH_HLEN + ETH_FCS_LEN);
>  
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 1);
> -
> -	if (priv->port_enabled_map & BIT(6))
> -		qca8k_port_set_status(priv, 6, 1);
> +restore:
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++)
> +		if (stopped & BIT(i)) {
> +			restore_ret = regmap_update_bits(priv->regmap,
> +							 QCA8K_REG_PORT_STATUS(i),
> +							 mask, status[i] & mask);
> +			if (restore_ret) {
> +				dev_err(priv->dev, "failed to restore MAC state on port %d: %d\n",
> +					i, restore_ret);
> +				if (!ret)
> +					ret = restore_ret;
> +			}
> +		}
>  
> +unlock:
> +	mutex_unlock(&priv->reg_mutex);
>  	return ret;
>  }
>  
> -- 
> 2.43.0
> 

-- 
	Ansuel