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