[PATCH 0/7] dsa: marvell: Add support for mv88e6071 and 6020 switches

[PATCH 0/7] dsa: marvell: Add support for mv88e6071 and 6020 switches

[Lukasz Majewski]

This patch set provides following changes:

- Provide support for mv88e6020 and mv88e6071 switch circuits (the
  "Link Street" family of products including added earlier to this
  driver mv88e6250 and mv88e6220).

- Add the max_frame size variable to specify the buffer size for the
  maximal frame size

- The above change required adjusting all supported devices in the
  mv88e6xxx driver, as the current value assignment is depending
  on the set of provided callbacks for each switch circuit - i.e.
  until now the value was not explicitly specified.

- As the driver for Marvell's mv88e6xxx switches was rather complicated
  the intermediate function (removed by the end of this patch set)
  has been introduced. It was supposed to both validate the provided
  values deduced from the code and leave a trace of the exact
  methodology used.

Lukasz Majewski (6):
  dsa: marvell: Provide per device information about max frame size
  net: dsa: mv88e6xxx: add support for MV88E6071 switch
  dsa: marvell: Define .set_max_frame_size() function for mv88e6250 SoC
    family
  dsa: marvell: Add helper function to validate the max_frame_size
    variable
  dsa: marvell: Correct value of max_frame_size variable after
    validation
  dsa: marvell: Modify get max MTU callback to use per switch provided
    value

Matthias Schiffer (1):
  net: dsa: mv88e6xxx: add support for MV88E6020 switch

 drivers/net/dsa/mv88e6xxx/chip.c | 83 ++++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/chip.h |  8 +++
 drivers/net/dsa/mv88e6xxx/port.h |  2 +
 3 files changed, 88 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/7] 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 interesting - 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 switch 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

Changes for v5:
- Move some code fragments (like get_mtu callback changes) to separate
  patches
---
 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
@@ -5626,6 +5626,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,
@@ -5648,6 +5649,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,
@@ -5669,6 +5671,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,
@@ -5693,6 +5696,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,
@@ -5716,6 +5720,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,
@@ -5738,6 +5743,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,
@@ -5762,6 +5768,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,
@@ -5787,6 +5794,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,
@@ -5811,6 +5819,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,
@@ -5836,6 +5845,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,
@@ -5860,6 +5870,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,
@@ -5885,6 +5896,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,
@@ -5908,6 +5920,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,
@@ -5931,6 +5944,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,
@@ -5955,6 +5969,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,
@@ -5978,6 +5993,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,
@@ -6001,6 +6017,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,
@@ -6024,6 +6041,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,
@@ -6051,6 +6069,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,
@@ -6075,6 +6094,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,
@@ -6098,6 +6118,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,
@@ -6121,6 +6142,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,
@@ -6145,6 +6167,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,
@@ -6170,6 +6193,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,
@@ -6195,6 +6219,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,
@@ -6220,6 +6245,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,
@@ -6244,6 +6270,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,
@@ -6269,6 +6296,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,
@@ -6294,6 +6322,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,
@@ -6319,6 +6348,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,
@@ -6343,6 +6373,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 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.
+	 */
+	unsigned int max_frame_size;
 	unsigned int port_base_addr;
 	unsigned int phy_base_addr;
 	unsigned int global1_addr;
-- 
2.20.1

[PATCH 2/7] 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

Changes for v5:
- 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 c097a0b19ba6..721bae2e579c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5616,6 +5616,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 e2b88f1f8376..1690b1a0f2e7 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

[PATCH 3/7] 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

Changes for v5:
- 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 721bae2e579c..26ab4d676615 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5637,6 +5637,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 1690b1a0f2e7..af42530da71e 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

[PATCH 4/7] dsa: marvell: Define .set_max_frame_size() function for mv88e6250 SoC family

[Lukasz Majewski]

Switches from mv88e6250 family (the marketing name - "Link Street",
including mv88e6020 and mv88e6071) need the possibility to setup the
maximal frame size, as they support frames up to 2048 bytes.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v5:
- New patch
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 26ab4d676615..9695a1af45a9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5018,6 +5018,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 = {
-- 
2.20.1

[PATCH 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Lukasz Majewski]

This commit shall be regarded as a transition one, as this function helps
to validate the correctness of max_frame_size variable added to
mv88e6xxx_info structure.

It is necessary to avoid regressions as manual assessment of this value
turned out to be error prone.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
---
Changes for v5:
- New patch
---
 drivers/net/dsa/mv88e6xxx/chip.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9695a1af45a9..af14eb8a1bfd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -7169,6 +7169,27 @@ static int __maybe_unused mv88e6xxx_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(mv88e6xxx_pm_ops, mv88e6xxx_suspend, mv88e6xxx_resume);
 
+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);
+	}
+}
+
 static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;
@@ -7302,6 +7323,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		goto out_mdio;
 
+	mv88e6xxx_validate_frame_size();
 	return 0;
 
 out_mdio:
-- 
2.20.1

[PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

Running of the mv88e6xxx_validate_frame_size() function provided following
results:

[    1.585565] BUG: Marvell 88E6020 has differing max_frame_size: 1632 != 2048
[    1.592540] BUG: Marvell 88E6071 has differing max_frame_size: 1632 != 2048
		^------ Correct -> mv88e6250 family max frame size = 2048B

[    1.599507] BUG: Marvell 88E6085 has differing max_frame_size: 1632 != 1522
[    1.606476] BUG: Marvell 88E6165 has differing max_frame_size: 1522 != 1632
[    1.613445] BUG: Marvell 88E6190X has differing max_frame_size: 10240 != 1522
[    1.620590] BUG: Marvell 88E6191X has differing max_frame_size: 10240 != 1522
[    1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240 != 1522
		^------ Needs to be fixed!!!

[    1.634871] BUG: Marvell 88E6220 has differing max_frame_size: 1632 != 2048
[    1.641842] BUG: Marvell 88E6250 has differing max_frame_size: 1632 != 2048
		^------ Correct -> mv88e6250 family max frame size = 2048B

This commit removes the validation function and provides correct values
for the max frame size field.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v5:
- New patch
---
 drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++---------------------------
 1 file changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index af14eb8a1bfd..dbb69787f4ef 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5669,7 +5669,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 5,
 		.max_vid = 4095,
 		.max_sid = 63,
-		.max_frame_size = 1522,
+		.max_frame_size = 1632,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -5837,7 +5837,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 0,
 		.max_vid = 4095,
 		.max_sid = 63,
-		.max_frame_size = 1632,
+		.max_frame_size = 1522,
 		.port_base_addr = 0x10,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6012,7 +6012,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_gpio = 16,
 		.max_vid = 8191,
 		.max_sid = 63,
-		.max_frame_size = 1522,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6060,7 +6060,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
-		.max_frame_size = 1522,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -6084,7 +6084,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_internal_phys = 9,
 		.max_vid = 8191,
 		.max_sid = 63,
-		.max_frame_size = 1522,
+		.max_frame_size = 10240,
 		.port_base_addr = 0x0,
 		.phy_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -7169,27 +7169,6 @@ static int __maybe_unused mv88e6xxx_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(mv88e6xxx_pm_ops, mv88e6xxx_suspend, mv88e6xxx_resume);
 
-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);
-	}
-}
-
 static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;
