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

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

[Vivien Didelot]

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.

This support is only compiled if CONFIG_DEBUG_FS is enabled. Below is
and example of usage of this interface on a multi-chip switch fabric:

    # mount -t debugfs none /sys/kernel/debug
    # cd /sys/kernel/debug/dsa/
    # ls
    switch0  switch1 switch2
    # 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
    -r--r--r-- 1 root root 0 Jan  1 00:00 tag_protocol
    -r--r--r-- 1 root root 0 Jan  1 00:00 tree
    # ls -l switch0/port6
    -r--r--r-- 1 root root 0 Jan  1 00:00 fdb
    -r--r--r-- 1 root root 0 Jan  1 00:00 mdb
    -r--r--r-- 1 root root 0 Jan  1 00:00 regs
    -r--r--r-- 1 root root 0 Jan  1 00:00 stats
    -r--r--r-- 1 root root 0 Jan  1 00:00 vlan
    # cat switch0/port2/vlan
    vid 42  untagged  pvid
    # cat switch0/port1/fdb
    vid 0  12:34:56:78:90:ab  unicast  static
    # 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
    ...
    # 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
    ...

where switch0 port5 and port6 are CPU and DSA ports of a ZII Rev B.

Changes in v2:
  - KISS, drop the WARN_ON if !dst->applied
  - use ds->enabled_port_mask instead of OF nodes
  - add a tag protocol to string helper
  - use %pM to print MAC addresses
  - explicit "tagged" VLANs

Vivien Didelot (10):
  net: dsa: add debugfs interface
  net: dsa: debugfs: add tree
  net: dsa: debugfs: add tag_protocol
  net: dsa: debugfs: add port stats
  net: dsa: debugfs: add port regs
  net: dsa: debugfs: add port fdb
  net: dsa: restore mdb dump
  net: dsa: debugfs: add port mdb
  net: dsa: restore VLAN dump
  net: dsa: debugfs: add port vlan

 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       |  82 ++++++-
 include/net/dsa.h                      |  41 ++++
 net/dsa/Kconfig                        |  14 ++
 net/dsa/Makefile                       |   1 +
 net/dsa/debugfs.c                      | 409 +++++++++++++++++++++++++++++++++
 net/dsa/dsa.c                          |   3 +
 net/dsa/dsa2.c                         |   4 +
 net/dsa/dsa_priv.h                     |  13 ++
 net/dsa/legacy.c                       |   4 +
 14 files changed, 686 insertions(+), 8 deletions(-)
 create mode 100644 net/dsa/debugfs.c

-- 
2.14.1

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
---
 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 <vivien.didelot@savoirfairelinux.com>
+ *
+ * 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

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

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.didelot@savoirfairelinux.com 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 <vivien.didelot@savoirfairelinux.com>

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.

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

[Florian Fainelli]

On 08/28/2017 12:50 PM, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.didelot@savoirfairelinux.com 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 <vivien.didelot@savoirfairelinux.com>
> 
> 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

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

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:58:12PM CEST, f.fainelli@gmail.com wrote:
>On 08/28/2017 12:50 PM, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.didelot@savoirfairelinux.com 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 <vivien.didelot@savoirfairelinux.com>
>> 
>> 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 :/

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

[Andrew Lunn]

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 <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Greg KH]

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

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

[Vivien Didelot]

Hi Greg,

Greg KH <gregkh@linuxfoundation.org> 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

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

[Maxim Uvarov]

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 <vivien.didelot@savoirfairelinux.com>:
> Hi Greg,
>
> Greg KH <gregkh@linuxfoundation.org> 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

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

[Alexander Duyck]

On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov <muvarov@gmail.com> 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

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

[Andrew Lunn]

> 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

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

[Jiri Pirko]

Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
>> 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.

I believe that is also big mistake.

Could you put together your requirements so we can work it out to extend
devlink to support them?

Thanks.

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

[Egil Hjelmeland]

On 15. sep. 2017 07:51, Jiri Pirko wrote:
> Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
>>> 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.
> 
> I believe that is also big mistake.
> 
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
> 
> Thanks.
> 

