[PATCH ethtool] ethtool: dump nested registers

[PATCH ethtool] ethtool: dump nested registers

[Vivien Didelot]

Usually kernel drivers set the regs->len value to the same length as
info->regdump_len, which was used for the allocation. In case where
regs->len is smaller than the allocated info->regdump_len length,
we may assume that the dump contains a nested set of registers.

This becomes handy for kernel drivers to expose registers of an
underlying network conduit unfortunately not exposed to userspace,
as found in network switching equipment for example.

This patch adds support for recursing into the dump operation if there
is enough room for a nested ethtool_drvinfo structure containing a
valid driver name, followed by a ethtool_regs structure like this:

    0      regs->len                        info->regdump_len
    v              v                                        v
    +--------------+-----------------+--------------+-- - --+
    | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
    +--------------+-----------------+--------------+-- - --+

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 ethtool.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 05fe05a08..c0e2903c5 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1245,7 +1245,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
 
 	if (gregs_dump_raw) {
 		fwrite(regs->data, regs->len, 1, stdout);
-		return 0;
+		goto nested;
 	}
 
 	if (!gregs_dump_hex)
@@ -1253,7 +1253,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
 			if (!strncmp(driver_list[i].name, info->driver,
 				     ETHTOOL_BUSINFO_LEN)) {
 				if (driver_list[i].func(info, regs) == 0)
-					return 0;
+					goto nested;
 				/* This version (or some other
 				 * variation in the dump format) is
 				 * not handled; fall back to hex
@@ -1263,6 +1263,15 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
 
 	dump_hex(stdout, regs->data, regs->len, 0);
 
+nested:
+	/* Recurse dump if some drvinfo and regs structures are nested */
+	if (info->regdump_len > regs->len + sizeof(*info) + sizeof(*regs)) {
+		info = (struct ethtool_drvinfo *)(&regs->data[0] + regs->len);
+		regs = (struct ethtool_regs *)(&regs->data[0] + regs->len + sizeof(*info));
+
+		return dump_regs(gregs_dump_raw, gregs_dump_hex, info, regs);
+	}
+
 	return 0;
 }
 
-- 
2.22.0

[PATCH net-next] net: dsa: dump CPU port regs through master

[Vivien Didelot]

Merge the CPU port registers dump into the master interface registers
dump through ethtool, by nesting the ethtool_drvinfo and ethtool_regs
structures of the CPU port into the dump.

drvinfo->regdump_len will contain the full data length, while regs->len
will contain only the master interface registers dump length.

This allows for example to dump the CPU port registers on a ZII Dev
C board like this:

    # ethtool -d eth1
    0x004:                                              0x00000000
    0x008:                                              0x0a8000aa
    0x010:                                              0x01000000
    0x014:                                              0x00000000
    0x024:                                              0xf0000102
    0x040:                                              0x6d82c800
    0x044:                                              0x00000020
    0x064:                                              0x40000000
    0x084: RCR (Receive Control Register)               0x47c00104
        MAX_FL (Maximum frame length)                   1984
        FCE (Flow control enable)                       0
        BC_REJ (Broadcast frame reject)                 0
        PROM (Promiscuous mode)                         0
        DRT (Disable receive on transmit)               0
        LOOP (Internal loopback)                        0
    0x0c4: TCR (Transmit Control Register)              0x00000004
        RFC_PAUSE (Receive frame control pause)         0
        TFC_PAUSE (Transmit frame control pause)        0
        FDEN (Full duplex enable)                       1
        HBC (Heartbeat control)                         0
        GTS (Graceful transmit stop)                    0
    0x0e4:                                              0x76735d6d
    0x0e8:                                              0x7e9e8808
    0x0ec:                                              0x00010000
    .
    .
    .
    88E6352  Switch Port Registers
    ------------------------------
    00: Port Status                            0x4d04
          Pause Enabled                        0
          My Pause                             1
          802.3 PHY Detected                   0
          Link Status                          Up
          Duplex                               Full
          Speed                                100 or 200 Mbps
          EEE Enabled                          0
          Transmitter Paused                   0
          Flow Control                         0
          Config Mode                          0x4
    01: Physical Control                       0x003d
          RGMII Receive Timing Control         Default
          RGMII Transmit Timing Control        Default
          200 BASE Mode                        100
          Flow Control's Forced value          0
          Force Flow Control                   0
          Link's Forced value                  Up
          Force Link                           1
          Duplex's Forced value                Full
          Force Duplex                         1
          Force Speed                          100 or 200 Mbps
    .
    .
    .

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/master.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/net/dsa/master.c b/net/dsa/master.c
index 4b52f8bac5e1..a8e52c9967f4 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -8,6 +8,70 @@
 
 #include "dsa_priv.h"
 
