| Message ID | 20240716120808.396514-1-martin@strongswan.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: mv88e6xxx: Respect other ports when setting chip-wide MTU | expand |
Hi Martin, On Tue, Jul 16, 2024 at 02:08:08PM +0200, Martin Willi wrote: > DSA chips not supporting per-port jumbo frame size configurations use a > chip-wide setting. In the commit referenced with the Fixes tag, the > setting is applied just for the last port changing its MTU. This may > result in two issues: > > * Starting with commit b9c587fed61c ("dsa: mv88e6xxx: Include tagger > overhead when setting MTU for DSA and CPU ports"), the CPU port > accounts for tagger overhead. If a user port is configured after > the CPU port, the chip-wide setting may be reduced again, as the > user port does not account for tagger overhead. > * If different user ports use different MTUs (say within different > L2 domains), setting the lower MTU after the higher MTU may result > in a chip-wide setting for the lower MTU, only. > > Any of the above may result in clearing MV88E6185_G1_CTL1_MAX_FRAME_1632 > while it is actually required for the current configuration on some (CPU) > ports. Specifically, on a MV88E6097 this results in dropped frames when > setting the MTU to 1500 and sending local full-sized frames over a user > port. > > To respect the MTU requirements of all CPU and user ports, get the maximum > frame size requirements over all ports when updating the chip-wide > setting. > > Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU") > Signed-off-by: Martin Willi <martin@strongswan.org> > --- There is actually a much simpler solution which I advise you to take. We already know, by construction, that the MTU applied to the CPU port is the largest among the MTUs of all user ports. So you can program to hardware a chip-wide value which corresponds only to the MTU of the CPU port. You can see that rtl8365mb_port_change_mtu(), qca8k_port_change_mtu(), mt7530_port_change_mtu(), ksz8_change_mtu(), ksz9477_change_mtu() already do this. And we also have rtl8366rb_change_mtu() which takes the long route, as you do, and which could be simplified. This fix should work, I believe: - else if (chip->info->ops->set_max_frame_size) + else if (chip->info->ops->set_max_frame_size && dsa_is_cpu_port(ds, port)) ret = chip->info->ops->set_max_frame_size(chip, new_mtu); BTW, I now realize b53_change_mtu() suffers from the same problem. It would be great if you could also send a patch fixing that driver in the same way.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 07c897b13de1..c231c956bf9a 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3604,9 +3604,19 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) return ETH_DATA_LEN; } +static int mv88e6xxx_get_port_mtu(struct dsa_port *dp) +{ + if (dsa_port_is_cpu(dp)) + return dp->conduit->mtu; + if (dsa_port_is_user(dp) && dp->user) + return dp->user->mtu; + return 0; +} + static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu) { struct mv88e6xxx_chip *chip = ds->priv; + struct dsa_port *dp; int ret = 0; /* For families where we don't know how to alter the MTU, @@ -3626,8 +3636,14 @@ static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu) mv88e6xxx_reg_lock(chip); if (chip->info->ops->port_set_jumbo_size) ret = chip->info->ops->port_set_jumbo_size(chip, port, new_mtu); - else if (chip->info->ops->set_max_frame_size) + else if (chip->info->ops->set_max_frame_size) { + dsa_switch_for_each_port(dp, ds) { + if (dp->index == port) + continue; + new_mtu = max(new_mtu, mv88e6xxx_get_port_mtu(dp)); + } ret = chip->info->ops->set_max_frame_size(chip, new_mtu); + } mv88e6xxx_reg_unlock(chip); return ret;
DSA chips not supporting per-port jumbo frame size configurations use a chip-wide setting. In the commit referenced with the Fixes tag, the setting is applied just for the last port changing its MTU. This may result in two issues: * Starting with commit b9c587fed61c ("dsa: mv88e6xxx: Include tagger overhead when setting MTU for DSA and CPU ports"), the CPU port accounts for tagger overhead. If a user port is configured after the CPU port, the chip-wide setting may be reduced again, as the user port does not account for tagger overhead. * If different user ports use different MTUs (say within different L2 domains), setting the lower MTU after the higher MTU may result in a chip-wide setting for the lower MTU, only. Any of the above may result in clearing MV88E6185_G1_CTL1_MAX_FRAME_1632 while it is actually required for the current configuration on some (CPU) ports. Specifically, on a MV88E6097 this results in dropped frames when setting the MTU to 1500 and sending local full-sized frames over a user port. To respect the MTU requirements of all CPU and user ports, get the maximum frame size requirements over all ports when updating the chip-wide setting. Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU") Signed-off-by: Martin Willi <martin@strongswan.org> --- drivers/net/dsa/mv88e6xxx/chip.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)