Messages in this thread Patch in this message | | | Date | Thu, 15 Feb 2024 17:31:21 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v4 06/15] net: dsa: vsc73xx: add port_stp_state_set function |
| |
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 >
| |