Messages in this thread | | | Date | Tue, 21 Jun 2022 17:04:13 +0200 | From | Christian Marangi <> | Subject | Re: [RESEND net-next PATCH 3/3] net: dsa: qca8k: reset cpu port on MTU change |
| |
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
-- Ansuel
| |