netdev - Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU handling

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: <kcqdranbwdvkgrambeg2f7cfxguzodtzgd2yrecu6scau27vzc@o6gxdpxuzotr>
Date: Sun, 10 Dec 2023 13:00:46 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Linus Walleij <linus.walleij@...aro.org>
CC: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU
 handling

On Sat, Dec 09, 2023 at 11:37:35PM +0100, Linus Walleij wrote:
> The MTU callbacks are in layer 1 size, so for example 1500
> bytes is a normal setting. Cache this size, and only add
> the layer 2 framing right before choosing the setting. On
> the CPU port this will however include the DSA tag since
> this is transmitted from the parent ethernet interface!
> 
> Add the layer 2 overhead such as ethernet and VLAN framing
> and FCS before selecting the size in the register.
> 
> This will make the code easier to understand.
> 
> The rtl8366rb_max_mtu() callback returns a bogus MTU
> just subtracting the CPU tag, which is the only thing
> we should NOT subtract. Return the correct layer 1
> max MTU after removing headers and checksum.
> 
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>

Reviewed-by: Alvin Šipraga <alsi@...g-olufsen.dk>

> ---
>  drivers/net/dsa/realtek/rtl8366rb.c | 48 +++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
> index 887afd1392cb..e3b6a470ca67 100644
> --- a/drivers/net/dsa/realtek/rtl8366rb.c
> +++ b/drivers/net/dsa/realtek/rtl8366rb.c
> @@ -15,6 +15,7 @@
>  #include <linux/bitops.h>
>  #include <linux/etherdevice.h>
>  #include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip/chained_irq.h>
> @@ -929,15 +930,19 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> -	/* Set maximum packet length to 1536 bytes */
> +	/* Set default maximum packet length to 1536 bytes */
>  	ret = regmap_update_bits(priv->map, RTL8366RB_SGCR,
>  				 RTL8366RB_SGCR_MAX_LENGTH_MASK,
>  				 RTL8366RB_SGCR_MAX_LENGTH_1536);
>  	if (ret)
>  		return ret;
> -	for (i = 0; i < RTL8366RB_NUM_PORTS; i++)
> -		/* layer 2 size, see rtl8366rb_change_mtu() */
> -		rb->max_mtu[i] = 1532;
> +	for (i = 0; i < RTL8366RB_NUM_PORTS; i++) {
> +		if (i == priv->cpu_port)
> +			/* CPU port need to also accept the tag */
> +			rb->max_mtu[i] = ETH_DATA_LEN + RTL8366RB_CPU_TAG_SIZE;
> +		else
> +			rb->max_mtu[i] = ETH_DATA_LEN;
> +	}
>  
>  	/* Disable learning for all ports */
>  	ret = regmap_write(priv->map, RTL8366RB_PORT_LEARNDIS_CTRL,
> @@ -1442,24 +1447,29 @@ static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	/* Roof out the MTU for the entire switch to the greatest
>  	 * common denominator: the biggest set for any one port will
>  	 * be the biggest MTU for the switch.
> -	 *
> -	 * The first setting, 1522 bytes, is max IP packet 1500 bytes,
> -	 * plus ethernet header, 1518 bytes, plus CPU tag, 4 bytes.
> -	 * This function should consider the parameter an SDU, so the
> -	 * MTU passed for this setting is 1518 bytes. The same logic
> -	 * of subtracting the DSA tag of 4 bytes apply to the other
> -	 * settings.
>  	 */
> -	max_mtu = 1518;
> +	max_mtu = ETH_DATA_LEN;
>  	for (i = 0; i < RTL8366RB_NUM_PORTS; i++) {
>  		if (rb->max_mtu[i] > max_mtu)
>  			max_mtu = rb->max_mtu[i];
>  	}
> -	if (max_mtu <= 1518)
> +
> +	/* Translate to layer 2 size.
> +	 * Add ethernet and (possible) VLAN headers, and checksum to the size.
> +	 * For ETH_DATA_LEN (1500 bytes) this will add up to 1522 bytes.
> +	 */
> +	max_mtu += VLAN_ETH_HLEN;
> +	max_mtu += ETH_FCS_LEN;
> +
> +	if (max_mtu <= 1522)
>  		len = RTL8366RB_SGCR_MAX_LENGTH_1522;
> -	else if (max_mtu > 1518 && max_mtu <= 1532)
> +	else if (max_mtu > 1522 && max_mtu <= 1536)
> +		/* This will be the most common default if using VLAN and
> +		 * CPU tagging on a port as both VLAN and CPU tag will
> +		 * result in 1518 + 4 + 4 = 1526 bytes.
> +		 */
>  		len = RTL8366RB_SGCR_MAX_LENGTH_1536;
> -	else if (max_mtu > 1532 && max_mtu <= 1548)
> +	else if (max_mtu > 1536 && max_mtu <= 1552)
>  		len = RTL8366RB_SGCR_MAX_LENGTH_1552;
>  	else
>  		len = RTL8366RB_SGCR_MAX_LENGTH_16000;
> @@ -1471,10 +1481,12 @@ static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  
>  static int rtl8366rb_max_mtu(struct dsa_switch *ds, int port)
>  {
> -	/* The max MTU is 16000 bytes, so we subtract the CPU tag
> -	 * and the max presented to the system is 15996 bytes.
> +	/* The max MTU is 16000 bytes, so we subtract the ethernet
> +	 * headers with VLAN and checksum and arrive at
> +	 * 16000 - 18 - 4 = 15978. This does not include the CPU tag
> +	 * since that is added to the requested MTU by the DSA framework.
>  	 */
> -	return 15996;
> +	return 16000 - VLAN_ETH_HLEN - ETH_FCS_LEN;
>  }
>  
>  static int rtl8366rb_get_vlan_4k(struct realtek_priv *priv, u32 vid,
> 
> -- 
> 2.34.1
>