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

From: Vladimir Oltean <olteanv@gmail.com>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Woojung Huh <woojung.huh@microchip.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Vivien Didelot <vivien.didelot@gmail.com>,
	kernel@pengutronix.de, Jakub Kicinski <kuba@kernel.org>,
	UNGLinuxDriver@microchip.com,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement MTU configuration
Date: Thu, 24 Feb 2022 11:33:29 +0200	[thread overview]
Message-ID: <20220224093329.hssghouq7hmgxvwb@skbuf> (raw)
In-Reply-To: <20220224045936.GB4594@pengutronix.de>

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.