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:

quoted

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@denx.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

quoted

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

quoted

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