+static int dsa_master_get_regs_len(struct net_device *dev)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	const struct ethtool_ops *ops = cpu_dp->orig_ethtool_ops;
+	struct dsa_switch *ds = cpu_dp->ds;
+	int port = cpu_dp->index;
+	int ret = 0;
+	int len;
+
+	if (ops->get_regs_len) {
+		len = ops->get_regs_len(dev);
+		if (len < 0)
+			return len;
+		ret += len;
+	}
+
+	ret += sizeof(struct ethtool_drvinfo);
+	ret += sizeof(struct ethtool_regs);
+
+	if (ds->ops->get_regs_len) {
+		len = ds->ops->get_regs_len(ds, port);
+		if (len < 0)
+			return len;
+		ret += len;
+	}
+
+	return ret;
+}
+
+static void dsa_master_get_regs(struct net_device *dev,
+				struct ethtool_regs *regs, void *data)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	const struct ethtool_ops *ops = cpu_dp->orig_ethtool_ops;
+	struct dsa_switch *ds = cpu_dp->ds;
+	struct ethtool_drvinfo *cpu_info;
+	struct ethtool_regs *cpu_regs;
+	int port = cpu_dp->index;
+	int len;
+
+	if (ops->get_regs_len && ops->get_regs) {
+		len = ops->get_regs_len(dev);
+		if (len < 0)
+			return;
+		regs->len = len;
+		ops->get_regs(dev, regs, data);
+		data += regs->len;
+	}
+
+	cpu_info = (struct ethtool_drvinfo *)data;
+	strlcpy(cpu_info->driver, "dsa", sizeof(cpu_info->driver));
+	data += sizeof(*cpu_info);
+	cpu_regs = (struct ethtool_regs *)data;
+	data += sizeof(*cpu_regs);
+
+	if (ds->ops->get_regs_len && ds->ops->get_regs) {
+		len = ds->ops->get_regs_len(ds, port);
+		if (len < 0)
+			return;
+		cpu_regs->len = len;
+		ds->ops->get_regs(ds, port, cpu_regs, data);
+	}
+}
+
 static void dsa_master_get_ethtool_stats(struct net_device *dev,
 					 struct ethtool_stats *stats,
 					 uint64_t *data)
@@ -147,6 +211,8 @@ static int dsa_master_ethtool_setup(struct net_device *dev)
 	if (cpu_dp->orig_ethtool_ops)
 		memcpy(ops, cpu_dp->orig_ethtool_ops, sizeof(*ops));
 
+	ops->get_regs_len = dsa_master_get_regs_len;
+	ops->get_regs = dsa_master_get_regs;
 	ops->get_sset_count = dsa_master_get_sset_count;
 	ops->get_ethtool_stats = dsa_master_get_ethtool_stats;
 	ops->get_strings = dsa_master_get_strings;
-- 
2.22.0

Re: [PATCH net-next] net: dsa: dump CPU port regs through master

[David Miller]

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Fri,  2 Aug 2019 15:34:55 -0400

> Merge the CPU port registers dump into the master interface registers
> dump through ethtool, by nesting the ethtool_drvinfo and ethtool_regs
> structures of the CPU port into the dump.
> 
> drvinfo->regdump_len will contain the full data length, while regs->len
> will contain only the master interface registers dump length.
> 
> This allows for example to dump the CPU port registers on a ZII Dev
> C board like this:
 ...
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Applied, thanks Vivien.

Re: [PATCH ethtool] ethtool: dump nested registers

[Michal Kubecek]

