[net-next PATCH 0/4] Reduce qca8k_priv space usage

[net-next PATCH 0/4] Reduce qca8k_priv space usage

[Ansuel Smith]

These 4 patch is a first attempt at reducting qca8k_priv space.
The code changed a lot during times and we have many old logic
that can be replaced with new implementation

The first patch drop the tracking of MTU. We now base all on the
MTU saved in the slave dev. Just like DSA core does to get the max
MTU across all port to set a correct MTU for the cpu port.

The second patch finally drop a piece of story of this driver.
The ar8xxx_port_status struct was used by the first implementation
of this driver to put all sort of status data for the port...
With the evolution of DSA all that stuff got dropped till only
the enabled state was the only part of the that struct.
Since it's overkill to keep an array of int, we convert the variable
to a simple u8 where we store the status of each port. This is needed
to don't reanable ports on system resume.

The third patch is a preparation for patch 4. As Vladimir explained
in another patch, we waste a tons of space by keeping a duplicate of
the switch dsa ops in qca8k_priv. The only reason for this is to
dynamically set the correct mdiobus configuration (a legacy dsa one,
or a custom dedicated one)
To solve this problem, we just drop the phy_read/phy_write and we
declare a custom mdiobus in any case. 
This way we can use a static dsa switch ops struct and we can drop it
from qca8k_priv

Patch 4 finally drop the duplicated dsa_switch_ops.

Ansuel Smith (4):
  drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv
  drivers: net: dsa: qca8k: drop port_sts from qca8k_priv
  drivers: net: dsa: qca8k: rework and simplify mdiobus logic
  drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

 drivers/net/dsa/qca8k.c | 157 +++++++++++++++++-----------------------
 drivers/net/dsa/qca8k.h |  12 +--
 2 files changed, 71 insertions(+), 98 deletions(-)

-- 
2.34.1

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

[Ansuel Smith]

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@gmail.com>
---
 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

[net-next PATCH 2/4] drivers: net: dsa: qca8k: drop port_sts from qca8k_priv

[Ansuel Smith]

Port_sts is a thing of the past for this driver. It was something
present on the initial implementation of this driver and parts of the
original struct were dropped over time. Using an array of int to store if
a port is enabled or not to handle PM operation seems overkill. Switch
and use a simple u8 to store the port status where each bit correspond
to a port. (bit is set port is enabled, bit is not set, port is disabled)
Also add some comments to better describe why we need to track port
status.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 15 +++++++++------
 drivers/net/dsa/qca8k.h |  9 ++++-----
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4366d87b4bbd..33cedae6875c 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2346,7 +2346,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
 	qca8k_port_set_status(priv, port, 1);
-	priv->port_sts[port].enabled = 1;
+	priv->port_enabled_map |= BIT(port);
 
 	if (dsa_is_user_port(ds, port))
 		phy_support_asym_pause(phy);
@@ -2360,7 +2360,7 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
 	qca8k_port_set_status(priv, port, 0);
-	priv->port_sts[port].enabled = 0;
+	priv->port_enabled_map &= ~BIT(port);
 }
 
 static int
@@ -3251,13 +3251,16 @@ static void qca8k_sw_shutdown(struct mdio_device *mdiodev)
 static void
 qca8k_set_pm(struct qca8k_priv *priv, int enable)
 {
-	int i;
+	int port;
 
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		if (!priv->port_sts[i].enabled)
+	for (port = 0; port < QCA8K_NUM_PORTS; port++) {
+		/* Do not enable on resume if the port was
+		 * disabled before.
+		 */
+		if (!(priv->port_enabled_map & BIT(port)))
 			continue;
 
-		qca8k_port_set_status(priv, i, enable);
+		qca8k_port_set_status(priv, port, enable);
 	}
 }
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 562d75997e55..12d8d090298b 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -324,10 +324,6 @@ enum qca8k_mid_cmd {
 	QCA8K_MIB_CAST = 3,
 };
 
