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

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

[Lukasz Majewski]

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;
 }
 
 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,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5565,6 +5566,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 11,
 		.num_internal_phys = 0,
 		.max_vid = 4095,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5586,6 +5588,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 8,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5610,6 +5613,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5633,6 +5637,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 8,
 		.num_internal_phys = 0,
 		.max_vid = 4095,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5655,6 +5660,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 11,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -5679,6 +5685,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5704,6 +5711,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 0,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5728,6 +5736,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5753,6 +5762,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 15,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5777,6 +5787,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5802,6 +5813,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 15,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5825,6 +5837,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 10,
 		.num_internal_phys = 0,
 		.max_vid = 4095,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5848,6 +5861,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5872,6 +5886,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5895,6 +5910,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5918,6 +5934,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5941,6 +5958,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5968,6 +5986,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 2,
 		.invalid_port_mask = BIT(2) | BIT(3) | BIT(4),
 		.max_vid = 4095,
+		.max_frame_size = 2048,
 		.port_base_addr = 0x08,
 		.phy_base_addr = 0x00,
 		.global1_addr = 0x0f,
@@ -5992,6 +6011,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 15,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6015,6 +6035,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.max_vid = 4095,
+		.max_frame_size = 2048,
 		.port_base_addr = 0x08,
 		.phy_base_addr = 0x00,
 		.global1_addr = 0x0f,
@@ -6038,6 +6059,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6062,6 +6084,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.num_gpio = 15,
 		.max_vid = 4095,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6087,6 +6110,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.num_gpio = 15,
 		.max_vid = 4095,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6112,6 +6136,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 11,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -6137,6 +6162,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6161,6 +6187,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6186,6 +6213,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 15,
 		.max_vid = 4095,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6211,6 +6239,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6236,6 +6265,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6260,6 +6290,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..31c09b66fbff 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.
+	 */
+	unsigned int max_frame_size;
 	unsigned int port_base_addr;
 	unsigned int phy_base_addr;
 	unsigned int global1_addr;
-- 
2.20.1

[PATCH v4 2/3] net: dsa: mv88e6xxx: add support for MV88E6020 switch

[Lukasz Majewski]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

A mv88e6250 family (i.e. LinkStreet) switch with 2 PHY and RMII ports
and no PTP support.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Add S-o-B
- Update commit message
- Add information about max packet size (2048 B)

Changes for v3:
- None

Changes for v4:
- Update the num_ports and num_internal_phys to be in sync with
  88e6020 documentation
---
 drivers/net/dsa/mv88e6xxx/chip.c | 21 +++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fc6d98c4a029..fb9b362c2a50 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5533,6 +5533,27 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 };
 
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
+	[MV88E6020] = {
+		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6020,
+		.family = MV88E6XXX_FAMILY_6250,
+		.name = "Marvell 88E6020",
+		.num_databases = 64,
+		.num_ports = 4,
+		.num_internal_phys = 2,
+		.max_vid = 4095,
+		.max_frame_size = 2048,
+		.port_base_addr = 0x8,
+		.phy_base_addr = 0x0,
+		.global1_addr = 0xf,
+		.global2_addr = 0x7,
+		.age_time_coeff = 15000,
+		.g1_irqs = 9,
+		.g2_irqs = 5,
+		.atu_move_port_mask = 0xf,
+		.dual_chip = true,
+		.ops = &mv88e6250_ops,
+	},
+
 	[MV88E6085] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6085,
 		.family = MV88E6XXX_FAMILY_6097,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 31c09b66fbff..dd3f777a7201 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -54,6 +54,7 @@ enum mv88e6xxx_frame_mode {
 
 /* List of supported models */
 enum mv88e6xxx_model {
+	MV88E6020,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index aec9d4fd20e3..169ce5b6fa31 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -111,6 +111,7 @@
 /* Offset 0x03: Switch Identifier Register */
 #define MV88E6XXX_PORT_SWITCH_ID		0x03
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_MASK	0xfff0
+#define MV88E6XXX_PORT_SWITCH_ID_PROD_6020	0x0200
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6085	0x04a0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6095	0x0950
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6097	0x0990
-- 
2.20.1

Re: [PATCH v4 2/3] net: dsa: mv88e6xxx: add support for MV88E6020 switch

[Andrew Lunn]

On Fri, Jan 06, 2023 at 11:16:50AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> A mv88e6250 family (i.e. LinkStreet) switch with 2 PHY and RMII ports
> and no PTP support.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 3/3] net: dsa: mv88e6xxx: add support for MV88E6071 switch

[Lukasz Majewski]

A mv88e6250 family (i.e. LinkStreet) switch with 5 internal PHYs,
2 RMIIs and no PTP support.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Update commit message
- Add information about max frame size

Changes for v3:
- None

Changes for v4:
- None
---
 drivers/net/dsa/mv88e6xxx/chip.c | 21 +++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fb9b362c2a50..dbb3b8d4ecaa 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5554,6 +5554,27 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.ops = &mv88e6250_ops,
 	},
 
