[PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

This chips supports two ways to configure max MTU size:
- by setting SW_LEGAL_PACKET_DISABLE bit: if this bit is 0 allowed packed size
  will be between 64 and bytes 1518. If this bit is 1, it will accept
  packets up to 2000 bytes.
- by setting SW_JUMBO_PACKET bit. If this bit is set, the chip will
  ignore SW_LEGAL_PACKET_DISABLE value and use REG_SW_MTU__2 register to
  configure MTU size.

Current driver has disabled SW_JUMBO_PACKET bit and activates
SW_LEGAL_PACKET_DISABLE. So the switch will pass all packets up to 2000 without
any way to configure it.

By providing port_change_mtu we are switch to SW_JUMBO_PACKET way and will
be able to configure MTU up to ~9000.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v2:
- rename max_mtu to max_frame and new_mtu to frame_size
- use max() instead of if(>)
---
 drivers/net/dsa/microchip/ksz9477.c     | 40 +++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz9477_reg.h |  4 +++
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 18ffc8ded7ee..5c5f78cb970e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -11,6 +11,7 @@
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
 
@@ -182,6 +183,33 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 			   bits, set ? bits : 0);
 }
 
+static int ksz9477_change_mtu(struct dsa_switch *ds, int port, int mtu)
+{
+	struct ksz_device *dev = ds->priv;
+	u16 frame_size, max_frame = 0;
+	int i;
+
+	frame_size = mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN;
+
+	if (dsa_is_cpu_port(ds, port))
+		frame_size += KSZ9477_INGRESS_TAG_LEN;
+
+	/* Cache the per-port MTU setting */
+	dev->ports[port].max_frame = frame_size;
+
+	for (i = 0; i < dev->port_cnt; i++)
+		max_frame = max(max_frame, dev->ports[i].max_frame);
+
+	return regmap_update_bits(dev->regmap[1], REG_SW_MTU__2,
+				  REG_SW_MTU_MASK, max_frame);
+}
+
+static int ksz9477_max_mtu(struct dsa_switch *ds, int port)
+{
+	return KSZ9477_MAX_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN - VLAN_HLEN -
+		KSZ9477_INGRESS_TAG_LEN;
+}
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
 {
 	unsigned int val;
@@ -1412,8 +1440,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	/* Do not work correctly with tail tagging. */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_0, SW_CHECK_LENGTH, false);
 
-	/* accept packet up to 2000bytes */
-	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_LEGAL_PACKET_DISABLE, true);
+	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
+
+	/* Now we can configure default MTU value */
+	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
+				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
+	if (ret)
+		return ret;
 
 	ksz9477_config_cpu_port(ds);
 
@@ -1460,6 +1494,8 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.port_mirror_add	= ksz9477_port_mirror_add,
 	.port_mirror_del	= ksz9477_port_mirror_del,
 	.get_stats64		= ksz9477_get_stats64,
+	.port_change_mtu	= ksz9477_change_mtu,
+	.port_max_mtu		= ksz9477_max_mtu,
 };
 
 static u32 ksz9477_get_port_addr(int port, int offset)
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index 16939f29faa5..2278e763ee3e 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -176,6 +176,7 @@
 #define REG_SW_MAC_ADDR_5		0x0307
 
 #define REG_SW_MTU__2			0x0308
+#define REG_SW_MTU_MASK			GENMASK(13, 0)
 
 #define REG_SW_ISP_TPID__2		0x030A
 
@@ -1662,4 +1663,7 @@
 /* 148,800 frames * 67 ms / 100 */
 #define BROADCAST_STORM_VALUE		9969
 
+#define KSZ9477_INGRESS_TAG_LEN		2
+#define KSZ9477_MAX_FRAME_SIZE		9000
+
 #endif /* KSZ9477_REGS_H */
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c6fa487fb006..739365bfceb2 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -41,6 +41,7 @@ struct ksz_port {
 
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
+	u16 max_frame;
 };
 
 struct ksz_device {
-- 
2.30.2

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Wed, Feb 23, 2022 at 09:40:55AM +0100, Oleksij Rempel wrote:
> This chips supports two ways to configure max MTU size:
> - by setting SW_LEGAL_PACKET_DISABLE bit: if this bit is 0 allowed packed size
>   will be between 64 and bytes 1518. If this bit is 1, it will accept
>   packets up to 2000 bytes.
> - by setting SW_JUMBO_PACKET bit. If this bit is set, the chip will
>   ignore SW_LEGAL_PACKET_DISABLE value and use REG_SW_MTU__2 register to
>   configure MTU size.
> 
> Current driver has disabled SW_JUMBO_PACKET bit and activates
> SW_LEGAL_PACKET_DISABLE. So the switch will pass all packets up to 2000 without
> any way to configure it.
> 
> By providing port_change_mtu we are switch to SW_JUMBO_PACKET way and will
> be able to configure MTU up to ~9000.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v2:
> - rename max_mtu to max_frame and new_mtu to frame_size
> - use max() instead of if(>)
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 40 +++++++++++++++++++++++--
>  drivers/net/dsa/microchip/ksz9477_reg.h |  4 +++
>  drivers/net/dsa/microchip/ksz_common.h  |  1 +
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 18ffc8ded7ee..5c5f78cb970e 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -11,6 +11,7 @@
>  #include <linux/platform_data/microchip-ksz.h>
>  #include <linux/phy.h>
>  #include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
>  
> @@ -182,6 +183,33 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
>  			   bits, set ? bits : 0);
>  }
>  
> +static int ksz9477_change_mtu(struct dsa_switch *ds, int port, int mtu)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	u16 frame_size, max_frame = 0;
> +	int i;
> +
> +	frame_size = mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN;