@@ -7323,7 +7302,6 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		goto out_mdio;
 
-	mv88e6xxx_validate_frame_size();
 	return 0;
 
 out_mdio:
-- 
2.20.1

[PATCH 7/7] dsa: marvell: Modify get max MTU callback to use per switch provided value

[Lukasz Majewski]

After this change the value specified in max_frame_size variable is used
to provide information regarding the maximal size of frame supported in
the switch.

This approach replaces the current detection scheme, which extracts
information about max frame size depending on set of provided callbacks.

This is wrong, as some switch ICs can have max frame size equal to 1632
or 2048 and both would provide set_max_frame_size callback.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v5:
- New patch
---
 drivers/net/dsa/mv88e6xxx/chip.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dbb69787f4ef..6a5acbba381e 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)
-- 
2.20.1

Re: [PATCH 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Russell King (Oracle)]

On Thu, Mar 09, 2023 at 01:54:19PM +0100, Lukasz Majewski wrote:
> This commit shall be regarded as a transition one, as this function helps
> to validate the correctness of max_frame_size variable added to
> mv88e6xxx_info structure.
> 
> It is necessary to avoid regressions as manual assessment of this value
> turned out to be error prone.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>

Shouldn't this be patch 2 - immediately after populating the
.max_frame_size members, and before adding any additional devices?

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

Re: [PATCH 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Russell King (Oracle)]

On Thu, Mar 09, 2023 at 01:21:13PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 09, 2023 at 01:54:19PM +0100, Lukasz Majewski wrote:
> > This commit shall be regarded as a transition one, as this function helps
> > to validate the correctness of max_frame_size variable added to
> > mv88e6xxx_info structure.
> > 
> > It is necessary to avoid regressions as manual assessment of this value
> > turned out to be error prone.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> 
> Shouldn't this be patch 2 - immediately after populating the
> .max_frame_size members, and before adding any additional devices?

Moreover, shouldn't the patch order be:

1, 5, 6 (fixing the entry that needs it), 7 (which then gets the
max frame size support in place), 4 (so that .set_max_frame_size for
6250 is in place), 2, 3

?

In other words, get the new infrastructure you need in place first
(that being the new .max_frame_size and the .set_max_frame_size
function) before then adding the new support.

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

Re: [PATCH 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Lukasz Majewski]

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

Hi Russell,

> On Thu, Mar 09, 2023 at 01:21:13PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 09, 2023 at 01:54:19PM +0100, Lukasz Majewski wrote:  
> > > This commit shall be regarded as a transition one, as this
> > > function helps to validate the correctness of max_frame_size
> > > variable added to mv88e6xxx_info structure.
> > > 
> > > It is necessary to avoid regressions as manual assessment of this
> > > value turned out to be error prone.
> > > 
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>  
> > 
> > Shouldn't this be patch 2 - immediately after populating the
> > .max_frame_size members, and before adding any additional devices?  
> 
> Moreover, shouldn't the patch order be:
> 
> 1, 5, 6 (fixing the entry that needs it), 7 (which then gets the
> max frame size support in place), 4 (so that .set_max_frame_size for
> 6250 is in place), 2, 3
> 
> ?
> 
> In other words, get the new infrastructure you need in place first
> (that being the new .max_frame_size and the .set_max_frame_size
> function) before then adding the new support.
> 

Ok, I will reorder those patches and submit v6.

Do you have any other comments regarding this patch set?


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 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Vladimir Oltean]

On Thu, Mar 09, 2023 at 02:47:52PM +0100, Lukasz Majewski wrote:
> Ok, I will reorder those patches and submit v6.
> 
> Do you have any other comments regarding this patch set?

Please allow for at least 24 hours between reposts. I would like to look
at this patch set too, later today or tomorrow.

Re: [PATCH 5/7] dsa: marvell: Add helper function to validate the max_frame_size variable

[Lukasz Majewski]

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

Hi Vladimir,

> On Thu, Mar 09, 2023 at 02:47:52PM +0100, Lukasz Majewski wrote:
> > Ok, I will reorder those patches and submit v6.
> > 
> > Do you have any other comments regarding this patch set?  
> 
> Please allow for at least 24 hours between reposts. I would like to
> look at this patch set too, later today or tomorrow.

Ok. No problem.


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 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Russell King (Oracle)]

