| Message ID | 7fbdc27fab4df365db91defca8037b87bdf49438.1718899575.git.mschiffer@universe-factory.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 412e1775f413c944b8c51bdadb675be957d83dc8 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: qca8k: cleanup and port isolation | expand |
On 20.06.2024 19:25, Matthias Schiffer wrote: > Most of the logic in qca8k_port_bridge_join() and qca8k_port_bridge_leave() > is the same. Refactor to reduce duplication and prepare for reusing the > code for implementing bridge port isolation. > > dsa_port_offloads_bridge_dev() is used instead of > dsa_port_offloads_bridge(), passing the bridge in as a struct netdevice *, > as we won't have a struct dsa_bridge in qca8k_port_bridge_flags(). > > The error handling is changed slightly in the bridge leave case, > returning early and emitting an error message when a regmap access fails. > This shouldn't matter in practice, as there isn't much we can do if > communication with the switch breaks down in the middle of reconfiguration. > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > --- One nit, other than that: Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > drivers/net/dsa/qca/qca8k-common.c | 101 ++++++++++++++--------------- > 1 file changed, 50 insertions(+), 51 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c > index b33df84070d3..09108fa99dbe 100644 > --- a/drivers/net/dsa/qca/qca8k-common.c > +++ b/drivers/net/dsa/qca/qca8k-common.c > @@ -614,6 +614,49 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > qca8k_port_configure_learning(ds, port, learning); > } > > +static int qca8k_update_port_member(struct qca8k_priv *priv, int port, > + const struct net_device *bridge_dev, > + bool join) > +{ > + struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp; > + u32 port_mask = BIT(dp->cpu_dp->index); > + int i, ret; > + > + for (i = 0; i < QCA8K_NUM_PORTS; i++) { > + if (i == port) > + continue; > + if (dsa_is_cpu_port(priv->ds, i)) > + continue; > + > + other_dp = dsa_to_port(priv->ds, i); > + if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev)) > + continue; > + > + /* Add/remove this port to/from the portvlan mask of the other > + * ports in the bridge > + */ > + if (join) { > + port_mask |= BIT(i); > + ret = regmap_set_bits(priv->regmap, > + QCA8K_PORT_LOOKUP_CTRL(i), > + BIT(port)); > + } else { > + ret = regmap_clear_bits(priv->regmap, > + QCA8K_PORT_LOOKUP_CTRL(i), > + BIT(port)); > + } > + > + if (ret) > + return ret; > + } > + > + /* Add/remove all other ports to/from this port's portvlan mask */ > + ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port), > + QCA8K_PORT_LOOKUP_MEMBER, port_mask); > + > + return ret; just return qca8k_rmw(...); > +} > + <...>
On Thu, Jun 20, 2024 at 07:25:49PM +0200, Matthias Schiffer wrote: > diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c > index b33df84070d3..09108fa99dbe 100644 > --- a/drivers/net/dsa/qca/qca8k-common.c > +++ b/drivers/net/dsa/qca/qca8k-common.c > @@ -614,6 +614,49 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > qca8k_port_configure_learning(ds, port, learning); > } > > +static int qca8k_update_port_member(struct qca8k_priv *priv, int port, > + const struct net_device *bridge_dev, > + bool join) > +{ > + struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp; > + u32 port_mask = BIT(dp->cpu_dp->index); > + int i, ret; > + > + for (i = 0; i < QCA8K_NUM_PORTS; i++) { > + if (i == port) > + continue; > + if (dsa_is_cpu_port(priv->ds, i)) > + continue; > + > + other_dp = dsa_to_port(priv->ds, i); I would have liked to see less of the "dsa_to_port() in a loop" antipattern. https://lore.kernel.org/netdev/20211018152136.2595220-7-vladimir.oltean@nxp.com/T/ I'll send a patch which refactors the new function to use dsa_switch_for_each_user_port().
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c index b33df84070d3..09108fa99dbe 100644 --- a/drivers/net/dsa/qca/qca8k-common.c +++ b/drivers/net/dsa/qca/qca8k-common.c @@ -614,6 +614,49 @@ void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) qca8k_port_configure_learning(ds, port, learning); } +static int qca8k_update_port_member(struct qca8k_priv *priv, int port, + const struct net_device *bridge_dev, + bool join) +{ + struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp; + u32 port_mask = BIT(dp->cpu_dp->index); + int i, ret; + + for (i = 0; i < QCA8K_NUM_PORTS; i++) { + if (i == port) + continue; + if (dsa_is_cpu_port(priv->ds, i)) + continue; + + other_dp = dsa_to_port(priv->ds, i); + if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev)) + continue; + + /* Add/remove this port to/from the portvlan mask of the other + * ports in the bridge + */ + if (join) { + port_mask |= BIT(i); + ret = regmap_set_bits(priv->regmap, + QCA8K_PORT_LOOKUP_CTRL(i), + BIT(port)); + } else { + ret = regmap_clear_bits(priv->regmap, + QCA8K_PORT_LOOKUP_CTRL(i), + BIT(port)); + } + + if (ret) + return ret; + } + + /* Add/remove all other ports to/from this port's portvlan mask */ + ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port), + QCA8K_PORT_LOOKUP_MEMBER, port_mask); + + return ret; +} + int qca8k_port_pre_bridge_flags(struct dsa_switch *ds, int port, struct switchdev_brport_flags flags, struct netlink_ext_ack *extack) @@ -646,65 +689,21 @@ int qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct netlink_ext_ack *extack) { struct qca8k_priv *priv = ds->priv; - int port_mask, cpu_port; - int i, ret; - - cpu_port = dsa_to_port(ds, port)->cpu_dp->index; - port_mask = BIT(cpu_port); - for (i = 0; i < QCA8K_NUM_PORTS; i++) { - if (i == port) - continue; - if (dsa_is_cpu_port(ds, i)) - continue; - if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge)) - continue; - /* Add this port to the portvlan mask of the other ports - * in the bridge - */ - ret = regmap_set_bits(priv->regmap, - QCA8K_PORT_LOOKUP_CTRL(i), - BIT(port)); - if (ret) - return ret; - port_mask |= BIT(i); - } - - /* Add all other ports to this ports portvlan mask */ - ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port), - QCA8K_PORT_LOOKUP_MEMBER, port_mask); - - return ret; + return qca8k_update_port_member(priv, port, bridge.dev, true); } void qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct dsa_bridge bridge) { struct qca8k_priv *priv = ds->priv; - int cpu_port, i; - - cpu_port = dsa_to_port(ds, port)->cpu_dp->index; + int err; - for (i = 0; i < QCA8K_NUM_PORTS; i++) { - if (i == port) - continue; - if (dsa_is_cpu_port(ds, i)) - continue; - if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge)) - continue; - /* Remove this port to the portvlan mask of the other ports - * in the bridge - */ - regmap_clear_bits(priv->regmap, - QCA8K_PORT_LOOKUP_CTRL(i), - BIT(port)); - } - - /* Set the cpu port to be the only one in the portvlan mask of - * this port - */ - qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port), - QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port)); + err = qca8k_update_port_member(priv, port, bridge.dev, false); + if (err) + dev_err(priv->dev, + "Failed to update switch config for bridge leave: %d\n", + err); } void qca8k_port_fast_age(struct dsa_switch *ds, int port)
Most of the logic in qca8k_port_bridge_join() and qca8k_port_bridge_leave() is the same. Refactor to reduce duplication and prepare for reusing the code for implementing bridge port isolation. dsa_port_offloads_bridge_dev() is used instead of dsa_port_offloads_bridge(), passing the bridge in as a struct netdevice *, as we won't have a struct dsa_bridge in qca8k_port_bridge_flags(). The error handling is changed slightly in the bridge leave case, returning early and emitting an error message when a regmap access fails. This shouldn't matter in practice, as there isn't much we can do if communication with the switch breaks down in the middle of reconfiguration. Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- drivers/net/dsa/qca/qca8k-common.c | 101 ++++++++++++++--------------- 1 file changed, 50 insertions(+), 51 deletions(-)