-struct ar8xxx_port_status {
-	int enabled;
-};
-
 struct qca8k_match_data {
 	u8 id;
 	bool reduced_package;
@@ -388,11 +384,14 @@ struct qca8k_priv {
 	u8 mirror_rx;
 	u8 mirror_tx;
 	u8 lag_hash_mode;
+	/* Each bit correspond to a port. This switch can support a max of 7 port.
+	 * Bit 1: port enabled. Bit 0: port disabled.
+	 */
+	u8 port_enabled_map;
 	bool legacy_phy_port_mapping;
 	struct qca8k_ports_config ports_config;
 	struct regmap *regmap;
 	struct mii_bus *bus;
-	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
-- 
2.34.1

[net-next PATCH 3/4] drivers: net: dsa: qca8k: rework and simplify mdiobus logic

[Ansuel Smith]

In an attempt to reduce qca8k_priv space, rework and simplify mdiobus
logic.
We now declare a mdiobus instead of relying on DSA phy_read/write even
if a mdio node is not present. This is all to make the qca8k ops static
and not switch specific. With a legacy implementation where port doesn't
have a phy map declared in the dts with a mdio node, we declare a
'qca8k-legacy' mdiobus. The conversion logic is used as legacy read and
write ops are used instead of the internal one.
While at it also improve the bus id to support multiple switch.
Also drop the legacy_phy_port_mapping as we now declare mdiobus with ops
that already address the workaround.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 101 ++++++++++++++--------------------------
 drivers/net/dsa/qca8k.h |   1 -
 2 files changed, 34 insertions(+), 68 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 33cedae6875c..c837444d37f6 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1287,87 +1287,71 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
 	if (ret >= 0)
 		return ret;
 
-	return qca8k_mdio_read(priv, phy, regnum);
+	ret = qca8k_mdio_read(priv, phy, regnum);
+
+	if (ret < 0)
+		return 0xffff;
+
+	return ret;
 }
 
 static int
-qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
+qca8k_legacy_mdio_write(struct mii_bus *slave_bus, int port, int regnum, u16 data)
 {
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	/* Check if the legacy mapping should be used and the
-	 * port is not correctly mapped to the right PHY in the
-	 * devicetree
-	 */
-	if (priv->legacy_phy_port_mapping)
-		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
-
-	/* Use mdio Ethernet when available, fallback to legacy one on error */
-	ret = qca8k_phy_eth_command(priv, false, port, regnum, 0);
-	if (!ret)
-		return ret;
+	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
-	return qca8k_mdio_write(priv, port, regnum, data);
+	return qca8k_internal_mdio_write(slave_bus, port, regnum, data);
 }
 
 static int
-qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
+qca8k_legacy_mdio_read(struct mii_bus *slave_bus, int port, int regnum)
 {
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	/* Check if the legacy mapping should be used and the
-	 * port is not correctly mapped to the right PHY in the
-	 * devicetree
-	 */
-	if (priv->legacy_phy_port_mapping)
-		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
-
-	/* Use mdio Ethernet when available, fallback to legacy one on error */
-	ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
-	if (ret >= 0)
-		return ret;
-
-	ret = qca8k_mdio_read(priv, port, regnum);
-
-	if (ret < 0)
-		return 0xffff;
+	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
-	return ret;
+	return qca8k_internal_mdio_read(slave_bus, port, regnum);
 }
 
 static int
-qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
+qca8k_mdio_register(struct qca8k_priv *priv)
 {
 	struct dsa_switch *ds = priv->ds;
+	struct device_node *mdio;
 	struct mii_bus *bus;
 
 	bus = devm_mdiobus_alloc(ds->dev);
-
 	if (!bus)
 		return -ENOMEM;
 
 	bus->priv = (void *)priv;
-	bus->name = "qca8k slave mii";
-	bus->read = qca8k_internal_mdio_read;
-	bus->write = qca8k_internal_mdio_write;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
-		 ds->index);
-
+	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
+		 ds->dst->index, ds->index);
 	bus->parent = ds->dev;
 	bus->phy_mask = ~ds->phys_mii_mask;
-
 	ds->slave_mii_bus = bus;
 
-	return devm_of_mdiobus_register(priv->dev, bus, mdio);
+	/* Check if the devicetree declare the port:phy mapping */
+	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
+	if (of_device_is_available(mdio)) {
+		bus->name = "qca8k slave mii";
+		bus->read = qca8k_internal_mdio_read;
+		bus->write = qca8k_internal_mdio_write;
+		return devm_of_mdiobus_register(priv->dev, bus, mdio);
+	}
+
+	/* If a mapping can't be found the legacy mapping is used,
+	 * using the qca8k_port_to_phy function
+	 */
+	bus->name = "qca8k-legacy slave mii";
+	bus->read = qca8k_legacy_mdio_read;
+	bus->write = qca8k_legacy_mdio_write;
+	return devm_mdiobus_register(priv->dev, bus);
 }
 
 static int
 qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 {
 	u32 internal_mdio_mask = 0, external_mdio_mask = 0, reg;
-	struct device_node *ports, *port, *mdio;
+	struct device_node *ports, *port;
 	phy_interface_t mode;
 	int err;
 
@@ -1429,24 +1413,7 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 					 QCA8K_MDIO_MASTER_EN);
 	}
 
-	/* Check if the devicetree declare the port:phy mapping */
-	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
-	if (of_device_is_available(mdio)) {
-		err = qca8k_mdio_register(priv, mdio);
-		if (err)
-			of_node_put(mdio);
-
-		return err;
-	}
-
-	/* If a mapping can't be found the legacy mapping is used,
-	 * using the qca8k_port_to_phy function
-	 */
-	priv->legacy_phy_port_mapping = true;
-	priv->ops.phy_read = qca8k_phy_read;
-	priv->ops.phy_write = qca8k_phy_write;
-
-	return 0;
+	return qca8k_mdio_register(priv);
 }
 
 static int
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 12d8d090298b..8bbe36f135b5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -388,7 +388,6 @@ struct qca8k_priv {
 	 * Bit 1: port enabled. Bit 0: port disabled.
 	 */
 	u8 port_enabled_map;
-	bool legacy_phy_port_mapping;
 	struct qca8k_ports_config ports_config;
 	struct regmap *regmap;
 	struct mii_bus *bus;
-- 
2.34.1

[net-next PATCH 4/4] drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

[Ansuel Smith]

Now that dsa_switch_ops is not switch specific anymore, we can drop it
from qca8k_priv and use the static ops directly for the dsa_switch
pointer.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 3 +--
 drivers/net/dsa/qca8k.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index c837444d37f6..38567080e7b3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -3177,8 +3177,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
 	priv->ds->priv = priv;
-	priv->ops = qca8k_switch_ops;
-	priv->ds->ops = &priv->ops;
+	priv->ds->ops = &qca8k_switch_ops;
 	mutex_init(&priv->reg_mutex);
 	dev_set_drvdata(&mdiodev->dev, priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 8bbe36f135b5..04408e11402a 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -394,7 +394,6 @@ struct qca8k_priv {
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
-	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
 	struct qca8k_mgmt_eth_data mgmt_eth_data;
-- 
2.34.1

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

[Vladimir Oltean]

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@gmail.com>
> ---

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?

>  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
> 

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@gmail.com>
> > ---
> 
> 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

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

[Vladimir Oltean]

On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote:
> 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@gmail.com>
> > > ---
> > 
> > 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.

Where is the bug?

> 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.

I never had the pleasure of dealing with such DSA masters. I wonder why
can't stmmac_change_mtu() check if netif_running(), call dev_close and
set a bool, and at the end, if the bool was set, call dev_open back?

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

[Ansuel Smith]

On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote:
> > 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@gmail.com>
> > > > ---
> > > 
> > > 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.
> 
> Where is the bug?
>

There was a bug where we tracked the MTU with the FCS and L2 added and
then in the change_mtu code we added another time the FCS and L2 header
just because we used this custom way and nobody notice that we were adding
2 times the same headers. (it's now fixed but still it's a reason why
using standard way to track MTU would have prevented that)

> > 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.
> 
> I never had the pleasure of dealing with such DSA masters. I wonder why
> can't stmmac_change_mtu() check if netif_running(), call dev_close and
> set a bool, and at the end, if the bool was set, call dev_open back?

Oh ok so it's not standard that stmmac_change_mtu() just refuse to
change the MTU instead of put the interface down, change MTU and reopen
it... Fun stuff...

From system side to change MTU to a new value (so lower MTU on any port
or set MTU to a higher value for one pot) I have to:
1. ifconfig eth0 down
2. ifconfig lan1 mtu 1600 up
3. ifconfig eth up

If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac
complaining.

-- 
	Ansuel

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

[Vladimir Oltean]

On Tue, Mar 22, 2022 at 03:03:36PM +0100, Ansuel Smith wrote:
> On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote:
> > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote:
> > > 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@gmail.com>
> > > > > ---
> > > > 
> > > > 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.
> > 
> > Where is the bug?
> 
> There was a bug where we tracked the MTU with the FCS and L2 added and
> then in the change_mtu code we added another time the FCS and L2 header
> just because we used this custom way and nobody notice that we were adding
> 2 times the same headers. (it's now fixed but still it's a reason why
> using standard way to track MTU would have prevented that)

No, I'm sorry, this is completely unjustified complexity - not to
mention it's buggy, too. Does qca8k support cascaded setups? Because if
it does:

	/* 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;

		if (dp->index == port)	// <- this will exclude from the max MTU calculation the ports in other switches that are numerically equal to @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;
	}

Not to mention it's missing the blatantly obvious. DSA calls
->port_change_mtu() on the CPU port with the max MTU, every time that
changes.

You need the max MTU.

Why calculate it again? Why don't you do what mt7530 does, which has a
similar restriction, and just program the hardware when the CPU port MTU
is updated?

You may think - does this work with multiple CPU ports? Well, yes it
does, since DSA calculates the largest MTU across the entire tree, and
not just across the user ports affine to a certain CPU port.

If it wasn't for this possibility, I would have been in favor of
introducing a dsa_tree_largest_mtu(dst) helper in the DSA core, but I
can't find it justifiable.

> > > 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.
> > 
> > I never had the pleasure of dealing with such DSA masters. I wonder why
> > can't stmmac_change_mtu() check if netif_running(), call dev_close and
> > set a bool, and at the end, if the bool was set, call dev_open back?
> 
> Oh ok so it's not standard that stmmac_change_mtu() just refuse to
> change the MTU instead of put the interface down, change MTU and reopen
> it... Fun stuff...
> 
> From system side to change MTU to a new value (so lower MTU on any port
> or set MTU to a higher value for one pot) I have to:
> 1. ifconfig eth0 down
> 2. ifconfig lan1 mtu 1600 up
> 3. ifconfig eth up
> 
> If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac
> complaining.

Not sure if there is any hard line on this. But I made a poll, and the
crushing majority of drivers in drivers/net/ethernet/ do not require
!netif_running() in ndo_change_mtu. The ones that do are:

nixge
macb
altera tse
axienet
renesas sh
ksz884x
bcm63xx_enet
sundance
stmmac

(compared to more than 100 that don't, and even have a dedicated code
path for live changes)

By the way, an an interesting aside - I've found the xgene, atl1c and
xgmac drivers to be obviously odd (meaning that more drivers might be
odd in the same way, but in more subtle ways I haven't noticed):
when netif_running() is false, they simply return 0, but they don't
change dev->mtu either, they just ignore the request.

So on one hand you have drivers that _want_ to be down to change the
MTU, and on the other you have drivers that silently ignore MTU changes
when they're down. Hard for DSA to do something reasonable to handle
both cases...

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

[Ansuel Smith]

On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 22, 2022 at 03:03:36PM +0100, Ansuel Smith wrote:
> > On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote:
> > > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote:
> > > > 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@gmail.com>
> > > > > > ---
> > > > > 
> > > > > 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.
> > > 
> > > Where is the bug?
> > 
> > There was a bug where we tracked the MTU with the FCS and L2 added and
> > then in the change_mtu code we added another time the FCS and L2 header
> > just because we used this custom way and nobody notice that we were adding
> > 2 times the same headers. (it's now fixed but still it's a reason why
> > using standard way to track MTU would have prevented that)
> 
> No, I'm sorry, this is completely unjustified complexity - not to
> mention it's buggy, too. Does qca8k support cascaded setups? Because if
> it does:
> 
> 	/* 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;
> 
> 		if (dp->index == port)	// <- this will exclude from the max MTU calculation the ports in other switches that are numerically equal to @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;
> 	}
> 
> Not to mention it's missing the blatantly obvious. DSA calls
> ->port_change_mtu() on the CPU port with the max MTU, every time that
> changes.
> 
> You need the max MTU.
> 
> Why calculate it again? Why don't you do what mt7530 does, which has a
> similar restriction, and just program the hardware when the CPU port MTU
> is updated?
>

I just checked and wow it was that easy...
Also wonder if I should add some check for jumbo frame... (I should
check what is the max MTU for the switch and if it can accept jumbo
frame+fcs+l2)

> You may think - does this work with multiple CPU ports? Well, yes it
> does, since DSA calculates the largest MTU across the entire tree, and
> not just across the user ports affine to a certain CPU port.
> 
> If it wasn't for this possibility, I would have been in favor of
> introducing a dsa_tree_largest_mtu(dst) helper in the DSA core, but I
> can't find it justifiable.
> 
> > > > 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.
> > > 
> > > I never had the pleasure of dealing with such DSA masters. I wonder why
> > > can't stmmac_change_mtu() check if netif_running(), call dev_close and
> > > set a bool, and at the end, if the bool was set, call dev_open back?
> > 
> > Oh ok so it's not standard that stmmac_change_mtu() just refuse to
> > change the MTU instead of put the interface down, change MTU and reopen
> > it... Fun stuff...
> > 
> > From system side to change MTU to a new value (so lower MTU on any port
> > or set MTU to a higher value for one pot) I have to:
> > 1. ifconfig eth0 down
> > 2. ifconfig lan1 mtu 1600 up
> > 3. ifconfig eth up
> > 
> > If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac
> > complaining.
> 
> Not sure if there is any hard line on this. But I made a poll, and the
> crushing majority of drivers in drivers/net/ethernet/ do not require
> !netif_running() in ndo_change_mtu. The ones that do are:
> 
> nixge
> macb
> altera tse
> axienet
> renesas sh
> ksz884x
> bcm63xx_enet
> sundance
> stmmac
> 
> (compared to more than 100 that don't, and even have a dedicated code
> path for live changes)
> 
> By the way, an an interesting aside - I've found the xgene, atl1c and
> xgmac drivers to be obviously odd (meaning that more drivers might be
> odd in the same way, but in more subtle ways I haven't noticed):
> when netif_running() is false, they simply return 0, but they don't
> change dev->mtu either, they just ignore the request.
> 
> So on one hand you have drivers that _want_ to be down to change the
> MTU, and on the other you have drivers that silently ignore MTU changes
> when they're down. Hard for DSA to do something reasonable to handle
> both cases...

Wonder if I should propose a change for stmmac and just drop the
interface and restart it when the change is down.

-- 
	Ansuel

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

[Vladimir Oltean]

On Thu, Mar 24, 2022 at 09:44:27PM +0100, Ansuel Smith wrote:
> On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote:
> > You need the max MTU.
> > 
> > Why calculate it again? Why don't you do what mt7530 does, which has a
> > similar restriction, and just program the hardware when the CPU port MTU
> > is updated?
> >
> 
> I just checked and wow it was that easy...
> Also wonder if I should add some check for jumbo frame... (I should
> check what is the max MTU for the switch and if it can accept jumbo
> frame+fcs+l2)

I'm not following you, sorry. What is your definition of "jumbo frame",
and what check is there to add?

> Wonder if I should propose a change for stmmac and just drop the
> interface and restart it when the change is down.

In the case of stmmac, dev->mtu is considered from stmmac_fix_features()
and from init_dma_rx_desc_rings(), so yes, putting the interface down
before updating the MTU and the device features, and then putting it
back up if it was previously up, should do the trick. Other drivers like
gianfar and many others do this too.

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

[Ansuel Smith]

On Thu, Mar 24, 2022 at 11:05:08PM +0200, Vladimir Oltean wrote:
> On Thu, Mar 24, 2022 at 09:44:27PM +0100, Ansuel Smith wrote:
> > On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote:
> > > You need the max MTU.
> > > 
> > > Why calculate it again? Why don't you do what mt7530 does, which has a
> > > similar restriction, and just program the hardware when the CPU port MTU
> > > is updated?
> > >
> > 
> > I just checked and wow it was that easy...
> > Also wonder if I should add some check for jumbo frame... (I should
> > check what is the max MTU for the switch and if it can accept jumbo
> > frame+fcs+l2)
> 
> I'm not following you, sorry. What is your definition of "jumbo frame",
> and what check is there to add?
>

With jumbo frame i mean frame with MTU of 9000. This is supported by
this switch. Anyway i just checked and the reg can totally accept a 9000
MTU + fcs + l2

> > Wonder if I should propose a change for stmmac and just drop the
> > interface and restart it when the change is down.
> 
> In the case of stmmac, dev->mtu is considered from stmmac_fix_features()
> and from init_dma_rx_desc_rings(), so yes, putting the interface down
> before updating the MTU and the device features, and then putting it
> back up if it was previously up, should do the trick. Other drivers like
> gianfar and many others do this too.

Ok i'm reworking this in v2 with the stmmac change and the change to
change mtu following what is done for mt7530. Thx a lot for the
suggestion. Happy that the additional space can be dropped and still use
a more correct and simple approach.

-- 
	Ansuel

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

[Vladimir Oltean]

On Fri, Mar 25, 2022 at 12:10:19AM +0100, Ansuel Smith wrote:
> Ok i'm reworking this in v2 with the stmmac change and the change to
> change mtu following what is done for mt7530. Thx a lot for the
> suggestion. Happy that the additional space can be dropped and still use
> a more correct and simple approach.

That's all fine, but if you read the news you'll notice that net-next is
currently closed and will probably be so for around 2 weeks.
The pull request for 5.18 was sent by Jakub yesterday:
https://patchwork.kernel.org/project/netdevbpf/patch/20220323180738.3978487-1-kuba@kernel.org/
Not sure why the status page says it's still open, it'll probably be
updated eventually:
http://vger.kernel.org/~davem/net-next.html

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

[Ansuel Smith]

On Fri, Mar 25, 2022 at 01:14:23AM +0200, Vladimir Oltean wrote:
> On Fri, Mar 25, 2022 at 12:10:19AM +0100, Ansuel Smith wrote:
> > Ok i'm reworking this in v2 with the stmmac change and the change to
> > change mtu following what is done for mt7530. Thx a lot for the
> > suggestion. Happy that the additional space can be dropped and still use
> > a more correct and simple approach.
> 
> That's all fine, but if you read the news you'll notice that net-next is
> currently closed and will probably be so for around 2 weeks.
> The pull request for 5.18 was sent by Jakub yesterday:
> https://patchwork.kernel.org/project/netdevbpf/patch/20220323180738.3978487-1-kuba@kernel.org/
> Not sure why the status page says it's still open, it'll probably be
> updated eventually:
> http://vger.kernel.org/~davem/net-next.html

Thanks for the alert! I will send an RFC then hoping to get some review
tag.
About the html page, I was also confused some times ago where net-next
was closed and the page was with the "We are open" image. Wonder if it's
a CDN problem? No idea but to me that page looks to be a quicker way
than checking net-next last commit or checking the mailing list.

-- 
	Ansuel