Messages in this thread | | | Date | Fri, 26 Nov 2021 03:59:36 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net v2 1/1] net: dsa: microchip: implement multi-bridge support |
| |
On Wed, Nov 24, 2021 at 02:31:43PM +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. > > Note: STP support was broken before this patch and should be fixed > separately. > > Fixes: c2e866911e25 ("net: dsa: microchip: break KSZ9477 DSA driver into two files") > 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 | 50 +++++++++++--------- > drivers/net/dsa/microchip/ksz_common.h | 1 - > 4 files changed, 45 insertions(+), 123 deletions(-)
Generally I like this patch, and the diffstat says it all, the existing logic is a huge mess that isn't even thought out properly.
> > 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)) {
Why did you make this change? The "cpu_port" argument is no longer used now.
> 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. > */
These comments seem out of date, and the ksz_cfg_port_member() function does not exist either. Can you please update them?
> - 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..39583920d6e9 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -22,21 +22,40 @@ > > 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;
Reverse Christmas notation.
> + u8 port_member = 0, cpu_port; > 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); > + cpu_port = BIT(dsa_upstream_port(ds, port)); > + > + for (i = 0; i < ds->num_ports; i++) { > + const struct dsa_port *other_dp = dsa_to_port(ds, i); > + struct ksz_port *other_p = &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 != other_dp->bridge_dev) > continue; > > - /* 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 (other_p->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 | cpu_port); > } > + > + dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port); > } > EXPORT_SYMBOL_GPL(ksz_update_port_member); > > @@ -175,12 +194,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 +205,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;
Can you also delete the remaining writes to p->member from ksz8795.c and ksz9477.c? It appears that it is no longer used now.
> bool remove_tag; /* Remove Tag flag set, for ksz8795 only */ > int stp_state; > struct phy_device phydev; > -- > 2.30.2 >
| |