From: Vivien Didelot <hidden> Date: 2017-08-28 19:21:41
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
From: Vivien Didelot <hidden> Date: 2017-08-28 19:21:43
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 <redacted>
---
include/net/dsa.h | 26 ++++++++++++++++++++++++++
net/dsa/debugfs.c | 23 +++++++++++++++++++++++
2 files changed, 49 insertions(+)
@@ -39,6 +39,32 @@ enum dsa_tag_protocol {DSA_TAG_LAST,/* MUST BE LAST */};+staticinlineconstchar*dsa_tag_protocol_name(enumdsa_tag_protocolproto)+{+switch(proto){+caseDSA_TAG_PROTO_NONE:+return"none";+caseDSA_TAG_PROTO_BRCM:+return"BRCM";+caseDSA_TAG_PROTO_DSA:+return"DSA";+caseDSA_TAG_PROTO_EDSA:+return"EDSA";+caseDSA_TAG_PROTO_KSZ:+return"KSZ";+caseDSA_TAG_PROTO_LAN9303:+return"LAN9303";+caseDSA_TAG_PROTO_MTK:+return"MTK";+caseDSA_TAG_PROTO_QCA:+return"QCA";+caseDSA_TAG_PROTO_TRAILER:+return"TRAILER";+default:+return"unknown";+}+}+#define DSA_MAX_SWITCHES 4#define DSA_MAX_PORTS 12
From: Vivien Didelot <hidden> Date: 2017-08-28 19:21:48
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 <redacted>
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(+)
@@ -1053,6 +1053,46 @@ int b53_vlan_del(struct dsa_switch *ds, int port,}EXPORT_SYMBOL(b53_vlan_del);+intb53_vlan_dump(structdsa_switch*ds,intport,dsa_vlan_dump_cb_t*cb,+void*data)+{+structb53_device*dev=ds->priv;+u16vid,vid_start=0,pvid;+structb53_vlan*vl;+booluntagged;+interr=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+*operationreturningonlytheused/validVLANs+*/+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;+}++returnerr;+}+EXPORT_SYMBOL(b53_vlan_dump);+/* Address Resolution Logic routines */staticintb53_arl_op_wait(structb53_device*dev){
@@ -257,6 +257,43 @@ static int dsa_loop_port_vlan_del(struct dsa_switch *ds, int port,return0;}+staticintdsa_loop_port_vlan_dump(structdsa_switch*ds,intport,+dsa_vlan_dump_cb_t*cb,void*data)+{+structdsa_loop_priv*ps=ds->priv;+structmii_bus*bus=ps->bus;+structdsa_loop_vlan*vl;+u16vid,vid_start=0;+boolpvid,untagged;+interr=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;+}++returnerr;+}+staticconststructdsa_switch_opsdsa_loop_driver={.get_tag_protocol=dsa_loop_get_protocol,.setup=dsa_loop_setup,
@@ -638,6 +638,46 @@ static int ksz_port_vlan_del(struct dsa_switch *ds, int port,return0;}+staticintksz_port_vlan_dump(structdsa_switch*ds,intport,+dsa_vlan_dump_cb_t*cb,void*data)+{+structksz_device*dev=ds->priv;+structvlan_table*vlan_cache;+boolpvid,untagged;+u16val;+intvid;+interr=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);++returnerr;+}+structalu_struct{/* entry 1 */u8is_static:1;
From: Vivien Didelot <hidden> Date: 2017-08-28 19:23:17
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 <redacted>
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(+)
From: Vivien Didelot <hidden> Date: 2017-08-28 19:23:19
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 <redacted>
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(+)
From: Vivien Didelot <hidden> Date: 2017-08-28 19:23:21
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 <redacted>
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(+)
@@ -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);+return0;}
@@ -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
@@ -671,6 +673,8 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst){inti;+dsa_debugfs_destroy_tree(dst);+dst->cpu_dp->netdev->dsa_ptr=NULL;/* If we used a tagging format that doesn't have an ethertype
From: Vivien Didelot <hidden> Date: 2017-08-28 19:25:21
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 <redacted>
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(-)
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 <redacted>
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.
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?
Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.didelot@savoirfairelinux.com wrote:
quoted
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 <redacted>
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
Mon, Aug 28, 2017 at 09:58:12PM CEST, f.fainelli@gmail.com wrote:
On 08/28/2017 12:50 PM, Jiri Pirko wrote:
quoted
Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.didelot@savoirfairelinux.com wrote:
quoted
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 <redacted>
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 :/
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-28 20:08:46
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-28 20:16:49
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 <redacted>
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-28 20:19:12
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 <redacted>
From: David Miller <davem@davemloft.net> Date: 2017-08-29 04:38:40
From: Vivien Didelot <redacted>
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.
Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch wrote:
quoted
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!
Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
From: Vivien Didelot <redacted>
Date: Mon, 28 Aug 2017 15:17:38 -0400
quoted
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.
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-29 12:50:13
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:
quoted
quoted
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
From: Vivien Didelot <hidden> Date: 2017-08-29 16:01:13
Hi David, Jiri,
Jiri Pirko [off-list ref] writes:
Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
quoted
From: Vivien Didelot <redacted>
Date: Mon, 28 Aug 2017 15:17:38 -0400
quoted
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
On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
quoted
Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch wrote:
quoted
quoted
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.
On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
quoted
Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch wrote:
quoted
quoted
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.
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-29 20:27:40
On Tue, Aug 29, 2017 at 12:19:08PM -0700, Florian Fainelli wrote:
On 08/29/2017 12:05 PM, Arkadi Sharshevsky wrote:
quoted
On 08/29/2017 03:50 PM, Andrew Lunn wrote:
quoted
On Tue, Aug 29, 2017 at 08:25:23AM +0200, Jiri Pirko wrote:
quoted
Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch wrote:
quoted
quoted
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.
quoted
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.
quoted
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
Tue, Aug 29, 2017 at 05:57:54PM CEST, vivien.didelot@savoirfairelinux.com wrote:
Hi David, Jiri,
Jiri Pirko [off-list ref] writes:
quoted
Tue, Aug 29, 2017 at 06:38:37AM CEST, davem@davemloft.net wrote:
quoted
From: Vivien Didelot <redacted>
Date: Mon, 28 Aug 2017 15:17:38 -0400
quoted
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
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:
quoted
Mon, Aug 28, 2017 at 10:08:34PM CEST, andrew@lunn.ch wrote:
quoted
quoted
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
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:
quoted hunk
+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.
quoted hunk
+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.
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?
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".
quoted hunk
+ 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.
From: Vivien Didelot <hidden> Date: 2017-09-08 14:02:22
Hi Greg,
Greg KH [off-list ref] 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
From: Vivien Didelot <hidden> Date: 2017-09-08 14:21:54
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
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
From: Vivien Didelot <hidden> Date: 2017-09-08 15:00:58
Hi Greg,
You wrote:
quoted
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.
quoted
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 <redacted>
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(+)
From: David Laight <hidden> Date: 2017-09-08 15:03:27
From: Vivien Didelot
Sent: 08 September 2017 15:57
...
quoted
quoted
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
On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote:
quoted hunk
Hi Greg,
You wrote:
quoted
quoted
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.
quoted
quoted
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 <redacted>
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(+)
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.
quoted hunk
+ 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).
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 [off-list ref]:
Hi Greg,
Greg KH [off-list ref] writes:
quoted
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?
quoted
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
From: Alexander Duyck <hidden> Date: 2017-09-14 20:12:36
On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov [off-list ref] 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
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-09-14 21:01:41
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
Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
quoted
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.
quoted
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.
Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
quoted
quoted
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.
quoted
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
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.
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
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