Are you sure the unit of measurement is ok? My KSZ9477 documentation
says this about register 0x0308:

Maximum Frame Length (MTU)
Specifies the maximum transmission unit (MTU), which is the maximum
frame payload size. Frames which exceed this maximum are truncated. This
value can be set as high as 9000 (= 0x2328) if jumbo frame support is
required.

"frame payload" to me means what MTU should mean. And ETH_HLEN +
VLAN_HLEN + ETH_FCS_LEN isn't part of that meaning.

> +
> +	if (dsa_is_cpu_port(ds, port))
> +		frame_size += KSZ9477_INGRESS_TAG_LEN;
> +
> +	/* Cache the per-port MTU setting */
> +	dev->ports[port].max_frame = frame_size;
> +
> +	for (i = 0; i < dev->port_cnt; i++)
> +		max_frame = max(max_frame, dev->ports[i].max_frame);
> +
> +	return regmap_update_bits(dev->regmap[1], REG_SW_MTU__2,
> +				  REG_SW_MTU_MASK, max_frame);
> +}
> +
> +static int ksz9477_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return KSZ9477_MAX_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN - VLAN_HLEN -
> +		KSZ9477_INGRESS_TAG_LEN;
> +}
> +
>  static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
>  {
>  	unsigned int val;
> @@ -1412,8 +1440,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
>  	/* Do not work correctly with tail tagging. */
>  	ksz_cfg(dev, REG_SW_MAC_CTRL_0, SW_CHECK_LENGTH, false);
>  
> -	/* accept packet up to 2000bytes */
> -	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_LEGAL_PACKET_DISABLE, true);
> +	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
> +	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
> +
> +	/* Now we can configure default MTU value */
> +	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
> +				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);

Why do you need this? Doesn't DSA call dsa_slave_create() ->
dsa_slave_change_mtu(ETH_DATA_LEN) on probe?

