netdev - Re: [PATCH net-next v4] net: dsa: realtek: rtl8365mb: add change

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: <ZAh5ocHELAK9PSux@corigine.com>
Date:   Wed, 8 Mar 2023 13:03:45 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     netdev@...r.kernel.org, linus.walleij@...aro.org,
        alsi@...g-olufsen.dk, andrew@...n.ch, vivien.didelot@...il.com,
        f.fainelli@...il.com, olteanv@...il.com, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org,
        krzk+dt@...nel.org, arinc.unal@...nc9.com,
        Alexander Duyck <alexanderduyck@...com>
Subject: Re: [PATCH net-next v4] net: dsa: realtek: rtl8365mb: add change_mtu

On Tue, Mar 07, 2023 at 06:02:46PM -0300, Luiz Angelo Daros de Luca wrote:
> The rtl8365mb was using a fixed MTU size of 1536, which was probably
> inspired by the rtl8366rb's initial packet size. However, unlike that
> family, the rtl8365mb family can specify the max packet size in bytes,
> rather than in fixed steps. The max packet size now defaults to
> VLAN_ETH_HLEN+ETH_DATA_LEN+ETH_FCS_LEN, which is 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 specifications do not directly define the max supported
> packet size, but it mentions a 16k limit. This driver will use the 0x3FFF
> limit as it is used in the vendor API code. However, the switch sets the
> max packet size to 16368 bytes (0x3FF0) after it resets.
> 
> change_mtu uses MTU size, or ethernet payload size, while the switch
> works with frame size. The frame size is calculated considering the
> ethernet header (14 bytes), a possible 802.1Q tag (4 bytes), the payload
> size (MTU), and the Ethernet FCS (4 bytes). The CPU tag (8 bytes) is
> consumed before the switch enforces the limit.
> 
> MTU was tested up to 2018 (with 802.1Q) as that is as far as mt7620
> (where rtl8367s is stacked) can go. The register was manually
> manipulated byte-by-byte to ensure the MTU to frame size conversion was
> correct. For frames without 802.1Q tag, the frame size limit will be 4
> bytes over the required size.
> 
> There is a jumbo register, enabled by default at 6k packet size.
> However, the jumbo settings do 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>
> Reviewed-by: Alexander Duyck <alexanderduyck@...com>
> Reviewed-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 40 ++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> v3->v4:
> - removed spurious newline after comment.
> 
> v2->v3:
> - changed max frame size to 0x3FFF (used by vendor API)
> - added info about how frame size is calculated, some more description
>   about the tests performed and the 4 extra bytes when untagged frame is
>   used.
> 
> v1->v2:
> - dropped jumbo code as it was not changing the behavior (up to 2k MTU)
> - fixed typos
> - fixed code alignment
> - renamed rtl8365mb_(change|max)_mtu to rtl8365mb_port_(change|max)_mtu
> 
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index da31d8b839ac..41ea3b5a42b1 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c

...

> @@ -1980,10 +2011,7 @@ 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));
> +	ret = rtl8365mb_port_change_mtu(ds, cpu->trap_port, ETH_DATA_LEN);

Hi Luiz,

Perhaps I am misreading this, perhaps it was discussed elsewhere (I did
look), and perhaps it's not important. But prior to this
patch a value of 1536 is used. Whereas with this patch the
value, calculated in rtl8365mb_port_change_mtu, is
ETH_DATA_LEN + VLAN_ETH_HLEN + ETH_FCS_LEN = 1500 + 18 + 4 = 1522.

>  	if (ret)
>  		goto out_teardown_irq;
>  

...