Date:   Thu, 9 Mar 2023 14:05:07 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Lukasz Majewski <lukma@denx.de>
Cc:     Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexander Duyck <alexander.duyck@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size
 variable after validation
Message-ID: <ZAnnk5MZc0w4VkDE@shell.armlinux.org.uk>
References: <20230309125421.3900962-1-lukma@denx.de>
 <20230309125421.3900962-7-lukma@denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309125421.3900962-7-lukma@denx.de>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Mar 09, 2023 at 01:54:20PM +0100, Lukasz Majewski wrote:
> Running of the mv88e6xxx_validate_frame_size() function provided following
> results:
> 
> [    1.585565] BUG: Marvell 88E6020 has differing max_frame_size: 1632 != 2048
> [    1.592540] BUG: Marvell 88E6071 has differing max_frame_size: 1632 != 2048
> 		^------ Correct -> mv88e6250 family max frame size = 2048B
> 
> [    1.599507] BUG: Marvell 88E6085 has differing max_frame_size: 1632 != 1522
> [    1.606476] BUG: Marvell 88E6165 has differing max_frame_size: 1522 != 1632
> [    1.613445] BUG: Marvell 88E6190X has differing max_frame_size: 10240 != 1522
> [    1.620590] BUG: Marvell 88E6191X has differing max_frame_size: 10240 != 1522
> [    1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240 != 1522
> 		^------ Needs to be fixed!!!
> 
> [    1.634871] BUG: Marvell 88E6220 has differing max_frame_size: 1632 != 2048
> [    1.641842] BUG: Marvell 88E6250 has differing max_frame_size: 1632 != 2048
> 		^------ Correct -> mv88e6250 family max frame size = 2048B

If I understand this correctly, in patch 4, you add a call to the 6250
family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
than 1518.

However, you're saying that 6250 has a frame size of 2048. That's fine,
but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading as a
definition. While the bit may increase the frame size, I think if we're
going to do this, then this definition ought to be renamed.

That said, I would like Andrew and Vladimir's thoughts on this too.

Finally, I would expect, if this series was done the way I suggested,
that patch 1 should set the max frame size according to how the
existing code works, which means patch 2, being the validation patch,
should be completely silent if patch 1 is correct - and that's the
entire point of validating. It's to make sure that patch 1 is
correct.

If it isn't correct, then patch 1 is wrong and should be updated.

Essentially, this patch should only exist if the values we are using
today are actually incorrect.

To put this another way, the conversion from our existing way of
determining the max mtu to using the .max_frame_size method should be
an entire no-op from the driver operation point of view. Then any
errors in those values should be fixed and explained in a separate
commit. Then the new support added.

At least that's how I see it. Andrew and Vladimir may disagree.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!