On Thu, Mar 09, 2023 at 01:54:20PM +0100, Lukasz Majewski wrote:
> Running of the mv88e6xxx_validate_frame_size() function provided following
> results:
> 
> [    1.585565] BUG: Marvell 88E6020 has differing max_frame_size: 1632 != 2048
> [    1.592540] BUG: Marvell 88E6071 has differing max_frame_size: 1632 != 2048
> 		^------ Correct -> mv88e6250 family max frame size = 2048B
> 
> [    1.599507] BUG: Marvell 88E6085 has differing max_frame_size: 1632 != 1522
> [    1.606476] BUG: Marvell 88E6165 has differing max_frame_size: 1522 != 1632
> [    1.613445] BUG: Marvell 88E6190X has differing max_frame_size: 10240 != 1522
> [    1.620590] BUG: Marvell 88E6191X has differing max_frame_size: 10240 != 1522
> [    1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240 != 1522
> 		^------ Needs to be fixed!!!
> 
> [    1.634871] BUG: Marvell 88E6220 has differing max_frame_size: 1632 != 2048
> [    1.641842] BUG: Marvell 88E6250 has differing max_frame_size: 1632 != 2048
> 		^------ Correct -> mv88e6250 family max frame size = 2048B

If I understand this correctly, in patch 4, you add a call to the 6250
family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
than 1518.

However, you're saying that 6250 has a frame size of 2048. That's fine,
but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading as a
definition. While the bit may increase the frame size, I think if we're
going to do this, then this definition ought to be renamed.

That said, I would like Andrew and Vladimir's thoughts on this too.

Finally, I would expect, if this series was done the way I suggested,
that patch 1 should set the max frame size according to how the
existing code works, which means patch 2, being the validation patch,
should be completely silent if patch 1 is correct - and that's the
entire point of validating. It's to make sure that patch 1 is
correct.

If it isn't correct, then patch 1 is wrong and should be updated.

Essentially, this patch should only exist if the values we are using
today are actually incorrect.

To put this another way, the conversion from our existing way of
determining the max mtu to using the .max_frame_size method should be
an entire no-op from the driver operation point of view. Then any
errors in those values should be fixed and explained in a separate
commit. Then the new support added.

At least that's how I see it. Andrew and Vladimir may disagree.

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

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

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

Hi Russell,

> On Thu, Mar 09, 2023 at 01:54:20PM +0100, Lukasz Majewski wrote:
> > Running of the mv88e6xxx_validate_frame_size() function provided
> > following results:
> > 
> > [    1.585565] BUG: Marvell 88E6020 has differing max_frame_size:
> > 1632 != 2048 [    1.592540] BUG: Marvell 88E6071 has differing
> > max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
> > max frame size = 2048B
> > 
> > [    1.599507] BUG: Marvell 88E6085 has differing max_frame_size:
> > 1632 != 1522 [    1.606476] BUG: Marvell 88E6165 has differing
> > max_frame_size: 1522 != 1632 [    1.613445] BUG: Marvell 88E6190X
> > has differing max_frame_size: 10240 != 1522 [    1.620590] BUG:
> > Marvell 88E6191X has differing max_frame_size: 10240 != 1522 [
> > 1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240
> > != 1522 ^------ Needs to be fixed!!!
> > 
> > [    1.634871] BUG: Marvell 88E6220 has differing max_frame_size:
> > 1632 != 2048 [    1.641842] BUG: Marvell 88E6250 has differing
> > max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
> > max frame size = 2048B  
> 
> If I understand this correctly, in patch 4, you add a call to the 6250
> family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
> called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
> than 1518.

Yes, correct.

> 
> However, you're saying that 6250 has a frame size of 2048. That's
> fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading
> as a definition. While the bit may increase the frame size, I think
> if we're going to do this, then this definition ought to be renamed.
> 

I thought about rename, but then I've double checked; register offset
and exact bit definition is the same as for 6185, so to avoid
unnecessary code duplication - I've reused the existing function.

Maybe comment would be just enough?

> That said, I would like Andrew and Vladimir's thoughts on this too.
> 

Ok.

> Finally, I would expect, if this series was done the way I suggested,
> that patch 1 should set the max frame size according to how the
> existing code works, which means patch 2, being the validation patch,
> should be completely silent if patch 1 is correct - and that's the
> entire point of validating. It's to make sure that patch 1 is
> correct.

Ok.

> 
> If it isn't correct, then patch 1 is wrong and should be updated.
> 

Please correct my understanding - I do see two approaches here:


A. In patch 1 I do set the max_frame_size values (deduced). Then I add
validation function (patch 2). This function shows "BUG:...." only when
we do have a mismatch. In patch 3 I do correct the max_frame_size
values (according to validation function) and remove the validation
function. This is how it is done in v5 and is going to be done in v6.


B. Having showed the v5 in public, the validation function is known.
Then I do prepare v6 with only patch 1 having correct values (from the
outset) and provide in the commit message the code for validation
function. Then patch 2 and 3 (validation function and the corrected
values of max_frame_size) can be omitted in v6.

For me it would be better to choose approach B.

> Essentially, this patch should only exist if the values we are using
> today are actually incorrect.
> 
> To put this another way, the conversion from our existing way of
> determining the max mtu to using the .max_frame_size method should be
> an entire no-op from the driver operation point of view. Then any
> errors in those values should be fixed and explained in a separate
> commit. Then the new support added.
> 
> At least that's how I see it. Andrew and Vladimir may disagree.
> 




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 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Russell King (Oracle)]

On Thu, Mar 09, 2023 at 03:43:50PM +0100, Lukasz Majewski wrote:
> Hi Russell,
> 
> Please correct my understanding - I do see two approaches here:
> 
> A. In patch 1 I do set the max_frame_size values (deduced). Then I add
> validation function (patch 2). This function shows "BUG:...." only when
> we do have a mismatch. In patch 3 I do correct the max_frame_size
> values (according to validation function) and remove the validation
> function. This is how it is done in v5 and is going to be done in v6.

I don't see much point in adding the validation, then correcting the
values that were added in patch 1 that were identified by patch 2 in
patch 3 - because that means patch 1's deduction was incorrect in
some way.

If there is any correction to be done, then it should be:

patch 1 - add the max_frame_size values
patch 2 - add validation (which should not produce any errors)
patch 3 - convert to use max_frame_size, and remove validation, stating
  that the validation had no errors
patch 4 (if necessary) - corrections to max_frame_size values if they
  are actually incorrect (in other words, they were buggy before patch
  1.)
patch 5 onwards - the rest of the series.

> B. Having showed the v5 in public, the validation function is known.
> Then I do prepare v6 with only patch 1 having correct values (from the
> outset) and provide in the commit message the code for validation
> function. Then patch 2 and 3 (validation function and the corrected
> values of max_frame_size) can be omitted in v6.
> 
> For me it would be better to choose approach B.

I would suggest that is acceptable for the final round of patches, but
I'm wary about saying "yes" to it because... what if something changes
in that table between the time you've validated it, and when it
eventually gets accepted. Keeping the validation code means that during
the review of the series, and subsequent updates onto net-next (which
should of course include re-running the validation code) we can be
more certain that nothing has changed that would impact it.

What I worry about is if something changes, the patch adding the
values mis-patches (e.g. due to other changes - much of the context
for each hunk is quite similar) then we will have quite a problem to
sort it out.

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

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Andrew Lunn]

