Messages in this thread | | | Date | Thu, 24 Feb 2022 11:46:57 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration |
| |
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.
| |