| Message ID | 20201028181221.30419-1-dqfext@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: mt7530: support setting MTU | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Guessed tree name to be net-next |
| jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Thu, Oct 29, 2020 at 02:12:21AM +0800, DENG Qingfang wrote: > MT7530/7531 has a global RX packet length register, which can be used > to set MTU. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Also, please format your patches with --subject-prefix="PATCH net-next" in the future. Jakub installed some patchwork scripts that "guess" the tree based on the commit message, but maybe sometimes they might fail: https://patchwork.ozlabs.org/project/netdev/patch/e5fdcddeda21884a21162e441d1e8a04994f2825.1603837679.git.pavana.sharma@digi.com/ > drivers/net/dsa/mt7530.c | 36 ++++++++++++++++++++++++++++++++++++ > drivers/net/dsa/mt7530.h | 12 ++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index de7692b763d8..7764c66a47c9 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -1021,6 +1021,40 @@ mt7530_port_disable(struct dsa_switch *ds, int port) > mutex_unlock(&priv->reg_mutex); > } > > +static int > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > +{ > + struct mt7530_priv *priv = ds->priv; > + int length; > + > + /* When a new MTU is set, DSA always set 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. > + */ Good point, please tell that to Linus (cc) - I'm talking about e0b2e0d8e669 ("net: dsa: rtl8366rb: Roof MTU for switch"), > + if (!dsa_is_cpu_port(ds, port)) > + return 0; > + > + /* RX length also includes Ethernet header, MTK tag, and FCS length */ > + length = new_mtu + ETH_HLEN + MTK_HDR_LEN + ETH_FCS_LEN; > + if (length <= 1522) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1522); > + else if (length <= 1536) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1536); > + else if (length <= 1552) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1552); > + else > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_JUMBO_MASK | MAX_RX_PKT_LEN_MASK, > + MAX_RX_JUMBO(DIV_ROUND_UP(length, 1024)) | MAX_RX_PKT_LEN_JUMBO); > + > + return 0; > +} > + > +static int > +mt7530_port_max_mtu(struct dsa_switch *ds, int port) > +{ > + return MT7530_MAX_MTU; > +} > + > static void > mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) > { > @@ -2519,6 +2553,8 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > .get_sset_count = mt7530_get_sset_count, > .port_enable = mt7530_port_enable, > .port_disable = mt7530_port_disable, > + .port_change_mtu = mt7530_port_change_mtu, > + .port_max_mtu = mt7530_port_max_mtu, > .port_stp_state_set = mt7530_stp_state_set, > .port_bridge_join = mt7530_port_bridge_join, > .port_bridge_leave = mt7530_port_bridge_leave,
On 10/28/2020 11:12 AM, DENG Qingfang wrote: > MT7530/7531 has a global RX packet length register, which can be used > to set MTU. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Thu, Oct 29, 2020 at 2:31 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 02:12:21AM +0800, DENG Qingfang wrote: > > MT7530/7531 has a global RX packet length register, which can be used > > to set MTU. > > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > Also, please format your patches with --subject-prefix="PATCH net-next" > in the future. Jakub installed some patchwork scripts that "guess" the > tree based on the commit message, but maybe sometimes they might fail: > > https://patchwork.ozlabs.org/project/netdev/patch/e5fdcddeda21884a21162e441d1e8a04994f2825.1603837679.git.pavana.sharma@digi.com/ > > > drivers/net/dsa/mt7530.c | 36 ++++++++++++++++++++++++++++++++++++ > > drivers/net/dsa/mt7530.h | 12 ++++++++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index de7692b763d8..7764c66a47c9 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -1021,6 +1021,40 @@ mt7530_port_disable(struct dsa_switch *ds, int port) > > mutex_unlock(&priv->reg_mutex); > > } > > > > +static int > > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > > +{ > > + struct mt7530_priv *priv = ds->priv; > > + int length; > > + > > + /* When a new MTU is set, DSA always set 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. > > + */ > > Good point, please tell that to Linus (cc) - I'm talking about > e0b2e0d8e669 ("net: dsa: rtl8366rb: Roof MTU for switch"), And 6ae5834b983a ("net: dsa: b53: add MTU configuration support"), 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU"), f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks") CC'd them as well. Also, the commit e0b2e0d8e669 states that the new_mtu parameter is L2 frame length instead of L2 payload. But according to my tests, it is L2 payload (i.e. the same as the MTU shown in `ip link` or `ifconfig`. Is that right? > > > + if (!dsa_is_cpu_port(ds, port)) > > + return 0; > > + > > + /* RX length also includes Ethernet header, MTK tag, and FCS length */ > > + length = new_mtu + ETH_HLEN + MTK_HDR_LEN + ETH_FCS_LEN; > > + if (length <= 1522) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1522); > > + else if (length <= 1536) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1536); > > + else if (length <= 1552) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1552); > > + else > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_JUMBO_MASK | MAX_RX_PKT_LEN_MASK, > > + MAX_RX_JUMBO(DIV_ROUND_UP(length, 1024)) | MAX_RX_PKT_LEN_JUMBO); > > + > > + return 0; > > +} > > + > > +static int > > +mt7530_port_max_mtu(struct dsa_switch *ds, int port) > > +{ > > + return MT7530_MAX_MTU; > > +} > > + > > static void > > mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) > > { > > @@ -2519,6 +2553,8 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > > .get_sset_count = mt7530_get_sset_count, > > .port_enable = mt7530_port_enable, > > .port_disable = mt7530_port_disable, > > + .port_change_mtu = mt7530_port_change_mtu, > > + .port_max_mtu = mt7530_port_max_mtu, > > .port_stp_state_set = mt7530_stp_state_set, > > .port_bridge_join = mt7530_port_bridge_join, > > .port_bridge_leave = mt7530_port_bridge_leave,
On Thu, Oct 29, 2020 at 11:32:36AM +0800, DENG Qingfang wrote: > On Thu, Oct 29, 2020 at 2:31 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > On Thu, Oct 29, 2020 at 02:12:21AM +0800, DENG Qingfang wrote: ... > > > +static int > > > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > > > +{ > > > + struct mt7530_priv *priv = ds->priv; > > > + int length; > > > + > > > + /* When a new MTU is set, DSA always set 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. > > > + */ > > > > Good point, please tell that to Linus (cc) - I'm talking about > > e0b2e0d8e669 ("net: dsa: rtl8366rb: Roof MTU for switch"), > > And 6ae5834b983a ("net: dsa: b53: add MTU configuration support"), > 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU"), > f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks") > > CC'd them as well. qca8k tracks and use the maximum provided mtu; perhaps that could be optimised by only allow the CPU port to be set but it feels a bit more future proof (e.g. if/when we support multiple CPU ports). > Also, the commit e0b2e0d8e669 states that the new_mtu parameter is L2 > frame length instead of L2 payload. But according to my tests, it is > L2 payload (i.e. the same as the MTU shown in `ip link` or `ifconfig`. > Is that right? Certainly that's what I saw; qca8k sets the MTU to the provided MTU + ETH_HLEN + ETH_FCS_LEN. J.
On Thu, Oct 29, 2020 at 4:11 PM Jonathan McDowell <noodles@earth.li> wrote: > > On Thu, Oct 29, 2020 at 11:32:36AM +0800, DENG Qingfang wrote: > > On Thu, Oct 29, 2020 at 2:31 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > > > On Thu, Oct 29, 2020 at 02:12:21AM +0800, DENG Qingfang wrote: > ... > > > > +static int > > > > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > > > > +{ > > > > + struct mt7530_priv *priv = ds->priv; > > > > + int length; > > > > + > > > > + /* When a new MTU is set, DSA always set 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. > > > > + */ > > > > > > Good point, please tell that to Linus (cc) - I'm talking about > > > e0b2e0d8e669 ("net: dsa: rtl8366rb: Roof MTU for switch"), > > > > And 6ae5834b983a ("net: dsa: b53: add MTU configuration support"), > > 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU"), > > f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks") > > > > CC'd them as well. > > qca8k tracks and use the maximum provided mtu; perhaps that could be > optimised by only allow the CPU port to be set but it feels a bit more > future proof (e.g. if/when we support multiple CPU ports). btw, there is a bug in your commit f58d2598cf70, in qca8k_port_change_mtu the loop variable is not used inside the for loop. > > > Also, the commit e0b2e0d8e669 states that the new_mtu parameter is L2 > > frame length instead of L2 payload. But according to my tests, it is > > L2 payload (i.e. the same as the MTU shown in `ip link` or `ifconfig`. > > Is that right? > > Certainly that's what I saw; qca8k sets the MTU to the provided MTU + > ETH_HLEN + ETH_FCS_LEN. > > J. > > -- > Pretty please, with sugar on top, clean the f**king car. > This .sig brought to you by the letter J and the number 13 > Product of the Republic of HuggieTag
On Thu, 29 Oct 2020 02:12:21 +0800 DENG Qingfang wrote: > MT7530/7531 has a global RX packet length register, which can be used > to set MTU. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> Please wrap your code at 80 chars. > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index de7692b763d8..7764c66a47c9 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -1021,6 +1021,40 @@ mt7530_port_disable(struct dsa_switch *ds, int port) > mutex_unlock(&priv->reg_mutex); > } > > +static int > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > +{ > + struct mt7530_priv *priv = ds->priv; > + int length; > + > + /* When a new MTU is set, DSA always set 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; > + > + /* RX length also includes Ethernet header, MTK tag, and FCS length */ > + length = new_mtu + ETH_HLEN + MTK_HDR_LEN + ETH_FCS_LEN; > + if (length <= 1522) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1522); > + else if (length <= 1536) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1536); > + else if (length <= 1552) > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1552); > + else > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_JUMBO_MASK | MAX_RX_PKT_LEN_MASK, > + MAX_RX_JUMBO(DIV_ROUND_UP(length, 1024)) | MAX_RX_PKT_LEN_JUMBO); this line should start under priv, so it aligns to the opening parenthesis. Besides, don't you need to reset the JUMBO bit when going from jumbo to non-jumbo? The mask should always include jumbo. I assume you're duplicating the mt7530_rmw() for the benefit of the constant validation, but it seems to be counterproductive here. > + return 0; > +}
On Fri, Oct 30, 2020 at 6:42 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 29 Oct 2020 02:12:21 +0800 DENG Qingfang wrote: > > MT7530/7531 has a global RX packet length register, which can be used > > to set MTU. > > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > Please wrap your code at 80 chars. > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index de7692b763d8..7764c66a47c9 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -1021,6 +1021,40 @@ mt7530_port_disable(struct dsa_switch *ds, int port) > > mutex_unlock(&priv->reg_mutex); > > } > > > > +static int > > +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > > +{ > > + struct mt7530_priv *priv = ds->priv; > > + int length; > > + > > + /* When a new MTU is set, DSA always set 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; > > + > > + /* RX length also includes Ethernet header, MTK tag, and FCS length */ > > + length = new_mtu + ETH_HLEN + MTK_HDR_LEN + ETH_FCS_LEN; > > + if (length <= 1522) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1522); > > + else if (length <= 1536) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1536); > > + else if (length <= 1552) > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1552); > > + else > > + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_JUMBO_MASK | MAX_RX_PKT_LEN_MASK, > > + MAX_RX_JUMBO(DIV_ROUND_UP(length, 1024)) | MAX_RX_PKT_LEN_JUMBO); > > this line should start under priv, so it aligns to the opening > parenthesis. > > Besides, don't you need to reset the JUMBO bit when going from jumbo to > non-jumbo? The mask should always include jumbo. MAX_RX_JUMBO works only when MAX_RX_PKT_LEN is set to 0x3, so just changing MAX_RX_PKT_LEN to non-jumbo is enough. FYI, the default value of MAX_RX_JUMBO is 0x9. > > I assume you're duplicating the mt7530_rmw() for the benefit of the > constant validation, but it seems to be counterproductive here. As I mentioned above, MAX_RX_JUMBO does not need to be changed when going from jumbo to non-jumbo. Perhaps I should use mt7530_mii_read() and mt7530_mii_write() instead? > > > + return 0; > > +} >
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index de7692b763d8..7764c66a47c9 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -1021,6 +1021,40 @@ mt7530_port_disable(struct dsa_switch *ds, int port) mutex_unlock(&priv->reg_mutex); } +static int +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) +{ + struct mt7530_priv *priv = ds->priv; + int length; + + /* When a new MTU is set, DSA always set 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; + + /* RX length also includes Ethernet header, MTK tag, and FCS length */ + length = new_mtu + ETH_HLEN + MTK_HDR_LEN + ETH_FCS_LEN; + if (length <= 1522) + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1522); + else if (length <= 1536) + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1536); + else if (length <= 1552) + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_PKT_LEN_MASK, MAX_RX_PKT_LEN_1552); + else + mt7530_rmw(priv, MT7530_GMACCR, MAX_RX_JUMBO_MASK | MAX_RX_PKT_LEN_MASK, + MAX_RX_JUMBO(DIV_ROUND_UP(length, 1024)) | MAX_RX_PKT_LEN_JUMBO); + + return 0; +} + +static int +mt7530_port_max_mtu(struct dsa_switch *ds, int port) +{ + return MT7530_MAX_MTU; +} + static void mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) { @@ -2519,6 +2553,8 @@ static const struct dsa_switch_ops mt7530_switch_ops = { .get_sset_count = mt7530_get_sset_count, .port_enable = mt7530_port_enable, .port_disable = mt7530_port_disable, + .port_change_mtu = mt7530_port_change_mtu, + .port_max_mtu = mt7530_port_max_mtu, .port_stp_state_set = mt7530_stp_state_set, .port_bridge_join = mt7530_port_bridge_join, .port_bridge_leave = mt7530_port_bridge_leave, diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index 9278a8e3d04e..77a6222d2635 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -11,6 +11,9 @@ #define MT7530_NUM_FDB_RECORDS 2048 #define MT7530_ALL_MEMBERS 0xff +#define MTK_HDR_LEN 4 +#define MT7530_MAX_MTU (15 * 1024 - ETH_HLEN - ETH_FCS_LEN - MTK_HDR_LEN) + enum mt753x_id { ID_MT7530 = 0, ID_MT7621 = 1, @@ -289,6 +292,15 @@ enum mt7530_vlan_port_attr { #define MT7531_DBG_CNT(x) (0x3018 + (x) * 0x100) #define MT7531_DIS_CLR BIT(31) +#define MT7530_GMACCR 0x30e0 +#define MAX_RX_JUMBO(x) ((x) << 2) +#define MAX_RX_JUMBO_MASK GENMASK(5, 2) +#define MAX_RX_PKT_LEN_MASK GENMASK(1, 0) +#define MAX_RX_PKT_LEN_1522 0x0 +#define MAX_RX_PKT_LEN_1536 0x1 +#define MAX_RX_PKT_LEN_1552 0x2 +#define MAX_RX_PKT_LEN_JUMBO 0x3 + /* Register for MIB */ #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) #define MT7530_MIB_CCR 0x4fe0
MT7530/7531 has a global RX packet length register, which can be used to set MTU. Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- drivers/net/dsa/mt7530.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/net/dsa/mt7530.h | 12 ++++++++++++ 2 files changed, 48 insertions(+)