Date:   Fri, 10 Mar 2023 12:25:59 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Lukasz Majewski <lukma@denx.de>, Andrew Lunn <andrew@lunn.ch>,
        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 1/7] dsa: marvell: Provide per device information about
 max frame size
Message-ID: <ZAsh12DdwDfKUW8F@shell.armlinux.org.uk>
References: <20230309125421.3900962-1-lukma@denx.de>
 <20230309125421.3900962-2-lukma@denx.de>
 <20230310120235.2cjxauvqxyei45li@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230310120235.2cjxauvqxyei45li@skbuf>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Mar 10, 2023 at 02:02:35PM +0200, Vladimir Oltean wrote:
> By ops:
> 
> port_set_jumbo_size:
> static const struct mv88e6xxx_ops mv88e6131_ops = {
> static const struct mv88e6xxx_ops mv88e6141_ops = {
> static const struct mv88e6xxx_ops mv88e6171_ops = {
> static const struct mv88e6xxx_ops mv88e6172_ops = {
> static const struct mv88e6xxx_ops mv88e6175_ops = {
> static const struct mv88e6xxx_ops mv88e6176_ops = {
> static const struct mv88e6xxx_ops mv88e6190_ops = {
> static const struct mv88e6xxx_ops mv88e6190x_ops = {
> static const struct mv88e6xxx_ops mv88e6240_ops = {
> static const struct mv88e6xxx_ops mv88e6320_ops = {
> static const struct mv88e6xxx_ops mv88e6321_ops = {
> static const struct mv88e6xxx_ops mv88e6341_ops = {
> static const struct mv88e6xxx_ops mv88e6350_ops = {
> static const struct mv88e6xxx_ops mv88e6351_ops = {
> static const struct mv88e6xxx_ops mv88e6352_ops = {
> static const struct mv88e6xxx_ops mv88e6390_ops = {
> static const struct mv88e6xxx_ops mv88e6390x_ops = {
> static const struct mv88e6xxx_ops mv88e6393x_ops = {
> 
> set_max_frame_size:
> static const struct mv88e6xxx_ops mv88e6085_ops = {
> static const struct mv88e6xxx_ops mv88e6095_ops = {
> static const struct mv88e6xxx_ops mv88e6097_ops = {
> static const struct mv88e6xxx_ops mv88e6123_ops = {
> static const struct mv88e6xxx_ops mv88e6161_ops = {
> static const struct mv88e6xxx_ops mv88e6185_ops = {
> 
> none of the above:
> static const struct mv88e6xxx_ops mv88e6165_ops = {
> static const struct mv88e6xxx_ops mv88e6191_ops = {
> static const struct mv88e6xxx_ops mv88e6250_ops = {
> static const struct mv88e6xxx_ops mv88e6290_ops = {
> 
> 
> By info:
> 
> port_set_jumbo_size (10240):
> 	[MV88E6131] = {
> 	[MV88E6141] = {
> 	[MV88E6171] = {
> 	[MV88E6172] = {
> 	[MV88E6175] = {
> 	[MV88E6176] = {
> 	[MV88E6190] = {
> 	[MV88E6190X] = {
> 	[MV88E6240] = {
> 	[MV88E6320] = {
> 	[MV88E6321] = {
> 	[MV88E6341] = {
> 	[MV88E6350] = {
> 	[MV88E6351] = {
> 	[MV88E6352] = {
> 	[MV88E6390] = {
> 	[MV88E6390X] = {
> 	[MV88E6191X] = {
> 	[MV88E6193X] = {
> 	[MV88E6393X] = {
> 
> set_max_frame_size (1632):
> 	[MV88E6085] = {
> 	[MV88E6095] = {
> 	[MV88E6097] = {
> 	[MV88E6123] = {
> 	[MV88E6161] = {
> 	[MV88E6185] = {
> 
> none of the above (1522):
> 	[MV88E6165] = {
> 	[MV88E6191] = {
> 	[MV88E6220] = {
> 	[MV88E6250] = {
> 	[MV88E6290] = {
> 
> 
> Whereas your analysis seems to have determined this:
> 
> port_set_jumbo_size (10240):
> 	[MV88E6131] = {
> 	[MV88E6141] = {
> 	[MV88E6171] = {
> 	[MV88E6172] = {
> 	[MV88E6175] = {
> 	[MV88E6176] = {
> 	[MV88E6190] = {
> 	[MV88E6240] = {
> 	[MV88E6320] = {
> 	[MV88E6321] = {
> 	[MV88E6341] = {
> 	[MV88E6350] = {
> 	[MV88E6351] = {
> 	[MV88E6352] = {
> 	[MV88E6390] = {
> 	[MV88E6390X] = {
> 	[MV88E6393X] = {
> 
> set_max_frame_size (1632):
> 	[MV88E6095] = {
> 	[MV88E6097] = {
> 	[MV88E6123] = {
> 	[MV88E6161] = {
> 	[MV88E6165] = {
> 	[MV88E6185] = {
> 
> none of the above (1522):
> 	[MV88E6085] = {
> 	[MV88E6190X] = {
> 	[MV88E6191] = {
> 	[MV88E6191X] = {
> 	[MV88E6193X] = {
> 	[MV88E6290] = {
> 
> what's up with these?! (no max_frame_size)
> 	[MV88E6220] = {
> 	[MV88E6250] = {
> 
> 
> So our analysis differs for:
> 
> MV88E6190X (I say 10240, you say 1522)
> MV88E6191X (I say 10240, you say 1522)
> MV88E6193X (I say 10240, you say 1522)
> MV88E6085 (I say 1632, you say 1522)
> MV88E6165 (I say 1522, you say 1632)
> MV88E6220 (I say 1522, not clear what you say)
> MV88E6250 (I say 1522, not clear what you say)
> 
> Double-checking with the code, I believe my analysis to be the correct one...

This is similar analysis to what I did for a previous patch set, and
came to the conclusion that we need code in the driver to validate
that the addition of these values is in fact correct. See my previous
reviews and my recommendations on how to structure these patch sets,
so we as reviewers don't _have_ to go to this level of verification.

> I have also noticed that you have not acted upon my previous review comment:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/
> 
> | 1522 - 30 = 1492.
> | 
> | I don't believe that there are switches which don't support the standard
> | MTU of 1500 ?!
> | 
> | >  		.port_base_addr = 0x10,
> | >  		.phy_base_addr = 0x0,
> | >  		.global1_addr = 0x1b,
> | 
> | Note that I see this behavior isn't new. But I've simulated it, and it
> | will produce the following messages on probe:
> | 
> | [    7.425752] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
> | [    7.437516] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 0
> | [    7.588585] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
> | [    7.600433] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 1
> | [    7.752613] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
> | [    7.764457] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 2
> | [    7.900771] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
> | [    7.912501] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 3
> | 
> | I wonder, shouldn't we first fix that, and apply this patch set afterwards?
> 
> I guess I will have to fix this now, since you haven't done it.

I'm sorry, but why is this Lukasz's problem to fix? If it's broken today
when using mv88e6xxx with this PHY, and Lukasz doesn't have this PHY,
why does Lukasz have to solve this?

> > +
> > +	/* Max Frame Size.
> > +	 * This value corresponds to the memory allocated in switch internal
> > +	 * memory to store single frame.
> > +	 */
> 
> What is the source of this definition?
> 
> I'm asking because I know of other switches where the internal memory
> allocation scheme has nothing to do with the frame size. Instead, there
> are SRAM cells of fixed and small size (say 60 octets) chained together.

The switch documentation only really talks about maximum frame sizes
that the switch can handle, with a few bits that configure what the
maximum frame size is. We also know how large the SRAM is, but how
the SRAM is allocated to packets is for Marvell engineers to know
and not us mere mortals.

So, the base definition for this is the information provided in the
switch documentation.

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