$ ack -i devlink Documentation/
$ ack -i dpipe Documentation/
$

How you expect new mechanisms to be taken into use with zero documentation?

To all: Why does reviewers nitpick about undocumented formatting rules, 
but not ask about documentation?

Egil

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

[Andrew Lunn]

> Could you put together your requirements so we can work it out to extend
> devlink to support them?

As i've said multiple times, generic two dimensional tables. Examples
could look like:

Stats                 cpu     lan0   lan1   lan2   lan3   lan4    dsa
---------------------------------------------------------------------
tx_packets:              2       0      0      0      0      0      0
tx_bytes:             1180    6666      0      0      0      0      0
rx_packets:              0       0      0      0      0      0      0
rx_bytes:                0    1180      0      0      0      0      0
in_good_octets:       6666    1188      0      0      0      0      0
in_bad_octets:           0       0      0      0      0      0      0
in_unicast:              0       0      0      0      0      0      0
in_broadcasts:           0       0      0      0      0      0      0
in_multicasts:          89       0      0      0      0      0      0
in_pause:                0       0      0      0      0      0      0
in_undersize:            0       0      0      0      0      0      0
in_fragments:            0       0      0      0      0      0      0
in_oversize:             0       0      0      0      0      0      0
in_jabber:               0       0      0      0      0      0      0
in_rx_error:             0       0      0      0      0      0      0
in_fcs_error:            0       0      0      0      0      0      0
out_octets:           1188    6666      0      0      0      0      0
out_unicast:             0       0      0      0      0      0      0
out_broadcasts:          2       2      0      0      0      0      0
out_multicasts:          0      89      0      0      0      0      0
out_pause:               0       0      0      0      0      0      0
excessive:               0       0      0      0      0      0      0
collisions:              0       0      0      0      0      0      0
deferred:                0       0      0      0      0      0      0
single:                  0       0      0      0      0      0      0
multiple:                0       0      0      0      0      0      0
out_fcs_error:           0       0      0      0      0      0      0
late:                    0       0      0      0      0      0      0
hist_64bytes:            0       0      0      0      0      0      0
hist_65_127bytes:        0       0      0      0      0      0      0
hist_128_255bytes:       0       0      0      0      0      0      0
hist_256_511bytes:       0       0      0      0      0      0      0
hist_512_1023bytes:      0       0      0      0      0      0      0
hist_1024_max_bytes:     0       0      0      0      0      0      0
sw_in_discards:          0       0      0      0      0      0      0
sw_in_filtered:          0       0      0      0      0      0      0
sw_out_filtered:        89      89      0      0      0      0      0


   Reg  cpu     lan0    lan1    lan2    lan3    lan4    lan5  global0 global1
-----------------------------------------------------------------------------
    00: 4e07	4d04	4d04	4d04	4d04	4d04	4d04     0000   00000
    01: 403e	003d	003d	003d	003d	003d	003d     0000   00000
    02: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    03: 3521	3521	3521	3521	3521	3521	3521     0000   00000
    04: 0533	373f	373f	373f	373f	373f	373f     0000   00000
    05: 8000	0000	0000	0000	0000	0000	0000     0000   00000
    06: 005f	003f	003f	003f	003f	003f	003f     0000   00000
    07: 002a	002a	002a	002a	002a	002a	002a     0000   00000
    08: 2080	2080	2080	2080	2080	2080	2080     0000   00000
    09: 0001	0001	0001	0001	0001	0001	0001     0000   00000
    0a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    0b: 0020	0000	0000	0000	0000	0000	0000     0000   00000
    0c: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    0d: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    0e: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    0f: 9100	dada	dada	dada	dada	dada	dada     0000   00000
    10: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    11: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    12: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    13: 0000	00d8	00d8	00d8	00d8	00d8	00d8     0000   00000
    14: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    15: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    16: 0022	0000	0000	0000	0000	0000	0000     0000   00000
    17: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    18: 3210	2210	2210	2210	2210	2210	2210     0000   00000
    19: 7654	7654	7654	7654	7654	7654	7654     0000   00000
    1a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
    1b: 8000	8000	8000	8000	8000	8000	8000     0000   00000
    1c: 0000    0000    0000    0000    0000    0000    0000     0000   00000
    1d: 0000    0000    0000    0000    0000    0000    0000     0000   00000
    1e: 0000    0000    0000    0000    0000    0000    0000     0000   00000
    1f: 0000    0000    0000    0000    0000    0000    0000     0000   00000

