Hi Pawel,
On Tue, Sep 12, 2023 at 02:21:58PM +0200, 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 <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 8f2285a03e82..541fbc195df1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1033,9 +1031,59 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> return 9600 - ETH_HLEN - ETH_FCS_LEN;
> }
>
> +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> +{
For bisectability, the series must build patch by patch.
Here, you are missing:
struct vsc73xx *vsc = ds->priv;
../drivers/net/dsa/vitesse-vsc73xx-core.c:1038:3: error: use of undeclared identifier 'vsc'
vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
^
../drivers/net/dsa/vitesse-vsc73xx-core.c:1041:3: error: use of undeclared identifier 'vsc'
vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
^
2 errors generated.
> + /* Configure forward map to CPU <-> port only */
> + if (port == CPU_PORT)
> + vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> + ~BIT(CPU_PORT);
vsc->forward_map[CPU_PORT] = dsa_user_ports(ds);
> + else
> + vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> + BIT(CPU_PORT);
vsc->forward_map[port] = BIT(CPU_PORT);
> +
> + return 0;
> +}
> +
> +/* 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.
> + */
> +
> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + struct vsc73xx *vsc = ds->priv;
> +
> + if (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED)
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_RECVMASK, BIT(port), 0);
> + else
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_RECVMASK, BIT(port), BIT(port));
> +
> + if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_LEARNMASK, BIT(port), BIT(port));
> + else
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_LEARNMASK, BIT(port), 0);
> +
> + if (state == BR_STATE_FORWARDING)
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK,
> + vsc->forward_map[port]);
To forward a packet between port A and port B, both of them must be in
BR_STATE_FORWARDING, not just A.
> + else
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +}
> +
> static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .get_tag_protocol = vsc73xx_get_tag_protocol,
> .setup = vsc73xx_setup,
> + .port_setup = vsc73xx_port_setup,
> .phy_read = vsc73xx_phy_read,
> .phy_write = vsc73xx_phy_write,
> .phylink_get_caps = vsc73xx_phylink_get_caps,
> @@ -1049,6 +1097,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,
> };
>
> static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index f79d81ef24fb..224e284a5573 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -18,6 +18,7 @@
>
> /**
> * struct vsc73xx - VSC73xx state container
> + * @forward_map: Forward table cache
If you start describing the member fields, shouldn't all be described?
I think there will be kdoc warnings otherwise.
> */
> struct vsc73xx {
> struct device *dev;
> @@ -28,6 +29,7 @@ struct vsc73xx {
> u8 addr[ETH_ALEN];
> const struct vsc73xx_ops *ops;
> void *priv;
> + u8 forward_map[VSC73XX_MAX_NUM_PORTS];
> };
>
> struct vsc73xx_ops {
> --
> 2.34.1
>