| Message ID | 20230106101651.1137755-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4,1/3] dsa: marvell: Provide per device information about max frame size | expand |
On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. This value corresponds to the memory allocated > in switch to store single frame. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 bytes. > To be more interresting - devices supporting jumbo frames - use yet > another value (10240 bytes) > > As this value is internal and may be different for each switch IC, > new entry in struct mv88e6xxx_info has been added to store it. > > This commit doesn't change the code functionality - it just provides > the max frame size value explicitly - up till now it has been > assigned depending on the callback provided by the IC driver > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > Signed-off-by: Lukasz Majewski <lukma@denx.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> FYI: It is normal to include a patch 0/X for a patchset, which explains the big picture of the patchset. Please try to remember this for your next patchset. Andrew
On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. This value corresponds to the memory allocated > in switch to store single frame. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 bytes. > To be more interresting - devices supporting jumbo frames - use yet > another value (10240 bytes) > > As this value is internal and may be different for each switch IC, > new entry in struct mv88e6xxx_info has been added to store it. > > This commit doesn't change the code functionality - it just provides > the max frame size value explicitly - up till now it has been > assigned depending on the callback provided by the IC driver > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > Changes for v2: > - Define max_frame_size with default value of 1632 bytes, > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > > Changes for v3: > - Add default value for 1632B of the max frame size (to avoid problems > with not defined values) > > Changes for v4: > - Rework the mv88e6xxx_get_max_mtu() by using per device defined > max_frame_size value > > - Add WARN_ON_ONCE() when max_frame_size is not defined > > - Add description for the new 'max_frame_size' member of mv88e6xxx_info > --- > drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++++---- > drivers/net/dsa/mv88e6xxx/chip.h | 6 +++++ > 2 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 242b8b325504..fc6d98c4a029 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -3545,11 +3545,10 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) > { > struct mv88e6xxx_chip *chip = ds->priv; > > - if (chip->info->ops->port_set_jumbo_size) > - return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > - else if (chip->info->ops->set_max_frame_size) > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > - return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > + WARN_ON_ONCE(!chip->info->max_frame_size); > + > + return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN > + - ETH_FCS_LEN; VLAN_ETH_HLEN (18) + EDSA_HLEN (8) + ETH_FCS_LEN (4) = 30 > } > > static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > @@ -4955,6 +4954,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = { > .avb_ops = &mv88e6352_avb_ops, > .ptp_ops = &mv88e6250_ptp_ops, > .phylink_get_caps = mv88e6250_phylink_get_caps, > + .set_max_frame_size = mv88e6185_g1_set_max_frame_size, > }; > > static const struct mv88e6xxx_ops mv88e6290_ops = { > @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { > .num_internal_phys = 5, > .max_vid = 4095, > .max_sid = 63, > + .max_frame_size = 1522, 1522 - 30 = 1492. I don't believe that there are switches which don't support the standard MTU of 1500 ?! > .port_base_addr = 0x10, > .phy_base_addr = 0x0, > .global1_addr = 0x1b, Note that I see this behavior isn't new. But I've simulated it, and it will produce the following messages on probe: [ 7.425752] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.437516] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 0 [ 7.588585] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.600433] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 1 [ 7.752613] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.764457] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 2 [ 7.900771] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.912501] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 3 I wonder, shouldn't we first fix that, and apply this patch set afterwards?
Hi Andrew, > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. This value corresponds to the memory allocated > > in switch to store single frame. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. To be more interresting - devices supporting jumbo > > frames - use yet another value (10240 bytes) > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > > > This commit doesn't change the code functionality - it just provides > > the max frame size value explicitly - up till now it has been > > assigned depending on the callback provided by the IC driver > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > FYI: It is normal to include a patch 0/X for a patchset, which > explains the big picture of the patchset. Please try to remember this > for your next patchset. Ok. I will. Thanks for the tip. > > Andrew > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Andrew, Vladimir, > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. This value corresponds to the memory allocated > > in switch to store single frame. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. To be more interresting - devices supporting jumbo > > frames - use yet another value (10240 bytes) > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > > > This commit doesn't change the code functionality - it just provides > > the max frame size value explicitly - up till now it has been > > assigned depending on the callback provided by the IC driver > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > FYI: It is normal to include a patch 0/X for a patchset, which > explains the big picture of the patchset. Please try to remember this > for your next patchset. > > Andrew > > Are there any more comments, or is this patch set eligible for pulling into net-next tree? Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote: > Are there any more comments, or is this patch set eligible for pulling > into net-next tree? How about responding to the comment that was already posted first?
Hi Vladimir, > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote: > > Are there any more comments, or is this patch set eligible for > > pulling into net-next tree? > > How about responding to the comment that was already posted first? Could you be more specific? On the beginning (first posted version) the patch included 9 patches (which included work for ADDR4 for some mv88e6020 setup). But after the discussion, I've decided to split this patch set to smaller pieces; First to add the set_max_frame size with basic definition for mv88e6020 and mv88e6071 and then follow with more complicated changes (for which there is no agreement on how to tackle them). For the 'set_max_frame' feature Alexander Dyuck had some comments regarding defensive programming approach, but finally he agreed with Andrew's approach. As of now - the v4 has been Acked by Andrew, so it looks like at least this "part" of the work is eligible for upstreaming. Or there are any more issues about which I've forgotten ? Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote: > Hi Vladimir, > > > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote: > > > Are there any more comments, or is this patch set eligible for > > > pulling into net-next tree? > > > > How about responding to the comment that was already posted first? > > Could you be more specific? > > > On the beginning (first posted version) the patch included 9 patches > (which included work for ADDR4 for some mv88e6020 setup). > > But after the discussion, I've decided to split this patch set to > smaller pieces; > > First to add the set_max_frame size with basic definition for mv88e6020 > and mv88e6071 and then follow with more complicated changes (for which > there is no agreement on how to tackle them). > > For the 'set_max_frame' feature Alexander Dyuck had some comments > regarding defensive programming approach, but finally he agreed with > Andrew's approach. > > As of now - the v4 has been Acked by Andrew, so it looks like at least > this "part" of the work is eligible for upstreaming. > > > Or there are any more issues about which I've forgotten ? Do you agree that for the chip families which neither implement port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will be returned, which currently produces warnings at probe time and should be fixed first, prior to refactoring the code? https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/#25149891
Hi Vladimir, > On Fri, Jan 13, 2023 at 12:02:19PM +0100, Lukasz Majewski wrote: > > Hi Vladimir, > > > > > On Fri, Jan 13, 2023 at 11:39:08AM +0100, Lukasz Majewski wrote: > > > > Are there any more comments, or is this patch set eligible for > > > > pulling into net-next tree? > > > > > > How about responding to the comment that was already posted > > > first? > > > > Could you be more specific? > > > > > > On the beginning (first posted version) the patch included 9 patches > > (which included work for ADDR4 for some mv88e6020 setup). > > > > But after the discussion, I've decided to split this patch set to > > smaller pieces; > > > > First to add the set_max_frame size with basic definition for > > mv88e6020 and mv88e6071 and then follow with more complicated > > changes (for which there is no agreement on how to tackle them). > > > > For the 'set_max_frame' feature Alexander Dyuck had some comments > > regarding defensive programming approach, but finally he agreed with > > Andrew's approach. > > > > As of now - the v4 has been Acked by Andrew, so it looks like at > > least this "part" of the work is eligible for upstreaming. > > > > > > Or there are any more issues about which I've forgotten ? > > Do you agree that for the chip families which neither implement > port_set_jumbo_size() nor set_max_frame_size(), a max MTU of 1492 will > be returned, which currently produces warnings at probe time and > should be fixed first, prior to refactoring the code? > https://patchwork.kernel.org/project/netdevbpf/patch/20230106101651.1137755-1-lukma@denx.de/#25149891 Sorry, but I've overlooked your reply. I will write my comments as a reply to it. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Vladimir, > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. This value corresponds to the memory allocated > > in switch to store single frame. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. To be more interresting - devices supporting jumbo > > frames - use yet another value (10240 bytes) > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > > > This commit doesn't change the code functionality - it just provides > > the max frame size value explicitly - up till now it has been > > assigned depending on the callback provided by the IC driver > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > --- > > Changes for v2: > > - Define max_frame_size with default value of 1632 bytes, > > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > > > > Changes for v3: > > - Add default value for 1632B of the max frame size (to avoid > > problems with not defined values) > > > > Changes for v4: > > - Rework the mv88e6xxx_get_max_mtu() by using per device defined > > max_frame_size value > > > > - Add WARN_ON_ONCE() when max_frame_size is not defined > > > > - Add description for the new 'max_frame_size' member of > > mv88e6xxx_info --- > > drivers/net/dsa/mv88e6xxx/chip.c | 41 > > ++++++++++++++++++++++++++++---- drivers/net/dsa/mv88e6xxx/chip.h | > > 6 +++++ 2 files changed, 42 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > b/drivers/net/dsa/mv88e6xxx/chip.c index 242b8b325504..fc6d98c4a029 > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > @@ -3545,11 +3545,10 @@ static int mv88e6xxx_get_max_mtu(struct > > dsa_switch *ds, int port) { > > struct mv88e6xxx_chip *chip = ds->priv; > > > > - if (chip->info->ops->port_set_jumbo_size) > > - return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - > > ETH_FCS_LEN; > > - else if (chip->info->ops->set_max_frame_size) > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > ETH_FCS_LEN; > > - return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > + WARN_ON_ONCE(!chip->info->max_frame_size); > > + > > + return chip->info->max_frame_size - VLAN_ETH_HLEN - > > EDSA_HLEN > > + - ETH_FCS_LEN; > > VLAN_ETH_HLEN (18) + EDSA_HLEN (8) + ETH_FCS_LEN (4) = 30 > > > } > > > > static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, > > int new_mtu) @@ -4955,6 +4954,7 @@ static const struct > > mv88e6xxx_ops mv88e6250_ops = { .avb_ops = &mv88e6352_avb_ops, > > .ptp_ops = &mv88e6250_ptp_ops, > > .phylink_get_caps = mv88e6250_phylink_get_caps, > > + .set_max_frame_size = mv88e6185_g1_set_max_frame_size, > > }; > > > > static const struct mv88e6xxx_ops mv88e6290_ops = { > > @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info > > mv88e6xxx_table[] = { .num_internal_phys = 5, > > .max_vid = 4095, > > .max_sid = 63, > > + .max_frame_size = 1522, > > 1522 - 30 = 1492. > > I don't believe that there are switches which don't support the > standard MTU of 1500 ?! I think that this commit [1], made the adjustment to fix yet another issue. It looks like the missing 8 bytes are added in the mv88e6xxx_change_mtu() function. > > > .port_base_addr = 0x10, > > .phy_base_addr = 0x0, > > .global1_addr = 0x1b, > > Note that I see this behavior isn't new. But I've simulated it, and it > will produce the following messages on probe: > > [ 7.425752] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY > [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ > 7.437516] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to > 1500 on port 0 [ 7.588585] mscc_felix 0000:00:00.5 swp1 > (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 > SyncE] (irq=POLL) [ 7.600433] mscc_felix 0000:00:00.5: nonfatal > error -34 setting MTU to 1500 on port 1 [ 7.752613] mscc_felix > 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver > [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.764457] mscc_felix > 0000:00:00.5: nonfatal error -34 setting MTU to 1500 on port 2 [ > 7.900771] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY > [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ > 7.912501] mscc_felix 0000:00:00.5: nonfatal error -34 setting MTU to > 1500 on port 3 > > I wonder, shouldn't we first fix that, and apply this patch set > afterwards? IMHO, it is up to Andrew to decide how to proceed, as the aforementioned patch [1] is an attempt to fix yet another issue [2]. Links: [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6 [2] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482 Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote: > I think that this commit [1], made the adjustment to fix yet another > issue. > [1] - > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6 It appears that this is the commit to blame, indeed. > It looks like the missing 8 bytes are added in the > mv88e6xxx_change_mtu() function. Only for DSA and CPU ports. The driver still behaves as if the max MTU on user ports is 1492 bytes. > > I wonder, shouldn't we first fix that, and apply this patch set > > afterwards? > > IMHO, it is up to Andrew to decide how to proceed, as the > aforementioned patch [1] is an attempt to fix yet another issue [2]. > [2] - > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482 I think the handling for those switches were neither port_set_jumbo_size() nor set_max_frame_size() is present is just a roundabout way of saying "hey, I only support ETH_DATA_LEN MTU and can't change it, leave me alone". But it isn't what the code does.
Hi Vladimir, > On Fri, Jan 13, 2023 at 01:13:31PM +0100, Lukasz Majewski wrote: > > I think that this commit [1], made the adjustment to fix yet another > > issue. > > [1] - > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9c587fed61cf88bd45822c3159644445f6d5aa6 > > > > It appears that this is the commit to blame, indeed. > > > It looks like the missing 8 bytes are added in the > > mv88e6xxx_change_mtu() function. > > Only for DSA and CPU ports. The driver still behaves as if the max MTU > on user ports is 1492 bytes. > It looks so... > > > I wonder, shouldn't we first fix that, and apply this patch set > > > afterwards? > > > > IMHO, it is up to Andrew to decide how to proceed, as the > > aforementioned patch [1] is an attempt to fix yet another issue [2]. > > [2] - > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1baf0fac10fbe3084975d7cb0a4378eb18871482 > > > > I think the handling for those switches were neither > port_set_jumbo_size() nor set_max_frame_size() is present is just a > roundabout way of saying "hey, I only support ETH_DATA_LEN MTU and > can't change it, leave me alone". But it isn't what the code does. I tend to agree... The number of switched which suppor 1522 B max frame is only six. This may be why the problem was not noticed. The fixed function maybe should look like below: static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) { .... int max_mtu; max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN - ETH_FCS_LE; if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)) max_mtu -= EDSA_HLEN; return max_mtu; } Comments more than welcome. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> I tend to agree... The number of switched which suppor 1522 B max frame > is only six. This may be why the problem was not noticed. > > The fixed function maybe should look like below: > > > static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) > { > .... > > > int max_mtu; > > max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN - > ETH_FCS_LE; > > if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)) > max_mtu -= EDSA_HLEN; > > return max_mtu; > } > > Comments more than welcome. I would suggest some comments are added, explaining what is going on here. Given the number of Fixes: tags in this area, it is clearly tricky to get right, given how different switches operate. I've not looked back to the email archive, but i have a vague recollection that it could be some switches don't impose the MTU limit on CPU and DSA ports, just the user ports. So the value reported for those ports can actually be bigger than then the max mtu, in order to accommodate the DSA header. Andrew
On Fri, Jan 13, 2023 at 02:20:17PM +0100, Lukasz Majewski wrote: > The fixed function maybe should look like below: > > static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) > { > .... > > int max_mtu; > > max_mtu = chip->info->max_frame_size - VLAN_ETH_HLEN - > ETH_FCS_LE; > > if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)) > max_mtu -= EDSA_HLEN; > > return max_mtu; > } > > Comments more than welcome. I suspect that looking at the DSA code which calls these methods will answer a lot of your questions. ds->ops->port_max_mtu() is only called for user ports. As for ds->ops->port_change_mtu(), this will always be called with the requested L2 payload length (default 1500) on user ports, and with the maximum among user ports for DSA and CPU ports.
On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. This value corresponds to the memory allocated > in switch to store single frame. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 bytes. > To be more interresting - devices supporting jumbo frames - use yet > another value (10240 bytes) > > As this value is internal and may be different for each switch IC, > new entry in struct mv88e6xxx_info has been added to store it. > > This commit doesn't change the code functionality - it just provides > the max frame size value explicitly - up till now it has been > assigned depending on the callback provided by the IC driver > (e.g. .set_max_frame_size, .port_set_jumbo_size). I don't think this patch is correct. One of the things that mv88e6xxx_setup_port() does when initialising each port is: if (chip->info->ops->port_set_jumbo_size) { err = chip->info->ops->port_set_jumbo_size(chip, port, 10218); if (err) return err; } There is one implementation of this, which is mv88e6165_port_set_jumbo_size() and that has the effect of setting port register 8 to the largest size. So any chip that supports the port_set_jumbo_size() method will be programmed on initialisation to support this larger size. However, you seem to be listing e.g. the 88e6190 (if I'm interpreting the horrid mv88e6xxx_table changes correctly) as having a maximum frame size of 1522, but it implements this method, supports 10240, and thus is programmed to support frames of that size rather than 1522.
Hi Russell, > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. This value corresponds to the memory allocated > > in switch to store single frame. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. To be more interresting - devices supporting jumbo > > frames - use yet another value (10240 bytes) > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > > > This commit doesn't change the code functionality - it just provides > > the max frame size value explicitly - up till now it has been > > assigned depending on the callback provided by the IC driver > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > I don't think this patch is correct. > > One of the things that mv88e6xxx_setup_port() does when initialising > each port is: > > if (chip->info->ops->port_set_jumbo_size) { > err = chip->info->ops->port_set_jumbo_size(chip, > port, 10218); if (err) > return err; > } > > There is one implementation of this, which is > mv88e6165_port_set_jumbo_size() and that has the effect of setting > port register 8 to the largest size. So any chip that supports the > port_set_jumbo_size() method will be programmed on initialisation to > support this larger size. > > However, you seem to be listing e.g. the 88e6190 (if I'm interpreting > the horrid mv88e6xxx_table changes correctly) Those changes were requested by the community. Previous versions of this patch were just changing things to allow correct operation of the switch ICs on which I do work (i.e. 88e6020 and 88e6071). And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the same value was not updated for 88e6190X. The question is - how shall I proceed? After the discussion about this code - it looks like approach from v3 [1] seems to be the most non-intrusive for other ICs. > as having a maximum > frame size of 1522, but it implements this method, supports 10240, and > thus is programmed to support frames of that size rather than 1522. > Links: [1] - https://lore.kernel.org/netdev/Y7M+mWMU+DJPYubp@lunn.ch/T/ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi, > Hi Russell, > > > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > > Different Marvell DSA switches support different size of max frame > > > bytes to be sent. This value corresponds to the memory allocated > > > in switch to store single frame. > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > in-driver standard value. On the other hand - mv88e6250 supports > > > 2048 bytes. To be more interresting - devices supporting jumbo > > > frames - use yet another value (10240 bytes) > > > > > > As this value is internal and may be different for each switch IC, > > > new entry in struct mv88e6xxx_info has been added to store it. > > > > > > This commit doesn't change the code functionality - it just > > > provides the max frame size value explicitly - up till now it has > > > been assigned depending on the callback provided by the IC driver > > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > I don't think this patch is correct. > > > > One of the things that mv88e6xxx_setup_port() does when initialising > > each port is: > > > > if (chip->info->ops->port_set_jumbo_size) { > > err = chip->info->ops->port_set_jumbo_size(chip, > > port, 10218); if (err) > > return err; > > } > > > > There is one implementation of this, which is > > mv88e6165_port_set_jumbo_size() and that has the effect of setting > > port register 8 to the largest size. So any chip that supports the > > port_set_jumbo_size() method will be programmed on initialisation to > > support this larger size. > > > > However, you seem to be listing e.g. the 88e6190 (if I'm > > interpreting the horrid mv88e6xxx_table changes correctly) > > Those changes were requested by the community. Previous versions of > this patch were just changing things to allow correct operation of the > switch ICs on which I do work (i.e. 88e6020 and 88e6071). > > And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the > same value was not updated for 88e6190X. > > The question is - how shall I proceed? > > After the discussion about this code - it looks like approach from v3 > [1] seems to be the most non-intrusive for other ICs. > I would appreciate _any_ hints on how shall I proceed to prepare those patches, so the community will accept them... Thanks in advance. > > as having a maximum > > frame size of 1522, but it implements this method, supports 10240, > > and thus is programmed to support frames of that size rather than > > 1522. > > Links: > > [1] - https://lore.kernel.org/netdev/Y7M+mWMU+DJPYubp@lunn.ch/T/ > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote: > Hi, > > > Hi Russell, > > > > > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski wrote: > > > > Different Marvell DSA switches support different size of max frame > > > > bytes to be sent. This value corresponds to the memory allocated > > > > in switch to store single frame. > > > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > > in-driver standard value. On the other hand - mv88e6250 supports > > > > 2048 bytes. To be more interresting - devices supporting jumbo > > > > frames - use yet another value (10240 bytes) > > > > > > > > As this value is internal and may be different for each switch IC, > > > > new entry in struct mv88e6xxx_info has been added to store it. > > > > > > > > This commit doesn't change the code functionality - it just > > > > provides the max frame size value explicitly - up till now it has > > > > been assigned depending on the callback provided by the IC driver > > > > (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > > > I don't think this patch is correct. > > > > > > One of the things that mv88e6xxx_setup_port() does when initialising > > > each port is: > > > > > > if (chip->info->ops->port_set_jumbo_size) { > > > err = chip->info->ops->port_set_jumbo_size(chip, > > > port, 10218); if (err) > > > return err; > > > } > > > > > > There is one implementation of this, which is > > > mv88e6165_port_set_jumbo_size() and that has the effect of setting > > > port register 8 to the largest size. So any chip that supports the > > > port_set_jumbo_size() method will be programmed on initialisation to > > > support this larger size. > > > > > > However, you seem to be listing e.g. the 88e6190 (if I'm > > > interpreting the horrid mv88e6xxx_table changes correctly) > > > > Those changes were requested by the community. Previous versions of > > this patch were just changing things to allow correct operation of the > > switch ICs on which I do work (i.e. 88e6020 and 88e6071). > > > > And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) the > > same value was not updated for 88e6190X. > > > > The question is - how shall I proceed? > > > > After the discussion about this code - it looks like approach from v3 > > [1] seems to be the most non-intrusive for other ICs. > > > > I would appreciate _any_ hints on how shall I proceed to prepare those > patches, so the community will accept them... What I'm concerned about, and why I replied, is that setting the devices to have a max frame size of 1522 when we program them to use a larger frame size means we break those switches for normal sized packets. The current logic in mv88e6xxx_get_max_mtu() is: If the chip implements port_set_jumbo_size, then packet sizes of up to 10240 are supported. (ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240, 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x) If the chip implements set_max_frame_size, then packet sizes of up to 1632 are supported. (ops: 6085, 6095, 6097, 6123, 6161, 6185) Otherwise, packets of up to 1522 are supported. Now, going through the patch, I see: 88e6085 has 10240 but currently has 1632 88e6095 has 1632 (no change) 88e6097 has 1632 (no change) 88e6123 has 10240 but currently has 1632 88e6131 has 10240 (no change) 88e6141 has 10240 (no change) 88e6161 has 1632 but currently has 10240 88e6165 has 1632 but currently has 1522 88e6171 has 1522 but currently has 10240 88e6172 has 10240 (no change) 88e6175 has 1632 but currently has 10240 88e6176 has 10240 (no change) 88e6185 has 1632 (no change) 88e6190 has 10240 (no change) 88e6190x has 10240 (no change) 88e6191 has 10240 but currently has 1522 88e6191x has 1522 but currently has 10240 88e6193x has 1522 but currently has 10240 88e6220 has 2048 but currently has 1522 88e6240 has 10240 (no change) 88e6250 has 2048 but currently has 1522 88e6290 has 10240 but currently has 1522 88e6320 has 10240 (no change) 88e6321 has 10240 (no change) 88e6341 has 10240 (no change) 88e6350 has 10240 (no change) 88e6351 has 10240 (no change) 88e6352 has 10240 (no change) 88e6390 has 1522 but currently has 10240 88e6390x has 1522 but currently has 10240 88e6393x has 1522 but currently has 10240 My point is that based on the above, there's an awful lot of changes that this one patch brings, and I'm not sure many of them are intended. All the ones with "but currently has 10240", it seems they implement port_set_jumbo_size() which, although the switch may default to a smaller frame size, we configure it to be higher. Maybe these don't implement the field that configures those? Maybe your patch is wrong? I don't know. Similarly for the ones with "but currently has 1632", it seems they implement set_max_frame_size(), but this is only called via mv88e6xxx_change_mtu(), and I haven't worked out whether that will be called during initialisation by the networking layer. Now, what really concerns me is the difficulty in making this change. As we can see from the above, there's a lot of changes going on here, and it's not obvious which are intentional and which may be bugs. So, I think it would be far better to introduce the "max_frame_size" field using the existing values, and then verify that value during initialisation time for every entry in mv88e6xxx_table[] using the rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and have it run that verification, and state that's what's happened and was successful in the commit message. In the next commit, change mv88e6xxx_get_max_mtu() to use those verified values and remove the verification code. Then in the following commit, update the "max_frame_size" values with the changes you intend to make. Then, we can (a) have confidence that each of the new members were properly initialised, and (b) we can also see what changes you're intentionally making. Right now, given that at least two of the "max_frame_size" values are wrong in this patch, I think we can say for certain that we've proven that trying to introduce this new member and use it in a single patch is too error prone. Thanks.
Hi Russell, > On Wed, Jan 25, 2023 at 12:24:12PM +0100, Lukasz Majewski wrote: > > Hi, > > > > > Hi Russell, > > > > > > > On Fri, Jan 06, 2023 at 11:16:49AM +0100, Lukasz Majewski > > > > wrote: > > > > > Different Marvell DSA switches support different size of max > > > > > frame bytes to be sent. This value corresponds to the memory > > > > > allocated in switch to store single frame. > > > > > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > > > in-driver standard value. On the other hand - mv88e6250 > > > > > supports 2048 bytes. To be more interresting - devices > > > > > supporting jumbo frames - use yet another value (10240 bytes) > > > > > > > > > > As this value is internal and may be different for each > > > > > switch IC, new entry in struct mv88e6xxx_info has been added > > > > > to store it. > > > > > > > > > > This commit doesn't change the code functionality - it just > > > > > provides the max frame size value explicitly - up till now it > > > > > has been assigned depending on the callback provided by the > > > > > IC driver (e.g. .set_max_frame_size, .port_set_jumbo_size). > > > > > > > > > > > > > I don't think this patch is correct. > > > > > > > > One of the things that mv88e6xxx_setup_port() does when > > > > initialising each port is: > > > > > > > > if (chip->info->ops->port_set_jumbo_size) { > > > > err = chip->info->ops->port_set_jumbo_size(chip, > > > > port, 10218); if (err) > > > > return err; > > > > } > > > > > > > > There is one implementation of this, which is > > > > mv88e6165_port_set_jumbo_size() and that has the effect of > > > > setting port register 8 to the largest size. So any chip that > > > > supports the port_set_jumbo_size() method will be programmed on > > > > initialisation to support this larger size. > > > > > > > > However, you seem to be listing e.g. the 88e6190 (if I'm > > > > interpreting the horrid mv88e6xxx_table changes correctly) > > > > > > Those changes were requested by the community. Previous versions > > > of this patch were just changing things to allow correct > > > operation of the switch ICs on which I do work (i.e. 88e6020 and > > > 88e6071). > > > > > > And yes, for 88e6190 the max_frame_size = 10240, but (by mistake) > > > the same value was not updated for 88e6190X. > > > > > > The question is - how shall I proceed? > > > > > > After the discussion about this code - it looks like approach > > > from v3 [1] seems to be the most non-intrusive for other ICs. > > > > > > > I would appreciate _any_ hints on how shall I proceed to prepare > > those patches, so the community will accept them... > Thanks for a very detailed reply. > What I'm concerned about, and why I replied, is that setting the > devices to have a max frame size of 1522 when we program them to use > a larger frame size means we break those switches for normal sized > packets. > > The current logic in mv88e6xxx_get_max_mtu() is: > > If the chip implements port_set_jumbo_size, then packet sizes > of up to 10240 are supported. > (ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240, > 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x) > If the chip implements set_max_frame_size, then packet sizes > of up to 1632 are supported. > (ops: 6085, 6095, 6097, 6123, 6161, 6185) > Otherwise, packets of up to 1522 are supported. > > Now, going through the patch, I see: > > 88e6085 has 10240 but currently has 1632 > 88e6095 has 1632 (no change) > 88e6097 has 1632 (no change) > 88e6123 has 10240 but currently has 1632 > 88e6131 has 10240 (no change) > 88e6141 has 10240 (no change) > 88e6161 has 1632 but currently has 10240 > 88e6165 has 1632 but currently has 1522 > 88e6171 has 1522 but currently has 10240 > 88e6172 has 10240 (no change) > 88e6175 has 1632 but currently has 10240 > 88e6176 has 10240 (no change) > 88e6185 has 1632 (no change) > 88e6190 has 10240 (no change) > 88e6190x has 10240 (no change) > 88e6191 has 10240 but currently has 1522 > 88e6191x has 1522 but currently has 10240 > 88e6193x has 1522 but currently has 10240 > 88e6220 has 2048 but currently has 1522 > 88e6240 has 10240 (no change) > 88e6250 has 2048 but currently has 1522 > 88e6290 has 10240 but currently has 1522 > 88e6320 has 10240 (no change) > 88e6321 has 10240 (no change) > 88e6341 has 10240 (no change) > 88e6350 has 10240 (no change) > 88e6351 has 10240 (no change) > 88e6352 has 10240 (no change) > 88e6390 has 1522 but currently has 10240 > 88e6390x has 1522 but currently has 10240 > 88e6393x has 1522 but currently has 10240 > > My point is that based on the above, there's an awful lot of changes > that this one patch brings, and I'm not sure many of them are > intended. As I only have access to mv88e60{20|71} SoCs I had to base on the code to deduce which max frame is supported. > > All the ones with "but currently has 10240", it seems they implement > port_set_jumbo_size() which, although the switch may default to a > smaller frame size, we configure it to be higher. Maybe these don't > implement the field that configures those? Maybe your patch is wrong? > I don't know. > > Similarly for the ones with "but currently has 1632", it seems they > implement set_max_frame_size(), but this is only called via > mv88e6xxx_change_mtu(), and I haven't worked out whether that will > be called during initialisation by the networking layer. > > Now, what really concerns me is the difficulty in making this change. > As we can see from the above, there's a lot of changes going on here, > and it's not obvious which are intentional and which may be bugs. I'm also quite surprised about the impact of this patch. > > So, I think it would be far better to introduce the "max_frame_size" > field using the existing values, and then verify that value during > initialisation time for every entry in mv88e6xxx_table[] using the > rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and > have it run that verification, and state that's what's happened and > was successful in the commit message. > > In the next commit, change mv88e6xxx_get_max_mtu() to use those > verified values and remove the verification code. > > Then in the following commit, update the "max_frame_size" values with > the changes you intend to make. > > Then, we can (a) have confidence that each of the new members were > properly initialised, and (b) we can also see what changes you're > intentionally making. > If I understood you correctly - the approach would be to "simulate" and obtain each max_frame_size assigned in mv88e6xxx_get_max_mtu() to be sure that we do preserve current (buggy or not) behaviour. Then I shall just add those two extra values for mv88e60{20|71}. > Right now, given that at least two of the "max_frame_size" values are > wrong in this patch, I think we can say for certain that we've proven > that trying to introduce this new member and use it in a single patch > is too error prone. I do agree here - the code for getting max frame size for each supported SoC is quite convoluted and difficult to deduce the right value from the outset. > > Thanks. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Mon, Jan 30, 2023 at 12:57:31PM +0100, Lukasz Majewski wrote: > Hi Russell, > > > What I'm concerned about, and why I replied, is that setting the > > devices to have a max frame size of 1522 when we program them to use > > a larger frame size means we break those switches for normal sized > > packets. > > > > The current logic in mv88e6xxx_get_max_mtu() is: > > > > If the chip implements port_set_jumbo_size, then packet sizes > > of up to 10240 are supported. > > (ops: 6131, 6141, 6171, 6172, 6175, 6176, 6190, 6190x, 6240, > > 6320, 6321, 6341, 6350, 6351, 6352, 6390, 6390x, 6393x) > > If the chip implements set_max_frame_size, then packet sizes > > of up to 1632 are supported. > > (ops: 6085, 6095, 6097, 6123, 6161, 6185) > > Otherwise, packets of up to 1522 are supported. > > > > Now, going through the patch, I see: > > > > 88e6085 has 10240 but currently has 1632 > > 88e6095 has 1632 (no change) > > 88e6097 has 1632 (no change) > > 88e6123 has 10240 but currently has 1632 > > 88e6131 has 10240 (no change) > > 88e6141 has 10240 (no change) > > 88e6161 has 1632 but currently has 10240 > > 88e6165 has 1632 but currently has 1522 > > 88e6171 has 1522 but currently has 10240 > > 88e6172 has 10240 (no change) > > 88e6175 has 1632 but currently has 10240 > > 88e6176 has 10240 (no change) > > 88e6185 has 1632 (no change) > > 88e6190 has 10240 (no change) > > 88e6190x has 10240 (no change) > > 88e6191 has 10240 but currently has 1522 > > 88e6191x has 1522 but currently has 10240 > > 88e6193x has 1522 but currently has 10240 > > 88e6220 has 2048 but currently has 1522 > > 88e6240 has 10240 (no change) > > 88e6250 has 2048 but currently has 1522 > > 88e6290 has 10240 but currently has 1522 > > 88e6320 has 10240 (no change) > > 88e6321 has 10240 (no change) > > 88e6341 has 10240 (no change) > > 88e6350 has 10240 (no change) > > 88e6351 has 10240 (no change) > > 88e6352 has 10240 (no change) > > 88e6390 has 1522 but currently has 10240 > > 88e6390x has 1522 but currently has 10240 > > 88e6393x has 1522 but currently has 10240 > > > > My point is that based on the above, there's an awful lot of changes > > that this one patch brings, and I'm not sure many of them are > > intended. > > As I only have access to mv88e60{20|71} SoCs I had to base on the code > to deduce which max frame is supported. The above list of differences are also derived from the code, and this rather proves my point that deriving these from the code is hard, and we need a way to programmatically verify that we get them correct. > > So, I think it would be far better to introduce the "max_frame_size" > > field using the existing values, and then verify that value during > > initialisation time for every entry in mv88e6xxx_table[] using the > > rules that mv88e6xxx_get_max_mtu() was using. Boot that kernel, and > > have it run that verification, and state that's what's happened and > > was successful in the commit message. > > > > In the next commit, change mv88e6xxx_get_max_mtu() to use those > > verified values and remove the verification code. > > > > Then in the following commit, update the "max_frame_size" values with > > the changes you intend to make. > > > > Then, we can (a) have confidence that each of the new members were > > properly initialised, and (b) we can also see what changes you're > > intentionally making. > > > > If I understood you correctly - the approach would be to "simulate" and > obtain each max_frame_size assigned in mv88e6xxx_get_max_mtu() to be > sure that we do preserve current (buggy or not) behaviour. What I'm suggesting is something like: static void mv88e6xxx_validate_frame_size(void) { int max; int i; for (i = 0; i < ARRAY_SIZE(mv88e6xxx_table); i++) { /* same logic as in mv88e6xxx_get_max_mtu() */ if (mv88e6xxx_table[i].ops->port_set_jumbo_size) max = 10240; else if (mv88e6xxx_table[i].ops->set_max_frame_size) max = 1632; else max = 1522; if (mv88e6xxx_table[i].max_frame_size != max) pr_err("BUG: %s has differing max_frame_size: %d != %d\n", mv88e6xxx_table[i].name, max, mv88e6xxx_table[i].max_frame_size); } } called from the mv88e6xxx_probe() function. I don't see any need to do much more than that to verify the table, and I don't see any need to make it only execute once - it's not like the code will be around for very long. Provided this code gets run, we can then be sure that the max_frame_size values initially added correspond with the values the driver currently uses.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 242b8b325504..fc6d98c4a029 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3545,11 +3545,10 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) { struct mv88e6xxx_chip *chip = ds->priv; - if (chip->info->ops->port_set_jumbo_size) - return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; - else if (chip->info->ops->set_max_frame_size) - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; - return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; + WARN_ON_ONCE(!chip->info->max_frame_size); + + return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN + - ETH_FCS_LEN; } static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu) @@ -4955,6 +4954,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = { .avb_ops = &mv88e6352_avb_ops, .ptp_ops = &mv88e6250_ptp_ops, .phylink_get_caps = mv88e6250_phylink_get_caps, + .set_max_frame_size = mv88e6185_g1_set_max_frame_size, }; static const struct mv88e6xxx_ops mv88e6290_ops = { @@ -5543,6 +5543,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5565,6 +5566,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_ports = 11, .num_internal_phys = 0, .max_vid = 4095, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5586,6 +5588,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 8, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5610,6 +5613,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5633,6 +5637,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_ports = 8, .num_internal_phys = 0, .max_vid = 4095, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5655,6 +5660,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 11, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x10, .global1_addr = 0x1b, @@ -5679,6 +5685,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5704,6 +5711,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 0, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5728,6 +5736,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5753,6 +5762,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 15, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5777,6 +5787,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5802,6 +5813,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 15, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5825,6 +5837,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_ports = 10, .num_internal_phys = 0, .max_vid = 4095, + .max_frame_size = 1632, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5848,6 +5861,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 16, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5872,6 +5886,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 16, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5895,6 +5910,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 9, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5918,6 +5934,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 9, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5941,6 +5958,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 9, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -5968,6 +5986,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 2, .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), .max_vid = 4095, + .max_frame_size = 2048, .port_base_addr = 0x08, .phy_base_addr = 0x00, .global1_addr = 0x0f, @@ -5992,6 +6011,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 15, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6015,6 +6035,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_ports = 7, .num_internal_phys = 5, .max_vid = 4095, + .max_frame_size = 2048, .port_base_addr = 0x08, .phy_base_addr = 0x00, .global1_addr = 0x0f, @@ -6038,6 +6059,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 16, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 1522, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6062,6 +6084,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .num_gpio = 15, .max_vid = 4095, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6087,6 +6110,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .num_gpio = 15, .max_vid = 4095, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6112,6 +6136,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 11, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x10, .global1_addr = 0x1b, @@ -6137,6 +6162,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6161,6 +6187,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 5, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6186,6 +6213,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 15, .max_vid = 4095, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x10, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6211,6 +6239,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 16, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6236,6 +6265,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_gpio = 16, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, @@ -6260,6 +6290,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 9, .max_vid = 8191, .max_sid = 63, + .max_frame_size = 10240, .port_base_addr = 0x0, .phy_base_addr = 0x0, .global1_addr = 0x1b, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index e693154cf803..31c09b66fbff 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -132,6 +132,12 @@ struct mv88e6xxx_info { unsigned int num_gpio; unsigned int max_vid; unsigned int max_sid; + + /* Max Frame Size. + * This value corresponds to the memory allocated in switch internal + * memory to store single frame. + */ + unsigned int max_frame_size; unsigned int port_base_addr; unsigned int phy_base_addr; unsigned int global1_addr;
Different Marvell DSA switches support different size of max frame bytes to be sent. This value corresponds to the memory allocated in switch to store single frame. For example mv88e6185 supports max 1632 bytes, which is now in-driver standard value. On the other hand - mv88e6250 supports 2048 bytes. To be more interresting - devices supporting jumbo frames - use yet another value (10240 bytes) As this value is internal and may be different for each switch IC, new entry in struct mv88e6xxx_info has been added to store it. This commit doesn't change the code functionality - it just provides the max frame size value explicitly - up till now it has been assigned depending on the callback provided by the IC driver (e.g. .set_max_frame_size, .port_set_jumbo_size). Signed-off-by: Lukasz Majewski <lukma@denx.de> --- Changes for v2: - Define max_frame_size with default value of 1632 bytes, - Set proper value for the mv88e6250 switch SoC (linkstreet) family Changes for v3: - Add default value for 1632B of the max frame size (to avoid problems with not defined values) Changes for v4: - Rework the mv88e6xxx_get_max_mtu() by using per device defined max_frame_size value - Add WARN_ON_ONCE() when max_frame_size is not defined - Add description for the new 'max_frame_size' member of mv88e6xxx_info --- drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++++---- drivers/net/dsa/mv88e6xxx/chip.h | 6 +++++ 2 files changed, 42 insertions(+), 5 deletions(-)