| Message ID | 20211209222424.124791-1-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e0068620e5e1779b3d5bb5649cd196e1cbf277a9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: mv88e6xxx: Add tx fwd offload PVT on intermediate devices | expand |
On Thu, Dec 09, 2021 at 11:24:24PM +0100, Tobias Waldekranz wrote: > In a typical mv88e6xxx switch tree like this: > > CPU > | .----. > .--0--. | .--0--. > | sw0 | | | sw1 | > '-1-2-' | '-1-2-' > '---' > > If sw1p{1,2} are added to a bridge that sw0p1 is not a part of, sw0 > still needs to add a crosschip PVT entry for the virtual DSA device > assigned to represent the bridge. > > Fixes: ce5df6894a57 ("net: dsa: mv88e6xxx: map virtual bridges with forwarding offload in the PVT") > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- This makes sense. Sorry, my Turris MOX has 3 cascaded switches but I only test it using a single bridge that spans all of the ports. So this is why in my case the DSA and CPU ports could receive packets using the virtual bridge device, because mv88e6xxx_port_vlan() had been called on them through the direct mv88e6xxx_port_bridge_join(), not through mv88e6xxx_crosschip_bridge_join(). I guess you have a use case where some leaf ports are in a bridge but some upstream ports aren't, and this is how you caught this? Thanks! Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Though this is a bugfix, it still targets net-next as it depends on > the recent work done by Vladimir here: > > https://lore.kernel.org/netdev/20211206165758.1553882-1-vladimir.oltean@nxp.com/ > > drivers/net/dsa/mv88e6xxx/chip.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 7fadbf987b23..85f5a35340d7 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -2522,6 +2522,7 @@ static int mv88e6xxx_crosschip_bridge_join(struct dsa_switch *ds, > > mv88e6xxx_reg_lock(chip); > err = mv88e6xxx_pvt_map(chip, sw_index, port); > + err = err ? : mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num); > mv88e6xxx_reg_unlock(chip); > > return err; > @@ -2537,7 +2538,8 @@ static void mv88e6xxx_crosschip_bridge_leave(struct dsa_switch *ds, > return; > > mv88e6xxx_reg_lock(chip); > - if (mv88e6xxx_pvt_map(chip, sw_index, port)) > + if (mv88e6xxx_pvt_map(chip, sw_index, port) || > + mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num)) > dev_err(ds->dev, "failed to remap cross-chip Port VLAN\n"); > mv88e6xxx_reg_unlock(chip); > } > -- > 2.25.1 >
On Fri, Dec 10, 2021 at 00:41, Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Dec 09, 2021 at 11:24:24PM +0100, Tobias Waldekranz wrote: >> In a typical mv88e6xxx switch tree like this: >> >> CPU >> | .----. >> .--0--. | .--0--. >> | sw0 | | | sw1 | >> '-1-2-' | '-1-2-' >> '---' >> >> If sw1p{1,2} are added to a bridge that sw0p1 is not a part of, sw0 >> still needs to add a crosschip PVT entry for the virtual DSA device >> assigned to represent the bridge. >> >> Fixes: ce5df6894a57 ("net: dsa: mv88e6xxx: map virtual bridges with forwarding offload in the PVT") >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- > > This makes sense. Sorry, my Turris MOX has 3 cascaded switches but I > only test it using a single bridge that spans all of the ports. > So this is why in my case the DSA and CPU ports could receive packets > using the virtual bridge device, because mv88e6xxx_port_vlan() had been > called on them through the direct mv88e6xxx_port_bridge_join(), not > through mv88e6xxx_crosschip_bridge_join(). Yeah this is by far the most common setup, that's why I missed it as well. > I guess you have a use case > where some leaf ports are in a bridge but some upstream ports aren't, > and this is how you caught this? I've been doing some work on running kselftest-like tests on a multichip mv88e6xxx system. In that process, I discovered this issue along with a whole slew of other nasty things related to isolation of standalone ports. I am finalizing a series to tackle that which (while not exactly elegant) should get the job done. Stay tuned :)
On Thu, 9 Dec 2021 23:24:24 +0100 Tobias Waldekranz wrote: > In a typical mv88e6xxx switch tree like this: > > CPU > | .----. > .--0--. | .--0--. > | sw0 | | | sw1 | > '-1-2-' | '-1-2-' > '---' > > If sw1p{1,2} are added to a bridge that sw0p1 is not a part of, sw0 > still needs to add a crosschip PVT entry for the virtual DSA device > assigned to represent the bridge. > > Fixes: ce5df6894a57 ("net: dsa: mv88e6xxx: map virtual bridges with forwarding offload in the PVT") Hm, should this go to net? The commit above is in 5.15 it seems.
On Fri, Dec 10, 2021 at 21:18, Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 9 Dec 2021 23:24:24 +0100 Tobias Waldekranz wrote: >> In a typical mv88e6xxx switch tree like this: >> >> CPU >> | .----. >> .--0--. | .--0--. >> | sw0 | | | sw1 | >> '-1-2-' | '-1-2-' >> '---' >> >> If sw1p{1,2} are added to a bridge that sw0p1 is not a part of, sw0 >> still needs to add a crosschip PVT entry for the virtual DSA device >> assigned to represent the bridge. >> >> Fixes: ce5df6894a57 ("net: dsa: mv88e6xxx: map virtual bridges with forwarding offload in the PVT") > > Hm, should this go to net? The commit above is in 5.15 it seems. Since there was no cover letter for this patch I put that motivation after the commit message cutoff: Though this is a bugfix, it still targets net-next as it depends on the recent work done by Vladimir here: https://lore.kernel.org/netdev/20211206165758.1553882-1-vladimir.oltean@nxp.com/ I have patches for 5.15 but the implementation is completely different. Given that (1) multichip devices are pretty uncommon to begin with and (2) that this configuration is also rare, I thought it might be more trouble than it was worth. Let me know if you want me to send that too - but independent of that, I think this should go in on net-next.
Hello: This patch was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Thu, 9 Dec 2021 23:24:24 +0100 you wrote: > In a typical mv88e6xxx switch tree like this: > > CPU > | .----. > .--0--. | .--0--. > | sw0 | | | sw1 | > '-1-2-' | '-1-2-' > '---' > > [...] Here is the summary with links: - [net-next] net: dsa: mv88e6xxx: Add tx fwd offload PVT on intermediate devices https://git.kernel.org/netdev/net-next/c/e0068620e5e1 You are awesome, thank you!
On Sat, 11 Dec 2021 22:20:28 +0100 Tobias Waldekranz wrote: > Since there was no cover letter for this patch I put that motivation > after the commit message cutoff: > > Though this is a bugfix, it still targets net-next as it depends on > the recent work done by Vladimir here: > > https://lore.kernel.org/netdev/20211206165758.1553882-1-vladimir.oltean@nxp.com/ Sigh, I should read the stuff under ---, sorry.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 7fadbf987b23..85f5a35340d7 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2522,6 +2522,7 @@ static int mv88e6xxx_crosschip_bridge_join(struct dsa_switch *ds, mv88e6xxx_reg_lock(chip); err = mv88e6xxx_pvt_map(chip, sw_index, port); + err = err ? : mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num); mv88e6xxx_reg_unlock(chip); return err; @@ -2537,7 +2538,8 @@ static void mv88e6xxx_crosschip_bridge_leave(struct dsa_switch *ds, return; mv88e6xxx_reg_lock(chip); - if (mv88e6xxx_pvt_map(chip, sw_index, port)) + if (mv88e6xxx_pvt_map(chip, sw_index, port) || + mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num)) dev_err(ds->dev, "failed to remap cross-chip Port VLAN\n"); mv88e6xxx_reg_unlock(chip); }
In a typical mv88e6xxx switch tree like this: CPU | .----. .--0--. | .--0--. | sw0 | | | sw1 | '-1-2-' | '-1-2-' '---' If sw1p{1,2} are added to a bridge that sw0p1 is not a part of, sw0 still needs to add a crosschip PVT entry for the virtual DSA device assigned to represent the bridge. Fixes: ce5df6894a57 ("net: dsa: mv88e6xxx: map virtual bridges with forwarding offload in the PVT") Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- Though this is a bugfix, it still targets net-next as it depends on the recent work done by Vladimir here: https://lore.kernel.org/netdev/20211206165758.1553882-1-vladimir.oltean@nxp.com/ drivers/net/dsa/mv88e6xxx/chip.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)