Message ID | 20211108111034.2735339-1-o.rempel@pengutronix.de (mailing list archive) |
---|---|
State | RFC |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [RFC,net-next] net: dsa: microchip: implement multi-bridge support | expand |
On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote: > Current driver version is able to handle only one bridge at time. > Configuring two bridges on two different ports would end up shorting this > bridges by HW. To reproduce it: > > ip l a name br0 type bridge > ip l a name br1 type bridge > ip l s dev br0 up > ip l s dev br1 up > ip l s lan1 master br0 > ip l s dev lan1 up > ip l s lan2 master br1 > ip l s dev lan2 up > > Ping on lan1 and get response on lan2, which should not happen. > > This happened, because current driver version is storing one global "Port VLAN > Membership" and applying it to all ports which are members of any > bridge. > To solve this issue, we need to handle each port separately. > > This patch is dropping the global port member storage and calculating > membership dynamically depending on STP state and bridge participation. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- Because there wasn't any restriction in the driver against multiple bridges, I would be tempted to send to the "net" tree and provide a Fixes: tag. > drivers/net/dsa/microchip/ksz8795.c | 53 ++++----------------- > drivers/net/dsa/microchip/ksz9477.c | 64 ++++---------------------- > drivers/net/dsa/microchip/ksz_common.c | 56 +++++++++++++--------- > drivers/net/dsa/microchip/ksz_common.h | 1 - > 4 files changed, 51 insertions(+), 123 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index 43fc3087aeb3..b5f6dff70c89 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -1008,51 +1008,27 @@ static void ksz8_cfg_port_member(struct ksz_device *dev, int port, u8 member) > static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > { > struct ksz_device *dev = ds->priv; > - int forward = dev->member; > struct ksz_port *p; > - int member = -1; > u8 data; > > - p = &dev->ports[port]; > - > ksz_pread8(dev, port, P_STP_CTRL, &data); > data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE); > > switch (state) { > case BR_STATE_DISABLED: > data |= PORT_LEARN_DISABLE; > - if (port < dev->phy_port_cnt) > - member = 0; > break; > case BR_STATE_LISTENING: > data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE); > - if (port < dev->phy_port_cnt && > - p->stp_state == BR_STATE_DISABLED) > - member = dev->host_mask | p->vid_member; > break; > case BR_STATE_LEARNING: > data |= PORT_RX_ENABLE; > break; > case BR_STATE_FORWARDING: > data |= (PORT_TX_ENABLE | PORT_RX_ENABLE); > - > - /* This function is also used internally. */ > - if (port == dev->cpu_port) > - break; > - > - /* Port is a member of a bridge. */ > - if (dev->br_member & BIT(port)) { > - dev->member |= BIT(port); > - member = dev->member; > - } else { > - member = dev->host_mask | p->vid_member; > - } > break; > case BR_STATE_BLOCKING: > data |= PORT_LEARN_DISABLE; > - if (port < dev->phy_port_cnt && > - p->stp_state == BR_STATE_DISABLED) > - member = dev->host_mask | p->vid_member; > break; > default: > dev_err(ds->dev, "invalid STP state: %d\n", state); > @@ -1060,22 +1036,11 @@ static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > } > > ksz_pwrite8(dev, port, P_STP_CTRL, data); > + > + p = &dev->ports[port]; > p->stp_state = state; > - /* Port membership may share register with STP state. */ > - if (member >= 0 && member != p->member) > - ksz8_cfg_port_member(dev, port, (u8)member); > - > - /* Check if forwarding needs to be updated. */ > - if (state != BR_STATE_FORWARDING) { > - if (dev->br_member & BIT(port)) > - dev->member &= ~BIT(port); > - } > > - /* When topology has changed the function ksz_update_port_member > - * should be called to modify port forwarding behavior. > - */ > - if (forward != dev->member) > - ksz_update_port_member(dev, port); > + ksz_update_port_member(dev, port); > } > > static void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port) > @@ -1341,7 +1306,7 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port) > > static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port) > { > - struct ksz_port *p = &dev->ports[port]; > + struct dsa_switch *ds = dev->ds; > struct ksz8 *ksz8 = dev->priv; > const u32 *masks; > u8 member; > @@ -1364,14 +1329,15 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port) > /* enable 802.1p priority */ > ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true); > > - if (cpu_port) { > + if (dsa_is_cpu_port(ds, port)) { > if (!ksz_is_ksz88x3(dev)) > ksz8795_cpu_interface_select(dev, port); > > - member = dev->port_mask; > + member = dsa_user_ports(ds); > } else { > - member = dev->host_mask | p->vid_member; > + member = BIT(dsa_upstream_port(ds, port)); > } > + > ksz8_cfg_port_member(dev, port, member); > } > > @@ -1392,11 +1358,9 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds) > ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true); > > p = &dev->ports[dev->cpu_port]; > - p->vid_member = dev->port_mask; > p->on = 1; > > ksz8_port_setup(dev, dev->cpu_port, true); > - dev->member = dev->host_mask; > > for (i = 0; i < dev->phy_port_cnt; i++) { > p = &dev->ports[i]; > @@ -1404,7 +1368,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds) > /* Initialize to non-zero so that ksz_cfg_port_member() will > * be called. > */ > - p->vid_member = BIT(i); > p->member = dev->port_mask; > ksz8_port_stp_state_set(ds, i, BR_STATE_DISABLED); > > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c > index 854e25f43fa7..3e7df6c0dc72 100644 > --- a/drivers/net/dsa/microchip/ksz9477.c > +++ b/drivers/net/dsa/microchip/ksz9477.c > @@ -400,8 +400,6 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, > struct ksz_device *dev = ds->priv; > struct ksz_port *p = &dev->ports[port]; > u8 data; > - int member = -1; > - int forward = dev->member; > > ksz_pread8(dev, port, P_STP_CTRL, &data); > data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE); > @@ -409,40 +407,18 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, > switch (state) { > case BR_STATE_DISABLED: > data |= PORT_LEARN_DISABLE; > - if (port != dev->cpu_port) > - member = 0; > break; > case BR_STATE_LISTENING: > data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE); > - if (port != dev->cpu_port && > - p->stp_state == BR_STATE_DISABLED) > - member = dev->host_mask | p->vid_member; > break; > case BR_STATE_LEARNING: > data |= PORT_RX_ENABLE; > break; > case BR_STATE_FORWARDING: > data |= (PORT_TX_ENABLE | PORT_RX_ENABLE); > - > - /* This function is also used internally. */ > - if (port == dev->cpu_port) > - break; > - > - member = dev->host_mask | p->vid_member; > - mutex_lock(&dev->dev_mutex); > - > - /* Port is a member of a bridge. */ > - if (dev->br_member & (1 << port)) { > - dev->member |= (1 << port); > - member = dev->member; > - } > - mutex_unlock(&dev->dev_mutex); > break; > case BR_STATE_BLOCKING: > data |= PORT_LEARN_DISABLE; > - if (port != dev->cpu_port && > - p->stp_state == BR_STATE_DISABLED) > - member = dev->host_mask | p->vid_member; > break; > default: > dev_err(ds->dev, "invalid STP state: %d\n", state); > @@ -451,23 +427,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, > > ksz_pwrite8(dev, port, P_STP_CTRL, data); > p->stp_state = state; > - mutex_lock(&dev->dev_mutex); > - /* Port membership may share register with STP state. */ > - if (member >= 0 && member != p->member) > - ksz9477_cfg_port_member(dev, port, (u8)member); > - > - /* Check if forwarding needs to be updated. */ > - if (state != BR_STATE_FORWARDING) { > - if (dev->br_member & (1 << port)) > - dev->member &= ~(1 << port); > - } > > - /* When topology has changed the function ksz_update_port_member > - * should be called to modify port forwarding behavior. > - */ > - if (forward != dev->member) > - ksz_update_port_member(dev, port); > - mutex_unlock(&dev->dev_mutex); > + ksz_update_port_member(dev, port); > } > > static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port) > @@ -1168,10 +1129,10 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port) > > static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port) > { > - u8 data8; > - u8 member; > - u16 data16; > struct ksz_port *p = &dev->ports[port]; > + struct dsa_switch *ds = dev->ds; > + u8 data8, member; > + u16 data16; > > /* enable tag tail for host port */ > if (cpu_port) > @@ -1250,12 +1211,12 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port) > ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8); > p->phydev.duplex = 1; > } > - mutex_lock(&dev->dev_mutex); > - if (cpu_port) > - member = dev->port_mask; > + > + if (dsa_is_cpu_port(ds, port)) > + member = dsa_user_ports(ds); > else > - member = dev->host_mask | p->vid_member; > - mutex_unlock(&dev->dev_mutex); > + member = BIT(dsa_upstream_port(ds, port)); > + > ksz9477_cfg_port_member(dev, port, member); > > /* clear pending interrupts */ > @@ -1276,8 +1237,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) > const char *prev_mode; > > dev->cpu_port = i; > - dev->host_mask = (1 << dev->cpu_port); > - dev->port_mask |= dev->host_mask; > p = &dev->ports[i]; > > /* Read from XMII register to determine host port > @@ -1312,13 +1271,10 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) > > /* enable cpu port */ > ksz9477_port_setup(dev, i, true); > - p->vid_member = dev->port_mask; > p->on = 1; > } > } > > - dev->member = dev->host_mask; > - > for (i = 0; i < dev->port_cnt; i++) { > if (i == dev->cpu_port) > continue; > @@ -1327,8 +1283,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) > /* Initialize to non-zero so that ksz_cfg_port_member() will > * be called. > */ > - p->vid_member = (1 << i); > - p->member = dev->port_mask; > ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED); > p->on = 1; > if (i < dev->phy_port_cnt) > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 7c2968a639eb..84b7ef511ff4 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -22,21 +22,46 @@ > > void ksz_update_port_member(struct ksz_device *dev, int port) > { > - struct ksz_port *p; > + struct ksz_port *p = &dev->ports[port]; > + struct dsa_switch *ds = dev->ds; > + const struct dsa_port *dp; > + u8 port_member = 0; > int i; > > - for (i = 0; i < dev->port_cnt; i++) { > - if (i == port || i == dev->cpu_port) > + if (!dsa_is_user_port(ds, port)) > + return; > + > + dp = dsa_to_port(ds, port); > + > + for (i = 0; i < ds->num_ports; i++) { > + const struct dsa_port *dpi = dsa_to_port(ds, i); Other drivers name this "other_dp", I don't think that name is too bad. Also, you can use "dsa_switch_for_each_user_port", which is also more efficient, although you can't if you target 'stable' with this change, since it has been introduced rather recently. > + struct ksz_port *pi = &dev->ports[i]; and this could be "other_p" rather than "pi". > + u8 val = 0; > + > + if (!dsa_is_user_port(ds, i)) > continue; > - p = &dev->ports[i]; > - if (!(dev->member & (1 << i))) > + if (port == i) > continue; > + if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev) > + continue; > + > + pi = &dev->ports[i]; > + if (pi->stp_state != BR_STATE_DISABLED) > + val |= BIT(dsa_upstream_port(ds, i)); > This is saying: For each {user port, other port} pair, if the other port isn't DISABLED, then allow the user port to forward towards the CPU port of the other port. What sense does that make? You don't support multiple CPU ports, so this port's CPU port is that port's CPU port, and you have one more (broken) forwarding rule towards the CPU port below. > - /* Port is a member of the bridge and is forwarding. */ > - if (p->stp_state == BR_STATE_FORWARDING && > - p->member != dev->member) > - dev->dev_ops->cfg_port_member(dev, i, dev->member); > + if (pi->stp_state == BR_STATE_FORWARDING && > + p->stp_state == BR_STATE_FORWARDING) { > + val |= BIT(port); > + port_member |= BIT(i); > + } > + > + dev->dev_ops->cfg_port_member(dev, i, val); > } > + > + if (p->stp_state != BR_STATE_DISABLED) > + port_member |= BIT(dsa_upstream_port(ds, port)); Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only data packet forwarding, not control packet forwarding, right? > + > + dev->dev_ops->cfg_port_member(dev, port, port_member); > } > EXPORT_SYMBOL_GPL(ksz_update_port_member); > > @@ -175,12 +200,6 @@ EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats); > int ksz_port_bridge_join(struct dsa_switch *ds, int port, > struct net_device *br) > { > - struct ksz_device *dev = ds->priv; > - > - mutex_lock(&dev->dev_mutex); > - dev->br_member |= (1 << port); > - mutex_unlock(&dev->dev_mutex); > - > /* port_stp_state_set() will be called after to put the port in > * appropriate state so there is no need to do anything. > */ > @@ -192,13 +211,6 @@ EXPORT_SYMBOL_GPL(ksz_port_bridge_join); > void ksz_port_bridge_leave(struct dsa_switch *ds, int port, > struct net_device *br) > { > - struct ksz_device *dev = ds->priv; > - > - mutex_lock(&dev->dev_mutex); > - dev->br_member &= ~(1 << port); > - dev->member &= ~(1 << port); > - mutex_unlock(&dev->dev_mutex); > - > /* port_stp_state_set() will be called after to put the port in > * forwarding state so there is no need to do anything. > */ > diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h > index 1597c63988b4..18c9b2e34cd1 100644 > --- a/drivers/net/dsa/microchip/ksz_common.h > +++ b/drivers/net/dsa/microchip/ksz_common.h > @@ -26,7 +26,6 @@ struct ksz_port_mib { > > struct ksz_port { > u16 member; > - u16 vid_member; > bool remove_tag; /* Remove Tag flag set, for ksz8795 only */ > int stp_state; > struct phy_device phydev; > -- > 2.30.2 >
On Wed, Nov 10, 2021 at 02:36:40PM +0200, Vladimir Oltean wrote: > On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote: > > Current driver version is able to handle only one bridge at time. > > Configuring two bridges on two different ports would end up shorting this > > bridges by HW. To reproduce it: > > > > ip l a name br0 type bridge > > ip l a name br1 type bridge > > ip l s dev br0 up > > ip l s dev br1 up > > ip l s lan1 master br0 > > ip l s dev lan1 up > > ip l s lan2 master br1 > > ip l s dev lan2 up > > > > Ping on lan1 and get response on lan2, which should not happen. > > > > This happened, because current driver version is storing one global "Port VLAN > > Membership" and applying it to all ports which are members of any > > bridge. > > To solve this issue, we need to handle each port separately. > > > > This patch is dropping the global port member storage and calculating > > membership dynamically depending on STP state and bridge participation. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > Because there wasn't any restriction in the driver against multiple > bridges, I would be tempted to send to the "net" tree and provide a > Fixes: tag. This patch looks too intrusive to me. It will be hard to backport it to older versions. How about have two patches: add limit to one bridge for net and add support for multiple bridges for net-next? > > + dp = dsa_to_port(ds, port); > > + > > + for (i = 0; i < ds->num_ports; i++) { > > + const struct dsa_port *dpi = dsa_to_port(ds, i); > > Other drivers name this "other_dp", I don't think that name is too bad. > Also, you can use "dsa_switch_for_each_user_port", which is also more > efficient, although you can't if you target 'stable' with this change, > since it has been introduced rather recently. ok > > + struct ksz_port *pi = &dev->ports[i]; > > and this could be "other_p" rather than "pi". ok > > + u8 val = 0; > > + > > + if (!dsa_is_user_port(ds, i)) > > continue; > > - p = &dev->ports[i]; > > - if (!(dev->member & (1 << i))) > > + if (port == i) > > continue; > > + if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev) > > + continue; > > + > > + pi = &dev->ports[i]; > > + if (pi->stp_state != BR_STATE_DISABLED) > > + val |= BIT(dsa_upstream_port(ds, i)); > > > > This is saying: > For each {user port, other port} pair, if the other port isn't DISABLED, > then allow the user port to forward towards the CPU port of the other port. > What sense does that make? You don't support multiple CPU ports, so this > port's CPU port is that port's CPU port, and you have one more (broken) > forwarding rule towards the CPU port below. Ok, understand. > > - /* Port is a member of the bridge and is forwarding. */ > > - if (p->stp_state == BR_STATE_FORWARDING && > > - p->member != dev->member) > > - dev->dev_ops->cfg_port_member(dev, i, dev->member); > > + if (pi->stp_state == BR_STATE_FORWARDING && > > + p->stp_state == BR_STATE_FORWARDING) { > > + val |= BIT(port); > > + port_member |= BIT(i); > > + } > > + > > + dev->dev_ops->cfg_port_member(dev, i, val); > > } > > + > > + if (p->stp_state != BR_STATE_DISABLED) > > + port_member |= BIT(dsa_upstream_port(ds, port)); > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only > data packet forwarding, not control packet forwarding, right? No. According to the KSZ9477S datasheet: "The processor should program the “Static MAC Table” with the entries that it needs to receive (for example, BPDU packets). The “overriding” bit should be set so that the switch will forward those specific packets to the processor. The processor may send packets to the port(s) in this state. Address learning is disabled on the port in this state." This part is not implemented. In current driver implementation (before or after this patch), all packets are forwarded. It looks like, current STP implementation in this driver is not complete. If I create a loop, the bridge will permanently toggle one of ports between blocking and listening. Currently I do not know how to proceed with it. Remove stp callback and make proper, straightforward bride_join/leave? Implement common soft STP for all switches without HW STP support? Regards, Oleksij
On Fri, Nov 12, 2021 at 08:58:23AM +0100, Oleksij Rempel wrote: > On Wed, Nov 10, 2021 at 02:36:40PM +0200, Vladimir Oltean wrote: > > On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote: > > > Current driver version is able to handle only one bridge at time. > > > Configuring two bridges on two different ports would end up shorting this > > > bridges by HW. To reproduce it: > > > > > > ip l a name br0 type bridge > > > ip l a name br1 type bridge > > > ip l s dev br0 up > > > ip l s dev br1 up > > > ip l s lan1 master br0 > > > ip l s dev lan1 up > > > ip l s lan2 master br1 > > > ip l s dev lan2 up > > > > > > Ping on lan1 and get response on lan2, which should not happen. > > > > > > This happened, because current driver version is storing one global "Port VLAN > > > Membership" and applying it to all ports which are members of any > > > bridge. > > > To solve this issue, we need to handle each port separately. > > > > > > This patch is dropping the global port member storage and calculating > > > membership dynamically depending on STP state and bridge participation. > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > --- > > > > Because there wasn't any restriction in the driver against multiple > > bridges, I would be tempted to send to the "net" tree and provide a > > Fixes: tag. > > This patch looks too intrusive to me. It will be hard to backport it to > older versions. How about have two patches: add limit to one bridge for > net and add support for multiple bridges for net-next? That would work. > > > + dp = dsa_to_port(ds, port); > > > + > > > + for (i = 0; i < ds->num_ports; i++) { > > > + const struct dsa_port *dpi = dsa_to_port(ds, i); > > > > Other drivers name this "other_dp", I don't think that name is too bad. > > Also, you can use "dsa_switch_for_each_user_port", which is also more > > efficient, although you can't if you target 'stable' with this change, > > since it has been introduced rather recently. > > ok > > > > + struct ksz_port *pi = &dev->ports[i]; > > > > and this could be "other_p" rather than "pi". > > ok > > > > + u8 val = 0; > > > + > > > + if (!dsa_is_user_port(ds, i)) > > > continue; > > > - p = &dev->ports[i]; > > > - if (!(dev->member & (1 << i))) > > > + if (port == i) > > > continue; > > > + if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev) > > > + continue; > > > + > > > + pi = &dev->ports[i]; > > > + if (pi->stp_state != BR_STATE_DISABLED) > > > + val |= BIT(dsa_upstream_port(ds, i)); > > > > > > > This is saying: > > For each {user port, other port} pair, if the other port isn't DISABLED, > > then allow the user port to forward towards the CPU port of the other port. > > What sense does that make? You don't support multiple CPU ports, so this > > port's CPU port is that port's CPU port, and you have one more (broken) > > forwarding rule towards the CPU port below. > > Ok, understand. > > > > - /* Port is a member of the bridge and is forwarding. */ > > > - if (p->stp_state == BR_STATE_FORWARDING && > > > - p->member != dev->member) > > > - dev->dev_ops->cfg_port_member(dev, i, dev->member); > > > + if (pi->stp_state == BR_STATE_FORWARDING && > > > + p->stp_state == BR_STATE_FORWARDING) { > > > + val |= BIT(port); > > > + port_member |= BIT(i); > > > + } > > > + > > > + dev->dev_ops->cfg_port_member(dev, i, val); > > > } > > > + > > > + if (p->stp_state != BR_STATE_DISABLED) > > > + port_member |= BIT(dsa_upstream_port(ds, port)); > > > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only > > data packet forwarding, not control packet forwarding, right? > > No. According to the KSZ9477S datasheet: > "The processor should program the “Static MAC Table” with the entries that it > needs to receive (for example, BPDU packets). The “overriding” bit should be set > so that the switch will forward those specific packets to the processor. The > processor may send packets to the port(s) in this state. Address learning is > disabled on the port in this state." > > This part is not implemented. > > In current driver implementation (before or after this patch), all > packets are forwarded. It looks like, current STP implementation in this driver > is not complete. If I create a loop, the bridge will permanently toggle one of > ports between blocking and listening. > > Currently I do not know how to proceed with it. Remove stp callback and > make proper, straightforward bride_join/leave? Implement common soft STP > for all switches without HW STP support? What does "soft STP" mean? You need to have a port state in which data plane packets are blocked, but BPDUs can pass. Unless you trap all packets to the CPU and make the selection in software (therefore, including the forwarding, I don't know if that is so desirable), you don't have much of a choice except to do what you've said above, program the static MAC table with entries for 01-80-c2-00-00-0x which trap those link-local multicast addresses to the CPU and set the STP state override bit for them and for them only. BTW, see the "bridge link set" section in "man bridge" for a list of what you should do in each STP state.
On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote: ..... > > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only > > > data packet forwarding, not control packet forwarding, right? > > > > No. According to the KSZ9477S datasheet: > > "The processor should program the “Static MAC Table” with the entries that it > > needs to receive (for example, BPDU packets). The “overriding” bit should be set > > so that the switch will forward those specific packets to the processor. The > > processor may send packets to the port(s) in this state. Address learning is > > disabled on the port in this state." > > > > This part is not implemented. > > > > In current driver implementation (before or after this patch), all > > packets are forwarded. It looks like, current STP implementation in this driver > > is not complete. If I create a loop, the bridge will permanently toggle one of > > ports between blocking and listening. > > > > Currently I do not know how to proceed with it. Remove stp callback and > > make proper, straightforward bride_join/leave? Implement common soft STP > > for all switches without HW STP support? > > What does "soft STP" mean? Some HW seems to provide configuration bits for ports STP states. For example by enabling it, I can just set listening state and it will only pass BPDU packets without need to program static MAC table. (At least, this would be my expectation) For example like this: https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/mt7530.c#L1121 If this HW really exist and works as expected, how should I name it? > You need to have a port state in which data plane packets are blocked, > but BPDUs can pass. ack. > Unless you trap all packets to the CPU and make the selection in software > (therefore, including the forwarding, I don't know if that is so desirable), Yes, this is my point, on plain linux bridge with two simple USB ethernet adapters, I'm able to use STP without any HW offloading. If my HW do not provide straightforward way to trap BPDU packets to CPU, i should be able to reuse functionality already provided by the linux bridge. Probably I need to signal it some how from dsa driver, to let linux bridge make proper decision and reduce logging noise. For example: - Have flag like: ds->sta_without_bpdu_trap = true; - If no .port_mdb_add/.port_fdb_add callbacks are implemented, handle all incoming packet by the linux bridge without making lots of noise, and use .port_bridge_join/.port_bridge_leave to separate ports. - If .port_mdb_add/.port_fdb_add are implemented, program the static MAC table. > you don't have much of a choice except to do what you've said above, program > the static MAC table with entries for 01-80-c2-00-00-0x which trap those > link-local multicast addresses to the CPU and set the STP state override > bit for them and for them only. Hm... Microchip documentation do not describes it as STP state override. Only as "port state override". And since STP state is not directly configurable on this switch, it probably means receive/transmit enable state of the port. So, packets with matching MAC should be forwarded even if port is in the receive disabled state. Correct? > BTW, see the "bridge link set" section in "man bridge" for a list of > what you should do in each STP state. ack. Thank you. Regards, Oleksij
On Tue, Nov 16, 2021 at 09:39:03AM +0100, Oleksij Rempel wrote: > On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote: > > ..... > > > > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only > > > > data packet forwarding, not control packet forwarding, right? > > > > > > No. According to the KSZ9477S datasheet: > > > "The processor should program the “Static MAC Table” with the entries that it > > > needs to receive (for example, BPDU packets). The “overriding” bit should be set > > > so that the switch will forward those specific packets to the processor. The > > > processor may send packets to the port(s) in this state. Address learning is > > > disabled on the port in this state." > > > > > > This part is not implemented. > > > > > > In current driver implementation (before or after this patch), all > > > packets are forwarded. It looks like, current STP implementation in this driver > > > is not complete. If I create a loop, the bridge will permanently toggle one of > > > ports between blocking and listening. > > > > > > Currently I do not know how to proceed with it. Remove stp callback and > > > make proper, straightforward bride_join/leave? Implement common soft STP > > > for all switches without HW STP support? > > > > What does "soft STP" mean? > > Some HW seems to provide configuration bits for ports STP states. For > example by enabling it, I can just set listening state and it will only pass > BPDU packets without need to program static MAC table. (At least, this > would be my expectation) > > For example like this: > https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/mt7530.c#L1121 > > If this HW really exist and works as expected, how should I name it? You are simply talking about the kind of registers that a switch exposes. That doesn't really matter, the end result should be the same. For example, sja1105 doesn't have named STP states, but it allows the driver to specify whether data plane packets can be received and sent on a given port. https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L2030 On other switches it's even more interesting, you need to calibrate STP states from the port forwarding mask. https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/ethernet/mscc/ocelot.c#L1476 In any case, the switch should be able (somehow) to distinguish link-local multicast packets and send them to the CPU port regardless of the source port's STP state, and also be able to inject a link-local multicast packet into a port regardless of its STP state. For Micrel/Microchip switches, at least BPDU injection should be supported, I believe that's what this piece of code does: https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_ksz.c#L127 > > You need to have a port state in which data plane packets are blocked, > > but BPDUs can pass. > > ack. > > > Unless you trap all packets to the CPU and make the selection in software > > (therefore, including the forwarding, I don't know if that is so desirable), > > Yes, this is my point, on plain linux bridge with two simple USB ethernet > adapters, I'm able to use STP without any HW offloading. > > If my HW do not provide straightforward way to trap BPDU packets to CPU, > i should be able to reuse functionality already provided by the linux > bridge. At least on net-next you can return -EOPNOTSUPP in ->port_bridge_join and DSA will leave your port to operate in standalone mode and what will happen is exactly what you describe. But if lack of STP support is what you're trying to fix, there might be better ways of fixing it than doing software bridging. > Probably I need to signal it some how from dsa driver, to let linux > bridge make proper decision and reduce logging noise. What logging noise? > For example: > - Have flag like: ds->sta_without_bpdu_trap = true; :-/ what would this flag do? > - If no .port_mdb_add/.port_fdb_add callbacks are implemented, handle > all incoming packet by the linux bridge without making lots of noise, > and use .port_bridge_join/.port_bridge_leave to separate ports. You can already let forwarding be partially done in software. For example qca8k sets up the CPU port as the only destination for multicast and for flooding: https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/qca8k.c#L1157 Notice how its tagging protocol driver does not call dsa_default_offload_fwd_mark(), in order to let the bridge forward the packets in software: https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_qca.c#L51 > - If .port_mdb_add/.port_fdb_add are implemented, program the static MAC table. You want DSA to program the static MAC table for link-local traffic? Why? DSA doesn't care how you trap your link-local traffic to the CPU, it might vary wildly between one switch and another. Also, ->port_mdb_add() is for data plane multicast packets (aka not BPDUs), and ->port_fdb_add() is for unicast data plane packets (again not BPDUs). Your driver wouldn't even be the first one that traps link-local traffic privately. https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/hirschmann/hellcreek.c#L1050 https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L868 There isn't any good way for user space to have visibility into which packets a switch will trap. There is devlink-trap which AFAIK allows you to see but not modify the traps that are built-in to the hardware/driver: https://www.kernel.org/doc/html/latest/networking/devlink/devlink-trap.html There are also traps which you can add using the tc-trap action (amazingly I could not find documentation for this). But I don't think it would surprise anyone if you would trap BPDUs by default in the driver - as mentioned, some switches already do this with no way to disable it. > > you don't have much of a choice except to do what you've said above, program > > the static MAC table with entries for 01-80-c2-00-00-0x which trap those > > link-local multicast addresses to the CPU and set the STP state override > > bit for them and for them only. > > Hm... Microchip documentation do not describes it as STP state override. Only > as "port state override". Potato, patato, it should be the same thing. > And since STP state is not directly configurable on this switch, it > probably means receive/transmit enable state of the port. So, packets > with matching MAC should be forwarded even if port is in the receive > disabled state. Correct? In the context we've been discussing so far, "forwarding" has a pretty specific meaning, which is autonomously redirecting from one front port to another. For link-local packets, what you want is "trapping", i.e. send to the CPU and to the CPU only. > > > BTW, see the "bridge link set" section in "man bridge" for a list of > > what you should do in each STP state. > > ack. Thank you. > > Regards, > Oleksij > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Nov 16, 2021 at 02:47:23PM +0200, Vladimir Oltean wrote: > On Tue, Nov 16, 2021 at 09:39:03AM +0100, Oleksij Rempel wrote: > > On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote: > > > > ..... > > Probably I need to signal it some how from dsa driver, to let linux > > bridge make proper decision and reduce logging noise. > > What logging noise? I get this with current ksz driver: [ 40.185928] br0: port 2(lan2) entered blocking state [ 40.190924] br0: port 2(lan2) entered listening state [ 41.043186] br0: port 2(lan2) entered blocking state [ 55.512832] br0: port 1(lan1) entered learning state [ 61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost [ 61.279192] br0: port 2(lan2) entered listening state [ 63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 63.123314] br0: port 2(lan2) entered blocking state [ 68.953098] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 70.872840] br0: port 1(lan1) entered forwarding state [ 70.878183] br0: topology change detected, propagating [ 70.883820] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 83.672804] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost [ 83.679181] br0: port 2(lan2) entered listening state [ 85.113244] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 85.123313] br0: port 2(lan2) entered blocking state [ 97.753160] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 103.513076] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 105.432801] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost [ 105.439221] br0: port 2(lan2) entered listening state [ 107.113238] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 107.123326] br0: port 2(lan2) entered blocking state [ 127.192807] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost [ 127.199220] br0: port 2(lan2) entered listening state [ 129.113249] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 129.123378] br0: port 2(lan2) entered blocking state [ 149.592804] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost [ 149.600308] br0: port 2(lan2) entered listening state [ 151.113276] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) [ 151.125213] br0: port 2(lan2) entered blocking state Probably I have wrong expectation... > > And since STP state is not directly configurable on this switch, it > > probably means receive/transmit enable state of the port. So, packets > > with matching MAC should be forwarded even if port is in the receive > > disabled state. Correct? > > In the context we've been discussing so far, "forwarding" has a pretty > specific meaning, which is autonomously redirecting from one front port > to another. For link-local packets, what you want is "trapping", i.e. > send to the CPU and to the CPU only. Ok. Thank you! Regards, Oleksij
> > What logging noise? > > I get this with current ksz driver: > [ 40.185928] br0: port 2(lan2) entered blocking state > [ 40.190924] br0: port 2(lan2) entered listening state > [ 41.043186] br0: port 2(lan2) entered blocking state > [ 55.512832] br0: port 1(lan1) entered learning state > [ 61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost > [ 61.279192] br0: port 2(lan2) entered listening state > [ 63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) I would guess that transmission from the CPU is broken in this case. It could be looking up the destination address in the translation table and not finding an entry. So it floods the packet out all interfaces, including the CPU. So the CPU receives its own packet and gives this warning. Flooding should exclude where the frame came from. Andrew
On Tue, Nov 16, 2021 at 02:40:06PM +0100, Andrew Lunn wrote: > > > What logging noise? > > > > I get this with current ksz driver: > > [ 40.185928] br0: port 2(lan2) entered blocking state > > [ 40.190924] br0: port 2(lan2) entered listening state > > [ 41.043186] br0: port 2(lan2) entered blocking state > > [ 55.512832] br0: port 1(lan1) entered learning state > > [ 61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost > > [ 61.279192] br0: port 2(lan2) entered listening state > > [ 63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) > > I would guess that transmission from the CPU is broken in this > case. It could be looking up the destination address in the > translation table and not finding an entry. So it floods the packet > out all interfaces, including the CPU. So the CPU receives its own > packet and gives this warning. > > Flooding should exclude where the frame came from. I interpret this very differently. If Oleksij is looping lan1 with lan2 and he keeps the MAC addresses the way DSA sets them up by default, i.e. equal and inherited from the DSA master, then receiving a packet with a MAC SA (lan2) equal with the address of the receiving interface (lan1) is absolutely natural. What is not natural is that the bridge attempts to learn from this packet (the message is printed from br_fdb_update), which in turn is caused by the fact that the port is allowed to proceed to the LEARNING state despite there being a loop (which is not detected by STP because STP is broken as Oleksij describes).
On Tue, Nov 16, 2021 at 03:53:35PM +0200, Vladimir Oltean wrote: > On Tue, Nov 16, 2021 at 02:40:06PM +0100, Andrew Lunn wrote: > > > > What logging noise? > > > > > > I get this with current ksz driver: > > > [ 40.185928] br0: port 2(lan2) entered blocking state > > > [ 40.190924] br0: port 2(lan2) entered listening state > > > [ 41.043186] br0: port 2(lan2) entered blocking state > > > [ 55.512832] br0: port 1(lan1) entered learning state > > > [ 61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost > > > [ 61.279192] br0: port 2(lan2) entered listening state > > > [ 63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0) > > > > I would guess that transmission from the CPU is broken in this > > case. It could be looking up the destination address in the > > translation table and not finding an entry. So it floods the packet > > out all interfaces, including the CPU. So the CPU receives its own > > packet and gives this warning. > > > > Flooding should exclude where the frame came from. > > I interpret this very differently. If Oleksij is looping lan1 with lan2 > and he keeps the MAC addresses the way DSA sets them up by default, i.e. > equal and inherited from the DSA master, then receiving a packet with a > MAC SA (lan2) equal with the address of the receiving interface (lan1) > is absolutely natural. What is not natural is that the bridge attempts > to learn from this packet (the message is printed from br_fdb_update), > which in turn is caused by the fact that the port is allowed to proceed > to the LEARNING state despite there being a loop (which is not detected > by STP because STP is broken as Oleksij describes). Ah, yes, that is more likely. Sorry, should not of jumped in without reading all the context. If STP is broken, odd things will happen. Andrew
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 43fc3087aeb3..b5f6dff70c89 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1008,51 +1008,27 @@ static void ksz8_cfg_port_member(struct ksz_device *dev, int port, u8 member) static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) { struct ksz_device *dev = ds->priv; - int forward = dev->member; struct ksz_port *p; - int member = -1; u8 data; - p = &dev->ports[port]; - ksz_pread8(dev, port, P_STP_CTRL, &data); data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE); switch (state) { case BR_STATE_DISABLED: data |= PORT_LEARN_DISABLE; - if (port < dev->phy_port_cnt) - member = 0; break; case BR_STATE_LISTENING: data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE); - if (port < dev->phy_port_cnt && - p->stp_state == BR_STATE_DISABLED) - member = dev->host_mask | p->vid_member; break; case BR_STATE_LEARNING: data |= PORT_RX_ENABLE; break; case BR_STATE_FORWARDING: data |= (PORT_TX_ENABLE | PORT_RX_ENABLE); - - /* This function is also used internally. */ - if (port == dev->cpu_port) - break; - - /* Port is a member of a bridge. */ - if (dev->br_member & BIT(port)) { - dev->member |= BIT(port); - member = dev->member; - } else { - member = dev->host_mask | p->vid_member; - } break; case BR_STATE_BLOCKING: data |= PORT_LEARN_DISABLE; - if (port < dev->phy_port_cnt && - p->stp_state == BR_STATE_DISABLED) - member = dev->host_mask | p->vid_member; break; default: dev_err(ds->dev, "invalid STP state: %d\n", state); @@ -1060,22 +1036,11 @@ static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) } ksz_pwrite8(dev, port, P_STP_CTRL, data); + + p = &dev->ports[port]; p->stp_state = state; - /* Port membership may share register with STP state. */ - if (member >= 0 && member != p->member) - ksz8_cfg_port_member(dev, port, (u8)member); - - /* Check if forwarding needs to be updated. */ - if (state != BR_STATE_FORWARDING) { - if (dev->br_member & BIT(port)) - dev->member &= ~BIT(port); - } - /* When topology has changed the function ksz_update_port_member - * should be called to modify port forwarding behavior. - */ - if (forward != dev->member) - ksz_update_port_member(dev, port); + ksz_update_port_member(dev, port); } static void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port) @@ -1341,7 +1306,7 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port) static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port) { - struct ksz_port *p = &dev->ports[port]; + struct dsa_switch *ds = dev->ds; struct ksz8 *ksz8 = dev->priv; const u32 *masks; u8 member; @@ -1364,14 +1329,15 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port) /* enable 802.1p priority */ ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true); - if (cpu_port) { + if (dsa_is_cpu_port(ds, port)) { if (!ksz_is_ksz88x3(dev)) ksz8795_cpu_interface_select(dev, port); - member = dev->port_mask; + member = dsa_user_ports(ds); } else { - member = dev->host_mask | p->vid_member; + member = BIT(dsa_upstream_port(ds, port)); } + ksz8_cfg_port_member(dev, port, member); } @@ -1392,11 +1358,9 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds) ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true); p = &dev->ports[dev->cpu_port]; - p->vid_member = dev->port_mask; p->on = 1; ksz8_port_setup(dev, dev->cpu_port, true); - dev->member = dev->host_mask; for (i = 0; i < dev->phy_port_cnt; i++) { p = &dev->ports[i]; @@ -1404,7 +1368,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds) /* Initialize to non-zero so that ksz_cfg_port_member() will * be called. */ - p->vid_member = BIT(i); p->member = dev->port_mask; ksz8_port_stp_state_set(ds, i, BR_STATE_DISABLED); diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 854e25f43fa7..3e7df6c0dc72 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -400,8 +400,6 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, struct ksz_device *dev = ds->priv; struct ksz_port *p = &dev->ports[port]; u8 data; - int member = -1; - int forward = dev->member; ksz_pread8(dev, port, P_STP_CTRL, &data); data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE); @@ -409,40 +407,18 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, switch (state) { case BR_STATE_DISABLED: data |= PORT_LEARN_DISABLE; - if (port != dev->cpu_port) - member = 0; break; case BR_STATE_LISTENING: data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE); - if (port != dev->cpu_port && - p->stp_state == BR_STATE_DISABLED) - member = dev->host_mask | p->vid_member; break; case BR_STATE_LEARNING: data |= PORT_RX_ENABLE; break; case BR_STATE_FORWARDING: data |= (PORT_TX_ENABLE | PORT_RX_ENABLE); - - /* This function is also used internally. */ - if (port == dev->cpu_port) - break; - - member = dev->host_mask | p->vid_member; - mutex_lock(&dev->dev_mutex); - - /* Port is a member of a bridge. */ - if (dev->br_member & (1 << port)) { - dev->member |= (1 << port); - member = dev->member; - } - mutex_unlock(&dev->dev_mutex); break; case BR_STATE_BLOCKING: data |= PORT_LEARN_DISABLE; - if (port != dev->cpu_port && - p->stp_state == BR_STATE_DISABLED) - member = dev->host_mask | p->vid_member; break; default: dev_err(ds->dev, "invalid STP state: %d\n", state); @@ -451,23 +427,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port, ksz_pwrite8(dev, port, P_STP_CTRL, data); p->stp_state = state; - mutex_lock(&dev->dev_mutex); - /* Port membership may share register with STP state. */ - if (member >= 0 && member != p->member) - ksz9477_cfg_port_member(dev, port, (u8)member); - - /* Check if forwarding needs to be updated. */ - if (state != BR_STATE_FORWARDING) { - if (dev->br_member & (1 << port)) - dev->member &= ~(1 << port); - } - /* When topology has changed the function ksz_update_port_member - * should be called to modify port forwarding behavior. - */ - if (forward != dev->member) - ksz_update_port_member(dev, port); - mutex_unlock(&dev->dev_mutex); + ksz_update_port_member(dev, port); } static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port) @@ -1168,10 +1129,10 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port) static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port) { - u8 data8; - u8 member; - u16 data16; struct ksz_port *p = &dev->ports[port]; + struct dsa_switch *ds = dev->ds; + u8 data8, member; + u16 data16; /* enable tag tail for host port */ if (cpu_port) @@ -1250,12 +1211,12 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port) ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8); p->phydev.duplex = 1; } - mutex_lock(&dev->dev_mutex); - if (cpu_port) - member = dev->port_mask; + + if (dsa_is_cpu_port(ds, port)) + member = dsa_user_ports(ds); else - member = dev->host_mask | p->vid_member; - mutex_unlock(&dev->dev_mutex); + member = BIT(dsa_upstream_port(ds, port)); + ksz9477_cfg_port_member(dev, port, member); /* clear pending interrupts */ @@ -1276,8 +1237,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) const char *prev_mode; dev->cpu_port = i; - dev->host_mask = (1 << dev->cpu_port); - dev->port_mask |= dev->host_mask; p = &dev->ports[i]; /* Read from XMII register to determine host port @@ -1312,13 +1271,10 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) /* enable cpu port */ ksz9477_port_setup(dev, i, true); - p->vid_member = dev->port_mask; p->on = 1; } } - dev->member = dev->host_mask; - for (i = 0; i < dev->port_cnt; i++) { if (i == dev->cpu_port) continue; @@ -1327,8 +1283,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) /* Initialize to non-zero so that ksz_cfg_port_member() will * be called. */ - p->vid_member = (1 << i); - p->member = dev->port_mask; ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED); p->on = 1; if (i < dev->phy_port_cnt) diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 7c2968a639eb..84b7ef511ff4 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -22,21 +22,46 @@ void ksz_update_port_member(struct ksz_device *dev, int port) { - struct ksz_port *p; + struct ksz_port *p = &dev->ports[port]; + struct dsa_switch *ds = dev->ds; + const struct dsa_port *dp; + u8 port_member = 0; int i; - for (i = 0; i < dev->port_cnt; i++) { - if (i == port || i == dev->cpu_port) + if (!dsa_is_user_port(ds, port)) + return; + + dp = dsa_to_port(ds, port); + + for (i = 0; i < ds->num_ports; i++) { + const struct dsa_port *dpi = dsa_to_port(ds, i); + struct ksz_port *pi = &dev->ports[i]; + u8 val = 0; + + if (!dsa_is_user_port(ds, i)) continue; - p = &dev->ports[i]; - if (!(dev->member & (1 << i))) + if (port == i) continue; + if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev) + continue; + + pi = &dev->ports[i]; + if (pi->stp_state != BR_STATE_DISABLED) + val |= BIT(dsa_upstream_port(ds, i)); - /* Port is a member of the bridge and is forwarding. */ - if (p->stp_state == BR_STATE_FORWARDING && - p->member != dev->member) - dev->dev_ops->cfg_port_member(dev, i, dev->member); + if (pi->stp_state == BR_STATE_FORWARDING && + p->stp_state == BR_STATE_FORWARDING) { + val |= BIT(port); + port_member |= BIT(i); + } + + dev->dev_ops->cfg_port_member(dev, i, val); } + + if (p->stp_state != BR_STATE_DISABLED) + port_member |= BIT(dsa_upstream_port(ds, port)); + + dev->dev_ops->cfg_port_member(dev, port, port_member); } EXPORT_SYMBOL_GPL(ksz_update_port_member); @@ -175,12 +200,6 @@ EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats); int ksz_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br) { - struct ksz_device *dev = ds->priv; - - mutex_lock(&dev->dev_mutex); - dev->br_member |= (1 << port); - mutex_unlock(&dev->dev_mutex); - /* port_stp_state_set() will be called after to put the port in * appropriate state so there is no need to do anything. */ @@ -192,13 +211,6 @@ EXPORT_SYMBOL_GPL(ksz_port_bridge_join); void ksz_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br) { - struct ksz_device *dev = ds->priv; - - mutex_lock(&dev->dev_mutex); - dev->br_member &= ~(1 << port); - dev->member &= ~(1 << port); - mutex_unlock(&dev->dev_mutex); - /* port_stp_state_set() will be called after to put the port in * forwarding state so there is no need to do anything. */ diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 1597c63988b4..18c9b2e34cd1 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -26,7 +26,6 @@ struct ksz_port_mib { struct ksz_port { u16 member; - u16 vid_member; bool remove_tag; /* Remove Tag flag set, for ksz8795 only */ int stp_state; struct phy_device phydev;
Current driver version is able to handle only one bridge at time. Configuring two bridges on two different ports would end up shorting this bridges by HW. To reproduce it: ip l a name br0 type bridge ip l a name br1 type bridge ip l s dev br0 up ip l s dev br1 up ip l s lan1 master br0 ip l s dev lan1 up ip l s lan2 master br1 ip l s dev lan2 up Ping on lan1 and get response on lan2, which should not happen. This happened, because current driver version is storing one global "Port VLAN Membership" and applying it to all ports which are members of any bridge. To solve this issue, we need to handle each port separately. This patch is dropping the global port member storage and calculating membership dynamically depending on STP state and bridge participation. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz8795.c | 53 ++++----------------- drivers/net/dsa/microchip/ksz9477.c | 64 ++++---------------------- drivers/net/dsa/microchip/ksz_common.c | 56 +++++++++++++--------- drivers/net/dsa/microchip/ksz_common.h | 1 - 4 files changed, 51 insertions(+), 123 deletions(-)