So a table would have an optional header row. Then a number of data
rows. The number of columns is the same for each row. The number of
columns is determined at run time, but is known at the beginning of
enumerating the table. The number of data rows is not known until the
last one is enumerated.

Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
ifindex, MAC address, IP address, devlink port, ....

Each cell in a row can have a different type. The types of the header
row cells can be different to the types of the data cells. All data
rows have the same type information. So you can enumerate the types
once, and use them for the whole table.

The userspace tool should make its best effort to print the table,
using the type info. It might need hits from the command line, like -x
to print in hex not decimal. The second example shows this. Registers
make more sense in hex, where as statistics make more sense in
decimal. But let the user choose, so keeping the kAPI simple.  This is
intended as a debug tool. The output does not need to be highly
polished. But it needs to be a lot more readable than the current
dpipe output which prints a table as a list, not a table.

The tables can be per port, or per switch. Remember that DSA allows a
cluster of switches within one DSA instance.

Vivien, Florian, did i miss anything?

	 Andrew

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

[Jiri Pirko]

Fri, Sep 15, 2017 at 04:08:39PM CEST, andrew@lunn.ch wrote:
>> Could you put together your requirements so we can work it out to extend
>> devlink to support them?
>
>As i've said multiple times, generic two dimensional tables. Examples
>could look like:
>
>Stats                 cpu     lan0   lan1   lan2   lan3   lan4    dsa
>---------------------------------------------------------------------
>tx_packets:              2       0      0      0      0      0      0
>tx_bytes:             1180    6666      0      0      0      0      0
>rx_packets:              0       0      0      0      0      0      0
>rx_bytes:                0    1180      0      0      0      0      0
>in_good_octets:       6666    1188      0      0      0      0      0
>in_bad_octets:           0       0      0      0      0      0      0
>in_unicast:              0       0      0      0      0      0      0
>in_broadcasts:           0       0      0      0      0      0      0
>in_multicasts:          89       0      0      0      0      0      0
>in_pause:                0       0      0      0      0      0      0
>in_undersize:            0       0      0      0      0      0      0
>in_fragments:            0       0      0      0      0      0      0
>in_oversize:             0       0      0      0      0      0      0
>in_jabber:               0       0      0      0      0      0      0
>in_rx_error:             0       0      0      0      0      0      0
>in_fcs_error:            0       0      0      0      0      0      0
>out_octets:           1188    6666      0      0      0      0      0
>out_unicast:             0       0      0      0      0      0      0
>out_broadcasts:          2       2      0      0      0      0      0
>out_multicasts:          0      89      0      0      0      0      0
>out_pause:               0       0      0      0      0      0      0
>excessive:               0       0      0      0      0      0      0
>collisions:              0       0      0      0      0      0      0
>deferred:                0       0      0      0      0      0      0
>single:                  0       0      0      0      0      0      0
>multiple:                0       0      0      0      0      0      0
>out_fcs_error:           0       0      0      0      0      0      0
>late:                    0       0      0      0      0      0      0
>hist_64bytes:            0       0      0      0      0      0      0
>hist_65_127bytes:        0       0      0      0      0      0      0
>hist_128_255bytes:       0       0      0      0      0      0      0
>hist_256_511bytes:       0       0      0      0      0      0      0
>hist_512_1023bytes:      0       0      0      0      0      0      0
>hist_1024_max_bytes:     0       0      0      0      0      0      0
>sw_in_discards:          0       0      0      0      0      0      0
>sw_in_filtered:          0       0      0      0      0      0      0
>sw_out_filtered:        89      89      0      0      0      0      0

I believe we discussed this already. You can have devlink_port instance
for each port, then you just have stats-per-devlink_port. Looks quite
easy to implement actually.

Would be mistake to have this as just 2 dim array.