> +	if (ret)
> +		return ret;
>  
>  	ksz9477_config_cpu_port(ds);
>  
> @@ -1460,6 +1494,8 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
>  	.port_mirror_add	= ksz9477_port_mirror_add,
>  	.port_mirror_del	= ksz9477_port_mirror_del,
>  	.get_stats64		= ksz9477_get_stats64,
> +	.port_change_mtu	= ksz9477_change_mtu,
> +	.port_max_mtu		= ksz9477_max_mtu,
>  };
>  
>  static u32 ksz9477_get_port_addr(int port, int offset)
> diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
> index 16939f29faa5..2278e763ee3e 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -176,6 +176,7 @@
>  #define REG_SW_MAC_ADDR_5		0x0307
>  
>  #define REG_SW_MTU__2			0x0308
> +#define REG_SW_MTU_MASK			GENMASK(13, 0)
>  
>  #define REG_SW_ISP_TPID__2		0x030A
>  
> @@ -1662,4 +1663,7 @@
>  /* 148,800 frames * 67 ms / 100 */
>  #define BROADCAST_STORM_VALUE		9969
>  
> +#define KSZ9477_INGRESS_TAG_LEN		2
> +#define KSZ9477_MAX_FRAME_SIZE		9000
> +
>  #endif /* KSZ9477_REGS_H */
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index c6fa487fb006..739365bfceb2 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -41,6 +41,7 @@ struct ksz_port {
>  
>  	struct ksz_port_mib mib;
>  	phy_interface_t interface;
> +	u16 max_frame;
>  };
>  
>  struct ksz_device {
> -- 
> 2.30.2
> 

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Thu, Feb 24, 2022 at 01:38:33AM +0200, Vladimir Oltean wrote:
> On Wed, Feb 23, 2022 at 09:40:55AM +0100, Oleksij Rempel wrote:
> > This chips supports two ways to configure max MTU size:
> > - by setting SW_LEGAL_PACKET_DISABLE bit: if this bit is 0 allowed packed size
> >   will be between 64 and bytes 1518. If this bit is 1, it will accept
> >   packets up to 2000 bytes.
> > - by setting SW_JUMBO_PACKET bit. If this bit is set, the chip will
> >   ignore SW_LEGAL_PACKET_DISABLE value and use REG_SW_MTU__2 register to
> >   configure MTU size.
> > 
> > Current driver has disabled SW_JUMBO_PACKET bit and activates
> > SW_LEGAL_PACKET_DISABLE. So the switch will pass all packets up to 2000 without
> > any way to configure it.
> > 
> > By providing port_change_mtu we are switch to SW_JUMBO_PACKET way and will
> > be able to configure MTU up to ~9000.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > changes v2:
> > - rename max_mtu to max_frame and new_mtu to frame_size
> > - use max() instead of if(>)
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c     | 40 +++++++++++++++++++++++--
> >  drivers/net/dsa/microchip/ksz9477_reg.h |  4 +++
> >  drivers/net/dsa/microchip/ksz_common.h  |  1 +
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index 18ffc8ded7ee..5c5f78cb970e 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/platform_data/microchip-ksz.h>
> >  #include <linux/phy.h>
> >  #include <linux/if_bridge.h>
> > +#include <linux/if_vlan.h>
> >  #include <net/dsa.h>
> >  #include <net/switchdev.h>
> >  
> > @@ -182,6 +183,33 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
> >  			   bits, set ? bits : 0);
> >  }
> >  
> > +static int ksz9477_change_mtu(struct dsa_switch *ds, int port, int mtu)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +	u16 frame_size, max_frame = 0;
> > +	int i;
> > +
> > +	frame_size = mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN;
> 
> Are you sure the unit of measurement is ok? My KSZ9477 documentation
> says this about register 0x0308:
> 
> Maximum Frame Length (MTU)
> Specifies the maximum transmission unit (MTU), which is the maximum
> frame payload size. Frames which exceed this maximum are truncated. This
> value can be set as high as 9000 (= 0x2328) if jumbo frame support is
> required.
> 
> "frame payload" to me means what MTU should mean. And ETH_HLEN +
> VLAN_HLEN + ETH_FCS_LEN isn't part of that meaning.

if I set this value to anything less then 1522, it breaks the NFS boot. Since
my NFS server is configured with MTU 1500, i tried to guess how
frame size is calculated on this chip.

> > +
> > +	if (dsa_is_cpu_port(ds, port))
> > +		frame_size += KSZ9477_INGRESS_TAG_LEN;
> > +
> > +	/* Cache the per-port MTU setting */
> > +	dev->ports[port].max_frame = frame_size;
> > +
> > +	for (i = 0; i < dev->port_cnt; i++)
> > +		max_frame = max(max_frame, dev->ports[i].max_frame);
> > +
> > +	return regmap_update_bits(dev->regmap[1], REG_SW_MTU__2,
> > +				  REG_SW_MTU_MASK, max_frame);
> > +}
> > +
> > +static int ksz9477_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > +	return KSZ9477_MAX_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN - VLAN_HLEN -
> > +		KSZ9477_INGRESS_TAG_LEN;
> > +}
> > +
> >  static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
> >  {
> >  	unsigned int val;
> > @@ -1412,8 +1440,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
> >  	/* Do not work correctly with tail tagging. */
> >  	ksz_cfg(dev, REG_SW_MAC_CTRL_0, SW_CHECK_LENGTH, false);
> >  
> > -	/* accept packet up to 2000bytes */
> > -	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_LEGAL_PACKET_DISABLE, true);
> > +	/* Enable REG_SW_MTU__2 reg by setting SW_JUMBO_PACKET */
> > +	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
> > +
> > +	/* Now we can configure default MTU value */
> > +	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
> > +				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
> 
> Why do you need this? Doesn't DSA call dsa_slave_create() ->
> dsa_slave_change_mtu(ETH_DATA_LEN) on probe?

This was my initial assumption as well, but cadence macb driver provides
buggy max MTU == -18. I hardcoded bigger MTU for now[1], but was not able to
find proper way to fix it. To avoid this kinds of regressions I decided
to keep some sane default configuration.

[1] - my workaround.
commit 5f8385e9641a383478a65f96ccee8fd992201f68
Author: Oleksij Rempel <linux@rempel-privat.de>
Date:   Mon Feb 14 14:41:06 2022 +0100

    WIP: net: macb: fix max mtu size
    
    The gem_readl(bp, JML) will return 0, so we get max_mtu size of -18,
    this is breaking MTU configuration for DSA
    
    Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a363da928e8b..454d811991bb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4727,7 +4727,7 @@ static int macb_probe(struct platform_device *pdev)
 	/* MTU range: 68 - 1500 or 10240 */
 	dev->min_mtu = GEM_MTU_MIN_SIZE;
 	if (bp->caps & MACB_CAPS_JUMBO)
