| 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
| ||
|
Message-ID: <CAJq09z4fJmc9=CwdVSS_+LfOS9ax+voWrkPMwDmiBtrCwzc20A@mail.gmail.com>
Date: Mon, 11 Dec 2023 00:14:39 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Alvin Šipraga <alsi@...g-olufsen.dk>,
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
Subject: Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU handling
> 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>
Don't we need a Fixes tag?
> ---
> 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];
> }
I'm not sure you need this old code. Whenever you change the MTU in a
user port, it will also call rtl8366rb_change_mtu() for the CPU port
if the max MTU changes. A call to change both the port and the CPU
port makes sense when you need to update something inside the switch
to set the MTU per port. However, these realtek switches only have a
global MTU size for all ports. What I did in rtl8365mb is to ignore
any MTU change except it is related to the CPU port. I hope this is a
"core feature" that you can rely on.
If that works for you, you can also drop the rb->max_mtu and the code
in rtl8366rb_setup(), calling rtl8366rb_change_mtu() for the CPU port
instead.
> - 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
>
>
Powered by blists - more mailing lists