Re: [PATCH net-next v4 06/15] net: dsa: vsc73xx: add port_stp_state_set function
From: Vladimir Oltean
Date: Thu Feb 15 2024 - 10:34:52 EST
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@xxxxxxxxxx>
> Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>
> ---
> 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
>