-		dev->max_mtu = gem_readl(bp, JML) - ETH_HLEN - ETH_FCS_LEN;
+		dev->max_mtu = 10240 - ETH_HLEN - ETH_FCS_LEN;
 	else
 		dev->max_mtu = ETH_DATA_LEN;
 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 05:59:36AM +0100, Oleksij Rempel wrote:
> > Are you sure the unit of measurement is ok? My KSZ9477 documentation
> > says this about register 0x0308:
> > 
> > Maximum Frame Length (MTU)
> > Specifies the maximum transmission unit (MTU), which is the maximum
> > frame payload size. Frames which exceed this maximum are truncated. This
> > value can be set as high as 9000 (= 0x2328) if jumbo frame support is
> > required.
> > 
> > "frame payload" to me means what MTU should mean. And ETH_HLEN +
> > VLAN_HLEN + ETH_FCS_LEN isn't part of that meaning.
> 
> if I set this value to anything less then 1522, it breaks the NFS boot. Since
> my NFS server is configured with MTU 1500, i tried to guess how
> frame size is calculated on this chip.

Sad that Microchip engineers can't decide on whether the MTU register
holds the "Maximum Frame Length" or the "maximum frame payload size".
They said both to have themselves covered, you understand what you will,
of course both are not right :)

> > > +	/* Now we can configure default MTU value */
> > > +	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
> > > +				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
> > 
> > Why do you need this? Doesn't DSA call dsa_slave_create() ->
> > dsa_slave_change_mtu(ETH_DATA_LEN) on probe?
> 
> This was my initial assumption as well, but cadence macb driver provides
> buggy max MTU == -18. I hardcoded bigger MTU for now[1], but was not able to
> find proper way to fix it. To avoid this kinds of regressions I decided
> to keep some sane default configuration.
> 
> [1] - my workaround.
> commit 5f8385e9641a383478a65f96ccee8fd992201f68
> Author: Oleksij Rempel <linux@rempel-privat.de>
> Date:   Mon Feb 14 14:41:06 2022 +0100
> 
>     WIP: net: macb: fix max mtu size
>     
>     The gem_readl(bp, JML) will return 0, so we get max_mtu size of -18,
>     this is breaking MTU configuration for DSA
>     
>     Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a363da928e8b..454d811991bb 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4727,7 +4727,7 @@ static int macb_probe(struct platform_device *pdev)
>  	/* MTU range: 68 - 1500 or 10240 */
>  	dev->min_mtu = GEM_MTU_MIN_SIZE;
>  	if (bp->caps & MACB_CAPS_JUMBO)
> -		dev->max_mtu = gem_readl(bp, JML) - ETH_HLEN - ETH_FCS_LEN;
> +		dev->max_mtu = 10240 - ETH_HLEN - ETH_FCS_LEN;
>  	else
>  		dev->max_mtu = ETH_DATA_LEN;

Yes, but the macb driver can be a DSA master for any switch, not just
for ksz9477. Better to fix this differently.

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Thu, Feb 24, 2022 at 11:33:29AM +0200, Vladimir Oltean wrote:
> On Thu, Feb 24, 2022 at 05:59:36AM +0100, Oleksij Rempel wrote:
> > > Are you sure the unit of measurement is ok? My KSZ9477 documentation
> > > says this about register 0x0308:
> > > 
> > > Maximum Frame Length (MTU)
> > > Specifies the maximum transmission unit (MTU), which is the maximum
> > > frame payload size. Frames which exceed this maximum are truncated. This
> > > value can be set as high as 9000 (= 0x2328) if jumbo frame support is
> > > required.
> > > 
> > > "frame payload" to me means what MTU should mean. And ETH_HLEN +
> > > VLAN_HLEN + ETH_FCS_LEN isn't part of that meaning.
> > 
> > if I set this value to anything less then 1522, it breaks the NFS boot. Since
> > my NFS server is configured with MTU 1500, i tried to guess how
> > frame size is calculated on this chip.
> 
> Sad that Microchip engineers can't decide on whether the MTU register
> holds the "Maximum Frame Length" or the "maximum frame payload size".
> They said both to have themselves covered, you understand what you will,
> of course both are not right :)

¯\_(ツ)_/¯

> > > > +	/* Now we can configure default MTU value */
> > > > +	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
> > > > +				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
> > > 
> > > Why do you need this? Doesn't DSA call dsa_slave_create() ->
> > > dsa_slave_change_mtu(ETH_DATA_LEN) on probe?
> > 
> > This was my initial assumption as well, but cadence macb driver provides
> > buggy max MTU == -18. I hardcoded bigger MTU for now[1], but was not able to
> > find proper way to fix it. To avoid this kinds of regressions I decided
> > to keep some sane default configuration.
> > 
> > [1] - my workaround.
> > commit 5f8385e9641a383478a65f96ccee8fd992201f68
> > Author: Oleksij Rempel <linux@rempel-privat.de>
> > Date:   Mon Feb 14 14:41:06 2022 +0100
> > 
> >     WIP: net: macb: fix max mtu size
> >     
> >     The gem_readl(bp, JML) will return 0, so we get max_mtu size of -18,
> >     this is breaking MTU configuration for DSA
> >     
> >     Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index a363da928e8b..454d811991bb 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4727,7 +4727,7 @@ static int macb_probe(struct platform_device *pdev)
> >  	/* MTU range: 68 - 1500 or 10240 */
> >  	dev->min_mtu = GEM_MTU_MIN_SIZE;
> >  	if (bp->caps & MACB_CAPS_JUMBO)
> > -		dev->max_mtu = gem_readl(bp, JML) - ETH_HLEN - ETH_FCS_LEN;
> > +		dev->max_mtu = 10240 - ETH_HLEN - ETH_FCS_LEN;
> >  	else
> >  		dev->max_mtu = ETH_DATA_LEN;
> 
> Yes, but the macb driver can be a DSA master for any switch, not just
> for ksz9477. Better to fix this differently.

Yes, it should be fixed. I just need some time to understand the proper
way to do so. For now, let's proceed with the ksz patch. Should I send
new version with some changes?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 10:38:27AM +0100, Oleksij Rempel wrote:
> > > > > +	/* Now we can configure default MTU value */
> > > > > +	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
> > > > > +				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
> > > > 
> > > > Why do you need this? Doesn't DSA call dsa_slave_create() ->
> > > > dsa_slave_change_mtu(ETH_DATA_LEN) on probe?
> > > 
> > > This was my initial assumption as well, but cadence macb driver provides
> > > buggy max MTU == -18. I hardcoded bigger MTU for now[1], but was not able to
> > > find proper way to fix it. To avoid this kinds of regressions I decided
> > > to keep some sane default configuration.
> > > 
> > > [1] - my workaround.
> > > commit 5f8385e9641a383478a65f96ccee8fd992201f68
> > > Author: Oleksij Rempel <linux@rempel-privat.de>
> > > Date:   Mon Feb 14 14:41:06 2022 +0100
> > > 
> > >     WIP: net: macb: fix max mtu size
> > >     
> > >     The gem_readl(bp, JML) will return 0, so we get max_mtu size of -18,
> > >     this is breaking MTU configuration for DSA
> > >     
> > >     Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > 
> > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > > index a363da928e8b..454d811991bb 100644
> > > --- a/drivers/net/ethernet/cadence/macb_main.c
> > > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > > @@ -4727,7 +4727,7 @@ static int macb_probe(struct platform_device *pdev)
> > >  	/* MTU range: 68 - 1500 or 10240 */
> > >  	dev->min_mtu = GEM_MTU_MIN_SIZE;
> > >  	if (bp->caps & MACB_CAPS_JUMBO)
> > > -		dev->max_mtu = gem_readl(bp, JML) - ETH_HLEN - ETH_FCS_LEN;
> > > +		dev->max_mtu = 10240 - ETH_HLEN - ETH_FCS_LEN;
> > >  	else
> > >  		dev->max_mtu = ETH_DATA_LEN;
> > 
> > Yes, but the macb driver can be a DSA master for any switch, not just
> > for ksz9477. Better to fix this differently.
> 
> Yes, it should be fixed. I just need some time to understand the proper
> way to do so. For now, let's proceed with the ksz patch. Should I send
> new version with some changes?

So where is it failing exactly? Here I guess, because mtu_limit will be
negative?

	mtu_limit = min_t(int, master->max_mtu, dev->max_mtu);
	old_master_mtu = master->mtu;
	new_master_mtu = largest_mtu + dsa_tag_protocol_overhead(cpu_dp->tag_ops);
	if (new_master_mtu > mtu_limit)
		return -ERANGE;

I don't think we can work around it in DSA, it's garbage in, garbage out.