+	[MV88E6071] = {
+		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6071,
+		.family = MV88E6XXX_FAMILY_6250,
+		.name = "Marvell 88E6071",
+		.num_databases = 64,
+		.num_ports = 7,
+		.num_internal_phys = 5,
+		.max_vid = 4095,
+		.max_frame_size = 2048,
+		.port_base_addr = 0x08,
+		.phy_base_addr = 0x00,
+		.global1_addr = 0x0f,
+		.global2_addr = 0x07,
+		.age_time_coeff = 15000,
+		.g1_irqs = 9,
+		.g2_irqs = 5,
+		.atu_move_port_mask = 0xf,
+		.dual_chip = true,
+		.ops = &mv88e6250_ops,
+	},
+
 	[MV88E6085] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6085,
 		.family = MV88E6XXX_FAMILY_6097,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index dd3f777a7201..eed59c595d11 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -55,6 +55,7 @@ enum mv88e6xxx_frame_mode {
 /* List of supported models */
 enum mv88e6xxx_model {
 	MV88E6020,
+	MV88E6071,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 169ce5b6fa31..494a221c9d9a 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -112,6 +112,7 @@
 #define MV88E6XXX_PORT_SWITCH_ID		0x03
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_MASK	0xfff0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6020	0x0200
+#define MV88E6XXX_PORT_SWITCH_ID_PROD_6071	0x0710
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6085	0x04a0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6095	0x0950
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6097	0x0990
-- 
2.20.1

Re: [PATCH v4 3/3] net: dsa: mv88e6xxx: add support for MV88E6071 switch

[Andrew Lunn]

On Fri, Jan 06, 2023 at 11:16:51AM +0100, Lukasz Majewski wrote:
> A mv88e6250 family (i.e. LinkStreet) switch with 5 internal PHYs,
> 2 RMIIs and no PTP support.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

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

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

FYI: It is normal to include a patch 0/X for a patchset, which
explains the big picture of the patchset. Please try to remember this
for your next patchset.

    Andrew

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

[Lukasz Majewski]

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

Hi Andrew,

> 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@denx.de>  
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> FYI: It is normal to include a patch 0/X for a patchset, which
> explains the big picture of the patchset. Please try to remember this
> for your next patchset.

Ok. I will. Thanks for the tip.

> 
>     Andrew
> 
> 




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 --]

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

[Lukasz Majewski]

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

Hi Andrew, 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@denx.de>  
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> FYI: It is normal to include a patch 0/X for a patchset, which
> explains the big picture of the patchset. Please try to remember this
> for your next patchset.
> 
>     Andrew
> 
> 

Are there any more comments, or is this patch set eligible for pulling
into net-next tree?


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 --]

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

[Vladimir Oltean]

On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> Are there any more comments, or is this patch set eligible for pulling
> into net-next tree?

How about responding to the comment that was already posted first?

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

[Lukasz Majewski]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> > Are there any more comments, or is this patch set eligible for
> > pulling into net-next tree?  
> 
> How about responding to the comment that was already posted first?

Could you be more specific?


On the beginning (first posted version) the patch included 9 patches
(which included work for ADDR4 for some mv88e6020 setup).

But after the discussion, I've decided to split this patch set to
smaller pieces;

First to add the set_max_frame size with basic definition for mv88e6020
and mv88e6071 and then follow with more complicated changes (for which
there is no agreement on how to tackle them).

For the 'set_max_frame' feature Alexander Dyuck had some comments
regarding defensive programming approach, but finally he agreed with
Andrew's approach.

As of now - the v4 has been Acked by Andrew, so it looks like at least
this "part" of the work is eligible for upstreaming.


Or there are any more issues about which I've forgotten ?

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 --]

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

[Vladimir Oltean]

On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote:
> Hi Vladimir,
> 
> > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:
> > > Are there any more comments, or is this patch set eligible for
> > > pulling into net-next tree?  
> > 
> > How about responding to the comment that was already posted first?
> 
> Could you be more specific?
> 
> 
> On the beginning (first posted version) the patch included 9 patches
> (which included work for ADDR4 for some mv88e6020 setup).
> 
> But after the discussion, I've decided to split this patch set to
> smaller pieces;
> 
> First to add the set_max_frame size with basic definition for mv88e6020
> and mv88e6071 and then follow with more complicated changes (for which
> there is no agreement on how to tackle them).
> 
> For the 'set_max_frame' feature Alexander Dyuck had some comments
> regarding defensive programming approach, but finally he agreed with
> Andrew's approach.
> 
> As of now - the v4 has been Acked by Andrew, so it looks like at least
> this "part" of the work is eligible for upstreaming.
> 
> 
> Or there are any more issues about which I've forgotten ?

Do you agree that for the chip families which neither implement
port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will
be returned, which currently produces warnings at probe time and should
be fixed first, prior to refactoring the code?
https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/#25149891

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

[Lukasz Majewski]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote:
> > Hi Vladimir,
> >   
> > > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote:  
> > > > Are there any more comments, or is this patch set eligible for
> > > > pulling into net-next tree?    
> > > 
> > > How about responding to the comment that was already posted
> > > first?  
> > 
> > Could you be more specific?
> > 
> > 
> > On the beginning (first posted version) the patch included 9 patches
> > (which included work for ADDR4 for some mv88e6020 setup).
> > 
> > But after the discussion, I've decided to split this patch set to
> > smaller pieces;
> > 
> > First to add the set_max_frame size with basic definition for
> > mv88e6020 and mv88e6071 and then follow with more complicated
> > changes (for which there is no agreement on how to tackle them).
> > 
> > For the 'set_max_frame' feature Alexander Dyuck had some comments
> > regarding defensive programming approach, but finally he agreed with
> > Andrew's approach.
> > 
> > As of now - the v4 has been Acked by Andrew, so it looks like at
> > least this "part" of the work is eligible for upstreaming.
> > 
> > 
> > Or there are any more issues about which I've forgotten ?  
> 
> Do you agree that for the chip families which neither implement
> port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will
> be returned, which currently produces warnings at probe time and
> should be fixed first, prior to refactoring the code?
> https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/#25149891

Sorry, but I've overlooked your reply.

I will write my comments as a reply to it.


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 --]

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

[Vladimir Oltean]

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

>  }
>  
>  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 ?!

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

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

[Lukasz Majewski]

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

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

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

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

[Vladimir Oltean]

On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote:
> I think that this commit [1], made the adjustment to fix yet another
> issue.
> [1] -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6

It appears that this is the commit to blame, indeed.

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

Only for DSA and CPU ports. The driver still behaves as if the max MTU
on user ports is 1492 bytes.

> > 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].
> [2] -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482

I think the handling for those switches were neither port_set_jumbo_size()
nor set_max_frame_size() is present is just a roundabout way of saying
"hey, I only support ETH_DATA_LEN MTU and can't change it, leave me alone".
But it isn't what the code does.

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

[Lukasz Majewski]

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

Hi Vladimir,

