Re: [PATCH net-next v6 06/16] net: dsa: vsc73xx: add port_stp_state_set function

Vladimir Oltean
2024-03-08T09:54:47Z

On Fri, Mar 01, 2024 at 11:16:28PM +0100, Pawel Dembicki wrote:
> This isn't a fully functional implementation of 802.1D, but
> port_stp_state_set is required for a future tag8021q operations.
>
> This implementation handles properly all states, but vsc73xx doesn't
> forward STP packets.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v6:
> - fix inconsistent indenting
> v5:
> - remove unneeded 'RECVMASK' operations
> - reorganise vsc73xx_refresh_fwd_map function
> v4:
> - fully reworked port_stp_state_set
> v3:
> - use 'VSC73XX_MAX_NUM_PORTS' define
> - add 'state == BR_STATE_DISABLED' condition
> - fix style issues
> v2:
> - fix kdoc
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 99 +++++++++++++++++++++++---
> 1 file changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 425999d7bf41..d1e84a9a83d1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1036,6 +1029,89 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
> }
>
> +static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
> +{
> + struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
> + struct vsc73xx *vsc = ds->priv;
> + u16 mask;
> +
> + if (state != BR_STATE_FORWARDING) {
> + /* Ports that aren't in the forwarding state must not
> + * forward packets anywhere.
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +
> + dsa_switch_for_each_available_port(other_dp, ds) {
> + if (other_dp == dp)
> + continue;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + other_dp->index,
> + BIT(port), 0);
> + }
> +
> + return;
> + }
> +
> + /* Forwarding ports must forward to the CPU and to other ports
> + * in the same bridge
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
> +
> + mask = BIT(CPU_PORT);
> +
> + if (dp->bridge) {
> + dsa_switch_for_each_user_port(other_dp, ds) {
> + if (other_dp->bridge == dp->bridge &&

You could use dsa_port_bridge_same(dp, other_dp) and that could
eliminate the extra "if (dp->bridge)" condition, because it explicitly
makes standalone ports isolated from other standalone ports.

> + other_dp->index != port &&

You could move the "int other_port" definition to dsa_switch_for_each_user_port()
scope, and thus reuse it here.

> + other_dp->stp_state == BR_STATE_FORWARDING) {

You could "continue" on the negated condition, and reduce the
indentation one level further.

> + int other_port = other_dp->index;
> +
> + mask |= BIT(other_port);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER,
> + 0,
> + VSC73XX_SRCMASKS +
> + other_port,
> + BIT(port), BIT(port));
> + }
> + }
> + }

All in all, I would have written this as:

dsa_switch_for_each_user_port(other_dp, ds) {
int other_port = other_dp->index;

if (port == other_port || !dsa_port_bridge_same(dp, other_dp) ||
other_dp->stp_state != BR_STATE_FORWARDING)
continue;

mask |= BIT(other_port);

vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_SRCMASKS + other_port,
BIT(port), BIT(port));
}

Anyway this does not affect functionality, and it is up to you if you
integrate these suggestions or not.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, mask);
> +}