In principle, I don't have such a big issue with writing the MTU
register as part of the switch initialization, especially if it's global
and not per port. But tell me something else. You pre-program the MTU
with VLAN_ETH_FRAME_LEN + ETH_FCS_LEN, but in the MTU change procedure,
you also add KSZ9477_INGRESS_TAG_LEN (2) to that. Is that needed at all?
I expect that if it's needed, it's needed in both places. Can you
sustain an iperf3 tcp session over a VLAN upper of a ksz9477 port?
I suspect that the missing VLAN_HLEN is masking a lack of KSZ9477_INGRESS_TAG_LEN.

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Thu, Feb 24, 2022 at 11:46:57AM +0200, Vladimir Oltean wrote:
 
> So where is it failing exactly? Here I guess, because mtu_limit will be
> negative?
> 
> 	mtu_limit = min_t(int, master->max_mtu, dev->max_mtu);
> 	old_master_mtu = master->mtu;
> 	new_master_mtu = largest_mtu + dsa_tag_protocol_overhead(cpu_dp->tag_ops);
> 	if (new_master_mtu > mtu_limit)
> 		return -ERANGE;
> 
> I don't think we can work around it in DSA, it's garbage in, garbage out.
> 
> In principle, I don't have such a big issue with writing the MTU
> register as part of the switch initialization, especially if it's global
> and not per port. But tell me something else. You pre-program the MTU
> with VLAN_ETH_FRAME_LEN + ETH_FCS_LEN, but in the MTU change procedure,
> you also add KSZ9477_INGRESS_TAG_LEN (2) to that. Is that needed at all?
> I expect that if it's needed, it's needed in both places. Can you
> sustain an iperf3 tcp session over a VLAN upper of a ksz9477 port?
> I suspect that the missing VLAN_HLEN is masking a lack of KSZ9477_INGRESS_TAG_LEN.

Hm... I assume I need to do something like this:
- build kernel with BRIDGE_VLAN_FILTERING
- |
  ip l a name br0 type bridge vlan_filtering 1
  ip l s dev br0 up
  ip l s lan1 master br0
  ip l s dev lan1 up
  bridge vlan add dev lan1 vid 5 pvid untagged
  ip link add link br0 name vlan5 type vlan id 5

I use lan5@ksz for net boot. As soon as i link lan1@ksz to the br0 with
vlan_filtering enabled, the nfs on lan5 will be broken. Currently I have
no time to investigate it. I'll try to fix VLAN support in a separate
task. What will is acceptable way to proceed with MTU patch?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 12:47:40PM +0100, Oleksij Rempel wrote:
> On Thu, Feb 24, 2022 at 11:46:57AM +0200, Vladimir Oltean wrote:
>  
> > So where is it failing exactly? Here I guess, because mtu_limit will be
> > negative?
> > 
> > 	mtu_limit = min_t(int, master->max_mtu, dev->max_mtu);
> > 	old_master_mtu = master->mtu;
> > 	new_master_mtu = largest_mtu + dsa_tag_protocol_overhead(cpu_dp->tag_ops);
> > 	if (new_master_mtu > mtu_limit)
> > 		return -ERANGE;
> > 
> > I don't think we can work around it in DSA, it's garbage in, garbage out.
> > 
> > In principle, I don't have such a big issue with writing the MTU
> > register as part of the switch initialization, especially if it's global
> > and not per port. But tell me something else. You pre-program the MTU
> > with VLAN_ETH_FRAME_LEN + ETH_FCS_LEN, but in the MTU change procedure,
> > you also add KSZ9477_INGRESS_TAG_LEN (2) to that. Is that needed at all?
> > I expect that if it's needed, it's needed in both places. Can you
> > sustain an iperf3 tcp session over a VLAN upper of a ksz9477 port?
> > I suspect that the missing VLAN_HLEN is masking a lack of KSZ9477_INGRESS_TAG_LEN.
> 
> Hm... I assume I need to do something like this:
> - build kernel with BRIDGE_VLAN_FILTERING
> - |
>   ip l a name br0 type bridge vlan_filtering 1
>   ip l s dev br0 up
>   ip l s lan1 master br0
>   ip l s dev lan1 up
>   bridge vlan add dev lan1 vid 5 pvid untagged
>   ip link add link br0 name vlan5 type vlan id 5
> 
> I use lan5@ksz for net boot. As soon as i link lan1@ksz to the br0 with
> vlan_filtering enabled, the nfs on lan5 will be broken. Currently I have
> no time to investigate it. I'll try to fix VLAN support in a separate
> task. What will is acceptable way to proceed with MTU patch?

No bridge, why create a bridge? And even if you do, why add lan5 to it?
The expectation is that standalone ports still remain functional when
other ports join a bridge.

I was saying:

