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

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

[Lukasz Majewski]

Different Marvell DSA switches support different size of max frame
bytes to be sent.

For example mv88e6185 supports max 1632 bytes, which is now in-driver
standard value. On the other hand - mv88e6250 supports 2048 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.

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
---
 drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2ca3cbba5764..7ae4c389ce50 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
 	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 (chip->info->max_frame_size  - VLAN_ETH_HLEN
+			- EDSA_HLEN - ETH_FCS_LEN);
+
 	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
 }
 
@@ -4461,6 +4463,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6250_ptp_ops,
 	.phylink_validate = mv88e6065_phylink_validate,
+	.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
 };
 
 static const struct mv88e6xxx_ops mv88e6290_ops = {
@@ -5060,6 +5063,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.atu_move_port_mask = 0xf,
 		.multi_chip = true,
 		.ops = &mv88e6095_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6097] = {
@@ -5083,6 +5087,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6097_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6123] = {
@@ -5106,6 +5111,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6123_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6131] = {
@@ -5174,6 +5180,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
 		.ops = &mv88e6161_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6165] = {
@@ -5197,6 +5204,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.ptp_support = true,
 		.ops = &mv88e6165_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6171] = {
@@ -5312,6 +5320,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6185_ops,
+		.max_frame_size = 1632,
 	},
 
 	[MV88E6190] = {
@@ -5440,6 +5449,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,
@@ -5486,6 +5496,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,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 80dc7b549e81..9712b10fc4ed 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -130,6 +130,7 @@ struct mv88e6xxx_info {
 	unsigned int num_internal_phys;
 	unsigned int num_gpio;
 	unsigned int max_vid;
+	unsigned int max_frame_size;
 	unsigned int port_base_addr;
 	unsigned int phy_base_addr;
 	unsigned int global1_addr;
-- 
2.37.3

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

[Lukasz Majewski]

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

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

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Add S-o-B
- Update commit message
- Add information about max packet size (2048 B)
---
 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 7ae4c389ce50..ac0794e405bd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5023,6 +5023,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 = 7,
+		.num_internal_phys = 5,
+		.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 9712b10fc4ed..58cf1e633ce3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -53,6 +53,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 5c347cc58baf..862d1fe1aa15 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -106,6 +106,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.37.3

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

[Lukasz Majewski]

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Update commit message
- Add information about max frame size
---
 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 ac0794e405bd..4277216af1cf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5044,6 +5044,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 58cf1e633ce3..a0fb2b465400 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,
+	MV88E6071,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 862d1fe1aa15..08e544bf54c5 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -107,6 +107,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.37.3

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

[Alexander H Duyck]

On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:
> Different Marvell DSA switches support different size of max frame
> bytes to be sent.
> 
> For example mv88e6185 supports max 1632 bytes, which is now in-driver
> standard value. On the other hand - mv88e6250 supports 2048 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.
> 
> 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
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
>  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 2ca3cbba5764..7ae4c389ce50 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
>  	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 (chip->info->max_frame_size  - VLAN_ETH_HLEN
> +			- EDSA_HLEN - ETH_FCS_LEN);
> +
>  	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
>  }
> 
> 

Is there any specific reason for triggering this based on the existance
of the function call? Why not just replace:
	else if (chip->info->ops->set_max_frame_size)
with:
	else if (chip->info->max_frame_size)

Otherwise my concern is one gets defined without the other leading to a
future issue as 0 - extra headers will likely wrap and while the return
value may be a signed int, it is usually stored in an unsigned int so
it would effectively uncap the MTU.

Actually you could take this one step further since all values should
be 1522 or greater you could just drop the else/if and replace the last
line with "max_t(int, chip->info->max_frame_size, 1522) - (headers)".

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

[Jakub Kicinski]

On Thu, 15 Dec 2022 15:45:34 +0100 Lukasz Majewski wrote:
> Different Marvell DSA switches support different size of max frame
> bytes to be sent.
> 
> For example mv88e6185 supports max 1632 bytes, which is now in-driver
> standard value. On the other hand - mv88e6250 supports 2048 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.

# Form letter - net-next is closed

We have already submitted the networking pull request to Linus
for v6.2 and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after Jan 2nd.

RFC patches sent for review only are obviously welcome at any time.

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

[Lukasz Majewski]

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

Hi Jakub,

> On Thu, 15 Dec 2022 15:45:34 +0100 Lukasz Majewski wrote:
> > Different Marvell DSA switches support different size of max frame
> > bytes to be sent.
> > 
> > For example mv88e6185 supports max 1632 bytes, which is now
> > in-driver standard value. On the other hand - mv88e6250 supports
> > 2048 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.  
> 
> # Form letter - net-next is closed
> 

I see....

> We have already submitted the networking pull request to Linus
> for v6.2 and therefore net-next is closed for new drivers, features,
> code refactoring and optimizations. We are currently accepting
> bug fixes only.

Ok.

> 
> Please repost when net-next reopens after Jan 2nd.
> 
> RFC patches sent for review only are obviously welcome at any time.

I hope that the discussion regarding those patches will be done by this
time.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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 v2 1/3] dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski]

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

