Message ID | 20230912122201.3752918-5-paweldembicki@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: dsa: vsc73xx: Make vsc73xx usable | expand |
Context | Check | Description |
---|---|---|
netdev/series_format | success | Posting correctly formatted |
netdev/tree_selection | success | Clearly marked for net-next |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | fail | Errors and warnings before: 1340 this patch: 17 |
netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
netdev/build_clang | fail | Errors and warnings before: 1363 this patch: 19 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/deprecated_api | success | None detected |
netdev/check_selftest | success | No net selftest shell script |
netdev/verify_fixes | success | No Fixes tag |
netdev/build_allmodconfig_warn | fail | Errors and warnings before: 1363 this patch: 17 |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 108 lines checked |
netdev/kdoc | success | Errors and warnings before: 8 this patch: 8 |
netdev/source_inline | success | Was 0 now: 0 |
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 <linus.walleij@linaro.org> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > --- > 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 >
wt., 12 wrz 2023 o 16:48 Vladimir Oltean <olteanv@gmail.com> 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@linaro.org> > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > --- > > 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@gmail.com/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/20230912122201.3752918-9-paweldembicki@gmail.com/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.
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 @@ -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); @@ -864,9 +865,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); } @@ -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) +{ + /* Configure forward map to CPU <-> port only */ + if (port == CPU_PORT) + vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & + ~BIT(CPU_PORT); + else + vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & + 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]); + 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 */ 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 {