Re: [PATCH v4 1/3] dsa: marvell: Provide per device information about max frame size

From: Lukasz Majewski <lukma@denx.de>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>,
	Andrew Lunn <andrew@lunn.ch>
Cc: 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 v4 1/3] dsa: marvell: Provide per device information about max frame size
Date: Mon, 16 Jan 2023 10:51:48 +0100	[thread overview]
Message-ID: <20230116105148.230ef4ae@wsk> (raw)
In-Reply-To: <Y8Fno+svcnNY4h/8@shell.armlinux.org.uk>

[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]

Hi Russell,

> On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote:
> > Different Marvell DSA switches support different size of max frame
> > bytes to be sent. This value corresponds to the memory allocated
> > in switch to store single frame.
> > 
> > For example mv88e6185 supports max 1632 bytes, which is now
> > in-driver standard value. On the other hand - mv88e6250 supports
> > 2048 bytes. To be more interresting - devices supporting jumbo
> > frames - use yet another value (10240 bytes)
> > 
> > As this value is internal and may be different for each switch IC,
> > new entry in struct mv88e6xxx_info has been added to store it.
> > 
> > This commit doesn't change the code functionality - it just provides
> > the max frame size value explicitly - up till now it has been
> > assigned depending on the callback provided by the IC driver
> > (e.g. .set_max_frame_size, .port_set_jumbo_size).  
> 
> I don't think this patch is correct.
> 
> One of the things that mv88e6xxx_setup_port() does when initialising
> each port is:
> 
>         if (chip->info->ops->port_set_jumbo_size) {
>                 err = chip->info->ops->port_set_jumbo_size(chip,
> port, 10218); if (err)
>                         return err;
>         }
> 
> There is one implementation of this, which is
> mv88e6165_port_set_jumbo_size() and that has the effect of setting
> port register 8 to the largest size. So any chip that supports the
> port_set_jumbo_size() method will be programmed on initialisation to
> support this larger size.
> 
> However, you seem to be listing e.g. the 88e6190 (if I'm interpreting
> the horrid mv88e6xxx_table changes correctly)

Those changes were requested by the community. Previous versions of
this patch were just changing things to allow correct operation of the
switch ICs on which I do work (i.e. 88e6020 and 88e6071).

And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the
same value was not updated for 88e6190X.

The question is - how shall I proceed? 

After the discussion about this code - it looks like approach from v3
[1] seems to be the most non-intrusive for other ICs.

> as having a maximum
> frame size of 1522, but it implements this method, supports 10240, and
> thus is programmed to support frames of that size rather than 1522.
> 

Links:

[1] - https://lore.kernel.org/netdev/Y7M+mWMU+DJPYubp@lunn.ch/T/

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]