This patch series focuses on making vsc73xx usable.
The first patch was added in v2; it switches from a poll loop to
read_poll_timeout.
The second patch is a simple conversion to phylink because adjust_link
won't work anymore.
The third patch is preparation for future use. Using the
"phy_interface_mode_is_rgmii" macro allows for the proper recognition
of all RGMII modes.
Patches 4-5 involve some cleanup: The fourth patch introduces
a definition with the maximum number of ports to avoid using
magic numbers. The next one fills in documentation.
The sixth patch implements port state configuration, which is required
for bridge functionality. STP frames are not forwarded at this moment.
BPDU frames are only forwarded from/to the PI/SI interface.
For more information, see chapter 2.7.1 (CPU Forwarding) in the
datasheet.
Patches 7, 12-15 provide a basic implementation of tag8021q
functionality with QinQ support, without VLAN filtering in
the bridge and simple VLAN awareness in VLAN filtering mode.
Patches 8-11 came from Vladimir Oltean. They prepare for making
tag8021q more common. VSC73XX uses very similar tag recognition,
and some code from tag_sja1105 could be moved to tag8021q for
common use.
Patch 16 is required to avoid problem with learning on standalone ports.
Pawel Dembicki (12):
net: dsa: vsc73xx: use read_poll_timeout instead delay loop
net: dsa: vsc73xx: convert to PHYLINK
net: dsa: vsc73xx: use macros for rgmii recognition
net: dsa: vsc73xx: Add define for max num of ports
net: dsa: vsc73xx: add structure descriptions
net: dsa: vsc73xx: add port_stp_state_set function
net: dsa: vsc73xx: Add vlan filtering
net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
net: dsa: vsc73xx: Implement the tag_8021q VLAN operations
net: dsa: Define max num of bridges in tag8021q implementation
net: dsa: vsc73xx: Add bridge support
net: dsa: vsc73xx: start treating the BR_LEARNING flag
Vladimir Oltean (4):
net: dsa: tag_sja1105: absorb logic for not overwriting precise info
into dsa_8021q_rcv()
net: dsa: tag_sja1105: absorb entire sja1105_vlan_rcv() into
dsa_8021q_rcv()
net: dsa: tag_sja1105: prefer precise source port info on SJA1110 too
net: dsa: tag_sja1105: refactor skb->dev assignment to
dsa_tag_8021q_find_user()
drivers/net/dsa/Kconfig | 2 +-
drivers/net/dsa/sja1105/sja1105_main.c | 3 +-
drivers/net/dsa/vitesse-vsc73xx-core.c | 990 +++++++++++++++++++++----
drivers/net/dsa/vitesse-vsc73xx.h | 56 +-
include/linux/dsa/8021q.h | 5 +
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +
net/dsa/Makefile | 1 +
net/dsa/tag_8021q.c | 81 +-
net/dsa/tag_8021q.h | 7 +-
net/dsa/tag_ocelot_8021q.c | 2 +-
net/dsa/tag_sja1105.c | 72 +-
net/dsa/tag_vsc73xx_8021q.c | 67 ++
13 files changed, 1072 insertions(+), 222 deletions(-)
create mode 100644 net/dsa/tag_vsc73xx_8021q.c
--
2.34.1
Switch the delay loop during the Arbiter empty check from
vsc73xx_adjust_link() to use read_poll_timeout(). Functionally,
one msleep() call is eliminated at the end of the loop in the timeout
case.
As Russell King suggested:
"This [change] avoids the issue that on the last iteration, the code reads
the register, tests it, finds 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."
Suggested-by: Russell King <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- improved commit description
- added two defines instead magic numbers
- added include with iopoll.h
v4:
- resend patch
v3:
- added "Reviewed-by" to commit message only
v2:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 30 ++++++++++++++------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ae70eac3be28..ab5771d4d828 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>
@@ -268,6 +269,9 @@
#define IS_7398(a) ((a)->chipid == VSC73XX_CHIPID_ID_7398)
#define IS_739X(a) (IS_7395(a) || IS_7398(a))
+#define VSC73XX_POLL_SLEEP_US 1000
+#define VSC73XX_POLL_TIMEOUT_US 10000
+
struct vsc73xx_counter {
u8 counter;
const char *name;
@@ -779,7 +783,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 +798,17 @@ 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)),
+ VSC73XX_POLL_SLEEP_US,
+ VSC73XX_POLL_TIMEOUT_US, 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
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_mac_config
phylink_mac_link_up
phylink_mac_link_down
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- introduce MAC reset function with procedure described in datasheet
- reorganise routines
- move 'Accept packets again' routine to 'phylink_mac_link_up'
- fix one FIXME
v4:
- update commit descripion
- remove phylink_get_caps after rebase to current net-next/main
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 | 235 ++++++++++++-------------
1 file changed, 109 insertions(+), 126 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ab5771d4d828..cb2e7e256279 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -717,52 +717,43 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
port, VSC73XX_C_RX0, 0);
}
-static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
- int port, struct phy_device *phydev,
- u32 initval)
+static void vsc73xx_reset_port(struct vsc73xx *vsc, int port, u32 initval)
{
- u32 val = initval;
- u8 seed;
-
- /* Reset this port FIXME: break out subroutine */
- val |= VSC73XX_MAC_CFG_RESET;
- vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
-
- /* Seed the port randomness with randomness */
- get_random_bytes(&seed, 1);
- val |= seed << VSC73XX_MAC_CFG_SEED_OFFSET;
- val |= VSC73XX_MAC_CFG_SEED_LOAD;
- val |= VSC73XX_MAC_CFG_WEXC_DIS;
- vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
+ int ret, err;
+ u32 val;
- /* Flow control for the PHY facing ports:
- * Use a zero delay pause frame when pause condition is left
- * Obey pause control frames
- * When generating pause frames, use 0xff as pause value
- */
- vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_FCCONF,
- VSC73XX_FCCONF_ZERO_PAUSE_EN |
- VSC73XX_FCCONF_FLOW_CTRL_OBEY |
- 0xff);
+ /* Disable RX on this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RX_EN, 0);
- /* Disallow backward dropping of frames from this port */
+ /* Discard packets */
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_SBACKWDROP, BIT(port), 0);
+ VSC73XX_ARBDISC, BIT(port), BIT(port));
+
+ /* Wait until queue is empty */
+ ret = read_poll_timeout(vsc73xx_read, err,
+ err < 0 || (val & BIT(port)),
+ VSC73XX_POLL_SLEEP_US,
+ VSC73XX_POLL_TIMEOUT_US, 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");
- /* Enable TX, RX, deassert reset, stop loading seed */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
- VSC73XX_MAC_CFG,
- VSC73XX_MAC_CFG_RESET | VSC73XX_MAC_CFG_SEED_LOAD |
- VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN,
- VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
+ /* Put this port into reset */
+ vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RESET | initval);
}
-static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
+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;
- u32 val;
-
/* Special handling of the CPU-facing port */
if (port == CPU_PORT) {
/* Other ports are already initialized but not this one */
@@ -778,102 +769,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
VSC73XX_ADVPORTM_ENA_GTX |
VSC73XX_ADVPORTM_DDR_MODE);
}
+}
+
+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;
- /* This is the MAC confiuration that always need to happen
- * after a PHY or the CPU port comes up or down.
+ /* This routine is described in the datasheet (below ARBDISC register
+ * description)
*/
- 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)),
- VSC73XX_POLL_SLEEP_US,
- VSC73XX_POLL_TIMEOUT_US, 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);
+ vsc73xx_reset_port(vsc, port, 0);
- return;
- }
+ /* Allow backward dropping of frames from this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_SBACKWDROP, BIT(port), BIT(port));
- /* 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 {
- dev_err(vsc->dev,
- "could not adjust link: unknown speed\n");
- }
+ /* 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;
+ u8 seed;
+
+ 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;
+
+ /* This routine is described in the datasheet (below ARBDISC register
+ * description)
+ */
+ vsc73xx_reset_port(vsc, port, val);
+
+ /* Seed the port randomness with randomness */
+ get_random_bytes(&seed, 1);
+ val |= seed << VSC73XX_MAC_CFG_SEED_OFFSET;
+ val |= VSC73XX_MAC_CFG_SEED_LOAD;
+ val |= VSC73XX_MAC_CFG_WEXC_DIS;
+ vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
+
+ /* Flow control for the PHY facing ports:
+ * Use a zero delay pause frame when pause condition is left
+ * Obey pause control frames
+ * When generating pause frames, use 0xff as pause value
+ */
+ vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_FCCONF,
+ VSC73XX_FCCONF_ZERO_PAUSE_EN |
+ VSC73XX_FCCONF_FLOW_CTRL_OBEY |
+ 0xff);
+
+ /* Accept packets again */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBDISC, BIT(port), 0);
/* Enable port (forwarding) in the receieve mask */
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_RECVMASK, BIT(port), BIT(port));
+
+ /* Disallow backward dropping of frames from this port */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_SBACKWDROP, BIT(port), 0);
+
+ /* Enable TX, RX, deassert reset, stop loading seed */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_MAC_CFG,
+ VSC73XX_MAC_CFG_RESET | VSC73XX_MAC_CFG_SEED_LOAD |
+ VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN,
+ VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
}
static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
@@ -1060,7 +1041,9 @@ 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_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
It's preparation for future use. At this moment, the RGMII port is used
only for a connection to the MAC interface, but in the future, someone
could connect a PHY to it. Using the "phy_interface_mode_is_rgmii" macro
allows for the proper recognition of all RGMII modes.
Suggested-by: Russell King <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v6:
- resend only
v5:
- added 'Reviewed-by' only
v4:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index cb2e7e256279..8ed8fc997d6d 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -807,7 +807,7 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
else
val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
- if (interface == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_mode_is_rgmii(interface))
val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
else
val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
--
2.34.1
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 <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v6:
- resend only
v5:
- added 'Reviewed-by' only
v4:
- Resend patch
v3:
- Introduce patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 13 +------------
drivers/net/dsa/vitesse-vsc73xx.h | 11 +++++++++++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 8ed8fc997d6d..425999d7bf41 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1180,23 +1180,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..fee1378508b5 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -3,6 +3,17 @@
#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 nonexistent ports.
+ */
+#define VSC73XX_MAX_NUM_PORTS 8
+
/**
* struct vsc73xx - VSC73xx state container
*/
--
2.34.1
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.
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- fix inconsistent indenting
v5:
- remove unneeded 'RECVMASK' operations
- reorganise vsc73xx_refresh_fwd_map function
v4:
- fully reworked port_stp_state_set
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 | 99 +++++++++++++++++++++++---
1 file changed, 88 insertions(+), 11 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 425999d7bf41..d1e84a9a83d1 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -164,6 +164,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)
@@ -623,9 +627,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);
@@ -785,10 +786,6 @@ static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
/* 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,
@@ -841,10 +838,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_ARBDISC, BIT(port), 0);
- /* Enable port (forwarding) in the receieve mask */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
- VSC73XX_RECVMASK, BIT(port), BIT(port));
-
/* Disallow backward dropping of frames from this port */
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_SBACKWDROP, BIT(port), 0);
@@ -1036,6 +1029,89 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
}
+static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
+{
+ struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
+ struct vsc73xx *vsc = ds->priv;
+ u16 mask;
+
+ if (state != BR_STATE_FORWARDING) {
+ /* Ports that aren't in the forwarding state must not
+ * forward packets anywhere.
+ */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + port,
+ VSC73XX_SRCMASKS_PORTS_MASK, 0);
+
+ dsa_switch_for_each_available_port(other_dp, ds) {
+ if (other_dp == dp)
+ continue;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + other_dp->index,
+ BIT(port), 0);
+ }
+
+ return;
+ }
+
+ /* Forwarding ports must forward to the CPU and to other ports
+ * in the same bridge
+ */
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
+
+ mask = BIT(CPU_PORT);
+
+ if (dp->bridge) {
+ dsa_switch_for_each_user_port(other_dp, ds) {
+ if (other_dp->bridge == dp->bridge &&
+ other_dp->index != port &&
+ other_dp->stp_state == BR_STATE_FORWARDING) {
+ int other_port = other_dp->index;
+
+ mask |= BIT(other_port);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER,
+ 0,
+ VSC73XX_SRCMASKS +
+ other_port,
+ BIT(port), BIT(port));
+ }
+ }
+ }
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + port,
+ VSC73XX_SRCMASKS_PORTS_MASK, mask);
+}
+
+/* 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 tag_8021q operations.
+ */
+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
+ u8 state)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u32 val;
+
+ val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ?
+ 0 : BIT(port);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_RECVMASK, BIT(port), val);
+
+ val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ?
+ BIT(port) : 0;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_LEARNMASK, BIT(port), val);
+
+ /* CPU Port should always forward packets when user ports are forwarding
+ * so let's configure it from other ports only.
+ */
+ if (port != CPU_PORT)
+ vsc73xx_refresh_fwd_map(ds, port, state);
+}
+
static const struct dsa_switch_ops vsc73xx_ds_ops = {
.get_tag_protocol = vsc73xx_get_tag_protocol,
.setup = vsc73xx_setup,
@@ -1051,6 +1127,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,
.phylink_get_caps = vsc73xx_phylink_get_caps,
};
--
2.34.1
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 <[email protected]>
---
v6:
- resend only
v5:
- fix possible leak in 'vsc73xx_port_vlan_add'
- use proper variable in statement from 'vsc73xx_port_vlan_filtering'
- change 'vlan_no' name to 'vid'
- codding style improvements
- comment improvements
- handle return of 'vsc73xx_update_bits'
- reduce I/O operations
- use 'size_t' for counting variables
v4:
- reworked most of conditional register configs
- simplified port_vlan function
- move vlan table clearing from port_setup to setup
- pvid configuration simplified (now kernel take care about no of
pvids per port)
- port vlans are stored in list now
- introduce implementation of all untagged vlans state
- many minor changes
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
v2:
- no changes done
drivers/net/dsa/vitesse-vsc73xx-core.c | 570 ++++++++++++++++++++++++-
drivers/net/dsa/vitesse-vsc73xx.h | 29 ++
2 files changed, 597 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index d1e84a9a83d1..c643f445f026 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -22,9 +22,11 @@
#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>
@@ -62,6 +64,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 */
@@ -122,6 +126,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)
@@ -135,6 +150,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
@@ -189,7 +213,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
@@ -347,6 +372,12 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
{ 29, "TxQoSClass3" }, /* non-standard counter */
};
+enum vsc73xx_port_vlan_conf {
+ VSC73XX_VLAN_FILTER,
+ VSC73XX_VLAN_FILTER_UNTAG_ALL,
+ VSC73XX_VLAN_IGNORE,
+};
+
int vsc73xx_is_addr_valid(u8 block, u8 subblock)
{
switch (block) {
@@ -564,6 +595,91 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
return DSA_TAG_PROTO_NONE;
}
+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),
+ VSC73XX_POLL_SLEEP_US, VSC73XX_POLL_TIMEOUT_US,
+ 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);
+ else
+ portmap &= ~BIT(port);
+
+ return vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
+}
+
static int vsc73xx_setup(struct dsa_switch *ds)
{
struct vsc73xx *vsc = ds->priv;
@@ -598,7 +714,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
VSC73XX_MACACCESS,
VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
- /* Clear VLAN table */
+ /* Set VLAN table to default values */
vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_VLANACCESS,
VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
@@ -627,6 +743,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);
@@ -639,6 +758,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
udelay(4);
+ /* Clear VLAN table */
+ for (i = 0; i < VLAN_N_VID; i++)
+ vsc73xx_write_vlan_table_entry(vsc, i, 0);
+
+ INIT_LIST_HEAD(&vsc->vlans);
+
return 0;
}
@@ -1029,6 +1154,443 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
}
+static void
+vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
+ enum vsc73xx_port_vlan_conf port_vlan_conf)
+{
+ u32 val = 0;
+
+ if (port_vlan_conf == VSC73XX_VLAN_IGNORE)
+ val = VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA |
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA;
+
+ 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_KEEP_TAG_ENA, val);
+
+ val = (port_vlan_conf == VSC73XX_VLAN_FILTER) ?
+ VSC73XX_TXUPDCFG_TX_INSERT_TAG : 0;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_INSERT_TAG, val);
+}
+
+static int
+vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set,
+ bool operate_on_storage)
+{
+ u32 val = 0;
+
+ if (operate_on_storage) {
+ vsc->untagged_storage[port] = set ? vid : VLAN_N_VID;
+ return 0;
+ }
+
+ if (set)
+ val = VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
+ ((vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
+
+ return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, val);
+}
+
+static int vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid,
+ bool set, bool operate_on_storage)
+{
+ int ret;
+ u32 val;
+
+ if (operate_on_storage) {
+ vsc->pvid_storage[port] = set ? vid : VLAN_N_VID;
+ return 0;
+ }
+
+ val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
+ vid = set ? vid : 0;
+
+ ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
+ if (ret)
+ return ret;
+
+ return 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);
+}
+
+static bool vsc73xx_port_get_pvid(struct vsc73xx *vsc, int port, u16 *vid,
+ bool operate_on_storage)
+{
+ u32 val;
+
+ if (operate_on_storage) {
+ if (vsc->pvid_storage[port] < VLAN_N_VID) {
+ *vid = vsc->pvid_storage[port];
+ return true;
+ }
+ return false;
+ }
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
+ if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
+ return false;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+ *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+
+ return true;
+}
+
+static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
+{
+ return !dsa_port_is_vlan_filtering(dp);
+}
+
+static bool vsc73xx_port_get_untagged(struct vsc73xx *vsc, int port, u16 *vid,
+ bool operate_on_storage)
+{
+ u32 val;
+
+ if (operate_on_storage) {
+ if (vsc->untagged_storage[port] < VLAN_N_VID) {
+ *vid = vsc->untagged_storage[port];
+ return true;
+ }
+ return false;
+ }
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+ if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
+ return false;
+
+ *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+
+ return true;
+}
+
+static struct vsc73xx_bridge_vlan *
+vsc73xx_bridge_vlan_find(struct vsc73xx *vsc, u16 vid)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+
+ list_for_each_entry(vlan, &vsc->vlans, list)
+ if (vlan->vid == vid)
+ return vlan;
+
+ return NULL;
+}
+
+static size_t
+vsc73xx_bridge_vlan_num_tagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+ size_t num_tagged = 0;
+
+ list_for_each_entry(vlan, &vsc->vlans, list)
+ if ((vlan->portmask & BIT(port)) &&
+ !(vlan->untagged & BIT(port)) &&
+ vlan->vid != ignored_vid)
+ num_tagged++;
+
+ return num_tagged;
+}
+
+static size_t
+vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+ size_t num_untagged = 0;
+
+ list_for_each_entry(vlan, &vsc->vlans, list)
+ if ((vlan->portmask & BIT(port)) &&
+ (vlan->untagged & BIT(port)) &&
+ vlan->vid != ignored_vid)
+ num_untagged++;
+
+ return num_untagged;
+}
+
+static u16 vsc73xx_find_first_vlan_untagged(struct vsc73xx *vsc, int port)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+
+ list_for_each_entry(vlan, &vsc->vlans, list)
+ if ((vlan->portmask & BIT(port)) &&
+ (vlan->untagged & BIT(port)))
+ return vlan->vid;
+
+ return VLAN_N_VID;
+}
+
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+ enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE;
+ struct vsc73xx *vsc = ds->priv;
+ bool store_untagged = false;
+ bool store_pvid = false;
+ u16 vid, vlan_untagged;
+
+ /* The swap processed below is required because vsc73xx is using
+ * tag_8021q. When vlan_filtering is disabled, tag_8021q uses
+ * pvid/untagged vlans for port recognition. The values configured for
+ * vlans < 3072 are stored in storage table. When vlan_filtering is
+ * enabled, we need to restore pvid/untagged from storage and keep
+ * values used for tag_8021q.
+ */
+ if (vlan_filtering) {
+ /* Use VLAN_N_VID to count all vlans */
+ size_t num_untagged =
+ vsc73xx_bridge_vlan_num_untagged(vsc, port, VLAN_N_VID);
+
+ port_vlan_conf = (num_untagged > 1) ?
+ VSC73XX_VLAN_FILTER_UNTAG_ALL :
+ VSC73XX_VLAN_FILTER;
+
+ vlan_untagged = vsc73xx_find_first_vlan_untagged(vsc, port);
+ if (vlan_untagged < VLAN_N_VID) {
+ store_untagged = vsc73xx_port_get_untagged(vsc, port,
+ &vid,
+ false);
+ vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
+ true, false);
+ vsc->untagged_storage[port] = store_untagged ?
+ vid : VLAN_N_VID;
+ }
+ } else {
+ vsc73xx_vlan_change_untagged(vsc, port,
+ vsc->untagged_storage[port],
+ vsc->untagged_storage[port] <
+ VLAN_N_VID, false);
+ }
+
+ vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
+
+ store_pvid = vsc73xx_port_get_pvid(vsc, port, &vid, false);
+ vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port],
+ vsc->pvid_storage[port] < VLAN_N_VID, false);
+ vsc->pvid_storage[port] = store_pvid ? vid : 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 dsa_port *dp = dsa_to_port(ds, port);
+ struct vsc73xx_bridge_vlan *vsc73xx_vlan;
+ size_t num_tagged, num_untagged;
+ struct vsc73xx *vsc = ds->priv;
+ int ret;
+ u16 vid;
+
+ /* 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;
+ }
+
+ /* The processed vlan->vid is excluded from the search because the VLAN
+ * can be re-added with a different set of flags, so it's easiest to
+ * ignore its old flags from the VLAN database software copy.
+ */
+ num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
+ num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
+
+ /* VSC73XX allow only three untagged states: none, one or all */
+ if ((untagged && num_tagged > 0 && num_untagged > 0) ||
+ (!untagged && num_untagged > 1)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Port can have only none, one or all untagged vlan");
+ return -EBUSY;
+ }
+
+ vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
+
+ if (!vsc73xx_vlan) {
+ vsc73xx_vlan = kzalloc(sizeof(*vsc73xx_vlan), GFP_KERNEL);
+ if (!vsc73xx_vlan)
+ return -ENOMEM;
+
+ vsc73xx_vlan->vid = vlan->vid;
+ vsc73xx_vlan->portmask = BIT(port);
+ vsc73xx_vlan->untagged = untagged ? BIT(port) : 0;
+
+ INIT_LIST_HEAD(&vsc73xx_vlan->list);
+ list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
+ } else {
+ vsc73xx_vlan->portmask |= BIT(port);
+
+ if (untagged)
+ vsc73xx_vlan->untagged |= BIT(port);
+ else
+ vsc73xx_vlan->untagged &= ~BIT(port);
+ }
+
+ /* CPU port must be always tagged because port separation is based on
+ * tag_8021q.
+ */
+ if (port != CPU_PORT) {
+ bool operate_on_storage = vsc73xx_tag_8021q_active(dp);
+
+ if (!operate_on_storage) {
+ enum vsc73xx_port_vlan_conf port_vlan_conf =
+ VSC73XX_VLAN_FILTER;
+
+ if (num_tagged == 0 && untagged)
+ port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
+ vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
+
+ if (port_vlan_conf == VSC73XX_VLAN_FILTER) {
+ if (untagged) {
+ ret = vsc73xx_vlan_change_untagged(vsc,
+ port,
+ vlan->vid,
+ true,
+ false);
+ if (ret)
+ goto err;
+ } else if (num_untagged == 1) {
+ vid = vsc73xx_find_first_vlan_untagged(vsc,
+ port);
+ ret = vsc73xx_vlan_change_untagged(vsc,
+ port,
+ vid,
+ true,
+ false);
+ if (ret)
+ goto err;
+ }
+ }
+ }
+
+ if (pvid) {
+ ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid,
+ true,
+ operate_on_storage);
+ if (ret)
+ goto err;
+ } else if (vsc73xx_port_get_pvid(vsc, port, &vid, false) &&
+ vid == vlan->vid) {
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
+ } else if (vsc->pvid_storage[port] == vlan->vid) {
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
+ }
+ }
+
+ ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
+ if (!ret)
+ return 0;
+err:
+ list_del(&vsc73xx_vlan->list);
+ kfree(vsc73xx_vlan);
+ return ret;
+}
+
+static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan)
+{
+ struct vsc73xx_bridge_vlan *vsc73xx_vlan;
+ size_t num_tagged, num_untagged;
+ struct vsc73xx *vsc = ds->priv;
+ bool operate_on_storage;
+ int ret;
+ u16 vid;
+
+ num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
+ num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
+
+ ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, false);
+ if (ret)
+ return ret;
+
+ operate_on_storage = vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
+
+ if (!operate_on_storage) {
+ enum vsc73xx_port_vlan_conf port_vlan_conf =
+ VSC73XX_VLAN_FILTER;
+
+ if (num_tagged == 0)
+ port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
+ vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
+
+ if (num_untagged <= 1) {
+ vid = vsc73xx_find_first_vlan_untagged(vsc, port);
+ vsc73xx_vlan_change_untagged(vsc, port, vid,
+ num_untagged, false);
+ }
+ } else if (vsc->untagged_storage[port] == vlan->vid) {
+ vsc73xx_vlan_change_untagged(vsc, port, 0, false, true);
+ }
+
+ if (vsc73xx_port_get_pvid(vsc, port, &vid, false) && vid == vlan->vid)
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
+ else if (vsc->pvid_storage[port] == vlan->vid)
+ vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
+
+ vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
+
+ if (vsc73xx_vlan) {
+ vsc73xx_vlan->portmask &= ~BIT(port);
+
+ if (vsc73xx_vlan->portmask)
+ return 0;
+
+ list_del(&vsc73xx_vlan->list);
+ kfree(vsc73xx_vlan);
+ }
+
+ return 0;
+}
+
+static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
+{
+ struct vsc73xx *vsc = ds->priv;
+
+ /* 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 |
+ 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 |
+ 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);
+
+ /* Initially, there is no backup VLAN configuration to keep track of, so
+ * configure the storage values out of range
+ */
+ vsc->pvid_storage[port] = VLAN_N_VID;
+ vsc->untagged_storage[port] = VLAN_N_VID;
+
+ return 0;
+}
+
static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
{
struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
@@ -1123,11 +1685,15 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.get_strings = vsc73xx_get_strings,
.get_ethtool_stats = vsc73xx_get_ethtool_stats,
.get_sset_count = vsc73xx_get_sset_count,
+ .port_setup = vsc73xx_port_setup,
.port_enable = vsc73xx_port_enable,
.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,
+ .port_vlan_filtering = vsc73xx_port_vlan_filtering,
+ .port_vlan_add = vsc73xx_port_vlan_add,
+ .port_vlan_del = vsc73xx_port_vlan_del,
.phylink_get_caps = vsc73xx_phylink_get_caps,
};
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index e7b08599a625..facc50f1e320 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -25,6 +25,17 @@
* @addr: MAC address used in flow control frames
* @ops: Structure with hardware-dependent operations
* @priv: Pointer to the configuration interface structure
+ * @pvid_storage: Storage table with PVID configured for other state of
+ * vlan_filtering. It has two alternating roles: it stores the PVID when
+ * configured by the bridge but VLAN filtering is off, and it stores the
+ * PVID necessary for tag_8021q operation when bridge VLAN filtering is
+ * enabled.
+ * @untagged_storage: Storage table with eggres untagged VLAN configured for
+ * other state of vlan_filtering.Keep VID necessary for tag8021q operations
+ * when vlan filtering is enabled.
+ * @vlans: List of configured vlans. Contains port mask and untagged status of
+ * every vlan configured in port vlan operation. It doesn't cover tag_8021q
+ * vlans.
*/
struct vsc73xx {
struct device *dev;
@@ -35,6 +46,9 @@ struct vsc73xx {
u8 addr[ETH_ALEN];
const struct vsc73xx_ops *ops;
void *priv;
+ u16 pvid_storage[VSC73XX_MAX_NUM_PORTS];
+ u16 untagged_storage[VSC73XX_MAX_NUM_PORTS];
+ struct list_head vlans;
};
/**
@@ -49,6 +63,21 @@ struct vsc73xx_ops {
u32 val);
};
+/**
+ * struct vsc73xx_bridge_vlan - VSC73xx driver structure which keeps vlan
+ * database copy
+ * @vid: VLAN number
+ * @portmask: each bit represends one port
+ * @untagged: each bit represends one port configured with @vid untagged
+ * @list: list structure
+ */
+struct vsc73xx_bridge_vlan {
+ u16 vid;
+ u8 portmask;
+ u8 untagged;
+ struct list_head list;
+};
+
int vsc73xx_is_addr_valid(u8 block, u8 subblock);
int vsc73xx_probe(struct vsc73xx *vsc);
void vsc73xx_remove(struct vsc73xx *vsc);
--
2.34.1
From: Vladimir Oltean <[email protected]>
In both sja1105_rcv() and sja1110_rcv(), we may have precise source port
information coming from parallel hardware mechanisms, in addition to the
tag_8021q header.
Only sja1105_rcv() has extra logic to not overwrite that precise info
with what's present in the VLAN tag. This is because sja1110_rcv() gets
by, by having a reversed set of checks when assigning skb->dev. When the
source port is imprecise (vbid >=1), source_port and switch_id will be
set to zeroes by dsa_8021q_rcv(), which might be problematic. But by
checking for vbid >= 1 first, sja1110_rcv() fends that off.
We would like to make more code common between sja1105_rcv() and
sja1110_rcv(), and for that, we need to make sure that sja1110_rcv()
also goes through the precise source port preservation logic.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- add missing SoB
v4:
- introduced patch
net/dsa/tag_8021q.c | 32 +++++++++++++++++++++++++++++---
net/dsa/tag_sja1105.c | 23 +++--------------------
2 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 71b26ae6db39..3cb0293793a5 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -497,9 +497,21 @@ struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
}
EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid);
+/**
+ * dsa_8021q_rcv - Decode source information from tag_8021q header
+ * @skb: RX socket buffer
+ * @source_port: pointer to storage for precise source port information.
+ * If this is known already from outside tag_8021q, the pre-initialized
+ * value is preserved. If not known, pass -1.
+ * @switch_id: similar to source_port.
+ * @vbid: pointer to storage for imprecise bridge ID. Must be pre-initialized
+ * with -1. If a positive value is returned, the source_port and switch_id
+ * are invalid.
+ */
void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
int *vbid)
{
+ int tmp_source_port, tmp_switch_id, tmp_vbid;
u16 vid, tci;
if (skb_vlan_tag_present(skb)) {
@@ -513,11 +525,25 @@ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
vid = tci & VLAN_VID_MASK;
- *source_port = dsa_8021q_rx_source_port(vid);
- *switch_id = dsa_8021q_rx_switch_id(vid);
+ tmp_source_port = dsa_8021q_rx_source_port(vid);
+ tmp_switch_id = dsa_8021q_rx_switch_id(vid);
+ tmp_vbid = dsa_tag_8021q_rx_vbid(vid);
+
+ /* Precise source port information is unknown when receiving from a
+ * VLAN-unaware bridging domain, and tmp_source_port and tmp_switch_id
+ * are zeroes in this case.
+ *
+ * Preserve the source information from hardware-specific mechanisms,
+ * if available. This allows us to not overwrite a valid source port
+ * and switch ID with less precise values.
+ */
+ if (tmp_vbid == 0 && *source_port == -1)
+ *source_port = tmp_source_port;
+ if (tmp_vbid == 0 && *switch_id == -1)
+ *switch_id = tmp_switch_id;
if (vbid)
- *vbid = dsa_tag_8021q_rx_vbid(vid);
+ *vbid = tmp_vbid;
skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
}
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 1aba1d05c27a..48886d4b7e3e 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -524,30 +524,13 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
/* Normal data plane traffic and link-local frames are tagged with
* a tag_8021q VLAN which we have to strip
*/
- if (sja1105_skb_has_tag_8021q(skb)) {
- int tmp_source_port = -1, tmp_switch_id = -1;
-
- sja1105_vlan_rcv(skb, &tmp_source_port, &tmp_switch_id, &vbid,
- &vid);
- /* Preserve the source information from the INCL_SRCPT option,
- * if available. This allows us to not overwrite a valid source
- * port and switch ID with zeroes when receiving link-local
- * frames from a VLAN-unaware bridged port (non-zero vbid) or a
- * VLAN-aware bridged port (non-zero vid). Furthermore, the
- * tag_8021q source port information is only of trust when the
- * vbid is 0 (precise port). Otherwise, tmp_source_port and
- * tmp_switch_id will be zeroes.
- */
- if (vbid == 0 && source_port == -1)
- source_port = tmp_source_port;
- if (vbid == 0 && switch_id == -1)
- switch_id = tmp_switch_id;
- } else if (source_port == -1 && switch_id == -1) {
+ if (sja1105_skb_has_tag_8021q(skb))
+ sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
+ else if (source_port == -1 && switch_id == -1)
/* Packets with no source information have no chance of
* getting accepted, drop them straight away.
*/
return NULL;
- }
if (source_port != -1 && switch_id != -1)
skb->dev = dsa_conduit_find_user(netdev, switch_id, source_port);
--
2.34.1
From: Vladimir Oltean <[email protected]>
tag_sja1105 has a wrapper over dsa_8021q_rcv(): sja1105_vlan_rcv(),
which determines whether the packet came from a bridge with
vlan_filtering=1 (the case resolved via
dsa_find_designated_bridge_port_by_vid()), or if it contains a tag_8021q
header.
Looking at a new tagger implementation for vsc73xx, based also on
tag_8021q, it is becoming clear that the logic is needed there as well.
So instead of forcing each tagger to wrap around dsa_8021q_rcv(), let's
merge the logic into the core.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- add missing SoB
v4:
- introduced patch
net/dsa/tag_8021q.c | 34 ++++++++++++++++++++++++++++------
net/dsa/tag_8021q.h | 2 +-
net/dsa/tag_ocelot_8021q.c | 2 +-
net/dsa/tag_sja1105.c | 32 ++++----------------------------
4 files changed, 34 insertions(+), 36 deletions(-)
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 3cb0293793a5..332b0ae02645 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -507,27 +507,39 @@ EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid);
* @vbid: pointer to storage for imprecise bridge ID. Must be pre-initialized
* with -1. If a positive value is returned, the source_port and switch_id
* are invalid.
+ * @vid: pointer to storage for original VID, in case tag_8021q decoding failed.
+ *
+ * If the packet has a tag_8021q header, decode it and set @source_port,
+ * @switch_id and @vbid, and strip the header. Otherwise set @vid and keep the
+ * header in the hwaccel area of the packet.
*/
void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
- int *vbid)
+ int *vbid, int *vid)
{
int tmp_source_port, tmp_switch_id, tmp_vbid;
- u16 vid, tci;
+ __be16 vlan_proto;
+ u16 tmp_vid, tci;
if (skb_vlan_tag_present(skb)) {
+ vlan_proto = skb->vlan_proto;
tci = skb_vlan_tag_get(skb);
__vlan_hwaccel_clear_tag(skb);
} else {
+ struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
+
+ vlan_proto = hdr->h_vlan_proto;
skb_push_rcsum(skb, ETH_HLEN);
__skb_vlan_pop(skb, &tci);
skb_pull_rcsum(skb, ETH_HLEN);
}
- vid = tci & VLAN_VID_MASK;
+ tmp_vid = tci & VLAN_VID_MASK;
+ if (!vid_is_dsa_8021q(tmp_vid))
+ goto not_tag_8021q;
- tmp_source_port = dsa_8021q_rx_source_port(vid);
- tmp_switch_id = dsa_8021q_rx_switch_id(vid);
- tmp_vbid = dsa_tag_8021q_rx_vbid(vid);
+ tmp_source_port = dsa_8021q_rx_source_port(tmp_vid);
+ tmp_switch_id = dsa_8021q_rx_switch_id(tmp_vid);
+ tmp_vbid = dsa_tag_8021q_rx_vbid(tmp_vid);
/* Precise source port information is unknown when receiving from a
* VLAN-unaware bridging domain, and tmp_source_port and tmp_switch_id
@@ -546,5 +558,15 @@ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
*vbid = tmp_vbid;
skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
+ return;
+
+not_tag_8021q:
+ if (vid)
+ *vid = tmp_vid;
+ if (vbid)
+ *vbid = -1;
+
+ /* Put the tag back */
+ __vlan_hwaccel_put_tag(skb, vlan_proto, tci);
}
EXPORT_SYMBOL_GPL(dsa_8021q_rcv);
diff --git a/net/dsa/tag_8021q.h b/net/dsa/tag_8021q.h
index 41f7167ac520..0c6671d7c1c2 100644
--- a/net/dsa/tag_8021q.h
+++ b/net/dsa/tag_8021q.h
@@ -14,7 +14,7 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
u16 tpid, u16 tci);
void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
- int *vbid);
+ int *vbid, int *vid);
struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
int vbid);
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index b059381310fe..8e8b1bef6af6 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -81,7 +81,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
{
int src_port, switch_id;
- dsa_8021q_rcv(skb, &src_port, &switch_id, NULL);
+ dsa_8021q_rcv(skb, &src_port, &switch_id, NULL, NULL);
skb->dev = dsa_conduit_find_user(netdev, switch_id, src_port);
if (!skb->dev)
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 48886d4b7e3e..7639ccb94d35 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -472,37 +472,14 @@ static bool sja1110_skb_has_inband_control_extension(const struct sk_buff *skb)
return ntohs(eth_hdr(skb)->h_proto) == ETH_P_SJA1110;
}
-/* If the VLAN in the packet is a tag_8021q one, set @source_port and
- * @switch_id and strip the header. Otherwise set @vid and keep it in the
- * packet.
- */
-static void sja1105_vlan_rcv(struct sk_buff *skb, int *source_port,
- int *switch_id, int *vbid, u16 *vid)
-{
- struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
- u16 vlan_tci;
-
- if (skb_vlan_tag_present(skb))
- vlan_tci = skb_vlan_tag_get(skb);
- else
- vlan_tci = ntohs(hdr->h_vlan_TCI);
-
- if (vid_is_dsa_8021q(vlan_tci & VLAN_VID_MASK))
- return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
-
- /* Try our best with imprecise RX */
- *vid = vlan_tci & VLAN_VID_MASK;
-}
-
static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
struct net_device *netdev)
{
- int source_port = -1, switch_id = -1, vbid = -1;
+ int source_port = -1, switch_id = -1, vbid = -1, vid = -1;
struct sja1105_meta meta = {0};
struct ethhdr *hdr;
bool is_link_local;
bool is_meta;
- u16 vid;
hdr = eth_hdr(skb);
is_link_local = sja1105_is_link_local(skb);
@@ -525,7 +502,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
* a tag_8021q VLAN which we have to strip
*/
if (sja1105_skb_has_tag_8021q(skb))
- sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
+ dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid);
else if (source_port == -1 && switch_id == -1)
/* Packets with no source information have no chance of
* getting accepted, drop them straight away.
@@ -660,9 +637,8 @@ static struct sk_buff *sja1110_rcv_inband_control_extension(struct sk_buff *skb,
static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
struct net_device *netdev)
{
- int source_port = -1, switch_id = -1, vbid = -1;
+ int source_port = -1, switch_id = -1, vbid = -1, vid = -1;
bool host_only = false;
- u16 vid = 0;
if (sja1110_skb_has_inband_control_extension(skb)) {
skb = sja1110_rcv_inband_control_extension(skb, &source_port,
@@ -674,7 +650,7 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
/* Packets with in-band control extensions might still have RX VLANs */
if (likely(sja1105_skb_has_tag_8021q(skb)))
- sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid);
+ dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid);
if (vbid >= 1)
skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
--
2.34.1
This commit adds updates to the documentation describing the structures
used in vsc73xx. This will help prevent kdoc-related issues in the future.
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- improved description and stick to 80 characters per line
v4:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx.h | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index fee1378508b5..e7b08599a625 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -15,7 +15,16 @@
#define VSC73XX_MAX_NUM_PORTS 8
/**
- * struct vsc73xx - VSC73xx state container
+ * struct vsc73xx - VSC73xx state container: main data structure
+ * @dev: The device pointer
+ * @reset: The descriptor for the GPIO line tied to the reset pin
+ * @ds: Pointer to the DSA core structure
+ * @gc: Main structure of the GPIO controller
+ * @chipid: Storage for the Chip ID value read from the CHIPID register of the
+ * sswitch
+ * @addr: MAC address used in flow control frames
+ * @ops: Structure with hardware-dependent operations
+ * @priv: Pointer to the configuration interface structure
*/
struct vsc73xx {
struct device *dev;
@@ -28,6 +37,11 @@ struct vsc73xx {
void *priv;
};
+/**
+ * struct vsc73xx_ops - VSC73xx methods container
+ * @read: Method for register reading over the hardware-dependent interface
+ * @write: Method for register writing over the hardware-dependent interface
+ */
struct vsc73xx_ops {
int (*read)(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
u32 *val);
--
2.34.1
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 <[email protected]>
---
v6:
- added missing MODULE_DESCRIPTION()
v5:
- removed skb_vlan_tag_present(skb) checking
- use 80 characters per line limit
v4:
- rebase to net-next/main
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 | 67 +++++++++++++++++++++++++++++++++++++
4 files changed, 76 insertions(+)
create mode 100644 net/dsa/tag_vsc73xx_8021q.c
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7c0da9effe4e..b79e136e4c41 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 8a1894a42552..555c07cfeb71 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..f7bc0261d54d
--- /dev/null
+++ b/net/dsa/tag_vsc73xx_8021q.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (C) 2024 Pawel Dembicki <[email protected]>
+ */
+#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_user_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 struct sk_buff *
+vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev)
+{
+ int src_port = -1, switch_id = -1, vbid = -1, vid = -1;
+
+ dsa_8021q_rcv(skb, &src_port, &switch_id, &vbid, &vid);
+
+ skb->dev = dsa_tag_8021q_find_user(netdev, src_port, switch_id,
+ vid, vbid);
+ if (!skb->dev) {
+ netdev_warn(netdev, "Couldn't decode source port\n");
+ return NULL;
+ }
+
+ dsa_default_offload_fwd_mark(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_conduit = true,
+};
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DSA tag driver for VSC73XX family of switches, using VLAN");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME);
+
+module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
--
2.34.1
From: Vladimir Oltean <[email protected]>
A new tagging protocol implementation based on tag_8021q is on the
horizon, and it appears that it also has to open-code the complicated
logic of finding a source port based on a VLAN header.
Create a single dsa_tag_8021q_find_user() and make sja1105 call it.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6, v5:
- resend only
v4:
- introduce patch and change from master to conduit and slave to user
net/dsa/tag_8021q.c | 19 ++++++++++++++++---
net/dsa/tag_8021q.h | 5 +++--
net/dsa/tag_sja1105.c | 17 +++++------------
3 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 332b0ae02645..454d36c84671 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -468,8 +468,8 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
}
EXPORT_SYMBOL_GPL(dsa_8021q_xmit);
-struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
- int vbid)
+static struct net_device *
+dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit, int vbid)
{
struct dsa_port *cpu_dp = conduit->dsa_ptr;
struct dsa_switch_tree *dst = cpu_dp->dst;
@@ -495,7 +495,20 @@ struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
return NULL;
}
-EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid);
+
+struct net_device *dsa_tag_8021q_find_user(struct net_device *conduit,
+ int source_port, int switch_id,
+ int vid, int vbid)
+{
+ /* Always prefer precise source port information, if available */
+ if (source_port != -1 && switch_id != -1)
+ return dsa_conduit_find_user(conduit, switch_id, source_port);
+ else if (vbid >= 1)
+ return dsa_tag_8021q_find_port_by_vbid(conduit, vbid);
+
+ return dsa_find_designated_bridge_port_by_vid(conduit, vid);
+}
+EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_user);
/**
* dsa_8021q_rcv - Decode source information from tag_8021q header
diff --git a/net/dsa/tag_8021q.h b/net/dsa/tag_8021q.h
index 0c6671d7c1c2..27b8906f99ec 100644
--- a/net/dsa/tag_8021q.h
+++ b/net/dsa/tag_8021q.h
@@ -16,8 +16,9 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
int *vbid, int *vid);
-struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
- int vbid);
+struct net_device *dsa_tag_8021q_find_user(struct net_device *conduit,
+ int source_port, int switch_id,
+ int vid, int vbid);
int dsa_switch_tag_8021q_vlan_add(struct dsa_switch *ds,
struct dsa_notifier_tag_8021q_vlan_info *info);
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 35a6346549f2..3e902af7eea6 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -509,12 +509,8 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
*/
return NULL;
- if (source_port != -1 && switch_id != -1)
- skb->dev = dsa_conduit_find_user(netdev, switch_id, source_port);
- else if (vbid >= 1)
- skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
- else
- skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
+ skb->dev = dsa_tag_8021q_find_user(netdev, source_port, switch_id,
+ vid, vbid);
if (!skb->dev) {
netdev_warn(netdev, "Couldn't decode source port\n");
return NULL;
@@ -652,12 +648,9 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
if (likely(sja1105_skb_has_tag_8021q(skb)))
dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid);
- if (source_port != -1 && switch_id != -1)
- skb->dev = dsa_conduit_find_user(netdev, switch_id, source_port);
- else if (vbid >= 1)
- skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
- else
- skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
+ skb->dev = dsa_tag_8021q_find_user(netdev, source_port, switch_id,
+ vid, vbid);
+
if (!skb->dev) {
netdev_warn(netdev, "Couldn't decode source port\n");
return NULL;
--
2.34.1
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.
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- resend only
v5:
- improve commit message
v4:
- adjust tag8021q implementation for changed untagged vlan storage
- minor fixes
v3:
- Split tagger and tag implementation into separate commits
drivers/net/dsa/Kconfig | 2 +-
drivers/net/dsa/vitesse-vsc73xx-core.c | 39 ++++++++++++++++++++++++--
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 3092b391031a..22a04636d09e 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 c643f445f026..f8175a33c0a6 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -592,7 +592,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_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
@@ -683,7 +683,7 @@ vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
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");
@@ -752,6 +752,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);
@@ -1554,6 +1560,33 @@ 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;
+ bool operate_on_storage;
+
+ operate_on_storage = !vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
+
+ if (untagged)
+ vsc73xx_vlan_change_untagged(vsc, port, vid, true,
+ operate_on_storage);
+ if (pvid)
+ vsc73xx_vlan_change_pvid(vsc, port, vid, true,
+ operate_on_storage);
+
+ return vsc73xx_update_vlan_table(vsc, port, vid, true);
+}
+
+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, false);
+}
+
static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
{
struct vsc73xx *vsc = ds->priv;
@@ -1695,6 +1728,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_vlan_add = vsc73xx_port_vlan_add,
.port_vlan_del = vsc73xx_port_vlan_del,
.phylink_get_caps = vsc73xx_phylink_get_caps,
+ .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
This patch implements .port_pre_bridge_flags() and .port_bridge_flags(),
which are required for properly treating the BR_LEARNING flag. Also,
port_stp_state_set() is tweaked and now disables learning for standalone
ports.
Disabling learning for standalone ports is required to avoid situations
where one port sees traffic originating from another, which could cause
invalid operations.
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6:
- fix arranging local variables in reverse xmas tree order
v5:
- introduce patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 41 ++++++++++++++++++++++----
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index e7dea48672d7..05dbeec8eb63 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1590,6 +1590,31 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
return vsc73xx_update_vlan_table(vsc, port, vid, false);
}
+static int vsc73xx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ if (flags.mask & ~BR_LEARNING)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int vsc73xx_port_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ if (flags.mask & BR_LEARNING) {
+ u32 val = flags.val & BR_LEARNING ? BIT(port) : 0;
+ struct vsc73xx *vsc = ds->priv;
+
+ return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_LEARNMASK, BIT(port), val);
+ }
+
+ return 0;
+}
+
static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
struct dsa_bridge bridge,
bool *tx_fwd_offload,
@@ -1706,19 +1731,21 @@ static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
u8 state)
{
+ struct dsa_port *dp = dsa_to_port(ds, port);
struct vsc73xx *vsc = ds->priv;
- u32 val;
+ u32 val = 0;
+
+ if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
+ val = dp->learning ? BIT(port) : 0;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_LEARNMASK, BIT(port), val);
val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ?
0 : BIT(port);
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_RECVMASK, BIT(port), val);
- val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ?
- BIT(port) : 0;
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
- VSC73XX_LEARNMASK, BIT(port), val);
-
/* CPU Port should always forward packets when user ports are forwarding
* so let's configure it from other ports only.
*/
@@ -1740,6 +1767,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_setup = vsc73xx_port_setup,
.port_enable = vsc73xx_port_enable,
.port_disable = vsc73xx_port_disable,
+ .port_pre_bridge_flags = vsc73xx_port_pre_bridge_flags,
+ .port_bridge_flags = vsc73xx_port_bridge_flags,
.port_bridge_join = vsc73xx_port_bridge_join,
.port_bridge_leave = vsc73xx_port_bridge_leave,
.port_change_mtu = vsc73xx_change_mtu,
--
2.34.1
From: Vladimir Oltean <[email protected]>
Now that dsa_8021q_rcv() handles better the case where we don't
overwrite the precise source information if it comes from an external
(non-tag_8021q) source, we can now unify the call sequence between
sja1105_rcv() and sja1110_rcv().
This is a preparatory change for creating a higher-level wrapper for the
entire sequence which will live in tag_8021q.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v6,v5:
- resend only
v4:
- introduce patch and replace 'slave' with 'conduit' after rebase
net/dsa/tag_sja1105.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 7639ccb94d35..35a6346549f2 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -652,12 +652,12 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb,
if (likely(sja1105_skb_has_tag_8021q(skb)))
dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid);
- if (vbid >= 1)
+ if (source_port != -1 && switch_id != -1)
+ skb->dev = dsa_conduit_find_user(netdev, switch_id, source_port);
+ else if (vbid >= 1)
skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
- else if (source_port == -1 || switch_id == -1)
- skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
else
- skb->dev = dsa_conduit_find_user(netdev, switch_id, source_port);
+ skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
if (!skb->dev) {
netdev_warn(netdev, "Couldn't decode source port\n");
return NULL;
--
2.34.1
Max number of bridges in tag8021q implementation is strictly limited
by VBID size: 3 bits. But zero is reserved and only 7 values can be used.
This patch adds define which describe maximum possible value.
Suggested-by: Vladimir Oltean <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
v6:
- resend only
v5:
- added 'Reviewed-by' only
v4:
- introduce patch
drivers/net/dsa/sja1105/sja1105_main.c | 3 +--
include/linux/dsa/8021q.h | 5 +++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 6646f7fb0f90..6e22d7a6bfa3 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3156,8 +3156,7 @@ static int sja1105_setup(struct dsa_switch *ds)
ds->vlan_filtering_is_global = true;
ds->untag_bridge_pvid = true;
ds->fdb_isolation = true;
- /* tag_8021q has 3 bits for the VBID, and the value 0 is reserved */
- ds->max_num_bridges = 7;
+ ds->max_num_bridges = DSA_TAG_8021Q_MAX_NUM_BRIDGES;
/* Advertise the 8 egress queues */
ds->num_tx_queues = SJA1105_NUM_TC;
diff --git a/include/linux/dsa/8021q.h b/include/linux/dsa/8021q.h
index f3664ee12170..1dda2a13b832 100644
--- a/include/linux/dsa/8021q.h
+++ b/include/linux/dsa/8021q.h
@@ -8,6 +8,11 @@
#include <net/dsa.h>
#include <linux/types.h>
+/* VBID is limited to three bits only and zero is reserved.
+ * Only 7 bridges can be enumerated.
+ */
+#define DSA_TAG_8021Q_MAX_NUM_BRIDGES 7
+
int dsa_tag_8021q_register(struct dsa_switch *ds, __be16 proto);
void dsa_tag_8021q_unregister(struct dsa_switch *ds);
--
2.34.1
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.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
v6:
- resend only
v5:
- added 'Reviewed-by' only
v4:
- remove forward configuration after stp patch refactoring
- implement new define with max num of bridges for tag8021q devices
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 | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index f8175a33c0a6..e7dea48672d7 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -687,6 +687,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 = DSA_TAG_8021Q_MAX_NUM_BRIDGES;
+
/* Issue RESET */
vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
VSC73XX_GLORESET_MASTER_RESET);
@@ -1587,6 +1590,22 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
return vsc73xx_update_vlan_table(vsc, port, vid, false);
}
+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)
+{
+ *tx_fwd_offload = true;
+
+ 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)
+{
+ dsa_tag_8021q_bridge_leave(ds, port, bridge);
+}
+
static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
{
struct vsc73xx *vsc = ds->priv;
@@ -1721,6 +1740,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_setup = vsc73xx_port_setup,
.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
On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <paweldembicki@gmailcom> wrote:
> 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 <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <paweldembicki@gmailcom> 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.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
This looks like the best effort to me.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 3/1/24 14:16, Pawel Dembicki wrote:
> 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_mac_config
> .phylink_mac_link_up
> .phylink_mac_link_down
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/1/24 14:16, 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.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <paweldembicki@gmailcom> wrote:
> This patch series focuses on making vsc73xx usable.
Can't help to think it is a bit funny regarding how many units
of this chips are shipped in e.g. home routers.
They all work pretty much like the in-kernel driver, or a bit
less than that, just hammered down spraying packets in all
directions.
Yours,
Linus Walleij
On 3/5/24 14:45, Linus Walleij wrote:
> On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <[email protected]> wrote:
>
>> This patch series focuses on making vsc73xx usable.
>
> Can't help to think it is a bit funny regarding how many units
> of this chips are shipped in e.g. home routers.
How many use this particular DSA driver as opposed to an user-space SDK
driver though?
>
> They all work pretty much like the in-kernel driver, or a bit
> less than that, just hammered down spraying packets in all
> directions.
Do you have a list of devices, so I make sure I don't buy those :P?
--
Florian
On 3/1/24 14:16, Pawel Dembicki wrote:
> 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 <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/1/24 14:16, 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 <[email protected]>
> ---
[snip]
> +static size_t
> +vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> + size_t num_untagged = 0;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if ((vlan->portmask & BIT(port)) &&
> + (vlan->untagged & BIT(port)) &&
> + vlan->vid != ignored_vid)
> + num_untagged++;
> +
> + return num_untagged;
> +}
You always use both helpers at the same time, so I would suggest
returning num_tagged and num_untagged by reference to have a single
linked list lookup.
> +
> +static u16 vsc73xx_find_first_vlan_untagged(struct vsc73xx *vsc, int port)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if ((vlan->portmask & BIT(port)) &&
> + (vlan->untagged & BIT(port)))
> + return vlan->vid;
> +
> + return VLAN_N_VID;
> +}
> +
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> + enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE;
> + struct vsc73xx *vsc = ds->priv;
> + bool store_untagged = false;
> + bool store_pvid = false;
> + u16 vid, vlan_untagged;
> +
> + /* The swap processed below is required because vsc73xx is using
> + * tag_8021q. When vlan_filtering is disabled, tag_8021q uses
> + * pvid/untagged vlans for port recognition. The values configured for
> + * vlans < 3072 are stored in storage table. When vlan_filtering is
> + * enabled, we need to restore pvid/untagged from storage and keep
> + * values used for tag_8021q.
> + */
> + if (vlan_filtering) {
> + /* Use VLAN_N_VID to count all vlans */
> + size_t num_untagged =
> + vsc73xx_bridge_vlan_num_untagged(vsc, port, VLAN_N_VID);
> +
> + port_vlan_conf = (num_untagged > 1) ?
> + VSC73XX_VLAN_FILTER_UNTAG_ALL :
> + VSC73XX_VLAN_FILTER;
> +
> + vlan_untagged = vsc73xx_find_first_vlan_untagged(vsc, port);
> + if (vlan_untagged < VLAN_N_VID) {
> + store_untagged = vsc73xx_port_get_untagged(vsc, port,
> + &vid,
> + false);
> + vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> + true, false);
> + vsc->untagged_storage[port] = store_untagged ?
> + vid : VLAN_N_VID;
> + }
> + } else {
> + vsc73xx_vlan_change_untagged(vsc, port,
> + vsc->untagged_storage[port],
> + vsc->untagged_storage[port] <
> + VLAN_N_VID, false);
> + }
> +
> + vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +
> + store_pvid = vsc73xx_port_get_pvid(vsc, port, &vid, false);
> + vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port],
> + vsc->pvid_storage[port] < VLAN_N_VID, false);
> + vsc->pvid_storage[port] = store_pvid ? vid : 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 dsa_port *dp = dsa_to_port(ds, port);
> + struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> + size_t num_tagged, num_untagged;
> + struct vsc73xx *vsc = ds->priv;
> + int ret;
> + u16 vid;
> +
> + /* 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;
> + }
> +
> + /* The processed vlan->vid is excluded from the search because the VLAN
> + * can be re-added with a different set of flags, so it's easiest to
> + * ignore its old flags from the VLAN database software copy.
> + */
> + num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> + num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
> +
> + /* VSC73XX allow only three untagged states: none, one or all */
> + if ((untagged && num_tagged > 0 && num_untagged > 0) ||
> + (!untagged && num_untagged > 1)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Port can have only none, one or all untagged vlan");
> + return -EBUSY;
> + }
> +
> + vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> + if (!vsc73xx_vlan) {
> + vsc73xx_vlan = kzalloc(sizeof(*vsc73xx_vlan), GFP_KERNEL);
> + if (!vsc73xx_vlan)
> + return -ENOMEM;
> +
> + vsc73xx_vlan->vid = vlan->vid;
> + vsc73xx_vlan->portmask = BIT(port);
> + vsc73xx_vlan->untagged = untagged ? BIT(port) : 0;
> +
> + INIT_LIST_HEAD(&vsc73xx_vlan->list);
> + list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
> + } else {
> + vsc73xx_vlan->portmask |= BIT(port);
> +
> + if (untagged)
> + vsc73xx_vlan->untagged |= BIT(port);
> + else
> + vsc73xx_vlan->untagged &= ~BIT(port);
These assignments should be working even when you have a freshly
allocated VLAN entry, so you can just re-factor this a bit and have a
common set of assignments applying to an existing or freshly allocated
VLAN entry?
> + }
> +
> + /* CPU port must be always tagged because port separation is based on
> + * tag_8021q.
> + */
> + if (port != CPU_PORT) {
Please reduce indentation here.
Have to admit the logic is a bit hard to follow, but that is also
because of my lack of understanding of the requirements surrounding the
use of tag_8021q.
--
Florian
On Fri, Mar 1, 2024 at 11:18 PM Pawel Dembicki <paweldembicki@gmailcom> wrote:
> This patch implements .port_pre_bridge_flags() and .port_bridge_flags(),
> which are required for properly treating the BR_LEARNING flag. Also,
> .port_stp_state_set() is tweaked and now disables learning for standalone
> ports.
>
> Disabling learning for standalone ports is required to avoid situations
> where one port sees traffic originating from another, which could cause
> invalid operations.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
This looks right to me:
Reviewed-by: Linus Walleij <[email protected]>
I looked at all patches that I understand, the VLAN tag stuff needs to
be reviewed by Vladimir who I think has a clear idea of how that should
be done. You can add my Acked-by because I looked at them and they
look right but I can't claim to have made a thorough review on those.
Yours,
Linus Walleij
On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <paweldembicki@gmailcom> wrote:
> 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_mac_config
> .phylink_mac_link_up
> .phylink_mac_link_down
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Fri, Mar 01, 2024 at 11:16:24PM +0100, Pawel Dembicki wrote:
> 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_mac_config
> .phylink_mac_link_up
> .phylink_mac_link_down
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Russell King (Oracle) <[email protected]>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Mar 01, 2024 at 11:16:25PM +0100, Pawel Dembicki wrote:
> It's preparation for future use. At this moment, the RGMII port is used
> only for a connection to the MAC interface, but in the future, someone
> could connect a PHY to it. Using the "phy_interface_mode_is_rgmii" macro
> allows for the proper recognition of all RGMII modes.
>
> Suggested-by: Russell King <[email protected]>
Please change this to:
Suggested-by: Russell King (Oracle) <[email protected]>
Also:
Reviewed-by: Russell King (Oracle) <[email protected]>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Mar 1, 2024 at 11:18 PM Pawel Dembicki <paweldembicki@gmailcom> wrote:
> Max number of bridges in tag8021q implementation is strictly limited
> by VBID size: 3 bits. But zero is reserved and only 7 values can be used.
>
> This patch adds define which describe maximum possible value.
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Mar 6, 2024 at 12:28 AM Florian Fainelli <[email protected]> wrote:
> On 3/5/24 14:45, Linus Walleij wrote:
> > On Fri, Mar 1, 2024 at 11:17 PM Pawel Dembicki <[email protected]> wrote:
> >
> >> This patch series focuses on making vsc73xx usable.
> >
> > Can't help to think it is a bit funny regarding how many units
> > of this chips are shipped in e.g. home routers.
>
> How many use this particular DSA driver as opposed to an user-space SDK
> driver though?
They don't have a userspace driver either, they all have a kernelspace
hack on kernel 2.6.15 or so :/
https://github.com/linusw/linux-SL3516/blob/SL93512R_v2.6.6/drivers/net/sl_switch.c
> Do you have a list of devices, so I make sure I don't buy those :P?
- StorLink reference design SL93512R (and the rest of them, and later resold
from Cortina Systems). I got this from Imre Kaloz who got it from StorLink.
- ITian Square One
https://dflund.se/~triad/krad/itian-squareone/
https://openwrt.org/toh/itian/square_one_sq201
(The place where I started this switch support.)
using the same hack as above.
HEY! You sent that to me :D
I think a bunch of other random router vendors copied this from the StorLink
reference design as well, I just don't have all the Gemini devices.
Yours,
Linus Walleij
On 3/1/24 14:16, Pawel Dembicki wrote:
> From: Vladimir Oltean <[email protected]>
>
> A new tagging protocol implementation based on tag_8021q is on the
> horizon, and it appears that it also has to open-code the complicated
> logic of finding a source port based on a VLAN header.
>
> Create a single dsa_tag_8021q_find_user() and make sja1105 call it.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/1/24 14:16, 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 <[email protected]>
> ---
> v6:
> - added missing MODULE_DESCRIPTION()
> v5:
> - removed skb_vlan_tag_present(skb) checking
> - use 80 characters per line limit
> v4:
> - rebase to net-next/main
> 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 | 67 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 76 insertions(+)
> create mode 100644 net/dsa/tag_vsc73xx_8021q.c
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7c0da9effe4e..b79e136e4c41 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 8a1894a42552..555c07cfeb71 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..f7bc0261d54d
> --- /dev/null
> +++ b/net/dsa/tag_vsc73xx_8021q.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (C) 2024 Pawel Dembicki <[email protected]>
> + */
> +#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_user_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 struct sk_buff *
> +vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev)
> +{
> + int src_port = -1, switch_id = -1, vbid = -1, vid = -1;
> +
> + dsa_8021q_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> +
> + skb->dev = dsa_tag_8021q_find_user(netdev, src_port, switch_id,
> + vid, vbid);
> + if (!skb->dev) {
> + netdev_warn(netdev, "Couldn't decode source port\n");
Please remove this message, or rate limit it, with that fixed:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/1/24 14:16, 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.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/1/24 14:16, Pawel Dembicki wrote:
> This patch implements .port_pre_bridge_flags() and .port_bridge_flags(),
> which are required for properly treating the BR_LEARNING flag. Also,
> .port_stp_state_set() is tweaked and now disables learning for standalone
> ports.
>
> Disabling learning for standalone ports is required to avoid situations
> where one port sees traffic originating from another, which could cause
> invalid operations.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Fri, Mar 01, 2024 at 11:16:34PM +0100, 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 <[email protected]>
> ---
With Florian's feedback addressed:
Reviewed-by: Vladimir Oltean <[email protected]>
On Fri, Mar 01, 2024 at 11:16:38PM +0100, Pawel Dembicki wrote:
> This patch implements .port_pre_bridge_flags() and .port_bridge_flags(),
> which are required for properly treating the BR_LEARNING flag. Also,
> .port_stp_state_set() is tweaked and now disables learning for standalone
> ports.
>
> Disabling learning for standalone ports is required to avoid situations
> where one port sees traffic originating from another, which could cause
> invalid operations.
"invalid operations" -> packet drops
>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Fri, Mar 01, 2024 at 11:16:28PM +0100, 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.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
> v6:
> - fix inconsistent indenting
> v5:
> - remove unneeded 'RECVMASK' operations
> - reorganise vsc73xx_refresh_fwd_map function
> v4:
> - fully reworked port_stp_state_set
> 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 | 99 +++++++++++++++++++++++---
> 1 file changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 425999d7bf41..d1e84a9a83d1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1036,6 +1029,89 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
> }
>
> +static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
> +{
> + struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
> + struct vsc73xx *vsc = ds->priv;
> + u16 mask;
> +
> + if (state != BR_STATE_FORWARDING) {
> + /* Ports that aren't in the forwarding state must not
> + * forward packets anywhere.
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +
> + dsa_switch_for_each_available_port(other_dp, ds) {
> + if (other_dp == dp)
> + continue;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + other_dp->index,
> + BIT(port), 0);
> + }
> +
> + return;
> + }
> +
> + /* Forwarding ports must forward to the CPU and to other ports
> + * in the same bridge
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
> +
> + mask = BIT(CPU_PORT);
> +
> + if (dp->bridge) {
> + dsa_switch_for_each_user_port(other_dp, ds) {
> + if (other_dp->bridge == dp->bridge &&
You could use dsa_port_bridge_same(dp, other_dp) and that could
eliminate the extra "if (dp->bridge)" condition, because it explicitly
makes standalone ports isolated from other standalone ports.
> + other_dp->index != port &&
You could move the "int other_port" definition to dsa_switch_for_each_user_port()
scope, and thus reuse it here.
> + other_dp->stp_state == BR_STATE_FORWARDING) {
You could "continue" on the negated condition, and reduce the
indentation one level further.
> + int other_port = other_dp->index;
> +
> + mask |= BIT(other_port);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER,
> + 0,
> + VSC73XX_SRCMASKS +
> + other_port,
> + BIT(port), BIT(port));
> + }
> + }
> + }
All in all, I would have written this as:
dsa_switch_for_each_user_port(other_dp, ds) {
int other_port = other_dp->index;
if (port == other_port || !dsa_port_bridge_same(dp, other_dp) ||
other_dp->stp_state != BR_STATE_FORWARDING)
continue;
mask |= BIT(other_port);
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_SRCMASKS + other_port,
BIT(port), BIT(port));
}
Anyway this does not affect functionality, and it is up to you if you
integrate these suggestions or not.
Reviewed-by: Vladimir Oltean <[email protected]>
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, mask);
> +}
On Fri, Mar 01, 2024 at 11:16:29PM +0100, Pawel Dembicki wrote:
> +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)
> + );
Close the parenthesis on the previous line.
> +
> + 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);
> + else
> + portmap &= ~BIT(port);
> +
> + return vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
> +}
> +
> static int vsc73xx_setup(struct dsa_switch *ds)
> {
> struct vsc73xx *vsc = ds->priv;
> @@ -598,7 +714,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
> VSC73XX_MACACCESS,
> VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
>
> - /* Clear VLAN table */
> + /* Set VLAN table to default values */
> vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> VSC73XX_VLANACCESS,
> VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
> @@ -627,6 +743,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);
It would have been nice for this to be expressed more symbolically than
the magic number 0x5f. Also, is it correct for vsc7398 (8 ports)?
> /* IP multicast flood mask (table 144) */
> vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> 0xff);
> @@ -639,6 +758,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>
> udelay(4);
>
> + /* Clear VLAN table */
> + for (i = 0; i < VLAN_N_VID; i++)
> + vsc73xx_write_vlan_table_entry(vsc, i, 0);
> +
> + INIT_LIST_HEAD(&vsc->vlans);
> +
> return 0;
> }
>
> @@ -1029,6 +1154,443 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
> }
>
> +static void
> +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> + enum vsc73xx_port_vlan_conf port_vlan_conf)
> +{
> + u32 val = 0;
> +
> + if (port_vlan_conf == VSC73XX_VLAN_IGNORE)
> + val = VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA |
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA;
> +
> + 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_KEEP_TAG_ENA, val);
> +
> + val = (port_vlan_conf == VSC73XX_VLAN_FILTER) ?
> + VSC73XX_TXUPDCFG_TX_INSERT_TAG : 0;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_INSERT_TAG, val);
> +}
> +
> +static int
> +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set,
> + bool operate_on_storage)
> +{
> + u32 val = 0;
> +
> + if (operate_on_storage) {
> + vsc->untagged_storage[port] = set ? vid : VLAN_N_VID;
> + return 0;
> + }
> +
> + if (set)
> + val = VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
> + ((vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> +
> + return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, val);
> +}
> +
> +static int vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid,
> + bool set, bool operate_on_storage)
> +{
> + int ret;
> + u32 val;
> +
> + if (operate_on_storage) {
> + vsc->pvid_storage[port] = set ? vid : VLAN_N_VID;
> + return 0;
> + }
> +
> + val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> + vid = set ? vid : 0;
Why overwrite "vid" to 0 if deleting the PVID? Does it even matter, since
you're setting VSC73XX_CAT_DROP_UNTAGGED_ENA? Or is it just due to the
weird calling convention, where "vid" does not even matter when set==false,
but some callers pass weird values like VLAN_N_VID, and you want to
avoid programming the HW with that? If so, see the more detailed
suggestion below.
> +
> + ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> + if (ret)
> + return ret;
> +
> + return 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);
> +}
> +
> +static bool vsc73xx_port_get_pvid(struct vsc73xx *vsc, int port, u16 *vid,
> + bool operate_on_storage)
> +{
> + u32 val;
> +
> + if (operate_on_storage) {
> + if (vsc->pvid_storage[port] < VLAN_N_VID) {
> + *vid = vsc->pvid_storage[port];
> + return true;
> + }
> + return false;
> + }
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
> + if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
> + return false;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> + *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> +
> + return true;
> +}
> +
> +static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
> +{
> + return !dsa_port_is_vlan_filtering(dp);
> +}
This and vsc73xx_port_get_untagged() are a bit misplaced in terms of
ordering. I guess you'd want the functions which handle the untagged
VLAN to be grouped together, then the functions which handle the PVID,
then the rest.
> +
> +static bool vsc73xx_port_get_untagged(struct vsc73xx *vsc, int port, u16 *vid,
> + bool operate_on_storage)
> +{
> + u32 val;
> +
> + if (operate_on_storage) {
> + if (vsc->untagged_storage[port] < VLAN_N_VID) {
> + *vid = vsc->untagged_storage[port];
> + return true;
> + }
> + return false;
> + }
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> + if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
> + return false;
> +
> + *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> +
> + return true;
> +}
> +
> +static struct vsc73xx_bridge_vlan *
> +vsc73xx_bridge_vlan_find(struct vsc73xx *vsc, u16 vid)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if (vlan->vid == vid)
> + return vlan;
> +
> + return NULL;
> +}
> +
> +static size_t
> +vsc73xx_bridge_vlan_num_tagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> + size_t num_tagged = 0;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if ((vlan->portmask & BIT(port)) &&
> + !(vlan->untagged & BIT(port)) &&
> + vlan->vid != ignored_vid)
> + num_tagged++;
> +
> + return num_tagged;
> +}
> +
> +static size_t
> +vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> + size_t num_untagged = 0;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if ((vlan->portmask & BIT(port)) &&
> + (vlan->untagged & BIT(port)) &&
> + vlan->vid != ignored_vid)
> + num_untagged++;
> +
> + return num_untagged;
> +}
> +
> +static u16 vsc73xx_find_first_vlan_untagged(struct vsc73xx *vsc, int port)
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if ((vlan->portmask & BIT(port)) &&
> + (vlan->untagged & BIT(port)))
> + return vlan->vid;
> +
> + return VLAN_N_VID;
> +}
> +
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> + enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE;
> + struct vsc73xx *vsc = ds->priv;
> + bool store_untagged = false;
> + bool store_pvid = false;
> + u16 vid, vlan_untagged;
> +
> + /* The swap processed below is required because vsc73xx is using
> + * tag_8021q. When vlan_filtering is disabled, tag_8021q uses
> + * pvid/untagged vlans for port recognition. The values configured for
> + * vlans < 3072 are stored in storage table. When vlan_filtering is
> + * enabled, we need to restore pvid/untagged from storage and keep
> + * values used for tag_8021q.
> + */
> + if (vlan_filtering) {
> + /* Use VLAN_N_VID to count all vlans */
> + size_t num_untagged =
> + vsc73xx_bridge_vlan_num_untagged(vsc, port, VLAN_N_VID);
> +
> + port_vlan_conf = (num_untagged > 1) ?
> + VSC73XX_VLAN_FILTER_UNTAG_ALL :
> + VSC73XX_VLAN_FILTER;
> +
> + vlan_untagged = vsc73xx_find_first_vlan_untagged(vsc, port);
> + if (vlan_untagged < VLAN_N_VID) {
> + store_untagged = vsc73xx_port_get_untagged(vsc, port,
A single space before =.
> + &vid,
> + false);
> + vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> + true, false);
> + vsc->untagged_storage[port] = store_untagged ?
> + vid : VLAN_N_VID;
> + }
> + } else {
> + vsc73xx_vlan_change_untagged(vsc, port,
> + vsc->untagged_storage[port],
> + vsc->untagged_storage[port] <
> + VLAN_N_VID, false);
> + }
> +
> + vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +
> + store_pvid = vsc73xx_port_get_pvid(vsc, port, &vid, false);
> + vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port],
> + vsc->pvid_storage[port] < VLAN_N_VID, false);
> + vsc->pvid_storage[port] = store_pvid ? vid : 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 dsa_port *dp = dsa_to_port(ds, port);
> + struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> + size_t num_tagged, num_untagged;
> + struct vsc73xx *vsc = ds->priv;
> + int ret;
> + u16 vid;
> +
> + /* 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;
> + }
> +
> + /* The processed vlan->vid is excluded from the search because the VLAN
> + * can be re-added with a different set of flags, so it's easiest to
> + * ignore its old flags from the VLAN database software copy.
> + */
> + num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> + num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
> +
> + /* VSC73XX allow only three untagged states: none, one or all */
> + if ((untagged && num_tagged > 0 && num_untagged > 0) ||
> + (!untagged && num_untagged > 1)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Port can have only none, one or all untagged vlan");
> + return -EBUSY;
> + }
> +
> + vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> + if (!vsc73xx_vlan) {
> + vsc73xx_vlan = kzalloc(sizeof(*vsc73xx_vlan), GFP_KERNEL);
> + if (!vsc73xx_vlan)
> + return -ENOMEM;
> +
> + vsc73xx_vlan->vid = vlan->vid;
> + vsc73xx_vlan->portmask = BIT(port);
> + vsc73xx_vlan->untagged = untagged ? BIT(port) : 0;
> +
> + INIT_LIST_HEAD(&vsc73xx_vlan->list);
> + list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
> + } else {
> + vsc73xx_vlan->portmask |= BIT(port);
> +
> + if (untagged)
> + vsc73xx_vlan->untagged |= BIT(port);
> + else
> + vsc73xx_vlan->untagged &= ~BIT(port);
> + }
> +
> + /* CPU port must be always tagged because port separation is based on
> + * tag_8021q.
> + */
> + if (port != CPU_PORT) {
> + bool operate_on_storage = vsc73xx_tag_8021q_active(dp);
> +
> + if (!operate_on_storage) {
> + enum vsc73xx_port_vlan_conf port_vlan_conf =
> + VSC73XX_VLAN_FILTER;
> +
> + if (num_tagged == 0 && untagged)
> + port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
> + vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +
> + if (port_vlan_conf == VSC73XX_VLAN_FILTER) {
> + if (untagged) {
> + ret = vsc73xx_vlan_change_untagged(vsc,
> + port,
> + vlan->vid,
> + true,
> + false);
> + if (ret)
> + goto err;
> + } else if (num_untagged == 1) {
> + vid = vsc73xx_find_first_vlan_untagged(vsc,
> + port);
> + ret = vsc73xx_vlan_change_untagged(vsc,
> + port,
> + vid,
> + true,
> + false);
> + if (ret)
> + goto err;
> + }
> + }
> + }
> +
> + if (pvid) {
> + ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid,
> + true,
> + operate_on_storage);
> + if (ret)
> + goto err;
> + } else if (vsc73xx_port_get_pvid(vsc, port, &vid, false) &&
> + vid == vlan->vid) {
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
> + } else if (vsc->pvid_storage[port] == vlan->vid) {
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
> + }
> + }
> +
> + ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
> + if (!ret)
> + return 0;
> +err:
> + list_del(&vsc73xx_vlan->list);
> + kfree(vsc73xx_vlan);
> + return ret;
> +}
> +
> +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan)
> +{
> + struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> + size_t num_tagged, num_untagged;
> + struct vsc73xx *vsc = ds->priv;
> + bool operate_on_storage;
> + int ret;
> + u16 vid;
> +
> + num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> + num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
> +
> + ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, false);
> + if (ret)
> + return ret;
> +
> + operate_on_storage = vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
> +
> + if (!operate_on_storage) {
> + enum vsc73xx_port_vlan_conf port_vlan_conf =
> + VSC73XX_VLAN_FILTER;
> +
> + if (num_tagged == 0)
> + port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
> + vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +
> + if (num_untagged <= 1) {
> + vid = vsc73xx_find_first_vlan_untagged(vsc, port);
> + vsc73xx_vlan_change_untagged(vsc, port, vid,
> + num_untagged, false);
> + }
> + } else if (vsc->untagged_storage[port] == vlan->vid) {
> + vsc73xx_vlan_change_untagged(vsc, port, 0, false, true);
The fact that the "vid" argument (here 0) is ignored when "set" is false
makes the calling convention confusing, and thus the caller itself.
I would have probably written 2 wrappers on top of
vsc73xx_vlan_change_untagged() as follows:
- vsc73xx_vlan_set_untagged(), which eliminates the "set"
argument from callers (implicitly true)
- vsc73xx_vlan_del_untagged(), which eliminates the "set"
argument (implicitly false) and the "vid" argument (unused)
Same for vsc73xx_vlan_change_pvid().
This will force you to eliminate the complex expressions given to the
"set" argument, like "vsc->untagged_storage[port] < VLAN_N_VID", and
transform them into "if (vsc->untagged_storage[port] < VLAN_N_VID) set() else del()",
which is probably a good thing in terms of readability.
Something like this:
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 05dbeec8eb63..464d74c891d7 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1205,6 +1205,20 @@ vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set,
VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, val);
}
+static int vsc73xx_vlan_set_untagged(struct vsc73xx *vsc, int port, u16 vid,
+ bool operate_on_storage)
+{
+ return vsc73xx_vlan_change_untagged(vsc, port, vid, true,
+ operate_on_storage);
+}
+
+static int vsc73xx_vlan_del_untagged(struct vsc73xx *vsc, int port,
+ bool operate_on_storage)
+{
+ return vsc73xx_vlan_change_untagged(vsc, port, 0, false,
+ operate_on_storage);
+}
+
static int vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid,
bool set, bool operate_on_storage)
{
@@ -1231,6 +1245,20 @@ static int vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid,
vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
}
+static int vsc73xx_vlan_set_pvid(struct vsc73xx *vsc, int port, u16 vid,
+ bool operate_on_storage)
+{
+ return vsc73xx_vlan_change_pvid(vsc, port, vid, true,
+ operate_on_storage);
+}
+
+static int vsc73xx_vlan_del_pvid(struct vsc73xx *vsc, int port,
+ bool operate_on_storage)
+{
+ return vsc73xx_vlan_change_pvid(vsc, port, 0, false,
+ operate_on_storage);
+}
+
static bool vsc73xx_port_get_pvid(struct vsc73xx *vsc, int port, u16 *vid,
bool operate_on_storage)
{
@@ -1367,23 +1395,25 @@ vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
store_untagged = vsc73xx_port_get_untagged(vsc, port,
&vid,
false);
- vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
- true, false);
+ vsc73xx_vlan_set_untagged(vsc, port, vlan_untagged,
+ false);
vsc->untagged_storage[port] = store_untagged ?
vid : VLAN_N_VID;
}
+ } else if (vsc->untagged_storage[port] < VLAN_N_VID) {
+ vsc73xx_vlan_set_untagged(vsc, port, vsc->untagged_storage[port],
+ false);
} else {
- vsc73xx_vlan_change_untagged(vsc, port,
- vsc->untagged_storage[port],
- vsc->untagged_storage[port] <
- VLAN_N_VID, false);
+ vsc73xx_vlan_del_untagged(vsc, port, false);
}
vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
store_pvid = vsc73xx_port_get_pvid(vsc, port, &vid, false);
- vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port],
- vsc->pvid_storage[port] < VLAN_N_VID, false);
+ if (vsc->pvid_storage[port] < VLAN_N_VID)
+ vsc73xx_vlan_set_pvid(vsc, port, vsc->pvid_storage[port], false);
+ else
+ vsc73xx_vlan_del_pvid(vsc, port, false);
vsc->pvid_storage[port] = store_pvid ? vid : VLAN_N_VID;
return 0;
@@ -1463,21 +1493,16 @@ static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
if (port_vlan_conf == VSC73XX_VLAN_FILTER) {
if (untagged) {
- ret = vsc73xx_vlan_change_untagged(vsc,
- port,
- vlan->vid,
- true,
- false);
+ ret = vsc73xx_vlan_set_untagged(vsc, port,
+ vlan->vid,
+ false);
if (ret)
goto err;
} else if (num_untagged == 1) {
vid = vsc73xx_find_first_vlan_untagged(vsc,
port);
- ret = vsc73xx_vlan_change_untagged(vsc,
- port,
- vid,
- true,
- false);
+ ret = vsc73xx_vlan_set_untagged(vsc, port,
+ vid, false);
if (ret)
goto err;
}
@@ -1485,16 +1510,15 @@ static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
}
if (pvid) {
- ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid,
- true,
- operate_on_storage);
+ ret = vsc73xx_vlan_set_pvid(vsc, port, vlan->vid,
+ operate_on_storage);
if (ret)
goto err;
} else if (vsc73xx_port_get_pvid(vsc, port, &vid, false) &&
vid == vlan->vid) {
- vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
+ vsc73xx_vlan_del_pvid(vsc, port, false);
} else if (vsc->pvid_storage[port] == vlan->vid) {
- vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
+ vsc73xx_vlan_del_pvid(vsc, port, true);
}
}
@@ -1534,19 +1558,20 @@ static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
- if (num_untagged <= 1) {
+ if (num_untagged == 1) {
vid = vsc73xx_find_first_vlan_untagged(vsc, port);
- vsc73xx_vlan_change_untagged(vsc, port, vid,
- num_untagged, false);
+ vsc73xx_vlan_set_untagged(vsc, port, vid, false);
+ } else if (num_untagged == 0) {
+ vsc73xx_vlan_del_untagged(vsc, port, false);
}
} else if (vsc->untagged_storage[port] == vlan->vid) {
- vsc73xx_vlan_change_untagged(vsc, port, 0, false, true);
+ vsc73xx_vlan_del_untagged(vsc, port, true);
}
if (vsc73xx_port_get_pvid(vsc, port, &vid, false) && vid == vlan->vid)
- vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
+ vsc73xx_vlan_del_pvid(vsc, port, false);
else if (vsc->pvid_storage[port] == vlan->vid)
- vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
+ vsc73xx_vlan_del_pvid(vsc, port, true);
vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
@@ -1574,11 +1599,9 @@ static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
operate_on_storage = !vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
if (untagged)
- vsc73xx_vlan_change_untagged(vsc, port, vid, true,
- operate_on_storage);
+ vsc73xx_vlan_set_untagged(vsc, port, vid, operate_on_storage);
if (pvid)
- vsc73xx_vlan_change_pvid(vsc, port, vid, true,
- operate_on_storage);
+ vsc73xx_vlan_set_pvid(vsc, port, vid, operate_on_storage);
return vsc73xx_update_vlan_table(vsc, port, vid, true);
}
> + }
> +
> + if (vsc73xx_port_get_pvid(vsc, port, &vid, false) && vid == vlan->vid)
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false, false);
> + else if (vsc->pvid_storage[port] == vlan->vid)
> + vsc73xx_vlan_change_pvid(vsc, port, 0, false, true);
> +
> + vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> + if (vsc73xx_vlan) {
> + vsc73xx_vlan->portmask &= ~BIT(port);
> +
> + if (vsc73xx_vlan->portmask)
> + return 0;
> +
> + list_del(&vsc73xx_vlan->list);
> + kfree(vsc73xx_vlan);
> + }
> +
> + return 0;
> +}
> +
> +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> +{
> + struct vsc73xx *vsc = ds->priv;
> +
> + /* Those bits are responsible for MTU only. Kernel take care about MTU,
takes care
> + * 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 |
> + 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 |
> + 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);
> +
> + /* Initially, there is no backup VLAN configuration to keep track of, so
> + * configure the storage values out of range
> + */
> + vsc->pvid_storage[port] = VLAN_N_VID;
> + vsc->untagged_storage[port] = VLAN_N_VID;
> +
> + return 0;
> +}
> +
> static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
> {
> struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
> @@ -1123,11 +1685,15 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .get_strings = vsc73xx_get_strings,
> .get_ethtool_stats = vsc73xx_get_ethtool_stats,
> .get_sset_count = vsc73xx_get_sset_count,
> + .port_setup = vsc73xx_port_setup,
> .port_enable = vsc73xx_port_enable,
> .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,
> + .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> + .port_vlan_add = vsc73xx_port_vlan_add,
> + .port_vlan_del = vsc73xx_port_vlan_del,
> .phylink_get_caps = vsc73xx_phylink_get_caps,
> };
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index e7b08599a625..facc50f1e320 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -25,6 +25,17 @@
> * @addr: MAC address used in flow control frames
> * @ops: Structure with hardware-dependent operations
> * @priv: Pointer to the configuration interface structure
> + * @pvid_storage: Storage table with PVID configured for other state of
> + * vlan_filtering. It has two alternating roles: it stores the PVID when
> + * configured by the bridge but VLAN filtering is off, and it stores the
> + * PVID necessary for tag_8021q operation when bridge VLAN filtering is
> + * enabled.
> + * @untagged_storage: Storage table with eggres untagged VLAN configured for
egress
> + * other state of vlan_filtering.Keep VID necessary for tag8021q operations
Space between "vlan_filtering." and "Keep".
tag_8021q
> + * when vlan filtering is enabled.
> + * @vlans: List of configured vlans. Contains port mask and untagged status of
> + * every vlan configured in port vlan operation. It doesn't cover tag_8021q
> + * vlans.
> */
> struct vsc73xx {
> struct device *dev;
> @@ -35,6 +46,9 @@ struct vsc73xx {
> u8 addr[ETH_ALEN];
> const struct vsc73xx_ops *ops;
> void *priv;
> + u16 pvid_storage[VSC73XX_MAX_NUM_PORTS];
> + u16 untagged_storage[VSC73XX_MAX_NUM_PORTS];
> + struct list_head vlans;
> };
>
> /**
> @@ -49,6 +63,21 @@ struct vsc73xx_ops {
> u32 val);
> };
>
> +/**
> + * struct vsc73xx_bridge_vlan - VSC73xx driver structure which keeps vlan
> + * database copy
> + * @vid: VLAN number
> + * @portmask: each bit represends one port
represents
> + * @untagged: each bit represends one port configured with @vid untagged
represents
> + * @list: list structure
> + */
> +struct vsc73xx_bridge_vlan {
> + u16 vid;
> + u8 portmask;
> + u8 untagged;
> + struct list_head list;
> +};
> +
> int vsc73xx_is_addr_valid(u8 block, u8 subblock);
> int vsc73xx_probe(struct vsc73xx *vsc);
> void vsc73xx_remove(struct vsc73xx *vsc);
> --
> 2.34.1
>
On Tue, Mar 05, 2024 at 03:51:11PM -0800, Florian Fainelli wrote:
> On 3/1/24 14:16, 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 <[email protected]>
> > ---
>
> [snip]
>
> > +static size_t
> > +vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
> > +{
> > + struct vsc73xx_bridge_vlan *vlan;
> > + size_t num_untagged = 0;
> > +
> > + list_for_each_entry(vlan, &vsc->vlans, list)
> > + if ((vlan->portmask & BIT(port)) &&
> > + (vlan->untagged & BIT(port)) &&
> > + vlan->vid != ignored_vid)
> > + num_untagged++;
> > +
> > + return num_untagged;
> > +}
>
> You always use both helpers at the same time, so I would suggest returning
> num_tagged and num_untagged by reference to have a single linked list
> lookup.
vsc73xx_port_vlan_filtering() calls vsc73xx_bridge_vlan_num_untagged()
but not vsc73xx_bridge_vlan_num_tagged(). Doing as you suggest, while
keeping the code otherwise the same, would imply adding a dummy
num_tagged variable in vlan_filtering().
Though I agree it is generally confusing, because "port_vlan_conf" is
assigned based on inconsistent conditions in vsc73xx_port_vlan_filtering()
vs vsc73xx_port_vlan_add() vs vsc73xx_port_vlan_del(). Namely the number
of tagged VLANs is taken into consideration only on vlan_add(), and of
remaining tagged VLANs on vlan_del(), respectively.
Maybe something like this could help?
struct vsc73xx_vlan_summary {
size_t num_tagged;
size_t num_untagged;
};
static void vsc73xx_bridge_vlan_summary(struct vsc73xx *vsc, int port,
struct vsc73xx_vlan_summary *summary,
u16 ignored_vid)
{
size_t num_tagged = 0, num_untagged = 0;
struct vsc73xx_bridge_vlan *vlan;
list_for_each_entry(vlan, &vsc->vlans, list) {
if (!(vlan->portmask & BIT(port)) || vlan->vid == ignored_vid)
continue;
if (vlan->untagged & BIT(port))
num_untagged++;
else
num_tagged++;
}
summary->num_untagged = num_untagged;
summary->num_tagged = num_tagged;
}
. and use what you need from the summary.
> > + }
> > +
> > + /* CPU port must be always tagged because port separation is based on
> > + * tag_8021q.
> > + */
> > + if (port != CPU_PORT) {
>
> Please reduce indentation here.
>
> Have to admit the logic is a bit hard to follow, but that is also because of
> my lack of understanding of the requirements surrounding the use of
> tag_8021q.
It's not only that. The code is also a bit hard on the brain for me.
An alternative coding pattern would be to observe that certain hardware
registers (the egress-untagged VLAN, the PVID) depend on a constellation
of N input variables (the bridge VLAN filtering state, the tag_8021q
active state, the bridge VLAN table). So, to make the code easier to
follow and to ensure correctness, in theory a central function could be
written, which embeds the same invariant logic of determining what to
program the registers with, depending on the N inputs. This invariant
function is called from every place that modifies any of the N inputs.
What Paweł did here was to have slightly different code paths for
modifying the hardware registers, each code path adjusted slightly on
the state change transition of individual inputs.
This was a design choice on which I commented very early on, stating
that it's unusual but that I can go along with it. It is probably very
ingrained with the choice of the untagged_storage[] and pvid_storage[]
arrays, the logic of swapping the storage with the hardware at VLAN
filtering state change, and thus very hard to change at this stage of
development.
pt., 8 mar 2024 o 14:09 Vladimir Oltean <[email protected]> napisał(a):
>
> On Tue, Mar 05, 2024 at 03:51:11PM -0800, Florian Fainelli wrote:
> > On 3/1/24 14:16, 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 <[email protected]>
> > > ---
> >
> > [snip]
> >
> > [snip]
> >
> > Have to admit the logic is a bit hard to follow, but that is also because of
> > my lack of understanding of the requirements surrounding the use of
> > tag_8021q.
>
> It's not only that. The code is also a bit hard on the brain for me.
>
> An alternative coding pattern would be to observe that certain hardware
> registers (the egress-untagged VLAN, the PVID) depend on a constellation
> of N input variables (the bridge VLAN filtering state, the tag_8021q
> active state, the bridge VLAN table). So, to make the code easier to
> follow and to ensure correctness, in theory a central function could be
> written, which embeds the same invariant logic of determining what to
> program the registers with, depending on the N inputs. This invariant
> function is called from every place that modifies any of the N inputs.
>
> What Paweł did here was to have slightly different code paths for
> modifying the hardware registers, each code path adjusted slightly on
> the state change transition of individual inputs.
>
> This was a design choice on which I commented very early on, stating
> that it's unusual but that I can go along with it. It is probably very
> ingrained with the choice of the untagged_storage[] and pvid_storage[]
> arrays, the logic of swapping the storage with the hardware at VLAN
> filtering state change, and thus very hard to change at this stage of
> development.
I have to admit that it wasn't an optimal implementation. I focused on
the wrong priorities, which led me astray. I prepared v7 and I tried
to maximize simplicity. I hope it will be more acceptable than the v6
version.