| Message ID | 20230307210245.542-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] net: dsa: realtek: rtl8365mb: add change_mtu | expand |
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@gmail.com> > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com> > Reviewed-by: Alvin Šipraga <alsi@bang-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; > ...
> Hi Luiz, Hi Simon, >> + ret = rtl8365mb_port_change_mtu(ds, cpu->trap_port, ETH_DATA_LEN); This call just synchronizes the switch frame size with the default MTU (1500). > 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. That value, as mentioned in the commit message, probably came from rtl8366rb driver jumbo frame settings. The "rtl8366rb family" has 4 levels of jumbo frame size: #define RTL8366RB_SGCR_MAX_LENGTH_1522 RTL8366RB_SGCR_MAX_LENGTH(0x0) #define RTL8366RB_SGCR_MAX_LENGTH_1536 RTL8366RB_SGCR_MAX_LENGTH(0x1) #define RTL8366RB_SGCR_MAX_LENGTH_1552 RTL8366RB_SGCR_MAX_LENGTH(0x2) #define RTL8366RB_SGCR_MAX_LENGTH_16000 RTL8366RB_SGCR_MAX_LENGTH(0x3) The first one might be the sum you did. I don't know what 1536 and 1552 are for. However, if those cases increase the MTU as well, the code will handle it. During my tests, changing those similar values or disabling jumbo frames wasn't enough to change the switch behavior. As "rtl8365mb family" can control frame size byte by byte, I believe it ignores the old jumbo registers. The 1522 size is already in use by other drivers. If there is something that requires more room without increasing the MTU, like QinQ, we would need to add that extra length to the rtl8365mb_port_change_mtu formula and not the initial value. If not, the switch will have different frame limits when the user leaves the default 1500 MTU or when it changes and reverts the MTU size. Regards,
On Wed, 8 Mar 2023 14:10:59 -0300 Luiz Angelo Daros de Luca wrote: > > 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. > > That value, as mentioned in the commit message, probably came from > rtl8366rb driver jumbo frame settings. > The "rtl8366rb family" has 4 levels of jumbo frame size: > > #define RTL8366RB_SGCR_MAX_LENGTH_1522 RTL8366RB_SGCR_MAX_LENGTH(0x0) > #define RTL8366RB_SGCR_MAX_LENGTH_1536 RTL8366RB_SGCR_MAX_LENGTH(0x1) > #define RTL8366RB_SGCR_MAX_LENGTH_1552 RTL8366RB_SGCR_MAX_LENGTH(0x2) > #define RTL8366RB_SGCR_MAX_LENGTH_16000 RTL8366RB_SGCR_MAX_LENGTH(0x3) > > The first one might be the sum you did. I don't know what 1536 and > 1552 are for. However, if those cases increase the MTU as well, the > code will handle it. > During my tests, changing those similar values or disabling jumbo > frames wasn't enough to change the switch behavior. As "rtl8365mb > family" can control frame size byte by byte, I believe it ignores the > old jumbo registers. > > The 1522 size is already in use by other drivers. If there is > something that requires more room without increasing the MTU, like > QinQ, we would need to add that extra length to the > rtl8365mb_port_change_mtu formula and not the initial value. If not, > the switch will have different frame limits when the user leaves the > default 1500 MTU or when it changes and reverts the MTU size. Could I trouble you for v5 with some form of this explanation in the commit message?
On Wed, Mar 08, 2023 at 10:45:29PM -0800, Jakub Kicinski wrote: > On Wed, 8 Mar 2023 14:10:59 -0300 Luiz Angelo Daros de Luca wrote: > > > 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. > > > > That value, as mentioned in the commit message, probably came from > > rtl8366rb driver jumbo frame settings. > > The "rtl8366rb family" has 4 levels of jumbo frame size: > > > > #define RTL8366RB_SGCR_MAX_LENGTH_1522 RTL8366RB_SGCR_MAX_LENGTH(0x0) > > #define RTL8366RB_SGCR_MAX_LENGTH_1536 RTL8366RB_SGCR_MAX_LENGTH(0x1) > > #define RTL8366RB_SGCR_MAX_LENGTH_1552 RTL8366RB_SGCR_MAX_LENGTH(0x2) > > #define RTL8366RB_SGCR_MAX_LENGTH_16000 RTL8366RB_SGCR_MAX_LENGTH(0x3) > > > > The first one might be the sum you did. I don't know what 1536 and > > 1552 are for. However, if those cases increase the MTU as well, the > > code will handle it. > > During my tests, changing those similar values or disabling jumbo > > frames wasn't enough to change the switch behavior. As "rtl8365mb > > family" can control frame size byte by byte, I believe it ignores the > > old jumbo registers. > > > > The 1522 size is already in use by other drivers. If there is > > something that requires more room without increasing the MTU, like > > QinQ, we would need to add that extra length to the > > rtl8365mb_port_change_mtu formula and not the initial value. If not, > > the switch will have different frame limits when the user leaves the > > default 1500 MTU or when it changes and reverts the MTU size. > > Could I trouble you for v5 with some form of this explanation in the > commit message? FWIIW, that would address my concern.
> Could I trouble you for v5 with some form of this explanation in the > commit message? Sure, here is a new proposed commit message: The rtl8365mb was using a fixed MTU size of 1536, which was probably inspired by the rtl8366rb's initial frame size. However, unlike that family, the rtl8365mb family can specify the max frame 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. + rather than in fixed steps. 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. + During setup, the driver will use the default 1500-byte MTU of DSA to set + the maximum frame size. The current sum will be + VLAN_ETH_HLEN+1500+ETH_FCS_LEN, which results in 1522 bytes. + Although it is lower than the previous initial value of 1536 bytes, the driver + will increase the frame size for a larger MTU. However, if something + requires more space without increasing the MTU, such as QinQ, + we would need to add the extra length to the rtl8365mb_port_change_mtu() + formula. + 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. Let me know if I'm still not clear or missed some important topic. Regards, Luiz
On Thu, 9 Mar 2023 23:55:46 -0300 Luiz Angelo Daros de Luca wrote:
> Let me know if I'm still not clear or missed some important topic.
LGTM, thanks!
On Thu, Mar 09, 2023 at 10:29:56PM -0800, Jakub Kicinski wrote: > On Thu, 9 Mar 2023 23:55:46 -0300 Luiz Angelo Daros de Luca wrote: > > Let me know if I'm still not clear or missed some important topic. > > LGTM, thanks! Thanks, this is crystal clear to me now.
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 @@ -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,7 @@ /* Maximum packet length register */ #define RTL8365MB_CFG0_MAX_LEN_REG 0x088C #define RTL8365MB_CFG0_MAX_LEN_MASK 0x3FFF +#define RTL8365MB_CFG0_MAX_LEN_MAX 0x3FFF /* Port learning limit registers */ #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE 0x0A20 @@ -1135,6 +1137,35 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port, } } +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, + int new_mtu) +{ + struct realtek_priv *priv = ds->priv; + int frame_size; + + /* When a new MTU is set, DSA always sets the CPU port's MTU to the + * 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; + + frame_size = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN; + + dev_dbg(priv->dev, "changing mtu to %d (frame size: %d)\n", + new_mtu, frame_size); + + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, + RTL8365MB_CFG0_MAX_LEN_MASK, + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, + frame_size)); +} + +static int rtl8365mb_port_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 +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); if (ret) goto out_teardown_irq; @@ -2103,6 +2131,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_port_change_mtu, + .port_max_mtu = rtl8365mb_port_max_mtu, }; static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { @@ -2124,6 +2154,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_port_change_mtu, + .port_max_mtu = rtl8365mb_port_max_mtu, }; static const struct realtek_ops rtl8365mb_ops = {