Message-ID: <20230310141719.7f691b45@wsk>
Date:   Fri, 10 Mar 2023 14:17:19 +0100
From:   Lukasz Majewski <lukma@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] dsa: marvell: Provide per device information about
 max frame size

Hi Vladimir,

> On Thu, Mar 09, 2023 at 01:54:15PM +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.  
> 
> What is the criterion based on which 1632 is the "in-driver standard
> value"?

It comes from the documentation I suppose. Moreover, this might be the
the "first" used value when set_max_mtu callback was introduced.

> 
> > On the other hand - mv88e6250 supports 2048 bytes.  
> 
> What you mean to suggest here is that, using the current
> classification from mv88e6xxx_get_max_mtu(), mv88e6250 falls into the
> "none of the above" bucket, for which the driver returns 1522 -
> VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN // 1492. But it truly
> supports a maximum frame length of 2048, per your research.
> 

And this cannot be easily fixed.

I could just provide callback to .set_max_frame_size in mv88e6250_ops
and the mv88e6250 would use 1632 bytes which would prevent errors.

However, when I dig deeper - it turned out that this value is larger.
And hence I wanted to do it in "right way".

> The problem is that I needed to spend 30 minutes to understand this,
> and the true motivation for this patch.
> 

The motivation is correctly stated in the commit message. There is a
bug - mv88e6250 and friends use 2048 max frame size value.

> > To be more interesting - devices supporting jumbo frames - use yet
> > another value (10240 bytes)  
> 
> What's interesting about this?
> 
> > 
> > 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.  
> 
> You need to provide a justification for why the existing code
> structure is not good enough.

It is clearly stated in patch 3/7 where the existing scheme is replaced
with this one.

> 
> > 
> > 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 switch driver
> > (e.g. .set_max_frame_size, .port_set_jumbo_size).
> > 
> > Signed-off-by: Lukasz Majewski <lukma@...x.de>
> > 
> > ---
> > Changes for v2:
> > - Define max_frame_size with default value of 1632 bytes,
> > - Set proper value for the mv88e6250 switch SoC (linkstreet) family
> > 
> > Changes for v3:
> > - Add default value for 1632B of the max frame size (to avoid
> > problems with not defined values)
> > 
> > Changes for v4:
> > - Rework the mv88e6xxx_get_max_mtu() by using per device defined
> >   max_frame_size value
> > 
> > - Add WARN_ON_ONCE() when max_frame_size is not defined
> > 
> > - Add description for the new 'max_frame_size' member of
> > mv88e6xxx_info
> > 
> > Changes for v5:
> > - Move some code fragments (like get_mtu callback changes) to
> > separate patches  
> 
> you have change log up to v5, but your subject prefix is [PATCH 1/7]
> which implies v1?

My mistake - I've forgotten to add proper subject-prefix parameter.

> 
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 31
> > +++++++++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/chip.h |
> > 6 ++++++ 2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c index 0a5d6c7bb128..c097a0b19ba6
> > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c  
> 
> It would be good if the commit message contained the procedure based
> on which you had made these changes - and preferably they were
> mechanical. Having a small C program written would be absolutely
> ideal. This is so that reviewers wouldn't have to do it in parallel...
> 
> My analysis has determined the following 3 categories:
> 
> static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
> {
> 	struct mv88e6xxx_chip *chip = ds->priv;
> 
> 	if (chip->info->ops->port_set_jumbo_size)
> 		return 10240 - VLAN_ETH_HLEN - EDSA_HLEN -
> ETH_FCS_LEN; // 10210 else if (chip->info->ops->set_max_frame_size)
> 		return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> ETH_FCS_LEN; // 1602 return 1522 - VLAN_ETH_HLEN - EDSA_HLEN -
> ETH_FCS_LEN; // 1492 }
> 
> 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 explained and provided in the following commits as advised by
Russel.

> 
> 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 do agree with Russel's reply here.

Moreover, as 6250 and 6220 also have max frame size equal to 2048
bytes, this would be fixed in v6 after getting the "validation"
function run.

This is the "patch 4" in the comment sent by Russel (to fix stuff which
is already broken, but it has been visible after running the validation
code):

https://lists.openwall.net/netdev/2023/03/09/233

> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h
> > b/drivers/net/dsa/mv88e6xxx/chip.h index da6e1339f809..e2b88f1f8376
> > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> > @@ -132,6 +132,12 @@ struct mv88e6xxx_info {
> >  	unsigned int num_gpio;
> >  	unsigned int max_vid;
> >  	unsigned int max_sid;
> > +
> > +	/* 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.
> 
> > +	unsigned int max_frame_size;
> >  	unsigned int port_base_addr;
> >  	unsigned int phy_base_addr;
> >  	unsigned int global1_addr;
> > -- 
> > 2.20.1
> >   




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@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists