Re: [PATCH net-next v4 06/15] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Feb 13, 2024 at 11:03:19PM +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.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v4:
>   - fully reworked port_stp_state_set

A "fully reworked" patch isn't exactly compatible with keeping attached
Reviewed-by tags attached to the patch. It implies Linus didn't actually
review the code you are submitting now.

> 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 | 83 ++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 75597daaad17..1dca3c476fac 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -163,6 +163,10 @@
>  #define VSC73XX_AGENCTRL	0xf0
>  #define VSC73XX_CAPRST		0xff
>  
> +#define VSC73XX_SRCMASKS_CPU_COPY		BIT(27)
> +#define VSC73XX_SRCMASKS_MIRROR			BIT(26)
> +#define VSC73XX_SRCMASKS_PORTS_MASK		GENMASK(7, 0)
> +
>  #define VSC73XX_MACACCESS_CPU_COPY		BIT(14)
>  #define VSC73XX_MACACCESS_FWD_KILL		BIT(13)
>  #define VSC73XX_MACACCESS_IGNORE_VLAN		BIT(12)
> @@ -619,9 +623,6 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
>  		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
>  		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> -	/* Enable reception of frames on all ports */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
> -		      0x5f);
>  	/* IP multicast flood mask (table 144) */
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
>  		      0xff);
> @@ -841,9 +842,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  	if (duplex == DUPLEX_FULL)
>  		val |= VSC73XX_MAC_CFG_FDX;
>  
> -	/* Enable port (forwarding) in the receieve mask */
> -	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> -			    VSC73XX_RECVMASK, BIT(port), BIT(port));
>  	vsc73xx_adjust_enable_port(vsc, port, val);
>  }
>  
> @@ -1026,6 +1024,78 @@ 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, bool configure)

Why have you chosen the name "bool configure" exactly? It is not
intuitive. Could you just pass the STP state of the port as an argument?

> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct vsc73xx *vsc = ds->priv;
> +	int i;
> +
> +	if (configure) {
> +		u16 mask = BIT(CPU_PORT);
> +
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
> +
> +		if (dp->bridge) {
> +			struct dsa_port *other_dp;
> +
> +			dsa_switch_for_each_user_port(other_dp, ds) {
> +				if (other_dp->bridge == dp->bridge &&
> +				    other_dp->index != port &&
> +				    other_dp->stp_state == BR_STATE_FORWARDING) {
> +					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));
> +				}
> +			}
> +		}
> +
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK, mask);
> +	} else {
> +		for (i = 0; i < vsc->ds->num_ports; i++) {

Try to refrain from doing plain integer iterations through ports when
there are holes in the port indices. Use dsa_switch_for_each_available_port()
which skips the unused/absent ports, and consequently you're not
touching unwanted registers.

> +			if (i == port)
> +				vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +						    VSC73XX_SRCMASKS + i,
> +						    VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +			else
> +				vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +						    VSC73XX_SRCMASKS + i, BIT(port), 0);
> +		}
> +	}

To save on indentation, I would simplify the big if () else () block
like this.

	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 (dp == other_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) {
		struct dsa_port *other_dp;

		dsa_switch_for_each_user_port(other_dp, ds) {
			if (other_dp->bridge == dp->bridge &&
			    other_dp->index != port &&
			    other_dp->stp_state == BR_STATE_FORWARDING) {
				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));
			}
		}
	}

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

> +}
> +
> +/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
> + * forwarded only from and to PI/SI interface. For more info see chapter
> + * 2.7.1 (CPU Forwarding) in datasheet.
> + * This function is required for tag8021q operations.

Please keep the name consistent at "tag_8021q".

> + */
> +

nitpick: remove the blank line.

> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u32 val;
> +
> +	val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ?
> +	      0 : BIT(port);
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +			    VSC73XX_RECVMASK, BIT(port), val);
> +
> +	val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ?
> +	      BIT(port) : 0;
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +			    VSC73XX_LEARNMASK, BIT(port), val);

I am extremely particular about this. Please also implement .port_pre_bridge_flags()
and .port_bridge_flags(), and treat the BR_LEARNING flag. The idea is
that standalone ports are in the BR_STATE_FORWARDING state (it's a DSA
API thing), but learning should _not_ be enabled (it breaks things when
one port sees traffic originated from the other). So you need to set the
bit in VSC73XX_LEARNMASK only if the port is in the BR_STATE_LEARNING ||
BR_STATE_FORWARDING, _and_ the BR_LEARNING flag is set.

This should be relatively easy to do. If you're not going to do it now
because of patch set size constraints, at least leave a big FIXME in the
code or in the commit message.

> +
> +	/* CPU Port should always forward packets when user ports are forwarding
> +	 * so let's configure it from other ports only.
> +	 */
> +	if (port != CPU_PORT)
> +		vsc73xx_refresh_fwd_map(ds, port, state == BR_STATE_FORWARDING);
> +}
> +
>  static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_tag_protocol = vsc73xx_get_tag_protocol,
>  	.setup = vsc73xx_setup,
> @@ -1041,6 +1111,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_disable = vsc73xx_port_disable,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
> +	.port_stp_state_set = vsc73xx_port_stp_state_set,
>  	.phylink_get_caps = vsc73xx_phylink_get_caps,
>  };
>  
> -- 
> 2.34.1
>