| 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: <20230121120744.4krs4esfab56pvdz@bang-olufsen.dk>
Date: Sat, 21 Jan 2023 12:07:45 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: add change_mtu
On Fri, Jan 20, 2023 at 02:00:26PM -0300, Luiz Angelo Daros de Luca wrote:
> rtl8365mb was using a fixed MTU size of 1536, probably inspired by
> rtl8366rb initial packet size. Different from that family, rtl8365mb
> family can specify the max packet size in bytes and not in fixed steps.
> Now it defaults to VLAN_ETH_HLEN+ETH_DATA_LEN+ETH_FCS_LEN (1522 bytes).
>
> DSA calls change_mtu for the CPU port once the max mtu value among the
> ports changes. As the max packet size is defined globally, the switch
> is configured only when the call affects the CPU port.
>
> The available specs do not directly define the max supported packet
> size, but it mentions a 16k limit. However, the switch sets the max
> packet size to 16368 bytes (0x3FF0) after it resets. That value was
> assumed as the maximum supported packet size.
>
> MTU was tested up to 2018 (with 802.1Q) as that is as far as mt7620
> (where rtl8367s is stacked) can go.
>
> There is a jumbo register, enabled by default at 6k packet size.
> However, the jumbo settings does not seem to limit nor expand the
> maximum tested MTU (2018), even when jumbo is disabled. More tests are
> needed with a device that can handle larger frames.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> drivers/net/dsa/realtek/rtl8365mb.c | 63 +++++++++++++++++++++++++++--
> 1 file changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index da31d8b839ac..da9e5f16c8cc 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -98,6 +98,7 @@
> #include <linux/of_irq.h>
> #include <linux/regmap.h>
> #include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
>
> #include "realtek.h"
>
> @@ -267,6 +268,12 @@
> /* Maximum packet length register */
> #define RTL8365MB_CFG0_MAX_LEN_REG 0x088C
> #define RTL8365MB_CFG0_MAX_LEN_MASK 0x3FFF
> +/* Not sure but it is the default value after reset */
> +#define RTL8365MB_CFG0_MAX_LEN_MAX 0x3FF0
The max length is 0x3FFF as seen in the mask defined right above.
> +
> +#define RTL8365MB_FLOWCTRL_JUMBO_SIZE_REG 0x123B
> +#define RTL8365MB_FLOWCTRL_JUMBO_SIZE_MASK GENMASK(1,0)
> +#define RTL8365MB_FLOWCTRL_JUMBO_ENABLE_MASK GENMASK(2,2)
You were unable to test this so please drop it from the patch.
>
> /* Port learning limit registers */
> #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE 0x0A20
> @@ -309,6 +316,14 @@
> */
> #define RTL8365MB_STATS_INTERVAL_JIFFIES (3 * HZ)
>
> +/* FIXME: is k in {3,4,6,9}k 1000 or 1024 */
> +enum rtl8365mb_jumbo_size {
> + RTL8365MB_JUMBO_SIZE_3K = 0,
> + RTL8365MB_JUMBO_SIZE_4K,
> + RTL8365MB_JUMBO_SIZE_6K,
> + RTL8365MB_JUMBO_SIZE_9K
> +};
> +
> enum rtl8365mb_mib_counter_index {
> RTL8365MB_MIB_ifInOctets,
> RTL8365MB_MIB_dot3StatsFCSErrors,
> @@ -1135,6 +1150,44 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
> }
> }
>
> +static int rtl8365mb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> +{
> + struct realtek_priv *priv = ds->priv;
> +
> + /* When a new MTU is set, DSA always set the CPU port's MTU to the
s/always set/always sets/
> + * largest MTU of the slave ports. Because the switch only has a global
> + * RX length register, only allowing CPU port here is enough.
> + */
> + if (!dsa_is_cpu_port(ds, port))
> + return 0;
> +
> + new_mtu += VLAN_ETH_HLEN + ETH_FCS_LEN;
Why VLAN_ETH_HLEN rather than ETH_HLEN?
> +
> + /* FIXME: We might need to adjust the jumbo size as well. However, the
> + * device seems to forward at least up to mtu=2018 (test device limit)
> + * even with jumbo frames disabled.
> + */
> + /* This is the switch state after reset */
> + /*enum rtl8365mb_jumbo_size jumbo_size = RTL8365MB_JUMBO_SIZE_6K;
> +
> + regmap_update_bits(priv->map, RTL8365MB_FLOWCTRL_JUMBO_SIZE_REG,
> + RTL8365MB_FLOWCTRL_JUMBO_ENABLE_MASK |
> + RTL8365MB_FLOWCTRL_JUMBO_SIZE_MASK,
> + FIELD_PREP(RTL8365MB_FLOWCTRL_JUMBO_ENABLE_MASK,1) |
> + FIELD_PREP(RTL8365MB_FLOWCTRL_JUMBO_SIZE_MASK,
> + jumbo_size));
> + */
No commented out code can be accepted into the driver, please drop it
from the patch if it is not able to be used.
> +
> + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> + RTL8365MB_CFG0_MAX_LEN_MASK,
> + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, new_mtu));
80 columns + looks like arguments aren't aligned with the (
> +}
> +
> +static int rtl8365mb_max_mtu(struct dsa_switch *ds, int port)
> +{
> + return RTL8365MB_CFG0_MAX_LEN_MAX - VLAN_ETH_HLEN - ETH_FCS_LEN;
> +}
> +
> static void rtl8365mb_port_stp_state_set(struct dsa_switch *ds, int port,
> u8 state)
> {
> @@ -1980,10 +2033,8 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> p->index = i;
> }
>
> - /* Set maximum packet length to 1536 bytes */
> - ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> - RTL8365MB_CFG0_MAX_LEN_MASK,
> - FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
> + /* Set packet length from 16368 to 1500+14+4+4=1522 */
> + ret = rtl8365mb_change_mtu(ds, cpu->trap_port, ETH_DATA_LEN);
I think DSA sets the MTU automatically, cf. dsa_slave_create(). This can
probably be dropped now that the dsa_switch_ops are implemented.
> if (ret)
> goto out_teardown_irq;
>
> @@ -2103,6 +2154,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
> .get_eth_mac_stats = rtl8365mb_get_mac_stats,
> .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats,
> .get_stats64 = rtl8365mb_get_stats64,
> + .port_change_mtu = rtl8365mb_change_mtu,
> + .port_max_mtu = rtl8365mb_max_mtu,
Please name the functions according to the dsa_switch_ops name:
rtl8365mb_change_mtu -> rtl8365mb_port_change_mtu
rtl8365mb_max_mtu -> rtl8365mb_port_max_mtu
> };
>
> static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
> @@ -2124,6 +2177,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
> .get_eth_mac_stats = rtl8365mb_get_mac_stats,
> .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats,
> .get_stats64 = rtl8365mb_get_stats64,
> + .port_change_mtu = rtl8365mb_change_mtu,
> + .port_max_mtu = rtl8365mb_max_mtu,
> };
>
> static const struct realtek_ops rtl8365mb_ops = {
> --
> 2.39.0
>
Kind regards,
Alvin
Powered by blists - more mailing lists