Re: [PATCH] net: dsa: qca8k: reset cpu port on MTU change

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Jun 28, 2022 at 04:30:10PM +0200, Christian Marangi wrote:
> commit 386228c694bf1e7a7688e44412cb33500b0ac585 upstream.
> 
> It was discovered that the Documentation lacks of a fundamental detail
> on how to correctly change the MAX_FRAME_SIZE of the switch.
> 
> In fact if the MAX_FRAME_SIZE is changed while the cpu port is on, the
> switch panics and cease to send any packet. This cause the mgmt ethernet
> system to not receive any packet (the slow fallback still works) and
> makes the device not reachable. To recover from this a switch reset is
> required.
> 
> To correctly handle this, turn off the cpu ports before changing the
> MAX_FRAME_SIZE and turn on again after the value is applied.
> 
> Fixes: f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Link: https://lore.kernel.org/r/20220621151122.10220-1-ansuelsmth@gmail.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> [ backport: fix conflict using the old port_sts struct instead of bitmap ]

Cc Greg wonder if this has some problems or I pushed it badly? (first
time I try to push a fixed backport)

> ---
>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a984f06f6f04..67869c8cbeaa 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1599,7 +1599,7 @@ static int
>  qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
>  	struct qca8k_priv *priv = ds->priv;
> -	int i, mtu = 0;
> +	int ret, i, mtu = 0;
>  
>  	priv->port_mtu[port] = new_mtu;
>  
> @@ -1607,8 +1607,27 @@ qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  		if (priv->port_mtu[i] > mtu)
>  			mtu = priv->port_mtu[i];
>  
> +	/* 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.
> +	 */
> +	if (priv->port_sts[0].enabled)
> +		qca8k_port_set_status(priv, 0, 0);
> +
> +	if (priv->port_sts[6].enabled)
> +		qca8k_port_set_status(priv, 6, 0);
> +
>  	/* Include L2 header / FCS length */
> -	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +
> +	if (priv->port_sts[0].enabled)
> +		qca8k_port_set_status(priv, 0, 1);
> +
> +	if (priv->port_sts[6].enabled)
> +		qca8k_port_set_status(priv, 6, 1);
> +
> +	return ret;
>  }
>  
>  static int
> -- 
> 2.36.1
> 

-- 
	Ansuel