> On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote:
> > I think that this commit [1], made the adjustment to fix yet another
> > issue.
> > [1] -
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6
> >  
> 
> It appears that this is the commit to blame, indeed.
> 
> > It looks like the missing 8 bytes are added in the
> > mv88e6xxx_change_mtu() function.  
> 
> Only for DSA and CPU ports. The driver still behaves as if the max MTU
> on user ports is 1492 bytes.
> 

It looks so...

> > > 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].
> > [2] -
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482
> >  
> 
> I think the handling for those switches were neither
> port_set_jumbo_size() nor set_max_frame_size() is present is just a
> roundabout way of saying "hey, I only support ETH_DATA_LEN MTU and
> can't change it, leave me alone". But it isn't what the code does.

I tend to agree... The number of switched which suppor 1522 B max frame
is only six. This may be why the problem was not noticed.

The fixed function maybe should look like below:


static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
{
	....

	
	int max_mtu;

	max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
		  ETH_FCS_LE;

	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
		  max_mtu -= EDSA_HLEN;

	return max_mtu;
}

Comments more than welcome.



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 --]

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

[Andrew Lunn]

> I tend to agree... The number of switched which suppor 1522 B max frame
> is only six. This may be why the problem was not noticed.
> 
> The fixed function maybe should look like below:
> 
> 
> static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
> {
> 	....
> 
> 	
> 	int max_mtu;
> 
> 	max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
> 		  ETH_FCS_LE;
> 
> 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> 		  max_mtu -= EDSA_HLEN;
> 
> 	return max_mtu;
> }
> 
> Comments more than welcome.


I would suggest some comments are added, explaining what is going on
here. Given the number of Fixes: tags in this area, it is clearly
tricky to get right, given how different switches operate.

I've not looked back to the email archive, but i have a vague
recollection that it could be some switches don't impose the MTU limit
on CPU and DSA ports, just the user ports. So the value reported for
those ports can actually be bigger than then the max mtu, in order to
accommodate the DSA header.

	Andrew

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

[Vladimir Oltean]

On Fri, Jan 13, 2023 at 02:20:17PM +0100, Lukasz Majewski wrote:
> The fixed function maybe should look like below:
> 
> static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
> {
> 	....
> 	
> 	int max_mtu;
> 
> 	max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN -
> 		  ETH_FCS_LE;
> 
> 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> 		  max_mtu -= EDSA_HLEN;
> 
> 	return max_mtu;
> }
> 
> Comments more than welcome.

I suspect that looking at the DSA code which calls these methods will
answer a lot of your questions. ds->ops->port_max_mtu() is only called
for user ports. As for ds->ops->port_change_mtu(), this will always be
called with the requested L2 payload length (default 1500) on user ports,
and with the maximum among user ports for DSA and CPU ports.

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

[Russell King (Oracle)]

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

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

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

[Lukasz Majewski]

[-- 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 --]

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

[Lukasz Majewski]

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

Hi,

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

I would appreciate _any_ hints on how shall I proceed to prepare those
patches, so the community will accept them...

Thanks in advance.

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




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 --]

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

[Russell King (Oracle)]

On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote:
> Hi,
> 
> > 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.
> > 
> 
> I would appreciate _any_ hints on how shall I proceed to prepare those
> patches, so the community will accept them...

What I'm concerned about, and why I replied, is that setting the devices
to have a max frame size of 1522 when we program them to use a larger
frame size means we break those switches for normal sized packets.

The current logic in mv88e6xxx_get_max_mtu() is:

	If the chip implements port_set_jumbo_size, then packet sizes of
	up to 10240 are supported.
	(ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240, 6320,
	6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x)
	If the chip implements set_max_frame_size, then packet sizes of
	up to 1632 are supported.
	(ops: 6085, 6095, 6097, 6123, 6161, 6185)
	Otherwise, packets of up to 1522 are supported.

Now, going through the patch, I see:

	88e6085 has 10240 but currently has 1632
	88e6095 has 1632 (no change)
	88e6097 has 1632 (no change)
	88e6123 has 10240 but currently has 1632
	88e6131 has 10240 (no change)
	88e6141 has 10240 (no change)
	88e6161 has 1632 but currently has 10240
	88e6165 has 1632 but currently has 1522
	88e6171 has 1522 but currently has 10240
	88e6172 has 10240 (no change)
	88e6175 has 1632 but currently has 10240
	88e6176 has 10240 (no change)
	88e6185 has 1632 (no change)
	88e6190 has 10240 (no change)
	88e6190x has 10240 (no change)
	88e6191 has 10240 but currently has 1522
	88e6191x has 1522 but currently has 10240
	88e6193x has 1522 but currently has 10240
	88e6220 has 2048 but currently has 1522
	88e6240 has 10240 (no change)
	88e6250 has 2048 but currently has 1522
	88e6290 has 10240 but currently has 1522
	88e6320 has 10240 (no change)
	88e6321 has 10240 (no change)
	88e6341 has 10240 (no change)
	88e6350 has 10240 (no change)
	88e6351 has 10240 (no change)
	88e6352 has 10240 (no change)
	88e6390 has 1522 but currently has 10240
	88e6390x has 1522 but currently has 10240
	88e6393x has 1522 but currently has 10240

My point is that based on the above, there's an awful lot of changes
that this one patch brings, and I'm not sure many of them are intended.

All the ones with "but currently has 10240", it seems they implement
port_set_jumbo_size() which, although the switch may default to a
smaller frame size, we configure it to be higher. Maybe these don't
implement the field that configures those? Maybe your patch is wrong?
I don't know.

Similarly for the ones with "but currently has 1632", it seems they
implement set_max_frame_size(), but this is only called via
mv88e6xxx_change_mtu(), and I haven't worked out whether that will
be called during initialisation by the networking layer.

Now, what really concerns me is the difficulty in making this change.
As we can see from the above, there's a lot of changes going on here,
and it's not obvious which are intentional and which may be bugs.

So, I think it would be far better to introduce the "max_frame_size"
field using the existing values, and then verify that value during
initialisation time for every entry in mv88e6xxx_table[] using the
rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and
have it run that verification, and state that's what's happened and
was successful in the commit message.

In the next commit, change mv88e6xxx_get_max_mtu() to use those
verified values and remove the verification code.

Then in the following commit, update the "max_frame_size" values with
the changes you intend to make.

Then, we can (a) have confidence that each of the new members were
properly initialised, and (b) we can also see what changes you're
intentionally making.

Right now, given that at least two of the "max_frame_size" values are
wrong in this patch, I think we can say for certain that we've proven
that trying to introduce this new member and use it in a single patch
is too error prone.

Thanks.

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

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

[Lukasz Majewski]

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

Hi Russell,

> On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote:
> > Hi,
> >   
> > > 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.
> > >   
> > 
> > I would appreciate _any_ hints on how shall I proceed to prepare
> > those patches, so the community will accept them...  
> 

Thanks for a very detailed reply.