> > If I understand this correctly, in patch 4, you add a call to the 6250
> > family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
> > called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
> > than 1518.
> 
> Yes, correct.
> 
> > 
> > However, you're saying that 6250 has a frame size of 2048. That's
> > fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading
> > as a definition. While the bit may increase the frame size, I think
> > if we're going to do this, then this definition ought to be renamed.
> > 
> 
> I thought about rename, but then I've double checked; register offset
> and exact bit definition is the same as for 6185, so to avoid
> unnecessary code duplication - I've reused the existing function.
> 
> Maybe comment would be just enough?

The driver takes care with its namespace in order to add per switch
family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It
does not matter if it is the same bit. You can also add a
mv88e6250_g1_set_max_frame_size() and it also does not matter if it is
in effect the same as mv88e6185_g1_set_max_frame_size().

We should always make the driver understandably first, compact and
without redundancy second. We are then less likely to get into
situations like this again where it is not clear what MTU a device
actually supports because the code is cryptic.

	 Andrew

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

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

Hi Russell,

> On Thu, Mar 09, 2023 at 03:43:50PM +0100, Lukasz Majewski wrote:
> > Hi Russell,
> > 
> > Please correct my understanding - I do see two approaches here:
> > 
> > A. In patch 1 I do set the max_frame_size values (deduced). Then I
> > add validation function (patch 2). This function shows "BUG:...."
> > only when we do have a mismatch. In patch 3 I do correct the
> > max_frame_size values (according to validation function) and remove
> > the validation function. This is how it is done in v5 and is going
> > to be done in v6.  
> 
> I don't see much point in adding the validation, then correcting the
> values that were added in patch 1 that were identified by patch 2 in
> patch 3 - because that means patch 1's deduction was incorrect in
> some way.

Yes. I do agree.

> 
> If there is any correction to be done, then it should be:
> 
> patch 1 - add the max_frame_size values
> patch 2 - add validation (which should not produce any errors)
> patch 3 - convert to use max_frame_size, and remove validation,
> stating that the validation had no errors
> patch 4 (if necessary) - corrections to max_frame_size values if they
>   are actually incorrect (in other words, they were buggy before patch
>   1.)
> patch 5 onwards - the rest of the series.
> 

Ok. I will restructure patches to follow above scheme.

> > B. Having showed the v5 in public, the validation function is known.
> > Then I do prepare v6 with only patch 1 having correct values (from
> > the outset) and provide in the commit message the code for
> > validation function. Then patch 2 and 3 (validation function and
> > the corrected values of max_frame_size) can be omitted in v6.
> > 
> > For me it would be better to choose approach B.  
> 
> I would suggest that is acceptable for the final round of patches, but
> I'm wary about saying "yes" to it because... what if something changes
> in that table between the time you've validated it, and when it
> eventually gets accepted.

The "peace" of changes for this code is rather slow, so the risk is
minimal.

Moreover, next ICs added would _require_ to have the max_frame_size
field set (the WARN_ON() clause).

> Keeping the validation code means that
> during the review of the series, and subsequent updates onto net-next
> (which should of course include re-running the validation code) we
> can be more certain that nothing has changed that would impact it.
> 
> What I worry about is if something changes, the patch adding the
> values mis-patches (e.g. due to other changes - much of the context
> for each hunk is quite similar) then we will have quite a problem to
> sort it out.
> 

Ok. I hope that we will avoid this threat.


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 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

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

Hi Andrew,

> > > If I understand this correctly, in patch 4, you add a call to the
> > > 6250 family to call mv88e6185_g1_set_max_frame_size(), which sets
> > > a bit called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size
> > > is larger than 1518.  
> > 
> > Yes, correct.
> >   
> > > 
> > > However, you're saying that 6250 has a frame size of 2048. That's
> > > fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather
> > > misleading as a definition. While the bit may increase the frame
> > > size, I think if we're going to do this, then this definition
> > > ought to be renamed. 
> > 
> > I thought about rename, but then I've double checked; register
> > offset and exact bit definition is the same as for 6185, so to avoid
> > unnecessary code duplication - I've reused the existing function.
> > 
> > Maybe comment would be just enough?  
> 
> The driver takes care with its namespace in order to add per switch
> family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It
> does not matter if it is the same bit. You can also add a
> mv88e6250_g1_set_max_frame_size() and it also does not matter if it is
> in effect the same as mv88e6185_g1_set_max_frame_size().
> 
> We should always make the driver understandably first, compact and
> without redundancy second. We are then less likely to get into
> situations like this again where it is not clear what MTU a device
> actually supports because the code is cryptic.

Ok, I will add new function.

Thanks for hints.

> 
> 	 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 1/7] dsa: marvell: Provide per device information about max frame size

[Vladimir Oltean]

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

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

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

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

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

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


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.

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

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Russell King (Oracle)]

On Fri, Mar 10, 2023 at 12:53:46PM +0100, Lukasz Majewski wrote:
> Hi Andrew,
> 
> > > > If I understand this correctly, in patch 4, you add a call to the
> > > > 6250 family to call mv88e6185_g1_set_max_frame_size(), which sets
> > > > a bit called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size
> > > > is larger than 1518.  
> > > 
> > > Yes, correct.
> > >   
> > > > 
> > > > However, you're saying that 6250 has a frame size of 2048. That's
> > > > fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather
> > > > misleading as a definition. While the bit may increase the frame
> > > > size, I think if we're going to do this, then this definition
> > > > ought to be renamed. 
> > > 
> > > I thought about rename, but then I've double checked; register
> > > offset and exact bit definition is the same as for 6185, so to avoid
> > > unnecessary code duplication - I've reused the existing function.
> > > 
> > > Maybe comment would be just enough?  
> > 
> > The driver takes care with its namespace in order to add per switch
> > family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It
> > does not matter if it is the same bit. You can also add a
> > mv88e6250_g1_set_max_frame_size() and it also does not matter if it is
> > in effect the same as mv88e6185_g1_set_max_frame_size().
> > 
> > We should always make the driver understandably first, compact and
> > without redundancy second. We are then less likely to get into
> > situations like this again where it is not clear what MTU a device
> > actually supports because the code is cryptic.
> 
> Ok, I will add new function.
> 
> Thanks for hints.