Hi Alexander,

> On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:
> > Different Marvell DSA switches support different size of max frame
> > bytes to be sent.
> > 
> > For example mv88e6185 supports max 1632 bytes, which is now
> > in-driver standard value. On the other hand - mv88e6250 supports
> > 2048 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.
> > 
> > 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
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
> >  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50
> > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > dsa_switch *ds, int port) 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 (chip->info->max_frame_size  - VLAN_ETH_HLEN
> > +			- EDSA_HLEN - ETH_FCS_LEN);
> > +
> >  	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> >  }
> > 
> >   
> 
> Is there any specific reason for triggering this based on the
> existance of the function call? 

This was the original code in this driver.

This value (1632 or 2048 bytes) is SoC (family) specific.

By checking which device defines set_max_frame_size callback, I could
fill the chip->info->max_frame_size with 1632 value.

> Why not just replace:
> 	else if (chip->info->ops->set_max_frame_size)
> with:
> 	else if (chip->info->max_frame_size)
> 

I think that the callback check is a bit "defensive" approach -> 1522B
is the default value and 1632 (or 10240 - jumbo) can be set only when
proper callback is defined.

> Otherwise my concern is one gets defined without the other leading to
> a future issue as 0 - extra headers will likely wrap and while the
> return value may be a signed int, it is usually stored in an unsigned
> int so it would effectively uncap the MTU.

Please correct me if I misunderstood something:

The problem is with new mv88eXXXX devices, which will not provide
max_frame_size information to their chip->info struct?

Or is there any other issue?

> 
> Actually you could take this one step further since all values should
> be 1522 or greater you could just drop the else/if and replace the
> last line with "max_t(int, chip->info->max_frame_size, 1522) -
> (headers)".

This would be possible, yes.

However, then we will not check if the proper callback (e.g.
chip->info->ops->set_max_frame_size) is available for specific SoC.

If this is OK for maintainers for this driver, then I don't mind.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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 v2 1/3] dsa: marvell: Provide per device information about max frame size

[Alexander Duyck]

On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alexander,
>
> > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:
> > > Different Marvell DSA switches support different size of max frame
> > > bytes to be sent.
> > >
> > > For example mv88e6185 supports max 1632 bytes, which is now
> > > in-driver standard value. On the other hand - mv88e6250 supports
> > > 2048 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.
> > >
> > > 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
> > > ---
> > >  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
> > >  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50
> > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > > dsa_switch *ds, int port) 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 (chip->info->max_frame_size  - VLAN_ETH_HLEN
> > > +                   - EDSA_HLEN - ETH_FCS_LEN);
> > > +
> > >     return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > >  }
> > >
> > >
> >
> > Is there any specific reason for triggering this based on the
> > existance of the function call?
>
> This was the original code in this driver.
>
> This value (1632 or 2048 bytes) is SoC (family) specific.
>
> By checking which device defines set_max_frame_size callback, I could
> fill the chip->info->max_frame_size with 1632 value.
>
> > Why not just replace:
> >       else if (chip->info->ops->set_max_frame_size)
> > with:
> >       else if (chip->info->max_frame_size)
> >
>
> I think that the callback check is a bit "defensive" approach -> 1522B
> is the default value and 1632 (or 10240 - jumbo) can be set only when
> proper callback is defined.
>
> > Otherwise my concern is one gets defined without the other leading to
> > a future issue as 0 - extra headers will likely wrap and while the
> > return value may be a signed int, it is usually stored in an unsigned
> > int so it would effectively uncap the MTU.
>
> Please correct me if I misunderstood something:
>
> The problem is with new mv88eXXXX devices, which will not provide
> max_frame_size information to their chip->info struct?
>
> Or is there any other issue?

That was mostly my concern. I was adding a bit of my own defensive
programming in the event that somebody forgot to fill out the
chip->info. If nothing else it might make sense to add a check to
verify that the max_frame_size is populated before blindly using it.
So perhaps you could do something similar to the max_t approach I had
called out earlier but instead of applying it on the last case you
could apply it for the "set_max_frame_size" case with 1632 being the
minimum and being overwritten by 2048 if it is set in max_frame_size.

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

[Lukasz Majewski]

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

Hi Alexander,