> What I'm concerned about, and why I replied, is that setting the
> devices to have a max frame size of 1522 when we program them to use
> a larger frame size means we break those switches for normal sized
> packets.
> 
> The current logic in mv88e6xxx_get_max_mtu() is:
> 
> 	If the chip implements port_set_jumbo_size, then packet sizes
> of up to 10240 are supported.
> 	(ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240,
> 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x)
> 	If the chip implements set_max_frame_size, then packet sizes
> of up to 1632 are supported.
> 	(ops: 6085, 6095, 6097, 6123, 6161, 6185)
> 	Otherwise, packets of up to 1522 are supported.
> 
> Now, going through the patch, I see:
> 
> 	88e6085 has 10240 but currently has 1632
> 	88e6095 has 1632 (no change)
> 	88e6097 has 1632 (no change)
> 	88e6123 has 10240 but currently has 1632
> 	88e6131 has 10240 (no change)
> 	88e6141 has 10240 (no change)
> 	88e6161 has 1632 but currently has 10240
> 	88e6165 has 1632 but currently has 1522
> 	88e6171 has 1522 but currently has 10240
> 	88e6172 has 10240 (no change)
> 	88e6175 has 1632 but currently has 10240
> 	88e6176 has 10240 (no change)
> 	88e6185 has 1632 (no change)
> 	88e6190 has 10240 (no change)
> 	88e6190x has 10240 (no change)
> 	88e6191 has 10240 but currently has 1522
> 	88e6191x has 1522 but currently has 10240
> 	88e6193x has 1522 but currently has 10240
> 	88e6220 has 2048 but currently has 1522
> 	88e6240 has 10240 (no change)
> 	88e6250 has 2048 but currently has 1522
> 	88e6290 has 10240 but currently has 1522
> 	88e6320 has 10240 (no change)
> 	88e6321 has 10240 (no change)
> 	88e6341 has 10240 (no change)
> 	88e6350 has 10240 (no change)
> 	88e6351 has 10240 (no change)
> 	88e6352 has 10240 (no change)
> 	88e6390 has 1522 but currently has 10240
> 	88e6390x has 1522 but currently has 10240
> 	88e6393x has 1522 but currently has 10240
> 
> My point is that based on the above, there's an awful lot of changes
> that this one patch brings, and I'm not sure many of them are
> intended.

As I only have access to mv88e60{20|71} SoCs I had to base on the code
to deduce which max frame is supported.

> 
> All the ones with "but currently has 10240", it seems they implement
> port_set_jumbo_size() which, although the switch may default to a
> smaller frame size, we configure it to be higher. Maybe these don't
> implement the field that configures those? Maybe your patch is wrong?
> I don't know.
> 
> Similarly for the ones with "but currently has 1632", it seems they
> implement set_max_frame_size(), but this is only called via
> mv88e6xxx_change_mtu(), and I haven't worked out whether that will
> be called during initialisation by the networking layer.
> 
> Now, what really concerns me is the difficulty in making this change.
> As we can see from the above, there's a lot of changes going on here,
> and it's not obvious which are intentional and which may be bugs.

I'm also quite surprised about the impact of this patch.

> 
> So, I think it would be far better to introduce the "max_frame_size"
> field using the existing values, and then verify that value during
> initialisation time for every entry in mv88e6xxx_table[] using the
> rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and
> have it run that verification, and state that's what's happened and
> was successful in the commit message.
> 
> In the next commit, change mv88e6xxx_get_max_mtu() to use those
> verified values and remove the verification code.
> 
> Then in the following commit, update the "max_frame_size" values with
> the changes you intend to make.
> 
> Then, we can (a) have confidence that each of the new members were
> properly initialised, and (b) we can also see what changes you're
> intentionally making.
> 

If I understood you correctly - the approach would be to "simulate" and
obtain each max_frame_size assigned in mv88e6xxx_get_max_mtu() to be
sure that we do preserve current (buggy or not) behaviour.

Then I shall just add those two extra values for mv88e60{20|71}.

> Right now, given that at least two of the "max_frame_size" values are
> wrong in this patch, I think we can say for certain that we've proven
> that trying to introduce this new member and use it in a single patch
> is too error prone.

I do agree here - the code for getting max frame size for each
supported SoC is quite convoluted and difficult to deduce the right
value from the outset.

> 
> Thanks.
> 




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 --]

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

[Russell King (Oracle)]

