| Message ID | 20220131154655.1614770-2-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Improve standalone port isolation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 24 this patch: 24 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 23 this patch: 23 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Tobias, On Mon, Jan 31, 2022 at 04:46:51PM +0100, Tobias Waldekranz wrote: > Clear MapDA on standalone ports to bypass any ATU lookup that might > point the packet in the wrong direction. This means that all packets > are flooded using the PVT config. So make sure that standalone ports > are only allowed to communicate with the CPU port. Actually "CPU port" != "upstream port" (the latter can be an upstream-facing DSA port). The distinction is quite important. > > Here is a scenario in which this is needed: > > CPU > | .----. > .---0---. | .--0--. > | sw0 | | | sw1 | > '-1-2-3-' | '-1-2-' > '---' > > - sw0p1 and sw1p1 are bridged Do sw0p1 and sw1p1 even matter? > - sw0p2 and sw1p2 are in standalone mode > - Learning must be enabled on sw0p3 in order for hardware forwarding > to work properly between bridged ports > > 1. A packet with SA :aa comes in on sw1p2 > 1a. Egresses sw1p0 > 1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3 > 1c. Egresses sw0p0 > > 2. A packet with DA :aa comes in on sw0p2 > 2a. If an ATU lookup is done at this point, the packet will be > incorrectly forwarded towards sw0p3. With this change in place, > the ATU is pypassed and the packet is forwarded in accordance s/pypassed/bypassed/ > whith the PVT, which only contains the CPU port. s/whith/with/ What you describe is a bit convoluted, so let me try to rephrase it. The mv88e6xxx driver configures all standalone ports to use the same DefaultVID(0)/FID(0), and configures standalone user ports with no learning via the Port Association Vector. Shared (cascade + CPU) ports have learning enabled so that cross-chip bridging works without floods. But since learning is per port and not per FID, it means that we enable learning in FID 0, the one where the ATU was supposed to be always empty. So we may end up taking wrong forwarding decisions for standalone ports, notably when we should do software forwarding between ports of different switches. By clearing MapDA, we force standalone ports to bypass any ATU entries that might exist. Question: can we disable learning per FID? I searched for this in the limited documentation that I have, but I didn't see such option. Doing this would be advantageous because we'd end up with a bit more space in the ATU. With your solution we're just doing damage control. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++------- > drivers/net/dsa/mv88e6xxx/port.c | 7 +++++-- > drivers/net/dsa/mv88e6xxx/port.h | 2 +- > include/net/dsa.h | 12 ++++++++++++ > 4 files changed, 43 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 1023e4549359..dde6a8d0ca36 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port) > > pvlan = 0; > > - /* Frames from user ports can egress any local DSA links and CPU ports, > - * as well as any local member of their bridge group. > + /* Frames from standalone user ports can only egress on the > + * CPU port. > + */ > + if (!dsa_port_bridge_dev_get(dp)) > + return BIT(dsa_switch_upstream_port(ds)); > + > + /* Frames from bridged user ports can egress any local DSA > + * links and CPU ports, as well as any local member of their > + * bridge group. > */ > dsa_switch_for_each_port(other_dp, ds) > if (other_dp->type == DSA_PORT_TYPE_CPU || > @@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, > if (err) > goto unlock; > > + err = mv88e6xxx_port_set_map_da(chip, port, true); > + if (err) > + return err; > + > err = mv88e6xxx_port_commit_pvid(chip, port); > if (err) > goto unlock; > @@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, > mv88e6xxx_port_vlan_map(chip, port)) > dev_err(ds->dev, "failed to remap in-chip Port VLAN\n"); > > + err = mv88e6xxx_port_set_map_da(chip, port, false); > + if (err) > + dev_err(ds->dev, > + "port %d failed to restore map-DA: %pe\n", > + port, ERR_PTR(err)); > + > err = mv88e6xxx_port_commit_pvid(chip, port); > if (err) > dev_err(ds->dev, > @@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > return err; > > /* Port Control 2: don't force a good FCS, set the MTU size to > - * 10222 bytes, disable 802.1q tags checking, don't discard tagged or > - * untagged frames on this port, do a destination address lookup on all > - * received packets as usual, disable ARP mirroring and don't send a > - * copy of all transmitted/received frames on this port to the CPU. > + * 10222 bytes, disable 802.1q tags checking, don't discard > + * tagged or untagged frames on this port, skip destination > + * address lookup on user ports, disable ARP mirroring and don't > + * send a copy of all transmitted/received frames on this port > + * to the CPU. > */ > - err = mv88e6xxx_port_set_map_da(chip, port); > + err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port)); > if (err) > return err; > > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c > index ab41619a809b..ceb450113f88 100644 > --- a/drivers/net/dsa/mv88e6xxx/port.c > +++ b/drivers/net/dsa/mv88e6xxx/port.c > @@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, > return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new); > } > > -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) > +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map) > { > u16 reg; > int err; > @@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) > if (err) > return err; > > - reg |= MV88E6XXX_PORT_CTL2_MAP_DA; > + if (map) > + reg |= MV88E6XXX_PORT_CTL2_MAP_DA; > + else > + reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA; > > return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg); > } > diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h > index 03382b66f800..5c347cc58baf 100644 > --- a/drivers/net/dsa/mv88e6xxx/port.h > +++ b/drivers/net/dsa/mv88e6xxx/port.h > @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); > int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); > int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, > bool drop_untagged); > -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); > +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map); > int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, > int upstream_port); > int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port, > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 57b3e4e7413b..30f3192616e5 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port) > return port == dsa_upstream_port(ds, port); > } > > +/* Return the local port used to reach the CPU port */ > +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds) > +{ > + int p; > + > + for (p = 0; p < ds->num_ports; p++) > + if (!dsa_is_unused_port(ds, p)) > + return dsa_upstream_port(ds, p); dsa_switch_for_each_available_port Although to be honest, the caller already has a dp, I wonder why you need to complicate things and don't just call dsa_upstream_port(ds, dp->index) directly. > + > + return ds->num_ports; > +} > + > /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning > * that the routing port from @downstream_ds to @upstream_ds is also the port > * which @downstream_ds uses to reach its dedicated CPU. > -- > 2.25.1 >
On Tue, Feb 01, 2022 at 07:06:34PM +0200, Vladimir Oltean wrote: > Question: can we disable learning per FID? I searched for this in the > limited documentation that I have, but I didn't see such option. > Doing this would be advantageous because we'd end up with a bit more > space in the ATU. With your solution we're just doing damage control. Answering my own question... Your patch 4/5 does disable learning for packets coming from standalone ports, but not by FID, but by VTU policy. So that's good. Reading on...
On Tue, Feb 01, 2022 at 19:06, Vladimir Oltean <olteanv@gmail.com> wrote: > Hi Tobias, > > On Mon, Jan 31, 2022 at 04:46:51PM +0100, Tobias Waldekranz wrote: >> Clear MapDA on standalone ports to bypass any ATU lookup that might >> point the packet in the wrong direction. This means that all packets >> are flooded using the PVT config. So make sure that standalone ports >> are only allowed to communicate with the CPU port. > > Actually "CPU port" != "upstream port" (the latter can be an > upstream-facing DSA port). The distinction is quite important. Yes that was sloppy of me, I'll rephrase. >> >> Here is a scenario in which this is needed: >> >> CPU >> | .----. >> .---0---. | .--0--. >> | sw0 | | | sw1 | >> '-1-2-3-' | '-1-2-' >> '---' >> >> - sw0p1 and sw1p1 are bridged > > Do sw0p1 and sw1p1 even matter? Strictly speaking, no - it was just to illustrate... >> - sw0p2 and sw1p2 are in standalone mode >> - Learning must be enabled on sw0p3 in order for hardware forwarding >> to work properly between bridged ports ... this point, i.e. a clear example of why learning can't be disabled on DSA ports. >> 1. A packet with SA :aa comes in on sw1p2 >> 1a. Egresses sw1p0 >> 1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3 >> 1c. Egresses sw0p0 >> >> 2. A packet with DA :aa comes in on sw0p2 >> 2a. If an ATU lookup is done at this point, the packet will be >> incorrectly forwarded towards sw0p3. With this change in place, >> the ATU is pypassed and the packet is forwarded in accordance > > s/pypassed/bypassed/ > >> whith the PVT, which only contains the CPU port. > > s/whith/with/ > > What you describe is a bit convoluted, so let me try to rephrase it. > The mv88e6xxx driver configures all standalone ports to use the same > DefaultVID(0)/FID(0), and configures standalone user ports with no > learning via the Port Association Vector. Shared (cascade + CPU) ports > have learning enabled so that cross-chip bridging works without floods. > But since learning is per port and not per FID, it means that we enable > learning in FID 0, the one where the ATU was supposed to be always empty. > So we may end up taking wrong forwarding decisions for standalone ports, > notably when we should do software forwarding between ports of different > switches. By clearing MapDA, we force standalone ports to bypass any ATU > entries that might exist. Are you saying you want me to replace the initial paragraph with your version, or are you saying the the example is convoluted and should be replaced by this text? Or is it only for the benefit of other readers? > Question: can we disable learning per FID? I searched for this in the > limited documentation that I have, but I didn't see such option. > Doing this would be advantageous because we'd end up with a bit more > space in the ATU. With your solution we're just doing damage control. As you discovered, and as I tried to lay out in the cover, this is only one part of the whole solution. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++------- >> drivers/net/dsa/mv88e6xxx/port.c | 7 +++++-- >> drivers/net/dsa/mv88e6xxx/port.h | 2 +- >> include/net/dsa.h | 12 ++++++++++++ >> 4 files changed, 43 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >> index 1023e4549359..dde6a8d0ca36 100644 >> --- a/drivers/net/dsa/mv88e6xxx/chip.c >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >> @@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port) >> >> pvlan = 0; >> >> - /* Frames from user ports can egress any local DSA links and CPU ports, >> - * as well as any local member of their bridge group. >> + /* Frames from standalone user ports can only egress on the >> + * CPU port. >> + */ >> + if (!dsa_port_bridge_dev_get(dp)) >> + return BIT(dsa_switch_upstream_port(ds)); >> + >> + /* Frames from bridged user ports can egress any local DSA >> + * links and CPU ports, as well as any local member of their >> + * bridge group. >> */ >> dsa_switch_for_each_port(other_dp, ds) >> if (other_dp->type == DSA_PORT_TYPE_CPU || >> @@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, >> if (err) >> goto unlock; >> >> + err = mv88e6xxx_port_set_map_da(chip, port, true); >> + if (err) >> + return err; >> + >> err = mv88e6xxx_port_commit_pvid(chip, port); >> if (err) >> goto unlock; >> @@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, >> mv88e6xxx_port_vlan_map(chip, port)) >> dev_err(ds->dev, "failed to remap in-chip Port VLAN\n"); >> >> + err = mv88e6xxx_port_set_map_da(chip, port, false); >> + if (err) >> + dev_err(ds->dev, >> + "port %d failed to restore map-DA: %pe\n", >> + port, ERR_PTR(err)); >> + >> err = mv88e6xxx_port_commit_pvid(chip, port); >> if (err) >> dev_err(ds->dev, >> @@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) >> return err; >> >> /* Port Control 2: don't force a good FCS, set the MTU size to >> - * 10222 bytes, disable 802.1q tags checking, don't discard tagged or >> - * untagged frames on this port, do a destination address lookup on all >> - * received packets as usual, disable ARP mirroring and don't send a >> - * copy of all transmitted/received frames on this port to the CPU. >> + * 10222 bytes, disable 802.1q tags checking, don't discard >> + * tagged or untagged frames on this port, skip destination >> + * address lookup on user ports, disable ARP mirroring and don't >> + * send a copy of all transmitted/received frames on this port >> + * to the CPU. >> */ >> - err = mv88e6xxx_port_set_map_da(chip, port); >> + err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port)); >> if (err) >> return err; >> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c >> index ab41619a809b..ceb450113f88 100644 >> --- a/drivers/net/dsa/mv88e6xxx/port.c >> +++ b/drivers/net/dsa/mv88e6xxx/port.c >> @@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, >> return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new); >> } >> >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map) >> { >> u16 reg; >> int err; >> @@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) >> if (err) >> return err; >> >> - reg |= MV88E6XXX_PORT_CTL2_MAP_DA; >> + if (map) >> + reg |= MV88E6XXX_PORT_CTL2_MAP_DA; >> + else >> + reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA; >> >> return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg); >> } >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h >> index 03382b66f800..5c347cc58baf 100644 >> --- a/drivers/net/dsa/mv88e6xxx/port.h >> +++ b/drivers/net/dsa/mv88e6xxx/port.h >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); >> int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); >> int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, >> bool drop_untagged); >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map); >> int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, >> int upstream_port); >> int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port, >> diff --git a/include/net/dsa.h b/include/net/dsa.h >> index 57b3e4e7413b..30f3192616e5 100644 >> --- a/include/net/dsa.h >> +++ b/include/net/dsa.h >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port) >> return port == dsa_upstream_port(ds, port); >> } >> >> +/* Return the local port used to reach the CPU port */ >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds) >> +{ >> + int p; >> + >> + for (p = 0; p < ds->num_ports; p++) >> + if (!dsa_is_unused_port(ds, p)) >> + return dsa_upstream_port(ds, p); > > dsa_switch_for_each_available_port > > Although to be honest, the caller already has a dp, I wonder why you > need to complicate things and don't just call dsa_upstream_port(ds, > dp->index) directly. Because dp refers to the port we are determining the permissions _for_, and ds refers to the chip we are configuring the PVT _on_. I think other_dp and dp should swap names with each other. Because it is very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/? >> + >> + return ds->num_ports; >> +} >> + >> /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning >> * that the routing port from @downstream_ds to @upstream_ds is also the port >> * which @downstream_ds uses to reach its dedicated CPU. >> -- >> 2.25.1 >>
On Tue, Feb 01, 2022 at 08:56:32PM +0100, Tobias Waldekranz wrote: > >> - sw0p1 and sw1p1 are bridged > > > > Do sw0p1 and sw1p1 even matter? > > Strictly speaking, no - it was just to illustrate... > > >> - sw0p2 and sw1p2 are in standalone mode > >> - Learning must be enabled on sw0p3 in order for hardware forwarding > >> to work properly between bridged ports > > ... this point, i.e. a clear example of why learning can't be disabled > on DSA ports. Ok, I understand now. It wasn't too clear. > >> 1. A packet with SA :aa comes in on sw1p2 > >> 1a. Egresses sw1p0 > >> 1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3 > >> 1c. Egresses sw0p0 > >> > >> 2. A packet with DA :aa comes in on sw0p2 > >> 2a. If an ATU lookup is done at this point, the packet will be > >> incorrectly forwarded towards sw0p3. With this change in place, > >> the ATU is pypassed and the packet is forwarded in accordance > > > > s/pypassed/bypassed/ > > > >> whith the PVT, which only contains the CPU port. > > > > s/whith/with/ > > > > What you describe is a bit convoluted, so let me try to rephrase it. > > The mv88e6xxx driver configures all standalone ports to use the same > > DefaultVID(0)/FID(0), and configures standalone user ports with no > > learning via the Port Association Vector. Shared (cascade + CPU) ports > > have learning enabled so that cross-chip bridging works without floods. > > But since learning is per port and not per FID, it means that we enable > > learning in FID 0, the one where the ATU was supposed to be always empty. > > So we may end up taking wrong forwarding decisions for standalone ports, > > notably when we should do software forwarding between ports of different > > switches. By clearing MapDA, we force standalone ports to bypass any ATU > > entries that might exist. > > Are you saying you want me to replace the initial paragraph with your > version, or are you saying the the example is convoluted and should be > replaced by this text? Or is it only for the benefit of other readers? Just for the sake of discussion, I wanted to make sure I understand what you describe. > > Question: can we disable learning per FID? I searched for this in the > > limited documentation that I have, but I didn't see such option. > > Doing this would be advantageous because we'd end up with a bit more > > space in the ATU. With your solution we're just doing damage control. > > As you discovered, and as I tried to lay out in the cover, this is only > one part of the whole solution. I'm not copied to the cover letter :) and I have some issues with my email client / vger, where emails that I receive through the mailing list sometimes take days to reach my inbox, whereas emails sent directly to me reach my inbox instantaneously. So don't assume I read email that wasn't targeted directly to me, sorry. > >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h > >> index 03382b66f800..5c347cc58baf 100644 > >> --- a/drivers/net/dsa/mv88e6xxx/port.h > >> +++ b/drivers/net/dsa/mv88e6xxx/port.h > >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); > >> int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); > >> int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, > >> bool drop_untagged); > >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); > >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map); > >> int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, > >> int upstream_port); > >> int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port, > >> diff --git a/include/net/dsa.h b/include/net/dsa.h > >> index 57b3e4e7413b..30f3192616e5 100644 > >> --- a/include/net/dsa.h > >> +++ b/include/net/dsa.h > >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port) > >> return port == dsa_upstream_port(ds, port); > >> } > >> > >> +/* Return the local port used to reach the CPU port */ > >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds) > >> +{ > >> + int p; > >> + > >> + for (p = 0; p < ds->num_ports; p++) > >> + if (!dsa_is_unused_port(ds, p)) > >> + return dsa_upstream_port(ds, p); > > > > dsa_switch_for_each_available_port > > > > Although to be honest, the caller already has a dp, I wonder why you > > need to complicate things and don't just call dsa_upstream_port(ds, > > dp->index) directly. > > Because dp refers to the port we are determining the permissions _for_, > and ds refers to the chip we are configuring the PVT _on_. > > I think other_dp and dp should swap names with each other. Because it is > very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/? Sorry, my mistake, I was looking at the patch in the email client and didn't recognize from the context that this is mv88e6xxx_port_vlan(), and that the port is remote. So I retract the part about calling dsa_upstream_port() directly, but please still consider using a more appropriate port iterator for the implementation of dsa_switch_upstream_port().
On Tue, Feb 01, 2022 at 22:11, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Feb 01, 2022 at 08:56:32PM +0100, Tobias Waldekranz wrote: >> >> - sw0p1 and sw1p1 are bridged >> > >> > Do sw0p1 and sw1p1 even matter? >> >> Strictly speaking, no - it was just to illustrate... >> >> >> - sw0p2 and sw1p2 are in standalone mode >> >> - Learning must be enabled on sw0p3 in order for hardware forwarding >> >> to work properly between bridged ports >> >> ... this point, i.e. a clear example of why learning can't be disabled >> on DSA ports. > > Ok, I understand now. It wasn't too clear. > >> >> 1. A packet with SA :aa comes in on sw1p2 >> >> 1a. Egresses sw1p0 >> >> 1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3 >> >> 1c. Egresses sw0p0 >> >> >> >> 2. A packet with DA :aa comes in on sw0p2 >> >> 2a. If an ATU lookup is done at this point, the packet will be >> >> incorrectly forwarded towards sw0p3. With this change in place, >> >> the ATU is pypassed and the packet is forwarded in accordance >> > >> > s/pypassed/bypassed/ >> > >> >> whith the PVT, which only contains the CPU port. >> > >> > s/whith/with/ >> > >> > What you describe is a bit convoluted, so let me try to rephrase it. >> > The mv88e6xxx driver configures all standalone ports to use the same >> > DefaultVID(0)/FID(0), and configures standalone user ports with no >> > learning via the Port Association Vector. Shared (cascade + CPU) ports >> > have learning enabled so that cross-chip bridging works without floods. >> > But since learning is per port and not per FID, it means that we enable >> > learning in FID 0, the one where the ATU was supposed to be always empty. >> > So we may end up taking wrong forwarding decisions for standalone ports, >> > notably when we should do software forwarding between ports of different >> > switches. By clearing MapDA, we force standalone ports to bypass any ATU >> > entries that might exist. >> >> Are you saying you want me to replace the initial paragraph with your >> version, or are you saying the the example is convoluted and should be >> replaced by this text? Or is it only for the benefit of other readers? > > Just for the sake of discussion, I wanted to make sure I understand what > you describe. > >> > Question: can we disable learning per FID? I searched for this in the >> > limited documentation that I have, but I didn't see such option. >> > Doing this would be advantageous because we'd end up with a bit more >> > space in the ATU. With your solution we're just doing damage control. >> >> As you discovered, and as I tried to lay out in the cover, this is only >> one part of the whole solution. > > I'm not copied to the cover letter :) and I have some issues with my > email client / vger, where emails that I receive through the mailing list > sometimes take days to reach my inbox, whereas emails sent directly to > me reach my inbox instantaneously. So don't assume I read email that > wasn't targeted directly to me, sorry. No worries. I have recently started using get_maintainers.pl to auto generate the recipient list, with the result that the cover is only sent to the list. Ideally I would like send-email to use the union of all recipients for the cover letter, but I haven't figured that one out yet. I actually gave up on getting my mailinglists from my email provider, now I just download it directly from lore. I hacked together a script that will scrape a public-inbox repo and convert it to a Maildir: https://github.com/wkz/notmuch-lore As you can tell from the name, it is tailored for plugging into notmuch, but the guts are pretty generic. >> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h >> >> index 03382b66f800..5c347cc58baf 100644 >> >> --- a/drivers/net/dsa/mv88e6xxx/port.h >> >> +++ b/drivers/net/dsa/mv88e6xxx/port.h >> >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); >> >> int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); >> >> int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, >> >> bool drop_untagged); >> >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); >> >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map); >> >> int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, >> >> int upstream_port); >> >> int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port, >> >> diff --git a/include/net/dsa.h b/include/net/dsa.h >> >> index 57b3e4e7413b..30f3192616e5 100644 >> >> --- a/include/net/dsa.h >> >> +++ b/include/net/dsa.h >> >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port) >> >> return port == dsa_upstream_port(ds, port); >> >> } >> >> >> >> +/* Return the local port used to reach the CPU port */ >> >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds) >> >> +{ >> >> + int p; >> >> + >> >> + for (p = 0; p < ds->num_ports; p++) >> >> + if (!dsa_is_unused_port(ds, p)) >> >> + return dsa_upstream_port(ds, p); >> > >> > dsa_switch_for_each_available_port >> > >> > Although to be honest, the caller already has a dp, I wonder why you >> > need to complicate things and don't just call dsa_upstream_port(ds, >> > dp->index) directly. >> >> Because dp refers to the port we are determining the permissions _for_, >> and ds refers to the chip we are configuring the PVT _on_. >> >> I think other_dp and dp should swap names with each other. Because it is >> very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/? > > Sorry, my mistake, I was looking at the patch in the email client and > didn't recognize from the context that this is mv88e6xxx_port_vlan(), > and that the port is remote. So I retract the part about calling > dsa_upstream_port() directly, but please still consider using a more > appropriate port iterator for the implementation of dsa_switch_upstream_port(). Will do!
On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote: > No worries. I have recently started using get_maintainers.pl to auto > generate the recipient list, with the result that the cover is only sent > to the list. Ideally I would like send-email to use the union of all > recipients for the cover letter, but I haven't figured that one out yet. Maybe auto-generating isn't the best solution? Wait until you need to post a link to https://patchwork.kernel.org/project/netdevbpf/, and get_maintainers.pl will draw in all the BPF maintainers for you... The union appears when you run get_maintainer.pl on multiple patch files. I typically run get_maintainer.pl on *.patch, and adjust the git-send-email list from there. > I actually gave up on getting my mailinglists from my email provider, > now I just download it directly from lore. I hacked together a script > that will scrape a public-inbox repo and convert it to a Maildir: > > https://github.com/wkz/notmuch-lore > > As you can tell from the name, it is tailored for plugging into notmuch, > but the guts are pretty generic. Thanks, I set that up, it's syncing right now, I'm also going to compare the size of the git tree vs the maildir I currently have.
On Thu, 3 Feb 2022 15:56:06 +0200 Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote: > > No worries. I have recently started using get_maintainers.pl to auto > > generate the recipient list, with the result that the cover is only sent > > to the list. Ideally I would like send-email to use the union of all > > recipients for the cover letter, but I haven't figured that one out yet. > > Maybe auto-generating isn't the best solution? Wait until you need to > post a link to https://patchwork.kernel.org/project/netdevbpf/, and > get_maintainers.pl will draw in all the BPF maintainers for you... > The union appears when you run get_maintainer.pl on multiple patch > files. I typically run get_maintainer.pl on *.patch, and adjust the > git-send-email list from there. > > > I actually gave up on getting my mailinglists from my email provider, > > now I just download it directly from lore. I hacked together a script > > that will scrape a public-inbox repo and convert it to a Maildir: > > > > https://github.com/wkz/notmuch-lore > > > > As you can tell from the name, it is tailored for plugging into notmuch, > > but the guts are pretty generic. > > Thanks, I set that up, it's syncing right now, I'm also going to compare > the size of the git tree vs the maildir I currently have. Hi Vladimir, please let me know the results. Marek
On Thu, Feb 03, 2022 at 05:01:04PM +0100, Marek Behún wrote: > On Thu, 3 Feb 2022 15:56:06 +0200 > Vladimir Oltean <olteanv@gmail.com> wrote: > > > On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote: > > > No worries. I have recently started using get_maintainers.pl to auto > > > generate the recipient list, with the result that the cover is only sent > > > to the list. Ideally I would like send-email to use the union of all > > > recipients for the cover letter, but I haven't figured that one out yet. > > > > Maybe auto-generating isn't the best solution? Wait until you need to > > post a link to https://patchwork.kernel.org/project/netdevbpf/, and > > get_maintainers.pl will draw in all the BPF maintainers for you... > > The union appears when you run get_maintainer.pl on multiple patch > > files. I typically run get_maintainer.pl on *.patch, and adjust the > > git-send-email list from there. > > > > > I actually gave up on getting my mailinglists from my email provider, > > > now I just download it directly from lore. I hacked together a script > > > that will scrape a public-inbox repo and convert it to a Maildir: > > > > > > https://github.com/wkz/notmuch-lore > > > > > > As you can tell from the name, it is tailored for plugging into notmuch, > > > but the guts are pretty generic. > > > > Thanks, I set that up, it's syncing right now, I'm also going to compare > > the size of the git tree vs the maildir I currently have. > > Hi Vladimir, please let me know the results. > Marek Sure, it's still syncing, at 19GB currently. Please remind me next week if I forget to check on it.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 1023e4549359..dde6a8d0ca36 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port) pvlan = 0; - /* Frames from user ports can egress any local DSA links and CPU ports, - * as well as any local member of their bridge group. + /* Frames from standalone user ports can only egress on the + * CPU port. + */ + if (!dsa_port_bridge_dev_get(dp)) + return BIT(dsa_switch_upstream_port(ds)); + + /* Frames from bridged user ports can egress any local DSA + * links and CPU ports, as well as any local member of their + * bridge group. */ dsa_switch_for_each_port(other_dp, ds) if (other_dp->type == DSA_PORT_TYPE_CPU || @@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, if (err) goto unlock; + err = mv88e6xxx_port_set_map_da(chip, port, true); + if (err) + return err; + err = mv88e6xxx_port_commit_pvid(chip, port); if (err) goto unlock; @@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, mv88e6xxx_port_vlan_map(chip, port)) dev_err(ds->dev, "failed to remap in-chip Port VLAN\n"); + err = mv88e6xxx_port_set_map_da(chip, port, false); + if (err) + dev_err(ds->dev, + "port %d failed to restore map-DA: %pe\n", + port, ERR_PTR(err)); + err = mv88e6xxx_port_commit_pvid(chip, port); if (err) dev_err(ds->dev, @@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) return err; /* Port Control 2: don't force a good FCS, set the MTU size to - * 10222 bytes, disable 802.1q tags checking, don't discard tagged or - * untagged frames on this port, do a destination address lookup on all - * received packets as usual, disable ARP mirroring and don't send a - * copy of all transmitted/received frames on this port to the CPU. + * 10222 bytes, disable 802.1q tags checking, don't discard + * tagged or untagged frames on this port, skip destination + * address lookup on user ports, disable ARP mirroring and don't + * send a copy of all transmitted/received frames on this port + * to the CPU. */ - err = mv88e6xxx_port_set_map_da(chip, port); + err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port)); if (err) return err; diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index ab41619a809b..ceb450113f88 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new); } -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map) { u16 reg; int err; @@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) if (err) return err; - reg |= MV88E6XXX_PORT_CTL2_MAP_DA; + if (map) + reg |= MV88E6XXX_PORT_CTL2_MAP_DA; + else + reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA; return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg); } diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index 03382b66f800..5c347cc58baf 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, bool drop_untagged); -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map); int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, int upstream_port); int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port, diff --git a/include/net/dsa.h b/include/net/dsa.h index 57b3e4e7413b..30f3192616e5 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port) return port == dsa_upstream_port(ds, port); } +/* Return the local port used to reach the CPU port */ +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds) +{ + int p; + + for (p = 0; p < ds->num_ports; p++) + if (!dsa_is_unused_port(ds, p)) + return dsa_upstream_port(ds, p); + + return ds->num_ports; +} + /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning * that the routing port from @downstream_ds to @upstream_ds is also the port * which @downstream_ds uses to reach its dedicated CPU.
Clear MapDA on standalone ports to bypass any ATU lookup that might point the packet in the wrong direction. This means that all packets are flooded using the PVT config. So make sure that standalone ports are only allowed to communicate with the CPU port. Here is a scenario in which this is needed: CPU | .----. .---0---. | .--0--. | sw0 | | | sw1 | '-1-2-3-' | '-1-2-' '---' - sw0p1 and sw1p1 are bridged - sw0p2 and sw1p2 are in standalone mode - Learning must be enabled on sw0p3 in order for hardware forwarding to work properly between bridged ports 1. A packet with SA :aa comes in on sw1p2 1a. Egresses sw1p0 1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3 1c. Egresses sw0p0 2. A packet with DA :aa comes in on sw0p2 2a. If an ATU lookup is done at this point, the packet will be incorrectly forwarded towards sw0p3. With this change in place, the ATU is pypassed and the packet is forwarded in accordance whith the PVT, which only contains the CPU port. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++------- drivers/net/dsa/mv88e6xxx/port.c | 7 +++++-- drivers/net/dsa/mv88e6xxx/port.h | 2 +- include/net/dsa.h | 12 ++++++++++++ 4 files changed, 43 insertions(+), 10 deletions(-)