[RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

This adds full 802.1q VLAN support to the qca8k, allowing the use of
vlan_filtering and more complicated bridging setups than allowed by
basic port VLAN support.

Tested with a number of untagged ports with separate VLANs and then a
trunk port with all the VLANs tagged on it.

Signed-off-by: Jonathan McDowell <noodles@earth.li>

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a5566de82853..cce05493075f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -408,6 +408,104 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
+static int
+qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
+{
+	u32 reg;
+
+	/* Set the command and VLAN index */
+	reg = QCA8K_VTU_FUNC1_BUSY;
+	reg |= cmd;
+	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
+
+	/* Write the function register triggering the table access */
+	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+
+	/* wait for completion */
+	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
+		return -1;
+
+	/* Check for table full violation when adding an entry */
+	if (cmd == QCA8K_VLAN_LOAD) {
+		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
+		if (reg & QCA8K_VTU_FUNC1_FULL)
+			return -1;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
+{
+	u32 reg;
+	int ret;
+
+	if (!vid)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret >= 0) {
+		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+		reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
+		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+		if (tagged)
+			reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
+					QCA8K_VTU_FUNC0_EG_MODE_S(port);
+		else
+			reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
+					QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+	}
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
+{
+	u32 reg;
+	u32 mask;
+	int ret;
+	int i;
+	bool del;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret >= 0) {
+		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
+				QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+		/* Check if we're the last member to be removed */
+		del = true;
+		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+			mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
+			mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
+
+			if ((reg & mask) != mask) {
+				del = false;
+				break;
+			}
+		}
+
+		if (del) {
+			ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
+		} else {
+			qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+			ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+		}
+	}
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
 static void
 qca8k_mib_init(struct qca8k_priv *priv)
 {
@@ -663,10 +761,11 @@ qca8k_setup(struct dsa_switch *ds)
 			 * default egress vid
 			 */
 			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-				  0xffff << shift, 1 << shift);
+				  0xffff << shift,
+				  QCA8K_PORT_VID_DEF << shift);
 			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
-				    QCA8K_PORT_VLAN_CVID(1) |
-				    QCA8K_PORT_VLAN_SVID(1));
+				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
+				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
 		}
 	}
 
@@ -1133,7 +1232,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 {
 	/* Set the vid to the port vlan id if no vid is set */
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_add(priv, addr, port_mask, vid,
 			     QCA8K_ATU_STATUS_STATIC);
@@ -1157,7 +1256,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 	u16 port_mask = BIT(port);
 
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_del(priv, addr, port_mask, vid);
 }
@@ -1186,6 +1285,71 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int
+qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	if (vlan_filtering) {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+	} else {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+	}
+
+	return 0;
+}
+
+static int
+qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_vlan *vlan)
+{
+	if (!vlan->vid_begin)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static void
+qca8k_port_vlan_add(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	struct qca8k_priv *priv = ds->priv;
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	u16 vid;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
+		qca8k_vlan_add(priv, port, vid, !untagged);
+
+	if (pvid) {
+		int shift = 16 * (port % 2);
+
+		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
+			  0xffff << shift,
+			  vlan->vid_end << shift);
+		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
+			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
+			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
+	}
+}
+
+static int
+qca8k_port_vlan_del(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u16 vid;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
+		qca8k_vlan_del(priv, port, vid);
+
+	return 0;
+}
+
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 		       enum dsa_tag_protocol mp)
@@ -1211,6 +1375,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.port_vlan_filtering	= qca8k_port_vlan_filtering,
+	.port_vlan_prepare	= qca8k_port_vlan_prepare,
+	.port_vlan_add		= qca8k_port_vlan_add,
+	.port_vlan_del		= qca8k_port_vlan_del,
 	.phylink_validate	= qca8k_phylink_validate,
 	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
 	.phylink_mac_config	= qca8k_phylink_mac_config,
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 31439396401c..4e96275cbc3e 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -22,6 +22,8 @@
 
 #define QCA8K_CPU_PORT					0
 
+#define QCA8K_PORT_VID_DEF				1
+
 /* Global control registers */
 #define QCA8K_REG_MASK_CTRL				0x000
 #define   QCA8K_MASK_CTRL_ID_M				0xff
@@ -126,6 +128,18 @@
 #define   QCA8K_ATU_FUNC_FULL				BIT(12)
 #define   QCA8K_ATU_FUNC_PORT_M				0xf
 #define   QCA8K_ATU_FUNC_PORT_S				8
+#define QCA8K_REG_VTU_FUNC0				0x610
+#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
+#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
+#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
+#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
+#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
+#define QCA8K_REG_VTU_FUNC1				0x614
+#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
+#define   QCA8K_VTU_FUNC1_VID_S				16
+#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
 #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
 #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
 #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
@@ -135,6 +149,11 @@
 #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
 #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
 #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
 #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
 #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
@@ -178,6 +197,15 @@ enum qca8k_fdb_cmd {
 	QCA8K_FDB_SEARCH = 7,
 };
 
+enum qca8k_vlan_cmd {
+	QCA8K_VLAN_FLUSH = 1,
+	QCA8K_VLAN_LOAD = 2,
+	QCA8K_VLAN_PURGE = 3,
+	QCA8K_VLAN_REMOVE_PORT = 4,
+	QCA8K_VLAN_NEXT = 5,
+	QCA8K_VLAN_READ = 6,
+};
+
 struct ar8xxx_port_status {
 	int enabled;
 };

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Florian Fainelli]

On 7/21/20 10:16 AM, Jonathan McDowell wrote:
> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.

This looks good to me at first glance, at least not seeing obvious
issue, however below are a few questions:

- vid == 0 appears to be unsupported according to your port_vlan_prepare
callback, is this really the case, or is it more a case of VID 0 should
be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
attempt to program

- since we have a qca8k_port_bridge_join() callback which programs the
port lookup control register, putting all ports by default in VID==1
does not break per-port isolation between non-bridged and bridged ports,
right?

- since you program the ports with a default VLAN ID upon startup of the
switch driver should not you also set
dsa_switch::configure_vlan_while_not_filtering to indicate to the DSA
layer that there is a VLAN table programmed regardless of VLAN filtering
being enabled in the bridge or not?

See some nitpicks below:

> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a5566de82853..cce05493075f 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -408,6 +408,104 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> +{
> +	u32 reg;
> +
> +	/* Set the command and VLAN index */
> +	reg = QCA8K_VTU_FUNC1_BUSY;> +	reg |= cmd;
> +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> +
> +	/* Write the function register triggering the table access */
> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> +
> +	/* wait for completion */
> +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> +		return -1;

Can you propagate the return value from qca8k_busy_wait() directly?

> +
> +	/* Check for table full violation when adding an entry */
> +	if (cmd == QCA8K_VLAN_LOAD) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> +		if (reg & QCA8K_VTU_FUNC1_FULL)
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	if (!vid)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret >= 0) {

Do an early return upon negative error code instead of reidenting the
code block?

> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +		reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> +		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +		if (tagged)
> +			reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +		else
> +			reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +	}
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
> +
> +static int
> +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> +{
> +	u32 reg;
> +	u32 mask;
> +	int ret;
> +	int i;
> +	bool del;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret >= 0) {

Likewise
-- 
Florian

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

On Tue, Jul 21, 2020 at 10:26:07AM -0700, Florian Fainelli wrote:
> On 7/21/20 10:16 AM, Jonathan McDowell wrote:
> > This adds full 802.1q VLAN support to the qca8k, allowing the use of
> > vlan_filtering and more complicated bridging setups than allowed by
> > basic port VLAN support.
> > 
> > Tested with a number of untagged ports with separate VLANs and then a
> > trunk port with all the VLANs tagged on it.
> 
> This looks good to me at first glance, at least not seeing obvious
> issue, however below are a few questions:

Thanks for the comments.

> - vid == 0 appears to be unsupported according to your port_vlan_prepare
> callback, is this really the case, or is it more a case of VID 0 should
> be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
> attempt to program

I don't quite follow you here. VID 0 doesn't appear to be supported by
the hardware (and several other DSA drivers don't seem to like it
either), hence the check; I'm not clear if there's something alternate I
should be doing in that case instead?

> - since we have a qca8k_port_bridge_join() callback which programs the
> port lookup control register, putting all ports by default in VID==1
> does not break per-port isolation between non-bridged and bridged ports,
> right?

VLAN_MODE_NONE (set when we don't have VLAN filtering enabled)
configures us for port filtering, ignoring the VLAN info, so I think
we're good here.

> - since you program the ports with a default VLAN ID upon startup of the
> switch driver should not you also set
> dsa_switch::configure_vlan_while_not_filtering to indicate to the DSA
> layer that there is a VLAN table programmed regardless of VLAN filtering
> being enabled in the bridge or not?

Yup, this should be set. I'd seen the vlan_bridge_vtu patch from
December but not noticed it had been renamed, just assumed it hadn't
been applied.

> See some nitpicks below:
> 
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a5566de82853..cce05493075f 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -408,6 +408,104 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
> >  	mutex_unlock(&priv->reg_mutex);
> >  }
> >  
> > +static int
> > +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> > +{
> > +	u32 reg;
> > +
> > +	/* Set the command and VLAN index */
> > +	reg = QCA8K_VTU_FUNC1_BUSY;> +	reg |= cmd;
> > +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> > +
> > +	/* Write the function register triggering the table access */
> > +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> > +
> > +	/* wait for completion */
> > +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> > +		return -1;
> 
> Can you propagate the return value from qca8k_busy_wait() directly?

It just returns a boolean. rmk makes a valid point about a sensible
errno instead, so I'll switch to ETIMEDOUT in v2 (and ENOMEM when the
VLAN table is full below).

> > +
> > +	/* Check for table full violation when adding an entry */
> > +	if (cmd == QCA8K_VLAN_LOAD) {
> > +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> > +		if (reg & QCA8K_VTU_FUNC1_FULL)
> > +			return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
> > +{
> > +	u32 reg;
> > +	int ret;
> > +
> > +	if (!vid)
> > +		return -EOPNOTSUPP;
> > +
> > +	mutex_lock(&priv->reg_mutex);
> > +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> > +	if (ret >= 0) {
> 
> Do an early return upon negative error code instead of reidenting the
> code block?

I'll switch over to a goto for cleanup (unlocking the mutex) to avoid
the extra indentation (and below).

> > +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> > +		reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> > +		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> > +		if (tagged)
> > +			reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> > +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> > +		else
> > +			reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> > +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> > +
> > +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> > +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> > +	}
> > +	mutex_unlock(&priv->reg_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> > +{
> > +	u32 reg;
> > +	u32 mask;
> > +	int ret;
> > +	int i;
> > +	bool del;
> > +
> > +	mutex_lock(&priv->reg_mutex);
> > +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> > +	if (ret >= 0) {
> 
> Likewise

J.

-- 
"Editing plain text configuration files has never been the Unix way,
and vi certainly isn't a standard unix tool." - mjg59, QOOC.

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Florian Fainelli]

On 7/22/20 12:38 PM, Jonathan McDowell wrote:
> On Tue, Jul 21, 2020 at 10:26:07AM -0700, Florian Fainelli wrote:
>> On 7/21/20 10:16 AM, Jonathan McDowell wrote:
>>> This adds full 802.1q VLAN support to the qca8k, allowing the use of
>>> vlan_filtering and more complicated bridging setups than allowed by
>>> basic port VLAN support.
>>>
>>> Tested with a number of untagged ports with separate VLANs and then a
>>> trunk port with all the VLANs tagged on it.
>>
>> This looks good to me at first glance, at least not seeing obvious
>> issue, however below are a few questions:
> 
> Thanks for the comments.
> 
>> - vid == 0 appears to be unsupported according to your port_vlan_prepare
>> callback, is this really the case, or is it more a case of VID 0 should
>> be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
>> attempt to program
> 
> I don't quite follow you here. VID 0 doesn't appear to be supported by
> the hardware (and several other DSA drivers don't seem to like it
> either), hence the check; I'm not clear if there's something alternate I
> should be doing in that case instead?

Is it really that the hardware does not support it, or is it that it was
not tried or poorly handled before? If the switch supports programming
the VID 0 entry as PVID egress untagged, which is what
dsa_slave_vlan_rx_add_vid() requests, then this is great, because you
have almost nothing to do.

If not, what you are doing is definitively okay, because you have a
port_bridge_join that ensures that the port matrix gets configured
correctly for bridging, if that was not supported we would be in trouble.

> 
>> - since we have a qca8k_port_bridge_join() callback which programs the
>> port lookup control register, putting all ports by default in VID==1
>> does not break per-port isolation between non-bridged and bridged ports,
>> right?
> 
> VLAN_MODE_NONE (set when we don't have VLAN filtering enabled)
> configures us for port filtering, ignoring the VLAN info, so I think
> we're good here.

OK, good, so just to be sure, there is no cross talk between non-bridged
ports and bridged ports even when VLAN filtering is not enabled on the
bridge, right?
-- 
Florian

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Vladimir Oltean]

On Wed, Jul 22, 2020 at 03:36:38PM -0700, Florian Fainelli wrote:
> On 7/22/20 12:38 PM, Jonathan McDowell wrote:
> > On Tue, Jul 21, 2020 at 10:26:07AM -0700, Florian Fainelli wrote:
> >> On 7/21/20 10:16 AM, Jonathan McDowell wrote:
> >>> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> >>> vlan_filtering and more complicated bridging setups than allowed by
> >>> basic port VLAN support.
> >>>
> >>> Tested with a number of untagged ports with separate VLANs and then a
> >>> trunk port with all the VLANs tagged on it.
> >>
> >> This looks good to me at first glance, at least not seeing obvious
> >> issue, however below are a few questions:
> > 
> > Thanks for the comments.
> > 
> >> - vid == 0 appears to be unsupported according to your port_vlan_prepare
> >> callback, is this really the case, or is it more a case of VID 0 should
> >> be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
> >> attempt to program
> > 
> > I don't quite follow you here. VID 0 doesn't appear to be supported by
> > the hardware (and several other DSA drivers don't seem to like it
> > either), hence the check; I'm not clear if there's something alternate I
> > should be doing in that case instead?
> 
> Is it really that the hardware does not support it, or is it that it was
> not tried or poorly handled before? If the switch supports programming
> the VID 0 entry as PVID egress untagged, which is what
> dsa_slave_vlan_rx_add_vid() requests, then this is great, because you
> have almost nothing to do.
> 
> If not, what you are doing is definitively okay, because you have a
> port_bridge_join that ensures that the port matrix gets configured
> correctly for bridging, if that was not supported we would be in trouble.

Things added by dsa_slave_vlan_rx_add_vid() are definitely not "pvid
untagged", they are installed with flags=0 which means "non-pvid,
egress-tagged", aka a simple vlan with tagged ingress and egress.

The case of VLAN 0 is special because according to 802.1Q it is "not a
VLAN", but simply a way to transmit the other stuff in a VLAN header,
namely a class of service (VLAN PCP). The VLAN ID should not be used for
segregation of forwarding domains, should not be assigned as port-based
VLAN to untagged traffic (from what I recall from the 802.1Q standard)
and should always be sent as egress-tagged.

The purpose of the code in the 8021q module that is adding VLAN 0 to our
RX filter is clear:

commit ad1afb00393915a51c21b1ae8704562bf036855f
Author: Pedro Garcia <pedro.netdev@dondevamos.com>
Date:   Sun Jul 18 15:38:44 2010 -0700

    vlan_dev: VLAN 0 should be treated as "no vlan tag" (802.1p packet)

    - Without the 8021q module loaded in the kernel, all 802.1p packets
    (VLAN 0 but QoS tagging) are silently discarded (as expected, as
    the protocol is not loaded).

    - Without this patch in 8021q module, these packets are forwarded to
    the module, but they are discarded also if VLAN 0 is not configured,
    which should not be the default behaviour, as VLAN 0 is not really
    a VLANed packet but a 802.1p packet. Defining VLAN 0 makes it almost
    impossible to communicate with mixed 802.1p and non 802.1p devices on
    the same network due to arp table issues.

    - Changed logic to skip vlan specific code in vlan_skb_recv if VLAN
    is 0 and we have not defined a VLAN with ID 0, but we accept the
    packet with the encapsulated proto and pass it later to netif_rx.

    - In the vlan device event handler, added some logic to add VLAN 0
    to HW filter in devices that support it (this prevented any traffic
    in VLAN 0 to reach the stack in e1000e with HW filter under 2.6.35,
    and probably also with other HW filtered cards, so we fix it here).

    - In the vlan unregister logic, prevent the elimination of VLAN 0
    in devices with HW filter.

    - The default behaviour is to ignore the VLAN 0 tagging and accept
    the packet as if it was not tagged, but we can still define a
    VLAN 0 if desired (so it is backwards compatible).

    Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

So maybe it's worth checking what is your switch's behavior with regard
to VLAN 0. If it already does what's supposed to, then you might just as
well stop fighting the system and silently accept the configuration
while not doing anything.  As Russell implied, the bridge can't add a
VLAN of 0. It is just the 8021q module that does it.  The fact that we
have the same callbacks being used for both in DSA is merely an artefact
of implementation.

> 
> > 
> >> - since we have a qca8k_port_bridge_join() callback which programs the
> >> port lookup control register, putting all ports by default in VID==1
> >> does not break per-port isolation between non-bridged and bridged ports,
> >> right?
> > 
> > VLAN_MODE_NONE (set when we don't have VLAN filtering enabled)
> > configures us for port filtering, ignoring the VLAN info, so I think
> > we're good here.
> 
> OK, good, so just to be sure, there is no cross talk between non-bridged
> ports and bridged ports even when VLAN filtering is not enabled on the
> bridge, right?
> -- 
> Florian

Thanks,
-Vladimir

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

On Thu, Jul 23, 2020 at 01:58:47AM +0300, Vladimir Oltean wrote:
> On Wed, Jul 22, 2020 at 03:36:38PM -0700, Florian Fainelli wrote:
> > On 7/22/20 12:38 PM, Jonathan McDowell wrote:
> > > On Tue, Jul 21, 2020 at 10:26:07AM -0700, Florian Fainelli wrote:
> > >> On 7/21/20 10:16 AM, Jonathan McDowell wrote:
> > >>> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> > >>> vlan_filtering and more complicated bridging setups than allowed by
> > >>> basic port VLAN support.
> > >>>
> > >>> Tested with a number of untagged ports with separate VLANs and then a
> > >>> trunk port with all the VLANs tagged on it.
> > >>
> > >> This looks good to me at first glance, at least not seeing obvious
> > >> issue, however below are a few questions:
> > > 
> > > Thanks for the comments.
> > > 
> > >> - vid == 0 appears to be unsupported according to your port_vlan_prepare
> > >> callback, is this really the case, or is it more a case of VID 0 should
> > >> be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
> > >> attempt to program
> > > 
> > > I don't quite follow you here. VID 0 doesn't appear to be supported by
> > > the hardware (and several other DSA drivers don't seem to like it
> > > either), hence the check; I'm not clear if there's something alternate I
> > > should be doing in that case instead?
> > 
> > Is it really that the hardware does not support it, or is it that it was
> > not tried or poorly handled before? If the switch supports programming
> > the VID 0 entry as PVID egress untagged, which is what
> > dsa_slave_vlan_rx_add_vid() requests, then this is great, because you
> > have almost nothing to do.
> > 
> > If not, what you are doing is definitively okay, because you have a
> > port_bridge_join that ensures that the port matrix gets configured
> > correctly for bridging, if that was not supported we would be in trouble.
> 
> Things added by dsa_slave_vlan_rx_add_vid() are definitely not "pvid
> untagged", they are installed with flags=0 which means "non-pvid,
> egress-tagged", aka a simple vlan with tagged ingress and egress.
> 
> The case of VLAN 0 is special because according to 802.1Q it is "not a
> VLAN", but simply a way to transmit the other stuff in a VLAN header,
> namely a class of service (VLAN PCP). The VLAN ID should not be used for
> segregation of forwarding domains, should not be assigned as port-based
> VLAN to untagged traffic (from what I recall from the 802.1Q standard)
> and should always be sent as egress-tagged.
...
> So maybe it's worth checking what is your switch's behavior with regard
> to VLAN 0. If it already does what's supposed to, then you might just as
> well stop fighting the system and silently accept the configuration
> while not doing anything.  As Russell implied, the bridge can't add a
> VLAN of 0. It is just the 8021q module that does it.  The fact that we
> have the same callbacks being used for both in DSA is merely an artefact
> of implementation.

Ok, thanks for the clarification, that helps a lot.

I've done some experimentation injecting packets on untagged ports with
VLAN 0 headers. It looks like it's doing the right thing; the intact
VLAN 0 / classification comes through to the CPU port, and the packet is
also correctly sent out tagged with the correct VLAN (from the untagged
port configuration) on a trunk port. So I think I can just silently drop
the request for VLAN 0 configuration rather than returning an error.

> > >> - since we have a qca8k_port_bridge_join() callback which programs the
> > >> port lookup control register, putting all ports by default in VID==1
> > >> does not break per-port isolation between non-bridged and bridged ports,
> > >> right?
> > > 
> > > VLAN_MODE_NONE (set when we don't have VLAN filtering enabled)
> > > configures us for port filtering, ignoring the VLAN info, so I think
> > > we're good here.
> > 
> > OK, good, so just to be sure, there is no cross talk between non-bridged
> > ports and bridged ports even when VLAN filtering is not enabled on the
> > bridge, right?

Yup. When VLAN filtering is off off we only allow ports to talk to each
other that we get bridge_join calls for (that's the way the device is
currently supported by the kernel).

J.

-- 
/-\                             |  Be Ye Not Lost Among Precepts of
|@/  Debian GNU/Linux Developer |                Order
\-                              |

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Russell King - ARM Linux admin]

On Tue, Jul 21, 2020 at 06:16:24PM +0100, Jonathan McDowell wrote:
> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a5566de82853..cce05493075f 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -408,6 +408,104 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> +{
> +	u32 reg;
> +
> +	/* Set the command and VLAN index */
> +	reg = QCA8K_VTU_FUNC1_BUSY;
> +	reg |= cmd;
> +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> +
> +	/* Write the function register triggering the table access */
> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> +
> +	/* wait for completion */
> +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> +		return -1;

You return -1 here.  Personally, I don't like this in the kernel, as
convention is for functions returning "int" to return negative errno
values, and this risks returning -1 (-EPERM) being returned to userspace
if someone decides to propagate the "error code".

> +
> +	/* Check for table full violation when adding an entry */
> +	if (cmd == QCA8K_VLAN_LOAD) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> +		if (reg & QCA8K_VTU_FUNC1_FULL)
> +			return -1;

... and here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	if (!vid)
> +		return -EOPNOTSUPP;

Have you checked whether this can be called with vid=0 ?

> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret >= 0) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +		reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> +		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +		if (tagged)
> +			reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +		else
> +			reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> +					QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +	}
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;

That return -1 can get propagated out this function - a function that
also returns an -ve errno value (-EOPNOTSUPP).

> +}
> +
> +static int
> +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> +{
> +	u32 reg;
> +	u32 mask;
> +	int ret;
> +	int i;
> +	bool del;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret >= 0) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +		reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +		reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
> +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +		/* Check if we're the last member to be removed */
> +		del = true;
> +		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +			mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
> +			mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
> +
> +			if ((reg & mask) != mask) {
> +				del = false;
> +				break;
> +			}
> +		}
> +
> +		if (del) {
> +			ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
> +		} else {
> +			qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +			ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +		}
> +	}
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;

Since qca8k_vlan_access() can return -1, so too can this function, and
the knowledge that this isn't an errno value is now two functions
deep...

> +}
> +
>  static void
>  qca8k_mib_init(struct qca8k_priv *priv)
>  {
> @@ -663,10 +761,11 @@ qca8k_setup(struct dsa_switch *ds)
>  			 * default egress vid
>  			 */
>  			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
> -				  0xffff << shift, 1 << shift);
> +				  0xffff << shift,
> +				  QCA8K_PORT_VID_DEF << shift);
>  			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
> -				    QCA8K_PORT_VLAN_CVID(1) |
> -				    QCA8K_PORT_VLAN_SVID(1));
> +				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
> +				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
>  		}
>  	}
>  
> @@ -1133,7 +1232,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  {
>  	/* Set the vid to the port vlan id if no vid is set */
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;
>  
>  	return qca8k_fdb_add(priv, addr, port_mask, vid,
>  			     QCA8K_ATU_STATUS_STATIC);
> @@ -1157,7 +1256,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
>  	u16 port_mask = BIT(port);
>  
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;
>  
>  	return qca8k_fdb_del(priv, addr, port_mask, vid);
>  }
> @@ -1186,6 +1285,71 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static int
> +qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	if (vlan_filtering) {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
> +	} else {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
> +			const struct switchdev_obj_port_vlan *vlan)
> +{
> +	if (!vlan->vid_begin)
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +static void
> +qca8k_port_vlan_add(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	u16 vid;
> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
> +		qca8k_vlan_add(priv, port, vid, !untagged);

The and ignored here, so is there any point qca8k_vlan_add() returning
an error?  If it fails, we'll never know... there even seems to be no
diagnostic gets logged in the kernel message log.

If you decide to add some logging, be careful how you do it - userspace
could ask for vids 1..4095 to be added, and we wouldn't want the
possibility of 4094 error messages.

Another issue may be the time taken to process 4094 VIDs if
qca8k_busy_wait() has to wait for every one.

> +
> +	if (pvid) {
> +		int shift = 16 * (port % 2);
> +
> +		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
> +			  0xffff << shift,
> +			  vlan->vid_end << shift);
> +		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
> +			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
> +			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
> +	}
> +}
> +
> +static int
> +qca8k_port_vlan_del(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	u16 vid;
> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
> +		qca8k_vlan_del(priv, port, vid);

Same here.

Thanks.

> +
> +	return 0;
> +}
> +
>  static enum dsa_tag_protocol
>  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
>  		       enum dsa_tag_protocol mp)
> @@ -1211,6 +1375,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_fdb_add		= qca8k_port_fdb_add,
>  	.port_fdb_del		= qca8k_port_fdb_del,
>  	.port_fdb_dump		= qca8k_port_fdb_dump,
> +	.port_vlan_filtering	= qca8k_port_vlan_filtering,
> +	.port_vlan_prepare	= qca8k_port_vlan_prepare,
> +	.port_vlan_add		= qca8k_port_vlan_add,
> +	.port_vlan_del		= qca8k_port_vlan_del,
>  	.phylink_validate	= qca8k_phylink_validate,
>  	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
>  	.phylink_mac_config	= qca8k_phylink_mac_config,
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 31439396401c..4e96275cbc3e 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -22,6 +22,8 @@
>  
>  #define QCA8K_CPU_PORT					0
>  
> +#define QCA8K_PORT_VID_DEF				1
> +
>  /* Global control registers */
>  #define QCA8K_REG_MASK_CTRL				0x000
>  #define   QCA8K_MASK_CTRL_ID_M				0xff
> @@ -126,6 +128,18 @@
>  #define   QCA8K_ATU_FUNC_FULL				BIT(12)
>  #define   QCA8K_ATU_FUNC_PORT_M				0xf
>  #define   QCA8K_ATU_FUNC_PORT_S				8
> +#define QCA8K_REG_VTU_FUNC0				0x610
> +#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
> +#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
> +#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
> +#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
> +#define QCA8K_REG_VTU_FUNC1				0x614
> +#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
> +#define   QCA8K_VTU_FUNC1_VID_S				16
> +#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
>  #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
>  #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
>  #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
> @@ -135,6 +149,11 @@
>  #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
>  #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
>  #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
>  #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
> @@ -178,6 +197,15 @@ enum qca8k_fdb_cmd {
>  	QCA8K_FDB_SEARCH = 7,
>  };
>  
> +enum qca8k_vlan_cmd {
> +	QCA8K_VLAN_FLUSH = 1,
> +	QCA8K_VLAN_LOAD = 2,
> +	QCA8K_VLAN_PURGE = 3,
> +	QCA8K_VLAN_REMOVE_PORT = 4,
> +	QCA8K_VLAN_NEXT = 5,
> +	QCA8K_VLAN_READ = 6,
> +};
> +
>  struct ar8xxx_port_status {
>  	int enabled;
>  };
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

