| Message ID | E1gvPMo-0000Ah-Vy@rmk-PC.armlinux.org.uk |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: mv88e6xxx: fix IPv6 | expand |
Hi Russell, On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote: > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, > + unsigned long flags) > +{ > + struct mv88e6xxx_chip *chip = ds->priv; > + bool unicast, multicast; > + int ret = -EOPNOTSUPP; > + > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) || > + flags & BR_FLOOD; > + multicast = flags & BR_MCAST_FLOOD; > + > + mutex_lock(&chip->reg_lock); > + if (chip->info->ops->port_set_egress_floods) > + ret = chip->info->ops->port_set_egress_floods(chip, port, > + unicast, > + multicast); > + mutex_unlock(&chip->reg_lock); > + > + return ret; > +} > + > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > +{ > + struct mv88e6xxx_chip *chip = ds->priv; > + unsigned long support = 0; > + > + if (chip->info->ops->port_set_egress_floods) > + support |= BR_FLOOD | BR_MCAST_FLOOD; > + > + return support; > +} I think that it isn't necessary to propagate the notion of bridge flags down to the DSA drivers. It might be just enough to add: port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, if the targeted driver has ds->ops->port_set_egress_flood. What do you think? Thanks, Vivien
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote: > Hi Russell, > > On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote: > > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, > > + unsigned long flags) > > +{ > > + struct mv88e6xxx_chip *chip = ds->priv; > > + bool unicast, multicast; > > + int ret = -EOPNOTSUPP; > > + > > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) || > > + flags & BR_FLOOD; > > + multicast = flags & BR_MCAST_FLOOD; > > + > > + mutex_lock(&chip->reg_lock); > > + if (chip->info->ops->port_set_egress_floods) > > + ret = chip->info->ops->port_set_egress_floods(chip, port, > > + unicast, > > + multicast); > > + mutex_unlock(&chip->reg_lock); > > + > > + return ret; > > +} > > + > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > +{ > > + struct mv88e6xxx_chip *chip = ds->priv; > > + unsigned long support = 0; > > + > > + if (chip->info->ops->port_set_egress_floods) > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > + > > + return support; > > +} > > I think that it isn't necessary to propagate the notion of bridge flags down > to the DSA drivers. It might be just enough to add: > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? There are two other flags that I haven't covered which the bridge code expects to be offloaded, and those are the broadcast flood flag and the learning flag. I know that the Marvell switches don't have a bit to control the broadcast flooding, that appears to be controlled via a static entry in the ATU which would have to be modified as the broadcast flood flag is manipulated. I don't know how that is handled in other bridges. Do we want to include the broadcast flood in the above prototype? If we go for this, how do we detect which options a switch supports?
Hi Russell, On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > > +{ > > > + struct mv88e6xxx_chip *chip = ds->priv; > > > + unsigned long support = 0; > > > + > > > + if (chip->info->ops->port_set_egress_floods) > > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > > + > > > + return support; > > > +} > > > > I think that it isn't necessary to propagate the notion of bridge flags down > > to the DSA drivers. It might be just enough to add: > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > There are two other flags that I haven't covered which the bridge code > expects to be offloaded, and those are the broadcast flood flag and > the learning flag. I see. What does the bridge code do if these flags are set? Does it expect the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it program this entry into the bridged ports? In the latter case we have almost nothing to do. In the former case, we can make the core call dsa_port_mdb_add on setup and when a VLAN is added. mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. If tomorrow there's a switch capable of simply toggling a bit to do that, we can add a new ops and skip the port_mdb_add call in the core. > I know that the Marvell switches don't have a bit to control the > broadcast flooding, that appears to be controlled via a static entry > in the ATU which would have to be modified as the broadcast flood flag > is manipulated. I don't know how that is handled in other bridges. > > Do we want to include the broadcast flood in the above prototype? > If we go for this, how do we detect which options a switch supports? If the necessary dsa_switch_ops routine is correctly prototyped, having it implemented by a driver or not should be enough to inform the core that the related feature(s) is/are supported by the switch. I'll try to give a bit more context on why I'd prefer this approach, hoping it makes sense: a switch driver does not need to understand bridge flags per-se, the core should give enough abstraction to this layer (and any other net-specifics). The core just needs to know if a driver can program this or that. More importantly, it can easily become messy to maintain switch-cases of arbitrary flags in all drivers and the core. Thanks, Vivien
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote: > Hi Russell, > > On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote: > > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, > > + unsigned long flags) > > +{ > > + struct mv88e6xxx_chip *chip = ds->priv; > > + bool unicast, multicast; > > + int ret = -EOPNOTSUPP; > > + > > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) || > > + flags & BR_FLOOD; > > + multicast = flags & BR_MCAST_FLOOD; > > + > > + mutex_lock(&chip->reg_lock); > > + if (chip->info->ops->port_set_egress_floods) > > + ret = chip->info->ops->port_set_egress_floods(chip, port, > > + unicast, > > + multicast); > > + mutex_unlock(&chip->reg_lock); > > + > > + return ret; > > +} > > + > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > +{ > > + struct mv88e6xxx_chip *chip = ds->priv; > > + unsigned long support = 0; > > + > > + if (chip->info->ops->port_set_egress_floods) > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > + > > + return support; > > +} > > I think that it isn't necessary to propagate the notion of bridge flags down > to the DSA drivers. It might be just enough to add: > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? I've just changed my last patch to set these modes from dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, I notice this on the ZII rev B board: At boot (without anything connected to any of the switch ports): br0: port 1(lan0) entered blocking state br0: port 1(lan0) entered disabled state device lan0 entered promiscuous mode device eth1 entered promiscuous mode br0: port 2(lan1) entered blocking state br0: port 2(lan1) entered disabled state device lan1 entered promiscuous mode ... I then removed lan0 from the bridge: device lan0 left promiscuous mode br0: port 1(lan0) entered disabled state and then added it back: br0: port 1(lan0) entered blocking state br0: port 1(lan0) entered disabled state device lan0 entered promiscuous mode Now, you'd expect lan0 and lan1 to be configured the same at this point, and the same as it was before lan0 was removed from the bridge? lan0 is port 0, lan1 is port 1 on this switch - and the register debug says: GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 ... 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f Note that port 0 is in disabled state, but port 1 and 2 are in blocking state... but wait, the kernel printed a message saying it was in disabled state! If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as expected, and then returns to 0x43c. It looks like DSA isn't always in sync with bridge as per port state.
Hi Vivien, On Tue, Feb 19, 2019 at 12:00:00PM -0500, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > > > +{ > > > > + struct mv88e6xxx_chip *chip = ds->priv; > > > > + unsigned long support = 0; > > > > + > > > > + if (chip->info->ops->port_set_egress_floods) > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > > > + > > > > + return support; > > > > +} > > > > > > I think that it isn't necessary to propagate the notion of bridge flags down > > > to the DSA drivers. It might be just enough to add: > > > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > > > There are two other flags that I haven't covered which the bridge code > > expects to be offloaded, and those are the broadcast flood flag and > > the learning flag. > > I see. What does the bridge code do if these flags are set? Does it expect > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it > program this entry into the bridged ports? The bridge code defaults to all four flags set. See new_nbp() in net/bridge/br_if.c: p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's no man page documentation for that flag. According to br_flood() in net/bridge/br_forward.c, it controls whether broadcast frames are flooded to all ports or not. Changing this flag is merely handled just like the multicast/unicast flooding flags - a call is made to set the offloaded flags, and if it isn't returned as being supported, a warning is printed. No attempt is made to create or change a forwarding entry for the broadcast MAC address. bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING: learning on or learning off Controls whether a given port will learn MAC addresses from received traffic or not. If learning if off, the bridge will end up flooding any traffic for which it has no FDB entry. By default this flag is on. > In the latter case we have almost nothing to do. In the former case, we can > make the core call dsa_port_mdb_add on setup and when a VLAN is added. > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. > > If tomorrow there's a switch capable of simply toggling a bit to do that, > we can add a new ops and skip the port_mdb_add call in the core. > > > I know that the Marvell switches don't have a bit to control the > > broadcast flooding, that appears to be controlled via a static entry > > in the ATU which would have to be modified as the broadcast flood flag > > is manipulated. I don't know how that is handled in other bridges. > > > > Do we want to include the broadcast flood in the above prototype? > > If we go for this, how do we detect which options a switch supports? > > If the necessary dsa_switch_ops routine is correctly prototyped, having it > implemented by a driver or not should be enough to inform the core that the > related feature(s) is/are supported by the switch. > > I'll try to give a bit more context on why I'd prefer this approach, hoping > it makes sense: a switch driver does not need to understand bridge flags > per-se, the core should give enough abstraction to this layer (and any other > net-specifics). The core just needs to know if a driver can program this or > that. More importantly, it can easily become messy to maintain switch-cases > of arbitrary flags in all drivers and the core. So, should we go the other way and have: int (*port_learning)(struct dsa_switch *ds, int port, bool enable); int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable); int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable); int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable); rather than trying to combine uc/mc into one? It would mean that we'd be performing more bus reads/writes, but I guess that doesn't matter for these configuration parameters.
Hi Russell, On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > I've just changed my last patch to set these modes from > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, > I notice this on the ZII rev B board: > > At boot (without anything connected to any of the switch ports): > > br0: port 1(lan0) entered blocking state > br0: port 1(lan0) entered disabled state > device lan0 entered promiscuous mode > device eth1 entered promiscuous mode > br0: port 2(lan1) entered blocking state > br0: port 2(lan1) entered disabled state > device lan1 entered promiscuous mode > ... > > I then removed lan0 from the bridge: > > device lan0 left promiscuous mode > br0: port 1(lan0) entered disabled state > > and then added it back: > > br0: port 1(lan0) entered blocking state > br0: port 1(lan0) entered disabled state > device lan0 entered promiscuous mode > > Now, you'd expect lan0 and lan1 to be configured the same at this > point, and the same as it was before lan0 was removed from the bridge? > lan0 is port 0, lan1 is port 1 on this switch - and the register debug > says: > > GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 > 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 > ... > 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f > > Note that port 0 is in disabled state, but port 1 and 2 are in > blocking state... but wait, the kernel printed a message saying it was > in disabled state! > > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as > expected, and then returns to 0x43c. > > It looks like DSA isn't always in sync with bridge as per port state. One thing we have to handle in DSA core are the unbridged ports. Such isolated ports must be in forwarding state if they are up, so that they can be used without a bridge, as a standard network interface. Maybe it'd be simpler if the bridge code would put the interface down when unbridging it. That way we could remove the dsa_port_set_state_now(dp, BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes sense though. Thanks, Vivien
On Tue, Feb 19, 2019 at 12:23:04PM -0500, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > > > I've just changed my last patch to set these modes from > > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, > > I notice this on the ZII rev B board: > > > > At boot (without anything connected to any of the switch ports): > > > > br0: port 1(lan0) entered blocking state > > br0: port 1(lan0) entered disabled state > > device lan0 entered promiscuous mode > > device eth1 entered promiscuous mode > > br0: port 2(lan1) entered blocking state > > br0: port 2(lan1) entered disabled state > > device lan1 entered promiscuous mode > > ... > > > > I then removed lan0 from the bridge: > > > > device lan0 left promiscuous mode > > br0: port 1(lan0) entered disabled state > > > > and then added it back: > > > > br0: port 1(lan0) entered blocking state > > br0: port 1(lan0) entered disabled state > > device lan0 entered promiscuous mode > > > > Now, you'd expect lan0 and lan1 to be configured the same at this > > point, and the same as it was before lan0 was removed from the bridge? > > lan0 is port 0, lan1 is port 1 on this switch - and the register debug > > says: > > > > GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 > > 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 > > ... > > 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f > > > > Note that port 0 is in disabled state, but port 1 and 2 are in > > blocking state... but wait, the kernel printed a message saying it was > > in disabled state! > > > > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as > > expected, and then returns to 0x43c. > > > > It looks like DSA isn't always in sync with bridge as per port state. > > One thing we have to handle in DSA core are the unbridged ports. Such isolated > ports must be in forwarding state if they are up, so that they can be used > without a bridge, as a standard network interface. > > Maybe it'd be simpler if the bridge code would put the interface down when > unbridging it. That way we could remove the dsa_port_set_state_now(dp, > BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes > sense though. I'm not concerned about what happens when the port is removed from the bridge above. What I'm concerned about is the two different states when the port is part of a bridge. First time, the DSA switch is set to blocking mode, despite the kernel saying the port is disabled. Second time, the DSA switch is set to disabled mode, and the kernel says the port is disabled. Why is the port in blocking mode first time around?
Hi Russell, On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > > > > +{ > > > > > + struct mv88e6xxx_chip *chip = ds->priv; > > > > > + unsigned long support = 0; > > > > > + > > > > > + if (chip->info->ops->port_set_egress_floods) > > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > > > > + > > > > > + return support; > > > > > +} > > > > > > > > I think that it isn't necessary to propagate the notion of bridge flags down > > > > to the DSA drivers. It might be just enough to add: > > > > > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > > > > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > > > > > There are two other flags that I haven't covered which the bridge code > > > expects to be offloaded, and those are the broadcast flood flag and > > > the learning flag. > > > > I see. What does the bridge code do if these flags are set? Does it expect > > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it > > program this entry into the bridged ports? > > The bridge code defaults to all four flags set. See new_nbp() in > net/bridge/br_if.c: > > p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; > > bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to > userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's > no man page documentation for that flag. > > According to br_flood() in net/bridge/br_forward.c, it controls > whether broadcast frames are flooded to all ports or not. Changing > this flag is merely handled just like the multicast/unicast flooding > flags - a call is made to set the offloaded flags, and if it isn't > returned as being supported, a warning is printed. No attempt is > made to create or change a forwarding entry for the broadcast MAC > address. OK, thanks for the details. The programming of the broadcast MAC address must be handled in the core then, I will move this from mv88e6xxx up to the DSA layer later, but that's totally orthogonal here. > > bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING: > > learning on or learning off > Controls whether a given port will learn MAC addresses from > received traffic or not. If learning if off, the bridge will end > up flooding any traffic for which it has no FDB entry. By > default this flag is on. > > > In the latter case we have almost nothing to do. In the former case, we can > > make the core call dsa_port_mdb_add on setup and when a VLAN is added. > > > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. > > > > If tomorrow there's a switch capable of simply toggling a bit to do that, > > we can add a new ops and skip the port_mdb_add call in the core. > > > > > I know that the Marvell switches don't have a bit to control the > > > broadcast flooding, that appears to be controlled via a static entry > > > in the ATU which would have to be modified as the broadcast flood flag > > > is manipulated. I don't know how that is handled in other bridges. > > > > > > Do we want to include the broadcast flood in the above prototype? > > > If we go for this, how do we detect which options a switch supports? > > > > If the necessary dsa_switch_ops routine is correctly prototyped, having it > > implemented by a driver or not should be enough to inform the core that the > > related feature(s) is/are supported by the switch. > > > > I'll try to give a bit more context on why I'd prefer this approach, hoping > > it makes sense: a switch driver does not need to understand bridge flags > > per-se, the core should give enough abstraction to this layer (and any other > > net-specifics). The core just needs to know if a driver can program this or > > that. More importantly, it can easily become messy to maintain switch-cases > > of arbitrary flags in all drivers and the core. > > So, should we go the other way and have: > > int (*port_learning)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable); > > rather than trying to combine uc/mc into one? It would mean that we'd > be performing more bus reads/writes, but I guess that doesn't matter > for these configuration parameters. I like this very much. As long as these flags can be programmed in switch devices, these ops totally make sense. Thanks, Vivien
On 2/19/19 9:38 AM, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: >>>>>> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) >>>>>> +{ >>>>>> + struct mv88e6xxx_chip *chip = ds->priv; >>>>>> + unsigned long support = 0; >>>>>> + >>>>>> + if (chip->info->ops->port_set_egress_floods) >>>>>> + support |= BR_FLOOD | BR_MCAST_FLOOD; >>>>>> + >>>>>> + return support; >>>>>> +} >>>>> >>>>> I think that it isn't necessary to propagate the notion of bridge flags down >>>>> to the DSA drivers. It might be just enough to add: >>>>> >>>>> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) >>>>> >>>>> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, >>>>> if the targeted driver has ds->ops->port_set_egress_flood. What do you think? >>>> >>>> There are two other flags that I haven't covered which the bridge code >>>> expects to be offloaded, and those are the broadcast flood flag and >>>> the learning flag. >>> >>> I see. What does the bridge code do if these flags are set? Does it expect >>> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it >>> program this entry into the bridged ports? >> >> The bridge code defaults to all four flags set. See new_nbp() in >> net/bridge/br_if.c: >> >> p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; >> >> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to >> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's >> no man page documentation for that flag. >> >> According to br_flood() in net/bridge/br_forward.c, it controls >> whether broadcast frames are flooded to all ports or not. Changing >> this flag is merely handled just like the multicast/unicast flooding >> flags - a call is made to set the offloaded flags, and if it isn't >> returned as being supported, a warning is printed. No attempt is >> made to create or change a forwarding entry for the broadcast MAC >> address. > > OK, thanks for the details. The programming of the broadcast MAC address > must be handled in the core then, I will move this from mv88e6xxx up to the > DSA layer later, but that's totally orthogonal here. I am not sure if it makes sense for us to work hard on supporting BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not appear to be an easy way to specify whether broadcast traffic will be flooded or not, it will be. The only way to tsolve that is to create a MDB/FDB entry with negative logic (e.g.: forward to a non-existing/connected port). Every other bridge flag typically maps 1:1 with a corresponding hardware feature, so we should support them. > >> >> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING: >> >> learning on or learning off >> Controls whether a given port will learn MAC addresses from >> received traffic or not. If learning if off, the bridge will end >> up flooding any traffic for which it has no FDB entry. By >> default this flag is on. >> >>> In the latter case we have almost nothing to do. In the former case, we can >>> make the core call dsa_port_mdb_add on setup and when a VLAN is added. >>> >>> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. >>> >>> If tomorrow there's a switch capable of simply toggling a bit to do that, >>> we can add a new ops and skip the port_mdb_add call in the core. >>> >>>> I know that the Marvell switches don't have a bit to control the >>>> broadcast flooding, that appears to be controlled via a static entry >>>> in the ATU which would have to be modified as the broadcast flood flag >>>> is manipulated. I don't know how that is handled in other bridges. >>>> >>>> Do we want to include the broadcast flood in the above prototype? >>>> If we go for this, how do we detect which options a switch supports? >>> >>> If the necessary dsa_switch_ops routine is correctly prototyped, having it >>> implemented by a driver or not should be enough to inform the core that the >>> related feature(s) is/are supported by the switch. >>> >>> I'll try to give a bit more context on why I'd prefer this approach, hoping >>> it makes sense: a switch driver does not need to understand bridge flags >>> per-se, the core should give enough abstraction to this layer (and any other >>> net-specifics). The core just needs to know if a driver can program this or >>> that. More importantly, it can easily become messy to maintain switch-cases >>> of arbitrary flags in all drivers and the core. >> >> So, should we go the other way and have: >> >> int (*port_learning)(struct dsa_switch *ds, int port, bool enable); >> int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable); >> int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable); >> int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable); >> >> rather than trying to combine uc/mc into one? It would mean that we'd >> be performing more bus reads/writes, but I guess that doesn't matter >> for these configuration parameters. > > I like this very much. As long as these flags can be programmed in switch > devices, these ops totally make sense. No objections or preference here, whatever works :)
On Tue, Feb 19, 2019 at 12:38:28PM -0500, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > > > > > +{ > > > > > > + struct mv88e6xxx_chip *chip = ds->priv; > > > > > > + unsigned long support = 0; > > > > > > + > > > > > > + if (chip->info->ops->port_set_egress_floods) > > > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > > > > > + > > > > > > + return support; > > > > > > +} > > > > > > > > > > I think that it isn't necessary to propagate the notion of bridge flags down > > > > > to the DSA drivers. It might be just enough to add: > > > > > > > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > > > > > > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > > > > > > > There are two other flags that I haven't covered which the bridge code > > > > expects to be offloaded, and those are the broadcast flood flag and > > > > the learning flag. > > > > > > I see. What does the bridge code do if these flags are set? Does it expect > > > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it > > > program this entry into the bridged ports? > > > > The bridge code defaults to all four flags set. See new_nbp() in > > net/bridge/br_if.c: > > > > p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; > > > > bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to > > userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's > > no man page documentation for that flag. > > > > According to br_flood() in net/bridge/br_forward.c, it controls > > whether broadcast frames are flooded to all ports or not. Changing > > this flag is merely handled just like the multicast/unicast flooding > > flags - a call is made to set the offloaded flags, and if it isn't > > returned as being supported, a warning is printed. No attempt is > > made to create or change a forwarding entry for the broadcast MAC > > address. > > OK, thanks for the details. The programming of the broadcast MAC address > must be handled in the core then, I will move this from mv88e6xxx up to the > DSA layer later, but that's totally orthogonal here. > > > > > bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING: > > > > learning on or learning off > > Controls whether a given port will learn MAC addresses from > > received traffic or not. If learning if off, the bridge will end > > up flooding any traffic for which it has no FDB entry. By > > default this flag is on. > > > > > In the latter case we have almost nothing to do. In the former case, we can > > > make the core call dsa_port_mdb_add on setup and when a VLAN is added. > > > > > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. > > > > > > If tomorrow there's a switch capable of simply toggling a bit to do that, > > > we can add a new ops and skip the port_mdb_add call in the core. > > > > > > > I know that the Marvell switches don't have a bit to control the > > > > broadcast flooding, that appears to be controlled via a static entry > > > > in the ATU which would have to be modified as the broadcast flood flag > > > > is manipulated. I don't know how that is handled in other bridges. > > > > > > > > Do we want to include the broadcast flood in the above prototype? > > > > If we go for this, how do we detect which options a switch supports? > > > > > > If the necessary dsa_switch_ops routine is correctly prototyped, having it > > > implemented by a driver or not should be enough to inform the core that the > > > related feature(s) is/are supported by the switch. > > > > > > I'll try to give a bit more context on why I'd prefer this approach, hoping > > > it makes sense: a switch driver does not need to understand bridge flags > > > per-se, the core should give enough abstraction to this layer (and any other > > > net-specifics). The core just needs to know if a driver can program this or > > > that. More importantly, it can easily become messy to maintain switch-cases > > > of arbitrary flags in all drivers and the core. > > > > So, should we go the other way and have: > > > > int (*port_learning)(struct dsa_switch *ds, int port, bool enable); > > int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable); > > int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable); > > int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable); > > > > rather than trying to combine uc/mc into one? It would mean that we'd > > be performing more bus reads/writes, but I guess that doesn't matter > > for these configuration parameters. > > I like this very much. As long as these flags can be programmed in switch > devices, these ops totally make sense. Having these as separate functions means that we would then need additional complexity in mv88e6xxx to store the per-port flooding state, so we can do this: reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK; if (unicast && multicast) reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA; else if (unicast) reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA; else if (multicast) reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA; else reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA; for some of the switches. It looks to me like mv88e6xxx would prefer having at least both the unicast and multicast flags together. Even without that, it means more code in mv88e6xxx to wrap each of these calls between the DSA ops and the chip specific ops...
Hi Florian, On Tue, 19 Feb 2019 09:44:32 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote: > > OK, thanks for the details. The programming of the broadcast MAC address > > must be handled in the core then, I will move this from mv88e6xxx up to the > > DSA layer later, but that's totally orthogonal here. > > I am not sure if it makes sense for us to work hard on supporting > BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not > appear to be an easy way to specify whether broadcast traffic will be > flooded or not, it will be. The only way to tsolve that is to create a > MDB/FDB entry with negative logic (e.g.: forward to a > non-existing/connected port). Every other bridge flag typically maps 1:1 > with a corresponding hardware feature, so we should support them. I think it's best to keep away from driver-specific hacks :-) Since broadcom floods multicast and BR_BCAST_FLOOD is set by default, this would simply translate as not implementing a port_flood_bc ops in the broadcom driver. But I agree that it doesn't seem necessary to implement support for this yet. Thanks, Vivien
Hi Russell, On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > Having these as separate functions means that we would then need > additional complexity in mv88e6xxx to store the per-port flooding state, > so we can do this: > > reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK; > > if (unicast && multicast) > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA; > else if (unicast) > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA; > else if (multicast) > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA; > else > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA; > > for some of the switches. It looks to me like mv88e6xxx would prefer > having at least both the unicast and multicast flags together. > > Even without that, it means more code in mv88e6xxx to wrap each of > these calls between the DSA ops and the chip specific ops... True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc). I do not think that it is necessary to add support for BR_BCAST_FLOOD yet, we can extend this routine later if we need to. Your dsa_port_bridge_flags() core function can notify the understood features. This will allow us to scope the support of the bridge flags in the core, and preventing the drivers to do that themselves. Thanks, Vivien
On Tue, Feb 19, 2019 at 02:04:44PM -0500, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > Having these as separate functions means that we would then need > > additional complexity in mv88e6xxx to store the per-port flooding state, > > so we can do this: > > > > reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK; > > > > if (unicast && multicast) > > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA; > > else if (unicast) > > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA; > > else if (multicast) > > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA; > > else > > reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA; > > > > for some of the switches. It looks to me like mv88e6xxx would prefer > > having at least both the unicast and multicast flags together. > > > > Even without that, it means more code in mv88e6xxx to wrap each of > > these calls between the DSA ops and the chip specific ops... > > True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc). > I do not think that it is necessary to add support for BR_BCAST_FLOOD yet, > we can extend this routine later if we need to. > > Your dsa_port_bridge_flags() core function can notify the understood > features. This will allow us to scope the support of the bridge flags in > the core, and preventing the drivers to do that themselves. So, if we have ops->port_egress_flood, then we tell bridge that we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the bridge actually supports both?
On 2/19/19 11:10 AM, Russell King - ARM Linux admin wrote: > On Tue, Feb 19, 2019 at 02:04:44PM -0500, Vivien Didelot wrote: >> Hi Russell, >> >> On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: >>> Having these as separate functions means that we would then need >>> additional complexity in mv88e6xxx to store the per-port flooding state, >>> so we can do this: >>> >>> reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK; >>> >>> if (unicast && multicast) >>> reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA; >>> else if (unicast) >>> reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA; >>> else if (multicast) >>> reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA; >>> else >>> reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA; >>> >>> for some of the switches. It looks to me like mv88e6xxx would prefer >>> having at least both the unicast and multicast flags together. >>> >>> Even without that, it means more code in mv88e6xxx to wrap each of >>> these calls between the DSA ops and the chip specific ops... >> >> True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc). >> I do not think that it is necessary to add support for BR_BCAST_FLOOD yet, >> we can extend this routine later if we need to. >> >> Your dsa_port_bridge_flags() core function can notify the understood >> features. This will allow us to scope the support of the bridge flags in >> the core, and preventing the drivers to do that themselves. > > So, if we have ops->port_egress_flood, then we tell bridge that > we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the > bridge actually supports both? I have a patch series which removes the need for BRIDGE_FLAGS_SUPPORT and requires you to implement an attribute setter for PRE_BRIDGE_FLAGS, there, you can map that to the same DSA switch operations and check the flags and deny one or both that the driver does not support. Should I re-send it and you submit on top, or the other way around? Either way is fine, just tell me.
Hi Russell, On Tue, 19 Feb 2019 19:10:16 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc). > > I do not think that it is necessary to add support for BR_BCAST_FLOOD yet, > > we can extend this routine later if we need to. > > > > Your dsa_port_bridge_flags() core function can notify the understood > > features. This will allow us to scope the support of the bridge flags in > > the core, and preventing the drivers to do that themselves. > > So, if we have ops->port_egress_flood, then we tell bridge that > we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the > bridge actually supports both? I would say so yes. If a driver implements port_egress_flood(), this means its switch device supports both BR_FLOOD | BR_MCAST_FLOOD. I have one concern though. The documentation of mcast_flood for bridge(8) says that this flag "controls whether a given port will *be flooded* with [unknown] multicast traffic". From this I understand allowing this port to *receive* frames with unknown destination addresses. But with mv88e6xxx, we program whether the port is allowed to egress a frame that has an unknown destination address. Otherwise, it will not go out this port. Am I mistaken? If I understood correctly, is it safe to assume it is the same thing we are implementing here? Thanks, Vivien
On Tue, Feb 19, 2019 at 02:56:27PM -0500, Vivien Didelot wrote: > Hi Russell, > > On Tue, 19 Feb 2019 19:10:16 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc). > > > I do not think that it is necessary to add support for BR_BCAST_FLOOD yet, > > > we can extend this routine later if we need to. > > > > > > Your dsa_port_bridge_flags() core function can notify the understood > > > features. This will allow us to scope the support of the bridge flags in > > > the core, and preventing the drivers to do that themselves. > > > > So, if we have ops->port_egress_flood, then we tell bridge that > > we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the > > bridge actually supports both? > > I would say so yes. If a driver implements port_egress_flood(), this means > its switch device supports both BR_FLOOD | BR_MCAST_FLOOD. > > I have one concern though. The documentation of mcast_flood for bridge(8) > says that this flag "controls whether a given port will *be flooded* with > [unknown] multicast traffic". From this I understand allowing this port to > *receive* frames with unknown destination addresses. But with mv88e6xxx, we > program whether the port is allowed to egress a frame that has an unknown > destination address. Otherwise, it will not go out this port. > > Am I mistaken? If I understood correctly, is it safe to assume it is the > same thing we are implementing here? Please look at the net/bridge code to resolve questions such as this. The relevant code is net/bridge/br_forward.c::br_flood(): void br_flood(struct net_bridge *br, struct sk_buff *skb, enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) { ... list_for_each_entry_rcu(p, &br->port_list, list) { /* Do not flood unicast traffic to ports that turn it off, nor * other traffic if flood off, except for traffic we originate */ switch (pkt_type) { case BR_PKT_UNICAST: if (!(p->flags & BR_FLOOD)) continue; break; case BR_PKT_MULTICAST: if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev) continue; break; case BR_PKT_BROADCAST: if (!(p->flags & BR_BCAST_FLOOD) && skb->dev != br->dev) continue; break; } ... prev = maybe_deliver(prev, p, skb, local_orig); } So, BR_FLOOD, BR_MCAST_FLOOD and BR_BCAST_FLOOD control whether the packet of type pkt_type being flooded on the bridge egresses from port p, where p is each port attached to the bridge.
On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote: > I've just changed my last patch to set these modes from > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, > I notice this on the ZII rev B board: > > At boot (without anything connected to any of the switch ports): > > br0: port 1(lan0) entered blocking state > br0: port 1(lan0) entered disabled state > device lan0 entered promiscuous mode > device eth1 entered promiscuous mode > br0: port 2(lan1) entered blocking state > br0: port 2(lan1) entered disabled state > device lan1 entered promiscuous mode > ... > > I then removed lan0 from the bridge: > > device lan0 left promiscuous mode > br0: port 1(lan0) entered disabled state > > and then added it back: > > br0: port 1(lan0) entered blocking state > br0: port 1(lan0) entered disabled state > device lan0 entered promiscuous mode > > Now, you'd expect lan0 and lan1 to be configured the same at this > point, and the same as it was before lan0 was removed from the bridge? > lan0 is port 0, lan1 is port 1 on this switch - and the register debug > says: > > GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 > 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 > ... > 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f > > Note that port 0 is in disabled state, but port 1 and 2 are in > blocking state... but wait, the kernel printed a message saying it was > in disabled state! > > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as > expected, and then returns to 0x43c. > > It looks like DSA isn't always in sync with bridge as per port state. Okay, the problem is what we do when we up the port. When the port is added to the bridge device, and it's down, the bridge code sets the STP state to "disabled". Then when we up the interface, dsa_slave_open() calls dsa_port_enable(), which then decides to change the STP state on its own without reference to the state assigned by net/bridge: int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy) { u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; ... dsa_port_set_state_now(dp, stp_state); ... } I can understand setting the state to BR_STATE_FORWARDING for stand-alone ports, but why for bridged ports when the bridge code has already taken care of configuring the STP state of the port?
On 2/19/19 3:34 PM, Russell King - ARM Linux admin wrote: > On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote: >> I've just changed my last patch to set these modes from >> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, >> I notice this on the ZII rev B board: >> >> At boot (without anything connected to any of the switch ports): >> >> br0: port 1(lan0) entered blocking state >> br0: port 1(lan0) entered disabled state >> device lan0 entered promiscuous mode >> device eth1 entered promiscuous mode >> br0: port 2(lan1) entered blocking state >> br0: port 2(lan1) entered disabled state >> device lan1 entered promiscuous mode >> ... >> >> I then removed lan0 from the bridge: >> >> device lan0 left promiscuous mode >> br0: port 1(lan0) entered disabled state >> >> and then added it back: >> >> br0: port 1(lan0) entered blocking state >> br0: port 1(lan0) entered disabled state >> device lan0 entered promiscuous mode >> >> Now, you'd expect lan0 and lan1 to be configured the same at this >> point, and the same as it was before lan0 was removed from the bridge? >> lan0 is port 0, lan1 is port 1 on this switch - and the register debug >> says: >> >> GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 >> 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 >> ... >> 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f >> >> Note that port 0 is in disabled state, but port 1 and 2 are in >> blocking state... but wait, the kernel printed a message saying it was >> in disabled state! >> >> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as >> expected, and then returns to 0x43c. >> >> It looks like DSA isn't always in sync with bridge as per port state. > > Okay, the problem is what we do when we up the port. > > When the port is added to the bridge device, and it's down, the bridge > code sets the STP state to "disabled". > > Then when we up the interface, dsa_slave_open() calls dsa_port_enable(), > which then decides to change the STP state on its own without reference > to the state assigned by net/bridge: > > int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy) > { > u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; > ... > dsa_port_set_state_now(dp, stp_state); > ... > } > > I can understand setting the state to BR_STATE_FORWARDING for > stand-alone ports, but why for bridged ports when the bridge code has > already taken care of configuring the STP state of the port? There was no reason for doing that in commit b73adef67765b72f2a0d01ef15aff9d784dc85da ("net: dsa: integrate with SWITCHDEV for HW bridging") other than copying what rocker had done (which served as model back then), and which got changed the next day in rocker with: e47172ab7e4176883077b454286bbd5b87b5f488 ("rocker: put port in FORWADING state after leaving bridge") Good catch! -- Florian
On Tue, Feb 19, 2019 at 03:53:50PM -0800, Florian Fainelli wrote: > On 2/19/19 3:34 PM, Russell King - ARM Linux admin wrote: > > On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote: > >> I've just changed my last patch to set these modes from > >> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing, > >> I notice this on the ZII rev B board: > >> > >> At boot (without anything connected to any of the switch ports): > >> > >> br0: port 1(lan0) entered blocking state > >> br0: port 1(lan0) entered disabled state > >> device lan0 entered promiscuous mode > >> device eth1 entered promiscuous mode > >> br0: port 2(lan1) entered blocking state > >> br0: port 2(lan1) entered disabled state > >> device lan1 entered promiscuous mode > >> ... > >> > >> I then removed lan0 from the bridge: > >> > >> device lan0 left promiscuous mode > >> br0: port 1(lan0) entered disabled state > >> > >> and then added it back: > >> > >> br0: port 1(lan0) entered blocking state > >> br0: port 1(lan0) entered disabled state > >> device lan0 entered promiscuous mode > >> > >> Now, you'd expect lan0 and lan1 to be configured the same at this > >> point, and the same as it was before lan0 was removed from the bridge? > >> lan0 is port 0, lan1 is port 1 on this switch - and the register debug > >> says: > >> > >> GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6 > >> 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04 > >> ... > >> 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f > >> > >> Note that port 0 is in disabled state, but port 1 and 2 are in > >> blocking state... but wait, the kernel printed a message saying it was > >> in disabled state! > >> > >> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as > >> expected, and then returns to 0x43c. > >> > >> It looks like DSA isn't always in sync with bridge as per port state. > > > > Okay, the problem is what we do when we up the port. > > > > When the port is added to the bridge device, and it's down, the bridge > > code sets the STP state to "disabled". > > > > Then when we up the interface, dsa_slave_open() calls dsa_port_enable(), > > which then decides to change the STP state on its own without reference > > to the state assigned by net/bridge: > > > > int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy) > > { > > u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; > > ... > > dsa_port_set_state_now(dp, stp_state); > > ... > > } > > > > I can understand setting the state to BR_STATE_FORWARDING for > > stand-alone ports, but why for bridged ports when the bridge code has > > already taken care of configuring the STP state of the port? > > There was no reason for doing that in commit > b73adef67765b72f2a0d01ef15aff9d784dc85da ("net: dsa: integrate with > SWITCHDEV for HW bridging") other than copying what rocker had done > (which served as model back then), and which got changed the next day in > rocker with: e47172ab7e4176883077b454286bbd5b87b5f488 ("rocker: put port > in FORWADING state after leaving bridge") > > Good catch! As mentioned on IRC, I'll send a patch for this tomorrow for the net tree once I've untangled it from the floods work.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 32e7af5caa69..72db6e74be48 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -4692,6 +4692,38 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port, return err; } +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, + unsigned long flags) +{ + struct mv88e6xxx_chip *chip = ds->priv; + bool unicast, multicast; + int ret = -EOPNOTSUPP; + + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) || + flags & BR_FLOOD; + multicast = flags & BR_MCAST_FLOOD; + + mutex_lock(&chip->reg_lock); + if (chip->info->ops->port_set_egress_floods) + ret = chip->info->ops->port_set_egress_floods(chip, port, + unicast, + multicast); + mutex_unlock(&chip->reg_lock); + + return ret; +} + +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) +{ + struct mv88e6xxx_chip *chip = ds->priv; + unsigned long support = 0; + + if (chip->info->ops->port_set_egress_floods) + support |= BR_FLOOD | BR_MCAST_FLOOD; + + return support; +} + static const struct dsa_switch_ops mv88e6xxx_switch_ops = { #if IS_ENABLED(CONFIG_NET_DSA_LEGACY) .probe = mv88e6xxx_drv_probe, @@ -4719,6 +4751,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .set_ageing_time = mv88e6xxx_set_ageing_time, .port_bridge_join = mv88e6xxx_port_bridge_join, .port_bridge_leave = mv88e6xxx_port_bridge_leave, + .port_bridge_flags = mv88e6xxx_port_bridge_flags, + .bridge_flags_support = mv88e6xxx_bridge_flags_support, .port_stp_state_set = mv88e6xxx_port_stp_state_set, .port_fast_age = mv88e6xxx_port_fast_age, .port_vlan_filtering = mv88e6xxx_port_vlan_filtering,
Add support for the bridge flags to Marvell 88e6xxx bridges, allowing the multicast and unicast flood properties to be controlled. These can be controlled on a per-port basis via commands such as: bridge link set dev lan1 flood on|off bridge link set dev lan1 mcast_flood on|off Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/mv88e6xxx/chip.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)