ip link set lan1 up
ip link add link lan1 name lan1.5 type vlan id 5
ip addr add 192.168.100.1/24 dev lan1.5 && ip link set lan1.5 up
iperf3 -c 192.168.100.2

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Fri, Feb 25, 2022 at 01:58:02PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 25, 2022 at 12:47:40PM +0100, Oleksij Rempel wrote:
> > On Thu, Feb 24, 2022 at 11:46:57AM +0200, Vladimir Oltean wrote:
> >  
> > > So where is it failing exactly? Here I guess, because mtu_limit will be
> > > negative?
> > > 
> > > 	mtu_limit = min_t(int, master->max_mtu, dev->max_mtu);
> > > 	old_master_mtu = master->mtu;
> > > 	new_master_mtu = largest_mtu + dsa_tag_protocol_overhead(cpu_dp->tag_ops);
> > > 	if (new_master_mtu > mtu_limit)
> > > 		return -ERANGE;
> > > 
> > > I don't think we can work around it in DSA, it's garbage in, garbage out.
> > > 
> > > In principle, I don't have such a big issue with writing the MTU
> > > register as part of the switch initialization, especially if it's global
> > > and not per port. But tell me something else. You pre-program the MTU
> > > with VLAN_ETH_FRAME_LEN + ETH_FCS_LEN, but in the MTU change procedure,
> > > you also add KSZ9477_INGRESS_TAG_LEN (2) to that. Is that needed at all?
> > > I expect that if it's needed, it's needed in both places. Can you
> > > sustain an iperf3 tcp session over a VLAN upper of a ksz9477 port?
> > > I suspect that the missing VLAN_HLEN is masking a lack of KSZ9477_INGRESS_TAG_LEN.
> > 
> > Hm... I assume I need to do something like this:
> > - build kernel with BRIDGE_VLAN_FILTERING
> > - |
> >   ip l a name br0 type bridge vlan_filtering 1
> >   ip l s dev br0 up
> >   ip l s lan1 master br0
> >   ip l s dev lan1 up
> >   bridge vlan add dev lan1 vid 5 pvid untagged
> >   ip link add link br0 name vlan5 type vlan id 5
> > 
> > I use lan5@ksz for net boot. As soon as i link lan1@ksz to the br0 with
> > vlan_filtering enabled, the nfs on lan5 will be broken. Currently I have
> > no time to investigate it. I'll try to fix VLAN support in a separate
> > task. What will is acceptable way to proceed with MTU patch?
> 
> No bridge, why create a bridge? And even if you do, why add lan5 to it?
> The expectation is that standalone ports still remain functional when
> other ports join a bridge.

No, lan5 is not added to the bridge, but stops functioning after creating
br with lan1 or any other lanX

> I was saying:
> 
> ip link set lan1 up
> ip link add link lan1 name lan1.5 type vlan id 5
> ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> iperf3 -c 172.17.0.10

It works.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 01:54:30PM +0100, Oleksij Rempel wrote:
> > No bridge, why create a bridge? And even if you do, why add lan5 to it?
> > The expectation is that standalone ports still remain functional when
> > other ports join a bridge.
> 
> No, lan5 is not added to the bridge, but stops functioning after creating
> br with lan1 or any other lanX

Please take time to investigate the problem and fix it.

> > I was saying:
> > 
> > ip link set lan1 up
> > ip link add link lan1 name lan1.5 type vlan id 5
> > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > iperf3 -c 172.17.0.10
> 
> It works.

This is akin to saying that without any calls to ksz9477_change_mtu(),
just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
port and the lan1 user port.

So my question is: is this necessary?

	if (dsa_is_cpu_port(ds, port))
		new_mtu += KSZ9477_INGRESS_TAG_LEN;

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Fri, Feb 25, 2022 at 06:35:43PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 25, 2022 at 01:54:30PM +0100, Oleksij Rempel wrote:
> > > No bridge, why create a bridge? And even if you do, why add lan5 to it?
> > > The expectation is that standalone ports still remain functional when
> > > other ports join a bridge.
> > 
> > No, lan5 is not added to the bridge, but stops functioning after creating
> > br with lan1 or any other lanX
> 
> Please take time to investigate the problem and fix it.

ack

> > > I was saying:
> > > 
> > > ip link set lan1 up
> > > ip link add link lan1 name lan1.5 type vlan id 5
> > > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > > iperf3 -c 172.17.0.10
> > 
> > It works.
> 
> This is akin to saying that without any calls to ksz9477_change_mtu(),
> just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
> sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
> port and the lan1 user port.
> 
> So my question is: is this necessary?
> 
> 	if (dsa_is_cpu_port(ds, port))
> 		new_mtu += KSZ9477_INGRESS_TAG_LEN;
> 

No.

I did some extra tests with following results: REG_SW_MTU__2 should be
configured to 1518 to pass 1514 frame. Independent if the frame is
passed between external ports or external to CPU port. So, I assume,
ETH_FRAME_LEN + ETH_FCS_LEN should be used instead of VLAN_ETH_FRAME_LEN
+ ETH_FCS_LEN. Correct?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Vladimir Oltean]

