Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function

[Paweł Dembicki]

wt., 12 wrz 2023 o 16:48 Vladimir Oltean <olteanv@xxxxxxxxx> napisał(a):
>
> Hi Pawel,
>
Hi Vladimir,

Thank you for Your time.

> 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 <linus.walleij@xxxxxxxxxx>
> > Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>
> > ---
> > 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.
>

In this patch bridges are unimplemented. Please look at 8/8 patch [0].

> > +     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.
>

Jakub in v1 series points kdoc warn in this case. I added a
description to the field added by me. Should I prepare in the v4
series a separate commit for other descriptions in this struct?

> >   */
> >  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
> >

[0] https://lore.kernel.org/netdev/20230912122201.3752918-9-paweldembicki@xxxxxxxxx/T/#u