[PATCH 0/3] net: dsa: mv88e6xxx: dump hardware VLANs

[PATCH 0/3] net: dsa: mv88e6xxx: dump hardware VLANs

[Vivien Didelot]

This patchset requires "net: dsa: add support for switchdev VLAN objects" [1].

Thanks to the switchdev bindings for ports' bridge_getlink, this patchset adds
support for dumping the hardware VLAN Table Unit of Marvell 88E6xxx compatible
switch chips.

It allows "bridge vlan" to query the hardware, and also brings a new debugfs
"vtu" file. A populated VLAN Table Unit can show the following output:

    # cat /sys/kernel/debug/dsa0/vtu
    VID  FID  SID  P0 P1 P2 P3 P4 P5 P6
    550  562  0    x  x  x  u  x  t  x
    1000 1012 0    x  x  t  x  x  t  x
    1200 1212 0    x  x  t  x  t  t  x

    # bridge vlan
    port	vlan ids
    swp0	None
    swp0
    swp1	None
    swp1
    swp2	1000
		1200

    swp2	1000
		1200

    swp3	550 PVID Egress Untagged

    swp3	550 PVID Egress Untagged

    swp4	1200

    swp4	1200

    br0		None

[1] https://lkml.org/lkml/2015/6/23/494

Vivien Didelot (3):
  net: dsa: mv88e6xxx: add debugfs interface for VTU
  net: dsa: mv88e6xxx: add support to dump VLANs
  net: dsa: mv88e6352: add support for port_vlan_dump

 drivers/net/dsa/mv88e6352.c |   1 +
 drivers/net/dsa/mv88e6xxx.c | 182 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  26 +++++++
 3 files changed, 209 insertions(+)

-- 
2.4.4

[PATCH 1/3] net: dsa: mv88e6xxx: add debugfs interface for VTU

[Vivien Didelot]

Implement the Get Next operation for the VLAN Table Unit, and a "vtu"
debugfs file to dump the hardware VLANs.

A populated VTU can look like this:

    # cat /sys/kernel/debug/dsa0/vtu
    VID  FID  SID  P0 P1 P2 P3 P4 P5 P6
    550  562  0    x  x  x  u  x  t  x
    1000 1012 0    x  x  t  x  x  t  x
    1200 1212 0    x  x  t  x  t  t  x

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  24 ++++++++
 2 files changed, 162 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8c130c0..0dce8e8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1366,6 +1366,81 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	}
 }
 
+static int _mv88e6xxx_vtu_wait(struct dsa_switch *ds)
+{
+	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_VTU_OP,
+			       GLOBAL_VTU_OP_BUSY);
+}
+
+static int _mv88e6xxx_vtu_cmd(struct dsa_switch *ds, u16 op)
+{
+	int ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_OP, op);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_wait(ds);
+}
+
+static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
+				  struct mv88e6xxx_vtu_entry *entry)
+{
+	int ret, i;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
+				   vid & GLOBAL_VTU_VID_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
+	if (ret < 0)
+		return ret;
+
+	entry->vid = ret & GLOBAL_VTU_VID_MASK;
+	entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
+
+	if (entry->valid) {
+		/* Ports 0-3, offsets 0, 4, 8, 12 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < 4; ++i)
+			entry->tags[i] = (ret >> (i * 4)) & 3;
+
+		/* Ports 4-6, offsets 0, 4, 8 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
+		if (ret < 0)
+			return ret;
+
+		for (i = 4; i < 7; ++i)
+			entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
+		if (ret < 0)
+			return ret;
+
+		entry->fid = ret & GLOBAL_VTU_FID_MASK;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
+		if (ret < 0)
+			return ret;
+
+		entry->sid = ret & GLOBAL_VTU_SID_MASK;
+	}
+
+	return 0;
+}
+
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -1739,6 +1814,66 @@ static const struct file_operations mv88e6xxx_atu_fops = {
 	.owner  = THIS_MODULE,
 };
 
+static int mv88e6xxx_vtu_show(struct seq_file *s, void *p)
+{
+	struct dsa_switch *ds = s->private;
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int vid = 0xfff; /* find the first or lowest VID */
+	int ret = 0;
+	int port;
+
+	seq_puts(s, "VID  FID  SID  P0 P1 P2 P3 P4 P5 P6\n");
+	mutex_lock(&ps->smi_mutex);
+
+	do {
+		struct mv88e6xxx_vtu_entry next = { 0 };
+
+		ret = _mv88e6xxx_vtu_getnext(ds, vid, &next);
+		if (ret < 0)
+			goto unlock;
+
+		if (!next.valid)
+			break;
+
+		seq_printf(s, "%-4d %-4d %-2d ", next.vid, next.fid, next.sid);
+		for (port = 0; port < 7; ++port) {
+			u8 tag = next.tags[port];
+
+			if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED)
+				seq_puts(s, "  -");
+			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED)
+				seq_puts(s, "  u");
+			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED)
+				seq_puts(s, "  t");
+			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
+				seq_puts(s, "  x");
+			else
+				seq_puts(s, "  ?"); /* unlikely */
+		}
+		seq_puts(s, "\n");
+
+		vid = next.vid;
+	} while (vid < 0xfff);
+
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+static int mv88e6xxx_vtu_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, mv88e6xxx_vtu_show, inode->i_private);
+}
+
+static const struct file_operations mv88e6xxx_vtu_fops = {
+	.open		= mv88e6xxx_vtu_open,
+	.read		= seq_read,
+	.llseek		= no_llseek,
+	.release	= single_release,
+	.owner		= THIS_MODULE,
+};
+
 static void mv88e6xxx_stats_show_header(struct seq_file *s,
 					struct mv88e6xxx_priv_state *ps)
 {
@@ -1901,6 +2036,9 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds)
 	debugfs_create_file("atu", S_IRUGO, ps->dbgfs, ds,
 			    &mv88e6xxx_atu_fops);
 
+	debugfs_create_file("vtu", S_IRUGO, ps->dbgfs, ds,
+			    &mv88e6xxx_vtu_fops);
+
 	debugfs_create_file("stats", S_IRUGO, ps->dbgfs, ds,
 			    &mv88e6xxx_stats_fops);
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 1bc5a3e..798b6b8 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -124,6 +124,7 @@
 #define PORT_CONTROL_1		0x05
 #define PORT_BASE_VLAN		0x06
 #define PORT_DEFAULT_VLAN	0x07
+#define PORT_DEFAULT_VLAN_MASK	0xfff
 #define PORT_CONTROL_2		0x08
 #define PORT_CONTROL_2_IGNORE_FCS	BIT(15)
 #define PORT_CONTROL_2_VTU_PRI_OVERRIDE	BIT(14)
@@ -170,6 +171,10 @@
 #define GLOBAL_MAC_01		0x01
 #define GLOBAL_MAC_23		0x02
 #define GLOBAL_MAC_45		0x03
+#define GLOBAL_VTU_FID		0x02 /* 6352 only? */
+#define GLOBAL_VTU_FID_MASK	0xfff
+#define GLOBAL_VTU_SID		0x03 /* 6352 only? */
+#define GLOBAL_VTU_SID_MASK	0x3f
 #define GLOBAL_CONTROL		0x04
 #define GLOBAL_CONTROL_SW_RESET		BIT(15)
 #define GLOBAL_CONTROL_PPU_ENABLE	BIT(14)
@@ -186,9 +191,20 @@
 #define GLOBAL_CONTROL_TCAM_EN		BIT(1)
 #define GLOBAL_CONTROL_EEPROM_DONE_EN	BIT(0)
 #define GLOBAL_VTU_OP		0x05
+#define GLOBAL_VTU_OP_BUSY	BIT(15)
+#define GLOBAL_VTU_OP_FLUSH_ALL		((1 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_LOAD_PURGE	((3 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_GET_NEXT	((4 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_STU_LOAD_PURGE	((5 << 12) | GLOBAL_VTU_OP_BUSY)
 #define GLOBAL_VTU_VID		0x06
+#define GLOBAL_VTU_VID_MASK	0xfff
+#define GLOBAL_VTU_VID_VALID	BIT(12)
 #define GLOBAL_VTU_DATA_0_3	0x07
 #define GLOBAL_VTU_DATA_4_7	0x08
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED	0
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED	1
+#define GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED	2
+#define GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER	3
 #define GLOBAL_VTU_DATA_8_11	0x09
 #define GLOBAL_ATU_CONTROL	0x0a
 #define GLOBAL_ATU_CONTROL_LEARN2ALL	BIT(3)
@@ -310,6 +326,14 @@
 #define GLOBAL2_QOS_WEIGHT	0x1c
 #define GLOBAL2_MISC		0x1d
 
+struct mv88e6xxx_vtu_entry {
+	u16	vid;
+	u16	fid;
+	u8	sid;
+	bool	valid;
+	u8	tags[DSA_MAX_PORTS];
+};
+
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
-- 
2.4.4

[PATCH 2/3] net: dsa: mv88e6xxx: add support to dump VLANs

[Vivien Didelot]

This commit implements the port_vlan_dump function in the
dsa_switch_driver structure for Marvell 88E6xxx compatible switches.

This allows to access a switch VLAN Table Unit from standard userspace
commands such as "bridge vlan show".

A populated VTU can give the following output:

    # bridge vlan
    port	vlan ids
    swp0	None
    swp0
    swp1	None
    swp1
    swp2	1000
		1200

    swp2	1000
		1200

    swp3	550 PVID Egress Untagged

    swp3	550 PVID Egress Untagged

    swp4	1200

    swp4	1200

    br0	None

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 0dce8e8..0d63bf6 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1441,6 +1441,50 @@ static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
 	return 0;
 }
 
+int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port, u16 vid,
+			     u16 *bridge_flags)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int prev_vid = vid ? vid - 1 : 4095;
+	int ret;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	if (entry.vid != vid || !entry.valid) {
+		ret = -ENOENT;
+		goto unlock;
+	}
+
+	switch (entry.tags[port]) {
+	case GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED:
+		*bridge_flags |= BRIDGE_VLAN_INFO_UNTAGGED;
+		/* fall through... */
+	case GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED:
+		break;
+	default:
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* check PVID */
+	ret = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_DEFAULT_VLAN);
+	if (ret < 0)
+		goto unlock;
+
+	if ((ret & PORT_DEFAULT_VLAN_MASK) == vid)
+		*bridge_flags |= BRIDGE_VLAN_INFO_PVID;
+
+	ret = 0;
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 798b6b8..7b75c2b 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -440,6 +440,8 @@ int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg);
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
+int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds,int port, u16 vid,
+			     u16 *bridge_flags);
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
 extern struct dsa_switch_driver mv88e6352_switch_driver;
-- 
2.4.4

[PATCH 3/3] net: dsa: mv88e6352: add support for port_vlan_dump

[Vivien Didelot]

Add support for dumping the VLAN Table Unit entries by pointing to the
port_vlan_dump function implemented for mv88e6xxx.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6352.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index b524bd3..c35f57f 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -397,6 +397,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.fdb_add		= mv88e6xxx_port_fdb_add,
 	.fdb_del		= mv88e6xxx_port_fdb_del,
 	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
+	.port_vlan_dump		= mv88e6xxx_port_vlan_dump,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
-- 
2.4.4

Re: [PATCH 3/3] net: dsa: mv88e6352: add support for port_vlan_dump

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 23 Jun 2015 17:46:10 -0400

> Add support for dumping the VLAN Table Unit entries by pointing to the
> port_vlan_dump function implemented for mv88e6xxx.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

There is no reason to separate this from patch #2.

Re: [PATCH 1/3] net: dsa: mv88e6xxx: add debugfs interface for VTU

[Andrew Lunn]

On Tue, Jun 23, 2015 at 05:46:08PM -0400, Vivien Didelot wrote:
> Implement the Get Next operation for the VLAN Table Unit, and a "vtu"
> debugfs file to dump the hardware VLANs.
> 
> A populated VTU can look like this:
> 
>     # cat /sys/kernel/debug/dsa0/vtu
>     VID  FID  SID  P0 P1 P2 P3 P4 P5 P6
>     550  562  0    x  x  x  u  x  t  x
>     1000 1012 0    x  x  t  x  x  t  x
>     1200 1212 0    x  x  t  x  t  t  x


Hi Vivien

Could you make this more generic and make use of ps->num_ports? Not
all switches have 7 ports.

> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx.h |  24 ++++++++
>  2 files changed, 162 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 8c130c0..0dce8e8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1366,6 +1366,81 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
>  	}
>  }
>  
> +static int _mv88e6xxx_vtu_wait(struct dsa_switch *ds)
> +{
> +	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_VTU_OP,
> +			       GLOBAL_VTU_OP_BUSY);
> +}
> +
> +static int _mv88e6xxx_vtu_cmd(struct dsa_switch *ds, u16 op)
> +{
> +	int ret;
> +
> +	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_OP, op);
> +	if (ret < 0)
> +		return ret;
> +
> +	return _mv88e6xxx_vtu_wait(ds);
> +}
> +
> +static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
> +				  struct mv88e6xxx_vtu_entry *entry)
> +{
> +	int ret, i;
> +
> +	ret = _mv88e6xxx_vtu_wait(ds);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
> +				   vid & GLOBAL_VTU_VID_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry->vid = ret & GLOBAL_VTU_VID_MASK;
> +	entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
> +
> +	if (entry->valid) {
> +		/* Ports 0-3, offsets 0, 4, 8, 12 */
> +		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (i = 0; i < 4; ++i)
> +			entry->tags[i] = (ret >> (i * 4)) & 3;
> +
> +		/* Ports 4-6, offsets 0, 4, 8 */
> +		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (i = 4; i < 7; ++i)
> +			entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
> +
> +		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
> +		if (ret < 0)
> +			return ret;
> +
> +		entry->fid = ret & GLOBAL_VTU_FID_MASK;
> +
> +		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
> +		if (ret < 0)
> +			return ret;
> +
> +		entry->sid = ret & GLOBAL_VTU_SID_MASK;

As you point out in the header file, not all switches have FID and
VID. A quick look at DSDT suggests for both FID and SID:

DEV_88E6097_FAMILY | DEV_88E6165_FAMILY | DEV_88E6351_FAMILY | DEV_88E6352_FAMILY

Please limit access to these registers to just these families.

> +	}
> +
> +	return 0;
> +}
> +
>  static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> @@ -1739,6 +1814,66 @@ static const struct file_operations mv88e6xxx_atu_fops = {
>  	.owner  = THIS_MODULE,
>  };
>  
> +static int mv88e6xxx_vtu_show(struct seq_file *s, void *p)
> +{
> +	struct dsa_switch *ds = s->private;
> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> +	int vid = 0xfff; /* find the first or lowest VID */
> +	int ret = 0;
> +	int port;
> +
> +	seq_puts(s, "VID  FID  SID  P0 P1 P2 P3 P4 P5 P6\n");
> +	mutex_lock(&ps->smi_mutex);
> +
> +	do {
> +		struct mv88e6xxx_vtu_entry next = { 0 };
> +
> +		ret = _mv88e6xxx_vtu_getnext(ds, vid, &next);
> +		if (ret < 0)
> +			goto unlock;
> +
> +		if (!next.valid)
> +			break;
> +
> +		seq_printf(s, "%-4d %-4d %-2d ", next.vid, next.fid, next.sid);
> +		for (port = 0; port < 7; ++port) {
> +			u8 tag = next.tags[port];
> +
> +			if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED)
> +				seq_puts(s, "  -");
> +			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED)
> +				seq_puts(s, "  u");
> +			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED)
> +				seq_puts(s, "  t");
> +			else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
> +				seq_puts(s, "  x");
> +			else
> +				seq_puts(s, "  ?"); /* unlikely */

Maybe use a switch statememt?

Thanks
	Andrew

Re: [PATCH 2/3] net: dsa: mv88e6xxx: add support to dump VLANs

[Andrew Lunn]

