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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d16ffd327d193f8c1f7c40f968fda90a267348e.camel@gmail.com>
Date:   Thu, 15 Dec 2022 08:48:10 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Lukasz Majewski <lukma@...x.de>, Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dsa: marvell: Provide per device information
 about max frame size

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