On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> Usually kernel drivers set the regs->len value to the same length as
> info->regdump_len, which was used for the allocation. In case where
> regs->len is smaller than the allocated info->regdump_len length,
> we may assume that the dump contains a nested set of registers.
> 
> This becomes handy for kernel drivers to expose registers of an
> underlying network conduit unfortunately not exposed to userspace,
> as found in network switching equipment for example.
> 
> This patch adds support for recursing into the dump operation if there
> is enough room for a nested ethtool_drvinfo structure containing a
> valid driver name, followed by a ethtool_regs structure like this:
> 
>     0      regs->len                        info->regdump_len
>     v              v                                        v
>     +--------------+-----------------+--------------+-- - --+
>     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
>     +--------------+-----------------+--------------+-- - --+
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---

I'm not sure about this approach. If these additional objects with their
own registers are represented by a network device, we can query their
registers directly. If they are not (which, IIUC, is the case in your
use case), we should use an appropriate interface. AFAIK the CPU ports
are already represented in devlink, shouldn't devlink be also used to
query their registers?

>  ethtool.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 05fe05a08..c0e2903c5 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -1245,7 +1245,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
>  
>  	if (gregs_dump_raw) {
>  		fwrite(regs->data, regs->len, 1, stdout);
> -		return 0;
> +		goto nested;
>  	}
>  
>  	if (!gregs_dump_hex)
> @@ -1253,7 +1253,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
>  			if (!strncmp(driver_list[i].name, info->driver,
>  				     ETHTOOL_BUSINFO_LEN)) {
>  				if (driver_list[i].func(info, regs) == 0)
> -					return 0;
> +					goto nested;
>  				/* This version (or some other
>  				 * variation in the dump format) is
>  				 * not handled; fall back to hex
> @@ -1263,6 +1263,15 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
>  
>  	dump_hex(stdout, regs->data, regs->len, 0);
>  
> +nested:
> +	/* Recurse dump if some drvinfo and regs structures are nested */
> +	if (info->regdump_len > regs->len + sizeof(*info) + sizeof(*regs)) {
> +		info = (struct ethtool_drvinfo *)(&regs->data[0] + regs->len);
> +		regs = (struct ethtool_regs *)(&regs->data[0] + regs->len + sizeof(*info));
> +
> +		return dump_regs(gregs_dump_raw, gregs_dump_hex, info, regs);
> +	}
> +
>  	return 0;
>  }
>  

For raw and hex dumps, this will dump only the payloads without any
metadata allowing to identify what are the additional blocks for the
other related objects, i.e. where they start, how long they are and what
they belong to. That doesn't seem very useful.

Michal Kubecek

Re: [PATCH ethtool] ethtool: dump nested registers

[Vivien Didelot]

Hi Michal!

On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > Usually kernel drivers set the regs->len value to the same length as
> > info->regdump_len, which was used for the allocation. In case where
> > regs->len is smaller than the allocated info->regdump_len length,
> > we may assume that the dump contains a nested set of registers.
> > 
> > This becomes handy for kernel drivers to expose registers of an
> > underlying network conduit unfortunately not exposed to userspace,
> > as found in network switching equipment for example.
> > 
> > This patch adds support for recursing into the dump operation if there
> > is enough room for a nested ethtool_drvinfo structure containing a
> > valid driver name, followed by a ethtool_regs structure like this:
> > 
> >     0      regs->len                        info->regdump_len
> >     v              v                                        v
> >     +--------------+-----------------+--------------+-- - --+
> >     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
> >     +--------------+-----------------+--------------+-- - --+
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > ---
> 
> I'm not sure about this approach. If these additional objects with their
> own registers are represented by a network device, we can query their
> registers directly. If they are not (which, IIUC, is the case in your
> use case), we should use an appropriate interface. AFAIK the CPU ports
> are already represented in devlink, shouldn't devlink be also used to
> query their registers?

Yet another interface wasn't that much appropriate for DSA, making the stack
unnecessarily complex. In fact we are already glueing the statistics of the CPU
port into the master's ethtool ops (both physical ports are wired together).
Adding support for nested registers dump in ethtool makes it simple to
(pretty) dump CPU port's registers without too much userspace addition.

> 
> >  ethtool.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ethtool.c b/ethtool.c
> > index 05fe05a08..c0e2903c5 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -1245,7 +1245,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >  
> >  	if (gregs_dump_raw) {
> >  		fwrite(regs->data, regs->len, 1, stdout);
> > -		return 0;
> > +		goto nested;
> >  	}

You're right regarding your comment about raw output. I can keep the return
0 here.

