Date:   Thu, 30 Jul 2020 11:40:29 +0100
From:   Jonathan McDowell <noodles@...th.li>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Matthew Hagan <mnhagan88@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: dsa: qca8k: Add 802.1q VLAN support

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@...th.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.

Powered by blists - more mailing lists