> On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alexander,
> >  
> > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:  
> > > > Different Marvell DSA switches support different size of max
> > > > frame bytes to be sent.
> > > >
> > > > For example mv88e6185 supports max 1632 bytes, which is now
> > > > in-driver standard value. On the other hand - mv88e6250 supports
> > > > 2048 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.
> > > >
> > > > 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 ---
> > > >  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
> > > >  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
> > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > b/drivers/net/dsa/mv88e6xxx/chip.c index
> > > > 2ca3cbba5764..7ae4c389ce50 100644 ---
> > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++
> > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3093,7 +3093,9 @@ static
> > > > int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) 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 (chip->info->max_frame_size  - VLAN_ETH_HLEN
> > > > +                   - EDSA_HLEN - ETH_FCS_LEN);
> > > > +
> > > >     return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > > >  }
> > > >
> > > >  
> > >
> > > Is there any specific reason for triggering this based on the
> > > existance of the function call?  
> >
> > This was the original code in this driver.
> >
> > This value (1632 or 2048 bytes) is SoC (family) specific.
> >
> > By checking which device defines set_max_frame_size callback, I
> > could fill the chip->info->max_frame_size with 1632 value.
> >  
> > > Why not just replace:
> > >       else if (chip->info->ops->set_max_frame_size)
> > > with:
> > >       else if (chip->info->max_frame_size)
> > >  
> >
> > I think that the callback check is a bit "defensive" approach ->
> > 1522B is the default value and 1632 (or 10240 - jumbo) can be set
> > only when proper callback is defined.
> >  
> > > Otherwise my concern is one gets defined without the other
> > > leading to a future issue as 0 - extra headers will likely wrap
> > > and while the return value may be a signed int, it is usually
> > > stored in an unsigned int so it would effectively uncap the MTU.  
> >
> > Please correct me if I misunderstood something:
> >
> > The problem is with new mv88eXXXX devices, which will not provide
> > max_frame_size information to their chip->info struct?
> >
> > Or is there any other issue?  
> 
> That was mostly my concern. I was adding a bit of my own defensive
> programming in the event that somebody forgot to fill out the
> chip->info. If nothing else it might make sense to add a check to
> verify that the max_frame_size is populated before blindly using it.
> So perhaps you could do something similar to the max_t approach I had
> called out earlier but instead of applying it on the last case you
> could apply it for the "set_max_frame_size" case with 1632 being the
> minimum and being overwritten by 2048 if it is set in max_frame_size.

I think that I shall add:

else if (chip->info->ops->set_max_frame_size)
	return max_t(int, chip->info->max_frame_size, 1632) - (headers)

So then the "default" value of 1632 will be overwritten by 2048 bytes.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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 v2 1/3] dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski]

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

Hi Alexander,

> Hi Alexander,
> 
> > On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > >
> > > Hi Alexander,
> > >    
> > > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote:    
> > > > > Different Marvell DSA switches support different size of max
> > > > > frame bytes to be sent.
> > > > >
> > > > > For example mv88e6185 supports max 1632 bytes, which is now
> > > > > in-driver standard value. On the other hand - mv88e6250
> > > > > supports 2048 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.
> > > > >
> > > > > 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 ---
> > > > >  drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
> > > > >  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
> > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > b/drivers/net/dsa/mv88e6xxx/chip.c index
> > > > > 2ca3cbba5764..7ae4c389ce50 100644 ---
> > > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++
> > > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3093,7 +3093,9 @@
> > > > > static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int
> > > > > port) 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 (chip->info->max_frame_size  -
> > > > > VLAN_ETH_HLEN
> > > > > +                   - EDSA_HLEN - ETH_FCS_LEN);
> > > > > +
> > > > >     return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > > > >  }
> > > > >
> > > > >    
> > > >
> > > > Is there any specific reason for triggering this based on the
> > > > existance of the function call?    
> > >
> > > This was the original code in this driver.
> > >
> > > This value (1632 or 2048 bytes) is SoC (family) specific.
> > >
> > > By checking which device defines set_max_frame_size callback, I
> > > could fill the chip->info->max_frame_size with 1632 value.
> > >    
> > > > Why not just replace:
> > > >       else if (chip->info->ops->set_max_frame_size)
> > > > with:
> > > >       else if (chip->info->max_frame_size)
> > > >    
> > >
> > > I think that the callback check is a bit "defensive" approach ->
> > > 1522B is the default value and 1632 (or 10240 - jumbo) can be set
> > > only when proper callback is defined.
> > >    
> > > > Otherwise my concern is one gets defined without the other
> > > > leading to a future issue as 0 - extra headers will likely wrap
> > > > and while the return value may be a signed int, it is usually
> > > > stored in an unsigned int so it would effectively uncap the
> > > > MTU.    
> > >
> > > Please correct me if I misunderstood something:
> > >
> > > The problem is with new mv88eXXXX devices, which will not provide
> > > max_frame_size information to their chip->info struct?
> > >
> > > Or is there any other issue?    
> > 
> > That was mostly my concern. I was adding a bit of my own defensive
> > programming in the event that somebody forgot to fill out the
> > chip->info. If nothing else it might make sense to add a check to
> > verify that the max_frame_size is populated before blindly using it.
> > So perhaps you could do something similar to the max_t approach I
> > had called out earlier but instead of applying it on the last case
> > you could apply it for the "set_max_frame_size" case with 1632
> > being the minimum and being overwritten by 2048 if it is set in
> > max_frame_size.  
> 
> I think that I shall add:
> 
> else if (chip->info->ops->set_max_frame_size)
> 	return max_t(int, chip->info->max_frame_size, 1632) -
> (headers)
> 
> So then the "default" value of 1632 will be overwritten by 2048 bytes.
> 

Is this approach acceptable for you?

> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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 --]