On Tue, Jul 21, 2020 at 09:48:18PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jul 21, 2020 at 06:16:24PM +0100, Jonathan McDowell wrote:
> > This adds full 802.1q VLAN support to the qca8k, allowing the use of
> > vlan_filtering and more complicated bridging setups than allowed by
> > basic port VLAN support.
> > 
> > Tested with a number of untagged ports with separate VLANs and then a
> > trunk port with all the VLANs tagged on it.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a5566de82853..cce05493075f 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -408,6 +408,104 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
> >  	mutex_unlock(&priv->reg_mutex);
> >  }
> >  
> > +static int
> > +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> > +{
> > +	u32 reg;
> > +
> > +	/* Set the command and VLAN index */
> > +	reg = QCA8K_VTU_FUNC1_BUSY;
> > +	reg |= cmd;
> > +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> > +
> > +	/* Write the function register triggering the table access */
> > +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> > +
> > +	/* wait for completion */
> > +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> > +		return -1;
> 
> You return -1 here.  Personally, I don't like this in the kernel, as
> convention is for functions returning "int" to return negative errno
> values, and this risks returning -1 (-EPERM) being returned to userspace
> if someone decides to propagate the "error code".

Reasonable. I based this code off the qca8k_fdb_access code, but I'll
switch over to more sensible returns (and clean the fdb stuff up in a
separate patch).

> > +
> > +	/* Check for table full violation when adding an entry */
> > +	if (cmd == QCA8K_VLAN_LOAD) {
> > +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> > +		if (reg & QCA8K_VTU_FUNC1_FULL)
> > +			return -1;
> 
> ... and here.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
> > +{
> > +	u32 reg;
> > +	int ret;
> > +
> > +	if (!vid)
> > +		return -EOPNOTSUPP;
> 
> Have you checked whether this can be called with vid=0 ?

It's called at startup with VID 0 (part of setting up the HW filter
according to the log message?) and the hardware isn't happy with that.

...
> > +
> > +static int
> > +qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
> > +			const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +	if (!vlan->vid_begin)
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> > +
> > +static void
> > +qca8k_port_vlan_add(struct dsa_switch *ds, int port,
> > +		    const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > +	u16 vid;
> > +
> > +	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
> > +		qca8k_vlan_add(priv, port, vid, !untagged);
> 
> The and ignored here, so is there any point qca8k_vlan_add() returning
> an error?  If it fails, we'll never know... there even seems to be no
> diagnostic gets logged in the kernel message log.
> 
> If you decide to add some logging, be careful how you do it - userspace
> could ask for vids 1..4095 to be added, and we wouldn't want the
> possibility of 4094 error messages.
> 
> Another issue may be the time taken to process 4094 VIDs if
> qca8k_busy_wait() has to wait for every one.

I'll add a break out on error (and a dev_err) for this and the del case
in v2.

J.

-- 
       Hell is other people.       |  .''`.  Debian GNU/Linux Developer
                                   | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.

[PATCH net-next v2] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

This adds full 802.1q VLAN support to the qca8k, allowing the use of
vlan_filtering and more complicated bridging setups than allowed by
basic port VLAN support.

Tested with a number of untagged ports with separate VLANs and then a
trunk port with all the VLANs tagged on it.

v2:
- Return sensible errnos on failure rather than -1 (rmk)
- Style cleanups based on Florian's feedback
- Silently allow VLAN 0 as device correctly treats this as no tag

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/qca8k.h |  28 ++++++
 2 files changed, 214 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a5566de82853..1cc61bc8929f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -408,6 +408,111 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
+static int
+qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
+{
+	u32 reg;
+
+	/* Set the command and VLAN index */
+	reg = QCA8K_VTU_FUNC1_BUSY;
+	reg |= cmd;
+	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
+
+	/* Write the function register triggering the table access */
+	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+
+	/* wait for completion */
+	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
+		return -ETIMEDOUT;
+
+	/* Check for table full violation when adding an entry */
+	if (cmd == QCA8K_VLAN_LOAD) {
+		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
+		if (reg & QCA8K_VTU_FUNC1_FULL)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
+{
+	u32 reg;
+	int ret;
+
+	/* We do the right thing with VLAN 0 and treat it as untagged */
+	if (vid == 0)
+		return 0;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
+	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+	if (tagged)
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
+				QCA8K_VTU_FUNC0_EG_MODE_S(port);
+	else
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
+				QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
+{
+	u32 reg;
+	u32 mask;
+	int ret;
+	int i;
+	bool del;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
+			QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+	/* Check if we're the last member to be removed */
+	del = true;
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
+		mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
+
+		if ((reg & mask) != mask) {
+			del = false;
+			break;
+		}
+	}
+
+	if (del) {
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
+	} else {
+		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+	}
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
 static void
 qca8k_mib_init(struct qca8k_priv *priv)
 {
@@ -663,10 +768,11 @@ qca8k_setup(struct dsa_switch *ds)
 			 * default egress vid
 			 */
 			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-				  0xffff << shift, 1 << shift);
+				  0xffff << shift,
+				  QCA8K_PORT_VID_DEF << shift);
 			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
-				    QCA8K_PORT_VLAN_CVID(1) |
-				    QCA8K_PORT_VLAN_SVID(1));
+				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
+				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
 		}
 	}
 
@@ -1133,7 +1239,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 {
 	/* Set the vid to the port vlan id if no vid is set */
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_add(priv, addr, port_mask, vid,
 			     QCA8K_ATU_STATUS_STATIC);
@@ -1157,7 +1263,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 	u16 port_mask = BIT(port);
 
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_del(priv, addr, port_mask, vid);
 }
@@ -1186,6 +1292,76 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int
+qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	if (vlan_filtering) {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+	} else {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+	}
+
+	return 0;
+}
+
+static int
+qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_vlan *vlan)
+{
+	return 0;
+}
+
+static void
+qca8k_port_vlan_add(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	struct qca8k_priv *priv = ds->priv;
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	u16 vid;
+	int ret = 0;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
+		ret = qca8k_vlan_add(priv, port, vid, !untagged);
+
+	if (ret)
+		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
+
+	if (pvid) {
+		int shift = 16 * (port % 2);
+
+		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
+			  0xffff << shift,
+			  vlan->vid_end << shift);
+		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
+			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
+			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
+	}
+}
+
+static int
+qca8k_port_vlan_del(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u16 vid;
+	int ret = 0;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
+		ret = qca8k_vlan_del(priv, port, vid);
+
+	if (ret)
+		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);
+
+	return ret;
+}
+
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 		       enum dsa_tag_protocol mp)
@@ -1211,6 +1387,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.port_vlan_filtering	= qca8k_port_vlan_filtering,
+	.port_vlan_prepare	= qca8k_port_vlan_prepare,
+	.port_vlan_add		= qca8k_port_vlan_add,
+	.port_vlan_del		= qca8k_port_vlan_del,
 	.phylink_validate	= qca8k_phylink_validate,
 	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
 	.phylink_mac_config	= qca8k_phylink_mac_config,
@@ -1261,6 +1441,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
+	priv->ds->configure_vlan_while_not_filtering = true;
 	priv->ds->priv = priv;
 	priv->ops = qca8k_switch_ops;
 	priv->ds->ops = &priv->ops;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 31439396401c..4e96275cbc3e 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -22,6 +22,8 @@
 
 #define QCA8K_CPU_PORT					0
 
+#define QCA8K_PORT_VID_DEF				1
+
 /* Global control registers */
 #define QCA8K_REG_MASK_CTRL				0x000
 #define   QCA8K_MASK_CTRL_ID_M				0xff
@@ -126,6 +128,18 @@
 #define   QCA8K_ATU_FUNC_FULL				BIT(12)
 #define   QCA8K_ATU_FUNC_PORT_M				0xf
 #define   QCA8K_ATU_FUNC_PORT_S				8
+#define QCA8K_REG_VTU_FUNC0				0x610
+#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
+#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
+#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
+#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
+#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
+#define QCA8K_REG_VTU_FUNC1				0x614
+#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
+#define   QCA8K_VTU_FUNC1_VID_S				16
+#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
 #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
 #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
 #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
@@ -135,6 +149,11 @@
 #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
 #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
 #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
 #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
 #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
@@ -178,6 +197,15 @@ enum qca8k_fdb_cmd {
 	QCA8K_FDB_SEARCH = 7,
 };
 
+enum qca8k_vlan_cmd {
+	QCA8K_VLAN_FLUSH = 1,
+	QCA8K_VLAN_LOAD = 2,
+	QCA8K_VLAN_PURGE = 3,
+	QCA8K_VLAN_REMOVE_PORT = 4,
+	QCA8K_VLAN_NEXT = 5,
+	QCA8K_VLAN_READ = 6,
+};
+
 struct ar8xxx_port_status {
 	int enabled;
 };
-- 
2.20.1

Re: [PATCH net-next v2] net: dsa: qca8k: Add 802.1q VLAN support

[Vladimir Oltean]

Hi Jonathan,

On Sun, Jul 26, 2020 at 03:56:11PM +0100, Jonathan McDowell wrote:
> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.
> 
> v2:
> - Return sensible errnos on failure rather than -1 (rmk)
> - Style cleanups based on Florian's feedback
> - Silently allow VLAN 0 as device correctly treats this as no tag
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---

This generally looks ok. The integration with the APIs is fine.
Some comments below.

>  drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h |  28 ++++++
>  2 files changed, 214 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a5566de82853..1cc61bc8929f 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -408,6 +408,111 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> +{
> +	u32 reg;
> +
> +	/* Set the command and VLAN index */
> +	reg = QCA8K_VTU_FUNC1_BUSY;
> +	reg |= cmd;
> +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> +
> +	/* Write the function register triggering the table access */
> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> +
> +	/* wait for completion */
> +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> +		return -ETIMEDOUT;
> +
> +	/* Check for table full violation when adding an entry */
> +	if (cmd == QCA8K_VLAN_LOAD) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> +		if (reg & QCA8K_VTU_FUNC1_FULL)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)

It is customary to keep referring to this bool as 'untagged' for
consistency with many other parts of the kernel.

> +{
> +	u32 reg;
> +	int ret;
> +
> +	/* We do the right thing with VLAN 0 and treat it as untagged */

...while also preserving the tag on egress.

> +	if (vid == 0)
> +		return 0;
> +
> +	mutex_lock(&priv->reg_mutex);

Unrelated, but what's the purpose of this mutex?

> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret < 0)
> +		goto out;
> +
> +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> +	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +	if (tagged)
> +		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +	else
> +		reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +

Not thrilled about the "3 <<" thing, maybe a definition like the one
below would look better:

#define QCA8K_VTU_FUNC_REG0_EG_VLAN_MODE_MASK(port) \
	GENMASK(5 + (port) * 2, 4 + (port) * 2)

...

	int eg_vlan_mode = QCA8K_VTU_FUNC_REG0_EG_MODE_TAG;

	reg &= ~QCA8K_VTU_FUNC_REG0_EG_VLAN_MODE_MASK(port);
	if (tagged)
		eg_vlan_mode = QCA8K_VTU_FUNC_REG0_EG_MODE_UNTAG;
	reg |= QCA8K_VTU_FUNC_REG0_EG_MODE(eg_vlan_mode, port);

Your call if you want to change this, though.

> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +
> +out:
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
> +
> +static int
> +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> +{
> +	u32 reg;
> +	u32 mask;
> +	int ret;
> +	int i;
> +	bool del;

How about:

	u32 reg, mask;
	int ret, i;
	bool del;

> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret < 0)
> +		goto out;
> +
> +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
> +			QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +	/* Check if we're the last member to be removed */
> +	del = true;
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
> +		mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
> +
> +		if ((reg & mask) != mask) {
> +			del = false;
> +			break;
> +		}
> +	}
> +
> +	if (del) {
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
> +	} else {
> +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +	}
> +
> +out:
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
> +
>  static void
>  qca8k_mib_init(struct qca8k_priv *priv)
>  {
> @@ -663,10 +768,11 @@ qca8k_setup(struct dsa_switch *ds)
>  			 * default egress vid
>  			 */
>  			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
> -				  0xffff << shift, 1 << shift);
> +				  0xffff << shift,
> +				  QCA8K_PORT_VID_DEF << shift);

This has telltale signs of copy-pasted code. ROUTER_DEFAULT_VID is a
12-bit register, so 0xffff is probably not the right mask. But, it is
true that the upper 4 bits are reserved, so it isn't quite a bug to
zero them out, just something that sticks out as not correct.

>  			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
> -				    QCA8K_PORT_VLAN_CVID(1) |
> -				    QCA8K_PORT_VLAN_SVID(1));
> +				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
> +				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
>  		}
>  	}
>  
> @@ -1133,7 +1239,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  {
>  	/* Set the vid to the port vlan id if no vid is set */
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;
>  
>  	return qca8k_fdb_add(priv, addr, port_mask, vid,
>  			     QCA8K_ATU_STATUS_STATIC);
> @@ -1157,7 +1263,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
>  	u16 port_mask = BIT(port);
>  
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;

Maybe you could split out this s/1/QCA8K_PORT_VID_DEF/g patch into a
separate one? For the purpose of the introduction of VLAN callbacks,
it's just noise.

>  
>  	return qca8k_fdb_del(priv, addr, port_mask, vid);
>  }
> @@ -1186,6 +1292,76 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static int
> +qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	if (vlan_filtering) {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
> +	} else {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
> +			const struct switchdev_obj_port_vlan *vlan)
> +{
> +	return 0;
> +}
> +
> +static void
> +qca8k_port_vlan_add(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct qca8k_priv *priv = ds->priv;

Reverse Christmas notation please.

> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	u16 vid;
> +	int ret = 0;

here too.

> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> +		ret = qca8k_vlan_add(priv, port, vid, !untagged);
> +
> +	if (ret)
> +		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
> +

If for some reason there is a temporary failure in qca8k_vlan_add, you'd
be swallowing it instead of printing the error and stopping the
execution.

> +	if (pvid) {
> +		int shift = 16 * (port % 2);
> +
> +		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),

What's up with this name? Why not "ROUTER_DEFAULT_VID" which is how the
hardware calls it? I had some trouble finding it.

> +			  0xffff << shift,
> +			  vlan->vid_end << shift);
> +		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
> +			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
> +			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
> +	}
> +}
> +
> +static int
> +qca8k_port_vlan_del(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	u16 vid;
> +	int ret = 0;

Reverse Christmas notation please.

> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> +		ret = qca8k_vlan_del(priv, port, vid);
> +
> +	if (ret)
> +		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);

Same comment, could you move the "if" inside the "for"?

> +
> +	return ret;
> +}
> +
>  static enum dsa_tag_protocol
>  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
>  		       enum dsa_tag_protocol mp)
> @@ -1211,6 +1387,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_fdb_add		= qca8k_port_fdb_add,
>  	.port_fdb_del		= qca8k_port_fdb_del,
>  	.port_fdb_dump		= qca8k_port_fdb_dump,
> +	.port_vlan_filtering	= qca8k_port_vlan_filtering,
> +	.port_vlan_prepare	= qca8k_port_vlan_prepare,
> +	.port_vlan_add		= qca8k_port_vlan_add,
> +	.port_vlan_del		= qca8k_port_vlan_del,
>  	.phylink_validate	= qca8k_phylink_validate,
>  	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
>  	.phylink_mac_config	= qca8k_phylink_mac_config,
> @@ -1261,6 +1441,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  
>  	priv->ds->dev = &mdiodev->dev;
>  	priv->ds->num_ports = QCA8K_NUM_PORTS;
> +	priv->ds->configure_vlan_while_not_filtering = true;

Nice that you've enabled this. Thanks.

>  	priv->ds->priv = priv;
>  	priv->ops = qca8k_switch_ops;
>  	priv->ds->ops = &priv->ops;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 31439396401c..4e96275cbc3e 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -22,6 +22,8 @@
>  
>  #define QCA8K_CPU_PORT					0
>  
> +#define QCA8K_PORT_VID_DEF				1
> +
>  /* Global control registers */
>  #define QCA8K_REG_MASK_CTRL				0x000
>  #define   QCA8K_MASK_CTRL_ID_M				0xff
> @@ -126,6 +128,18 @@
>  #define   QCA8K_ATU_FUNC_FULL				BIT(12)
>  #define   QCA8K_ATU_FUNC_PORT_M				0xf
>  #define   QCA8K_ATU_FUNC_PORT_S				8
> +#define QCA8K_REG_VTU_FUNC0				0x610
> +#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
> +#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
> +#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
> +#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
> +#define QCA8K_REG_VTU_FUNC1				0x614
> +#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
> +#define   QCA8K_VTU_FUNC1_VID_S				16
> +#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
>  #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
>  #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
>  #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
> @@ -135,6 +149,11 @@
>  #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
>  #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
>  #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
>  #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
> @@ -178,6 +197,15 @@ enum qca8k_fdb_cmd {
>  	QCA8K_FDB_SEARCH = 7,
>  };
>  
> +enum qca8k_vlan_cmd {
> +	QCA8K_VLAN_FLUSH = 1,
> +	QCA8K_VLAN_LOAD = 2,
> +	QCA8K_VLAN_PURGE = 3,
> +	QCA8K_VLAN_REMOVE_PORT = 4,
> +	QCA8K_VLAN_NEXT = 5,
> +	QCA8K_VLAN_READ = 6,
> +};
> +
>  struct ar8xxx_port_status {
>  	int enabled;
>  };
> -- 
> 2.20.1

Thanks,
-Vladimir

Re: [PATCH net-next v2] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

On Tue, Jul 28, 2020 at 07:34:57PM +0300, Vladimir Oltean wrote:
> Hi Jonathan,
> 
> On Sun, Jul 26, 2020 at 03:56:11PM +0100, Jonathan McDowell wrote:
> > This adds full 802.1q VLAN support to the qca8k, allowing the use of
> > vlan_filtering and more complicated bridging setups than allowed by
> > basic port VLAN support.
> > 
> > Tested with a number of untagged ports with separate VLANs and then a
> > trunk port with all the VLANs tagged on it.
> > 
> > v2:
> > - Return sensible errnos on failure rather than -1 (rmk)
> > - Style cleanups based on Florian's feedback
> > - Silently allow VLAN 0 as device correctly treats this as no tag
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> 
> This generally looks ok. The integration with the APIs is fine.
> Some comments below.
> 
> >  drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++--
> >  drivers/net/dsa/qca8k.h |  28 ++++++
> >  2 files changed, 214 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a5566de82853..1cc61bc8929f 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -408,6 +408,111 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
> >  	mutex_unlock(&priv->reg_mutex);
> >  }
> >  
> > +static int
> > +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> > +{
> > +	u32 reg;
> > +
> > +	/* Set the command and VLAN index */
> > +	reg = QCA8K_VTU_FUNC1_BUSY;
> > +	reg |= cmd;
> > +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> > +
> > +	/* Write the function register triggering the table access */
> > +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> > +
> > +	/* wait for completion */
> > +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> > +		return -ETIMEDOUT;
> > +
> > +	/* Check for table full violation when adding an entry */
> > +	if (cmd == QCA8K_VLAN_LOAD) {
> > +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> > +		if (reg & QCA8K_VTU_FUNC1_FULL)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool tagged)
> 
> It is customary to keep referring to this bool as 'untagged' for
> consistency with many other parts of the kernel.

Ok, changed.

> > +{
> > +	u32 reg;
> > +	int ret;
> > +
> > +	/* We do the right thing with VLAN 0 and treat it as untagged */
> 
> ...while also preserving the tag on egress.
> 
> > +	if (vid == 0)
> > +		return 0;
> > +
> > +	mutex_lock(&priv->reg_mutex);
> 
> Unrelated, but what's the purpose of this mutex?

The access to the VLAN configuration is a set of writes to multiple
registers, so the mutex is to avoid trying to do 2 updates at the same
time. Same principle as is applied for the existing FDB accesses.

> > +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> > +	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> > +	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> > +	if (tagged)
> > +		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> > +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> > +	else
> > +		reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> > +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> > +
> 
> Not thrilled about the "3 <<" thing, maybe a definition like the one
> below would look better:
> 
> #define QCA8K_VTU_FUNC_REG0_EG_VLAN_MODE_MASK(port) \
> 	GENMASK(5 + (port) * 2, 4 + (port) * 2)
> 
> ...
> 
> 	int eg_vlan_mode = QCA8K_VTU_FUNC_REG0_EG_MODE_TAG;
> 
> 	reg &= ~QCA8K_VTU_FUNC_REG0_EG_VLAN_MODE_MASK(port);
> 	if (tagged)
> 		eg_vlan_mode = QCA8K_VTU_FUNC_REG0_EG_MODE_UNTAG;
> 	reg |= QCA8K_VTU_FUNC_REG0_EG_MODE(eg_vlan_mode, port);
> 
> Your call if you want to change this, though.

I've added QCA8K_VTU_FUNC_REG0_EG_MODE_MASK instead of using the hard
coded 3, I think it's clearer when the mask + the values are both
getting the shift in the same manner.

> > +	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> > +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> > +
> > +out:
> > +	mutex_unlock(&priv->reg_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> > +{
> > +	u32 reg;
> > +	u32 mask;
> > +	int ret;
> > +	int i;
> > +	bool del;
> 
> How about:
> 
> 	u32 reg, mask;
> 	int ret, i;
> 	bool del;

Ok.

> > +
> > +	mutex_lock(&priv->reg_mutex);
> > +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> > +	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> > +	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
> > +			QCA8K_VTU_FUNC0_EG_MODE_S(port);
> > +
> > +	/* Check if we're the last member to be removed */
> > +	del = true;
> > +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > +		mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
> > +		mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
> > +
> > +		if ((reg & mask) != mask) {
> > +			del = false;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (del) {
> > +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
> > +	} else {
> > +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> > +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&priv->reg_mutex);
> > +
> > +	return ret;
> > +}
> > +
> >  static void
> >  qca8k_mib_init(struct qca8k_priv *priv)
> >  {
> > @@ -663,10 +768,11 @@ qca8k_setup(struct dsa_switch *ds)
> >  			 * default egress vid
> >  			 */
> >  			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
> > -				  0xffff << shift, 1 << shift);
> > +				  0xffff << shift,
> > +				  QCA8K_PORT_VID_DEF << shift);
> 
> This has telltale signs of copy-pasted code. ROUTER_DEFAULT_VID is a
> 12-bit register, so 0xffff is probably not the right mask. But, it is
> true that the upper 4 bits are reserved, so it isn't quite a bug to
> zero them out, just something that sticks out as not correct.

Not my code originally, can fix up.

> >  			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
> > -				    QCA8K_PORT_VLAN_CVID(1) |
> > -				    QCA8K_PORT_VLAN_SVID(1));
> > +				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
> > +				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
> >  		}
> >  	}
> >  
> > @@ -1133,7 +1239,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
> >  {
> >  	/* Set the vid to the port vlan id if no vid is set */
> >  	if (!vid)
> > -		vid = 1;
> > +		vid = QCA8K_PORT_VID_DEF;
> >  
> >  	return qca8k_fdb_add(priv, addr, port_mask, vid,
> >  			     QCA8K_ATU_STATUS_STATIC);
> > @@ -1157,7 +1263,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
> >  	u16 port_mask = BIT(port);
> >  
> >  	if (!vid)
> > -		vid = 1;
> > +		vid = QCA8K_PORT_VID_DEF;
> 
> Maybe you could split out this s/1/QCA8K_PORT_VID_DEF/g patch into a
> separate one? For the purpose of the introduction of VLAN callbacks,
> it's just noise.

Ok.

> >  	return qca8k_fdb_del(priv, addr, port_mask, vid);
> >  }
> > @@ -1186,6 +1292,76 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
> >  	return 0;
> >  }
> >  
> > +static int
> > +qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +
> > +	if (vlan_filtering) {
> > +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> > +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> > +			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
> > +	} else {
> > +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> > +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> > +			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
> > +			const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void
> > +qca8k_port_vlan_add(struct dsa_switch *ds, int port,
> > +		    const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> 
> Reverse Christmas notation please.

Sure, fixed.

> > +
> > +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> > +		ret = qca8k_vlan_add(priv, port, vid, !untagged);
> > +
> > +	if (ret)
> > +		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
> > +
> 
> If for some reason there is a temporary failure in qca8k_vlan_add, you'd
> be swallowing it instead of printing the error and stopping the
> execution.

I don't follow; I'm breaking out of the for loop when we get an error? I
figured that was a better move than potentially printing 4095 error
messages if they were all going to fail.

> > +	if (pvid) {
> > +		int shift = 16 * (port % 2);
> > +
> > +		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
> 
> What's up with this name? Why not "ROUTER_DEFAULT_VID" which is how the
> hardware calls it? I had some trouble finding it.

Not my naming; it's how the driver already defined it.

J.

-- 
... Nice world. Let's make it weirder.

Re: [PATCH net-next v2] net: dsa: qca8k: Add 802.1q VLAN support

[Vladimir Oltean]

On Thu, Jul 30, 2020 at 11:40:29AM +0100, Jonathan McDowell wrote:
> > > +
> > > +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> > > +		ret = qca8k_vlan_add(priv, port, vid, !untagged);
> > > +
> > > +	if (ret)
> > > +		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
> > > +
> > 
> > If for some reason there is a temporary failure in qca8k_vlan_add, you'd
> > be swallowing it instead of printing the error and stopping the
> > execution.
> 
> I don't follow; I'm breaking out of the for loop when we get an error? I
> figured that was a better move than potentially printing 4095 error
> messages if they were all going to fail.
> 

Oh, you are. What an exotic way to write this loop, my brain stopped
parsing beyond "vid_end".

Thanks,
-Vladimir

[PATCH net-next v3 1/2] net: dsa: qca8k: Add define for port VID

[Jonathan McDowell]

Rather than using a magic value of 1 when configuring the port VIDs add
a QCA8K_PORT_VID_DEF define and use that instead. Also fix up the
bitmask in the process; the top 4 bits are reserved so this wasn't a
problem, but only masking 12 bits is the correct approach.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 11 ++++++-----
 drivers/net/dsa/qca8k.h |  2 ++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a5566de82853..3ebc4da63074 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -663,10 +663,11 @@ qca8k_setup(struct dsa_switch *ds)
 			 * default egress vid
 			 */
 			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-				  0xffff << shift, 1 << shift);
+				  0xfff << shift,
+				  QCA8K_PORT_VID_DEF << shift);
 			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
-				    QCA8K_PORT_VLAN_CVID(1) |
-				    QCA8K_PORT_VLAN_SVID(1));
+				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
+				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
 		}
 	}
 
@@ -1133,7 +1134,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 {
 	/* Set the vid to the port vlan id if no vid is set */
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_add(priv, addr, port_mask, vid,
 			     QCA8K_ATU_STATUS_STATIC);
@@ -1157,7 +1158,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 	u16 port_mask = BIT(port);
 
 	if (!vid)
-		vid = 1;
+		vid = QCA8K_PORT_VID_DEF;
 
 	return qca8k_fdb_del(priv, addr, port_mask, vid);
 }
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 31439396401c..92216a52daa5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -22,6 +22,8 @@
 
 #define QCA8K_CPU_PORT					0
 
+#define QCA8K_PORT_VID_DEF				1
+
 /* Global control registers */
 #define QCA8K_REG_MASK_CTRL				0x000
 #define   QCA8K_MASK_CTRL_ID_M				0xff
-- 
2.20.1

Re: [PATCH net-next v3 1/2] net: dsa: qca8k: Add define for port VID

[Florian Fainelli]

On 8/1/2020 10:05 AM, Jonathan McDowell wrote:
> Rather than using a magic value of 1 when configuring the port VIDs add
> a QCA8K_PORT_VID_DEF define and use that instead. Also fix up the
> bitmask in the process; the top 4 bits are reserved so this wasn't a
> problem, but only masking 12 bits is the correct approach.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next v3 1/2] net: dsa: qca8k: Add define for port VID

[Vladimir Oltean]

On Sat, Aug 01, 2020 at 06:05:54PM +0100, Jonathan McDowell wrote:
> Rather than using a magic value of 1 when configuring the port VIDs add
> a QCA8K_PORT_VID_DEF define and use that instead. Also fix up the
> bitmask in the process; the top 4 bits are reserved so this wasn't a
> problem, but only masking 12 bits is the correct approach.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---

Acked-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/qca8k.c | 11 ++++++-----
>  drivers/net/dsa/qca8k.h |  2 ++
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a5566de82853..3ebc4da63074 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -663,10 +663,11 @@ qca8k_setup(struct dsa_switch *ds)
>  			 * default egress vid
>  			 */
>  			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
> -				  0xffff << shift, 1 << shift);
> +				  0xfff << shift,
> +				  QCA8K_PORT_VID_DEF << shift);
>  			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
> -				    QCA8K_PORT_VLAN_CVID(1) |
> -				    QCA8K_PORT_VLAN_SVID(1));
> +				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
> +				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
>  		}
>  	}
>  
> @@ -1133,7 +1134,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  {
>  	/* Set the vid to the port vlan id if no vid is set */
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;
>  
>  	return qca8k_fdb_add(priv, addr, port_mask, vid,
>  			     QCA8K_ATU_STATUS_STATIC);
> @@ -1157,7 +1158,7 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
>  	u16 port_mask = BIT(port);
>  
>  	if (!vid)
> -		vid = 1;
> +		vid = QCA8K_PORT_VID_DEF;
>  
>  	return qca8k_fdb_del(priv, addr, port_mask, vid);
>  }
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 31439396401c..92216a52daa5 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -22,6 +22,8 @@
>  
>  #define QCA8K_CPU_PORT					0
>  
> +#define QCA8K_PORT_VID_DEF				1
> +
>  /* Global control registers */
>  #define QCA8K_REG_MASK_CTRL				0x000
>  #define   QCA8K_MASK_CTRL_ID_M				0xff
> -- 
> 2.20.1
> 

Re: [PATCH net-next v3 1/2] net: dsa: qca8k: Add define for port VID

[David Miller]

From: Jonathan McDowell <noodles@earth.li>
Date: Sat, 1 Aug 2020 18:05:54 +0100

> Rather than using a magic value of 1 when configuring the port VIDs add
> a QCA8K_PORT_VID_DEF define and use that instead. Also fix up the
> bitmask in the process; the top 4 bits are reserved so this wasn't a
> problem, but only masking 12 bits is the correct approach.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Applied.

[PATCH net-next v3 2/2] net: dsa: qca8k: Add 802.1q VLAN support

[Jonathan McDowell]

This adds full 802.1q VLAN support to the qca8k, allowing the use of
vlan_filtering and more complicated bridging setups than allowed by
basic port VLAN support.

Tested with a number of untagged ports with separate VLANs and then a
trunk port with all the VLANs tagged on it.

v3:
- Pull QCA8K_PORT_VID_DEF changes into separate cleanup patch
- Reverse Christmas tree notation for variable definitions
- Use untagged instead of tagged for consistency
v2:
- Return sensible errnos on failure rather than -1 (rmk)
- Style cleanups based on Florian's feedback
- Silently allow VLAN 0 as device correctly treats this as no tag

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 181 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  27 ++++++
 2 files changed, 208 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3ebc4da63074..f1e484477e35 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -408,6 +408,112 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
+static int
+qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
+{
+	u32 reg;
+
+	/* Set the command and VLAN index */
+	reg = QCA8K_VTU_FUNC1_BUSY;
+	reg |= cmd;
+	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
+
+	/* Write the function register triggering the table access */
+	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+
+	/* wait for completion */
+	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
+		return -ETIMEDOUT;
+
+	/* Check for table full violation when adding an entry */
+	if (cmd == QCA8K_VLAN_LOAD) {
+		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
+		if (reg & QCA8K_VTU_FUNC1_FULL)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
+{
+	u32 reg;
+	int ret;
+
+	/*
+	   We do the right thing with VLAN 0 and treat it as untagged while
+	   preserving the tag on egress.
+	 */
+	if (vid == 0)
+		return 0;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
+	reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+	if (untagged)
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
+				QCA8K_VTU_FUNC0_EG_MODE_S(port);
+	else
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
+				QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
+{
+	u32 reg, mask;
+	int ret, i;
+	bool del;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
+	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
+			QCA8K_VTU_FUNC0_EG_MODE_S(port);
+
+	/* Check if we're the last member to be removed */
+	del = true;
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
+		mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
+
+		if ((reg & mask) != mask) {
+			del = false;
+			break;
+		}
+	}
+
+	if (del) {
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
+	} else {
+		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+	}
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
 static void
 qca8k_mib_init(struct qca8k_priv *priv)
 {
@@ -1187,6 +1293,76 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int
+qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	if (vlan_filtering) {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+	} else {
+		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			  QCA8K_PORT_LOOKUP_VLAN_MODE,
+			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+	}
+
+	return 0;
+}
+
+static int
+qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_vlan *vlan)
+{
+	return 0;
+}
+
+static void
+qca8k_port_vlan_add(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	struct qca8k_priv *priv = ds->priv;
+	int ret = 0;
+	u16 vid;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
+		ret = qca8k_vlan_add(priv, port, vid, untagged);
+
+	if (ret)
+		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
+
+	if (pvid) {
+		int shift = 16 * (port % 2);
+
+		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
+			  0xfff << shift,
+			  vlan->vid_end << shift);
+		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
+			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
+			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
+	}
+}
+
+static int
+qca8k_port_vlan_del(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_vlan *vlan)
+{
+	struct qca8k_priv *priv = ds->priv;
+	int ret = 0;
+	u16 vid;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
+		ret = qca8k_vlan_del(priv, port, vid);
+
+	if (ret)
+		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);
+
+	return ret;
+}
+
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 		       enum dsa_tag_protocol mp)
@@ -1212,6 +1388,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.port_vlan_filtering	= qca8k_port_vlan_filtering,
+	.port_vlan_prepare	= qca8k_port_vlan_prepare,
+	.port_vlan_add		= qca8k_port_vlan_add,
+	.port_vlan_del		= qca8k_port_vlan_del,
 	.phylink_validate	= qca8k_phylink_validate,
 	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
 	.phylink_mac_config	= qca8k_phylink_mac_config,
@@ -1262,6 +1442,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
+	priv->ds->configure_vlan_while_not_filtering = true;
 	priv->ds->priv = priv;
 	priv->ops = qca8k_switch_ops;
 	priv->ds->ops = &priv->ops;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 92216a52daa5..7ca4b93e0bb5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -128,6 +128,19 @@
 #define   QCA8K_ATU_FUNC_FULL				BIT(12)
 #define   QCA8K_ATU_FUNC_PORT_M				0xf
 #define   QCA8K_ATU_FUNC_PORT_S				8
+#define QCA8K_REG_VTU_FUNC0				0x610
+#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
+#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
+#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
+#define   QCA8K_VTU_FUNC0_EG_MODE_MASK			3
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
+#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
+#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
+#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
+#define QCA8K_REG_VTU_FUNC1				0x614
+#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
+#define   QCA8K_VTU_FUNC1_VID_S				16
+#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
 #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
 #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
 #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
@@ -137,6 +150,11 @@
 #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
 #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
 #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
+#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
 #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
 #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
@@ -180,6 +198,15 @@ enum qca8k_fdb_cmd {
 	QCA8K_FDB_SEARCH = 7,
 };
 
+enum qca8k_vlan_cmd {
+	QCA8K_VLAN_FLUSH = 1,
+	QCA8K_VLAN_LOAD = 2,
+	QCA8K_VLAN_PURGE = 3,
+	QCA8K_VLAN_REMOVE_PORT = 4,
+	QCA8K_VLAN_NEXT = 5,
+	QCA8K_VLAN_READ = 6,
+};
+
 struct ar8xxx_port_status {
 	int enabled;
 };
-- 
2.20.1

Re: [PATCH net-next v3 2/2] net: dsa: qca8k: Add 802.1q VLAN support

[Florian Fainelli]

On 8/1/2020 10:06 AM, Jonathan McDowell wrote:
> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.
> 
> v3:
> - Pull QCA8K_PORT_VID_DEF changes into separate cleanup patch
> - Reverse Christmas tree notation for variable definitions
> - Use untagged instead of tagged for consistency
> v2:
> - Return sensible errnos on failure rather than -1 (rmk)
> - Style cleanups based on Florian's feedback
> - Silently allow VLAN 0 as device correctly treats this as no tag
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next v3 2/2] net: dsa: qca8k: Add 802.1q VLAN support

[Vladimir Oltean]

On Sat, Aug 01, 2020 at 06:06:46PM +0100, Jonathan McDowell wrote:
> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.
> 
> v3:
> - Pull QCA8K_PORT_VID_DEF changes into separate cleanup patch
> - Reverse Christmas tree notation for variable definitions
> - Use untagged instead of tagged for consistency
> v2:
> - Return sensible errnos on failure rather than -1 (rmk)
> - Style cleanups based on Florian's feedback
> - Silently allow VLAN 0 as device correctly treats this as no tag
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---

My comments have been addressed, thanks.

Acked-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/qca8k.c | 181 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  27 ++++++
>  2 files changed, 208 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 3ebc4da63074..f1e484477e35 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -408,6 +408,112 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> +{
> +	u32 reg;
> +
> +	/* Set the command and VLAN index */
> +	reg = QCA8K_VTU_FUNC1_BUSY;
> +	reg |= cmd;
> +	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
> +
> +	/* Write the function register triggering the table access */
> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
> +
> +	/* wait for completion */
> +	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
> +		return -ETIMEDOUT;
> +
> +	/* Check for table full violation when adding an entry */
> +	if (cmd == QCA8K_VLAN_LOAD) {
> +		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
> +		if (reg & QCA8K_VTU_FUNC1_FULL)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	/*
> +	   We do the right thing with VLAN 0 and treat it as untagged while
> +	   preserving the tag on egress.
> +	 */
> +	if (vid == 0)
> +		return 0;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret < 0)
> +		goto out;
> +
> +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
> +	reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +	if (untagged)
> +		reg |= QCA8K_VTU_FUNC0_EG_MODE_UNTAG <<
> +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +	else
> +		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
> +				QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +
> +out:
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
> +
> +static int
> +qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
> +{
> +	u32 reg, mask;
> +	int ret, i;
> +	bool del;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
> +	if (ret < 0)
> +		goto out;
> +
> +	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
> +	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
> +	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
> +			QCA8K_VTU_FUNC0_EG_MODE_S(port);
> +
> +	/* Check if we're the last member to be removed */
> +	del = true;
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		mask = QCA8K_VTU_FUNC0_EG_MODE_NOT;
> +		mask <<= QCA8K_VTU_FUNC0_EG_MODE_S(i);
> +
> +		if ((reg & mask) != mask) {
> +			del = false;
> +			break;
> +		}
> +	}
> +
> +	if (del) {
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
> +	} else {
> +		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
> +		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
> +	}
> +
> +out:
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
> +
>  static void
>  qca8k_mib_init(struct qca8k_priv *priv)
>  {
> @@ -1187,6 +1293,76 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static int
> +qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	if (vlan_filtering) {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
> +	} else {
> +		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE,
> +			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_vlan_prepare(struct dsa_switch *ds, int port,
> +			const struct switchdev_obj_port_vlan *vlan)
> +{
> +	return 0;
> +}
> +
> +static void
> +qca8k_port_vlan_add(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct qca8k_priv *priv = ds->priv;
> +	int ret = 0;
> +	u16 vid;
> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> +		ret = qca8k_vlan_add(priv, port, vid, untagged);
> +
> +	if (ret)
> +		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
> +
> +	if (pvid) {
> +		int shift = 16 * (port % 2);
> +
> +		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
> +			  0xfff << shift,
> +			  vlan->vid_end << shift);
> +		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
> +			    QCA8K_PORT_VLAN_CVID(vlan->vid_end) |
> +			    QCA8K_PORT_VLAN_SVID(vlan->vid_end));
> +	}
> +}
> +
> +static int
> +qca8k_port_vlan_del(struct dsa_switch *ds, int port,
> +		    const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	int ret = 0;
> +	u16 vid;
> +
> +	for (vid = vlan->vid_begin; vid <= vlan->vid_end && !ret; ++vid)
> +		ret = qca8k_vlan_del(priv, port, vid);
> +
> +	if (ret)
> +		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);
> +
> +	return ret;
> +}
> +
>  static enum dsa_tag_protocol
>  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
>  		       enum dsa_tag_protocol mp)
> @@ -1212,6 +1388,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_fdb_add		= qca8k_port_fdb_add,
>  	.port_fdb_del		= qca8k_port_fdb_del,
>  	.port_fdb_dump		= qca8k_port_fdb_dump,
> +	.port_vlan_filtering	= qca8k_port_vlan_filtering,
> +	.port_vlan_prepare	= qca8k_port_vlan_prepare,
> +	.port_vlan_add		= qca8k_port_vlan_add,
> +	.port_vlan_del		= qca8k_port_vlan_del,
>  	.phylink_validate	= qca8k_phylink_validate,
>  	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
>  	.phylink_mac_config	= qca8k_phylink_mac_config,
> @@ -1262,6 +1442,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  
>  	priv->ds->dev = &mdiodev->dev;
>  	priv->ds->num_ports = QCA8K_NUM_PORTS;
> +	priv->ds->configure_vlan_while_not_filtering = true;
>  	priv->ds->priv = priv;
>  	priv->ops = qca8k_switch_ops;
>  	priv->ds->ops = &priv->ops;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 92216a52daa5..7ca4b93e0bb5 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -128,6 +128,19 @@
>  #define   QCA8K_ATU_FUNC_FULL				BIT(12)
>  #define   QCA8K_ATU_FUNC_PORT_M				0xf
>  #define   QCA8K_ATU_FUNC_PORT_S				8
> +#define QCA8K_REG_VTU_FUNC0				0x610
> +#define   QCA8K_VTU_FUNC0_VALID				BIT(20)
> +#define   QCA8K_VTU_FUNC0_IVL_EN			BIT(19)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_S(_i)			(4 + (_i) * 2)
> +#define   QCA8K_VTU_FUNC0_EG_MODE_MASK			3
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNMOD			0
> +#define   QCA8K_VTU_FUNC0_EG_MODE_UNTAG			1
> +#define   QCA8K_VTU_FUNC0_EG_MODE_TAG			2
> +#define   QCA8K_VTU_FUNC0_EG_MODE_NOT			3
> +#define QCA8K_REG_VTU_FUNC1				0x614
> +#define   QCA8K_VTU_FUNC1_BUSY				BIT(31)
> +#define   QCA8K_VTU_FUNC1_VID_S				16
> +#define   QCA8K_VTU_FUNC1_FULL				BIT(4)
>  #define QCA8K_REG_GLOBAL_FW_CTRL0			0x620
>  #define   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN		BIT(10)
>  #define QCA8K_REG_GLOBAL_FW_CTRL1			0x624
> @@ -137,6 +150,11 @@
>  #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_S			0
>  #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
>  #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE			GENMASK(9, 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_NONE		(0 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_FALLBACK		(1 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_CHECK		(2 << 8)
> +#define   QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE		(3 << 8)
>  #define   QCA8K_PORT_LOOKUP_STATE_MASK			GENMASK(18, 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_DISABLED		(0 << 16)
>  #define   QCA8K_PORT_LOOKUP_STATE_BLOCKING		(1 << 16)
> @@ -180,6 +198,15 @@ enum qca8k_fdb_cmd {
>  	QCA8K_FDB_SEARCH = 7,
>  };
>  
> +enum qca8k_vlan_cmd {
> +	QCA8K_VLAN_FLUSH = 1,
> +	QCA8K_VLAN_LOAD = 2,
> +	QCA8K_VLAN_PURGE = 3,
> +	QCA8K_VLAN_REMOVE_PORT = 4,
> +	QCA8K_VLAN_NEXT = 5,
> +	QCA8K_VLAN_READ = 6,
> +};
> +
>  struct ar8xxx_port_status {
>  	int enabled;
>  };
> -- 
> 2.20.1
> 

Re: [PATCH net-next v3 2/2] net: dsa: qca8k: Add 802.1q VLAN support

[David Miller]

From: Jonathan McDowell <noodles@earth.li>
Date: Sat, 1 Aug 2020 18:06:46 +0100

> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> vlan_filtering and more complicated bridging setups than allowed by
> basic port VLAN support.
> 
> Tested with a number of untagged ports with separate VLANs and then a
> trunk port with all the VLANs tagged on it.
> 
> v3:
> - Pull QCA8K_PORT_VID_DEF changes into separate cleanup patch
> - Reverse Christmas tree notation for variable definitions
> - Use untagged instead of tagged for consistency
> v2:
> - Return sensible errnos on failure rather than -1 (rmk)
> - Style cleanups based on Florian's feedback
> - Silently allow VLAN 0 as device correctly treats this as no tag
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

Applied.