Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721925 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-28 21:30 +0200
    [PATCH net-next v2 10/10] net: dsa: debugfs: add port vlan Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-28 21:30 +0200
    [PATCH net-next v2 01/10] net: dsa: add debugfs interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-28 21:30 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Florian Fainelli <f.fainelli@gmail.com> - 2017-08-28 22:00 +0200
        Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-28 22:10 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-28 22:00 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Andrew Lunn <andrew@lunn.ch> - 2017-08-28 22:20 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Greg KH <gregkh@linuxfoundation.org> - 2017-09-08 07:20 +0200
        Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-08 16:10 +0200
    Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-28 22:00 +0200
      Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Andrew Lunn <andrew@lunn.ch> - 2017-08-28 22:10 +0200
        Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-29 08:30 +0200
          Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Andrew Lunn <andrew@lunn.ch> - 2017-08-29 15:00 +0200
            Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Florian Fainelli <f.fainelli@gmail.com> - 2017-08-29 21:20 +0200
              Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Andrew Lunn <andrew@lunn.ch> - 2017-08-29 22:30 +0200
            Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-30 09:50 +0200
    Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs  interface David Miller <davem@davemloft.net> - 2017-08-29 06:40 +0200
      Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-29 08:30 +0200
        Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-29 18:10 +0200
          Re: [PATCH net-next v2 00/10] net: dsa: add generic debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-08-30 09:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

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

Here are the VLAN entries for a CPU port:

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

Signed-off-by: Vivien Didelot <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

[toc] | [prev] | [next] | [standalone]

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

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

The interface can be mounted with:

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

The dsa directory contains one directory per switch chip:

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

Each chip directory contains one directory per port:

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

Future patches will add entry files to these directories.

Signed-off-by: Vivien Didelot <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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 :/

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

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

    Andrew

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Never complain to the syslog about a debugfs issue.

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

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

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1721936

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?

[toc] | [prev] | [next] | [standalone]

#1721942

> 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

[toc] | [prev] | [next] | [standalone]

#1722159

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!

[toc] | [prev] | [next] | [standalone]

#1722450

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

[toc] | [prev] | [next] | [standalone]

#1722722

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

[toc] | [prev] | [next] | [standalone]

#1722822

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

[toc] | [prev] | [next] | [standalone]

#1723080

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
>

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1722169

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.

[toc] | [prev] | [next] | [standalone]

#1722586

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

[toc] | [prev] | [next] | [standalone]

#1723078

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web