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.
Pawel Dembicki (11):
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 vsc73xx 8021q tagger
net: dsa: Define max num of bridges in tag8021q implementation
net: dsa: vsc73xx: Add bridge support
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 | 837 +++++++++++++++++++++----
drivers/net/dsa/vitesse-vsc73xx.h | 53 +-
include/linux/dsa/8021q.h | 5 +
include/linux/dsa/vsc73xx.h | 20 +
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 | 69 ++
14 files changed, 968 insertions(+), 192 deletions(-)
create mode 100644 include/linux/dsa/vsc73xx.h
create mode 100644 net/dsa/tag_vsc73xx_8021q.c
--
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]>
---
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 70dd3f96dafb..5b54823b2caa 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -833,7 +833,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
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]>
---
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
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]>
---
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 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]>
---
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 | 69 +++++++++++++++++++++++++++++++++++++
4 files changed, 78 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..0bf150a10576
--- /dev/null
+++ b/net/dsa/tag_vsc73xx_8021q.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (C) 2023 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;
+
+ if (skb_vlan_tag_present(skb)) {
+ /* Normal traffic path. */
+ dsa_8021q_rcv(skb, &src_port, &switch_id, &vbid, &vid);
+ } else {
+ netdev_warn(netdev, "Couldn't decode source port\n");
+ return NULL;
+ }
+
+ 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_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME);
+
+module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
--
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.
Signed-off-by: Pawel Dembicki <[email protected]>
---
v4:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx.h | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index fee1378508b5..99c5c24ffde0 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -15,7 +15,15 @@
#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 switch
+ * @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 +36,11 @@ struct vsc73xx {
void *priv;
};
+/**
+ * struct vsc73xx_ops - VSC73xx methods container: pointers to hardware-dependent functions
+ * @read: Pointer to the read function from the hardware-dependent interface
+ * @write: Pointer to the write function from the hardware-dependent interface
+ */
struct vsc73xx_ops {
int (*read)(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
u32 *val);
--
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.
The tag driver introduces the most basic functionality required for
proper tagging support.
Signed-off-by: Pawel Dembicki <[email protected]>
---
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 | 38 ++++++++++++++++++++++++--
2 files changed, 37 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 6c7bd1c200b4..9f94ae8c763a 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>
@@ -588,7 +589,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)
@@ -670,7 +671,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");
@@ -739,6 +740,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);
@@ -1504,6 +1511,31 @@ 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;
@@ -1632,6 +1664,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 isn't a fully functional implementation of 802.1D, but
port_stp_state_set is required for a future tag8021q operations.
This implementation handles properly all states, but vsc73xx doesn't
forward STP packets.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
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 | 83 ++++++++++++++++++++++++--
1 file changed, 77 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 75597daaad17..1dca3c476fac 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -163,6 +163,10 @@
#define VSC73XX_AGENCTRL 0xf0
#define VSC73XX_CAPRST 0xff
+#define VSC73XX_SRCMASKS_CPU_COPY BIT(27)
+#define VSC73XX_SRCMASKS_MIRROR BIT(26)
+#define VSC73XX_SRCMASKS_PORTS_MASK GENMASK(7, 0)
+
#define VSC73XX_MACACCESS_CPU_COPY BIT(14)
#define VSC73XX_MACACCESS_FWD_KILL BIT(13)
#define VSC73XX_MACACCESS_IGNORE_VLAN BIT(12)
@@ -619,9 +623,6 @@ static int vsc73xx_setup(struct dsa_switch *ds)
vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
- /* Enable reception of frames on all ports */
- vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
- 0x5f);
/* IP multicast flood mask (table 144) */
vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
0xff);
@@ -841,9 +842,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
if (duplex == DUPLEX_FULL)
val |= VSC73XX_MAC_CFG_FDX;
- /* Enable port (forwarding) in the receieve mask */
- vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
- VSC73XX_RECVMASK, BIT(port), BIT(port));
vsc73xx_adjust_enable_port(vsc, port, val);
}
@@ -1026,6 +1024,78 @@ 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, bool configure)
+{
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct vsc73xx *vsc = ds->priv;
+ int i;
+
+ if (configure) {
+ u16 mask = BIT(CPU_PORT);
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
+
+ if (dp->bridge) {
+ struct dsa_port *other_dp;
+
+ 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);
+ } else {
+ for (i = 0; i < vsc->ds->num_ports; i++) {
+ if (i == port)
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i,
+ VSC73XX_SRCMASKS_PORTS_MASK, 0);
+ else
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+ VSC73XX_SRCMASKS + i, BIT(port), 0);
+ }
+ }
+}
+
+/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
+ * forwarded only from and to PI/SI interface. For more info see chapter
+ * 2.7.1 (CPU Forwarding) in datasheet.
+ * This function is required for tag8021q operations.
+ */
+
+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+ struct vsc73xx *vsc = ds->priv;
+ 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 == BR_STATE_FORWARDING);
+}
+
static const struct dsa_switch_ops vsc73xx_ds_ops = {
.get_tag_protocol = vsc73xx_get_tag_protocol,
.setup = vsc73xx_setup,
@@ -1041,6 +1111,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
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]>
---
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 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]>
---
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 | 523 ++++++++++++++++++++++++-
drivers/net/dsa/vitesse-vsc73xx.h | 25 ++
2 files changed, 546 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 1dca3c476fac..6c7bd1c200b4 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -21,9 +21,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>
@@ -61,6 +63,8 @@
#define VSC73XX_CAT_DROP 0x6e
#define VSC73XX_CAT_PR_MISC_L2 0x6f
#define VSC73XX_CAT_PR_USR_PRIO 0x75
+#define VSC73XX_CAT_VLAN_MISC 0x79
+#define VSC73XX_CAT_PORT_VLAN 0x7a
#define VSC73XX_Q_MISC_CONF 0xdf
/* MAC_CFG register bits */
@@ -121,6 +125,17 @@
#define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1)
#define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0)
+/* TXUPDCFG transmit modify setup bits */
+#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19)
+#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18)
+#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3)
+#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1)
+#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0)
+
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
+
/* CAT_DROP categorizer frame dropping register bits */
#define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
#define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
@@ -134,6 +149,15 @@
#define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1)
#define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0)
+/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
+#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
+#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
+
+/* CAT_PORT_VLAN categorizer port VLAN*/
+#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
+
/* Frame analyzer block 2 registers */
#define VSC73XX_STORMLIMIT 0x02
#define VSC73XX_ADVLEARN 0x03
@@ -188,7 +212,8 @@
#define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
#define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
#define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
-#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
+#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
+#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
@@ -343,6 +368,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) {
@@ -560,6 +591,82 @@ 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),
+ 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
+ 0, VSC73XX_VLANACCESS, &val);
+ if (ret)
+ return ret;
+ return err;
+}
+
+static int
+vsc73xx_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
+{
+ u32 val;
+ int ret;
+
+ vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+ vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
+ *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
+ VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
+ return 0;
+}
+
+static int
+vsc73xx_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
+{
+ int ret;
+
+ vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+ ret = vsc73xx_wait_for_vlan_table_cmd(vsc);
+ if (ret)
+ return ret;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
+ VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+ VSC73XX_VLANACCESS_VLAN_PORT_MASK,
+ VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
+ VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+ (portmap << VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
+
+ return vsc73xx_wait_for_vlan_table_cmd(vsc);
+}
+
+static int
+vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+ u8 portmap;
+ int ret;
+
+ ret = vsc73xx_read_vlan_table_entry(vsc, vid, &portmap);
+ if (ret)
+ return ret;
+
+ if (set)
+ portmap |= BIT(port);
+ 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;
@@ -594,7 +701,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);
@@ -623,6 +730,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);
@@ -635,6 +745,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;
}
@@ -1024,6 +1140,405 @@ 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 = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
+ VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA : 0;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, val);
+
+ val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
+ VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
+
+ val = (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;
+
+ if (operate_on_storage) {
+ vsc->untagged_storage[port] = set ? vid : VLAN_N_VID;
+ return 0;
+ }
+
+ val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
+ vid = set ? vid : 0;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
+ (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
+ return 0;
+}
+
+static int
+vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set, bool operate_on_storage)
+{
+ 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;
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
+
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID,
+ vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
+ return 0;
+}
+
+static 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 u16 vsc73xx_bridge_vlan_num_tagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+ u16 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 u16 vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
+{
+ struct vsc73xx_bridge_vlan *vlan;
+ u16 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 vlan_no, vlan_untagged;
+
+ /* The swap processed bellow is required because vsc73xx is using tag8021q.
+ * When vlan_filtering is disabled, tag8021q use 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 tag8021q.
+ */
+
+ if (vlan_filtering) {
+ /* Use VLAN_N_VID to count all vlans */
+ u16 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_no < VLAN_N_VID) {
+ store_untagged = vsc73xx_port_get_untagged(vsc, port, &vlan_no, false);
+ vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
+ vlan_untagged < VLAN_N_VID, false);
+ vsc->untagged_storage[port] = store_untagged ? vlan_no : 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, &vlan_no, 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 ? vlan_no : VLAN_N_VID;
+
+ return 0;
+}
+
+static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct netlink_ext_ack *extack)
+{
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ struct vsc73xx_bridge_vlan *vsc73xx_vlan;
+ u16 vlan_no, num_tagged, num_untagged;
+ struct vsc73xx *vsc = ds->priv;
+ int ret;
+
+ /* Be sure to deny alterations to the configuration done by tag_8021q.
+ */
+ if (vid_is_dsa_8021q(vlan->vid)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Range 3072-4095 reserved for dsa_8021q operation");
+ return -EBUSY;
+ }
+
+ 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!\n");
+ 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 are based on 8021q.*/
+ if (port != CPU_PORT) {
+ bool 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 && 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,
+ operate_on_storage);
+ if (ret)
+ return ret;
+ } else if (num_untagged == 1) {
+ vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
+ ret = vsc73xx_vlan_change_untagged(vsc, port, vlan_no, true,
+ operate_on_storage);
+ if (ret)
+ return ret;
+ }
+ }
+ }
+
+ if (pvid) {
+ ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid, true,
+ operate_on_storage);
+ if (ret)
+ return ret;
+ } else {
+ if (vsc73xx_port_get_pvid(vsc, port, &vlan_no, false) &&
+ vlan_no == 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);
+ }
+ }
+
+ return vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
+}
+
+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;
+ u16 vlan_no, num_tagged, num_untagged;
+ struct vsc73xx *vsc = ds->priv;
+ bool operate_on_storage;
+ int ret;
+
+ 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) {
+ vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
+ vsc73xx_vlan_change_untagged(vsc, port, vlan_no, 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, &vlan_no, false) && vlan_no == 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, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA,
+ VSC73XX_CAT_DROP_UNTAGGED_ENA);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+
+ if (port == CPU_PORT)
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
+ else
+ vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
+
+ /* Set 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, bool configure)
{
struct dsa_port *dp = dsa_to_port(ds, port);
@@ -1107,11 +1622,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 99c5c24ffde0..01eb2dd48f03 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -24,6 +24,14 @@
* @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 have two roles: Keep PVID when PVID is configured but vlan filtering is off
+ * and keep PVID necessary for tag8021q operations when vlan filtering is enabled.
+ * @untagged_storage: Storage table with eggress 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. Contain port mask and untagged status of every
+ * vlan configured in port vlan operation. It doesn't cover tag8021q vlans.
*/
struct vsc73xx {
struct device *dev;
@@ -34,6 +42,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;
};
/**
@@ -48,6 +59,20 @@ 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
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]>
---
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 9f94ae8c763a..ade90a48c53f 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -675,6 +675,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);
@@ -1536,6 +1539,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;
@@ -1657,6 +1676,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
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]>
---
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]>
---
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
On Tue, Feb 13, 2024 at 11:05 PM Pawel Dembicki <[email protected]> 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]>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Tue, Feb 13, 2024 at 11:05 PM Pawel Dembicki <[email protected]> wrote:
> This commit adds updates to the documentation describing the structures
> used in vsc73xx. This will help prevent kdoc-related issues in the future.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 2/13/24 14:03, 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]>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Tue, Feb 13, 2024 at 11:05 PM Pawel Dembicki <[email protected]> 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]>
As far as I can tell it does the right thing! Good work.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 2/13/24 14:03, Pawel Dembicki 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]>
--
Florian
On 2/13/24 14:03, Pawel Dembicki wrote:
> This commit adds updates to the documentation describing the structures
> used in vsc73xx. This will help prevent kdoc-related issues in the future.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hi Paweł,
On Tue, Feb 13, 2024 at 11:03:22PM +0100, Pawel Dembicki wrote:
> 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]>
> ---
This needs your Signed-off-by tag as well. Multiple patches have this issue.
On Tue, Feb 13, 2024 at 11:03:16PM +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]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Tue, Feb 13, 2024 at 11:03:18PM +0100, Pawel Dembicki wrote:
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index fee1378508b5..99c5c24ffde0 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -15,7 +15,15 @@
> #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 switch
> + * @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 +36,11 @@ struct vsc73xx {
> void *priv;
> };
>
> +/**
> + * struct vsc73xx_ops - VSC73xx methods container: pointers to hardware-dependent functions
The netdev coding style still sticks to the "80 characters per line" rule.
I can't find other deviations from this rule in the driver.
Maybe you can cut down on the boilerplate a little bit. Like:
/**
* 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
> + * @read: Pointer to the read function from the hardware-dependent interface
> + * @write: Pointer to the write function from the hardware-dependent interface
> + */
> struct vsc73xx_ops {
> int (*read)(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
> u32 *val);
> --
> 2.34.1
>
On Tue, Feb 13, 2024 at 11:03:19PM +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.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
> v4:
> - fully reworked port_stp_state_set
A "fully reworked" patch isn't exactly compatible with keeping attached
Reviewed-by tags attached to the patch. It implies Linus didn't actually
review the code you are submitting now.
> 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 | 83 ++++++++++++++++++++++++--
> 1 file changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 75597daaad17..1dca3c476fac 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -163,6 +163,10 @@
> #define VSC73XX_AGENCTRL 0xf0
> #define VSC73XX_CAPRST 0xff
>
> +#define VSC73XX_SRCMASKS_CPU_COPY BIT(27)
> +#define VSC73XX_SRCMASKS_MIRROR BIT(26)
> +#define VSC73XX_SRCMASKS_PORTS_MASK GENMASK(7, 0)
> +
> #define VSC73XX_MACACCESS_CPU_COPY BIT(14)
> #define VSC73XX_MACACCESS_FWD_KILL BIT(13)
> #define VSC73XX_MACACCESS_IGNORE_VLAN BIT(12)
> @@ -619,9 +623,6 @@ static int vsc73xx_setup(struct dsa_switch *ds)
> vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
> VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
> VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> - /* Enable reception of frames on all ports */
> - vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
> - 0x5f);
> /* IP multicast flood mask (table 144) */
> vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> 0xff);
> @@ -841,9 +842,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> if (duplex == DUPLEX_FULL)
> val |= VSC73XX_MAC_CFG_FDX;
>
> - /* Enable port (forwarding) in the receieve mask */
> - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> - VSC73XX_RECVMASK, BIT(port), BIT(port));
> vsc73xx_adjust_enable_port(vsc, port, val);
> }
>
> @@ -1026,6 +1024,78 @@ 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, bool configure)
Why have you chosen the name "bool configure" exactly? It is not
intuitive. Could you just pass the STP state of the port as an argument?
> +{
> + struct dsa_port *dp = dsa_to_port(ds, port);
> + struct vsc73xx *vsc = ds->priv;
> + int i;
> +
> + if (configure) {
> + u16 mask = BIT(CPU_PORT);
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
> +
> + if (dp->bridge) {
> + struct dsa_port *other_dp;
> +
> + 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);
> + } else {
> + for (i = 0; i < vsc->ds->num_ports; i++) {
Try to refrain from doing plain integer iterations through ports when
there are holes in the port indices. Use dsa_switch_for_each_available_port()
which skips the unused/absent ports, and consequently you're not
touching unwanted registers.
> + if (i == port)
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + i,
> + VSC73XX_SRCMASKS_PORTS_MASK, 0);
> + else
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + i, BIT(port), 0);
> + }
> + }
To save on indentation, I would simplify the big if () else () block
like this.
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 (dp == other_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) {
struct dsa_port *other_dp;
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 tag8021q operations.
Please keep the name consistent at "tag_8021q".
> + */
> +
nitpick: remove the blank line.
> +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);
I am extremely particular about this. Please also implement .port_pre_bridge_flags()
and .port_bridge_flags(), and treat the BR_LEARNING flag. The idea is
that standalone ports are in the BR_STATE_FORWARDING state (it's a DSA
API thing), but learning should _not_ be enabled (it breaks things when
one port sees traffic originated from the other). So you need to set the
bit in VSC73XX_LEARNMASK only if the port is in the BR_STATE_LEARNING ||
BR_STATE_FORWARDING, _and_ the BR_LEARNING flag is set.
This should be relatively easy to do. If you're not going to do it now
because of patch set size constraints, at least leave a big FIXME in the
code or in the commit message.
> +
> + /* 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 == BR_STATE_FORWARDING);
> +}
> +
> static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .get_tag_protocol = vsc73xx_get_tag_protocol,
> .setup = vsc73xx_setup,
> @@ -1041,6 +1111,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
>
On Tue, Feb 13, 2024 at 11:03:20PM +0100, 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]>
> ---
This looks good, please keep working on it. I have a lot of comments
below, but they are all minor and don't involve structural rework.
> 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 | 523 ++++++++++++++++++++++++-
> drivers/net/dsa/vitesse-vsc73xx.h | 25 ++
> 2 files changed, 546 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 1dca3c476fac..6c7bd1c200b4 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -21,9 +21,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>
>
> @@ -61,6 +63,8 @@
> #define VSC73XX_CAT_DROP 0x6e
> #define VSC73XX_CAT_PR_MISC_L2 0x6f
> #define VSC73XX_CAT_PR_USR_PRIO 0x75
> +#define VSC73XX_CAT_VLAN_MISC 0x79
> +#define VSC73XX_CAT_PORT_VLAN 0x7a
> #define VSC73XX_Q_MISC_CONF 0xdf
>
> /* MAC_CFG register bits */
> @@ -121,6 +125,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
nitpick: align a tab, not a space.
> +
> /* CAT_DROP categorizer frame dropping register bits */
> #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
> #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
> @@ -134,6 +149,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*/
nitpick: space before */
> +#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*/
nitpick: space before */
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> +
> /* Frame analyzer block 2 registers */
> #define VSC73XX_STORMLIMIT 0x02
> #define VSC73XX_ADVLEARN 0x03
> @@ -188,7 +212,8 @@
> #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
> #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
> #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
> -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
> +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
> +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
> #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
> @@ -343,6 +368,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) {
> @@ -560,6 +591,82 @@ 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),
> + 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
#define VSC73XX_VLAN_TBL_SLEEP_US
#define VSC73XX_VLAN_TBL_TIMEOUT_US
> + 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;
Some blank lines before and after the sequences:
ret = do_x();
if (ret)
return ret;
would improve the readability.
> + 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);
nitpick: remove extra space.
> +}
> +
> static int vsc73xx_setup(struct dsa_switch *ds)
> {
> struct vsc73xx *vsc = ds->priv;
> @@ -594,7 +701,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);
> @@ -623,6 +730,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);
> @@ -635,6 +745,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>
> udelay(4);
>
> + /* Clear VLAN table*/
nitpick: space before */
> + for (i = 0; i < VLAN_N_VID; i++)
> + vsc73xx_write_vlan_table_entry(vsc, i, 0);
> +
> + INIT_LIST_HEAD(&vsc->vlans);
> +
> return 0;
> }
>
> @@ -1024,6 +1140,405 @@ 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 = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
> + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA : 0;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, val);
> +
> + val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
Could you perform the VSC73XX_CAT_VLAN_MISC modification as part of a
single call? Something like this:
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)
This also returns 0, so it can be made void. Although vsc73xx_update_bits()
can fail on register access too, but... yeah. We don't have a great story
for that. Your choice on whether to propagate the unlikely error, or just
print it somewhere. Just don't let it fail completely silently.
> +{
> + u32 val;
> +
> + if (operate_on_storage) {
> + vsc->untagged_storage[port] = set ? vid : VLAN_N_VID;
> + return 0;
> + }
> +
> + val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> + vid = set ? vid : 0;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> + (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
Same here, try to merge, otherwise you get 4 I/O operations on a slow
bus instead of 2.
> + return 0;
> +}
> +
> +static int
> +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set, bool operate_on_storage)
nitpick: 80 character line limit
> +{
> + 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;
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> + VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> + vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> + return 0;
> +}
> +
> +static bool
> +vsc73xx_port_get_pvid(struct vsc73xx *vsc, int port, u16 *vid, bool operate_on_storage)
nitpick: 80 character line limit
All comments apply elsewhere too, I might have missed some, so please
take a second look.
> +{
> + 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)
nitpick: 80 character line limit
> +{
> + 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)
nitpick: 80 character line limit
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> +
> + list_for_each_entry(vlan, &vsc->vlans, list)
> + if (vlan->vid == vid)
> + return vlan;
> +
> + return NULL;
> +}
> +
> +static u16 vsc73xx_bridge_vlan_num_tagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
nitpick: 80 character line limit
Also, any reason why this has to return a fixed-size u16? You could use
the size_t type for object counts.
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> + u16 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 u16 vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
nitpick: 80 character line limit
> +{
> + struct vsc73xx_bridge_vlan *vlan;
> + u16 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;
nitpick: remove one extra space
> + bool store_pvid = false;
> + u16 vlan_no, vlan_untagged;
nitpick: order variable declaration lines in the order of decreasing
line length.
Also, "vid" is a more conventional name than "vlan_no".
> +
> + /* The swap processed bellow is required because vsc73xx is using tag8021q.
s/bellow/below/
s/tag8021q/tag_8021q/
> + * When vlan_filtering is disabled, tag8021q use pvid/untagged vlans for
s/tag8021q use/tag_8021q uses/
> + * 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 tag8021q.
> + */
> +
nitpick: remove blank line
> + if (vlan_filtering) {
> + /* Use VLAN_N_VID to count all vlans */
> + u16 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_no < VLAN_N_VID) {
> + store_untagged = vsc73xx_port_get_untagged(vsc, port, &vlan_no, false);
> + vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> + vlan_untagged < VLAN_N_VID, false);
> + vsc->untagged_storage[port] = store_untagged ? vlan_no : 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, &vlan_no, 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 ? vlan_no : VLAN_N_VID;
> +
> + return 0;
> +}
> +
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan,
> + struct netlink_ext_ack *extack)
> +{
> + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> + struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> + u16 vlan_no, num_tagged, num_untagged;
> + struct vsc73xx *vsc = ds->priv;
> + int ret;
> +
> + /* Be sure to deny alterations to the configuration done by tag_8021q.
> + */
> + if (vid_is_dsa_8021q(vlan->vid)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Range 3072-4095 reserved for dsa_8021q operation");
> + return -EBUSY;
> + }
> +
> + num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> + num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
Could you add a comment explaining why you need to exclude vlan->vid
from the search? I guess it's 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.
> +
> + /* 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!\n");
No need for \n in extack message. Also, no need for exclamation mark.
> + 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;
kzalloc zero-initializes the memory, so |= can be simplified to just =.
> +
> + 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 are based on 8021q.*/
nitpick: space before */
s/are based/is based/
s/8021q/tag_8021q/
> + if (port != CPU_PORT) {
> + bool operate_on_storage = vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
It would look better if you had a local variable declaration at the
beginning:
struct dsa_port *dp = dsa_to_port(ds, port);
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,
> + operate_on_storage);
> + if (ret)
> + return ret;
This leaks vsc73xx_vlan.
> + } else if (num_untagged == 1) {
> + vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
> + ret = vsc73xx_vlan_change_untagged(vsc, port, vlan_no, true,
> + operate_on_storage);
> + if (ret)
> + return ret;
Same here.
> + }
> + }
> + }
> +
> + if (pvid) {
> + ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid, true,
> + operate_on_storage);
> + if (ret)
> + return ret;
Same here.
> + } else {
> + if (vsc73xx_port_get_pvid(vsc, port, &vlan_no, false) &&
> + vlan_no == 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);
Nitpick:
if () {
} else {
if ()
else if
}
can be expressed with one indentation level less as:
if () {
} else if () {
} else if () {
}
> + }
> + }
> +
> + return vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
If this returns an error, it also leaks vsc73xx_vlan.
> +}
> +
> +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;
> + u16 vlan_no, num_tagged, num_untagged;
> + struct vsc73xx *vsc = ds->priv;
> + bool operate_on_storage;
> + int ret;
> +
> + 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) {
> + vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
> + vsc73xx_vlan_change_untagged(vsc, port, vlan_no, 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, &vlan_no, false) && vlan_no == 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, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA,
> + VSC73XX_CAT_DROP_UNTAGGED_ENA);
Merge operations.
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
Merge operations.
> + 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);
> +
> + /* Set storage values out of range */
please make the comment more useful by adding an explanation why. Like
"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, bool configure)
> {
> struct dsa_port *dp = dsa_to_port(ds, port);
> @@ -1107,11 +1622,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 99c5c24ffde0..01eb2dd48f03 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -24,6 +24,14 @@
> * @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 have two roles: Keep PVID when PVID is configured but vlan filtering is off
"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".
> + * and keep PVID necessary for tag8021q operations when vlan filtering is enabled.
> + * @untagged_storage: Storage table with eggress untagged VLAN configured for
s/eggress/egress/
> + * other state of vlan_filtering.Keep VID necessary for tag8021q operations when
> + * vlan filtering is enabled.
> + * @vlans: List of configured vlans. Contain port mask and untagged status of every
Contains
> + * vlan configured in port vlan operation. It doesn't cover tag8021q vlans.
> */
> struct vsc73xx {
> struct device *dev;
> @@ -34,6 +42,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;
> };
>
> /**
> @@ -48,6 +59,20 @@ struct vsc73xx_ops {
> u32 val);
> };
>
> +/**
> + * struct vsc73xx_bridge_vlan - VSC73xx driver structure which keeps vlan database copy
More succinct: "Copy of VLAN database entry"
> + * @vid: VLAN number
> + * @portmask: each bit represends one port
s/represends/represents/
> + * @untagged: each bit represends one port configured with @vid untagged
s/represends/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, Feb 13, 2024 at 11:03:25PM +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]>
> ---
> 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 | 69 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 78 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..0bf150a10576
> --- /dev/null
> +++ b/net/dsa/tag_vsc73xx_8021q.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (C) 2023 Pawel Dembicki <[email protected]>
It's 2024 already :)
> + */
> +#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;
> +
> + if (skb_vlan_tag_present(skb)) {
> + /* Normal traffic path. */
> + dsa_8021q_rcv(skb, &src_port, &switch_id, &vbid, &vid);
dsa_8021q_rcv() also works with VLAN tags in the skb head, not in the
hwaccel area. So please remove "if (skb_vlan_tag_present())" and the
"else" clause, and let dsa_tag_8021q_find_user() below fail if it will.
> + } else {
> + netdev_warn(netdev, "Couldn't decode source port\n");
> + return NULL;
> + }
> +
> + 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_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME);
> +
> +module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
> --
> 2.34.1
>
In the context of DSA, tagger is synonymous with "tagging protocol
driver", aka what the previous patch implemented in
net/dsa/tag_vsc73xx_8021q.c. It would be better to rename the commit
title to "Implement the tag_8021q VLAN operations".
On Tue, Feb 13, 2024 at 11:03:26PM +0100, Pawel Dembicki wrote:
> This patch is a simple implementation of 802.1q tagging in the vsc73xx
> driver. Currently, devices with DSA_TAG_PROTO_NONE are not functional.
> The VSC73XX family doesn't provide any tag support for external Ethernet
> ports.
>
> The only option available is VLAN-based tagging, which requires constant
> hardware VLAN filtering. While the VSC73XX family supports provider
> bridging, it only supports QinQ without full implementation of 802.1AD.
> This means it only allows the doubled 0x8100 TPID.
>
> In the simple port mode, QinQ is enabled to preserve forwarding of
> VLAN-tagged frames.
>
> The tag driver introduces the most basic functionality required for
> proper tagging support.
I guess this sentence is in place from before the tagging protocol
driver split in v3. Please remove it, it is confusing now.
>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
> 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 | 38 ++++++++++++++++++++++++--
> 2 files changed, 37 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 6c7bd1c200b4..9f94ae8c763a 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>
I believe iopoll.h is misplaced in this patch. Probably the first one to
have used read_poll_timeout() should have included it.
> #include <linux/of.h>
> #include <linux/of_mdio.h>
> #include <linux/bitops.h>
On Tue, Feb 13, 2024 at 11:03:28PM +0100, 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]>
On Tue, Feb 13, 2024 at 11:03:20PM +0100, Pawel Dembicki wrote:
> +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 vlan_no, vlan_untagged;
> +
> + /* The swap processed bellow is required because vsc73xx is using tag8021q.
> + * When vlan_filtering is disabled, tag8021q use 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 tag8021q.
> + */
> +
> + if (vlan_filtering) {
> + /* Use VLAN_N_VID to count all vlans */
> + u16 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_no < VLAN_N_VID) {
One more that I've missed. There's a bug here. You don't mean "vlan_no"
(which at this stage is uninitialized), but "vlan_untagged".
> + store_untagged = vsc73xx_port_get_untagged(vsc, port, &vlan_no, false);
> + vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> + vlan_untagged < VLAN_N_VID, false);
> + vsc->untagged_storage[port] = store_untagged ? vlan_no : 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, &vlan_no, 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 ? vlan_no : VLAN_N_VID;
> +
> + return 0;
> +}
czw., 15 lut 2024 o 18:22 Vladimir Oltean <[email protected]> napisał(a):
>
> On Tue, Feb 13, 2024 at 11:03:20PM +0100, 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]>
> > ---
>
> This looks good, please keep working on it. I have a lot of comments
> below, but they are all minor and don't involve structural rework.
>
> > 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 | 523 ++++++++++++++++++++++++-
> > drivers/net/dsa/vitesse-vsc73xx.h | 25 ++
> > 2 files changed, 546 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 1dca3c476fac..6c7bd1c200b4 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -21,9 +21,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>
> >
> > @@ -61,6 +63,8 @@
> > #define VSC73XX_CAT_DROP 0x6e
> > #define VSC73XX_CAT_PR_MISC_L2 0x6f
> > #define VSC73XX_CAT_PR_USR_PRIO 0x75
> > +#define VSC73XX_CAT_VLAN_MISC 0x79
> > +#define VSC73XX_CAT_PORT_VLAN 0x7a
> > #define VSC73XX_Q_MISC_CONF 0xdf
> >
> > /* MAC_CFG register bits */
> > @@ -121,6 +125,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
>
> nitpick: align a tab, not a space.
>
> > +
> > /* CAT_DROP categorizer frame dropping register bits */
> > #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6)
> > #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4)
> > @@ -134,6 +149,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*/
>
> nitpick: space before */
>
> > +#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*/
>
> nitpick: space before */
>
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> > +
> > /* Frame analyzer block 2 registers */
> > #define VSC73XX_STORMLIMIT 0x02
> > #define VSC73XX_ADVLEARN 0x03
> > @@ -188,7 +212,8 @@
> > #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29)
> > #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28)
> > #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2)
> > -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
> > +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2
> > +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1
> > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2
> > @@ -343,6 +368,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) {
> > @@ -560,6 +591,82 @@ 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),
> > + 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
>
> #define VSC73XX_VLAN_TBL_SLEEP_US
> #define VSC73XX_VLAN_TBL_TIMEOUT_US
>
> > + 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;
>
> Some blank lines before and after the sequences:
>
> ret = do_x();
> if (ret)
> return ret;
>
> would improve the readability.
>
> > + 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);
>
> nitpick: remove extra space.
>
> > +}
> > +
> > static int vsc73xx_setup(struct dsa_switch *ds)
> > {
> > struct vsc73xx *vsc = ds->priv;
> > @@ -594,7 +701,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);
> > @@ -623,6 +730,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);
> > @@ -635,6 +745,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
> >
> > udelay(4);
> >
> > + /* Clear VLAN table*/
>
> nitpick: space before */
>
> > + for (i = 0; i < VLAN_N_VID; i++)
> > + vsc73xx_write_vlan_table_entry(vsc, i, 0);
> > +
> > + INIT_LIST_HEAD(&vsc->vlans);
> > +
> > return 0;
> > }
> >
> > @@ -1024,6 +1140,405 @@ 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 = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
> > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA : 0;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, val);
> > +
> > + val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
> > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
>
> Could you perform the VSC73XX_CAT_VLAN_MISC modification as part of a
> single call? Something like this:
>
> 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)
>
> This also returns 0, so it can be made void. Although vsc73xx_update_bits()
> can fail on register access too, but... yeah. We don't have a great story
> for that. Your choice on whether to propagate the unlikely error, or just
> print it somewhere. Just don't let it fail completely silently.
>
> > +{
> > + u32 val;
> > +
> > + if (operate_on_storage) {
> > + vsc->untagged_storage[port] = set ? vid : VLAN_N_VID;
> > + return 0;
> > + }
> > +
> > + val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> > + vid = set ? vid : 0;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> > + (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
>
> Same here, try to merge, otherwise you get 4 I/O operations on a slow
> bus instead of 2.
>
> > + return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set, bool operate_on_storage)
>
> nitpick: 80 character line limit
>
> > +{
> > + 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;
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> > +
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> > + VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> > + vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> > + return 0;
> > +}
> > +
> > +static bool
> > +vsc73xx_port_get_pvid(struct vsc73xx *vsc, int port, u16 *vid, bool operate_on_storage)
>
> nitpick: 80 character line limit
>
> All comments apply elsewhere too, I might have missed some, so please
> take a second look.
>
> > +{
> > + 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)
>
> nitpick: 80 character line limit
>
> > +{
> > + 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)
>
> nitpick: 80 character line limit
>
> > +{
> > + struct vsc73xx_bridge_vlan *vlan;
> > +
> > + list_for_each_entry(vlan, &vsc->vlans, list)
> > + if (vlan->vid == vid)
> > + return vlan;
> > +
> > + return NULL;
> > +}
> > +
> > +static u16 vsc73xx_bridge_vlan_num_tagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
>
> nitpick: 80 character line limit
>
> Also, any reason why this has to return a fixed-size u16? You could use
> the size_t type for object counts.
>
I used the smallest size. It will never exceed 3076.
But I will use size_t .
> > +{
> > + struct vsc73xx_bridge_vlan *vlan;
> > + u16 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 u16 vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
>
> nitpick: 80 character line limit
>
> > +{
> > + struct vsc73xx_bridge_vlan *vlan;
> > + u16 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;
>
> nitpick: remove one extra space
>
> > + bool store_pvid = false;
> > + u16 vlan_no, vlan_untagged;
>
> nitpick: order variable declaration lines in the order of decreasing
> line length.
>
> Also, "vid" is a more conventional name than "vlan_no".
>
> > +
> > + /* The swap processed bellow is required because vsc73xx is using tag8021q.
>
> s/bellow/below/
> s/tag8021q/tag_8021q/
>
> > + * When vlan_filtering is disabled, tag8021q use pvid/untagged vlans for
>
> s/tag8021q use/tag_8021q uses/
>
> > + * 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 tag8021q.
> > + */
> > +
>
> nitpick: remove blank line
>
> > + if (vlan_filtering) {
> > + /* Use VLAN_N_VID to count all vlans */
> > + u16 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_no < VLAN_N_VID) {
> > + store_untagged = vsc73xx_port_get_untagged(vsc, port, &vlan_no, false);
> > + vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> > + vlan_untagged < VLAN_N_VID, false);
> > + vsc->untagged_storage[port] = store_untagged ? vlan_no : 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, &vlan_no, 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 ? vlan_no : VLAN_N_VID;
> > +
> > + return 0;
> > +}
> > +
> > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > + const struct switchdev_obj_port_vlan *vlan,
> > + struct netlink_ext_ack *extack)
> > +{
> > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > + struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> > + u16 vlan_no, num_tagged, num_untagged;
> > + struct vsc73xx *vsc = ds->priv;
> > + int ret;
> > +
> > + /* Be sure to deny alterations to the configuration done by tag_8021q.
> > + */
> > + if (vid_is_dsa_8021q(vlan->vid)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Range 3072-4095 reserved for dsa_8021q operation");
> > + return -EBUSY;
> > + }
> > +
> > + num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> > + num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
>
> Could you add a comment explaining why you need to exclude vlan->vid
> from the search? I guess it's 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.
>
> > +
> > + /* 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!\n");
>
> No need for \n in extack message. Also, no need for exclamation mark.
>
> > + 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;
>
> kzalloc zero-initializes the memory, so |= can be simplified to just =.
>
> > +
> > + 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 are based on 8021q.*/
>
> nitpick: space before */
>
> s/are based/is based/
> s/8021q/tag_8021q/
>
> > + if (port != CPU_PORT) {
> > + bool operate_on_storage = vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
>
> It would look better if you had a local variable declaration at the
> beginning:
>
> struct dsa_port *dp = dsa_to_port(ds, port);
>
> 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,
> > + operate_on_storage);
> > + if (ret)
> > + return ret;
>
> This leaks vsc73xx_vlan.
>
> > + } else if (num_untagged == 1) {
> > + vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
> > + ret = vsc73xx_vlan_change_untagged(vsc, port, vlan_no, true,
> > + operate_on_storage);
> > + if (ret)
> > + return ret;
>
> Same here.
>
> > + }
> > + }
> > + }
> > +
> > + if (pvid) {
> > + ret = vsc73xx_vlan_change_pvid(vsc, port, vlan->vid, true,
> > + operate_on_storage);
> > + if (ret)
> > + return ret;
>
> Same here.
>
> > + } else {
> > + if (vsc73xx_port_get_pvid(vsc, port, &vlan_no, false) &&
> > + vlan_no == 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);
>
> Nitpick:
>
> if () {
> } else {
> if ()
> else if
> }
>
> can be expressed with one indentation level less as:
>
> if () {
> } else if () {
> } else if () {
> }
>
> > + }
> > + }
> > +
> > + return vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
>
> If this returns an error, it also leaks vsc73xx_vlan.
>
> > +}
> > +
> > +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;
> > + u16 vlan_no, num_tagged, num_untagged;
> > + struct vsc73xx *vsc = ds->priv;
> > + bool operate_on_storage;
> > + int ret;
> > +
> > + 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) {
> > + vlan_no = vsc73xx_find_first_vlan_untagged(vsc, port);
> > + vsc73xx_vlan_change_untagged(vsc, port, vlan_no, 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, &vlan_no, false) && vlan_no == 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, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA,
> > + VSC73XX_CAT_DROP_UNTAGGED_ENA);
>
> Merge operations.
>
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
>
> Merge operations.
>
> > + 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);
> > +
> > + /* Set storage values out of range */
>
> please make the comment more useful by adding an explanation why. Like
> "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, bool configure)
> > {
> > struct dsa_port *dp = dsa_to_port(ds, port);
> > @@ -1107,11 +1622,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 99c5c24ffde0..01eb2dd48f03 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -24,6 +24,14 @@
> > * @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 have two roles: Keep PVID when PVID is configured but vlan filtering is off
>
> "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".
>
> > + * and keep PVID necessary for tag8021q operations when vlan filtering is enabled.
> > + * @untagged_storage: Storage table with eggress untagged VLAN configured for
>
> s/eggress/egress/
>
> > + * other state of vlan_filtering.Keep VID necessary for tag8021q operations when
> > + * vlan filtering is enabled.
> > + * @vlans: List of configured vlans. Contain port mask and untagged status of every
>
> Contains
>
> > + * vlan configured in port vlan operation. It doesn't cover tag8021q vlans.
> > */
> > struct vsc73xx {
> > struct device *dev;
> > @@ -34,6 +42,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;
> > };
> >
> > /**
> > @@ -48,6 +59,20 @@ struct vsc73xx_ops {
> > u32 val);
> > };
> >
> > +/**
> > + * struct vsc73xx_bridge_vlan - VSC73xx driver structure which keeps vlan database copy
>
> More succinct: "Copy of VLAN database entry"
>
> > + * @vid: VLAN number
> > + * @portmask: each bit represends one port
>
> s/represends/represents/
>
> > + * @untagged: each bit represends one port configured with @vid untagged
>
> s/represends/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
> >
>