| Message ID | 20230102150209.985419-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,1/3] dsa: marvell: Provide per device information about max frame size | expand |
On Mon, Jan 02, 2023 at 04:02:07PM +0100, Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 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. > > 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) I would add a WARN_ON_ONCE(!chip->info->max_frame_size) so a missing value is detected. You can then skip the complexity of a default. Andrew
> @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) > 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 (max_t(int, chip->info->max_frame_size, 1632) > + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN); > + > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; I would also prefer if all this if/else logic is removed, and the code simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -132,6 +132,7 @@ struct mv88e6xxx_info { > unsigned int num_gpio; > unsigned int max_vid; > unsigned int max_sid; > + unsigned int max_frame_size; It might be worth adding a comment here what this value actually represents. We don't want any mixups where the value already has the frame checksum removed for example. Andrew
Hi Andrew, > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct > > dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, > > 1632) > > + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN); > > + > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > I would also prefer if all this if/else logic is removed, and the code > simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - > EDSA_HLEN - ETH_FCS_LEN; > So then the mv88e6xxx_get_max_mtu shall look like: WARN_ON_ONCE(!chip->info->max_frame_size) if (chip->info->ops->port_set_jumbo_size) ... else return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function? The above approach is contrary to one proposed by Alexander, who wanted to improve the defensive approach in this driver (to avoid situation where the max_frame_size callback is not defined and max_frame_size member of *_info struct is not added by developer). Which approach is the recommended one for this driver? > > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > > @@ -132,6 +132,7 @@ struct mv88e6xxx_info { > > unsigned int num_gpio; > > unsigned int max_vid; > > unsigned int max_sid; > > + unsigned int max_frame_size; > > It might be worth adding a comment here what this value actually > represents. Ok. I will add proper comment. > We don't want any mixups where the value already has the > frame checksum removed for example. Could you be more specific here about this use case? The max_frame_size is the maximal size of the ethernet frame for which the IC designer provided specified amount of RAM (it is a different value for different SoCs in the Link Street family). > > 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
On Tue, Jan 03, 2023 at 10:02:51AM +0100, Lukasz Majewski wrote: > Hi Andrew, > > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, > > > 1632) > > > + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN); > > > + > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > I would also prefer if all this if/else logic is removed, and the code > > simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - > > EDSA_HLEN - ETH_FCS_LEN; > > > > So then the mv88e6xxx_get_max_mtu shall look like: > > WARN_ON_ONCE(!chip->info->max_frame_size) > > if (chip->info->ops->port_set_jumbo_size) > ... > else > return chip->info->max_frame_size - VLAN_ETH_HLEN - > EDSA_HLEN - ETH_FCS_LEN; I think it should go even further: { WARN_ON_ONCE(!chip->info->max_frame_size) return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; } If we are going to use info->max_frame_size, we should always use info->max_frame_size. Andrew
Hi Andrew, Alexander, > Hi Andrew, > > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, > > > 1632) > > > + - VLAN_ETH_HLEN - EDSA_HLEN - > > > ETH_FCS_LEN); + > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > > > > I would also prefer if all this if/else logic is removed, and the > > code simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - > > EDSA_HLEN - ETH_FCS_LEN; > > > > So then the mv88e6xxx_get_max_mtu shall look like: > > WARN_ON_ONCE(!chip->info->max_frame_size) > > if (chip->info->ops->port_set_jumbo_size) > ... > else > return chip->info->max_frame_size - VLAN_ETH_HLEN - > EDSA_HLEN - ETH_FCS_LEN; > > > Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function? > > > The above approach is contrary to one proposed by Alexander, who > wanted to improve the defensive approach in this driver (to avoid > situation where the max_frame_size callback is not defined and > max_frame_size member of *_info struct is not added by developer). > > Which approach is the recommended one for this driver? Is there any decision regarding the preferred approach to rewrite this code? > > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > > > @@ -132,6 +132,7 @@ struct mv88e6xxx_info { > > > unsigned int num_gpio; > > > unsigned int max_vid; > > > unsigned int max_sid; > > > + unsigned int max_frame_size; > > > > It might be worth adding a comment here what this value actually > > represents. > > Ok. I will add proper comment. > > > We don't want any mixups where the value already has the > > frame checksum removed for example. > > Could you be more specific here about this use case? > > The max_frame_size is the maximal size of the ethernet frame for which > the IC designer provided specified amount of RAM (it is a different > value for different SoCs in the Link Street family). > > > > > 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 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 Thu, Jan 5, 2023 at 2:37 AM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Andrew, Alexander, > > > Hi Andrew, > > > > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > > dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, > > > > 1632) > > > > + - VLAN_ETH_HLEN - EDSA_HLEN - > > > > ETH_FCS_LEN); + > > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > > > > > > > I would also prefer if all this if/else logic is removed, and the > > > code simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - > > > EDSA_HLEN - ETH_FCS_LEN; > > > > > > > So then the mv88e6xxx_get_max_mtu shall look like: > > > > WARN_ON_ONCE(!chip->info->max_frame_size) > > > > if (chip->info->ops->port_set_jumbo_size) > > ... > > else > > return chip->info->max_frame_size - VLAN_ETH_HLEN - > > EDSA_HLEN - ETH_FCS_LEN; > > > > > > Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function? > > > > > > The above approach is contrary to one proposed by Alexander, who > > wanted to improve the defensive approach in this driver (to avoid > > situation where the max_frame_size callback is not defined and > > max_frame_size member of *_info struct is not added by developer). > > > > Which approach is the recommended one for this driver? > > Is there any decision regarding the preferred approach to rewrite this > code? I would defer to what Andrew proposed since he has more experience with the DSA code than I do. Thanks, - Alex
Hi Alexander > On Thu, Jan 5, 2023 at 2:37 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > Hi Andrew, Alexander, > > > > > Hi Andrew, > > > > > > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > > > dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, > > > > > 1632) > > > > > + - VLAN_ETH_HLEN - EDSA_HLEN - > > > > > ETH_FCS_LEN); + > > > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > > > > > > > > > > I would also prefer if all this if/else logic is removed, and > > > > the code simply returned chip->info->max_frame_size - > > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > > > > > > > So then the mv88e6xxx_get_max_mtu shall look like: > > > > > > WARN_ON_ONCE(!chip->info->max_frame_size) > > > > > > if (chip->info->ops->port_set_jumbo_size) > > > ... > > > else > > > return chip->info->max_frame_size - VLAN_ETH_HLEN - > > > EDSA_HLEN - ETH_FCS_LEN; > > > > > > > > > Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function? > > > > > > > > > The above approach is contrary to one proposed by Alexander, who > > > wanted to improve the defensive approach in this driver (to avoid > > > situation where the max_frame_size callback is not defined and > > > max_frame_size member of *_info struct is not added by developer). > > > > > > Which approach is the recommended one for this driver? > > > > Is there any decision regarding the preferred approach to rewrite > > this code? > > I would defer to what Andrew proposed since he has more experience > with the DSA code than I do. > Ok, then I will prepare v4 according to Andrew suggestions. Thanks for the update :-) > Thanks, > > - Alex 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
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 242b8b325504..19668e549391 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) 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 (max_t(int, chip->info->max_frame_size, 1632) + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN); + return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; } @@ -4955,6 +4957,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 = { @@ -5574,6 +5577,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .atu_move_port_mask = 0xf, .multi_chip = true, .ops = &mv88e6095_ops, + .max_frame_size = 1632, }, [MV88E6097] = { @@ -5598,6 +5602,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6097_ops, + .max_frame_size = 1632, }, [MV88E6123] = { @@ -5622,6 +5627,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6123_ops, + .max_frame_size = 1632, }, [MV88E6131] = { @@ -5692,6 +5698,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ptp_support = true, .ops = &mv88e6161_ops, + .max_frame_size = 1632, }, [MV88E6165] = { @@ -5716,6 +5723,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .ptp_support = true, .ops = &mv88e6165_ops, + .max_frame_size = 1632, }, [MV88E6171] = { @@ -5835,6 +5843,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6185_ops, + .max_frame_size = 1632, }, [MV88E6190] = { @@ -5968,6 +5977,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, @@ -6015,6 +6025,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, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index e693154cf803..55948ef56cd0 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -132,6 +132,7 @@ struct mv88e6xxx_info { unsigned int num_gpio; unsigned int max_vid; unsigned int max_sid; + 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. For example mv88e6185 supports max 1632 bytes, which is now in-driver standard value. On the other hand - mv88e6250 supports 2048 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. 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) --- drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- drivers/net/dsa/mv88e6xxx/chip.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-)