Message ID | 20220223084055.2719969-1-o.rempel@pengutronix.de (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net-next,v2,1/1] net: dsa: microchip: ksz9477: implement MTU configuration | expand |
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 >
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;
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.
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
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.
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
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
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
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;
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
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.
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
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 {
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(-)