Re: [net-next PATCH 1/4] drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv

[Ansuel Smith]

On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote:
> > Drop the MTU array from qca8k_priv and use slave net dev to get the max
> > MTU across all user port. CPU port can be skipped as DSA already make
> > sure CPU port are set to the max MTU across all ports.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > ---
> 
> I hardly find this to be an improvement and I would rather not see such
> unjustified complexity in a device driver. What are the concrete
> benefits, size wise?
>

The main idea here is, if the value is already present and accessible,
why should we duplicate it? Tracking the MTU in this custom way already
caused some bugs (check the comment i'm removing). We both use standard
way to track ports MTU and we save some additional space. At the cost of
2 additional checks are are not that much of a problem.

Also from this I discovered that (at least on ipq806x that use stmmac)
when master needs to change MTU, stmmac complains that the interface is
up and it must be put down. Wonder if that's common across other drivers
or it's only specific to stmmac.

> >  drivers/net/dsa/qca8k.c | 38 +++++++++++++++++++++++---------------
> >  drivers/net/dsa/qca8k.h |  1 -
> >  2 files changed, 23 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index d3ed0a7f8077..4366d87b4bbd 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -2367,13 +2367,31 @@ 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;
> > +	struct dsa_port *dp;
> > +	int mtu = new_mtu;
> >  
> > -	priv->port_mtu[port] = new_mtu;
> > +	/* We have only have a general MTU setting. So check
> > +	 * every port and set the max across all port.
> > +	 */
> > +	list_for_each_entry(dp, &ds->dst->ports, list) {
> > +		/* We can ignore cpu port, DSA will itself chose
> > +		 * the max MTU across all port
> > +		 */
> > +		if (!dsa_port_is_user(dp))
> > +			continue;
> >  
> > -	for (i = 0; i < QCA8K_NUM_PORTS; i++)
> > -		if (priv->port_mtu[i] > mtu)
> > -			mtu = priv->port_mtu[i];
> > +		if (dp->index == port)
> > +			continue;
> > +
> > +		/* Address init phase where not every port have
> > +		 * a slave device
> > +		 */
> > +		if (!dp->slave)
> > +			continue;
> > +
> > +		if (mtu < dp->slave->mtu)
> > +			mtu = dp->slave->mtu;
> > +	}
> >  
> >  	/* Include L2 header / FCS length */
> >  	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> > @@ -3033,16 +3051,6 @@ qca8k_setup(struct dsa_switch *ds)
> >  				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
> >  				  mask);
> >  		}
> > -
> > -		/* Set initial MTU for every port.
> > -		 * We have only have a general MTU setting. So track
> > -		 * every port and set the max across all port.
> > -		 * Set per port MTU to 1500 as the MTU change function
> > -		 * will add the overhead and if its set to 1518 then it
> > -		 * will apply the overhead again and we will end up with
> > -		 * MTU of 1536 instead of 1518
> > -		 */
> > -		priv->port_mtu[i] = ETH_DATA_LEN;
> >  	}
> >  
> >  	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index f375627174c8..562d75997e55 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -398,7 +398,6 @@ struct qca8k_priv {
> >  	struct device *dev;
> >  	struct dsa_switch_ops ops;
> >  	struct gpio_desc *reset_gpio;
> > -	unsigned int port_mtu[QCA8K_NUM_PORTS];
> >  	struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
> >  	struct qca8k_mgmt_eth_data mgmt_eth_data;
> >  	struct qca8k_mib_eth_data mib_eth_data;
> > -- 
> > 2.34.1
> > 
> 

-- 
	Ansuel