wt., 12 wrz 2023 o 16:48 Vladimir Oltean <[email protected]> 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 <[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.
>
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/[email protected]/T/#u
On Tue, Sep 12, 2023 at 05:27:45PM +0200, Paweł Dembicki wrote:
> > 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].
>
> [0] https://lore.kernel.org/netdev/[email protected]/T/#u
Yes, but vsc73xx_port_stp_state_set() remains unchanged until the end.
What am I missing? In your implementation, nothing prevents port i
(which is in BR_STATE_FORWARDING) from forwarding packets towards a port j,
present in vsc->forward_map[i] & BIT(j), which is *not* in BR_STATE_FORWARDING.
If you don't have access to the STP protocol yet, you can put port j
down and it will go to the DISABLED state and you can confirm that other
ports in the bridge will still remain configured to forward to it.
> > > 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?
Yes, but please hold off posting it until I'm done reviewing this version.