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

[Vladimir Oltean]

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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>

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