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

Hi Vladimir,

> 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).
> > 
> > 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 ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 41
> > ++++++++++++++++++++++++++++---- drivers/net/dsa/mv88e6xxx/chip.h |
> >  6 +++++ 2 files changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c index 242b8b325504..fc6d98c4a029
> > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -3545,11 +3545,10 @@ 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;
> > -	else if (chip->info->ops->set_max_frame_size)
> > -		return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > ETH_FCS_LEN;
> > -	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > +	WARN_ON_ONCE(!chip->info->max_frame_size);
> > +
> > +	return chip->info->max_frame_size - VLAN_ETH_HLEN -
> > EDSA_HLEN
> > +		- ETH_FCS_LEN;  
> 
> VLAN_ETH_HLEN (18) + EDSA_HLEN (8) + ETH_FCS_LEN (4) = 30
> 
> >  }
> >  
> >  static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port,
> > int new_mtu) @@ -4955,6 +4954,7 @@ static const struct
> > mv88e6xxx_ops mv88e6250_ops = { .avb_ops = &mv88e6352_avb_ops,
> >  	.ptp_ops = &mv88e6250_ptp_ops,
> >  	.phylink_get_caps = mv88e6250_phylink_get_caps,
> > +	.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
> >  };
> >  
> >  static const struct mv88e6xxx_ops mv88e6290_ops = {
> > @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info
> > mv88e6xxx_table[] = { .num_internal_phys = 5,
> >  		.max_vid = 4095,
> >  		.max_sid = 63,
> > +		.max_frame_size = 1522,  
> 
> 1522 - 30 = 1492.
> 
> I don't believe that there are switches which don't support the
> standard MTU of 1500 ?!

I think that this commit [1], made the adjustment to fix yet another
issue.

It looks like the missing 8 bytes are added in the
mv88e6xxx_change_mtu() function.

> 
> >  		.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?

IMHO, it is up to Andrew to decide how to proceed, as the
aforementioned patch [1] is an attempt to fix yet another issue [2].



Links:

[1] -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6

[2] -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482

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