Re: [PATCH net-next 2/3] net: dsa: qca8k: factor out bridge join/leave logic

[Wojciech Drewek <wojciech.drewek@intel.com>]

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(...);

> +}
> +

<...>