>
>
>   Reg  cpu     lan0    lan1    lan2    lan3    lan4    lan5  global0 global1
>-----------------------------------------------------------------------------
>    00: 4e07	4d04	4d04	4d04	4d04	4d04	4d04     0000   00000
>    01: 403e	003d	003d	003d	003d	003d	003d     0000   00000
>    02: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    03: 3521	3521	3521	3521	3521	3521	3521     0000   00000
>    04: 0533	373f	373f	373f	373f	373f	373f     0000   00000
>    05: 8000	0000	0000	0000	0000	0000	0000     0000   00000
>    06: 005f	003f	003f	003f	003f	003f	003f     0000   00000
>    07: 002a	002a	002a	002a	002a	002a	002a     0000   00000
>    08: 2080	2080	2080	2080	2080	2080	2080     0000   00000
>    09: 0001	0001	0001	0001	0001	0001	0001     0000   00000
>    0a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    0b: 0020	0000	0000	0000	0000	0000	0000     0000   00000
>    0c: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    0d: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    0e: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    0f: 9100	dada	dada	dada	dada	dada	dada     0000   00000
>    10: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    11: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    12: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    13: 0000	00d8	00d8	00d8	00d8	00d8	00d8     0000   00000
>    14: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    15: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    16: 0022	0000	0000	0000	0000	0000	0000     0000   00000
>    17: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    18: 3210	2210	2210	2210	2210	2210	2210     0000   00000
>    19: 7654	7654	7654	7654	7654	7654	7654     0000   00000
>    1a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
>    1b: 8000	8000	8000	8000	8000	8000	8000     0000   00000
>    1c: 0000    0000    0000    0000    0000    0000    0000     0000   00000
>    1d: 0000    0000    0000    0000    0000    0000    0000     0000   00000
>    1e: 0000    0000    0000    0000    0000    0000    0000     0000   00000
>    1f: 0000    0000    0000    0000    0000    0000    0000     0000   00000
>

Is this a reg dump per-port? Also, with per-port devlink_port instance,
you can have reg array for each. How the values can change? Is this
change result of driver<->hw communication? If yes, you might consider
using devlink hwmsg trace to expose the communication to userspace.


>So a table would have an optional header row. Then a number of data
>rows. The number of columns is the same for each row. The number of
>columns is determined at run time, but is known at the beginning of
>enumerating the table. The number of data rows is not known until the
>last one is enumerated.

I would rather focus on what exactly you need to expose to userspace,
then we can figure out how to do it. Generic multipurpose arrays should
be considered as last-resort solution in my opinion.


>
>Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
>ifindex, MAC address, IP address, devlink port, ....
>
>Each cell in a row can have a different type. The types of the header
>row cells can be different to the types of the data cells. All data
>rows have the same type information. So you can enumerate the types
>once, and use them for the whole table.
>
>The userspace tool should make its best effort to print the table,
>using the type info. It might need hits from the command line, like -x
>to print in hex not decimal. The second example shows this. Registers
>make more sense in hex, where as statistics make more sense in
>decimal. But let the user choose, so keeping the kAPI simple.  This is
>intended as a debug tool. The output does not need to be highly
>polished. But it needs to be a lot more readable than the current
>dpipe output which prints a table as a list, not a table.
>
>The tables can be per port, or per switch. Remember that DSA allows a
>cluster of switches within one DSA instance.
>
>Vivien, Florian, did i miss anything?
>
>	 Andrew

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

[Andrew Lunn]

