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@xxxxxxx>
> ---
> 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)".