On Mon, Jan 30, 2023 at 12:57:31PM +0100, Lukasz Majewski wrote:
> Hi Russell,
> 
> > What I'm concerned about, and why I replied, is that setting the
> > devices to have a max frame size of 1522 when we program them to use
> > a larger frame size means we break those switches for normal sized
> > packets.
> > 
> > The current logic in mv88e6xxx_get_max_mtu() is:
> > 
> > 	If the chip implements port_set_jumbo_size, then packet sizes
> > of up to 10240 are supported.
> > 	(ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240,
> > 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x)
> > 	If the chip implements set_max_frame_size, then packet sizes
> > of up to 1632 are supported.
> > 	(ops: 6085, 6095, 6097, 6123, 6161, 6185)
> > 	Otherwise, packets of up to 1522 are supported.
> > 
> > Now, going through the patch, I see:
> > 
> > 	88e6085 has 10240 but currently has 1632
> > 	88e6095 has 1632 (no change)
> > 	88e6097 has 1632 (no change)
> > 	88e6123 has 10240 but currently has 1632
> > 	88e6131 has 10240 (no change)
> > 	88e6141 has 10240 (no change)
> > 	88e6161 has 1632 but currently has 10240
> > 	88e6165 has 1632 but currently has 1522
> > 	88e6171 has 1522 but currently has 10240
> > 	88e6172 has 10240 (no change)
> > 	88e6175 has 1632 but currently has 10240
> > 	88e6176 has 10240 (no change)
> > 	88e6185 has 1632 (no change)
> > 	88e6190 has 10240 (no change)
> > 	88e6190x has 10240 (no change)
> > 	88e6191 has 10240 but currently has 1522
> > 	88e6191x has 1522 but currently has 10240
> > 	88e6193x has 1522 but currently has 10240
> > 	88e6220 has 2048 but currently has 1522
> > 	88e6240 has 10240 (no change)
> > 	88e6250 has 2048 but currently has 1522
> > 	88e6290 has 10240 but currently has 1522
> > 	88e6320 has 10240 (no change)
> > 	88e6321 has 10240 (no change)
> > 	88e6341 has 10240 (no change)
> > 	88e6350 has 10240 (no change)
> > 	88e6351 has 10240 (no change)
> > 	88e6352 has 10240 (no change)
> > 	88e6390 has 1522 but currently has 10240
> > 	88e6390x has 1522 but currently has 10240
> > 	88e6393x has 1522 but currently has 10240
> > 
> > My point is that based on the above, there's an awful lot of changes
> > that this one patch brings, and I'm not sure many of them are
> > intended.
> 
> As I only have access to mv88e60{20|71} SoCs I had to base on the code
> to deduce which max frame is supported.

The above list of differences are also derived from the code, and this
rather proves my point that deriving these from the code is hard, and
we need a way to programmatically verify that we get them correct.

> > So, I think it would be far better to introduce the "max_frame_size"
> > field using the existing values, and then verify that value during
> > initialisation time for every entry in mv88e6xxx_table[] using the
> > rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and
> > have it run that verification, and state that's what's happened and
> > was successful in the commit message.
> > 
> > In the next commit, change mv88e6xxx_get_max_mtu() to use those
> > verified values and remove the verification code.
> > 
> > Then in the following commit, update the "max_frame_size" values with
> > the changes you intend to make.
> > 
> > Then, we can (a) have confidence that each of the new members were
> > properly initialised, and (b) we can also see what changes you're
> > intentionally making.
> > 
> 
> If I understood you correctly - the approach would be to "simulate" and
> obtain each max_frame_size assigned in mv88e6xxx_get_max_mtu() to be
> sure that we do preserve current (buggy or not) behaviour.

What I'm suggesting is something like:

static void mv88e6xxx_validate_frame_size(void)
{
	int max;
	int i;

	for (i = 0; i < ARRAY_SIZE(mv88e6xxx_table); i++) {
		/* same logic as in mv88e6xxx_get_max_mtu() */
		if (mv88e6xxx_table[i].ops->port_set_jumbo_size)
			max = 10240;
		else if (mv88e6xxx_table[i].ops->set_max_frame_size)
			max = 1632;
		else
			max = 1522;

		if (mv88e6xxx_table[i].max_frame_size != max)
			pr_err("BUG: %s has differing max_frame_size: %d != %d\n",
			       mv88e6xxx_table[i].name, max,
			       mv88e6xxx_table[i].max_frame_size);
	}
}

called from the mv88e6xxx_probe() function. I don't see any need to
do much more than that to verify the table, and I don't see any need
to make it only execute once - it's not like the code will be around
for very long.

Provided this code gets run, we can then be sure that the
max_frame_size values initially added correspond with the values
the driver currently uses.

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