On Tue, Mar 08, 2022 at 11:06:44AM +0100, Oleksij Rempel wrote:
> > > > I was saying:
> > > > 
> > > > ip link set lan1 up
> > > > ip link add link lan1 name lan1.5 type vlan id 5
> > > > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > > > iperf3 -c 172.17.0.10
> > > 
> > > It works.
> > 
> > This is akin to saying that without any calls to ksz9477_change_mtu(),
> > just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
> > sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
> > port and the lan1 user port.
> > 
> > So my question is: is this necessary?
> > 
> > 	if (dsa_is_cpu_port(ds, port))
> > 		new_mtu += KSZ9477_INGRESS_TAG_LEN;
> > 
> 
> No.
> 
> I did some extra tests with following results: REG_SW_MTU__2 should be
> configured to 1518 to pass 1514 frame. Independent if the frame is
> passed between external ports or external to CPU port. So, I assume,
> ETH_FRAME_LEN + ETH_FCS_LEN should be used instead of VLAN_ETH_FRAME_LEN
> + ETH_FCS_LEN. Correct?

Oleksij, to be clear, I only had an issue with consistency.
You were adding KSZ9477_INGRESS_TAG_LEN during ksz9477_change_mtu() but
not during initial setup. That prompted the question: is that particular
member of the sum needed or not? Either it's needed in both places, or
in none.

Then, apart from removing KSZ9477_INGRESS_TAG_LEN, you've also made an
unsolicited change (subtracted VLAN_HLEN from the value programmed to
hardware) without a clear confirmation that you understand what this
does, and without explicitly saying that the iperf3 test above still
works with this formula applied.

Since the VLAN header is part of L2, it means that a port configured for
MTU 1500 must also support VLAN-tagged packets with an L2 payload of
1500 octets. 1500 + ETH_HLEN + VLAN_HLEN == 1518 octets.
And since you need to add ETH_HLEN + ETH_FCS_LEN, I have an unconfirmed
hunch that VLAN_HLEN is also needed for the case above.

So, I'm sorry for being paranoid, but you aren't really giving me a
choice but to ask again, and again.

Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration

[Oleksij Rempel]

On Tue, Mar 08, 2022 at 01:21:56PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 08, 2022 at 11:06:44AM +0100, Oleksij Rempel wrote:
> > > > > I was saying:
> > > > > 
> > > > > ip link set lan1 up
> > > > > ip link add link lan1 name lan1.5 type vlan id 5
> > > > > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > > > > iperf3 -c 172.17.0.10
> > > > 
> > > > It works.
> > > 
> > > This is akin to saying that without any calls to ksz9477_change_mtu(),
> > > just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
> > > sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
> > > port and the lan1 user port.
> > > 
> > > So my question is: is this necessary?
> > > 
> > > 	if (dsa_is_cpu_port(ds, port))
> > > 		new_mtu += KSZ9477_INGRESS_TAG_LEN;
> > > 
> > 
> > No.
> > 
> > I did some extra tests with following results: REG_SW_MTU__2 should be
> > configured to 1518 to pass 1514 frame. Independent if the frame is
> > passed between external ports or external to CPU port. So, I assume,
> > ETH_FRAME_LEN + ETH_FCS_LEN should be used instead of VLAN_ETH_FRAME_LEN
> > + ETH_FCS_LEN. Correct?
> 
> Oleksij, to be clear, I only had an issue with consistency.
> You were adding KSZ9477_INGRESS_TAG_LEN during ksz9477_change_mtu() but
> not during initial setup. That prompted the question: is that particular
> member of the sum needed or not? Either it's needed in both places, or
> in none.
> 
> Then, apart from removing KSZ9477_INGRESS_TAG_LEN, you've also made an
> unsolicited change (subtracted VLAN_HLEN from the value programmed to
> hardware) without a clear confirmation that you understand what this
> does, and without explicitly saying that the iperf3 test above still
> works with this formula applied.
> 
> Since the VLAN header is part of L2, it means that a port configured for
> MTU 1500 must also support VLAN-tagged packets with an L2 payload of
> 1500 octets. 1500 + ETH_HLEN + VLAN_HLEN == 1518 octets.

Ah.. now it solves my brain knot.

> And since you need to add ETH_HLEN + ETH_FCS_LEN, I have an unconfirmed
> hunch that VLAN_HLEN is also needed for the case above.

Yes, you are right.

> So, I'm sorry for being paranoid, but you aren't really giving me a
> choice but to ask again, and again.

Thank you for being paranoid. Sorry, i'll send new patch.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |