[PATCH] net: dsa: qca8k: implement the port MTU callbacks

[PATCH] net: dsa: qca8k: implement the port MTU callbacks

[Jonathan McDowell]

This switch has a single max frame size configuration register, so we
track the requested MTU for each port and apply the largest.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4acad5fa0c84..3690f02aea3a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -670,6 +670,12 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* Setup our port MTUs to match power on defaults */
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
+	}
+	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
@@ -1098,6 +1104,36 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
 	priv->port_sts[port].enabled = 0;
 }
 
+static int
+qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
+{
+	struct qca8k_priv *priv = ds->priv;
+	int i, mtu;
+
+	if ((new_mtu < ETH_MIN_MTU) || (new_mtu > QCA8K_MAX_MTU)) {
+		return -EINVAL;
+	}
+
+	priv->port_mtu[port] = new_mtu;
+
+	mtu = 0;
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		if (priv->port_mtu[port] > mtu)
+			mtu = priv->port_mtu[port];
+	}
+
+	/* Include L2 header / FCS length */
+	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
+
+	return 0;
+}
+
+static int
+qca8k_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	return QCA8K_MAX_MTU;
+}
+
 static int
 qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 		      u16 port_mask, u16 vid)
@@ -1174,6 +1210,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.set_mac_eee		= qca8k_set_mac_eee,
 	.port_enable		= qca8k_port_enable,
 	.port_disable		= qca8k_port_disable,
+	.port_change_mtu	= qca8k_port_change_mtu,
+	.port_max_mtu		= qca8k_port_max_mtu,
 	.port_stp_state_set	= qca8k_port_stp_state_set,
 	.port_bridge_join	= qca8k_port_bridge_join,
 	.port_bridge_leave	= qca8k_port_bridge_leave,
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 10ef2bca2cde..31439396401c 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -13,6 +13,7 @@
 #include <linux/gpio.h>
 
 #define QCA8K_NUM_PORTS					7
+#define QCA8K_MAX_MTU					9000
 
 #define PHY_ID_QCA8337					0x004dd036
 #define QCA8K_ID_QCA8337				0x13
@@ -58,6 +59,7 @@
 #define   QCA8K_MDIO_MASTER_MAX_REG			32
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
+#define QCA8K_MAX_FRAME_SIZE				0x78
 #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
 #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
 #define   QCA8K_PORT_STATUS_SPEED_10			0
@@ -189,6 +191,7 @@ struct qca8k_priv {
 	struct device *dev;
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
+	unsigned int port_mtu[QCA8K_NUM_PORTS];
 };
 
 struct qca8k_mib_desc {
-- 
2.27.0

Re: [PATCH] net: dsa: qca8k: implement the port MTU callbacks

[Vladimir Oltean]

On Sat, Jul 18, 2020 at 10:35:55AM +0100, Jonathan McDowell wrote:
> This switch has a single max frame size configuration register, so we
> track the requested MTU for each port and apply the largest.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 4acad5fa0c84..3690f02aea3a 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -670,6 +670,12 @@ qca8k_setup(struct dsa_switch *ds)
>  		}
>  	}
>  
> +	/* Setup our port MTUs to match power on defaults */
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
> +	}

I am not quite sure the curly brackets are needed. And nowhere else in
qca8k.c is this convention being used.

> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
> +
>  	/* Flush the FDB table */
>  	qca8k_fdb_flush(priv);
>  
> @@ -1098,6 +1104,36 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
>  	priv->port_sts[port].enabled = 0;
>  }
>  
> +static int
> +qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	int i, mtu;
> +
> +	if ((new_mtu < ETH_MIN_MTU) || (new_mtu > QCA8K_MAX_MTU)) {
> +		return -EINVAL;
> +	}

I'm pretty sure this check should not be needed.
The only reason why slave_dev->min_mtu is 0 seems to be:

commit 8b1efc0f83f1f75b8f85c70d2211007de8fd7633
Author: Jarod Wilson <jarod@redhat.com>
Date:   Thu Oct 20 23:25:27 2016 -0400

    net: remove MTU limits on a few ether_setup callers

    These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
    were overlooked for changes to MTU range checking behavior. They
    previously had no range checks, so for feature-parity, set their min_mtu
    to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
    inherited from the ether_setup() changes. Fine-tuning can come after we get
    back to full feature-parity here.

    CC: netdev@vger.kernel.org
    Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
    CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
    CC: R Parameswaran <parameswaran.r7@gmail.com>
    Signed-off-by: Jarod Wilson <jarod@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

which is an oversight on my part. Since now DSA supports
ndo_change_mtu(), the "slave_dev->min_mtu = 0;" line in net/dsa/slave.c
can be removed and so can this check.

> +
> +	priv->port_mtu[port] = new_mtu;
> +
> +	mtu = 0;

I think it's more typical to initialize mtu to 0 at declaration time.

> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		if (priv->port_mtu[port] > mtu)
> +			mtu = priv->port_mtu[port];
> +	}

Again, curly brackets are not needed here, although some might feel it
aids readability.

> +
> +	/* Include L2 header / FCS length */
> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return QCA8K_MAX_MTU;

So what is the maximum value that you can write into
QCA8K_MAX_FRAME_SIZE? 9000 or 9018? If it's 9000, you should report a
max MTU of 8982, and let the network stack do the range check for you,
that's why this callback exists in the first place.

Do you know how are VLAN tags accounted for (i.e. does iperf3 TCP work
over a VLAN sub-interface after your patch)? There are 2 options:
- The ports automatically increase the maximum accepted frame size by 4
  (or 8, in case of double tag) bytes if they see VLAN tagged traffic.
  Case in which you don't need to do anything.
- You need to manually account for the possibility that VLAN-tagged
  traffic will be received, since the 802.1Q header is not part of the
  SDU whose max length is measured by the MTU. So you might want to
  write a value to QCA8K_MAX_FRAME_SIZE that is either "mtu +
  VLAN_ETH_HLEN + ETH_FCS_LEN", or "mtu + ETH_HLEN + 2 * VLAN_HLEN +
  ETH_FCS_LEN", depending on whether you foresee double-tagging being
  used.

> +}
> +
>  static int
>  qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  		      u16 port_mask, u16 vid)
> @@ -1174,6 +1210,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.set_mac_eee		= qca8k_set_mac_eee,
>  	.port_enable		= qca8k_port_enable,
>  	.port_disable		= qca8k_port_disable,
> +	.port_change_mtu	= qca8k_port_change_mtu,
> +	.port_max_mtu		= qca8k_port_max_mtu,
>  	.port_stp_state_set	= qca8k_port_stp_state_set,
>  	.port_bridge_join	= qca8k_port_bridge_join,
>  	.port_bridge_leave	= qca8k_port_bridge_leave,
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 10ef2bca2cde..31439396401c 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -13,6 +13,7 @@
>  #include <linux/gpio.h>
>  
>  #define QCA8K_NUM_PORTS					7
> +#define QCA8K_MAX_MTU					9000
>  
>  #define PHY_ID_QCA8337					0x004dd036
>  #define QCA8K_ID_QCA8337				0x13
> @@ -58,6 +59,7 @@
>  #define   QCA8K_MDIO_MASTER_MAX_REG			32
>  #define QCA8K_GOL_MAC_ADDR0				0x60
>  #define QCA8K_GOL_MAC_ADDR1				0x64
> +#define QCA8K_MAX_FRAME_SIZE				0x78
>  #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
>  #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
>  #define   QCA8K_PORT_STATUS_SPEED_10			0
> @@ -189,6 +191,7 @@ struct qca8k_priv {
>  	struct device *dev;
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
> +	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.27.0
> 

Thanks,
-Vladimir

Re: [PATCH] net: dsa: qca8k: implement the port MTU callbacks

[Jonathan McDowell]

On Sat, Jul 18, 2020 at 01:38:08PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 18, 2020 at 10:35:55AM +0100, Jonathan McDowell wrote:
> > This switch has a single max frame size configuration register, so we
> > track the requested MTU for each port and apply the largest.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/net/dsa/qca8k.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |  3 +++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 4acad5fa0c84..3690f02aea3a 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -670,6 +670,12 @@ qca8k_setup(struct dsa_switch *ds)
> >  		}
> >  	}
> >  
> > +	/* Setup our port MTUs to match power on defaults */
> > +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > +		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
> > +	}
> 
> I am not quite sure the curly brackets are needed. And nowhere else in
> qca8k.c is this convention being used.

Good point; force of habit from coding standards elsewhere. Fixed (and
also the instance below).

> > +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
> > +
> >  	/* Flush the FDB table */
> >  	qca8k_fdb_flush(priv);
> >  
> > @@ -1098,6 +1104,36 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
> >  	priv->port_sts[port].enabled = 0;
> >  }
> >  
> > +static int
> > +qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	int i, mtu;
> > +
> > +	if ((new_mtu < ETH_MIN_MTU) || (new_mtu > QCA8K_MAX_MTU)) {
> > +		return -EINVAL;
> > +	}
> 
> I'm pretty sure this check should not be needed.
> The only reason why slave_dev->min_mtu is 0 seems to be:
> 
> commit 8b1efc0f83f1f75b8f85c70d2211007de8fd7633
> Author: Jarod Wilson <jarod@redhat.com>
> Date:   Thu Oct 20 23:25:27 2016 -0400
> 
>     net: remove MTU limits on a few ether_setup callers
> 
>     These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
>     were overlooked for changes to MTU range checking behavior. They
>     previously had no range checks, so for feature-parity, set their min_mtu
>     to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
>     inherited from the ether_setup() changes. Fine-tuning can come after we get
>     back to full feature-parity here.
> 
>     CC: netdev@vger.kernel.org
>     Reported-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
>     CC: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
>     CC: R Parameswaran <parameswaran.r7@gmail.com>
>     Signed-off-by: Jarod Wilson <jarod@redhat.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> which is an oversight on my part. Since now DSA supports
> ndo_change_mtu(), the "slave_dev->min_mtu = 0;" line in net/dsa/slave.c
> can be removed and so can this check.

Ok.

> > +
> > +	priv->port_mtu[port] = new_mtu;
> > +
> > +	mtu = 0;
> 
> I think it's more typical to initialize mtu to 0 at declaration time.

Sure, fixed.

> > +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > +		if (priv->port_mtu[port] > mtu)
> > +			mtu = priv->port_mtu[port];
> > +	}
> 
> Again, curly brackets are not needed here, although some might feel it
> aids readability.
> 
> > +
> > +	/* Include L2 header / FCS length */
> > +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_port_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > +	return QCA8K_MAX_MTU;
> 
> So what is the maximum value that you can write into
> QCA8K_MAX_FRAME_SIZE? 9000 or 9018? If it's 9000, you should report a
> max MTU of 8982, and let the network stack do the range check for you,
> that's why this callback exists in the first place.

9018 (including header + fcs), hence returning QCA8K_MAX_MTU here.

> Do you know how are VLAN tags accounted for (i.e. does iperf3 TCP work
> over a VLAN sub-interface after your patch)? There are 2 options:
> - The ports automatically increase the maximum accepted frame size by 4
>   (or 8, in case of double tag) bytes if they see VLAN tagged traffic.
>   Case in which you don't need to do anything.
> - You need to manually account for the possibility that VLAN-tagged
>   traffic will be received, since the 802.1Q header is not part of the
>   SDU whose max length is measured by the MTU. So you might want to
>   write a value to QCA8K_MAX_FRAME_SIZE that is either "mtu +
>   VLAN_ETH_HLEN + ETH_FCS_LEN", or "mtu + ETH_HLEN + 2 * VLAN_HLEN +
>   ETH_FCS_LEN", depending on whether you foresee double-tagging being
>   used.

The value is for a normal packet; the switch accounts for VLAN / double
VLAN / the switch tagging header size itself.

> > +}
> > +
> >  static int
> >  qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
> >  		      u16 port_mask, u16 vid)
> > @@ -1174,6 +1210,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.set_mac_eee		= qca8k_set_mac_eee,
> >  	.port_enable		= qca8k_port_enable,
> >  	.port_disable		= qca8k_port_disable,
> > +	.port_change_mtu	= qca8k_port_change_mtu,
> > +	.port_max_mtu		= qca8k_port_max_mtu,
> >  	.port_stp_state_set	= qca8k_port_stp_state_set,
> >  	.port_bridge_join	= qca8k_port_bridge_join,
> >  	.port_bridge_leave	= qca8k_port_bridge_leave,
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 10ef2bca2cde..31439396401c 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -13,6 +13,7 @@
> >  #include <linux/gpio.h>
> >  
> >  #define QCA8K_NUM_PORTS					7
> > +#define QCA8K_MAX_MTU					9000
> >  
> >  #define PHY_ID_QCA8337					0x004dd036
> >  #define QCA8K_ID_QCA8337				0x13
> > @@ -58,6 +59,7 @@
> >  #define   QCA8K_MDIO_MASTER_MAX_REG			32
> >  #define QCA8K_GOL_MAC_ADDR0				0x60
> >  #define QCA8K_GOL_MAC_ADDR1				0x64
> > +#define QCA8K_MAX_FRAME_SIZE				0x78
> >  #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
> >  #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
> >  #define   QCA8K_PORT_STATUS_SPEED_10			0
> > @@ -189,6 +191,7 @@ struct qca8k_priv {
> >  	struct device *dev;
> >  	struct dsa_switch_ops ops;
> >  	struct gpio_desc *reset_gpio;
> > +	unsigned int port_mtu[QCA8K_NUM_PORTS];
> >  };
> >  
> >  struct qca8k_mib_desc {
> > -- 
> > 2.27.0

J.

-- 
/-\                             | 101 things you can't have too much
|@/  Debian GNU/Linux Developer |       of : 19 - A Good Thing.
\-                              |

[PATCH v2] net: dsa: qca8k: implement the port MTU callbacks

[Jonathan McDowell]

This switch has a single max frame size configuration register, so we
track the requested MTU for each port and apply the largest.

v2:
- Address review feedback from Vladimir Oltean

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 31 +++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4acad5fa0c84..a5566de82853 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -670,6 +670,11 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* Setup our port MTUs to match power on defaults */
+	for (i = 0; i < QCA8K_NUM_PORTS; i++)
+		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
+	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
@@ -1098,6 +1103,30 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
 	priv->port_sts[port].enabled = 0;
 }
 
+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;
+
+	priv->port_mtu[port] = new_mtu;
+
+	for (i = 0; i < QCA8K_NUM_PORTS; i++)
+		if (priv->port_mtu[port] > mtu)
+			mtu = priv->port_mtu[port];
+
+	/* Include L2 header / FCS length */
+	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
+
+	return 0;
+}
+
+static int
+qca8k_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	return QCA8K_MAX_MTU;
+}
+
 static int
 qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 		      u16 port_mask, u16 vid)
@@ -1174,6 +1203,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.set_mac_eee		= qca8k_set_mac_eee,
 	.port_enable		= qca8k_port_enable,
 	.port_disable		= qca8k_port_disable,
+	.port_change_mtu	= qca8k_port_change_mtu,
+	.port_max_mtu		= qca8k_port_max_mtu,
 	.port_stp_state_set	= qca8k_port_stp_state_set,
 	.port_bridge_join	= qca8k_port_bridge_join,
 	.port_bridge_leave	= qca8k_port_bridge_leave,
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 10ef2bca2cde..31439396401c 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -13,6 +13,7 @@
 #include <linux/gpio.h>
 
 #define QCA8K_NUM_PORTS					7
+#define QCA8K_MAX_MTU					9000
 
 #define PHY_ID_QCA8337					0x004dd036
 #define QCA8K_ID_QCA8337				0x13
@@ -58,6 +59,7 @@
 #define   QCA8K_MDIO_MASTER_MAX_REG			32
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
+#define QCA8K_MAX_FRAME_SIZE				0x78
 #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
 #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
 #define   QCA8K_PORT_STATUS_SPEED_10			0
@@ -189,6 +191,7 @@ struct qca8k_priv {
 	struct device *dev;
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
+	unsigned int port_mtu[QCA8K_NUM_PORTS];
 };
 
 struct qca8k_mib_desc {
-- 
2.27.0

Re: [PATCH v2] net: dsa: qca8k: implement the port MTU callbacks

[Vladimir Oltean]

On Sat, Jul 18, 2020 at 05:32:14PM +0100, Jonathan McDowell wrote:
> This switch has a single max frame size configuration register, so we
> track the requested MTU for each port and apply the largest.
> 
> v2:
> - Address review feedback from Vladimir Oltean
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---

I think there's still an open question on whether the slave_dev->mtu of
all other ports should be updated to reflect the new, potentially larger
value. But we could always update drivers after the fact, if that ever
becomes required.

Acked-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/qca8k.c | 31 +++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 4acad5fa0c84..a5566de82853 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -670,6 +670,11 @@ qca8k_setup(struct dsa_switch *ds)
>  		}
>  	}
>  
> +	/* Setup our port MTUs to match power on defaults */
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++)
> +		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
> +
>  	/* Flush the FDB table */
>  	qca8k_fdb_flush(priv);
>  
> @@ -1098,6 +1103,30 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
>  	priv->port_sts[port].enabled = 0;
>  }
>  
> +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;
> +
> +	priv->port_mtu[port] = new_mtu;
> +
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++)
> +		if (priv->port_mtu[port] > mtu)
> +			mtu = priv->port_mtu[port];
> +
> +	/* Include L2 header / FCS length */
> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return QCA8K_MAX_MTU;
> +}
> +
>  static int
>  qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  		      u16 port_mask, u16 vid)
> @@ -1174,6 +1203,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.set_mac_eee		= qca8k_set_mac_eee,
>  	.port_enable		= qca8k_port_enable,
>  	.port_disable		= qca8k_port_disable,
> +	.port_change_mtu	= qca8k_port_change_mtu,
> +	.port_max_mtu		= qca8k_port_max_mtu,
>  	.port_stp_state_set	= qca8k_port_stp_state_set,
>  	.port_bridge_join	= qca8k_port_bridge_join,
>  	.port_bridge_leave	= qca8k_port_bridge_leave,
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 10ef2bca2cde..31439396401c 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -13,6 +13,7 @@
>  #include <linux/gpio.h>
>  
>  #define QCA8K_NUM_PORTS					7
> +#define QCA8K_MAX_MTU					9000
>  
>  #define PHY_ID_QCA8337					0x004dd036
>  #define QCA8K_ID_QCA8337				0x13
> @@ -58,6 +59,7 @@
>  #define   QCA8K_MDIO_MASTER_MAX_REG			32
>  #define QCA8K_GOL_MAC_ADDR0				0x60
>  #define QCA8K_GOL_MAC_ADDR1				0x64
> +#define QCA8K_MAX_FRAME_SIZE				0x78
>  #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
>  #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
>  #define   QCA8K_PORT_STATUS_SPEED_10			0
> @@ -189,6 +191,7 @@ struct qca8k_priv {
>  	struct device *dev;
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
> +	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.27.0
> 

Re: [PATCH v2] net: dsa: qca8k: implement the port MTU callbacks

[David Miller]

From: Jonathan McDowell <noodles@earth.li>
Date: Sat, 18 Jul 2020 17:32:14 +0100

> This switch has a single max frame size configuration register, so we
> track the requested MTU for each port and apply the largest.
> 
> v2:
> - Address review feedback from Vladimir Oltean
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Applied to net-next, thanks.