> >   Reg  cpu     lan0    lan1    lan2    lan3    lan4    lan5  global0 global1
> >-----------------------------------------------------------------------------
> >    00: 4e07	4d04	4d04	4d04	4d04	4d04	4d04     0000   00000
> >    01: 403e	003d	003d	003d	003d	003d	003d     0000   00000
> >    02: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    03: 3521	3521	3521	3521	3521	3521	3521     0000   00000
> >    04: 0533	373f	373f	373f	373f	373f	373f     0000   00000
> >    05: 8000	0000	0000	0000	0000	0000	0000     0000   00000
> >    06: 005f	003f	003f	003f	003f	003f	003f     0000   00000
> >    07: 002a	002a	002a	002a	002a	002a	002a     0000   00000
> >    08: 2080	2080	2080	2080	2080	2080	2080     0000   00000
> >    09: 0001	0001	0001	0001	0001	0001	0001     0000   00000
> >    0a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    0b: 0020	0000	0000	0000	0000	0000	0000     0000   00000
> >    0c: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    0d: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    0e: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    0f: 9100	dada	dada	dada	dada	dada	dada     0000   00000
> >    10: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    11: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    12: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    13: 0000	00d8	00d8	00d8	00d8	00d8	00d8     0000   00000
> >    14: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    15: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    16: 0022	0000	0000	0000	0000	0000	0000     0000   00000
> >    17: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    18: 3210	2210	2210	2210	2210	2210	2210     0000   00000
> >    19: 7654	7654	7654	7654	7654	7654	7654     0000   00000
> >    1a: 0000	0000	0000	0000	0000	0000	0000     0000   00000
> >    1b: 8000	8000	8000	8000	8000	8000	8000     0000   00000
> >    1c: 0000    0000    0000    0000    0000    0000    0000     0000   00000
> >    1d: 0000    0000    0000    0000    0000    0000    0000     0000   00000
> >    1e: 0000    0000    0000    0000    0000    0000    0000     0000   00000
> >    1f: 0000    0000    0000    0000    0000    0000    0000     0000   00000
> >
> 
> Is this a reg dump per-port?

No. Look at the global0 and global1 columns. These are not port
registers, but switch global registers. The other columns are per
port. We could have a two column table per port for these
registers. However for debugging, it is useful to see the port
registers side-by-side. The difference between ports often gives you
clues as to what is wrong.

The global registers however are scoped to a switch, not a port.

> you can have reg array for each. How the values can change? Is this
> change result of driver<->hw communication? If yes, you might consider
> using devlink hwmsg trace to expose the communication to userspace.

The reason a value changes varies per bit. Some are status, set by the
switch. Some are configuration, set by the driver. They are all mixed
up together. And some registers are actually just multiplexors onto
other tables of registers. You set an opcode in the lower nibble,
address in the middle, set the top bit and busy loop waiting for it to
clear. You can then read a result out of the register, or a near by
register.  So the register dump gives you an idea what the last access
to such a table was.

> I would rather focus on what exactly you need to expose to userspace,
> then we can figure out how to do it. Generic multipurpose arrays should
> be considered as last-resort solution in my opinion.

So go look at Vivien's patches. That is what we want to expose. For a
start. Viviens patches are at the DSA level. Once we get something
accepted at that level, i can imaging drivers want to augment it with
driver specific tables. But we can handle that later.

       Andrew

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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

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

[Greg Kroah-Hartman]

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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[David Laight]

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

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

[Greg Kroah-Hartman]

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 <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
---
 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

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

[Andrew Lunn]

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 <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Vivien Didelot]

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 <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

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

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:17:38PM CEST, vivien.didelot@savoirfairelinux.com 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?

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

[Andrew Lunn]

> 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

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

[Jiri Pirko]

Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch 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!

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

[Andrew Lunn]

On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch 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

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

[Arkadi Sharshevsky]

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, andrew@lunn.ch 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
> 

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

[Florian Fainelli]

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, andrew@lunn.ch 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

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

[Andrew Lunn]

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, andrew@lunn.ch 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

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

[Jiri Pirko]

Tue, Aug 29, 2017 at 02:50:04PM CEST, andrew@lunn.ch wrote:
>On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch 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
>

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

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
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.

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

[Jiri Pirko]

Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
>From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>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.

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

[Vivien Didelot]

Hi David, Jiri,

Jiri Pirko <jiri@resnulli.us> writes:

> Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
>>From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>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

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

[Jiri Pirko]

Tue, Aug 29, 2017 at 05:57:54PM CEST, vivien.didelot@savoirfairelinux.com wrote:
>Hi David, Jiri,
>
>Jiri Pirko <jiri@resnulli.us> writes:
>
>> Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
>>>From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>>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