It may be worth doing:

static int mv88e6xxx_g1_modify(struct mv88e6xxx_chip *chip, int reg,
			       u16 mask, u16 val)
{
	int addr = chip->info->global1_addr;
	int err;
	u16 v;

	err = mv88e6xxx_read(chip, addr, reg, &v);
	if (err < 0)
		return err;

	v = (v & ~mask) | val;

	return mv88e6xxx_write(chip, addr, reg, v);
}

Then, mv88e6185_g1_set_max_frame_size() becomes:

int mv88e6185_g1_set_max_frame_size(struct mv88e6xxx_chip *chip, int mtu)
{
	u16 val = 0;

	if (mtu + ETH_HLEN + ETH_FCS_LEN > 1518)
		val = MV88E6185_G1_CTL1_MAX_FRAME_1632;

	return mv88e6xxx_g1_modify(chip, MV88E6XXX_G1_CTL1,
				   MV88E6185_G1_CTL1_MAX_FRAME_1632, val);
}

The 6250 variant becomes similar.

We can also think about converting all those other read-modify-writes
to use mv88e6xxx_g1_modify().

The strange thing is... we already have mv88e6xxx_g1_ctl2_mask() which
is an implementation of mv88e6xxx_g1_modify() specifically for
MV88E6XXX_G1_CTL2 register, although it uses (val & mask) rather than
just val. That wouldn't be necessary if the bitfield macros (e.g.
FIELD_PREP() were used rather than explicit __bf_shf().

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

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

[Russell King (Oracle)]

On Fri, Mar 10, 2023 at 02:02:35PM +0200, Vladimir Oltean wrote:
> 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 similar analysis to what I did for a previous patch set, and
came to the conclusion that we need code in the driver to validate
that the addition of these values is in fact correct. See my previous
reviews and my recommendations on how to structure these patch sets,
so we as reviewers don't _have_ to go to this level of verification.

> 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'm sorry, but why is this Lukasz's problem to fix? If it's broken today
when using mv88e6xxx with this PHY, and Lukasz doesn't have this PHY,
why does Lukasz have to solve this?

