diff mbox series

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

Message ID 20200721171624.GK23489@earth.li
State RFC
Delegated to: David Miller
Headers show
Series [RFC] net: dsa: qca8k: Add 802.1q VLAN support | expand

Commit Message

Jonathan McDowell July 21, 2020, 5:16 p.m. UTC
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>

Comments

Florian Fainelli July 21, 2020, 5:26 p.m. UTC | #1
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
Russell King (Oracle) July 21, 2020, 8:48 p.m. UTC | #2
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;
>  };
>
Jonathan McDowell July 22, 2020, 7:33 p.m. UTC | #3
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.
Jonathan McDowell July 22, 2020, 7:38 p.m. UTC | #4
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.
Florian Fainelli July 22, 2020, 10:36 p.m. UTC | #5
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?
Vladimir Oltean July 22, 2020, 10:58 p.m. UTC | #6
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
Jonathan McDowell July 25, 2020, 5:35 p.m. UTC | #7
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.
diff mbox series

Patch

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;
 };