* [PATCH net-next v3 1/8] net: dsa: vsc73xx: use read_poll_timeout instead delay loop
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
@ 2023-09-12 12:21 ` Pawel Dembicki
2023-09-12 12:21 ` [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK Pawel Dembicki
` (6 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:21 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Russell King,
Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
This commit switches delay loop to read_poll_timeout macro during
Arbiter empty check in adjust link function.
As Russel King suggested:
"This [change] avoids the issue that on the last iteration, the code reads
the register, test it, find the condition that's being waiting for is
false, _then_ waits and end up printing the error message - that last
wait is rather useless, and as the arbiter state isn't checked after
waiting, it could be that we had success during the last wait."
It also remove one short msleep delay.
Suggested-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
v3:
- Add "Reviewed-by" to commit message only
v2:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 4f09e7438f3b..b117c0c18e36 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -779,7 +779,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
* after a PHY or the CPU port comes up or down.
*/
if (!phydev->link) {
- int maxloop = 10;
+ int ret, err;
dev_dbg(vsc->dev, "port %d: went down\n",
port);
@@ -794,19 +794,16 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
VSC73XX_ARBDISC, BIT(port), BIT(port));
/* Wait until queue is empty */
- vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- while (!(val & BIT(port))) {
- msleep(1);
- vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- if (--maxloop == 0) {
- dev_err(vsc->dev,
- "timeout waiting for block arbiter\n");
- /* Continue anyway */
- break;
- }
- }
+ ret = read_poll_timeout(vsc73xx_read, err,
+ err < 0 || (val & BIT(port)),
+ 1000, 10000, false,
+ vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBEMPTY, &val);
+ if (ret)
+ dev_err(vsc->dev,
+ "timeout waiting for block arbiter\n");
+ else if (err < 0)
+ dev_err(vsc->dev, "error reading arbiter\n");
/* Put this port into reset */
vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
2023-09-12 12:21 ` [PATCH net-next v3 1/8] net: dsa: vsc73xx: use read_poll_timeout instead delay loop Pawel Dembicki
@ 2023-09-12 12:21 ` Pawel Dembicki
2023-09-12 16:49 ` Russell King (Oracle)
2023-09-12 12:21 ` [PATCH net-next v3 3/8] net: dsa: vsc73xx: Add define for max num of ports Pawel Dembicki
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:21 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Linus Walleij,
Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King,
linux-kernel
This patch replaces the adjust_link api with the phylink apis that provide
equivalent functionality.
The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_* and phylink_mac_config.
Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_down
.phylink_mac_link_up
.phylink_mac_link_down
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- remove legacy_pre_march2020 after rebase
v2:
- replace switch to if and get rid of macros in
vsc73xx_phylink_mac_link_up function
drivers/net/dsa/vitesse-vsc73xx-core.c | 190 +++++++++++++------------
1 file changed, 96 insertions(+), 94 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index b117c0c18e36..39d3d78f4bc3 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -714,8 +714,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
}
static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
- int port, struct phy_device *phydev,
- u32 initval)
+ int port, u32 initval)
{
u32 val = initval;
u8 seed;
@@ -753,12 +752,34 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
}
-static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
+static void vsc73xx_phylink_get_caps(struct dsa_switch *ds, int port,
+ struct phylink_config *config)
{
- struct vsc73xx *vsc = ds->priv;
- u32 val;
+ /* This switch only supports full-duplex at 1Gbps */
+ config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000FD |
+ MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
+ if (port == CPU_PORT) {
+ __set_bit(PHY_INTERFACE_MODE_RGMII,
+ config->supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_GMII,
+ config->supported_interfaces);
+ } else {
+ __set_bit(PHY_INTERFACE_MODE_INTERNAL,
+ config->supported_interfaces);
+ /* Compatibility for phylib's default interface type when the
+ * phy-mode property is absent
+ */
+ __set_bit(PHY_INTERFACE_MODE_GMII,
+ config->supported_interfaces);
+ }
+}
+
+static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ const struct phylink_link_state *state)
+{
+ struct vsc73xx *vsc = ds->priv;
/* Special handling of the CPU-facing port */
if (port == CPU_PORT) {
/* Other ports are already initialized but not this one */
@@ -774,101 +795,79 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
VSC73XX_ADVPORTM_ENA_GTX |
VSC73XX_ADVPORTM_DDR_MODE);
}
+}
- /* This is the MAC confiuration that always need to happen
- * after a PHY or the CPU port comes up or down.
- */
- if (!phydev->link) {
- int ret, err;
-
- dev_dbg(vsc->dev, "port %d: went down\n",
- port);
-
- /* Disable RX on this port */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
- VSC73XX_MAC_CFG,
- VSC73XX_MAC_CFG_RX_EN, 0);
-
- /* Discard packets */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBDISC, BIT(port), BIT(port));
-
- /* Wait until queue is empty */
- ret = read_poll_timeout(vsc73xx_read, err,
- err < 0 || (val & BIT(port)),
- 1000, 10000, false,
- vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- if (ret)
- dev_err(vsc->dev,
- "timeout waiting for block arbiter\n");
- else if (err < 0)
- dev_err(vsc->dev, "error reading arbiter\n");
-
- /* Put this port into reset */
- vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
- VSC73XX_MAC_CFG_RESET);
-
- /* Accept packets again */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBDISC, BIT(port), 0);
-
- /* Allow backward dropping of frames from this port */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_SBACKWDROP, BIT(port), BIT(port));
-
- /* Receive mask (disable forwarding) */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
- VSC73XX_RECVMASK, BIT(port), 0);
+static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface)
+{
+ struct vsc73xx *vsc = ds->priv;
+ int ret, err;
+ u32 val;
- return;
- }
+ /* Disable RX on this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RX_EN, 0);
- /* Figure out what speed was negotiated */
- if (phydev->speed == SPEED_1000) {
- dev_dbg(vsc->dev, "port %d: 1000 Mbit mode full duplex\n",
- port);
-
- /* Set up default for internal port or external RGMII */
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
- val = VSC73XX_MAC_CFG_1000M_F_RGMII;
- else
- val = VSC73XX_MAC_CFG_1000M_F_PHY;
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else if (phydev->speed == SPEED_100) {
- if (phydev->duplex == DUPLEX_FULL) {
- val = VSC73XX_MAC_CFG_100_10M_F_PHY;
- dev_dbg(vsc->dev,
- "port %d: 100 Mbit full duplex mode\n",
- port);
- } else {
- val = VSC73XX_MAC_CFG_100_10M_H_PHY;
- dev_dbg(vsc->dev,
- "port %d: 100 Mbit half duplex mode\n",
- port);
- }
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else if (phydev->speed == SPEED_10) {
- if (phydev->duplex == DUPLEX_FULL) {
- val = VSC73XX_MAC_CFG_100_10M_F_PHY;
- dev_dbg(vsc->dev,
- "port %d: 10 Mbit full duplex mode\n",
- port);
- } else {
- val = VSC73XX_MAC_CFG_100_10M_H_PHY;
- dev_dbg(vsc->dev,
- "port %d: 10 Mbit half duplex mode\n",
- port);
- }
- vsc73xx_adjust_enable_port(vsc, port, phydev, val);
- } else {
+ /* Discard packets */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBDISC, BIT(port), BIT(port));
+
+ /* Wait until queue is empty */
+ ret = read_poll_timeout(vsc73xx_read, err, err < 0 || (val & BIT(port)),
+ 1000, 10000, false, vsc, VSC73XX_BLOCK_ARBITER,
+ 0, VSC73XX_ARBEMPTY, &val);
+ if (ret)
dev_err(vsc->dev,
- "could not adjust link: unknown speed\n");
- }
+ "timeout waiting for block arbiter\n");
+ else if (err < 0)
+ dev_err(vsc->dev, "error reading arbiter\n");
+
+ /* Put this port into reset */
+ vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RESET);
+
+ /* Accept packets again */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBDISC, BIT(port), 0);
+
+ /* Allow backward dropping of frames from this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_SBACKWDROP, BIT(port), BIT(port));
+
+ /* Receive mask (disable forwarding) */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_RECVMASK, BIT(port), 0);
+}
+
+static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev,
+ int speed, int duplex,
+ bool tx_pause, bool rx_pause)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u32 val;
+
+ if (speed == SPEED_1000)
+ val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
+ else
+ val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
+
+ if (interface == PHY_INTERFACE_MODE_RGMII)
+ val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
+ else
+ val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
+
+ 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);
}
static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
@@ -1039,7 +1038,10 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.setup = vsc73xx_setup,
.phy_read = vsc73xx_phy_read,
.phy_write = vsc73xx_phy_write,
- .adjust_link = vsc73xx_adjust_link,
+ .phylink_get_caps = vsc73xx_phylink_get_caps,
+ .phylink_mac_config = vsc73xx_phylink_mac_config,
+ .phylink_mac_link_down = vsc73xx_phylink_mac_link_down,
+ .phylink_mac_link_up = vsc73xx_phylink_mac_link_up,
.get_strings = vsc73xx_get_strings,
.get_ethtool_stats = vsc73xx_get_ethtool_stats,
.get_sset_count = vsc73xx_get_sset_count,
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-09-12 12:21 ` [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK Pawel Dembicki
@ 2023-09-12 16:49 ` Russell King (Oracle)
2023-09-26 23:03 ` Vladimir Oltean
0 siblings, 1 reply; 27+ messages in thread
From: Russell King (Oracle) @ 2023-09-12 16:49 UTC (permalink / raw)
To: Pawel Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Linus Walleij, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-kernel
On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev,
> + int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
> +
> + if (speed == SPEED_1000)
> + val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
> + else
> + val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
> +
> + if (interface == PHY_INTERFACE_MODE_RGMII)
> + val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
> + else
> + val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
is this correct, or should it be:
if (phy_interface_is_rgmii(interface))
since the various RGMII* modes are used to determine the delay on the
PHY side.
Even so, I don't think that is a matter for this patch, but a future
(or maybe a preceeding patch) to address.
Other than that, I think it looks okay.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-09-12 16:49 ` Russell King (Oracle)
@ 2023-09-26 23:03 ` Vladimir Oltean
2023-10-03 20:45 ` Paweł Dembicki
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-26 23:03 UTC (permalink / raw)
To: Pawel Dembicki, Russell King (Oracle)
Cc: netdev, Dan Carpenter, Simon Horman, Linus Walleij, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel
On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface,
> > + struct phy_device *phydev,
> > + int speed, int duplex,
> > + bool tx_pause, bool rx_pause)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u32 val;
> > +
> > + if (speed == SPEED_1000)
> > + val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
> > + else
> > + val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
> > +
> > + if (interface == PHY_INTERFACE_MODE_RGMII)
> > + val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
> > + else
> > + val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
>
> I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> is this correct, or should it be:
>
> if (phy_interface_is_rgmii(interface))
>
> since the various RGMII* modes are used to determine the delay on the
> PHY side.
>
> Even so, I don't think that is a matter for this patch, but a future
> (or maybe a preceeding patch) to address.
>
> Other than that, I think it looks okay.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
I also agree with adding one more patch to this which converts to
phy_interface_is_rgmii(). Paweł: there was a recent discussion about
the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-09-26 23:03 ` Vladimir Oltean
@ 2023-10-03 20:45 ` Paweł Dembicki
2023-10-03 21:14 ` Vladimir Oltean
2023-10-03 21:32 ` Andrew Lunn
0 siblings, 2 replies; 27+ messages in thread
From: Paweł Dembicki @ 2023-10-03 20:45 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Russell King (Oracle), netdev, Dan Carpenter, Simon Horman,
Linus Walleij, Andrew Lunn, Florian Fainelli, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
śr., 27 wrz 2023 o 01:03 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> > On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > > +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > + unsigned int mode,
> > > + phy_interface_t interface,
> > > + struct phy_device *phydev,
> > > + int speed, int duplex,
> > > + bool tx_pause, bool rx_pause)
> > > +{
> > > + struct vsc73xx *vsc = ds->priv;
> > > + u32 val;
> > > +
> > > + if (speed == SPEED_1000)
> > > + val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
> > > + else
> > > + val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
> > > +
> > > + if (interface == PHY_INTERFACE_MODE_RGMII)
> > > + val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
> > > + else
> > > + val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
> >
> > I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> > is this correct, or should it be:
> >
> > if (phy_interface_is_rgmii(interface))
> >
> > since the various RGMII* modes are used to determine the delay on the
> > PHY side.
> >
> > Even so, I don't think that is a matter for this patch, but a future
> > (or maybe a preceeding patch) to address.
> >
> > Other than that, I think it looks okay.
> >
> > Thanks.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>
> I also agree with adding one more patch to this which converts to
> phy_interface_is_rgmii(). Paweł: there was a recent discussion about
> the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
> https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/
I plan to make rgmii delays configurable from the device tree. Should I?
a. switch to phy_interface_is_rgmii in the current patch?
b. add another patch in this series?
c. wait with change to phy_interface_is_rgmii for patch with rgmii
delays configuration?
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-10-03 20:45 ` Paweł Dembicki
@ 2023-10-03 21:14 ` Vladimir Oltean
2023-10-03 21:32 ` Andrew Lunn
1 sibling, 0 replies; 27+ messages in thread
From: Vladimir Oltean @ 2023-10-03 21:14 UTC (permalink / raw)
To: Paweł Dembicki
Cc: Russell King (Oracle), netdev, Dan Carpenter, Simon Horman,
Linus Walleij, Andrew Lunn, Florian Fainelli, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
Hi Paweł,
On Tue, Oct 03, 2023 at 10:45:45PM +0200, Paweł Dembicki wrote:
> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?
If you want to configure the RGMII delays in the vsc73xx MAC, you should
look at the "rx-internal-delay-ps" and "tx-internal-delay-ps" properties
in the MAC OF node, rather than at the phy-mode. The phy-mode is for the
internal delays from the PHY.
In any case, you should accept any phy_interface_is_rgmii() regardless
of whether internal delays are configurable in the MAC. And yes, parsing
those MAC OF properties should be a separate change.
If the series exceeds 15 patches, I would consider splitting it per
topic and submitting separate series (link management would be one,
tag_8021q/bridging/VLAN would be another). If they don't conflict and
can be applied independently, you could also send the 2 series
simultaneously.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-10-03 20:45 ` Paweł Dembicki
2023-10-03 21:14 ` Vladimir Oltean
@ 2023-10-03 21:32 ` Andrew Lunn
2023-10-03 21:50 ` Paweł Dembicki
1 sibling, 1 reply; 27+ messages in thread
From: Andrew Lunn @ 2023-10-03 21:32 UTC (permalink / raw)
To: Paweł Dembicki
Cc: Vladimir Oltean, Russell King (Oracle), netdev, Dan Carpenter,
Simon Horman, Linus Walleij, Florian Fainelli, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?
Do you actually need this feature? Does the PHY you are using already
support fine tuning of the delays?
Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK
2023-10-03 21:32 ` Andrew Lunn
@ 2023-10-03 21:50 ` Paweł Dembicki
0 siblings, 0 replies; 27+ messages in thread
From: Paweł Dembicki @ 2023-10-03 21:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vladimir Oltean, Russell King (Oracle), netdev, Dan Carpenter,
Simon Horman, Linus Walleij, Florian Fainelli, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
wt., 3 paź 2023 o 23:32 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> > I plan to make rgmii delays configurable from the device tree. Should I?
> > a. switch to phy_interface_is_rgmii in the current patch?
> > b. add another patch in this series?
> > c. wait with change to phy_interface_is_rgmii for patch with rgmii
> > delays configuration?
>
> Do you actually need this feature? Does the PHY you are using already
> support fine tuning of the delays?
>
After Vladimir answer I know that it should be a separate change.
I need it for MAC <-> switch connection, the rgmii port connected to
the cpu. At this moment, rgmii delays are hardcoded in vsc73xx_setup
and it should be tunable for the p2020rdb board. I plan to work on it
after the driver becomes usable.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next v3 3/8] net: dsa: vsc73xx: Add define for max num of ports
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
2023-09-12 12:21 ` [PATCH net-next v3 1/8] net: dsa: vsc73xx: use read_poll_timeout instead delay loop Pawel Dembicki
2023-09-12 12:21 ` [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK Pawel Dembicki
@ 2023-09-12 12:21 ` Pawel Dembicki
2023-09-12 12:21 ` [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function Pawel Dembicki
` (4 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:21 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Vladimir Oltean,
Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel
This patch introduces a new define: VSC73XX_MAX_NUM_PORTS, which can be
used in the future instead of a hardcoded value.
Currently, the only hardcoded value is vsc->ds->num_ports. It is being
replaced with the new define.
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- Introduce patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 13 +------------
drivers/net/dsa/vitesse-vsc73xx.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 39d3d78f4bc3..8f2285a03e82 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1175,23 +1175,12 @@ int vsc73xx_probe(struct vsc73xx *vsc)
vsc->addr[0], vsc->addr[1], vsc->addr[2],
vsc->addr[3], vsc->addr[4], vsc->addr[5]);
- /* The VSC7395 switch chips have 5+1 ports which means 5
- * ordinary ports and a sixth CPU port facing the processor
- * with an RGMII interface. These ports are numbered 0..4
- * and 6, so they leave a "hole" in the port map for port 5,
- * which is invalid.
- *
- * The VSC7398 has 8 ports, port 7 is again the CPU port.
- *
- * We allocate 8 ports and avoid access to the nonexistant
- * ports.
- */
vsc->ds = devm_kzalloc(dev, sizeof(*vsc->ds), GFP_KERNEL);
if (!vsc->ds)
return -ENOMEM;
vsc->ds->dev = dev;
- vsc->ds->num_ports = 8;
+ vsc->ds->num_ports = VSC73XX_MAX_NUM_PORTS;
vsc->ds->priv = vsc;
vsc->ds->ops = &vsc73xx_ds_ops;
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index 30b1f0a36566..f79d81ef24fb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -3,6 +3,19 @@
#include <linux/etherdevice.h>
#include <linux/gpio/driver.h>
+/* The VSC7395 switch chips have 5+1 ports which means 5
+ * ordinary ports and a sixth CPU port facing the processor
+ * with an RGMII interface. These ports are numbered 0..4
+ * and 6, so they leave a "hole" in the port map for port 5,
+ * which is invalid.
+ *
+ * The VSC7398 has 8 ports, port 7 is again the CPU port.
+ *
+ * We allocate 8 ports and avoid access to the nonexistant
+ * ports.
+ */
+#define VSC73XX_MAX_NUM_PORTS 8
+
/**
* struct vsc73xx - VSC73xx state container
*/
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
` (2 preceding siblings ...)
2023-09-12 12:21 ` [PATCH net-next v3 3/8] net: dsa: vsc73xx: Add define for max num of ports Pawel Dembicki
@ 2023-09-12 12:21 ` Pawel Dembicki
2023-09-12 14:48 ` Vladimir Oltean
2023-09-12 12:21 ` [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering Pawel Dembicki
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:21 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Linus Walleij,
Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King,
linux-kernel
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>
---
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 | 61 +++++++++++++++++++++++---
drivers/net/dsa/vitesse-vsc73xx.h | 2 +
2 files changed, 57 insertions(+), 6 deletions(-)
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 {
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function
2023-09-12 12:21 ` [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function Pawel Dembicki
@ 2023-09-12 14:48 ` Vladimir Oltean
2023-09-12 15:27 ` Paweł Dembicki
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-12 14:48 UTC (permalink / raw)
To: Pawel Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Linus Walleij, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function
2023-09-12 14:48 ` Vladimir Oltean
@ 2023-09-12 15:27 ` Paweł Dembicki
2023-09-12 15:42 ` Vladimir Oltean
0 siblings, 1 reply; 27+ messages in thread
From: Paweł Dembicki @ 2023-09-12 15:27 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Dan Carpenter, Simon Horman, Linus Walleij, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function
2023-09-12 15:27 ` Paweł Dembicki
@ 2023-09-12 15:42 ` Vladimir Oltean
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-12 15:42 UTC (permalink / raw)
To: Paweł Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Linus Walleij, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
` (3 preceding siblings ...)
2023-09-12 12:21 ` [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function Pawel Dembicki
@ 2023-09-12 12:21 ` Pawel Dembicki
2023-09-12 16:17 ` Vladimir Oltean
2023-09-12 12:22 ` [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx Pawel Dembicki
` (2 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:21 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel
This patch implements VLAN filtering for the vsc73xx driver.
After starting VLAN filtering, the switch is reconfigured from QinQ to
a simple VLAN aware mode. This is required because VSC73XX chips do not
support inner VLAN tag filtering.
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- reworked all vlan commits
- added storage variables for pvid and untagged vlans
- move length extender settings to port setup
- remove vlan table cleaning in wrong places
- note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
v2:
- no changes done
drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++-
drivers/net/dsa/vitesse-vsc73xx.h | 2 +
2 files changed, 425 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 541fbc195df1..d9a6eac1fcce 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -21,14 +21,18 @@
#include <linux/of_mdio.h>
#include <linux/bitops.h>
#include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
#include <linux/etherdevice.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio/driver.h>
+#include <linux/dsa/8021q.h>
#include <linux/random.h>
#include <net/dsa.h>
#include "vitesse-vsc73xx.h"
+#define VSC73XX_IS_CONFIGURED 0x1
+
#define VSC73XX_BLOCK_MAC 0x1 /* Subblocks 0-4, 6 (CPU port) */
#define VSC73XX_BLOCK_ANALYZER 0x2 /* Only subblock 0 */
#define VSC73XX_BLOCK_MII 0x3 /* Subblocks 0 and 1 */
@@ -61,6 +65,8 @@
#define VSC73XX_CAT_DROP 0x6e
#define VSC73XX_CAT_PR_MISC_L2 0x6f
#define VSC73XX_CAT_PR_USR_PRIO 0x75
+#define VSC73XX_CAT_VLAN_MISC 0x79
+#define VSC73XX_CAT_PORT_VLAN 0x7a
#define VSC73XX_Q_MISC_CONF 0xdf
/* MAC_CFG register bits */
@@ -121,6 +127,17 @@
#define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1)
#define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0)
+/* TXUPDCFG transmit modify setup bits */
+#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19)
+#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18)
+#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3)
+#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1)
+#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0)
+
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
+
/* CAT_DROP categorizer frame dropping register bits */
#define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
#define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
@@ -134,6 +151,15 @@
#define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1)
#define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0)
+/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
+#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
+#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
+
+/* CAT_PORT_VLAN categorizer port VLAN*/
+#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
+
/* Frame analyzer block 2 registers */
#define VSC73XX_STORMLIMIT 0x02
#define VSC73XX_ADVLEARN 0x03
@@ -188,7 +214,8 @@
#define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
#define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
#define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
-#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
+#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
+#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
@@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
{ 29, "TxQoSClass3" }, /* non-standard counter */
};
+enum vsc73xx_port_vlan_conf {
+ VSC73XX_VLAN_FILTER,
+ VSC73XX_VLAN_IGNORE,
+};
+
int vsc73xx_is_addr_valid(u8 block, u8 subblock)
{
switch (block) {
@@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
static int vsc73xx_setup(struct dsa_switch *ds)
{
struct vsc73xx *vsc = ds->priv;
- int i;
+ int i, ret;
dev_info(vsc->dev, "set up the switch\n");
@@ -623,6 +655,9 @@ 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);
+ /* Ingess VLAN reception mask (table 145) */
+ vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
+ 0x5f);
/* IP multicast flood mask (table 144) */
vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
0xff);
@@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
return 9600 - ETH_HLEN - ETH_FCS_LEN;
}
+static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
+{
+ int ret, err;
+ u32 val;
+
+ ret = read_poll_timeout(vsc73xx_read, err,
+ err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
+ 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
+ 0, VSC73XX_VLANACCESS, &val);
+ if (ret)
+ return ret;
+ return err;
+}
+
+static int
+vsc73xx_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
+{
+ u32 val;
+ int ret;
+
+ vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+ vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
+ *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
+ VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
+ return 0;
+}
+
+static int
+vsc73xx_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
+{
+ int ret;
+
+ vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
+ VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+ VSC73XX_VLANACCESS_VLAN_PORT_MASK,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
+ VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+ (portmap <<
+ VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
+
+ return vsc73xx_wait_for_vlan_table_cmd(vsc);
+}
+
+static int
+vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+ u8 portmap;
+ int ret;
+
+ ret = vsc73xx_read_vlan_table_entry(vsc, vid, &portmap);
+ if (ret)
+ return ret;
+
+ if (set)
+ portmap |= BIT(port) | BIT(CPU_PORT);
+ else
+ portmap &= ~BIT(port);
+
+ if (portmap == BIT(CPU_PORT))
+ portmap = 0;
+
+ return vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
+}
+
+static void
+vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
+ enum vsc73xx_port_vlan_conf port_vlan_conf)
+{
+ if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+ VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
+ } else {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+ 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_INSERT_TAG,
+ VSC73XX_TXUPDCFG_TX_INSERT_TAG);
+ }
+}
+
+static int
+vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+ u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
+ (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
+ return 0;
+}
+
+static int
+vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+ u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID,
+ vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
+ return 0;
+}
+
+static int vsc73xx_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)
+{
+ u32 val;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
+ if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
+ return 0;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+ *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+
+ return VSC73XX_IS_CONFIGURED;
+}
+
+static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
+{
+ u32 val;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+ if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
+ return 0;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ &val);
+ *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+
+ return VSC73XX_IS_CONFIGURED;
+}
+
+static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
+ bool port_vlan)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no = 0;
+
+ if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {
+ if (vsc->untagged_storage[port] < VLAN_N_VID &&
+ !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
+ !vid_is_dsa_8021q(vid) &&
+ vsc->untagged_storage[port] != vid) {
+ dev_warn(vsc->dev,
+ "Port %d can have only one untagged vid! Now is: %d.\n",
+ port, vsc->untagged_storage[port]);
+ return -EOPNOTSUPP;
+ }
+ vsc->untagged_storage[port] = vid;
+ } else {
+ if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+ !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) && vlan_no != vid) {
+ dev_warn(vsc->dev,
+ "Port %d can have only one untagged vid! Now is: %d.\n",
+ port, vlan_no);
+ return -EOPNOTSUPP;
+ }
+ return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
+ }
+
+ return 0;
+}
+
+static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
+ bool port_vlan)
+{
+ struct dsa_port *dsa_port = dsa_to_port(ds, port);
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no;
+
+ if (!dsa_port)
+ return -EINVAL;
+
+ if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
+ if (vsc->pvid_storage[port] < VLAN_N_VID &&
+ !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
+ !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {
+ dev_warn(vsc->dev,
+ "Port %d can have only one pvid! Now is: %d.\n",
+ port, vsc->pvid_storage[port]);
+ return -EOPNOTSUPP;
+ }
+ vsc->pvid_storage[port] = vid;
+ } else {
+ if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+ !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
+ vlan_no != vid) {
+ dev_warn(vsc->dev,
+ "Port %d can have only one pvid! Now is: %d.\n",
+ port, vlan_no);
+ return -EOPNOTSUPP;
+ }
+ return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
+ }
+
+ return 0;
+}
+
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+ struct vsc73xx *vsc = ds->priv;
+ bool store_untagged = false;
+ bool store_pvid = false;
+ u16 vlan_no;
+
+ if (vlan_filtering)
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
+ else
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
+
+ if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
+ store_pvid = true;
+
+ if (vsc->pvid_storage[port] < VLAN_N_VID)
+ vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
+ else
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+
+ vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
+
+ if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
+ store_untagged = true;
+
+ if (vsc->untagged_storage[port] < VLAN_N_VID)
+ vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
+ else
+ vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+
+ vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_VID;
+
+ return 0;
+}
+
+static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct netlink_ext_ack *extack)
+{
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no;
+ int ret;
+
+ /* Be sure to deny alterations to the configuration done by tag_8021q.
+ */
+ if (vid_is_dsa_8021q(vlan->vid)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Range 3072-4095 reserved for dsa_8021q operation");
+ return -EBUSY;
+ }
+
+ if (port != CPU_PORT) {
+ if (untagged) {
+ ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ } else {
+ if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
+ == VSC73XX_IS_CONFIGURED &&
+ vlan_no == vlan->vid)
+ vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+ else if (vsc->untagged_storage[port] == vlan->vid)
+ vsc->untagged_storage[port] = VLAN_N_VID;
+ }
+ if (pvid) {
+ ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ } else {
+ if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
+ == VSC73XX_IS_CONFIGURED &&
+ vlan_no == vlan->vid)
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+ else if (vsc->pvid_storage[port] == vlan->vid)
+ vsc->pvid_storage[port] = VLAN_N_VID;
+ }
+ }
+
+ return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);
+}
+
+static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no;
+ int ret;
+
+ ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
+ if (ret)
+ return ret;
+
+ if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+ vlan_no == vlan->vid)
+ vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+ else if (vsc->untagged_storage[port] == vlan->vid)
+ vsc->untagged_storage[port] = VLAN_N_VID;
+
+ if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+ vlan_no == vlan->vid)
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+ else if (vsc->pvid_storage[port] == vlan->vid)
+ vsc->pvid_storage[port] = VLAN_N_VID;
+
+ return 0;
+}
+
static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
{
+ struct vsc73xx *vsc = ds->priv;
+ int ret, i;
+
+ /* Those bits are responsible for MTU only. Kernel take care about MTU,
+ * let's enable +8 bytes frame length unconditionally.
+ */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
+ VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_TAGGED_ENA, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+
+ if (port == CPU_PORT)
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
+ else
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
+
+ for (i = 0; i <= VLAN_N_VID; i++) {
+ ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
+ if (ret)
+ return ret;
+ }
+
/* Configure forward map to CPU <-> port only */
if (port == CPU_PORT)
vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
@@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
BIT(CPU_PORT);
+ /* Set storage values out of range*/
+ vsc->pvid_storage[port] = VLAN_N_VID;
+ vsc->untagged_storage[port] = VLAN_N_VID;
+
return 0;
}
@@ -1098,6 +1516,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_change_mtu = vsc73xx_change_mtu,
.port_max_mtu = vsc73xx_get_max_mtu,
.port_stp_state_set = vsc73xx_port_stp_state_set,
+ .port_vlan_filtering = vsc73xx_port_vlan_filtering,
+ .port_vlan_add = vsc73xx_port_vlan_add,
+ .port_vlan_del = vsc73xx_port_vlan_del,
};
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 224e284a5573..b7614dd7d0eb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -30,6 +30,8 @@ struct vsc73xx {
const struct vsc73xx_ops *ops;
void *priv;
u8 forward_map[VSC73XX_MAX_NUM_PORTS];
+ u16 pvid_storage[VSC73XX_MAX_NUM_PORTS];
+ u16 untagged_storage[VSC73XX_MAX_NUM_PORTS];
};
struct vsc73xx_ops {
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-09-12 12:21 ` [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering Pawel Dembicki
@ 2023-09-12 16:17 ` Vladimir Oltean
2023-09-22 14:26 ` Paweł Dembicki
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-12 16:17 UTC (permalink / raw)
To: Pawel Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
On Tue, Sep 12, 2023 at 02:21:59PM +0200, Pawel Dembicki wrote:
> This patch implements VLAN filtering for the vsc73xx driver.
>
> After starting VLAN filtering, the switch is reconfigured from QinQ to
> a simple VLAN aware mode. This is required because VSC73XX chips do not
> support inner VLAN tag filtering.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v3:
> - reworked all vlan commits
> - added storage variables for pvid and untagged vlans
> - move length extender settings to port setup
> - remove vlan table cleaning in wrong places
> - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
> and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
> v2:
> - no changes done
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++-
> drivers/net/dsa/vitesse-vsc73xx.h | 2 +
> 2 files changed, 425 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 541fbc195df1..d9a6eac1fcce 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -21,14 +21,18 @@
> #include <linux/of_mdio.h>
> #include <linux/bitops.h>
> #include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
> #include <linux/etherdevice.h>
> #include <linux/gpio/consumer.h>
> #include <linux/gpio/driver.h>
> +#include <linux/dsa/8021q.h>
> #include <linux/random.h>
> #include <net/dsa.h>
>
> #include "vitesse-vsc73xx.h"
>
> +#define VSC73XX_IS_CONFIGURED 0x1
> +
> #define VSC73XX_BLOCK_MAC 0x1 /* Subblocks 0-4, 6 (CPU port) */
> #define VSC73XX_BLOCK_ANALYZER 0x2 /* Only subblock 0 */
> #define VSC73XX_BLOCK_MII 0x3 /* Subblocks 0 and 1 */
> @@ -61,6 +65,8 @@
> #define VSC73XX_CAT_DROP 0x6e
> #define VSC73XX_CAT_PR_MISC_L2 0x6f
> #define VSC73XX_CAT_PR_USR_PRIO 0x75
> +#define VSC73XX_CAT_VLAN_MISC 0x79
> +#define VSC73XX_CAT_PORT_VLAN 0x7a
> #define VSC73XX_Q_MISC_CONF 0xdf
>
> /* MAC_CFG register bits */
> @@ -121,6 +127,17 @@
> #define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1)
> #define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0)
>
> +/* TXUPDCFG transmit modify setup bits */
> +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19)
> +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18)
> +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17)
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4)
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3)
> +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1)
> +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0)
> +
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
> +
> /* CAT_DROP categorizer frame dropping register bits */
> #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
> #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
> @@ -134,6 +151,15 @@
> #define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1)
> #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0)
>
> +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
> +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
> +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
> +
> +/* CAT_PORT_VLAN categorizer port VLAN*/
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> +
> /* Frame analyzer block 2 registers */
> #define VSC73XX_STORMLIMIT 0x02
> #define VSC73XX_ADVLEARN 0x03
> @@ -188,7 +214,8 @@
> #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
> #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
> #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
> -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
> +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
> +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
> @@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
> { 29, "TxQoSClass3" }, /* non-standard counter */
> };
>
> +enum vsc73xx_port_vlan_conf {
> + VSC73XX_VLAN_FILTER,
> + VSC73XX_VLAN_IGNORE,
> +};
> +
> int vsc73xx_is_addr_valid(u8 block, u8 subblock)
> {
> switch (block) {
> @@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
> static int vsc73xx_setup(struct dsa_switch *ds)
> {
> struct vsc73xx *vsc = ds->priv;
> - int i;
> + int i, ret;
../drivers/net/dsa/vitesse-vsc73xx-core.c:598:9: warning: unused variable 'ret' [-Wunused-variable]
int i, ret;
^
>
> dev_info(vsc->dev, "set up the switch\n");
>
> @@ -623,6 +655,9 @@ 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);
> + /* Ingess VLAN reception mask (table 145) */
> + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
> + 0x5f);
> /* IP multicast flood mask (table 144) */
> vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> 0xff);
> @@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> return 9600 - ETH_HLEN - ETH_FCS_LEN;
> }
>
> +static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> +{
> + int ret, err;
> + u32 val;
> +
> + ret = read_poll_timeout(vsc73xx_read, err,
> + err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
> + VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
> + 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
> + 0, VSC73XX_VLANACCESS, &val);
> + if (ret)
> + return ret;
> + return err;
> +}
> +
> +static int
> +vsc73xx_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
> +{
> + u32 val;
> + int ret;
> +
> + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> + if (ret)
> + return ret;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
> + VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
> + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> + if (ret)
> + return ret;
> + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
> + *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
> + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
> + return 0;
> +}
> +
> +static int
> +vsc73xx_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
> +{
> + int ret;
> +
> + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> + if (ret)
> + return ret;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
> + VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> + VSC73XX_VLANACCESS_VLAN_PORT_MASK,
> + VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
> + VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> + (portmap <<
> + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
> +
> + return vsc73xx_wait_for_vlan_table_cmd(vsc);
> +}
> +
> +static int
> +vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> + u8 portmap;
> + int ret;
> +
> + ret = vsc73xx_read_vlan_table_entry(vsc, vid, &portmap);
> + if (ret)
> + return ret;
> +
> + if (set)
> + portmap |= BIT(port) | BIT(CPU_PORT);
> + else
> + portmap &= ~BIT(port);
> +
> + if (portmap == BIT(CPU_PORT))
> + portmap = 0;
Why did you need to do this? To my knowledge, the DSA framework code
will decide when to remove VLANs from the CPU port.
> +
> + return vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
> +}
> +
> +static void
> +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> + enum vsc73xx_port_vlan_conf port_vlan_conf)
> +{
> + if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
IMO this, and all the other places that write the same registers from 2
branches, would look less clutered like this:
u32 val;
val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
val = ...
Otherwise, the larger the branches become, the harder it gets to see
what's common and what's different.
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
> + } else {
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> + 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_INSERT_TAG,
> + VSC73XX_TXUPDCFG_TX_INSERT_TAG);
> + }
> +}
> +
> +static int
> +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> + u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> + (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> + return 0;
> +}
> +
> +static int
> +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
Would it simplify callers to add a "bool operate_on_storage" argument here,
and absorb that logic? The callers could look like this, notice how there is
less code duplication between the "if" and "else" branches.
static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
bool tag_8021q_vlan)
{
struct dsa_port *dp = dsa_to_port(ds, port);
struct vsc73xx *vsc = ds->priv;
bool operate_on_storage;
u16 existing_pvid;
bool had_pvid;
operate_on_storage = vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan;
had_pvid = vsc73xx_port_get_pvid(vsc, port, &existing_pvid,
&operate_on_storage);
if (had_pvid && existing_pvid != vid) {
dev_warn(vsc->dev,
"Port %d can have only one pvid! Now is: %d.\n",
port, existing_pvid);
return -EOPNOTSUPP;
}
return vsc73xx_vlan_change_pvid(vsc, port, vid, true, operate_on_storage);
}
> +{
> + u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> + VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> + vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> + return 0;
> +}
> +
> +static int vsc73xx_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)
I believe it would look better if this would return bool, and if it had
a shorter name (vsc73xx_port_get_pvid).
> +{
> + u32 val;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
> + if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
> + return 0;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> + *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> +
> + return VSC73XX_IS_CONFIGURED;
> +}
> +
> +static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
> +{
> + u32 val;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> + if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
> + return 0;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + &val);
Don't need to read the same register twice.
> + *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> +
> + return VSC73XX_IS_CONFIGURED;
> +}
> +
> +static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> + bool port_vlan)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u16 vlan_no = 0;
> +
> + if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {
I think it would be valuable for readability to create a helper function named:
static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
{
return !dsa_port_is_vlan_filtering(dp);
}
and then, to rename "port_vlan" to "!tag_8021q_vlan". It would make it
clearer as to what the checks are about.
if (vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan) {
// update cached storage
} else {
// update hardware
}
> + if (vsc->untagged_storage[port] < VLAN_N_VID &&
> + !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> + !vid_is_dsa_8021q(vid) &&
The problem here which led to these vid_is_dsa_8021q() checks is that
dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
those, correct? But when the port is VSC73XX_VLAN_IGNORE mode (and
tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
*all* VLANs are egress-untagged VLANs, correct?
If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
first place, for tag_8021q VLANs, if you don't rely on the port's native
VLAN for egress untagging?
> + vsc->untagged_storage[port] != vid) {
> + dev_warn(vsc->dev,
> + "Port %d can have only one untagged vid! Now is: %d.\n",
> + port, vsc->untagged_storage[port]);
> + return -EOPNOTSUPP;
> + }
> + vsc->untagged_storage[port] = vid;
> + } else {
> + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) && vlan_no != vid) {
> + dev_warn(vsc->dev,
> + "Port %d can have only one untagged vid! Now is: %d.\n",
> + port, vlan_no);
> + return -EOPNOTSUPP;
> + }
> + return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
> + }
> +
> + return 0;
> +}
> +
> +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> + bool port_vlan)
> +{
> + struct dsa_port *dsa_port = dsa_to_port(ds, port);
> + struct vsc73xx *vsc = ds->priv;
> + u16 vlan_no;
> +
> + if (!dsa_port)
> + return -EINVAL;
Why is this check here?
> +
> + if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
> + if (vsc->pvid_storage[port] < VLAN_N_VID &&
> + !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
> + !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {
What are the vid_is_dsa_8021q() checks for?
> + dev_warn(vsc->dev,
> + "Port %d can have only one pvid! Now is: %d.\n",
> + port, vsc->pvid_storage[port]);
> + return -EOPNOTSUPP;
> + }
> + vsc->pvid_storage[port] = vid;
> + } else {
> + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
> + vlan_no != vid) {
> + dev_warn(vsc->dev,
> + "Port %d can have only one pvid! Now is: %d.\n",
> + port, vlan_no);
> + return -EOPNOTSUPP;
> + }
> + return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering, struct netlink_ext_ack *extack)
A comment would be good which states that the flipping between the
hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
only gets called on actual changes to vlan_filtering, and thus, to
vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
needs to go to hardware, and what is in hardware needs to go to storage.
It's an interesting implementation for sure.
> +{
> + struct vsc73xx *vsc = ds->priv;
> + bool store_untagged = false;
> + bool store_pvid = false;
nit: replace the 2 spaces before the "=" with a single one.
> + u16 vlan_no;
> +
> + if (vlan_filtering)
> + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> + else
> + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> +
> + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> + store_pvid = true;
> +
> + if (vsc->pvid_storage[port] < VLAN_N_VID)
> + vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
> + else
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> +
> + vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
> +
> + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> + store_untagged = true;
> +
> + if (vsc->untagged_storage[port] < VLAN_N_VID)
> + vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
> + else
> + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> +
> + vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_VID;
> +
> + return 0;
> +}
> +
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan,
> + struct netlink_ext_ack *extack)
> +{
> + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> + struct vsc73xx *vsc = ds->priv;
> + u16 vlan_no;
> + int ret;
> +
> + /* Be sure to deny alterations to the configuration done by tag_8021q.
> + */
> + if (vid_is_dsa_8021q(vlan->vid)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Range 3072-4095 reserved for dsa_8021q operation");
> + return -EBUSY;
> + }
> +
> + if (port != CPU_PORT) {
> + if (untagged) {
> + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> + if (ret)
> + return ret;
> + } else {
> + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
> + == VSC73XX_IS_CONFIGURED &&
> + vlan_no == vlan->vid)
> + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> + else if (vsc->untagged_storage[port] == vlan->vid)
> + vsc->untagged_storage[port] = VLAN_N_VID;
> + }
> + if (pvid) {
> + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> + if (ret)
> + return ret;
> + } else {
> + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
> + == VSC73XX_IS_CONFIGURED &&
> + vlan_no == vlan->vid)
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> + else if (vsc->pvid_storage[port] == vlan->vid)
> + vsc->pvid_storage[port] = VLAN_N_VID;
> + }
> + }
> +
> + return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);
last argument is bool, so pass true or false (here and everywhere else)
> +}
> +
> +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u16 vlan_no;
> + int ret;
> +
> + ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
> + if (ret)
> + return ret;
> +
> + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> + vlan_no == vlan->vid)
> + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> + else if (vsc->untagged_storage[port] == vlan->vid)
> + vsc->untagged_storage[port] = VLAN_N_VID;
> +
> + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> + vlan_no == vlan->vid)
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> + else if (vsc->pvid_storage[port] == vlan->vid)
> + vsc->pvid_storage[port] = VLAN_N_VID;
> +
> + return 0;
> +}
> +
> static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> {
> + struct vsc73xx *vsc = ds->priv;
> + int ret, i;
> +
> + /* Those bits are responsible for MTU only. Kernel take care about MTU,
> + * let's enable +8 bytes frame length unconditionally.
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
> + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
> +
> + if (port == CPU_PORT)
> + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> + else
> + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> +
> + for (i = 0; i <= VLAN_N_VID; i++) {
VLAN_N_VID is 4096, so i <= VLAN_N_VID must be a mistake?
> + ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
> + if (ret)
> + return ret;
> + }
Also, could you write an expedited version of this, which is not called
from ds->ops->port_setup() but from ds->ops->setup()? It is wasteful to
traverse the VLAN table for each port, when we know from the get go that
we need to clear all ports.
> +
> /* Configure forward map to CPU <-> port only */
> if (port == CPU_PORT)
> vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> @@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> BIT(CPU_PORT);
>
> + /* Set storage values out of range*/
Nit: Space after "*/" (there are a few other places left which have
this style issue)
> + vsc->pvid_storage[port] = VLAN_N_VID;
> + vsc->untagged_storage[port] = VLAN_N_VID;
> +
> return 0;
> }
>
> @@ -1098,6 +1516,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .port_change_mtu = vsc73xx_change_mtu,
> .port_max_mtu = vsc73xx_get_max_mtu,
> .port_stp_state_set = vsc73xx_port_stp_state_set,
> + .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> + .port_vlan_add = vsc73xx_port_vlan_add,
> + .port_vlan_del = vsc73xx_port_vlan_del,
> };
>
> 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 224e284a5573..b7614dd7d0eb 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -30,6 +30,8 @@ struct vsc73xx {
> const struct vsc73xx_ops *ops;
> void *priv;
> u8 forward_map[VSC73XX_MAX_NUM_PORTS];
> + u16 pvid_storage[VSC73XX_MAX_NUM_PORTS];
> + u16 untagged_storage[VSC73XX_MAX_NUM_PORTS];
> };
>
> struct vsc73xx_ops {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-09-12 16:17 ` Vladimir Oltean
@ 2023-09-22 14:26 ` Paweł Dembicki
2023-09-26 23:58 ` Vladimir Oltean
0 siblings, 1 reply; 27+ messages in thread
From: Paweł Dembicki @ 2023-09-22 14:26 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
Hi Vladimir,
wt., 12 wrz 2023 o 18:17 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Tue, Sep 12, 2023 at 02:21:59PM +0200, Pawel Dembicki wrote:
> > This patch implements VLAN filtering for the vsc73xx driver.
> >
> > After starting VLAN filtering, the switch is reconfigured from QinQ to
> > a simple VLAN aware mode. This is required because VSC73XX chips do not
> > support inner VLAN tag filtering.
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > v3:
> > - reworked all vlan commits
> > - added storage variables for pvid and untagged vlans
> > - move length extender settings to port setup
> > - remove vlan table cleaning in wrong places
> > - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
> > and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
> > v2:
> > - no changes done
> >
> > drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++-
> > drivers/net/dsa/vitesse-vsc73xx.h | 2 +
> > 2 files changed, 425 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 541fbc195df1..d9a6eac1fcce 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -21,14 +21,18 @@
> > #include <linux/of_mdio.h>
> > #include <linux/bitops.h>
> > #include <linux/if_bridge.h>
> > +#include <linux/if_vlan.h>
> > #include <linux/etherdevice.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/gpio/driver.h>
> > +#include <linux/dsa/8021q.h>
> > #include <linux/random.h>
> > #include <net/dsa.h>
> >
> > #include "vitesse-vsc73xx.h"
> >
> > +#define VSC73XX_IS_CONFIGURED 0x1
> > +
> > #define VSC73XX_BLOCK_MAC 0x1 /* Subblocks 0-4, 6 (CPU port) */
> > #define VSC73XX_BLOCK_ANALYZER 0x2 /* Only subblock 0 */
> > #define VSC73XX_BLOCK_MII 0x3 /* Subblocks 0 and 1 */
> > @@ -61,6 +65,8 @@
> > #define VSC73XX_CAT_DROP 0x6e
> > #define VSC73XX_CAT_PR_MISC_L2 0x6f
> > #define VSC73XX_CAT_PR_USR_PRIO 0x75
> > +#define VSC73XX_CAT_VLAN_MISC 0x79
> > +#define VSC73XX_CAT_PORT_VLAN 0x7a
> > #define VSC73XX_Q_MISC_CONF 0xdf
> >
> > /* MAC_CFG register bits */
> > @@ -121,6 +127,17 @@
> > #define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1)
> > #define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0)
> >
> > +/* TXUPDCFG transmit modify setup bits */
> > +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19)
> > +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18)
> > +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17)
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4)
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3)
> > +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1)
> > +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0)
> > +
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
> > +
> > /* CAT_DROP categorizer frame dropping register bits */
> > #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
> > #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
> > @@ -134,6 +151,15 @@
> > #define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1)
> > #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0)
> >
> > +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
> > +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
> > +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
> > +
> > +/* CAT_PORT_VLAN categorizer port VLAN*/
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> > +
> > /* Frame analyzer block 2 registers */
> > #define VSC73XX_STORMLIMIT 0x02
> > #define VSC73XX_ADVLEARN 0x03
> > @@ -188,7 +214,8 @@
> > #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
> > #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
> > #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
> > -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
> > +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
> > +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
> > @@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
> > { 29, "TxQoSClass3" }, /* non-standard counter */
> > };
> >
> > +enum vsc73xx_port_vlan_conf {
> > + VSC73XX_VLAN_FILTER,
> > + VSC73XX_VLAN_IGNORE,
> > +};
> > +
> > int vsc73xx_is_addr_valid(u8 block, u8 subblock)
> > {
> > switch (block) {
> > @@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
> > static int vsc73xx_setup(struct dsa_switch *ds)
> > {
> > struct vsc73xx *vsc = ds->priv;
> > - int i;
> > + int i, ret;
>
> ../drivers/net/dsa/vitesse-vsc73xx-core.c:598:9: warning: unused variable 'ret' [-Wunused-variable]
> int i, ret;
> ^
>
> >
> > dev_info(vsc->dev, "set up the switch\n");
> >
> > @@ -623,6 +655,9 @@ 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);
> > + /* Ingess VLAN reception mask (table 145) */
> > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
> > + 0x5f);
> > /* IP multicast flood mask (table 144) */
> > vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> > 0xff);
> > @@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> > return 9600 - ETH_HLEN - ETH_FCS_LEN;
> > }
> >
> > +static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> > +{
> > + int ret, err;
> > + u32 val;
> > +
> > + ret = read_poll_timeout(vsc73xx_read, err,
> > + err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
> > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
> > + 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
> > + 0, VSC73XX_VLANACCESS, &val);
> > + if (ret)
> > + return ret;
> > + return err;
> > +}
> > +
> > +static int
> > +vsc73xx_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
> > +{
> > + u32 val;
> > + int ret;
> > +
> > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> > + if (ret)
> > + return ret;
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
> > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
> > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> > + if (ret)
> > + return ret;
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
> > + *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
> > + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
> > + return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
> > +{
> > + int ret;
> > +
> > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
> > + if (ret)
> > + return ret;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
> > + VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> > + VSC73XX_VLANACCESS_VLAN_PORT_MASK,
> > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
> > + VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> > + (portmap <<
> > + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
> > +
> > + return vsc73xx_wait_for_vlan_table_cmd(vsc);
> > +}
> > +
> > +static int
> > +vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
> > +{
> > + u8 portmap;
> > + int ret;
> > +
> > + ret = vsc73xx_read_vlan_table_entry(vsc, vid, &portmap);
> > + if (ret)
> > + return ret;
> > +
> > + if (set)
> > + portmap |= BIT(port) | BIT(CPU_PORT);
> > + else
> > + portmap &= ~BIT(port);
> > +
> > + if (portmap == BIT(CPU_PORT))
> > + portmap = 0;
>
> Why did you need to do this? To my knowledge, the DSA framework code
> will decide when to remove VLANs from the CPU port.
>
I will fix it.
> > +
> > + return vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
> > +}
> > +
> > +static void
> > +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> > + enum vsc73xx_port_vlan_conf port_vlan_conf)
> > +{
> > + if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
>
> IMO this, and all the other places that write the same registers from 2
> branches, would look less clutered like this:
>
> u32 val;
>
> val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
> VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
>
> vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
>
> val = ...
>
> Otherwise, the larger the branches become, the harder it gets to see
> what's common and what's different.
>
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
> > + } else {
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > + 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_INSERT_TAG,
> > + VSC73XX_TXUPDCFG_TX_INSERT_TAG);
> > + }
> > +}
> > +
> > +static int
> > +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
> > +{
> > + u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> > + (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> > + return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
>
> Would it simplify callers to add a "bool operate_on_storage" argument here,
> and absorb that logic? The callers could look like this, notice how there is
> less code duplication between the "if" and "else" branches.
>
I will rework it.
> static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> bool tag_8021q_vlan)
> {
> struct dsa_port *dp = dsa_to_port(ds, port);
> struct vsc73xx *vsc = ds->priv;
> bool operate_on_storage;
> u16 existing_pvid;
> bool had_pvid;
>
> operate_on_storage = vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan;
>
> had_pvid = vsc73xx_port_get_pvid(vsc, port, &existing_pvid,
> &operate_on_storage);
> if (had_pvid && existing_pvid != vid) {
> dev_warn(vsc->dev,
> "Port %d can have only one pvid! Now is: %d.\n",
> port, existing_pvid);
> return -EOPNOTSUPP;
> }
>
> return vsc73xx_vlan_change_pvid(vsc, port, vid, true, operate_on_storage);
> }
>
> > +{
> > + u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> > + VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> > + vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> > + return 0;
> > +}
> > +
> > +static int vsc73xx_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)
>
> I believe it would look better if this would return bool, and if it had
> a shorter name (vsc73xx_port_get_pvid).
>
> > +{
> > + u32 val;
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
> > + if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
> > + return 0;
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > + *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > +
> > + return VSC73XX_IS_CONFIGURED;
> > +}
> > +
> > +static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
> > +{
> > + u32 val;
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > + if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
> > + return 0;
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + &val);
>
> Don't need to read the same register twice.
>
> > + *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > +
> > + return VSC73XX_IS_CONFIGURED;
> > +}
> > +
> > +static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> > + bool port_vlan)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u16 vlan_no = 0;
> > +
> > + if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {
>
> I think it would be valuable for readability to create a helper function named:
>
> static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
> {
> return !dsa_port_is_vlan_filtering(dp);
> }
>
> and then, to rename "port_vlan" to "!tag_8021q_vlan". It would make it
> clearer as to what the checks are about.
>
> if (vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan) {
> // update cached storage
> } else {
> // update hardware
> }
>
> > + if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > + !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > + !vid_is_dsa_8021q(vid) &&
>
> The problem here which led to these vid_is_dsa_8021q() checks is that
> dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> those, correct?
In my case, the major problem with tag8021q vlans is
"dsa_tag_8021q_bridge_join" function:
"dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
I must disable pvid/untagged checking, because it will always fail. I
let kernel do the job,
it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
> But when the port is VSC73XX_VLAN_IGNORE mode (and
> tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> *all* VLANs are egress-untagged VLANs, correct?
>
> If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> first place, for tag_8021q VLANs, if you don't rely on the port's native
> VLAN for egress untagging?
>
> > + vsc->untagged_storage[port] != vid) {
> > + dev_warn(vsc->dev,
> > + "Port %d can have only one untagged vid! Now is: %d.\n",
> > + port, vsc->untagged_storage[port]);
> > + return -EOPNOTSUPP;
> > + }
> > + vsc->untagged_storage[port] = vid;
> > + } else {
> > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) && vlan_no != vid) {
> > + dev_warn(vsc->dev,
> > + "Port %d can have only one untagged vid! Now is: %d.\n",
> > + port, vlan_no);
> > + return -EOPNOTSUPP;
> > + }
> > + return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> > + bool port_vlan)
> > +{
> > + struct dsa_port *dsa_port = dsa_to_port(ds, port);
> > + struct vsc73xx *vsc = ds->priv;
> > + u16 vlan_no;
> > +
> > + if (!dsa_port)
> > + return -EINVAL;
>
> Why is this check here?
>
Unnecessary defensive programming.
> > +
> > + if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
> > + if (vsc->pvid_storage[port] < VLAN_N_VID &&
> > + !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
> > + !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {
>
> What are the vid_is_dsa_8021q() checks for?
The same reason as the "untagged" case.
>
> > + dev_warn(vsc->dev,
> > + "Port %d can have only one pvid! Now is: %d.\n",
> > + port, vsc->pvid_storage[port]);
> > + return -EOPNOTSUPP;
> > + }
> > + vsc->pvid_storage[port] = vid;
> > + } else {
> > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
> > + vlan_no != vid) {
> > + dev_warn(vsc->dev,
> > + "Port %d can have only one pvid! Now is: %d.\n",
> > + port, vlan_no);
> > + return -EOPNOTSUPP;
> > + }
> > + return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> > + bool vlan_filtering, struct netlink_ext_ack *extack)
>
> A comment would be good which states that the flipping between the
> hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> only gets called on actual changes to vlan_filtering, and thus, to
> vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> needs to go to hardware, and what is in hardware needs to go to storage.
>
> It's an interesting implementation for sure.
>
Thank you.
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + bool store_untagged = false;
> > + bool store_pvid = false;
>
> nit: replace the 2 spaces before the "=" with a single one.
>
> > + u16 vlan_no;
> > +
> > + if (vlan_filtering)
> > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> > + else
> > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> > +
> > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> > + store_pvid = true;
> > +
> > + if (vsc->pvid_storage[port] < VLAN_N_VID)
> > + vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
> > + else
> > + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > +
> > + vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
> > +
> > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> > + store_untagged = true;
> > +
> > + if (vsc->untagged_storage[port] < VLAN_N_VID)
> > + vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
> > + else
> > + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > +
> > + vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_VID;
> > +
> > + return 0;
> > +}
> > +
> > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > + const struct switchdev_obj_port_vlan *vlan,
> > + struct netlink_ext_ack *extack)
> > +{
> > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > + struct vsc73xx *vsc = ds->priv;
> > + u16 vlan_no;
> > + int ret;
> > +
> > + /* Be sure to deny alterations to the configuration done by tag_8021q.
> > + */
> > + if (vid_is_dsa_8021q(vlan->vid)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Range 3072-4095 reserved for dsa_8021q operation");
> > + return -EBUSY;
> > + }
> > +
> > + if (port != CPU_PORT) {
> > + if (untagged) {
> > + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> > + if (ret)
> > + return ret;
> > + } else {
> > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
> > + == VSC73XX_IS_CONFIGURED &&
> > + vlan_no == vlan->vid)
> > + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > + else if (vsc->untagged_storage[port] == vlan->vid)
> > + vsc->untagged_storage[port] = VLAN_N_VID;
> > + }
> > + if (pvid) {
> > + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> > + if (ret)
> > + return ret;
> > + } else {
> > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
> > + == VSC73XX_IS_CONFIGURED &&
> > + vlan_no == vlan->vid)
> > + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > + else if (vsc->pvid_storage[port] == vlan->vid)
> > + vsc->pvid_storage[port] = VLAN_N_VID;
> > + }
> > + }
> > +
> > + return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);
>
> last argument is bool, so pass true or false (here and everywhere else)
>
> > +}
> > +
> > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> > + const struct switchdev_obj_port_vlan *vlan)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u16 vlan_no;
> > + int ret;
> > +
> > + ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
> > + if (ret)
> > + return ret;
> > +
> > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > + vlan_no == vlan->vid)
> > + vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > + else if (vsc->untagged_storage[port] == vlan->vid)
> > + vsc->untagged_storage[port] = VLAN_N_VID;
> > +
> > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > + vlan_no == vlan->vid)
> > + vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > + else if (vsc->pvid_storage[port] == vlan->vid)
> > + vsc->pvid_storage[port] = VLAN_N_VID;
> > +
> > + return 0;
> > +}
> > +
> > static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> > {
> > + struct vsc73xx *vsc = ds->priv;
> > + int ret, i;
> > +
> > + /* Those bits are responsible for MTU only. Kernel take care about MTU,
> > + * let's enable +8 bytes frame length unconditionally.
> > + */
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> > + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
> > + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > + VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
> > +
> > + if (port == CPU_PORT)
> > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> > + else
> > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> > +
> > + for (i = 0; i <= VLAN_N_VID; i++) {
>
> VLAN_N_VID is 4096, so i <= VLAN_N_VID must be a mistake?
>
Yeep.
> > + ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
> > + if (ret)
> > + return ret;
> > + }
>
> Also, could you write an expedited version of this, which is not called
> from ds->ops->port_setup() but from ds->ops->setup()? It is wasteful to
> traverse the VLAN table for each port, when we know from the get go that
> we need to clear all ports.
>
I'll rework it in setup.
> > +
> > /* Configure forward map to CPU <-> port only */
> > if (port == CPU_PORT)
> > vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> > @@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> > vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> > BIT(CPU_PORT);
> >
> > + /* Set storage values out of range*/
>
> Nit: Space after "*/" (there are a few other places left which have
> this style issue)
>
> > + vsc->pvid_storage[port] = VLAN_N_VID;
> > + vsc->untagged_storage[port] = VLAN_N_VID;
> > +
> > return 0;
> > }
> >
> > @@ -1098,6 +1516,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> > .port_change_mtu = vsc73xx_change_mtu,
> > .port_max_mtu = vsc73xx_get_max_mtu,
> > .port_stp_state_set = vsc73xx_port_stp_state_set,
> > + .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> > + .port_vlan_add = vsc73xx_port_vlan_add,
> > + .port_vlan_del = vsc73xx_port_vlan_del,
> > };
> >
> > 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 224e284a5573..b7614dd7d0eb 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -30,6 +30,8 @@ struct vsc73xx {
> > const struct vsc73xx_ops *ops;
> > void *priv;
> > u8 forward_map[VSC73XX_MAX_NUM_PORTS];
> > + u16 pvid_storage[VSC73XX_MAX_NUM_PORTS];
> > + u16 untagged_storage[VSC73XX_MAX_NUM_PORTS];
> > };
> >
> > struct vsc73xx_ops {
> > --
> > 2.34.1
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-09-22 14:26 ` Paweł Dembicki
@ 2023-09-26 23:58 ` Vladimir Oltean
2023-10-03 21:14 ` Paweł Dembicki
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-26 23:58 UTC (permalink / raw)
To: Paweł Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > + if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > + !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > + !vid_is_dsa_8021q(vid) &&
> >
> > The problem here which led to these vid_is_dsa_8021q() checks is that
> > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > those, correct?
>
> In my case, the major problem with tag8021q vlans is
> "dsa_tag_8021q_bridge_join" function:
> "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> I must disable pvid/untagged checking, because it will always fail. I
> let kernel do the job,
> it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
I'm not sure that you described the problem in a way that I can understand, here.
After dsa_tag_8021q_bridge_join():
-> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
-> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)
it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.
For context, consider the fact that you can run the following commands:
bridge vlan add dev eth0 vid 10 pvid
bridge vlan add dev eth0 vid 11 pvid
and after the second command, vid 10 stops being a pvid.
So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
is not correct. You need to implement the pvid overwriting behavior, since
there's always only 1 pvid.
So that leaves the "untagged" flag as being problematic, correct? Could
you comment...
>
> > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > *all* VLANs are egress-untagged VLANs, correct?
> >
> > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > VLAN for egress untagging?
... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
is a configuration that can actually be supported by vsc73xx. You just
need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
basically requests exactly that configuration on user ports (both the
bridge_vid and the standalone_vid are egress-untagged). So your check is
too restrictive, you are denying a configuration that would work.
The problem only appears when you mix egress-tagged with egress-untagged
VLANs on a port. Only then there can be at most 1 egress-untagged VID,
because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
egress-tagged VIDs to work.
> > A comment would be good which states that the flipping between the
> > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > only gets called on actual changes to vlan_filtering, and thus, to
> > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > needs to go to hardware, and what is in hardware needs to go to storage.
> >
> > It's an interesting implementation for sure.
> >
>
> Thank you.
I'm not sure if that was a compliment :) At least in this form, it's
certainly non-trivial to determine by looking at the code if it is
correct or not, and it uses different patterns than the other VLAN
implementations in DSA drivers. Generally, boring and obvious is
preferable. But after I took the time to understand, it seems plausible
that the approach might work.
Let's see how the same idea looks, cleaned up a bit but not redesigned,
in v4.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-09-26 23:58 ` Vladimir Oltean
@ 2023-10-03 21:14 ` Paweł Dembicki
2023-10-03 21:27 ` Vladimir Oltean
0 siblings, 1 reply; 27+ messages in thread
From: Paweł Dembicki @ 2023-10-03 21:14 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
śr., 27 wrz 2023 o 01:58 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > > + if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > > + !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > > + !vid_is_dsa_8021q(vid) &&
> > >
> > > The problem here which led to these vid_is_dsa_8021q() checks is that
> > > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > > those, correct?
> >
> > In my case, the major problem with tag8021q vlans is
> > "dsa_tag_8021q_bridge_join" function:
> > "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> > I must disable pvid/untagged checking, because it will always fail. I
> > let kernel do the job,
> > it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
>
> I'm not sure that you described the problem in a way that I can understand, here.
>
> After dsa_tag_8021q_bridge_join():
> -> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
> -> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)
>
> it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.
>
> For context, consider the fact that you can run the following commands:
>
> bridge vlan add dev eth0 vid 10 pvid
> bridge vlan add dev eth0 vid 11 pvid
>
> and after the second command, vid 10 stops being a pvid.
>
> So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
> is not correct. You need to implement the pvid overwriting behavior, since
> there's always only 1 pvid.
>
Yes, overwriting pvid is the only proper way. Kernel mechanism will
take care about the number of pvids. I will fixit in v4.
> So that leaves the "untagged" flag as being problematic, correct? Could
> you comment...
>
> >
> > > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > > *all* VLANs are egress-untagged VLANs, correct?
> > >
> > > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > > VLAN for egress untagging?
>
> ... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
> is a configuration that can actually be supported by vsc73xx. You just
> need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
> basically requests exactly that configuration on user ports (both the
> bridge_vid and the standalone_vid are egress-untagged). So your check is
> too restrictive, you are denying a configuration that would work.
> The problem only appears when you mix egress-tagged with egress-untagged
> VLANs on a port. Only then there can be at most 1 egress-untagged VID,
> because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
> egress-tagged VIDs to work.
Should I make a local copy of the quantity of egress untagged and
tagged vlans per port to resolve this issue, shouldn't I?
And then I check how many vlans are egress tagged or untagged for a
properly restricted solution?
I see another problem. Even if I return an error value, the untagged
will be marked in 'bridge vlan' listing. I'm not sure how it should
work in this case.
>
> > > A comment would be good which states that the flipping between the
> > > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > > only gets called on actual changes to vlan_filtering, and thus, to
> > > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > > needs to go to hardware, and what is in hardware needs to go to storage.
> > >
> > > It's an interesting implementation for sure.
> > >
> >
> > Thank you.
>
> I'm not sure if that was a compliment :)
Touché. :)
>At least in this form, it's
> certainly non-trivial to determine by looking at the code if it is
> correct or not, and it uses different patterns than the other VLAN
> implementations in DSA drivers. Generally, boring and obvious is
> preferable. But after I took the time to understand, it seems plausible
> that the approach might work.
>
> Let's see how the same idea looks, cleaned up a bit but not redesigned,
> in v4.
I try to at least clean pvid and untagged issues before v4.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
2023-10-03 21:14 ` Paweł Dembicki
@ 2023-10-03 21:27 ` Vladimir Oltean
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Oltean @ 2023-10-03 21:27 UTC (permalink / raw)
To: Paweł Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
On Tue, Oct 03, 2023 at 11:14:55PM +0200, Paweł Dembicki wrote:
> Should I make a local copy of the quantity of egress untagged and
> tagged vlans per port to resolve this issue, shouldn't I?
> And then I check how many vlans are egress tagged or untagged for a
> properly restricted solution?
Yeah, I guess so. It is the same problem that ocelot_vlan_prepare()
tries to solve, and that counts ocelot_port_num_untagged_vlans() and
ocelot_port_num_tagged_vlans() indeed.
> I see another problem. Even if I return an error value, the untagged
> will be marked in 'bridge vlan' listing. I'm not sure how it should
> work in this case.
What error code are you returning, -EOPNOTSUPP I guess? That's
deliberately ignored by callers of br_switchdev_port_vlan_add(), because
it means "no one responded to the switchdev notifier, so the VLAN is not
offloaded". If you want to deny the configuration you need to return
something else, like -EBUSY.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
` (4 preceding siblings ...)
2023-09-12 12:21 ` [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering Pawel Dembicki
@ 2023-09-12 12:22 ` Pawel Dembicki
2023-09-12 21:39 ` Vladimir Oltean
2023-09-12 12:22 ` [PATCH net-next v3 7/8] net: dsa: vsc73xx: Implement vsc73xx 8021q tagger Pawel Dembicki
2023-09-12 12:22 ` [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support Pawel Dembicki
7 siblings, 1 reply; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:22 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel
This commit introduces a new tagger based on 802.1q tagging.
It's designed for the vsc73xx driver. The VSC73xx family doesn't have
any tag support for the RGMII port, but it could be based on VLANs.
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- Introduce a patch after the tagging patch split
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +++
net/dsa/Makefile | 1 +
net/dsa/tag_vsc73xx_8021q.c | 91 +++++++++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
create mode 100644 net/dsa/tag_vsc73xx_8021q.c
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 0b9c6aa27047..f83454d4ad02 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -56,6 +56,7 @@ struct phylink_link_state;
#define DSA_TAG_PROTO_RTL8_4T_VALUE 25
#define DSA_TAG_PROTO_RZN1_A5PSW_VALUE 26
#define DSA_TAG_PROTO_LAN937X_VALUE 27
+#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE 28
enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
@@ -86,6 +87,7 @@ enum dsa_tag_protocol {
DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE,
DSA_TAG_PROTO_RZN1_A5PSW = DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
DSA_TAG_PROTO_LAN937X = DSA_TAG_PROTO_LAN937X_VALUE,
+ DSA_TAG_PROTO_VSC73XX_8021Q = DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
};
struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 8e698bea99a3..e59360071c67 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -166,6 +166,12 @@ config NET_DSA_TAG_TRAILER
Say Y or M if you want to enable support for tagging frames at
with a trailed. e.g. Marvell 88E6060.
+config NET_DSA_TAG_VSC73XX_8021Q
+ tristate "Tag driver for Microchip/Vitesse VSC73xx family of switches, using VLAN"
+ help
+ Say Y or M if you want to enable support for tagging frames with a
+ custom VLAN-based header.
+
config NET_DSA_TAG_XRS700X
tristate "Tag driver for XRS700x switches"
help
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 12e305824a96..bab8a933c514 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
obj-$(CONFIG_NET_DSA_TAG_RZN1_A5PSW) += tag_rzn1_a5psw.o
obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
+obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
# for tracing framework to find trace.h
diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
new file mode 100644
index 000000000000..9093a71e6eb0
--- /dev/null
+++ b/net/dsa/tag_vsc73xx_8021q.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
+ * Based on tag_sja1105.c:
+ * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
+ */
+#include <linux/dsa/8021q.h>
+
+#include "tag.h"
+#include "tag_8021q.h"
+
+#define VSC73XX_8021Q_NAME "vsc73xx-8021q"
+
+static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev)
+{
+ struct dsa_port *dp = dsa_slave_to_port(netdev);
+ u16 queue_mapping = skb_get_queue_mapping(skb);
+ u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
+ u8 pcp;
+
+ if (skb->offload_fwd_mark) {
+ unsigned int bridge_num = dsa_port_bridge_num_get(dp);
+ struct net_device *br = dsa_port_bridge_dev_get(dp);
+
+ if (br_vlan_enabled(br))
+ return skb;
+
+ tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
+ }
+
+ pcp = netdev_txq_to_tc(netdev, queue_mapping);
+
+ return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
+ ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
+}
+
+static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
+ int *switch_id, int *vbid, u16 *vid)
+{
+ if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
+ return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
+
+ /* Try our best with imprecise RX */
+ *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
+}
+
+static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev)
+{
+ int src_port = -1, switch_id = -1, vbid = -1;
+ u16 vid;
+
+ if (skb_vlan_tag_present(skb)) {
+ /* Normal traffic path. */
+ vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
+ } else {
+ netdev_warn(netdev, "Couldn't decode source port\n");
+ return NULL;
+ }
+
+ if (vbid >= 1)
+ skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
+ else if (src_port == -1 || switch_id == -1)
+ skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
+ else
+ skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
+ if (!skb->dev) {
+ netdev_warn(netdev, "Couldn't decode source port\n");
+ return NULL;
+ }
+
+ dsa_default_offload_fwd_mark(skb);
+
+ if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
+ eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
+ __vlan_hwaccel_clear_tag(skb);
+
+ return skb;
+}
+
+static const struct dsa_device_ops vsc73xx_8021q_netdev_ops = {
+ .name = VSC73XX_8021Q_NAME,
+ .proto = DSA_TAG_PROTO_VSC73XX_8021Q,
+ .xmit = vsc73xx_xmit,
+ .rcv = vsc73xx_rcv,
+ .needed_headroom = VLAN_HLEN,
+ .promisc_on_master = true,
+};
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME);
+
+module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
2023-09-12 12:22 ` [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx Pawel Dembicki
@ 2023-09-12 21:39 ` Vladimir Oltean
2023-09-25 20:55 ` Paweł Dembicki
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-12 21:39 UTC (permalink / raw)
To: Pawel Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
Hi Pawel,
On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote:
> This commit introduces a new tagger based on 802.1q tagging.
> It's designed for the vsc73xx driver. The VSC73xx family doesn't have
> any tag support for the RGMII port, but it could be based on VLANs.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
> new file mode 100644
> index 000000000000..9093a71e6eb0
> --- /dev/null
> +++ b/net/dsa/tag_vsc73xx_8021q.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
2022-2023 by now, maybe?
> + * Based on tag_sja1105.c:
> + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
> + */
> +#include <linux/dsa/8021q.h>
> +
> +#include "tag.h"
> +#include "tag_8021q.h"
> +
> +#define VSC73XX_8021Q_NAME "vsc73xx-8021q"
> +
> +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct dsa_port *dp = dsa_slave_to_port(netdev);
> + u16 queue_mapping = skb_get_queue_mapping(skb);
> + u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
> + u8 pcp;
> +
> + if (skb->offload_fwd_mark) {
> + unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> + struct net_device *br = dsa_port_bridge_dev_get(dp);
> +
> + if (br_vlan_enabled(br))
> + return skb;
> +
> + tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
> + }
> +
> + pcp = netdev_txq_to_tc(netdev, queue_mapping);
> +
> + return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
> + ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
> +}
> +
> +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> + int *switch_id, int *vbid, u16 *vid)
> +{
> + if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> + return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> +
> + /* Try our best with imprecise RX */
> + *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> +}
> +
> +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev)
> +{
> + int src_port = -1, switch_id = -1, vbid = -1;
> + u16 vid;
> +
> + if (skb_vlan_tag_present(skb)) {
> + /* Normal traffic path. */
> + vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> + } else {
> + netdev_warn(netdev, "Couldn't decode source port\n");
> + return NULL;
> + }
> +
> + if (vbid >= 1)
> + skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> + else if (src_port == -1 || switch_id == -1)
> + skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
> + else
> + skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
Hmm, this isn't looking too good.
I think the fact that you had to add my copyright on what should be such
a simple thing as a VLAN-based tagger is a bad sign :)
It's time to consolidate some more stuff that currently lives in
tag_sja1105 and move it into tag_8021q so that you can reuse it more
easily.
I've prepared some (only compile-tested) patches on this branch here:
https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3
I need to double-check that they don't introduce regressions, and we
should discuss the merging strategy. Probably you're going to submit
them together with your patch set.
With that, you can drop my part of the copyright :) The remainder should
look like straightforward use of API which can be written in only a
limited number of ways.
> + if (!skb->dev) {
> + netdev_warn(netdev, "Couldn't decode source port\n");
> + return NULL;
> + }
> +
> + dsa_default_offload_fwd_mark(skb);
> +
> + if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
> + eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
> + __vlan_hwaccel_clear_tag(skb);
Why do you need to do this? We send VLAN-tagged packets to the
VLAN-aware bridge intentionally, so that it knows what VLAN they come
from (in the dsa_find_designated_bridge_port_by_vid() case). So don't
strip it if it's not causing a problem.
> +
> + return skb;
> +}
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
2023-09-12 21:39 ` Vladimir Oltean
@ 2023-09-25 20:55 ` Paweł Dembicki
2023-09-27 0:11 ` Vladimir Oltean
0 siblings, 1 reply; 27+ messages in thread
From: Paweł Dembicki @ 2023-09-25 20:55 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
wt., 12 wrz 2023 o 23:39 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> Hi Pawel,
Hi Vladimir,
>
> On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote:
> > This commit introduces a new tagger based on 802.1q tagging.
> > It's designed for the vsc73xx driver. The VSC73xx family doesn't have
> > any tag support for the RGMII port, but it could be based on VLANs.
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
> > new file mode 100644
> > index 000000000000..9093a71e6eb0
> > --- /dev/null
> > +++ b/net/dsa/tag_vsc73xx_8021q.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
>
> 2022-2023 by now, maybe?
>
> > + * Based on tag_sja1105.c:
> > + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
> > + */
> > +#include <linux/dsa/8021q.h>
> > +
> > +#include "tag.h"
> > +#include "tag_8021q.h"
> > +
> > +#define VSC73XX_8021Q_NAME "vsc73xx-8021q"
> > +
> > +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > + struct dsa_port *dp = dsa_slave_to_port(netdev);
> > + u16 queue_mapping = skb_get_queue_mapping(skb);
> > + u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
> > + u8 pcp;
> > +
> > + if (skb->offload_fwd_mark) {
> > + unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> > + struct net_device *br = dsa_port_bridge_dev_get(dp);
> > +
> > + if (br_vlan_enabled(br))
> > + return skb;
> > +
> > + tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
> > + }
> > +
> > + pcp = netdev_txq_to_tc(netdev, queue_mapping);
> > +
> > + return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
> > + ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
> > +}
> > +
> > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> > + int *switch_id, int *vbid, u16 *vid)
> > +{
> > + if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> > + return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> > +
> > + /* Try our best with imprecise RX */
> > + *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> > +}
> > +
> > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > + int src_port = -1, switch_id = -1, vbid = -1;
> > + u16 vid;
> > +
> > + if (skb_vlan_tag_present(skb)) {
> > + /* Normal traffic path. */
> > + vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> > + } else {
> > + netdev_warn(netdev, "Couldn't decode source port\n");
> > + return NULL;
> > + }
> > +
> > + if (vbid >= 1)
> > + skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> > + else if (src_port == -1 || switch_id == -1)
> > + skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
> > + else
> > + skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
>
> Hmm, this isn't looking too good.
>
> I think the fact that you had to add my copyright on what should be such
> a simple thing as a VLAN-based tagger is a bad sign :)
>
> It's time to consolidate some more stuff that currently lives in
> tag_sja1105 and move it into tag_8021q so that you can reuse it more
> easily.
>
> I've prepared some (only compile-tested) patches on this branch here:
> https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3
>
> I need to double-check that they don't introduce regressions,
I tested it on my device and I couldn't find any regressions. vlan
filtering and tagging work as expected.
> and we
> should discuss the merging strategy. Probably you're going to submit
> them together with your patch set.
I prepared the v4 patch series. Please look if that format is ok with you.
https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next
>
> With that, you can drop my part of the copyright :) The remainder should
> look like straightforward use of API which can be written in only a
> limited number of ways.
Now it is much simpler.
>
> > + if (!skb->dev) {
> > + netdev_warn(netdev, "Couldn't decode source port\n");
> > + return NULL;
> > + }
> > +
> > + dsa_default_offload_fwd_mark(skb);
> > +
> > + if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
> > + eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
> > + __vlan_hwaccel_clear_tag(skb);
>
> Why do you need to do this? We send VLAN-tagged packets to the
> VLAN-aware bridge intentionally, so that it knows what VLAN they come
> from (in the dsa_find_designated_bridge_port_by_vid() case). So don't
> strip it if it's not causing a problem.
>
I dropped it in v4. I needed it when I started with this patch series,
because bridge in vlan filtering causes double tagged frames (one from
hardware and one from bridge). But after recent changes it is useless
and it could be dropped because vlan works as expected without it.
> > +
> > + return skb;
> > +}
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
2023-09-25 20:55 ` Paweł Dembicki
@ 2023-09-27 0:11 ` Vladimir Oltean
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-27 0:11 UTC (permalink / raw)
To: Paweł Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
On Mon, Sep 25, 2023 at 10:55:29PM +0200, Paweł Dembicki wrote:
> > I've prepared some (only compile-tested) patches on this branch here:
> > https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3
> >
> > I need to double-check that they don't introduce regressions,
>
> I tested it on my device and I couldn't find any regressions. vlan
> filtering and tagging work as expected.
Thanks for testing. Obviously I still haven't :-/ Please feel free to
post the next version and I'll try to find some time to test.
> > and we
> > should discuss the merging strategy. Probably you're going to submit
> > them together with your patch set.
>
> I prepared the v4 patch series. Please look if that format is ok with you.
> https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next
I still have some style nitpicks similar to the ones expressed already,
but it isn't trivial for me to leave them through Github, so you can
post your best attempt at v4. You should also try to add a comment
explaining what's the reason for the special case for "if (port !=
CPU_PORT)" in vsc73xx_port_vlan_add().
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next v3 7/8] net: dsa: vsc73xx: Implement vsc73xx 8021q tagger
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
` (5 preceding siblings ...)
2023-09-12 12:22 ` [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx Pawel Dembicki
@ 2023-09-12 12:22 ` Pawel Dembicki
2023-09-12 12:22 ` [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support Pawel Dembicki
7 siblings, 0 replies; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:22 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel
This patch is a simple implementation of 802.1q tagging in the vsc73xx
driver. Currently, devices with DSA_TAG_PROTO_NONE are not functional.
The VSC73XX family doesn't provide any tag support for external Ethernet
ports.
The only option available is VLAN-based tagging, which requires constant
hardware VLAN filtering. While the VSC73XX family supports provider
bridging, it only supports QinQ without full implementation of 802.1AD.
This means it only allows the doubled 0x8100 TPID.
In the simple port mode, QinQ is enabled to preserve forwarding of
VLAN-tagged frames.
The tag driver introduces the most basic functionality required for
proper tagging support.
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- Split tagger and tag implementation into separate commits
drivers/net/dsa/Kconfig | 2 +-
drivers/net/dsa/vitesse-vsc73xx-core.c | 48 +++++++++++++++++++++++++-
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index f8c1d73b251d..120bc3ded9f2 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -126,7 +126,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
config NET_DSA_VITESSE_VSC73XX
tristate
- select NET_DSA_TAG_NONE
+ select NET_DSA_TAG_VSC73XX_8021Q
select FIXED_PHY
select VITESSE_PHY
select GPIOLIB
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index d9a6eac1fcce..bf903502bac1 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/device.h>
+#include <linux/iopoll.h>
#include <linux/of.h>
#include <linux/of_mdio.h>
#include <linux/bitops.h>
@@ -589,7 +590,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
* cannot access the tag. (See "Internal frame header" section
* 3.9.1 in the manual.)
*/
- return DSA_TAG_PROTO_NONE;
+ return DSA_TAG_PROTO_VSC73XX_8021Q;
}
static int vsc73xx_setup(struct dsa_switch *ds)
@@ -664,6 +665,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
mdelay(50);
+ rtnl_lock();
+ ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
+ rtnl_unlock();
+ if (ret)
+ return ret;
+
/* Release reset from the internal PHYs */
vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
VSC73XX_GLORESET_PHY_RESET);
@@ -1412,6 +1419,43 @@ static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
return 0;
}
+static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
+ u16 flags)
+{
+ bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
+ struct vsc73xx *vsc = ds->priv;
+ int ret;
+
+ if (untagged) {
+ if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) {
+ ret = vsc73xx_vlan_set_untagged(ds, port, vid, false);
+ if (ret)
+ return ret;
+ } else {
+ vsc->untagged_storage[port] = vid;
+ }
+ }
+ if (pvid) {
+ if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) {
+ ret = vsc73xx_vlan_set_pvid(ds, port, vid, false);
+ if (ret)
+ return ret;
+ } else {
+ vsc->pvid_storage[port] = vid;
+ }
+ }
+
+ return vsc73xx_update_vlan_table(vsc, port, vid, 1);
+}
+
+static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
+{
+ struct vsc73xx *vsc = ds->priv;
+
+ return vsc73xx_update_vlan_table(vsc, port, vid, 0);
+}
+
static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
{
struct vsc73xx *vsc = ds->priv;
@@ -1519,6 +1563,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_vlan_filtering = vsc73xx_port_vlan_filtering,
.port_vlan_add = vsc73xx_port_vlan_add,
.port_vlan_del = vsc73xx_port_vlan_del,
+ .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
+ .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
};
static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support
2023-09-12 12:21 [PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable Pawel Dembicki
` (6 preceding siblings ...)
2023-09-12 12:22 ` [PATCH net-next v3 7/8] net: dsa: vsc73xx: Implement vsc73xx 8021q tagger Pawel Dembicki
@ 2023-09-12 12:22 ` Pawel Dembicki
2023-09-12 22:23 ` Vladimir Oltean
7 siblings, 1 reply; 27+ messages in thread
From: Pawel Dembicki @ 2023-09-12 12:22 UTC (permalink / raw)
To: netdev
Cc: Dan Carpenter, Simon Horman, Pawel Dembicki, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel
This patch adds bridge support for vsc73xx driver.
It introduce two functions for port_bridge_join and
vsc73xx_port_bridge_leave handling.
Those functions implement forwarding adjust and use
dsa_tag_8021q_bridge_* api for adjust VLAN configuration.
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
- All vlan commits was reworked
- move VLAN_AWR and VLAN_DBLAWR to port setup in other commit
- drop vlan table upgrade
v2:
- no changes done
drivers/net/dsa/vitesse-vsc73xx-core.c | 72 ++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index bf903502bac1..9d0367c2c52c 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -600,6 +600,9 @@ static int vsc73xx_setup(struct dsa_switch *ds)
dev_info(vsc->dev, "set up the switch\n");
+ ds->untag_bridge_pvid = true;
+ ds->max_num_bridges = 7;
+
/* Issue RESET */
vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
VSC73XX_GLORESET_MASTER_RESET);
@@ -1456,6 +1459,73 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
return vsc73xx_update_vlan_table(vsc, port, vid, 0);
}
+static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
+{
+ int i;
+
+ for (i = 0; i < vsc->ds->num_ports; i++) {
+ u32 val;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i, &val);
+ /* update only if port is in forwarding state */
+ if (val & VSC73XX_SRCMASKS_PORTS_MASK)
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i,
+ VSC73XX_SRCMASKS_PORTS_MASK,
+ vsc->forward_map[i]);
+ }
+}
+
+static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge,
+ bool *tx_fwd_offload,
+ struct netlink_ext_ack *extack)
+{
+ struct vsc73xx *vsc = ds->priv;
+ int i;
+
+ *tx_fwd_offload = true;
+
+ for (i = 0; i < ds->num_ports; i++) {
+ /* Add this port to the forwarding matrix of the
+ * other ports in the same bridge, and viceversa.
+ */
+ if (!dsa_is_user_port(ds, i))
+ continue;
+
+ if (i == port)
+ continue;
+
+ if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+ continue;
+
+ vsc->forward_map[port] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(i);
+ vsc->forward_map[i] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(port);
+ }
+ vsc73xx_update_forwarding_map(vsc);
+
+ return dsa_tag_8021q_bridge_join(ds, port, bridge);
+}
+
+static void vsc73xx_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge)
+{
+ struct vsc73xx *vsc = ds->priv;
+ int i;
+
+ /* configure forward map to CPU <-> port only */
+ for (i = 0; i < vsc->ds->num_ports; i++) {
+ if (i == CPU_PORT)
+ continue;
+ vsc->forward_map[i] &= VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(port);
+ }
+ vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
+
+ vsc73xx_update_forwarding_map(vsc);
+ dsa_tag_8021q_bridge_leave(ds, port, bridge);
+}
+
static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
{
struct vsc73xx *vsc = ds->priv;
@@ -1557,6 +1627,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.get_sset_count = vsc73xx_get_sset_count,
.port_enable = vsc73xx_port_enable,
.port_disable = vsc73xx_port_disable,
+ .port_bridge_join = vsc73xx_port_bridge_join,
+ .port_bridge_leave = vsc73xx_port_bridge_leave,
.port_change_mtu = vsc73xx_change_mtu,
.port_max_mtu = vsc73xx_get_max_mtu,
.port_stp_state_set = vsc73xx_port_stp_state_set,
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support
2023-09-12 12:22 ` [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support Pawel Dembicki
@ 2023-09-12 22:23 ` Vladimir Oltean
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Oltean @ 2023-09-12 22:23 UTC (permalink / raw)
To: Pawel Dembicki
Cc: netdev, Dan Carpenter, Simon Horman, Andrew Lunn,
Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Russell King, linux-kernel
On Tue, Sep 12, 2023 at 02:22:02PM +0200, Pawel Dembicki wrote:
> This patch adds bridge support for vsc73xx driver.
> It introduce two functions for port_bridge_join and
> vsc73xx_port_bridge_leave handling.
>
> Those functions implement forwarding adjust and use
> dsa_tag_8021q_bridge_* api for adjust VLAN configuration.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v3:
> - All vlan commits was reworked
> - move VLAN_AWR and VLAN_DBLAWR to port setup in other commit
> - drop vlan table upgrade
> v2:
> - no changes done
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 72 ++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index bf903502bac1..9d0367c2c52c 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -600,6 +600,9 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>
> dev_info(vsc->dev, "set up the switch\n");
>
> + ds->untag_bridge_pvid = true;
> + ds->max_num_bridges = 7;
Can you please refactor this into DSA_TAG_8021Q_MAX_NUM_BRIDGES and use
it in sja1105 too?
> +
> /* Issue RESET */
> vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> VSC73XX_GLORESET_MASTER_RESET);
> @@ -1456,6 +1459,73 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
> return vsc73xx_update_vlan_table(vsc, port, vid, 0);
> }
>
> +static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> +{
> + int i;
> +
> + for (i = 0; i < vsc->ds->num_ports; i++) {
> + u32 val;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + i, &val);
> + /* update only if port is in forwarding state */
> + if (val & VSC73XX_SRCMASKS_PORTS_MASK)
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + i,
> + VSC73XX_SRCMASKS_PORTS_MASK,
> + vsc->forward_map[i]);
> + }
> +}
I suspect you'll have to rethink this. If you look at del_nbp() and dsa_port_bridge_leave(),
you'll see it goes through a few phases. First the bridge calls br_stp_disable_port(p),
then netdev_upper_dev_unlink(dev, br->dev) which invokes dsa_port_bridge_leave().
So at this stage, the port is in BR_STATE_DISABLED and ds->ops->port_stp_state_set()
duly notifies the driver of that.
Then, ds->ops->port_bridge_leave() gets called, and then, ds->ops->port_stp_state_set()
again, for the standalone port, in BR_STATE_FORWARDING.
So actually, the last to take effect upon the forwarding map is port_stp_state_set(),
not port_bridge_leave().
I suspect you can remove the vsc73xx_update_forwarding_map() and just
rely on the STP implementation, and fix that while you're at it.
On the other switch ports except the one on which the STP state is changing,
you can look at dp->stp_state. There needs to be an "administrative" forwarding
mask (determined by port_bridge_join and port_bridge_leave), and an "operational"
one (determined by STP states).
> +
> +static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge,
> + bool *tx_fwd_offload,
> + struct netlink_ext_ack *extack)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + int i;
> +
> + *tx_fwd_offload = true;
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + /* Add this port to the forwarding matrix of the
> + * other ports in the same bridge, and viceversa.
> + */
> + if (!dsa_is_user_port(ds, i))
> + continue;
dsa_switch_for_each_user_port(other_dp, ds) please
it is a lower-complexity iteration over the port list, which should be
preferred over a for loop + any dsa_to_port() wrapper like dsa_is_user_port().
> +
> + if (i == port)
> + continue;
> +
> + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
and "other_dp" here
> + continue;
> +
> + vsc->forward_map[port] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(i);
> + vsc->forward_map[i] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(port);
> + }
> + vsc73xx_update_forwarding_map(vsc);
> +
> + return dsa_tag_8021q_bridge_join(ds, port, bridge);
> +}
> +
> +static void vsc73xx_port_bridge_leave(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + int i;
> +
> + /* configure forward map to CPU <-> port only */
> + for (i = 0; i < vsc->ds->num_ports; i++) {
> + if (i == CPU_PORT)
> + continue;
> + vsc->forward_map[i] &= VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(port);
> + }
> + vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
> +
> + vsc73xx_update_forwarding_map(vsc);
> + dsa_tag_8021q_bridge_leave(ds, port, bridge);
> +}
> +
> static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> {
> struct vsc73xx *vsc = ds->priv;
> @@ -1557,6 +1627,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .get_sset_count = vsc73xx_get_sset_count,
> .port_enable = vsc73xx_port_enable,
> .port_disable = vsc73xx_port_disable,
> + .port_bridge_join = vsc73xx_port_bridge_join,
> + .port_bridge_leave = vsc73xx_port_bridge_leave,
> .port_change_mtu = vsc73xx_change_mtu,
> .port_max_mtu = vsc73xx_get_max_mtu,
> .port_stp_state_set = vsc73xx_port_stp_state_set,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread