LinuxLists.cc - [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

[permalink] [raw]

Subject: [PATCH net-next v2 06/10] net: dsa: debugfs: add port fdb

Add a debug filesystem "fdb" entry to query a port's hardware FDB
entries through the .port_fdb_dump switch operation.

This is really convenient to query directly the hardware or inspect DSA
or CPU links, since these ports are not exposed to userspace.

# cat port1/fdb
vid 0 12:34:56:78:90:ab unicast static

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index 7b299c9d9892..59c09a67bc23 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
*/

#include <linux/debugfs.h>
+#include <linux/etherdevice.h>
#include <linux/seq_file.h>

#include "dsa_priv.h"
@@ -109,6 +110,31 @@ static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
return 0;
}

+static int dsa_debugfs_fdb_dump_cb(const unsigned char *addr, u16 vid,
+ bool is_static, void *data)
+{
+ struct seq_file *seq = data;
+
+ seq_printf(seq, "vid %d %pM %s %s\n", vid, addr,
+ is_unicast_ether_addr(addr) ? "unicast" : "multicast",
+ is_static ? "static" : "dynamic");
+
+ return 0;
+}
+
+static int dsa_debugfs_fdb_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ if (!ds->ops->port_fdb_dump)
+ return -EOPNOTSUPP;
+
+ return ds->ops->port_fdb_dump(ds, id, dsa_debugfs_fdb_dump_cb, seq);
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_fdb_ops = {
+ .read = dsa_debugfs_fdb_read,
+};
+
static void dsa_debugfs_regs_read_count(struct dsa_switch *ds, int id,
struct seq_file *seq, int count)
{
@@ -222,6 +248,11 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
if (IS_ERR_OR_NULL(dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, dir, "fdb", port,
+ &dsa_debugfs_fdb_ops);
+ if (err)
+ return err;
+
err = dsa_debugfs_create_file(ds, dir, "regs", port,
&dsa_debugfs_regs_ops);
if (err)
--
2.14.1

2017-08-28 19:21:47

[permalink] [raw]

Subject: [PATCH net-next v2 05/10] net: dsa: debugfs: add port regs

Add a debug filesystem "regs" entry to query a port's hardware registers
through the .get_regs_len and .get_regs_len switch operations.

This is very convenient because it allows one to dump the registers of
DSA links, which are not exposed to userspace.

Here are the registers of a zii-rev-b CPU and DSA ports:

# pr -mt switch0/port{5,6}/regs
0: 4e07 0: 4d04
1: 403e 1: 003d
2: 0000 2: 0000
3: 3521 3: 3521
4: 0533 4: 373f
5: 8000 5: 0000
6: 005f 6: 003f
7: 002a 7: 002a
8: 2080 8: 2080
9: 0001 9: 0001
10: 0000 10: 0000
11: 0020 11: 0000
12: 0000 12: 0000
13: 0000 13: 0000
14: 0000 14: 0000
15: 9100 15: dada
16: 0000 16: 0000
17: 0000 17: 0000
18: 0000 18: 0000
19: 0000 19: 00d8
20: 0000 20: 0000
21: 0000 21: 0000
22: 0022 22: 0000
23: 0000 23: 0000
24: 3210 24: 3210
25: 7654 25: 7654
26: 0000 26: 0000
27: 8000 27: 8000
28: 0000 28: 0000
29: 0000 29: 0000
30: 0000 30: 0000
31: 0000 31: 0000

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index 997bbc8eb502..7b299c9d9892 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -109,6 +109,40 @@ static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
return 0;
}

+static void dsa_debugfs_regs_read_count(struct dsa_switch *ds, int id,
+ struct seq_file *seq, int count)
+{
+ u16 data[count * ETH_GSTRING_LEN];
+ struct ethtool_regs regs;
+ int i;
+
+ ds->ops->get_regs(ds, id, &regs, data);
+
+ for (i = 0; i < count / 2; i++)
+ seq_printf(seq, "%2d: %04x\n", i, data[i]);
+}
+
+static int dsa_debugfs_regs_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ int count;
+
+ if (!ds->ops->get_regs_len || !ds->ops->get_regs)
+ return -EOPNOTSUPP;
+
+ count = ds->ops->get_regs_len(ds, id);
+ if (count < 0)
+ return count;
+
+ dsa_debugfs_regs_read_count(ds, id, seq, count);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_regs_ops = {
+ .read = dsa_debugfs_regs_read,
+};
+
static void dsa_debugfs_stats_read_count(struct dsa_switch *ds, int id,
struct seq_file *seq, int count)
{
@@ -188,6 +222,11 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
if (IS_ERR_OR_NULL(dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, dir, "regs", port,
+ &dsa_debugfs_regs_ops);
+ if (err)
+ return err;
+
err = dsa_debugfs_create_file(ds, dir, "stats", port,
&dsa_debugfs_stats_ops);
if (err)
--
2.14.1

2017-08-28 19:21:45

[permalink] [raw]

Subject: [PATCH net-next v2 04/10] net: dsa: debugfs: add port stats

Add a debug filesystem "stats" entry to query a port's hardware
statistics through the DSA switch .get_sset_count, .get_strings and
.get_ethtool_stats operations.

This allows one to get statistics about DSA links interconnecting
switches, which is very convenient because this kind of port is not
exposed to userspace.

Here are the stats of a zii-rev-b DSA and CPU ports:

# pr -mt switch0/port{5,6}/stats
in_good_octets : 0 in_good_octets : 13824
in_bad_octets : 0 in_bad_octets : 0
in_unicast : 0 in_unicast : 0
in_broadcasts : 0 in_broadcasts : 216
in_multicasts : 0 in_multicasts : 0
in_pause : 0 in_pause : 0
in_undersize : 0 in_undersize : 0
in_fragments : 0 in_fragments : 0
in_oversize : 0 in_oversize : 0
in_jabber : 0 in_jabber : 0
in_rx_error : 0 in_rx_error : 0
in_fcs_error : 0 in_fcs_error : 0
out_octets : 9216 out_octets : 0
out_unicast : 0 out_unicast : 0
out_broadcasts : 144 out_broadcasts : 0
out_multicasts : 0 out_multicasts : 0
out_pause : 0 out_pause : 0
excessive : 0 excessive : 0
collisions : 0 collisions : 0
deferred : 0 deferred : 0
single : 0 single : 0
multiple : 0 multiple : 0
out_fcs_error : 0 out_fcs_error : 0
late : 0 late : 0
hist_64bytes : 0 hist_64bytes : 0
hist_65_127bytes : 0 hist_65_127bytes : 0
hist_128_255bytes : 0 hist_128_255bytes : 0
hist_256_511bytes : 0 hist_256_511bytes : 0
hist_512_1023bytes : 0 hist_512_1023bytes : 0
hist_1024_max_bytes : 0 hist_1024_max_bytes : 0
sw_in_discards : 0 sw_in_discards : 0
sw_in_filtered : 0 sw_in_filtered : 0
sw_out_filtered : 0 sw_out_filtered : 216

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index 8a0e4311ff8c..997bbc8eb502 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -109,6 +109,43 @@ static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
return 0;
}

+static void dsa_debugfs_stats_read_count(struct dsa_switch *ds, int id,
+ struct seq_file *seq, int count)
+{
+ u8 strings[count * ETH_GSTRING_LEN];
+ u64 stats[count];
+ int i;
+
+ ds->ops->get_strings(ds, id, strings);
+ ds->ops->get_ethtool_stats(ds, id, stats);
+
+ for (i = 0; i < count; i++)
+ seq_printf(seq, "%-20s: %lld\n", strings + i * ETH_GSTRING_LEN,
+ stats[i]);
+}
+
+static int dsa_debugfs_stats_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ int count;
+
+ if (!ds->ops->get_sset_count || !ds->ops->get_strings ||
+ !ds->ops->get_ethtool_stats)
+ return -EOPNOTSUPP;
+
+ count = ds->ops->get_sset_count(ds);
+ if (count < 0)
+ return count;
+
+ dsa_debugfs_stats_read_count(ds, id, seq, count);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_stats_ops = {
+ .read = dsa_debugfs_stats_read,
+};
+
static int dsa_debugfs_tag_protocol_read(struct dsa_switch *ds, int id,
struct seq_file *seq)
{
@@ -143,6 +180,7 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
char name[32];
+ int err;

snprintf(name, sizeof(name), DSA_PORT_FMT, port);

@@ -150,6 +188,11 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
if (IS_ERR_OR_NULL(dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, dir, "stats", port,
+ &dsa_debugfs_stats_ops);
+ if (err)
+ return err;
+
return 0;
}

--
2.14.1

2017-08-28 19:21:43

[permalink] [raw]

Subject: [PATCH net-next v2 03/10] net: dsa: debugfs: add tag_protocol

Add a debug filesystem "tag_protocol" entry to query the switch tagging
protocol through the .get_tag_protocol operation.

# cat switch1/tag_protocol
EDSA

To ease maintenance of tag protocols, add a dsa_tag_protocol_name helper
to the public API which to convert a tag protocol enum to a string.

Signed-off-by: Vivien Didelot <[email protected]>
---
include/net/dsa.h | 26 ++++++++++++++++++++++++++
net/dsa/debugfs.c | 23 +++++++++++++++++++++++
2 files changed, 49 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7341178319f5..1309ba0376ae 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -39,6 +39,32 @@ enum dsa_tag_protocol {
DSA_TAG_LAST, /* MUST BE LAST */
};

+static inline const char *dsa_tag_protocol_name(enum dsa_tag_protocol proto)
+{
+ switch (proto) {
+ case DSA_TAG_PROTO_NONE:
+ return "none";
+ case DSA_TAG_PROTO_BRCM:
+ return "BRCM";
+ case DSA_TAG_PROTO_DSA:
+ return "DSA";
+ case DSA_TAG_PROTO_EDSA:
+ return "EDSA";
+ case DSA_TAG_PROTO_KSZ:
+ return "KSZ";
+ case DSA_TAG_PROTO_LAN9303:
+ return "LAN9303";
+ case DSA_TAG_PROTO_MTK:
+ return "MTK";
+ case DSA_TAG_PROTO_QCA:
+ return "QCA";
+ case DSA_TAG_PROTO_TRAILER:
+ return "TRAILER";
+ default:
+ return "unknown";
+ }
+}
+
#define DSA_MAX_SWITCHES 4
#define DSA_MAX_PORTS 12

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index 54e97e05a9d7..8a0e4311ff8c 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -109,6 +109,24 @@ static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
return 0;
}

+static int dsa_debugfs_tag_protocol_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ enum dsa_tag_protocol proto;
+
+ if (!ds->ops->get_tag_protocol)
+ return -EOPNOTSUPP;
+
+ proto = ds->ops->get_tag_protocol(ds);
+ seq_printf(seq, "%s\n", dsa_tag_protocol_name(proto));
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tag_protocol_ops = {
+ .read = dsa_debugfs_tag_protocol_read,
+};
+
static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
struct seq_file *seq)
{
@@ -150,6 +168,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
if (IS_ERR_OR_NULL(ds->debugfs_dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tag_protocol", -1,
+ &dsa_debugfs_tag_protocol_ops);
+ if (err)
+ return err;
+
err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
&dsa_debugfs_tree_ops);
if (err)
--
2.14.1

2017-08-28 19:22:53

[permalink] [raw]

Subject: [PATCH net-next v2 09/10] net: dsa: restore VLAN dump

This commit defines a dsa_vlan_dump_cb_t callback, similar to the FDB
dump callback and partly reverts commit a0b6b8c9fa3c ("net: dsa: Remove
support for vlan dump from DSA's drivers") to restore the DSA drivers
VLAN dump operations.

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 41 ++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_priv.h | 2 ++
drivers/net/dsa/bcm_sf2.c | 1 +
drivers/net/dsa/dsa_loop.c | 38 ++++++++++++++++++++++++++
drivers/net/dsa/microchip/ksz_common.c | 41 ++++++++++++++++++++++++++++
drivers/net/dsa/mv88e6xxx/chip.c | 49 ++++++++++++++++++++++++++++++++++
include/net/dsa.h | 5 ++++
7 files changed, 177 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f3679f33d..be0c5fa8bd9b 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1053,6 +1053,46 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
}
EXPORT_SYMBOL(b53_vlan_del);

+int b53_vlan_dump(struct dsa_switch *ds, int port, dsa_vlan_dump_cb_t *cb,
+ void *data)
+{
+ struct b53_device *dev = ds->priv;
+ u16 vid, vid_start = 0, pvid;
+ struct b53_vlan *vl;
+ bool untagged;
+ int err = 0;
+
+ if (is5325(dev) || is5365(dev))
+ vid_start = 1;
+
+ b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
+
+ /* Use our software cache for dumps, since we do not have any HW
+ * operation returning only the used/valid VLANs
+ */
+ for (vid = vid_start; vid < dev->num_vlans; vid++) {
+ vl = &dev->vlans[vid];
+
+ if (!vl->valid)
+ continue;
+
+ if (!(vl->members & BIT(port)))
+ continue;
+
+ untagged = false;
+
+ if (vl->untag & BIT(port))
+ untagged = true;
+
+ err = cb(vid, pvid == vid, untagged, data);
+ if (err)
+ break;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL(b53_vlan_dump);
+
/* Address Resolution Logic routines */
static int b53_arl_op_wait(struct b53_device *dev)
{
@@ -1503,6 +1543,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
.port_vlan_prepare = b53_vlan_prepare,
.port_vlan_add = b53_vlan_add,
.port_vlan_del = b53_vlan_del,
+ .port_vlan_dump = b53_vlan_dump,
.port_fdb_dump = b53_fdb_dump,
.port_fdb_add = b53_fdb_add,
.port_fdb_del = b53_fdb_del,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 01bd8cbe9a3f..2b3e59d80fdb 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -393,6 +393,8 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
struct switchdev_trans *trans);
int b53_vlan_del(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan);
+int b53_vlan_dump(struct dsa_switch *ds, int port, dsa_vlan_dump_cb_t *cb,
+ void *data);
int b53_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid);
int b53_fdb_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index bbcb4053e04e..1907b27297c3 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1021,6 +1021,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
.port_vlan_prepare = b53_vlan_prepare,
.port_vlan_add = b53_vlan_add,
.port_vlan_del = b53_vlan_del,
+ .port_vlan_dump = b53_vlan_dump,
.port_fdb_dump = b53_fdb_dump,
.port_fdb_add = b53_fdb_add,
.port_fdb_del = b53_fdb_del,
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 7819a9fe8321..0407533f725f 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -257,6 +257,43 @@ static int dsa_loop_port_vlan_del(struct dsa_switch *ds, int port,
return 0;
}

+static int dsa_loop_port_vlan_dump(struct dsa_switch *ds, int port,
+ dsa_vlan_dump_cb_t *cb, void *data)
+{
+ struct dsa_loop_priv *ps = ds->priv;
+ struct mii_bus *bus = ps->bus;
+ struct dsa_loop_vlan *vl;
+ u16 vid, vid_start = 0;
+ bool pvid, untagged;
+ int err = 0;
+
+ dev_dbg(ds->dev, "%s\n", __func__);
+
+ /* Just do a sleeping operation to make lockdep checks effective */
+ mdiobus_read(bus, ps->port_base + port, MII_BMSR);
+
+ for (vid = vid_start; vid < DSA_LOOP_VLANS; vid++) {
+ vl = &ps->vlans[vid];
+
+ if (!(vl->members & BIT(port)))
+ continue;
+
+ untagged = false;
+ pvid = false;
+
+ if (vl->untagged & BIT(port))
+ untagged = true;
+ if (ps->pvid == vid)
+ pvid = true;
+
+ err = cb(vid, pvid, untagged, data);
+ if (err)
+ break;
+ }
+
+ return err;
+}
+
static const struct dsa_switch_ops dsa_loop_driver = {
.get_tag_protocol = dsa_loop_get_protocol,
.setup = dsa_loop_setup,
@@ -273,6 +310,7 @@ static const struct dsa_switch_ops dsa_loop_driver = {
.port_vlan_prepare = dsa_loop_port_vlan_prepare,
.port_vlan_add = dsa_loop_port_vlan_add,
.port_vlan_del = dsa_loop_port_vlan_del,
+ .port_vlan_dump = dsa_loop_port_vlan_dump,
};

static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 56cd6d365352..52c7849acc3c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -638,6 +638,46 @@ static int ksz_port_vlan_del(struct dsa_switch *ds, int port,
return 0;
}

+static int ksz_port_vlan_dump(struct dsa_switch *ds, int port,
+ dsa_vlan_dump_cb_t *cb, void *data)
+{
+ struct ksz_device *dev = ds->priv;
+ struct vlan_table *vlan_cache;
+ bool pvid, untagged;
+ u16 val;
+ int vid;
+ int err = 0;
+
+ mutex_lock(&dev->vlan_mutex);
+
+ /* use dev->vlan_cache due to lack of searching valid vlan entry */
+ for (vid = 0; vid < dev->num_vlans; vid++) {
+ vlan_cache = &dev->vlan_cache[vid];
+
+ if (!(vlan_cache->table[0] & VLAN_VALID))
+ continue;
+
+ untagged = false;
+ pvid = false;
+
+ if (vlan_cache->table[2] & BIT(port)) {
+ if (vlan_cache->table[1] & BIT(port))
+ untagged = true;
+ ksz_pread16(dev, port, REG_PORT_DEFAULT_VID, &val);
+ if (vid == (val & 0xFFFFF))
+ pvid = true;
+
+ err = cb(vid, pvid, untagged, data);
+ if (err)
+ break;
+ }
+ }
+
+ mutex_unlock(&dev->vlan_mutex);
+
+ return err;
+}
+
struct alu_struct {
/* entry 1 */
u8 is_static:1;
@@ -1068,6 +1108,7 @@ static const struct dsa_switch_ops ksz_switch_ops = {
.port_vlan_prepare = ksz_port_vlan_prepare,
.port_vlan_add = ksz_port_vlan_add,
.port_vlan_del = ksz_port_vlan_del,
+ .port_vlan_dump = ksz_port_vlan_dump,
.port_fdb_dump = ksz_port_fdb_dump,
.port_fdb_add = ksz_port_fdb_add,
.port_fdb_del = ksz_port_fdb_del,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c66204423641..3717ae098d58 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1011,6 +1011,54 @@ static int mv88e6xxx_vtu_loadpurge(struct mv88e6xxx_chip *chip,
return chip->info->ops->vtu_loadpurge(chip, entry);
}

+static int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port,
+ dsa_vlan_dump_cb_t *cb, void *data)
+{
+ struct mv88e6xxx_chip *chip = ds->priv;
+ struct mv88e6xxx_vtu_entry next = {
+ .vid = chip->info->max_vid,
+ };
+ bool untagged;
+ u16 pvid;
+ int err;
+
+ if (!chip->info->max_vid)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&chip->reg_lock);
+
+ err = mv88e6xxx_port_get_pvid(chip, port, &pvid);
+ if (err)
+ goto unlock;
+
+ do {
+ err = mv88e6xxx_vtu_getnext(chip, &next);
+ if (err)
+ break;
+
+ if (!next.valid)
+ break;
+
+ if (next.member[port] ==
+ MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_NON_MEMBER)
+ continue;
+
+ untagged = false;
+ if (next.member[port] ==
+ MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED)
+ untagged = true;
+
+ err = cb(next.vid, next.vid == pvid, untagged, data);
+ if (err)
+ break;
+ } while (next.vid < chip->info->max_vid);
+
+unlock:
+ mutex_unlock(&chip->reg_lock);
+
+ return err;
+}
+
static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
{
DECLARE_BITMAP(fid_bitmap, MV88E6XXX_N_FID);
@@ -3820,6 +3868,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
.port_vlan_prepare = mv88e6xxx_port_vlan_prepare,
.port_vlan_add = mv88e6xxx_port_vlan_add,
.port_vlan_del = mv88e6xxx_port_vlan_del,
+ .port_vlan_dump = mv88e6xxx_port_vlan_dump,
.port_fdb_add = mv88e6xxx_port_fdb_add,
.port_fdb_del = mv88e6xxx_port_fdb_del,
.port_fdb_dump = mv88e6xxx_port_fdb_dump,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index c0d1b6c47a50..b4994c58547f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -315,6 +315,8 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
/* FDB (and MDB) dump callback */
typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
bool is_static, void *data);
+typedef int dsa_vlan_dump_cb_t(u16 vid, bool pvid, bool untagged, void *data);
+
struct dsa_switch_ops {
/*
* Legacy probing.
@@ -421,6 +423,9 @@ struct dsa_switch_ops {
struct switchdev_trans *trans);
int (*port_vlan_del)(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan);
+ int (*port_vlan_dump)(struct dsa_switch *ds, int port,
+ dsa_vlan_dump_cb_t *cb, void *data);
+
/*
* Forwarding database
*/
--
2.14.1

2017-08-28 19:23:21

[permalink] [raw]

Subject: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.

# cat switch1/tree
0

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
*/

#include <linux/debugfs.h>
+#include <linux/seq_file.h>

#include "dsa_priv.h"

@@ -19,6 +20,107 @@
/* DSA module debugfs directory */
static struct dentry *dsa_debugfs_dir;

+struct dsa_debugfs_ops {
+ int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+ int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+ const struct dsa_debugfs_ops *ops;
+ struct dsa_switch *ds;
+ int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->read)
+ return -EOPNOTSUPP;
+
+ return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *seq = file->private_data;
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+ char buf[count + 1];
+ int err;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->write)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+
+ buf[count] = '\0';
+
+ err = priv->ops->write(ds, priv->id, buf);
+
+ return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+ .open = dsa_debugfs_open,
+ .read = seq_read,
+ .write = dsa_debugfs_write,
+ .llseek = no_llseek,
+ .release = single_release,
+ .owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+ char *name, int id,
+ const struct dsa_debugfs_ops *ops)
+{
+ struct dsa_debugfs_priv *priv;
+ struct dentry *entry;
+ umode_t mode;
+
+ priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ops = ops;
+ priv->ds = ds;
+ priv->id = id;
+
+ mode = 0;
+ if (ops->read)
+ mode |= 0444;
+ if (ops->write)
+ mode |= 0200;
+
+ entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+ if (IS_ERR_OR_NULL(entry))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ seq_printf(seq, "%d\n", ds->dst->tree);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+ .read = dsa_debugfs_tree_read,
+};
+
static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
if (IS_ERR_OR_NULL(ds->debugfs_dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+ &dsa_debugfs_tree_ops);
+ if (err)
+ return err;
+
for (i = 0; i < ds->num_ports; i++) {
if (ds->enabled_port_mask & BIT(i)) {
err = dsa_debugfs_create_port(ds, i);
--
2.14.1

2017-08-28 19:23:19

[permalink] [raw]

Subject: [PATCH net-next v2 10/10] net: dsa: debugfs: add port vlan

Add a debug filesystem "vlan" entry to query a port's hardware VLAN
entries through the .port_vlan_dump switch operation.

This is really convenient to query directly the hardware or inspect DSA
or CPU links, since these ports are not exposed to userspace.

Here are the VLAN entries for a CPU port:

# cat port5/vlan
vid 1 untagged pvid
vid 42 tagged

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index bed8e1d5cfe1..40fe19872ab1 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -250,6 +250,30 @@ static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
.read = dsa_debugfs_tree_read,
};

+static int dsa_debugfs_vlan_dump_cb(u16 vid, bool pvid, bool untagged,
+ void *data)
+{
+ struct seq_file *seq = data;
+
+ seq_printf(seq, "vid %d %s %s\n", vid,
+ untagged ? "untagged" : "tagged", pvid ? "pvid" : "");
+
+ return 0;
+}
+
+static int dsa_debugfs_vlan_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ if (!ds->ops->port_vlan_dump)
+ return -EOPNOTSUPP;
+
+ return ds->ops->port_vlan_dump(ds, id, dsa_debugfs_vlan_dump_cb, seq);
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_vlan_ops = {
+ .read = dsa_debugfs_vlan_read,
+};
+
static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
@@ -282,6 +306,11 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
if (err)
return err;

+ err = dsa_debugfs_create_file(ds, dir, "vlan", port,
+ &dsa_debugfs_vlan_ops);
+ if (err)
+ return err;
+
return 0;
}

--
2.14.1

2017-08-28 19:23:17

[permalink] [raw]

Subject: [PATCH net-next v2 08/10] net: dsa: debugfs: add port mdb

Add a debug filesystem "mdb" entry to query a port's hardware MDB
entries through the .port_mdb_dump switch operation.

This is really convenient to query directly the hardware or inspect DSA
or CPU links, since these ports are not exposed to userspace.

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index 59c09a67bc23..bed8e1d5cfe1 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -135,6 +135,20 @@ static const struct dsa_debugfs_ops dsa_debugfs_fdb_ops = {
.read = dsa_debugfs_fdb_read,
};

+static int dsa_debugfs_mdb_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ if (!ds->ops->port_mdb_dump)
+ return -EOPNOTSUPP;
+
+ /* same callback as for FDB dump */
+ return ds->ops->port_mdb_dump(ds, id, dsa_debugfs_fdb_dump_cb, seq);
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_mdb_ops = {
+ .read = dsa_debugfs_mdb_read,
+};
+
static void dsa_debugfs_regs_read_count(struct dsa_switch *ds, int id,
struct seq_file *seq, int count)
{
@@ -253,6 +267,11 @@ static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
if (err)
return err;

+ err = dsa_debugfs_create_file(ds, dir, "mdb", port,
+ &dsa_debugfs_mdb_ops);
+ if (err)
+ return err;
+
err = dsa_debugfs_create_file(ds, dir, "regs", port,
&dsa_debugfs_regs_ops);
if (err)
--
2.14.1

2017-08-28 19:24:39

[permalink] [raw]

Subject: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

This commit adds a DEBUG_FS dependent DSA core file creating a generic
debug filesystem interface for the DSA switch devices.

The interface can be mounted with:

# mount -t debugfs none /sys/kernel/debug

The dsa directory contains one directory per switch chip:

# cd /sys/kernel/debug/dsa/
# ls
switch0 switch1 switch2

Each chip directory contains one directory per port:

# ls -l switch0/
drwxr-xr-x 2 root root 0 Jan 1 00:00 port0
drwxr-xr-x 2 root root 0 Jan 1 00:00 port1
drwxr-xr-x 2 root root 0 Jan 1 00:00 port2
drwxr-xr-x 2 root root 0 Jan 1 00:00 port5
drwxr-xr-x 2 root root 0 Jan 1 00:00 port6

Future patches will add entry files to these directories.

Signed-off-by: Vivien Didelot <[email protected]>
---
include/net/dsa.h | 7 ++++
net/dsa/Kconfig | 14 +++++++
net/dsa/Makefile | 1 +
net/dsa/debugfs.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
net/dsa/dsa.c | 3 ++
net/dsa/dsa2.c | 4 ++
net/dsa/dsa_priv.h | 13 ++++++
net/dsa/legacy.c | 4 ++
8 files changed, 164 insertions(+)
create mode 100644 net/dsa/debugfs.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 398ca8d70ccd..7341178319f5 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -210,6 +210,13 @@ struct dsa_switch {
*/
void *priv;

+#ifdef CONFIG_NET_DSA_DEBUGFS
+ /*
+ * Debugfs interface.
+ */
+ struct dentry *debugfs_dir;
+#endif
+
/*
* Configuration data for this switch.
*/
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index cc5f8f971689..0f05a1e59dd2 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -15,6 +15,20 @@ config NET_DSA

if NET_DSA

+config NET_DSA_DEBUGFS
+ bool "Distributed Switch Architecture debugfs interface"
+ depends on DEBUG_FS
+ ---help---
+ Enable creation of debugfs files for the DSA core.
+
+ These debugfs files provide per-switch information, such as the tag
+ protocol in use and ports connectivity. They also allow querying the
+ hardware directly through the switch operations for debugging instead
+ of going through the bridge, switchdev and DSA layers.
+
+ This is also a way to inspect the stats and FDB, MDB or VLAN entries
+ of CPU and DSA links, since they are not exposed to userspace.
+
# tagging formats
config NET_DSA_TAG_BRCM
bool
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index fcce25da937c..7f60c6dfaffb 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,6 +1,7 @@
# the core
obj-$(CONFIG_NET_DSA) += dsa_core.o
dsa_core-y += dsa.o dsa2.o legacy.o port.o slave.o switch.o
+dsa_core-$(CONFIG_NET_DSA_DEBUGFS) += debugfs.o

# tagging formats
dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
new file mode 100644
index 000000000000..b6b5e5c97389
--- /dev/null
+++ b/net/dsa/debugfs.c
@@ -0,0 +1,118 @@
+/*
+ * net/dsa/debugfs.c - DSA debugfs interface
+ * Copyright (c) 2017 Savoir-faire Linux, Inc.
+ * Vivien Didelot <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/debugfs.h>
+
+#include "dsa_priv.h"
+
+#define DSA_SWITCH_FMT "switch%d"
+#define DSA_PORT_FMT "port%d"
+
+/* DSA module debugfs directory */
+static struct dentry *dsa_debugfs_dir;
+
+static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
+{
+ struct dentry *dir;
+ char name[32];
+
+ snprintf(name, sizeof(name), DSA_PORT_FMT, port);
+
+ dir = debugfs_create_dir(name, ds->debugfs_dir);
+ if (IS_ERR_OR_NULL(dir))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int dsa_debugfs_create_switch(struct dsa_switch *ds)
+{
+ char name[32];
+ int i, err;
+
+ /* skip if there is no debugfs support */
+ if (!dsa_debugfs_dir)
+ return 0;
+
+ snprintf(name, sizeof(name), DSA_SWITCH_FMT, ds->index);
+
+ ds->debugfs_dir = debugfs_create_dir(name, dsa_debugfs_dir);
+ if (IS_ERR_OR_NULL(ds->debugfs_dir))
+ return -EFAULT;
+
+ for (i = 0; i < ds->num_ports; i++) {
+ if (ds->enabled_port_mask & BIT(i)) {
+ err = dsa_debugfs_create_port(ds, i);
+ if (err)
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+static void dsa_debugfs_destroy_switch(struct dsa_switch *ds)
+{
+ /* handles NULL */
+ debugfs_remove_recursive(ds->debugfs_dir);
+}
+
+void dsa_debugfs_create_tree(struct dsa_switch_tree *dst)
+{
+ struct dsa_switch *ds;
+ int i, err;
+
+ for (i = 0; i < DSA_MAX_SWITCHES; i++) {
+ ds = dst->ds[i];
+ if (!ds)
+ continue;
+
+ err = dsa_debugfs_create_switch(ds);
+ if (err) {
+ pr_warn("DSA: failed to create debugfs interface for switch %d (%d)\n",
+ ds->index, err);
+ dsa_debugfs_destroy_tree(dst);
+ break;
+ }
+ }
+}
+
+void dsa_debugfs_destroy_tree(struct dsa_switch_tree *dst)
+{
+ struct dsa_switch *ds;
+ int i;
+
+ for (i = 0; i < DSA_MAX_SWITCHES; i++) {
+ ds = dst->ds[i];
+ if (!ds)
+ continue;
+
+ dsa_debugfs_destroy_switch(ds);
+ }
+}
+
+void dsa_debugfs_create_module(void)
+{
+ dsa_debugfs_dir = debugfs_create_dir("dsa", NULL);
+ if (IS_ERR(dsa_debugfs_dir)) {
+ pr_warn("DSA: failed to create debugfs interface\n");
+ dsa_debugfs_dir = NULL;
+ }
+
+ if (dsa_debugfs_dir)
+ pr_info("DSA: debugfs interface created\n");
+}
+
+void dsa_debugfs_destroy_module(void)
+{
+ /* handles NULL */
+ debugfs_remove_recursive(dsa_debugfs_dir);
+}
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 03c58b0eb082..b23f1be50c71 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -308,12 +308,15 @@ static int __init dsa_init_module(void)

dev_add_pack(&dsa_pack_type);

+ dsa_debugfs_create_module();
+
return 0;
}
module_init(dsa_init_module);

static void __exit dsa_cleanup_module(void)
{
+ dsa_debugfs_destroy_module();
dsa_slave_unregister_notifier();
dev_remove_pack(&dsa_pack_type);
dsa_legacy_unregister();
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cceaa4dd9f53..5912618ad63d 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -447,6 +447,8 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
dst->cpu_dp->netdev->dsa_ptr = dst;
dst->applied = true;

+ dsa_debugfs_create_tree(dst);
+
return 0;
}

@@ -458,6 +460,8 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
if (!dst->applied)
return;

+ dsa_debugfs_destroy_tree(dst);
+
dst->cpu_dp->netdev->dsa_ptr = NULL;

/* If we used a tagging format that doesn't have an ethertype
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9c3eeb72462d..84ca3a50a58b 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -93,6 +93,19 @@ struct dsa_slave_priv {
struct list_head mall_tc_list;
};

+/* debugfs.c */
+#ifdef CONFIG_NET_DSA_DEBUGFS
+void dsa_debugfs_create_module(void);
+void dsa_debugfs_destroy_module(void);
+void dsa_debugfs_create_tree(struct dsa_switch_tree *dst);
+void dsa_debugfs_destroy_tree(struct dsa_switch_tree *dst);
+#else
+static inline void dsa_debugfs_create_module(void) { }
+static inline void dsa_debugfs_destroy_module(void) { }
+static inline void dsa_debugfs_create_tree(struct dsa_switch_tree *dst) { }
+static inline void dsa_debugfs_destroy_tree(struct dsa_switch_tree *dst) { }
+#endif
+
/* dsa.c */
int dsa_cpu_dsa_setup(struct dsa_port *port);
void dsa_cpu_dsa_destroy(struct dsa_port *dport);
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 91e6f7981d39..8aa3de540552 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -606,6 +606,8 @@ static int dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
wmb();
dev->dsa_ptr = dst;

+ dsa_debugfs_create_tree(dst);
+
return 0;
}

@@ -671,6 +673,8 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
{
int i;

+ dsa_debugfs_destroy_tree(dst);
+
dst->cpu_dp->netdev->dsa_ptr = NULL;

/* If we used a tagging format that doesn't have an ethertype
--
2.14.1

2017-08-28 19:25:21

[permalink] [raw]

Subject: [PATCH net-next v2 07/10] net: dsa: restore mdb dump

The same dsa_fdb_dump_cb_t callback is used since there is no
distinction to do between FDB and MDB entries at this layer.

Implement mv88e6xxx_port_mdb_dump so that multicast addresses associated
to a switch port can be dumped.

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 33 +++++++++++++++++++++++++--------
include/net/dsa.h | 3 +++
2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6678aa9b4ef..c66204423641 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1380,7 +1380,7 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
}

static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
- u16 fid, u16 vid, int port,
+ u16 fid, u16 vid, int port, bool mc,
dsa_fdb_dump_cb_t *cb, void *data)
{
struct mv88e6xxx_atu_entry addr;
@@ -1401,11 +1401,14 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
if (addr.trunk || (addr.portvec & BIT(port)) == 0)
continue;

- if (!is_unicast_ether_addr(addr.mac))
+ if ((is_unicast_ether_addr(addr.mac) && mc) ||
+ (is_multicast_ether_addr(addr.mac) && !mc))
continue;

- is_static = (addr.state ==
- MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
+ is_static = addr.state == mc ?
+ MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC :
+ MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC;
+
err = cb(addr.mac, vid, is_static, data);
if (err)
return err;
@@ -1415,7 +1418,7 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
}

static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
- dsa_fdb_dump_cb_t *cb, void *data)
+ bool mc, dsa_fdb_dump_cb_t *cb, void *data)
{
struct mv88e6xxx_vtu_entry vlan = {
.vid = chip->info->max_vid,
@@ -1428,7 +1431,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
if (err)
return err;

- err = mv88e6xxx_port_db_dump_fid(chip, fid, 0, port, cb, data);
+ err = mv88e6xxx_port_db_dump_fid(chip, fid, 0, port, mc, cb, data);
if (err)
return err;

@@ -1442,7 +1445,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
break;

err = mv88e6xxx_port_db_dump_fid(chip, vlan.fid, vlan.vid, port,
- cb, data);
+ mc, cb, data);
if (err)
return err;
} while (vlan.vid < chip->info->max_vid);
@@ -1457,7 +1460,7 @@ static int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,
int err;

mutex_lock(&chip->reg_lock);
- err = mv88e6xxx_port_db_dump(chip, port, cb, data);
+ err = mv88e6xxx_port_db_dump(chip, port, false, cb, data);
mutex_unlock(&chip->reg_lock);

return err;
@@ -3777,6 +3780,19 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
return err;
}

+static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
+ dsa_fdb_dump_cb_t *cb, void *data)
+{
+ struct mv88e6xxx_chip *chip = ds->priv;
+ int err;
+
+ mutex_lock(&chip->reg_lock);
+ err = mv88e6xxx_port_db_dump(chip, port, true, cb, data);
+ mutex_unlock(&chip->reg_lock);
+
+ return err;
+}
+
static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
.probe = mv88e6xxx_drv_probe,
.get_tag_protocol = mv88e6xxx_get_tag_protocol,
@@ -3810,6 +3826,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
.port_mdb_prepare = mv88e6xxx_port_mdb_prepare,
.port_mdb_add = mv88e6xxx_port_mdb_add,
.port_mdb_del = mv88e6xxx_port_mdb_del,
+ .port_mdb_dump = mv88e6xxx_port_mdb_dump,
.crosschip_bridge_join = mv88e6xxx_crosschip_bridge_join,
.crosschip_bridge_leave = mv88e6xxx_crosschip_bridge_leave,
};
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 1309ba0376ae..c0d1b6c47a50 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -312,6 +312,7 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
return ds->rtable[dst->cpu_dp->ds->index];
}

+/* FDB (and MDB) dump callback */
typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
bool is_static, void *data);
struct dsa_switch_ops {
@@ -441,6 +442,8 @@ struct dsa_switch_ops {
struct switchdev_trans *trans);
int (*port_mdb_del)(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_mdb *mdb);
+ int (*port_mdb_dump)(struct dsa_switch *ds, int port,
+ dsa_fdb_dump_cb_t *cb, void *data);
/*
* RXNFC
*/
--
2.14.1

2017-08-28 19:50:44

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

Mon, Aug 28, 2017 at 09:17:39PM CEST, [email protected] wrote:
>This commit adds a DEBUG_FS dependent DSA core file creating a generic
>debug filesystem interface for the DSA switch devices.
>
>The interface can be mounted with:
>
> # mount -t debugfs none /sys/kernel/debug
>
>The dsa directory contains one directory per switch chip:
>
> # cd /sys/kernel/debug/dsa/
> # ls
> switch0 switch1 switch2
>
>Each chip directory contains one directory per port:
>
> # ls -l switch0/
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port0
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port1
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port2
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port5
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port6
>
>Future patches will add entry files to these directories.
>
>Signed-off-by: Vivien Didelot <[email protected]>

Oh no, no debugfs please!

What do you need to expose? I'm sure we can find out some generic, well
defined and reusable way.

2017-08-28 19:53:36

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Mon, Aug 28, 2017 at 09:17:38PM CEST, [email protected] wrote:
>This patch series adds a generic debugfs interface for the DSA
>framework, so that all switch devices benefit from it, e.g. Marvell,
>Broadcom, Microchip or any other DSA driver.
>
>This is really convenient for debugging, especially CPU ports and DSA
>links which are not exposed to userspace as net device. This interface
>is currently the only way to easily inspect the hardware for such ports.
>
>With the patch series, any switch device user is able to query the
>hardware for the supported tagging protocol, the ports stats and
>registers, as well as their FDB, MDB and VLAN entries.

I see this overlaps a lot with DPIPE. Why won't you use that to expose
your hw state?

2017-08-28 19:58:22

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

On 08/28/2017 12:50 PM, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 09:17:39PM CEST, [email protected] wrote:
>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>> debug filesystem interface for the DSA switch devices.
>>
>> The interface can be mounted with:
>>
>> # mount -t debugfs none /sys/kernel/debug
>>
>> The dsa directory contains one directory per switch chip:
>>
>> # cd /sys/kernel/debug/dsa/
>> # ls
>> switch0 switch1 switch2
>>
>> Each chip directory contains one directory per port:
>>
>> # ls -l switch0/
>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port0
>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port1
>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port2
>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port5
>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port6
>>
>> Future patches will add entry files to these directories.
>>
>> Signed-off-by: Vivien Didelot <[email protected]>
>
> Oh no, no debugfs please!
>
> What do you need to expose? I'm sure we can find out some generic, well
> defined and reusable way.

We have no CPU or DSA (cross switches) net_device reprensentors because
those would be two ends of the same pipe so it would be both confusing
and a duplication. For a CPU interface, one side goes to the switch, the
other one is the master net_device (normal Ethernet MAC). For a DSA
interface, one interface is on one switch, and the other is on the other
switch.

If you look at the patch series it's pretty obvious what is being exposed :)
--
Florian

2017-08-28 20:05:05

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

Mon, Aug 28, 2017 at 09:58:12PM CEST, [email protected] wrote:
>On 08/28/2017 12:50 PM, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 09:17:39PM CEST, [email protected] wrote:
>>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>>> debug filesystem interface for the DSA switch devices.
>>>
>>> The interface can be mounted with:
>>>
>>> # mount -t debugfs none /sys/kernel/debug
>>>
>>> The dsa directory contains one directory per switch chip:
>>>
>>> # cd /sys/kernel/debug/dsa/
>>> # ls
>>> switch0 switch1 switch2
>>>
>>> Each chip directory contains one directory per port:
>>>
>>> # ls -l switch0/
>>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port0
>>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port1
>>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port2
>>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port5
>>> drwxr-xr-x 2 root root 0 Jan 1 00:00 port6
>>>
>>> Future patches will add entry files to these directories.
>>>
>>> Signed-off-by: Vivien Didelot <[email protected]>
>>
>> Oh no, no debugfs please!
>>
>> What do you need to expose? I'm sure we can find out some generic, well
>> defined and reusable way.
>
>We have no CPU or DSA (cross switches) net_device reprensentors because
>those would be two ends of the same pipe so it would be both confusing

So? That is certainly not an argument for debugfs. Just have all ports
as devlink port, and you can introduce special new kind of port for cpu
port. Note that devlink port does not have to have netdev association.

>and a duplication. For a CPU interface, one side goes to the switch, the
>other one is the master net_device (normal Ethernet MAC). For a DSA
>interface, one interface is on one switch, and the other is on the other
>switch.
>
>If you look at the patch series it's pretty obvious what is being exposed :)

Sure. But lets use existing interfaces and extend them if needed. Please
don't use some made-up debugfs mess. That is never the correct answer :/

2017-08-28 20:08:46

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

> I see this overlaps a lot with DPIPE. Why won't you use that to expose
> your hw state?

We took a look at dpipe and i talked to you about using it for this
sort of thing at netconf/netdev. But dpipe has issues displaying the
sort of information we have. I never figured out how to do two
dimensional tables. The output of the dpipe command is pretty
unreadable. A lot of the information being dumped here is not about
the data pipe, etc.

There is a lot of pushback on debugfs for individual drivers. As i
said recently to somebody, debugfs is a bit of a wild west. When
designing this code, we thought about that. This debugfs is not at the
driver level. It is at the DSA level. All DSA drivers will benefit
from this code, and all DSA drivers will get the same information
exposed in debugfs. It is generic, well defined and structured, with
respect to DSA.

Andrew

2017-08-28 20:16:49

[permalink] [raw]

Subject: Re: [PATCH net-next v2 03/10] net: dsa: debugfs: add tag_protocol

On Mon, Aug 28, 2017 at 03:17:41PM -0400, Vivien Didelot wrote:
> Add a debug filesystem "tag_protocol" entry to query the switch tagging
> protocol through the .get_tag_protocol operation.
>
> # cat switch1/tag_protocol
> EDSA
>
> To ease maintenance of tag protocols, add a dsa_tag_protocol_name helper
> to the public API which to convert a tag protocol enum to a string.
>
> Signed-off-by: Vivien Didelot <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2017-08-28 20:19:12

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> This commit adds a DEBUG_FS dependent DSA core file creating a generic
> debug filesystem interface for the DSA switch devices.
>
> The interface can be mounted with:
>
> # mount -t debugfs none /sys/kernel/debug
>
> The dsa directory contains one directory per switch chip:
>
> # cd /sys/kernel/debug/dsa/
> # ls
> switch0 switch1 switch2
>
> Each chip directory contains one directory per port:
>
> # ls -l switch0/
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port0
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port1
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port2
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port5
> drwxr-xr-x 2 root root 0 Jan 1 00:00 port6
>
> Future patches will add entry files to these directories.
>
> Signed-off-by: Vivien Didelot <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2017-08-29 04:38:40

by David Miller

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

From: Vivien Didelot <[email protected]>
Date: Mon, 28 Aug 2017 15:17:38 -0400

> This patch series adds a generic debugfs interface for the DSA
> framework, so that all switch devices benefit from it, e.g. Marvell,
> Broadcom, Microchip or any other DSA driver.

I've been thinking this over and I agree with the feedback given that
debugfs really isn't appropriate for this.

Please create a DSA device class, and hang these values under
appropriate sysfs device nodes that can be easily found via
/sys/class/dsa/ just as easily as they would be /sys/kernel/debug/dsa/

You really intend these values to be consistent across DSA devices,
and you don't intend to go willy-nilly changig these exported values
arbitrarily over time. That's what debugfs is for, throw-away
stuff.

So please make these proper device sysfs attributes rather than
debugfs.

Thank you.

2017-08-29 06:25:31

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
>> I see this overlaps a lot with DPIPE. Why won't you use that to expose
>> your hw state?
>
>We took a look at dpipe and i talked to you about using it for this
>sort of thing at netconf/netdev. But dpipe has issues displaying the
>sort of information we have. I never figured out how to do two
>dimensional tables. The output of the dpipe command is pretty
>unreadable. A lot of the information being dumped here is not about
>the data pipe, etc.

So improve it. No problem. Also, we extend it to support what you neede.

>
>There is a lot of pushback on debugfs for individual drivers. As i
>said recently to somebody, debugfs is a bit of a wild west. When
>designing this code, we thought about that. This debugfs is not at the
>driver level. It is at the DSA level. All DSA drivers will benefit
>from this code, and all DSA drivers will get the same information
>exposed in debugfs. It is generic, well defined and structured, with
>respect to DSA.

Still, it has *a lot* of overlap with devlink and dpipe. So instead of
making devlink and dpipe work for you, you introduced completely
separated debugfs interface specific to a list of drivers. That is just
wrong. Debugfs is never the correct answer! Please work with us on
devlink and dpipe so they are used for all drivers, mlxsw, dsa and others.

Thanks!

2017-08-29 06:29:36

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Tue, Aug 29, 2017 at 06:38:37AM CEST, [email protected] wrote:
>From: Vivien Didelot <[email protected]>
>Date: Mon, 28 Aug 2017 15:17:38 -0400
>
>> This patch series adds a generic debugfs interface for the DSA
>> framework, so that all switch devices benefit from it, e.g. Marvell,
>> Broadcom, Microchip or any other DSA driver.
>
>I've been thinking this over and I agree with the feedback given that
>debugfs really isn't appropriate for this.
>
>Please create a DSA device class, and hang these values under
>appropriate sysfs device nodes that can be easily found via
>/sys/class/dsa/ just as easily as they would be /sys/kernel/debug/dsa/
>
>You really intend these values to be consistent across DSA devices,
>and you don't intend to go willy-nilly changig these exported values
>arbitrarily over time. That's what debugfs is for, throw-away
>stuff.
>
>So please make these proper device sysfs attributes rather than
>debugfs.

As I wrote, I believe that there is a big overlap with devlink and its
dpipe subset. I think that primary we should focus on extending whatever
is needed for dsa there. The iface should be generic for all drivers,
not only dsa. dsa-specific sysfs attributes should be last-resort solution,
I believe we can avoid them.

2017-08-29 16:01:13

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Hi David, Jiri,

Jiri Pirko <[email protected]> writes:

> Tue, Aug 29, 2017 at 06:38:37AM CEST, [email protected] wrote:
>>From: Vivien Didelot <[email protected]>
>>Date: Mon, 28 Aug 2017 15:17:38 -0400
>>
>>> This patch series adds a generic debugfs interface for the DSA
>>> framework, so that all switch devices benefit from it, e.g. Marvell,
>>> Broadcom, Microchip or any other DSA driver.
>>
>>I've been thinking this over and I agree with the feedback given that
>>debugfs really isn't appropriate for this.
>>
>>Please create a DSA device class, and hang these values under
>>appropriate sysfs device nodes that can be easily found via
>>/sys/class/dsa/ just as easily as they would be /sys/kernel/debug/dsa/
>>
>>You really intend these values to be consistent across DSA devices,
>>and you don't intend to go willy-nilly changig these exported values
>>arbitrarily over time. That's what debugfs is for, throw-away
>>stuff.
>>
>>So please make these proper device sysfs attributes rather than
>>debugfs.
>
> As I wrote, I believe that there is a big overlap with devlink and its
> dpipe subset. I think that primary we should focus on extending whatever
> is needed for dsa there. The iface should be generic for all drivers,
> not only dsa. dsa-specific sysfs attributes should be last-resort solution,
> I believe we can avoid them.

Please note that this interface is only meant to provide a _debug_ and
_development_ interface to DSA users. It is enableable at compile time
and can be ditched anytime we want, in contrary to other interfaces
which cannot be broken or changed because they are part of the ABI.

I see sysfs as a script-friendly way to access and configure kernel
structures, so I agree with Jiri that it doesn't seem appropriate.

Extending devlink is a good option for long term, but it'll take a bit
of time to extend data structures and not duplicate stats and regs
accesses for ports which have a net device attached to it or not.

In the meantime, I didn't find anything more useful and easier to debug
a switch fabric than dumping side-by-side stats of all ports part of the
data plane, for example like this:

# watch -n1 pr -mt {switch0/port5,switch0/port6,switch1/port5,switch1/port3}/stats

where ports 5 and 6 of both switches are DSA/CPU ports (without net
devices attached to them) and port3 is a user port. This way one can
easily see where and why packets get dropped.

We could keep this interface and simply ditch net/dsa/debugfs.c when a
convenient devlink alternative is in place.

Thanks,

Vivien

2017-08-29 12:50:13

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
> >> I see this overlaps a lot with DPIPE. Why won't you use that to expose
> >> your hw state?
> >
> >We took a look at dpipe and i talked to you about using it for this
> >sort of thing at netconf/netdev. But dpipe has issues displaying the
> >sort of information we have. I never figured out how to do two
> >dimensional tables. The output of the dpipe command is pretty
> >unreadable. A lot of the information being dumped here is not about
> >the data pipe, etc.
>
> So improve it. No problem. Also, we extend it to support what you neede.

Will i did try to do this back in March. And i failed.

Lets start with stats. Vivien gives an example on the cover letter:

# pr -mt switch0/port{5,6}/stats
in_good_octets : 0 in_good_octets : 13824
in_bad_octets : 0 in_bad_octets : 0
in_unicast : 0 in_unicast : 0
in_broadcasts : 0 in_broadcasts : 216
in_multicasts : 0 in_multicasts : 0
in_pause : 0 in_pause : 0
in_undersize : 0 in_undersize : 0

This is what i tried to implement using dpipe. It is a simple two
dimensional table. First column is a string, second a u64. In debugfs
we have such a table per port. That fits with the hierarchy that each
port is a directory in debugfs. But it could also be implemented as
one table with N+1 columns, for N switch ports.

How about you, or one of your team, implement that. It should be able
to use the dsa_loop driver, which is a simple dummy switch. But it
does have statistics counters for all ports. Florian or I can help you
get it running if needed.

This branch contains some of the basic plumbing code from my previous
attempt:

https://github.com/lunn/linux.git v4.11-rc4-net-next-dpipe

Andrew

2017-08-29 19:05:26

by Arkadi Sharshevsky

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

On 08/29/2017 03:50 PM, Andrew Lunn wrote:
> On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
>>>> I see this overlaps a lot with DPIPE. Why won't you use that to expose
>>>> your hw state?
>>>
>>> We took a look at dpipe and i talked to you about using it for this
>>> sort of thing at netconf/netdev. But dpipe has issues displaying the
>>> sort of information we have. I never figured out how to do two
>>> dimensional tables. The output of the dpipe command is pretty
>>> unreadable. A lot of the information being dumped here is not about
>>> the data pipe, etc.
>>
>> So improve it. No problem. Also, we extend it to support what you neede.
>
> Will i did try to do this back in March. And i failed.
>
> Lets start with stats. Vivien gives an example on the cover letter:
>
> # pr -mt switch0/port{5,6}/stats
> in_good_octets : 0 in_good_octets : 13824
> in_bad_octets : 0 in_bad_octets : 0
> in_unicast : 0 in_unicast : 0
> in_broadcasts : 0 in_broadcasts : 216
> in_multicasts : 0 in_multicasts : 0
> in_pause : 0 in_pause : 0
> in_undersize : 0 in_undersize : 0
>
> This is what i tried to implement using dpipe. It is a simple two
> dimensional table. First column is a string, second a u64. In debugfs
> we have such a table per port. That fits with the hierarchy that each
> port is a directory in debugfs. But it could also be implemented as
> one table with N+1 columns, for N switch ports.
>

Hi Andrew,

This looks to me like basic L2 statistics that are obtained via
ethtool, I remember you had this problem with the DSA and CPU port.
Is this still the case?

I remembered we wanted to use dpipe for the DSA routing table
and IP priority table.

I think both those processes really look like match/action table
, thus they can be modeled successfully by dpipe.

> How about you, or one of your team, implement that. It should be able
> to use the dsa_loop driver, which is a simple dummy switch. But it
> does have statistics counters for all ports. Florian or I can help you
> get it running if needed.
>
> This branch contains some of the basic plumbing code from my previous
> attempt:
>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flunn%2Flinux.git&data=02%7C01%7Carkadis%40mellanox.com%7Cb3cac139af204f79259c08d4eedc8410%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636396078291326351&sdata=K5D3TAb2spckuF5k88oOaVt0dmtHj0AwE8bEEGPPdGI%3D&reserved=0 v4.11-rc4-net-next-dpipe
>
> Andrew
>

2017-08-29 19:19:22

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

On 08/29/2017 12:05 PM, Arkadi Sharshevsky wrote:
>
>
> On 08/29/2017 03:50 PM, Andrew Lunn wrote:
>> On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
>>> Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
>>>>> I see this overlaps a lot with DPIPE. Why won't you use that to expose
>>>>> your hw state?
>>>>
>>>> We took a look at dpipe and i talked to you about using it for this
>>>> sort of thing at netconf/netdev. But dpipe has issues displaying the
>>>> sort of information we have. I never figured out how to do two
>>>> dimensional tables. The output of the dpipe command is pretty
>>>> unreadable. A lot of the information being dumped here is not about
>>>> the data pipe, etc.
>>>
>>> So improve it. No problem. Also, we extend it to support what you neede.
>>
>> Will i did try to do this back in March. And i failed.
>>
>> Lets start with stats. Vivien gives an example on the cover letter:
>>
>> # pr -mt switch0/port{5,6}/stats
>> in_good_octets : 0 in_good_octets : 13824
>> in_bad_octets : 0 in_bad_octets : 0
>> in_unicast : 0 in_unicast : 0
>> in_broadcasts : 0 in_broadcasts : 216
>> in_multicasts : 0 in_multicasts : 0
>> in_pause : 0 in_pause : 0
>> in_undersize : 0 in_undersize : 0
>>
>> This is what i tried to implement using dpipe. It is a simple two
>> dimensional table. First column is a string, second a u64. In debugfs
>> we have such a table per port. That fits with the hierarchy that each
>> port is a directory in debugfs. But it could also be implemented as
>> one table with N+1 columns, for N switch ports.
>>
>
> Hi Andrew,
>
> This looks to me like basic L2 statistics that are obtained via
> ethtool, I remember you had this problem with the DSA and CPU port.
> Is this still the case?

Yes, there are no net_device representors for CPU and DSA ports because
if we did that, it would be confusing as we would be creating two
network devices for both ends of the pipe. For DSA (inter-switch)
interfaces you would have one "dsa" netdev for each adjacent switch so
two DSA interface represent the inter switch link.

For the "CPU" port, you have the master network device (e.g: eth0) and
the "cpu" network device, this is confusing. "cpu" is not usable, since
it does not make sense for the "cpu" to send traffic via this interface,
the model is to terminate user-facing ports and use a tag to deliver
packets to the right interfaces. For "dsa" it's pretty much the same story.

>
> I remembered we wanted to use dpipe for the DSA routing table
> and IP priority table.
>
> I think both those processes really look like match/action table
> , thus they can be modeled successfully by dpipe.
>
>> How about you, or one of your team, implement that. It should be able
>> to use the dsa_loop driver, which is a simple dummy switch. But it
>> does have statistics counters for all ports. Florian or I can help you
>> get it running if needed.
>>
>> This branch contains some of the basic plumbing code from my previous
>> attempt:
>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flunn%2Flinux.git&data=02%7C01%7Carkadis%40mellanox.com%7Cb3cac139af204f79259c08d4eedc8410%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636396078291326351&sdata=K5D3TAb2spckuF5k88oOaVt0dmtHj0AwE8bEEGPPdGI%3D&reserved=0 v4.11-rc4-net-next-dpipe
>>
>> Andrew
>>

--
Florian

2017-08-29 20:27:40

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

On Tue, Aug 29, 2017 at 12:19:08PM -0700, Florian Fainelli wrote:
> On 08/29/2017 12:05 PM, Arkadi Sharshevsky wrote:
> >
> >
> > On 08/29/2017 03:50 PM, Andrew Lunn wrote:
> >> On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
> >>> Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
> >>>>> I see this overlaps a lot with DPIPE. Why won't you use that to expose
> >>>>> your hw state?
> >>>>
> >>>> We took a look at dpipe and i talked to you about using it for this
> >>>> sort of thing at netconf/netdev. But dpipe has issues displaying the
> >>>> sort of information we have. I never figured out how to do two
> >>>> dimensional tables. The output of the dpipe command is pretty
> >>>> unreadable. A lot of the information being dumped here is not about
> >>>> the data pipe, etc.
> >>>
> >>> So improve it. No problem. Also, we extend it to support what you neede.
> >>
> >> Will i did try to do this back in March. And i failed.
> >>
> >> Lets start with stats. Vivien gives an example on the cover letter:
> >>
> >> # pr -mt switch0/port{5,6}/stats
> >> in_good_octets : 0 in_good_octets : 13824
> >> in_bad_octets : 0 in_bad_octets : 0
> >> in_unicast : 0 in_unicast : 0
> >> in_broadcasts : 0 in_broadcasts : 216
> >> in_multicasts : 0 in_multicasts : 0
> >> in_pause : 0 in_pause : 0
> >> in_undersize : 0 in_undersize : 0
> >>
> >> This is what i tried to implement using dpipe. It is a simple two
> >> dimensional table. First column is a string, second a u64. In debugfs
> >> we have such a table per port. That fits with the hierarchy that each
> >> port is a directory in debugfs. But it could also be implemented as
> >> one table with N+1 columns, for N switch ports.
> >>
> >
> > Hi Andrew,
> >
> > This looks to me like basic L2 statistics that are obtained via
> > ethtool, I remember you had this problem with the DSA and CPU port.
> > Is this still the case?
>
> Yes, there are no net_device representors for CPU and DSA ports because
> if we did that, it would be confusing as we would be creating two
> network devices for both ends of the pipe. For DSA (inter-switch)
> interfaces you would have one "dsa" netdev for each adjacent switch so
> two DSA interface represent the inter switch link.
>
> For the "CPU" port, you have the master network device (e.g: eth0) and
> the "cpu" network device, this is confusing. "cpu" is not usable, since
> it does not make sense for the "cpu" to send traffic via this interface,
> the model is to terminate user-facing ports and use a tag to deliver
> packets to the right interfaces. For "dsa" it's pretty much the same story.

The point of the story is that ethtool does not cover this use
case. We need a different way to expose these statistics for
debugging, and ideally the statistics for all the ports, not just DSA
and CPU.

> > I remembered we wanted to use dpipe for the DSA routing table
> > and IP priority table.

No, we wanted to use dpipe as a generic mechanism to get debug tables
out of the switch. The DSA routing table and the IP priority tables
could be candidates sometime in the future. But since most switches
don't actually have these, we are not so interested in them at the
moment. We are concentrating on tables that all DSA switches are
likely to have. Stuff we can implement once, and it works for all DSA
switches.

> > I think both those processes really look like match/action table
> > , thus they can be modeled successfully by dpipe.

And this is probably the core of the problem with dpipe. Very little
in an average switch is a match/action. We need a generic table. The
table is well specified, in that i can tell you the types of the
columns. We know the number of columns in the table at runtime, but
maybe not the number of rows until we reach the end of the table. And
ideally, we don't want to have to change the user space tool every
time we add a new table. The type info and the number of columns
should be enough for the user space tool to print it.

Andrew

2017-08-30 07:40:40

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Tue, Aug 29, 2017 at 05:57:54PM CEST, [email protected] wrote:
>Hi David, Jiri,
>
>Jiri Pirko <[email protected]> writes:
>
>> Tue, Aug 29, 2017 at 06:38:37AM CEST, [email protected] wrote:
>>>From: Vivien Didelot <[email protected]>
>>>Date: Mon, 28 Aug 2017 15:17:38 -0400
>>>
>>>> This patch series adds a generic debugfs interface for the DSA
>>>> framework, so that all switch devices benefit from it, e.g. Marvell,
>>>> Broadcom, Microchip or any other DSA driver.
>>>
>>>I've been thinking this over and I agree with the feedback given that
>>>debugfs really isn't appropriate for this.
>>>
>>>Please create a DSA device class, and hang these values under
>>>appropriate sysfs device nodes that can be easily found via
>>>/sys/class/dsa/ just as easily as they would be /sys/kernel/debug/dsa/
>>>
>>>You really intend these values to be consistent across DSA devices,
>>>and you don't intend to go willy-nilly changig these exported values
>>>arbitrarily over time. That's what debugfs is for, throw-away
>>>stuff.
>>>
>>>So please make these proper device sysfs attributes rather than
>>>debugfs.
>>
>> As I wrote, I believe that there is a big overlap with devlink and its
>> dpipe subset. I think that primary we should focus on extending whatever
>> is needed for dsa there. The iface should be generic for all drivers,
>> not only dsa. dsa-specific sysfs attributes should be last-resort solution,
>> I believe we can avoid them.
>
>Please note that this interface is only meant to provide a _debug_ and
>_development_ interface to DSA users. It is enableable at compile time
>and can be ditched anytime we want, in contrary to other interfaces
>which cannot be broken or changed because they are part of the ABI.
>
>I see sysfs as a script-friendly way to access and configure kernel
>structures, so I agree with Jiri that it doesn't seem appropriate.
>
>Extending devlink is a good option for long term, but it'll take a bit
>of time to extend data structures and not duplicate stats and regs
>accesses for ports which have a net device attached to it or not.
>
>In the meantime, I didn't find anything more useful and easier to debug
>a switch fabric than dumping side-by-side stats of all ports part of the
>data plane, for example like this:

So in the meantime, if you need some quick ugly think, you can always
have it out of the tree. Sorry but these are just excuses :/

>
> # watch -n1 pr -mt {switch0/port5,switch0/port6,switch1/port5,switch1/port3}/stats
>
>where ports 5 and 6 of both switches are DSA/CPU ports (without net
>devices attached to them) and port3 is a user port. This way one can
>easily see where and why packets get dropped.
>
>We could keep this interface and simply ditch net/dsa/debugfs.c when a
>convenient devlink alternative is in place.
>
>
>Thanks,
>
> Vivien

2017-08-30 07:43:08

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface

Tue, Aug 29, 2017 at 02:50:04PM CEST, [email protected] wrote:
>On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 10:08:34PM CEST, [email protected] wrote:
>> >> I see this overlaps a lot with DPIPE. Why won't you use that to expose
>> >> your hw state?
>> >
>> >We took a look at dpipe and i talked to you about using it for this
>> >sort of thing at netconf/netdev. But dpipe has issues displaying the
>> >sort of information we have. I never figured out how to do two
>> >dimensional tables. The output of the dpipe command is pretty
>> >unreadable. A lot of the information being dumped here is not about
>> >the data pipe, etc.
>>
>> So improve it. No problem. Also, we extend it to support what you neede.
>
>Will i did try to do this back in March. And i failed.
>
>Lets start with stats. Vivien gives an example on the cover letter:
>
> # pr -mt switch0/port{5,6}/stats
> in_good_octets : 0 in_good_octets : 13824
> in_bad_octets : 0 in_bad_octets : 0
> in_unicast : 0 in_unicast : 0
> in_broadcasts : 0 in_broadcasts : 216
> in_multicasts : 0 in_multicasts : 0
> in_pause : 0 in_pause : 0
> in_undersize : 0 in_undersize : 0
>
>This is what i tried to implement using dpipe. It is a simple two
>dimensional table. First column is a string, second a u64. In debugfs
>we have such a table per port. That fits with the hierarchy that each
>port is a directory in debugfs. But it could also be implemented as
>one table with N+1 columns, for N switch ports.

Andrew, we talked about this in Montreal. What I suggested then was for
port stats to introduce devlink port statistics. Then you can have stats
for all sorts of ports that don't have netlink instance associated,
including cpu port. It aligns.

>
>How about you, or one of your team, implement that. It should be able
>to use the dsa_loop driver, which is a simple dummy switch. But it
>does have statistics counters for all ports. Florian or I can help you
>get it running if needed.
>
>This branch contains some of the basic plumbing code from my previous
>attempt:
>
>https://github.com/lunn/linux.git v4.11-rc4-net-next-dpipe
>
> Andrew
>

2017-09-08 05:15:46

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

I agree you shouldn't be using debugfs for this, but in the future, if
you do write debugfs code, please take the following review into
account:

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> +static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
> +{
> + struct dentry *dir;
> + char name[32];
> +
> + snprintf(name, sizeof(name), DSA_PORT_FMT, port);
> +
> + dir = debugfs_create_dir(name, ds->debugfs_dir);
> + if (IS_ERR_OR_NULL(dir))
> + return -EFAULT;

You should _never_ care about the return value of a debugfs call, and
you should not need to ever propagate the error upward. The api was
written to not need this.

Just call the function, and return, that's it. If you need to save the
return value (i.e. it's a dentry), you also don't care, just save it and
pass it to some other debugfs call, and all will still be fine. Your
code should never do anything different if a debugfs call succeeds or
fails.

> +static int dsa_debugfs_create_switch(struct dsa_switch *ds)
> +{
> + char name[32];
> + int i, err;
> +
> + /* skip if there is no debugfs support */
> + if (!dsa_debugfs_dir)
> + return 0;

Again, you don't care, all of these functions should return void.

> + snprintf(name, sizeof(name), DSA_SWITCH_FMT, ds->index);
> +
> + ds->debugfs_dir = debugfs_create_dir(name, dsa_debugfs_dir);
> + if (IS_ERR_OR_NULL(ds->debugfs_dir))
> + return -EFAULT;

See, that's horrid, you should never need to make such a bad check.

Also, even if it were the correct way to do this you never return EFAULT
unless there is a memory copy error to/from userspace. That is not the
case here, or in any of this code, right?

> +static void dsa_debugfs_destroy_switch(struct dsa_switch *ds)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(ds->debugfs_dir);

Of course it handles NULL, why comment that? That's the whole goal of
debugfs, to be dirt simple, allow you to do anything you want, in almost
no lines of code.

Also, it will never be mounted on a "real" system, so you better not
rely on it for anything "real".

> + err = dsa_debugfs_create_switch(ds);
> + if (err) {
> + pr_warn("DSA: failed to create debugfs interface for switch %d (%d)\n",
> + ds->index, err);

Never complain to the syslog about a debugfs issue.

> +void dsa_debugfs_destroy_module(void)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(dsa_debugfs_dir);

again, of course it does, do you think we don't know how to write an
api? :)

thanks,

greg k-h

2017-09-08 14:02:22

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

Hi Greg,

Greg KH <[email protected]> writes:

> I agree you shouldn't be using debugfs for this, but in the future, if
> you do write debugfs code, please take the following review into
> account:

Humm sorry I may not have given enough details. This was really meant
for debug and dev only, because DSA makes it hard to query directly the
hardware (some switch ports are not exposed to userspace as well.)

This is not meant to be used for anything real at all, or even be
compiled-in in a production kernel. That's why I found it appropriate.

So I am still wondering why it doesn't fit here, can you tell me why?

> You should _never_ care about the return value of a debugfs call, and
> you should not need to ever propagate the error upward. The api was
> written to not need this.
>
> Just call the function, and return, that's it. If you need to save the
> return value (i.e. it's a dentry), you also don't care, just save it and
> pass it to some other debugfs call, and all will still be fine. Your
> code should never do anything different if a debugfs call succeeds or
> fails.

Thank for your interesting review! I'll cleanup my out-of-tree patches.

Vivien

2017-09-08 14:21:54

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

Hi Greg,

Can I ask for a quick review of this patch as well? It's the one adding
the boilerplate for a single debugfs file, and I'm pretty sure it can be
reduced somehow.

Also more important, you will notice what seems to be a bug to me:
I can read or write a file even if I didn't mask the corresponding mode,
hence the double check in dsa_debugfs_show and dsa_debugfs_write.

Thanks,

Vivien

2017-09-08 14:40:32

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

On Fri, Sep 08, 2017 at 10:18:27AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> Can I ask for a quick review of this patch as well? It's the one adding
> the boilerplate for a single debugfs file, and I'm pretty sure it can be
> reduced somehow.

I don't see a patch here :(

> Also more important, you will notice what seems to be a bug to me:
> I can read or write a file even if I didn't mask the corresponding mode,
> hence the double check in dsa_debugfs_show and dsa_debugfs_write.

The mode can be changed by userspace, you shouldn't ever need to check
it in any debugfs calls, right?

thanks,

greg k-h

2017-09-08 15:00:58

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

Hi Greg,

You wrote:

> > Can I ask for a quick review of this patch as well? It's the one adding
> > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > reduced somehow.
>
> I don't see a patch here :(

Oops, you weren't originally in Cc. Please find the patch below.

> > Also more important, you will notice what seems to be a bug to me:
> > I can read or write a file even if I didn't mask the corresponding mode
> > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
>
> The mode can be changed by userspace, you shouldn't ever need to check
> it in any debugfs calls, right?

Correct. But this happens even if the file mode isn't changed by
userspace in the meantime, which seemed weird to me. e.g. echo
redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.

Thanks,

Vivien

------ Beginning of the patch ------

This commit adds the boiler plate to create a DSA related debug
filesystem entry as well as a "tree" file, containing the tree index.

# cat switch1/tree
0

Signed-off-by: Vivien Didelot <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)

diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
index b6b5e5c97389..54e97e05a9d7 100644
--- a/net/dsa/debugfs.c
+++ b/net/dsa/debugfs.c
@@ -10,6 +10,7 @@
*/

#include <linux/debugfs.h>
+#include <linux/seq_file.h>

#include "dsa_priv.h"

@@ -19,6 +20,107 @@
/* DSA module debugfs directory */
static struct dentry *dsa_debugfs_dir;

+struct dsa_debugfs_ops {
+ int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
+ int (*write)(struct dsa_switch *ds, int id, char *buf);
+};
+
+struct dsa_debugfs_priv {
+ const struct dsa_debugfs_ops *ops;
+ struct dsa_switch *ds;
+ int id;
+};
+
+static int dsa_debugfs_show(struct seq_file *seq, void *p)
+{
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->read)
+ return -EOPNOTSUPP;
+
+ return priv->ops->read(ds, priv->id, seq);
+}
+
+static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *seq = file->private_data;
+ struct dsa_debugfs_priv *priv = seq->private;
+ struct dsa_switch *ds = priv->ds;
+ char buf[count + 1];
+ int err;
+
+ /* Somehow file mode is bypassed... Double check here */
+ if (!priv->ops->write)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+
+ buf[count] = '\0';
+
+ err = priv->ops->write(ds, priv->id, buf);
+
+ return err ? err : count;
+}
+
+static int dsa_debugfs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dsa_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations dsa_debugfs_fops = {
+ .open = dsa_debugfs_open,
+ .read = seq_read,
+ .write = dsa_debugfs_write,
+ .llseek = no_llseek,
+ .release = single_release,
+ .owner = THIS_MODULE,
+};
+
+static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
+ char *name, int id,
+ const struct dsa_debugfs_ops *ops)
+{
+ struct dsa_debugfs_priv *priv;
+ struct dentry *entry;
+ umode_t mode;
+
+ priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ops = ops;
+ priv->ds = ds;
+ priv->id = id;
+
+ mode = 0;
+ if (ops->read)
+ mode |= 0444;
+ if (ops->write)
+ mode |= 0200;
+
+ entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
+ if (IS_ERR_OR_NULL(entry))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int dsa_debugfs_tree_read(struct dsa_switch *ds, int id,
+ struct seq_file *seq)
+{
+ seq_printf(seq, "%d\n", ds->dst->tree);
+
+ return 0;
+}
+
+static const struct dsa_debugfs_ops dsa_debugfs_tree_ops = {
+ .read = dsa_debugfs_tree_read,
+};
+
static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
{
struct dentry *dir;
@@ -48,6 +150,11 @@ static int dsa_debugfs_create_switch(struct dsa_switch *ds)
if (IS_ERR_OR_NULL(ds->debugfs_dir))
return -EFAULT;

+ err = dsa_debugfs_create_file(ds, ds->debugfs_dir, "tree", -1,
+ &dsa_debugfs_tree_ops);
+ if (err)
+ return err;
+
for (i = 0; i < ds->num_ports; i++) {
if (ds->enabled_port_mask & BIT(i)) {
err = dsa_debugfs_create_port(ds, i);
--
2.14.1

2017-09-08 15:03:27

by David Laight

[permalink] [raw]

Subject: RE: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

From: Vivien Didelot
> Sent: 08 September 2017 15:57
...
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.

root will be able to write using 'root' permissions, regardless of
the directory entry.

David

2017-09-08 15:29:28

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree

On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote:
> Hi Greg,
>
> You wrote:
>
> > > Can I ask for a quick review of this patch as well? It's the one adding
> > > the boilerplate for a single debugfs file, and I'm pretty sure it can be
> > > reduced somehow.
> >
> > I don't see a patch here :(
>
> Oops, you weren't originally in Cc. Please find the patch below.
>
> > > Also more important, you will notice what seems to be a bug to me:
> > > I can read or write a file even if I didn't mask the corresponding mode
> > > hence the double check in dsa_debugfs_show and dsa_debugfs_write.
> >
> > The mode can be changed by userspace, you shouldn't ever need to check
> > it in any debugfs calls, right?
>
> Correct. But this happens even if the file mode isn't changed by
> userspace in the meantime, which seemed weird to me. e.g. echo
> redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write.
>
>
> Thanks,
>
> Vivien
>
>
> ------ Beginning of the patch ------
>
> This commit adds the boiler plate to create a DSA related debug
> filesystem entry as well as a "tree" file, containing the tree index.
>
> # cat switch1/tree
> 0
>
> Signed-off-by: Vivien Didelot <[email protected]>
> Reviewed-by: Florian Fainelli <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
> net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 107 insertions(+)
>
> diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c
> index b6b5e5c97389..54e97e05a9d7 100644
> --- a/net/dsa/debugfs.c
> +++ b/net/dsa/debugfs.c
> @@ -10,6 +10,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
> #include "dsa_priv.h"
>
> @@ -19,6 +20,107 @@
> /* DSA module debugfs directory */
> static struct dentry *dsa_debugfs_dir;
>
> +struct dsa_debugfs_ops {
> + int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq);
> + int (*write)(struct dsa_switch *ds, int id, char *buf);
> +};
> +
> +struct dsa_debugfs_priv {
> + const struct dsa_debugfs_ops *ops;
> + struct dsa_switch *ds;
> + int id;
> +};
> +
> +static int dsa_debugfs_show(struct seq_file *seq, void *p)
> +{
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> +
> + /* Somehow file mode is bypassed... Double check here */

As was said, root can do this, change your comment, just delete it :)

> + if (!priv->ops->read)
> + return -EOPNOTSUPP;
> +
> + return priv->ops->read(ds, priv->id, seq);
> +}
> +
> +static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *seq = file->private_data;
> + struct dsa_debugfs_priv *priv = seq->private;
> + struct dsa_switch *ds = priv->ds;
> + char buf[count + 1];

Nice, userspace asks to write 100Gb, and boom, you just smashed the
stack!

Repeat after me:
All input is evil.

Say it again.

Always remember it.

> + int err;
> +
> + /* Somehow file mode is bypassed... Double check here */
> + if (!priv->ops->write)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[count] = '\0';

Be careful here.

Use the kernel library functions instead of a "raw" copy_from/to_user()
calls, that is what they are there for (simple_read_to_buffer,
simple_write_to_buffer).

> +
> + err = priv->ops->write(ds, priv->id, buf);
> +
> + return err ? err : count;
> +}
> +
> +static int dsa_debugfs_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, dsa_debugfs_show, inode->i_private);
> +}
> +
> +static const struct file_operations dsa_debugfs_fops = {
> + .open = dsa_debugfs_open,
> + .read = seq_read,
> + .write = dsa_debugfs_write,
> + .llseek = no_llseek,
> + .release = single_release,
> + .owner = THIS_MODULE,
> +};
> +
> +static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir,
> + char *name, int id,
> + const struct dsa_debugfs_ops *ops)
> +{
> + struct dsa_debugfs_priv *priv;
> + struct dentry *entry;
> + umode_t mode;
> +
> + priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ops = ops;
> + priv->ds = ds;
> + priv->id = id;
> +
> + mode = 0;
> + if (ops->read)
> + mode |= 0444;
> + if (ops->write)
> + mode |= 0200;
> +
> + entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops);
> + if (IS_ERR_OR_NULL(entry))
> + return -EFAULT;

Again, you don't care, don't check!

thanks,

greg k-h

2017-09-14 19:59:29

by Maxim Uvarov

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

debugfs here is very very useful to read registers directly and
compare what use space tools see. Cool feature to get regs by port and
use standard tools to diff and print them. Even might be better to
allow drivers to decode register names and bits values. Once that is
done driver mainaince will be much easy. I.e. you need only match regs
with spec from one side and regs with user space tools from other
side. Of course it's needed only for debuging, not for production. But
even for production regs dump on something wrong might tell a lot.

Maxim.

2017-09-08 16:58 GMT+03:00 Vivien Didelot <[email protected]>:
> Hi Greg,
>
> Greg KH <[email protected]> writes:
>
>> I agree you shouldn't be using debugfs for this, but in the future, if
>> you do write debugfs code, please take the following review into
>> account:
>
> Humm sorry I may not have given enough details. This was really meant
> for debug and dev only, because DSA makes it hard to query directly the
> hardware (some switch ports are not exposed to userspace as well.)
>
> This is not meant to be used for anything real at all, or even be
> compiled-in in a production kernel. That's why I found it appropriate.
>
> So I am still wondering why it doesn't fit here, can you tell me why?
>
>> You should _never_ care about the return value of a debugfs call, and
>> you should not need to ever propagate the error upward. The api was
>> written to not need this.
>>
>> Just call the function, and return, that's it. If you need to save the
>> return value (i.e. it's a dentry), you also don't care, just save it and
>> pass it to some other debugfs call, and all will still be fine. Your
>> code should never do anything different if a debugfs call succeeds or
>> fails.
>
> Thank for your interesting review! I'll cleanup my out-of-tree patches.
>
>
> Vivien

--
Best regards,
Maxim Uvarov

2017-09-14 20:12:36

by Alexander Duyck

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov <[email protected]> wrote:
> debugfs here is very very useful to read registers directly and
> compare what use space tools see. Cool feature to get regs by port and
> use standard tools to diff and print them. Even might be better to
> allow drivers to decode register names and bits values. Once that is
> done driver mainaince will be much easy. I.e. you need only match regs
> with spec from one side and regs with user space tools from other
> side. Of course it's needed only for debuging, not for production. But
> even for production regs dump on something wrong might tell a lot.
>
> Maxim.

Can you clarify what type of registers it is you are wanting to read?
We already have ethtool which is meant to allow reading the device
registers for a given netdev. As long as the port has a netdev
associated it then there is no need to be getting into debugfs since
we should probably just be using ethtool.

Also as Jiri pointed out there is already devlink which would probably
be a better way to get the associated information for those pieces
that don't have a netdev associated with them.

- Alex

2017-09-14 21:01:41

[permalink] [raw]

Subject: Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

> Can you clarify what type of registers it is you are wanting to read?
> We already have ethtool which is meant to allow reading the device
> registers for a given netdev. As long as the port has a netdev
> associated it then there is no need to be getting into debugfs since
> we should probably just be using ethtool.

Not all ports of a DSA switch have a netdev. This is by design. The
presentation we gave to Netdev 2.1 gives some of the background.

Plus a switch has a lot of registers not associated to port. Often a
switch has more global registers than port registers.

> Also as Jiri pointed out there is already devlink which would probably
> be a better way to get the associated information for those pieces
> that don't have a netdev associated with them.

We have looked at the devlink a few times. The current dpipe code is
not generic enough. It makes assumptions about the architecture of the
switch, that it is all match/action based. The niche of top of rack
switches might be like that, but average switches are not.

If dpipe was to support simple generic two dimensional tables, we
probably would use it.

David suggested making a class device for DSA. It is not ideal, but we
are probably going to go that way.

Andrew

2017-09-15 05:51:12