Re: [RESEND net-next PATCH 3/3] net: dsa: qca8k: reset cpu port on MTU change

[Christian Marangi <ansuelsmth@gmail.com>]

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