On Tue, Jun 23, 2015 at 05:46:09PM -0400, Vivien Didelot wrote:
> This commit implements the port_vlan_dump function in the
> dsa_switch_driver structure for Marvell 88E6xxx compatible switches.
> 
> This allows to access a switch VLAN Table Unit from standard userspace
> commands such as "bridge vlan show".
> 
> A populated VTU can give the following output:
> 
>     # bridge vlan
>     port	vlan ids
>     swp0	None
>     swp0
>     swp1	None
>     swp1
>     swp2	1000
> 		1200
> 
>     swp2	1000
> 		1200
> 
>     swp3	550 PVID Egress Untagged
> 
>     swp3	550 PVID Egress Untagged
> 
>     swp4	1200
> 
>     swp4	1200
> 
>     br0	None
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 0dce8e8..0d63bf6 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1441,6 +1441,50 @@ static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
>  	return 0;
>  }
>  
> +int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port, u16 vid,
> +			     u16 *bridge_flags)
> +{
> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> +	struct mv88e6xxx_vtu_entry entry = { 0 };
> +	int prev_vid = vid ? vid - 1 : 4095;
> +	int ret;
> +
> +	mutex_lock(&ps->smi_mutex);
> +	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	if (entry.vid != vid || !entry.valid) {
> +		ret = -ENOENT;
> +		goto unlock;
> +	}
> +
> +	switch (entry.tags[port]) {
> +	case GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED:
> +		*bridge_flags |= BRIDGE_VLAN_INFO_UNTAGGED;
> +		/* fall through... */
> +	case GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED:
> +		break;

A nit pick, but fall through catches people out. Having a fall through
to a break statements seems like a trap for future coders to fall
into. I would replace the comment with a break, and let the compiler
do the optimisation.

   Andrew

Re: [PATCH 3/3] net: dsa: mv88e6352: add support for port_vlan_dump

[Andrew Lunn]

On Tue, Jun 23, 2015 at 05:46:10PM -0400, Vivien Didelot wrote:
> Add support for dumping the VLAN Table Unit entries by pointing to the
> port_vlan_dump function implemented for mv88e6xxx.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6352.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
> index b524bd3..c35f57f 100644
> --- a/drivers/net/dsa/mv88e6352.c
> +++ b/drivers/net/dsa/mv88e6352.c
> @@ -397,6 +397,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
>  	.fdb_add		= mv88e6xxx_port_fdb_add,
>  	.fdb_del		= mv88e6xxx_port_fdb_del,
>  	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
> +	.port_vlan_dump		= mv88e6xxx_port_vlan_dump,
>  };

Once the code is made generic in terms of number of ports, i would
suggest adding this to all drivers which support it. Between us, we
can test most of them.

Thanks
	Andrew

Re: [PATCH 3/3] net: dsa: mv88e6352: add support for port_vlan_dump

[Vivien Didelot]

Hi Andrew,

On Jun 26, 2015, at 10:12 AM, Andrew Lunn andrew@lunn.ch wrote:

> On Tue, Jun 23, 2015 at 05:46:10PM -0400, Vivien Didelot wrote:
>> Add support for dumping the VLAN Table Unit entries by pointing to the
>> port_vlan_dump function implemented for mv88e6xxx.
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  drivers/net/dsa/mv88e6352.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
>> index b524bd3..c35f57f 100644
>> --- a/drivers/net/dsa/mv88e6352.c
>> +++ b/drivers/net/dsa/mv88e6352.c
>> @@ -397,6 +397,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
>>  	.fdb_add		= mv88e6xxx_port_fdb_add,
>>  	.fdb_del		= mv88e6xxx_port_fdb_del,
>>  	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
>> +	.port_vlan_dump		= mv88e6xxx_port_vlan_dump,
>>  };
> 
> Once the code is made generic in terms of number of ports, i would
> suggest adding this to all drivers which support it. Between us, we
> can test most of them.
> 
> Thanks
> 	Andrew

OK. I'll add it in mv88e6352.c, mv88e6171.c, mv88e6131.c, mv88e6123_61_65.c.

Thanks,
-v

Re: [PATCH 2/3] net: dsa: mv88e6xxx: add support to dump VLANs

[Vivien Didelot]

Hi Andrew,

On Jun 26, 2015, at 10:10 AM, Andrew Lunn andrew@lunn.ch wrote:

> On Tue, Jun 23, 2015 at 05:46:09PM -0400, Vivien Didelot wrote:
>> This commit implements the port_vlan_dump function in the
>> dsa_switch_driver structure for Marvell 88E6xxx compatible switches.
>> 
>> This allows to access a switch VLAN Table Unit from standard userspace
>> commands such as "bridge vlan show".
>> 
>> A populated VTU can give the following output:
>> 
>>     # bridge vlan
>>     port	vlan ids
>>     swp0	None
>>     swp0
>>     swp1	None
>>     swp1
>>     swp2	1000
>> 		1200
>> 
>>     swp2	1000
>> 		1200
>> 
>>     swp3	550 PVID Egress Untagged
>> 
>>     swp3	550 PVID Egress Untagged
>> 
>>     swp4	1200
>> 
>>     swp4	1200
>> 
>>     br0	None
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/net/dsa/mv88e6xxx.h |  2 ++
>>  2 files changed, 46 insertions(+)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index 0dce8e8..0d63bf6 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -1441,6 +1441,50 @@ static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds,
>> u16 vid,
>>  	return 0;
>>  }
>>  
>> +int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port, u16 vid,
>> +			     u16 *bridge_flags)
>> +{
>> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> +	struct mv88e6xxx_vtu_entry entry = { 0 };
>> +	int prev_vid = vid ? vid - 1 : 4095;
>> +	int ret;
>> +
>> +	mutex_lock(&ps->smi_mutex);
>> +	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
>> +	if (ret < 0)
>> +		goto unlock;
>> +
>> +	if (entry.vid != vid || !entry.valid) {
>> +		ret = -ENOENT;
>> +		goto unlock;
>> +	}
>> +
>> +	switch (entry.tags[port]) {
>> +	case GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED:
>> +		*bridge_flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>> +		/* fall through... */
>> +	case GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED:
>> +		break;
> 
> A nit pick, but fall through catches people out. Having a fall through
> to a break statements seems like a trap for future coders to fall
> into. I would replace the comment with a break, and let the compiler
> do the optimisation.
> 
>    Andrew

Done.

Thanks,
-v

Re: [PATCH 1/3] net: dsa: mv88e6xxx: add debugfs interface for VTU

[Vivien Didelot]

Hi Andrew,

On Jun 26, 2015, at 10:04 AM, Andrew Lunn andrew@lunn.ch wrote:
> On Tue, Jun 23, 2015 at 05:46:08PM -0400, Vivien Didelot wrote:
>> Implement the Get Next operation for the VLAN Table Unit, and a "vtu"
>> debugfs file to dump the hardware VLANs.
>> 
>> A populated VTU can look like this:
>> 
>>     # cat /sys/kernel/debug/dsa0/vtu
>>     VID  FID  SID  P0 P1 P2 P3 P4 P5 P6
>>     550  562  0    x  x  x  u  x  t  x
>>     1000 1012 0    x  x  t  x  x  t  x
>>     1200 1212 0    x  x  t  x  t  t  x
> 
> 
> Hi Vivien
> 
> Could you make this more generic and make use of ps->num_ports? Not
> all switches have 7 ports.

Sure, done.

>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/net/dsa/mv88e6xxx.h |  24 ++++++++
>>  2 files changed, 162 insertions(+)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index 8c130c0..0dce8e8 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -1366,6 +1366,81 @@ static void mv88e6xxx_bridge_work(struct work_struct
>> *work)
>>      }
>>  }
>>  
>> +static int _mv88e6xxx_vtu_wait(struct dsa_switch *ds)
>> +{
>> +    return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_VTU_OP,
>> +                   GLOBAL_VTU_OP_BUSY);
>> +}
>> +
>> +static int _mv88e6xxx_vtu_cmd(struct dsa_switch *ds, u16 op)
>> +{
>> +    int ret;
>> +
>> +    ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_OP, op);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return _mv88e6xxx_vtu_wait(ds);
>> +}
>> +
>> +static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
>> +                  struct mv88e6xxx_vtu_entry *entry)
>> +{
>> +    int ret, i;
>> +
>> +    ret = _mv88e6xxx_vtu_wait(ds);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
>> +                   vid & GLOBAL_VTU_VID_MASK);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    entry->vid = ret & GLOBAL_VTU_VID_MASK;
>> +    entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
>> +
>> +    if (entry->valid) {
>> +        /* Ports 0-3, offsets 0, 4, 8, 12 */
>> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        for (i = 0; i < 4; ++i)
>> +            entry->tags[i] = (ret >> (i * 4)) & 3;
>> +
>> +        /* Ports 4-6, offsets 0, 4, 8 */
>> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        for (i = 4; i < 7; ++i)
>> +            entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
>> +
>> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        entry->fid = ret & GLOBAL_VTU_FID_MASK;
>> +
>> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        entry->sid = ret & GLOBAL_VTU_SID_MASK;
> 
> As you point out in the header file, not all switches have FID and
> VID. A quick look at DSDT suggests for both FID and SID:
> 
> DEV_88E6097_FAMILY | DEV_88E6165_FAMILY | DEV_88E6351_FAMILY |
> DEV_88E6352_FAMILY
> 
> Please limit access to these registers to just these families.

OK. Thanks for pointing this out. I think you meant SID instead of VID.
Would something like the following be correct then?

#define GLOBAL_VTU_FID          0x02    /* 6097 6165 6351 6352 */
#define GLOBAL_VTU_SID          0x03    /* 6097 6165 6351 6352 */

    if (mv88e6xxx_6097_family(ds) || mv88e6xxx_6165_family(ds) ||
        mv88e6xxx_6351_family(ds) || mv88e6xxx_6352_family(ds)) {
        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
        if (ret < 0)
            return ret;

        entry->fid = ret & GLOBAL_VTU_FID_MASK;

        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
        if (ret < 0)
            return ret;

        entry->sid = ret & GLOBAL_VTU_SID_MASK;
    }

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>>  {
>>      struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> @@ -1739,6 +1814,66 @@ static const struct file_operations mv88e6xxx_atu_fops =
>> {
>>      .owner  = THIS_MODULE,
>>  };
>>  
>> +static int mv88e6xxx_vtu_show(struct seq_file *s, void *p)
>> +{
>> +    struct dsa_switch *ds = s->private;
>> +    struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> +    int vid = 0xfff; /* find the first or lowest VID */
>> +    int ret = 0;
>> +    int port;
>> +
>> +    seq_puts(s, "VID  FID  SID  P0 P1 P2 P3 P4 P5 P6\n");
>> +    mutex_lock(&ps->smi_mutex);
>> +
>> +    do {
>> +        struct mv88e6xxx_vtu_entry next = { 0 };
>> +
>> +        ret = _mv88e6xxx_vtu_getnext(ds, vid, &next);
>> +        if (ret < 0)
>> +            goto unlock;
>> +
>> +        if (!next.valid)
>> +            break;
>> +
>> +        seq_printf(s, "%-4d %-4d %-2d ", next.vid, next.fid, next.sid);
>> +        for (port = 0; port < 7; ++port) {
>> +            u8 tag = next.tags[port];
>> +
>> +            if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED)
>> +                seq_puts(s, "  -");
>> +            else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED)
>> +                seq_puts(s, "  u");
>> +            else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED)
>> +                seq_puts(s, "  t");
>> +            else if (tag == GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
>> +                seq_puts(s, "  x");
>> +            else
>> +                seq_puts(s, "  ?"); /* unlikely */
> 
> Maybe use a switch statememt?

No problem.

Thanks,
-v

Re: [PATCH 1/3] net: dsa: mv88e6xxx: add debugfs interface for VTU

[Andrew Lunn]

> >> +static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
> >> +                  struct mv88e6xxx_vtu_entry *entry)
> >> +{
> >> +    int ret, i;
> >> +
> >> +    ret = _mv88e6xxx_vtu_wait(ds);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
> >> +                   vid & GLOBAL_VTU_VID_MASK);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    entry->vid = ret & GLOBAL_VTU_VID_MASK;
> >> +    entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
> >> +
> >> +    if (entry->valid) {
> >> +        /* Ports 0-3, offsets 0, 4, 8, 12 */
> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        for (i = 0; i < 4; ++i)
> >> +            entry->tags[i] = (ret >> (i * 4)) & 3;
> >> +
> >> +        /* Ports 4-6, offsets 0, 4, 8 */
> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        for (i = 4; i < 7; ++i)
> >> +            entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
> >> +
> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        entry->fid = ret & GLOBAL_VTU_FID_MASK;
> >> +
> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        entry->sid = ret & GLOBAL_VTU_SID_MASK;
> > 
> > As you point out in the header file, not all switches have FID and
> > VID. A quick look at DSDT suggests for both FID and SID:
> > 
> > DEV_88E6097_FAMILY | DEV_88E6165_FAMILY | DEV_88E6351_FAMILY |
> > DEV_88E6352_FAMILY
> > 
> > Please limit access to these registers to just these families.
> 
> OK. Thanks for pointing this out. I think you meant SID instead of VID.

Yes, my error.

> Would something like the following be correct then?
> 
> #define GLOBAL_VTU_FID          0x02    /* 6097 6165 6351 6352 */
> #define GLOBAL_VTU_SID          0x03    /* 6097 6165 6351 6352 */

Yes. I've not annotated all #defines, but here there is a clear
overlap with the MAC address for some chips, so the comment is
justified.
 
>     if (mv88e6xxx_6097_family(ds) || mv88e6xxx_6165_family(ds) ||
>         mv88e6xxx_6351_family(ds) || mv88e6xxx_6352_family(ds)) {
>         ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
>         if (ret < 0)
>             return ret;
> 
>         entry->fid = ret & GLOBAL_VTU_FID_MASK;
> 
>         ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
>         if (ret < 0)
>             return ret;
> 
>         entry->sid = ret & GLOBAL_VTU_SID_MASK;
>     }

Maybe add an else to set sid and fid to 0?

Thanks
	Andrew

Re: [PATCH 1/3] net: dsa: mv88e6xxx: add debugfs interface for VTU

[Vivien Didelot]

Hi Andrew,

On Jun 26, 2015, at 11:23 AM, Andrew Lunn andrew@lunn.ch wrote:

>> >> +static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
>> >> +                  struct mv88e6xxx_vtu_entry *entry)
>> >> +{
>> >> +    int ret, i;
>> >> +
>> >> +    ret = _mv88e6xxx_vtu_wait(ds);
>> >> +    if (ret < 0)
>> >> +        return ret;
>> >> +
>> >> +    ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
>> >> +                   vid & GLOBAL_VTU_VID_MASK);
>> >> +    if (ret < 0)
>> >> +        return ret;
>> >> +
>> >> +    ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
>> >> +    if (ret < 0)
>> >> +        return ret;
>> >> +
>> >> +    ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
>> >> +    if (ret < 0)
>> >> +        return ret;
>> >> +
>> >> +    entry->vid = ret & GLOBAL_VTU_VID_MASK;
>> >> +    entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
>> >> +
>> >> +    if (entry->valid) {
>> >> +        /* Ports 0-3, offsets 0, 4, 8, 12 */
>> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
>> >> +        if (ret < 0)
>> >> +            return ret;
>> >> +
>> >> +        for (i = 0; i < 4; ++i)
>> >> +            entry->tags[i] = (ret >> (i * 4)) & 3;
>> >> +
>> >> +        /* Ports 4-6, offsets 0, 4, 8 */
>> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
>> >> +        if (ret < 0)
>> >> +            return ret;
>> >> +
>> >> +        for (i = 4; i < 7; ++i)
>> >> +            entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
>> >> +
>> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
>> >> +        if (ret < 0)
>> >> +            return ret;
>> >> +
>> >> +        entry->fid = ret & GLOBAL_VTU_FID_MASK;
>> >> +
>> >> +        ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
>> >> +        if (ret < 0)
>> >> +            return ret;
>> >> +
>> >> +        entry->sid = ret & GLOBAL_VTU_SID_MASK;
>> > 
>> > As you point out in the header file, not all switches have FID and
>> > VID. A quick look at DSDT suggests for both FID and SID:
>> > 
>> > DEV_88E6097_FAMILY | DEV_88E6165_FAMILY | DEV_88E6351_FAMILY |
>> > DEV_88E6352_FAMILY
>> > 
>> > Please limit access to these registers to just these families.
>> 
>> OK. Thanks for pointing this out. I think you meant SID instead of VID.
> 
> Yes, my error.
> 
>> Would something like the following be correct then?
>> 
>> #define GLOBAL_VTU_FID          0x02    /* 6097 6165 6351 6352 */
>> #define GLOBAL_VTU_SID          0x03    /* 6097 6165 6351 6352 */
> 
> Yes. I've not annotated all #defines, but here there is a clear
> overlap with the MAC address for some chips, so the comment is
> justified.
> 
>>     if (mv88e6xxx_6097_family(ds) || mv88e6xxx_6165_family(ds) ||
>>         mv88e6xxx_6351_family(ds) || mv88e6xxx_6352_family(ds)) {
>>         ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
>>         if (ret < 0)
>>             return ret;
>> 
>>         entry->fid = ret & GLOBAL_VTU_FID_MASK;
>> 
>>         ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
>>         if (ret < 0)
>>             return ret;
>> 
>>         entry->sid = ret & GLOBAL_VTU_SID_MASK;
>>     }
> 
> Maybe add an else to set sid and fid to 0?

Sure, done. Thanks for the reviews.

-v