> >  
> >  	if (!gregs_dump_hex)
> > @@ -1253,7 +1253,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >  			if (!strncmp(driver_list[i].name, info->driver,
> >  				     ETHTOOL_BUSINFO_LEN)) {
> >  				if (driver_list[i].func(info, regs) == 0)
> > -					return 0;
> > +					goto nested;
> >  				/* This version (or some other
> >  				 * variation in the dump format) is
> >  				 * not handled; fall back to hex
> > @@ -1263,6 +1263,15 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >  
> >  	dump_hex(stdout, regs->data, regs->len, 0);
> >  
> > +nested:
> > +	/* Recurse dump if some drvinfo and regs structures are nested */
> > +	if (info->regdump_len > regs->len + sizeof(*info) + sizeof(*regs)) {
> > +		info = (struct ethtool_drvinfo *)(&regs->data[0] + regs->len);
> > +		regs = (struct ethtool_regs *)(&regs->data[0] + regs->len + sizeof(*info));
> > +
> > +		return dump_regs(gregs_dump_raw, gregs_dump_hex, info, regs);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> 
> For raw and hex dumps, this will dump only the payloads without any
> metadata allowing to identify what are the additional blocks for the
> other related objects, i.e. where they start, how long they are and what
> they belong to. That doesn't seem very useful.


Thanks,

	Vivien

Re: [PATCH ethtool] ethtool: dump nested registers

[Michal Kubecek]

On Mon, Aug 05, 2019 at 10:52:16AM -0400, Vivien Didelot wrote:
> Hi Michal!
> 
> On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> > On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > > Usually kernel drivers set the regs->len value to the same length as
> > > info->regdump_len, which was used for the allocation. In case where
> > > regs->len is smaller than the allocated info->regdump_len length,
> > > we may assume that the dump contains a nested set of registers.
> > > 
> > > This becomes handy for kernel drivers to expose registers of an
> > > underlying network conduit unfortunately not exposed to userspace,
> > > as found in network switching equipment for example.
> > > 
> > > This patch adds support for recursing into the dump operation if there
> > > is enough room for a nested ethtool_drvinfo structure containing a
> > > valid driver name, followed by a ethtool_regs structure like this:
> > > 
> > >     0      regs->len                        info->regdump_len
> > >     v              v                                        v
> > >     +--------------+-----------------+--------------+-- - --+
> > >     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
> > >     +--------------+-----------------+--------------+-- - --+
> > > 
> > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > > ---
> > 
> > I'm not sure about this approach. If these additional objects with their
> > own registers are represented by a network device, we can query their
> > registers directly. If they are not (which, IIUC, is the case in your
> > use case), we should use an appropriate interface. AFAIK the CPU ports
> > are already represented in devlink, shouldn't devlink be also used to
> > query their registers?
> 
> Yet another interface wasn't that much appropriate for DSA, making the
> stack unnecessarily complex.

AFAICS, there is already devlink support in dsa and CPU ports are
presented as devlink ports. So it wouldn't be a completely new
interface.

> In fact we are already glueing the statistics of the CPU port into the
> master's ethtool ops (both physical ports are wired together).

The ethtool statistics (in general) are one big mess, I don't think it's
an example worth following; rather one showing us what to avoid.

> Adding support for nested registers dump in ethtool makes it simple to
> (pretty) dump CPU port's registers without too much userspace
> addition.

It is indeed convenient for pretty print - but very hard to use for any
automated processing. My point is that CPU port is not represented by
a network device but it is already represented by a devlink port so that
it makes much more sense to tie its register dump to that object than to
add add it to register dump of port's master.

In the future, I would like to transform the ethtool register dump from
current opaque block of data to an actual dump of registers. It is
unfortunate that drivers are already mixing information specific to
a network device with information common for the whole physical device.
Adding more data which is actually related to a different object would
only make things worse.

Michal Kubecek

Re: [PATCH ethtool] ethtool: dump nested registers

[Vivien Didelot]

Hi Michal,

On Tue, 6 Aug 2019 07:20:02 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Mon, Aug 05, 2019 at 10:52:16AM -0400, Vivien Didelot wrote:
> > Hi Michal!
> > 
> > On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> > > On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > > > Usually kernel drivers set the regs->len value to the same length as
> > > > info->regdump_len, which was used for the allocation. In case where
> > > > regs->len is smaller than the allocated info->regdump_len length,
> > > > we may assume that the dump contains a nested set of registers.
> > > > 
> > > > This becomes handy for kernel drivers to expose registers of an
> > > > underlying network conduit unfortunately not exposed to userspace,
> > > > as found in network switching equipment for example.
> > > > 
> > > > This patch adds support for recursing into the dump operation if there
> > > > is enough room for a nested ethtool_drvinfo structure containing a
> > > > valid driver name, followed by a ethtool_regs structure like this:
> > > > 
> > > >     0      regs->len                        info->regdump_len
> > > >     v              v                                        v
> > > >     +--------------+-----------------+--------------+-- - --+
> > > >     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
> > > >     +--------------+-----------------+--------------+-- - --+
> > > > 
> > > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > > > ---
> > > 
> > > I'm not sure about this approach. If these additional objects with their
> > > own registers are represented by a network device, we can query their
> > > registers directly. If they are not (which, IIUC, is the case in your
> > > use case), we should use an appropriate interface. AFAIK the CPU ports
> > > are already represented in devlink, shouldn't devlink be also used to
> > > query their registers?
> > 
> > Yet another interface wasn't that much appropriate for DSA, making the
> > stack unnecessarily complex.
> 
> AFAICS, there is already devlink support in dsa and CPU ports are
> presented as devlink ports. So it wouldn't be a completely new
> interface.
> 
> > In fact we are already glueing the statistics of the CPU port into the
> > master's ethtool ops (both physical ports are wired together).
> 
> The ethtool statistics (in general) are one big mess, I don't think it's
> an example worth following; rather one showing us what to avoid.
> 
> > Adding support for nested registers dump in ethtool makes it simple to
> > (pretty) dump CPU port's registers without too much userspace
> > addition.
> 
> It is indeed convenient for pretty print - but very hard to use for any
> automated processing. My point is that CPU port is not represented by
> a network device but it is already represented by a devlink port so that
> it makes much more sense to tie its register dump to that object than to
> add add it to register dump of port's master.

How would that be presented to userspace? The pretty printing for some
DSA switch ports (e.g. mv88e6xxx) are already added in ethtool. Is there
a devlink-ethtool glue of some kind, or should the pretty printing be
duplicated in yet another tool? I'd prefer to avoid the latter...

> In the future, I would like to transform the ethtool register dump from
> current opaque block of data to an actual dump of registers. It is
> unfortunate that drivers are already mixing information specific to
> a network device with information common for the whole physical device.
> Adding more data which is actually related to a different object would
> only make things worse.

I totally understand and I'm interested to follow this work.


Thanks,

	Vivien

Re: [PATCH ethtool] ethtool: dump nested registers

[Jiri Pirko]

Tue, Aug 06, 2019 at 07:20:02AM CEST, mkubecek@suse.cz wrote:
>On Mon, Aug 05, 2019 at 10:52:16AM -0400, Vivien Didelot wrote:
>> Hi Michal!
>> 
>> On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
>> > On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
>> > > Usually kernel drivers set the regs->len value to the same length as
>> > > info->regdump_len, which was used for the allocation. In case where
>> > > regs->len is smaller than the allocated info->regdump_len length,
>> > > we may assume that the dump contains a nested set of registers.
>> > > 
>> > > This becomes handy for kernel drivers to expose registers of an
>> > > underlying network conduit unfortunately not exposed to userspace,
>> > > as found in network switching equipment for example.
>> > > 
>> > > This patch adds support for recursing into the dump operation if there
>> > > is enough room for a nested ethtool_drvinfo structure containing a
>> > > valid driver name, followed by a ethtool_regs structure like this:
>> > > 
>> > >     0      regs->len                        info->regdump_len
>> > >     v              v                                        v
>> > >     +--------------+-----------------+--------------+-- - --+
>> > >     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
>> > >     +--------------+-----------------+--------------+-- - --+
>> > > 
>> > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>> > > ---
>> > 
>> > I'm not sure about this approach. If these additional objects with their
>> > own registers are represented by a network device, we can query their
>> > registers directly. If they are not (which, IIUC, is the case in your
>> > use case), we should use an appropriate interface. AFAIK the CPU ports
>> > are already represented in devlink, shouldn't devlink be also used to
>> > query their registers?
>> 
>> Yet another interface wasn't that much appropriate for DSA, making the
>> stack unnecessarily complex.
>
>AFAICS, there is already devlink support in dsa and CPU ports are
>presented as devlink ports. So it wouldn't be a completely new
>interface.

I agree that since we have cpu devlink-port object, we should use this
object to carry info of it. Not to "abuse" netdevice of front panel
ports. We already have devlink regions for the purpose of binary dumps.
Currently they are per-devlink, but it should be easy to extend them
per-devlink-port.

Similar to the statistics. I think that they should go to devlink-port
object too.


>
>> In fact we are already glueing the statistics of the CPU port into the
>> master's ethtool ops (both physical ports are wired together).
>
>The ethtool statistics (in general) are one big mess, I don't think it's
>an example worth following; rather one showing us what to avoid.
>
>> Adding support for nested registers dump in ethtool makes it simple to
>> (pretty) dump CPU port's registers without too much userspace
>> addition.
>
>It is indeed convenient for pretty print - but very hard to use for any
>automated processing. My point is that CPU port is not represented by
>a network device but it is already represented by a devlink port so that
>it makes much more sense to tie its register dump to that object than to
>add add it to register dump of port's master.
>
>In the future, I would like to transform the ethtool register dump from
>current opaque block of data to an actual dump of registers. It is
>unfortunate that drivers are already mixing information specific to
>a network device with information common for the whole physical device.
>Adding more data which is actually related to a different object would
>only make things worse.
>
>Michal Kubecek

Re: [PATCH ethtool] ethtool: dump nested registers

[John W. Linville]

On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> Usually kernel drivers set the regs->len value to the same length as
> info->regdump_len, which was used for the allocation. In case where
> regs->len is smaller than the allocated info->regdump_len length,
> we may assume that the dump contains a nested set of registers.
> 
> This becomes handy for kernel drivers to expose registers of an
> underlying network conduit unfortunately not exposed to userspace,
> as found in network switching equipment for example.
> 
> This patch adds support for recursing into the dump operation if there
> is enough room for a nested ethtool_drvinfo structure containing a
> valid driver name, followed by a ethtool_regs structure like this:
> 
>     0      regs->len                        info->regdump_len
>     v              v                                        v
>     +--------------+-----------------+--------------+-- - --+
>     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
>     +--------------+-----------------+--------------+-- - --+
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

I wasn't sure what to do with this one, given the disucssion that
followed. But since Dave merged "net: dsa: dump CPU port regs through
master" for net-next, I went ahead and queued this one for the next
release. If that was the wrong thing to do, speak-up now!

Thanks,

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH ethtool] ethtool: dump nested registers

[Michal Kubecek]

On Fri, Aug 09, 2019 at 12:23:36PM -0400, John W. Linville wrote:
> On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > Usually kernel drivers set the regs->len value to the same length as
> > info->regdump_len, which was used for the allocation. In case where
> > regs->len is smaller than the allocated info->regdump_len length,
> > we may assume that the dump contains a nested set of registers.
> > 
> > This becomes handy for kernel drivers to expose registers of an
> > underlying network conduit unfortunately not exposed to userspace,
> > as found in network switching equipment for example.
> > 
> > This patch adds support for recursing into the dump operation if there
> > is enough room for a nested ethtool_drvinfo structure containing a
> > valid driver name, followed by a ethtool_regs structure like this:
> > 
> >     0      regs->len                        info->regdump_len
> >     v              v                                        v
> >     +--------------+-----------------+--------------+-- - --+
> >     | ethtool_regs | ethtool_drvinfo | ethtool_regs |       |
> >     +--------------+-----------------+--------------+-- - --+
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> 
> I wasn't sure what to do with this one, given the disucssion that
> followed. But since Dave merged "net: dsa: dump CPU port regs through
> master" for net-next, I went ahead and queued this one for the next
> release. If that was the wrong thing to do, speak-up now!

I'm certainly not happy about it as I'm afraid it's going to give me
a headache one day. But as long as the driver packs CPU port registers
into the device's register dump, it doesn't make sense not to allow
ethtool to parse them. And I'm not sure my objections are strong enough
to request a revert of the kernel part as I'm not sure the idea of
transforming the register dump into a structured format (an actual list
of registers rather than an opaque data block) is feasible at all.

So let's keep the patch in.

Michal