[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220620215619.2209533a@kernel.org>
Date: Mon, 20 Jun 2022 21:56:19 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Jonathan McDowell <noodles@...th.li>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RESEND net-next PATCH 3/3] net: dsa: qca8k: reset cpu port on
MTU change
On Sat, 18 Jun 2022 09:26:50 +0200 Christian Marangi wrote:
> 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")
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
It reads like this patch should be backported to 5.10 and 5.15 stable
branches. While patches 1 and 2 are cleanups. In which case you should
reports just patch 3 against net/master first, we'll send it to Linus at
the end of the week and then you can send the cleanups on top for -next.
One extra question below.
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index eaaf80f96fa9..0b92b9d5954a 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -2334,6 +2334,7 @@ static int
> qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> {
> struct qca8k_priv *priv = ds->priv;
> + int ret;
>
> /* We have only have a general MTU setting.
> * DSA always set the CPU port's MTU to the largest MTU of the slave
> @@ -2344,10 +2345,29 @@ qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> if (!dsa_is_cpu_port(ds, port))
> return 0;
>
> + /* 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_enabled_map & BIT(0))
> + qca8k_port_set_status(priv, 0, 0);
> +
> + if (priv->port_enabled_map & BIT(6))
> + qca8k_port_set_status(priv, 6, 0);
> +
> /* Include L2 header / FCS length */
> - return regmap_update_bits(priv->regmap, QCA8K_MAX_FRAME_SIZE_REG,
> - QCA8K_MAX_FRAME_SIZE_MASK,
> - new_mtu + ETH_HLEN + ETH_FCS_LEN);
> + ret = regmap_update_bits(priv->regmap, QCA8K_MAX_FRAME_SIZE_REG,
Why care about the return code of this regmap access but not the ones
inside the *port_set_status() calls?
> + QCA8K_MAX_FRAME_SIZE_MASK,
> + 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);
> +
> + return ret;
> }
>
> static int
Powered by blists - more mailing lists