> > +
> > +	/* 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.

The switch documentation only really talks about maximum frame sizes
that the switch can handle, with a few bits that configure what the
maximum frame size is. We also know how large the SRAM is, but how
the SRAM is allocated to packets is for Marvell engineers to know
and not us mere mortals.

So, the base definition for this is the information provided in the
switch documentation.

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

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

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 12:25:59PM +0000, Russell King (Oracle) wrote:
> This is similar analysis to what I did for a previous patch set, and
> came to the conclusion that we need code in the driver to validate
> that the addition of these values is in fact correct. See my previous
> reviews and my recommendations on how to structure these patch sets,
> so we as reviewers don't _have_ to go to this level of verification.

Ok, I haven't read other patches or comments except for 1/7 yet.

> > I guess I will have to fix this now, since you haven't done it.
> 
> I'm sorry, but why is this Lukasz's problem to fix? If it's broken today
> when using mv88e6xxx with this PHY, and Lukasz doesn't have this PHY,
> why does Lukasz have to solve this?

Well, in principle no one has to solve it. It would be good to not move
around broken code if we know it's broken, is what I'm saying. This is
because eventually, someone who *is* affected *will* want to fix it, and
that fix will conflict with the refactoring. Lukasz would have had the
interest in fixing it because he's touching that code. Again, I will do
this when I find the time.

> > > +	/* 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.
> 
> The switch documentation only really talks about maximum frame sizes
> that the switch can handle, with a few bits that configure what the
> maximum frame size is. We also know how large the SRAM is, but how
> the SRAM is allocated to packets is for Marvell engineers to know
> and not us mere mortals.
> 
> So, the base definition for this is the information provided in the
> switch documentation.

Agree with the "mere mortals" comment. I was trying to suggest that the
given definition tries to make it appear that we know more about the
switch internal memory allocation than we really do. "This value
corresponds to the memory allocated in switch internal memory to store
single frame" -> how do we know that it corresponds?

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

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 03:04:46PM +0200, Vladimir Oltean wrote:
> > I'm sorry, but why is this Lukasz's problem to fix? If it's broken today
> > when using mv88e6xxx with this PHY, and Lukasz doesn't have this PHY,
> > why does Lukasz have to solve this?
> 
> Well, in principle no one has to solve it. It would be good to not move
> around broken code if we know it's broken, is what I'm saying. This is
> because eventually, someone who *is* affected *will* want to fix it, and
> that fix will conflict with the refactoring. Lukasz would have had the
> interest in fixing it because he's touching that code. Again, I will do
> this when I find the time.

also: what PHY? :-/

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

[Lukasz Majewski]

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

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

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

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Lukasz Majewski]

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

Hi Russell,

> On Fri, Mar 10, 2023 at 12:53:46PM +0100, Lukasz Majewski wrote:
> > Hi Andrew,
> >   
> > > > > If I understand this correctly, in patch 4, you add a call to
> > > > > the 6250 family to call mv88e6185_g1_set_max_frame_size(),
> > > > > which sets a bit called MV88E6185_G1_CTL1_MAX_FRAME_1632 if
> > > > > the frame size is larger than 1518.    
> > > > 
> > > > Yes, correct.
> > > >     
> > > > > 
> > > > > However, you're saying that 6250 has a frame size of 2048.
> > > > > That's fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632
> > > > > rather misleading as a definition. While the bit may increase
> > > > > the frame size, I think if we're going to do this, then this
> > > > > definition ought to be renamed.   
> > > > 
> > > > I thought about rename, but then I've double checked; register
> > > > offset and exact bit definition is the same as for 6185, so to
> > > > avoid unnecessary code duplication - I've reused the existing
> > > > function.
> > > > 
> > > > Maybe comment would be just enough?    
> > > 
> > > The driver takes care with its namespace in order to add per
> > > switch family defines. So you can add
> > > MV88E6250_G1_CTL1_MAX_FRAME_2048. It does not matter if it is the
> > > same bit. You can also add a mv88e6250_g1_set_max_frame_size()
> > > and it also does not matter if it is in effect the same as
> > > mv88e6185_g1_set_max_frame_size().
> > > 
> > > We should always make the driver understandably first, compact and
> > > without redundancy second. We are then less likely to get into
> > > situations like this again where it is not clear what MTU a device
> > > actually supports because the code is cryptic.  
> > 
> > Ok, I will add new function.
> > 
> > Thanks for hints.  
> 
> It may be worth doing:
> 
> static int mv88e6xxx_g1_modify(struct mv88e6xxx_chip *chip, int reg,
> 			       u16 mask, u16 val)
> {
> 	int addr = chip->info->global1_addr;
> 	int err;
> 	u16 v;
> 
> 	err = mv88e6xxx_read(chip, addr, reg, &v);
> 	if (err < 0)
> 		return err;
> 
> 	v = (v & ~mask) | val;
> 
> 	return mv88e6xxx_write(chip, addr, reg, v);
> }
> 
> Then, mv88e6185_g1_set_max_frame_size() becomes:
> 
> int mv88e6185_g1_set_max_frame_size(struct mv88e6xxx_chip *chip, int
> mtu) {
> 	u16 val = 0;
> 
> 	if (mtu + ETH_HLEN + ETH_FCS_LEN > 1518)
> 		val = MV88E6185_G1_CTL1_MAX_FRAME_1632;
> 
> 	return mv88e6xxx_g1_modify(chip, MV88E6XXX_G1_CTL1,
> 				   MV88E6185_G1_CTL1_MAX_FRAME_1632,
> val); }
> 

Yes, correct.

> The 6250 variant becomes similar.
> 
> We can also think about converting all those other read-modify-writes
> to use mv88e6xxx_g1_modify().
> 
> The strange thing is... we already have mv88e6xxx_g1_ctl2_mask() which
> is an implementation of mv88e6xxx_g1_modify() specifically for
> MV88E6XXX_G1_CTL2 register, although it uses (val & mask) rather than
> just val. That wouldn't be necessary if the bitfield macros (e.g.
> FIELD_PREP() were used rather than explicit __bf_shf().
> 

I do have the impression that major refactoring of the mv6xxx driver
would be 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 1/7] dsa: marvell: Provide per device information about max frame size

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> > > 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.

I'm not playing dumb, I just don't understand what is meant by
"in-driver standard value". Is it the return value of mv88e6xxx_get_max_mtu()
for the MV88E6185 chip? Because I understood it to be somehow the value
returned by default, for chips which don't have a way to change the MTU
(because of the "standard" word).

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

Correct, I'm not debating this. I'm just saying, as a reader of this
patch set in linear order, that the justification is not obvious.

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

It's possible that Russell might have slightly misunderstood my quoted
reply here, because he said something about a PHY.

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

The problem with this kind of fix is that it should go to the "net" tree;
it removes a user-visible warning that could have been avoided.

OTOH, the kind of "fix" for 6250 and 6220 is different. It is
sub-optimal use of hardware capabilities. That classifies as net-next
material.

The 2 go to different kernel branches.

> 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

I will get there.. eventually.

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

[Russell King (Oracle)]

On Fri, Mar 10, 2023 at 02:02:35PM +0200, Vladimir Oltean wrote:
> 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

The question concerning the 1492 MTU (and the others above) is
something that does need to be addressed, but I don't believe it
should be part of this patch series.

In order to properly address this, we need to do a bit of research.
Originally, the driver calculated the MTU by taking the frame size
(1522, 1632 or 10240) and subtracting VLAN_ETH_HLEN and ETH_FCS_LEN.

This would mean the frame sizes were 1500, 1610 and 10218. However,
as a result of:

commit b9c587fed61cf88bd45822c3159644445f6d5aa6
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Sun Sep 26 19:41:26 2021 +0200

    dsa: mv88e6xxx: Include tagger overhead when setting MTU for DSA and CPU ports

This was changed to include the EDSA_HLEN of 8 bytes. The question
is why - and that's a question for Andrew.

The frame size check is not well described looking at the 6176
functional specification. It takes about using an adjusted frame
size in the paragraph that talks about ingress headers, but then
it only takes about adjusting by two bytes which are sent before
the DA, only if MV88E6XXX_PORT_CTL0_HEADER is set (which we don't
touch).

Against the bits that control the maximum frame size, it does state
that "the definition of frame size is counting the frame bytes from
MAC-DA through Layer2 CRC of the frame".

No mention is made whether the EDSA header is included or not, the
assumption was that it wasn't prior to the commit above, but it
would appear that caused a problem, so the EDSA header was added.

Now, obviously, on external ports (those which don't use the EDSA
header) the EDSA header doesn't restrict the size of packets sent
or received on that port. However, the header does exist on the
CPU port - and the obvious question is, does the max frame size
apply, and if so does it apply with the EDSA header included or
excluded. We don't know from the documentation.

DSA ports (those between switches) don't use the EDSA header, but
instead use the DSA header which is four bytes long. Again, whether
that is included in the maximum frame size is unspecified.

Maybe Andrew has some input here as he made the above commit and
can remember why it was necessary.

However, to me, it seems to be rather absurd as it would mean that
on a device that only supports 1522 maximum packet size, the CPU
port using an EDSA header would be incapable of sending or
receiving a packet containing 1500 bytes of payload, VLAN header
and ethernet header, because as soon as the EDSA header is added
we're over the 1522 limit - and that would basically mean the
switch can't be used in a normal ethernet network to switch
such packets.

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

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

[Lukasz Majewski]

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

Hi Vladimir,

> On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> > > > 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.  
> 
> I'm not playing dumb, I just don't understand what is meant by
> "in-driver standard value". Is it the return value of
> mv88e6xxx_get_max_mtu() for the MV88E6185 chip?

The 1632 is a value from added early switch IC to this driver.

Then the get_max_mtu function was extended to support jumbo frames.

And the extension was based on having the .set_max_frame_size defined
in *_ops structure.

> Because I understood
> it to be somehow the value returned by default, for chips which don't
> have a way to change the MTU (because of the "standard" word).
> 

Probably the "standard" shall be replaced above - it might be
misleading. "Default" would be better.

> > > > 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".  
> 
> Correct, I'm not debating this. I'm just saying, as a reader of this
> patch set in linear order, that the justification is not obvious.
> 
> > > 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.  
> 
> It's possible that Russell might have slightly misunderstood my quoted
> reply here, because he said something about a PHY.
> 
> > 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.  
> 
> The problem with this kind of fix is that it should go to the "net"
> tree; it removes a user-visible warning that could have been avoided.
> 
> OTOH, the kind of "fix" for 6250 and 6220 is different. It is
> sub-optimal use of hardware capabilities. That classifies as net-next
> material.
> 
> The 2 go to different kernel branches.
> 

As I said - v6 fixes it in the way which Russel proposed. I also do
like this approach, so to avoid "wasting effort" I would opt for having
this fix in this patchset.

(And I hope that we will finish work on it before MW closes).

> > 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  
> 
> I will get there.. eventually.




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 2/7] net: dsa: mv88e6xxx: add support for MV88E6020 switch

[Vladimir Oltean]

This patch has a prefix of "net: dsa: mv88e6xxx: " but the previous one
has "dsa: marvell: " even though it touches the same file. Please use
a single prefix consistently, I would suggest "net: dsa: mv88e6xxx: "
since that is what this driver uses in the majority of patches.

Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 12:06:15PM +0000, Russell King (Oracle) wrote:
> It may be worth doing:
> 
> static int mv88e6xxx_g1_modify(struct mv88e6xxx_chip *chip, int reg,
> 			       u16 mask, u16 val)
> {
> 	int addr = chip->info->global1_addr;
> 	int err;
> 	u16 v;
> 
> 	err = mv88e6xxx_read(chip, addr, reg, &v);
> 	if (err < 0)
> 		return err;
> 
> 	v = (v & ~mask) | val;
> 
> 	return mv88e6xxx_write(chip, addr, reg, v);
> }
> 
> Then, mv88e6185_g1_set_max_frame_size() becomes:
> 
> int mv88e6185_g1_set_max_frame_size(struct mv88e6xxx_chip *chip, int mtu)
> {
> 	u16 val = 0;
> 
> 	if (mtu + ETH_HLEN + ETH_FCS_LEN > 1518)
> 		val = MV88E6185_G1_CTL1_MAX_FRAME_1632;
> 
> 	return mv88e6xxx_g1_modify(chip, MV88E6XXX_G1_CTL1,
> 				   MV88E6185_G1_CTL1_MAX_FRAME_1632, val);
> }
> 
> The 6250 variant becomes similar.

+1, sounds good to have separate mv88e6185_g1_set_max_frame_size() and
mv88e6250_g1_set_max_frame_size() with a common implementation. It is a
lot less confusing than the two driving a bit named in the same way but
meaning different things.

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

[Vladimir Oltean]

On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> 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

Ok, so nope, what I was talking about here (MTU 1492) is *not* what you
have discussed with Russell in patch 4.

What I was talking about is this:
https://patchwork.kernel.org/project/netdevbpf/patch/20230309125421.3900962-2-lukma@denx.de/#25245979
and Russell now seems to agree with me that it should be addressed
separately, and prior to the extra development work done here.

It looks like it will also need a bit of assistance from Andrew to
untangle whether EDSA_HLEN should be included in the max_mtu calculations
for some switch families only, rather than for all.

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

[Andrew Lunn]

On Fri, Mar 10, 2023 at 05:45:11PM +0200, Vladimir Oltean wrote:
> On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> > 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
> 
> Ok, so nope, what I was talking about here (MTU 1492) is *not* what you
> have discussed with Russell in patch 4.
> 
> What I was talking about is this:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230309125421.3900962-2-lukma@denx.de/#25245979
> and Russell now seems to agree with me that it should be addressed
> separately, and prior to the extra development work done here.
> 
> It looks like it will also need a bit of assistance from Andrew to
> untangle whether EDSA_HLEN should be included in the max_mtu calculations
> for some switch families only, rather than for all.

That is hard to say. From other peoples experiments, it seems like
some families don't impose the frame length check on DSA and CPU
ports. Hence they accept frames with DSA or EDSA headers, even if that
puts them over the theoretical port max. Other families do seem to
perform check on DSA and CPU ports. I don't think the datasheets say
anything about this.

The safe thing to do is:

Assume all switches can accept the standard minimum MTU + DSA/EDSA
headers on their CPU ports.

For those switches which accept frames bigger than the standard,
assume the DSA/EDSA header is counted as part of the limit, and so
adjust the calculation.

This might short change a few switches 4/8 bytes, but that is better
than being broken because frames are dropped.

     Andrew

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

[Lukasz Majewski]

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

Hi Vladimir,

> On Fri, Mar 10, 2023 at 02:17:19PM +0100, Lukasz Majewski wrote:
> > 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  
> 
> Ok, so nope, what I was talking about here (MTU 1492) is *not* what
> you have discussed with Russell in patch 4.

The patch 4 would be related to mv88e6220 and 6250 only. It would
provide correct size of MTU.

> 
> What I was talking about is this:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230309125421.3900962-2-lukma@denx.de/#25245979
> and Russell now seems to agree with me that it should be addressed
> separately,

Ok.

> and prior to the extra development work done here.
> 

Why? Up till mine patch set was introduced the problem was unnoticed.
Could this be fixed after it is applied?

> It looks like it will also need a bit of assistance from Andrew to
> untangle whether EDSA_HLEN should be included in the max_mtu
> calculations for some switch families only, rather than for all.


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 1/7] dsa: marvell: Provide per device information about max frame size

[Vladimir Oltean]

On Sun, Mar 12, 2023 at 04:55:50PM +0100, Lukasz Majewski wrote:
> > What I was talking about is this:
> > https://patchwork.kernel.org/project/netdevbpf/patch/20230309125421.3900962-2-lukma@denx.de/#25245979
> > and Russell now seems to agree with me that it should be addressed
> > separately,
> 
> Ok.
> 
> > and prior to the extra development work done here.
> 
> Why? Up till mine patch set was introduced the problem was unnoticed.
> Could this be fixed after it is applied?

I have already explained why, here:

| in principle no one has to solve it. It would be good to not move
| around broken code if we know it's broken, is what I'm saying. This is
| because eventually, someone who *is* affected *will* want to fix it, and
| that fix will conflict with the refactoring.

Translated/rephrased.

The 1492 max_mtu issue for MV88E6165, MV88E6191, MV88E6220, MV88E6250
and MV88E6290 was introduced, according to my code analysis, by commit
b9c587fed61c ("dsa: mv88e6xxx: Include tagger overhead when setting MTU
for DSA and CPU ports").

That patch, having a Fixes: tag of 1baf0fac10fb ("net: dsa: mv88e6xxx:
Use chip-wide max frame size for MTU"), was backported to all stable
kernel trees which included commit 1baf0fac10fb.

Running "git tag --contains 1baf0fac10fb", I see that it was first
included in kernel tag v5.9. I deduce that commit b9c587fed61c ("dsa:
mv88e6xxx: Include tagger overhead when setting MTU for DSA and CPU
ports") was also backported to all stable branches more recent than the
v5.9 tag.

Consulting https://www.kernel.org/ and https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/,
I can see that the branches linux-6.2.y, linux-6.1.y, linux-5.15.y and
linux-5.10.y are still maintained by the linux-stable repository, and
contain commit b9c587fed61c (either directly or through backports).

As hinted at by Documentation/process/maintainer-netdev.rst but perhaps
insufficiently clarified, bug fixes to code maintained by netdev go to
the "main" branch of https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git,
a git tree which tracks the main branch of Linus Torvalds (which today
is in the v6.3 release candidates). From there, patches are
automatically backported by the linux-stable maintainers.

The problem with you making changes to code which was pointed out as
incorrect is that these changes will land in net-next.git, in kernel
v6.4 at the earliest.

Assuming somebody else will fix the 1492 MTU issue, there are 2 distinct
moments when that can occur, relative to when the net-next pull request
is sent to Linus Torvalds' main branch:

1. before the net-next pull request. In that case, one of the netdev
   maintainers will have to manually resolve the conflict between one of
   net.git or Linus Torvalds' git tree.

2. after the net-next pull request was accepted. What will happen is
   that net.git will merge with Linus Torvalds' changes, and it will no
   longer contain the same code as on branches 6.2, 6.1, 5.15 and 5.10,
   but rather, the code with your changes. But it is always net.git that
   someone has to develop against when submitting the 1492 MTU change.
   That fix cannot apply any further than the net-next pull request,
   which is the v6.4-rc1 tag at the earliest.

   So, for the bug fix for 1492 MTU to reach the stable branches which
   are still maintained by then, 2 strategies will be taken into
   consideration:

   - the conflicting changes (yours) are also backported along with the
     real bug fix. This is because linux-stable has a preference to not
     diverge from the code in the main branch, and would rather backport
     more than less, to achieve that. But your patch set is quite noisy.
     It touches the entire mv88e6xxx_table[] of switches. It is hard to
     predict how well this chain of dependencies will get backported all
     the way down to linux-5.10.y. If any switch family was added to
     this table since v5.10, then the backporting of your changes would
     also conflict with that addition.

   - if the linux-stable team gives up with the backporting, an email
     will be sent letting the people know, and a manually created series
     of backports can be submitted for direct inclusion into the stable
     trees.

As you can see, the complexity of fixing code in stable that has been
changed in mainline is quite a bit higher than fixing it before changing it.
Also, the result is not as clean, if you add third-party backports into
the equation. For example, someone takes a linux-6.1.y stable kernel
from the future (with the 1492 MTU issue solved) and wants to backport
v6.4 material, which includes your changes. It will conflict, because
there is no linear history. The only way to achieve linear history is to
fix the buggy code before changing it.

To your point that it's not you who chose the 1492 MTU bug but rather
that it chose you, I'm not trying to minimize that, except that I need
to point out that things like this are to be expected when you are
working on a project where you aren't the only contributor.

Since we are already so deep in process-related explanations, I don't
know how aware you are of what fixing the 1492 MTU bug entails.
One would have to prepare a patch that limits the max_mtu to ETH_DATA_LEN
for the switch families where MTU changing is not possible. Once that
gets reviewed and accepted, one would need to wait for no longer than
the next Thursday (when the net.git pull request is sent, and net.git is
merged back into net-next.git, for history linearization), then work on
net-next.git can resume.

Re: [PATCH 0/7] dsa: marvell: Add support for mv88e6071 and 6020 switches

[Vladimir Oltean]

Hello Lukasz,

On Thu, Mar 09, 2023 at 01:54:14PM +0100, Lukasz Majewski wrote:
> This patch set provides following changes:
> 
> - Provide support for mv88e6020 and mv88e6071 switch circuits (the
>   "Link Street" family of products including added earlier to this
>   driver mv88e6250 and mv88e6220).
> 
> - Add the max_frame size variable to specify the buffer size for the
>   maximal frame size
> 
> - The above change required adjusting all supported devices in the
>   mv88e6xxx driver, as the current value assignment is depending
>   on the set of provided callbacks for each switch circuit - i.e.
>   until now the value was not explicitly specified.
> 
> - As the driver for Marvell's mv88e6xxx switches was rather complicated
>   the intermediate function (removed by the end of this patch set)
>   has been introduced. It was supposed to both validate the provided
>   values deduced from the code and leave a trace of the exact
>   methodology used.

The problem with MTU 1492 has been resolved as commit 7e9517375a14
("net: dsa: mv88e6xxx: fix max_mtu of 1492 on 6165, 6191, 6220, 6250,
6290"), is present in net-next, and as such, you are free to resubmit
this patchset on top of the current net-next.gi/main whenever you feel
like it, no need to wait for a merge window.