Message ID | 20220618072650.3502-3-ansuelsmth@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [RESEND,net-next,1/3] net: dsa: qca8k: reduce mgmt ethernet timeout | expand |
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@gmail.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
On Mon, Jun 20, 2022 at 09:56:19PM -0700, Jakub Kicinski wrote: > 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@gmail.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. > Ok will split this series. > 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? > No reason just following old bad behaviour done in other function where qca8k_port_set_status is used. Will send v2 with the error handled. > > + 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
On Tue, Jun 21, 2022 at 04:51:46PM +0200, Christian Marangi wrote: > On Mon, Jun 20, 2022 at 09:56:19PM -0700, Jakub Kicinski wrote: > > 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@gmail.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. > > > > Ok will split this series. > > > 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? > > > > No reason just following old bad behaviour done in other function where > qca8k_port_set_status is used. Will send v2 with the error handled. > Actually now that i checked, the qca8k_port_set_status is void... So it would require an additional change to that function. Will make it part of the net-next series... > > > + 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 > > -- > Ansuel
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, + 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
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@gmail.com> --- drivers/net/dsa/qca8k.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)