Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

[Jiri Pirko]

Wed, Dec 13, 2017 at 12:54:39AM CET, mkube...@suse.cz wrote:
>On Mon, Dec 11, 2017 at 05:16:01PM +0100, Jiri Pirko wrote:
>> Mon, Dec 11, 2017 at 02:54:01PM CET, mkube...@suse.cz wrote:
>> >+
>> >+ETHA_DRVINFO_DRIVER(string)driver name
>> >+ETHA_DRVINFO_VERSION   (string)driver version
>> 
>> You use 3 prefixes:
>> ETHTOOL_ for cmd
>> ETHA_ for attrs
>> ethnl_ for function
>> 
>> I suggest to sync this, perhaps to:
>> ETHNL_CMD_*
>> ETHNL_ATTR_*
>> ethnl_*
>> ?
>
>Switching to ETHNL_CMD_* is certainly a good idea. For attributes, I'm a
>bit worried that ETHNL_ATTR_ prefix might make it even harder to fit
>into the 80 characters per line limit. I'll try and see what it would
>look like.
>
>> >+ETHA_DRVINFO_FWVERSION (string)firmware version
>> >+ETHA_DRVINFO_BUSINFO   (string)device bus address
>> >+ETHA_DRVINFO_EROM_VER  (string)expansion ROM version
>> >+ETHA_DRVINFO_N_PRIV_FLAGS  (u32)   number of private flags
>> >+ETHA_DRVINFO_N_STATS   (u32)   number of device stats
>> >+ETHA_DRVINFO_TESTINFO_LEN  (u32)   number of test results
>> >+ETHA_DRVINFO_EEDUMP_LEN(u32)   EEPROM dump size
>> >+ETHA_DRVINFO_REGDUMP_LEN   (u32)   register dump size
>> 
>> We are now working on providing various fw memory regions dump in
>> devlink. It makes sense to have it in devlink for couple of reasons:
>> 1) per-asic, not netdev specific, therefore does not really make sense
>>to have netdev as handle, but rather devlink handle.
>> 2) snapshot support - we need to provide support for getting snapshot
>>(for example on failure), transferring to user and deleting it
>>(remove from driver memory).
>> 
>> Also, driver name, version, fwversion, etc is per-asic. Would make sense
>> to have it in devlink as well.
>> 
>> I think this is great opprotunity to move things where they should be to
>> be alligned with the current world and kernel infrastructure.
>
>IMHO this depends on the question whether we are going to rework also
>the interface between kernel and NIC drivers (currently ethtool_ops).
>If we are, it would make good sense to split the information in both
>interfaces. If not, it doesn't seem to make userspace do two queries,
>each getting one part of the same information provided by NIC.

Yeah, agreed. I believe we should.


>
>Also, I must admit that one thing I dislike about the iw tool is that it
>pushes me to distinguish between operations on "phy" and "dev". While
>I understand that it makes perfect sense for someone familiar with the
>internals, it's quite annoying for an occasional "consumer". It would be
>sad if we created a set of perfectly logical and clean tools and
>(almost) 20 years later, people still used old ioctl based ethtool
>because "it's what they are used to and it works just fine for them"
>(like they do with ifconfig, netstat or brctl).

Good documentation should take care of that.



>
>Michal

Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

[Michal Kubecek]

On Mon, Dec 11, 2017 at 05:16:01PM +0100, Jiri Pirko wrote:
> Mon, Dec 11, 2017 at 02:54:01PM CET, mkube...@suse.cz wrote:
> >+
> >+ETHA_DRVINFO_DRIVER (string)driver name
> >+ETHA_DRVINFO_VERSION(string)driver version
> 
> You use 3 prefixes:
> ETHTOOL_ for cmd
> ETHA_ for attrs
> ethnl_ for function
> 
> I suggest to sync this, perhaps to:
> ETHNL_CMD_*
> ETHNL_ATTR_*
> ethnl_*
> ?

Switching to ETHNL_CMD_* is certainly a good idea. For attributes, I'm a
bit worried that ETHNL_ATTR_ prefix might make it even harder to fit
into the 80 characters per line limit. I'll try and see what it would
look like.

> >+ETHA_DRVINFO_FWVERSION  (string)firmware version
> >+ETHA_DRVINFO_BUSINFO(string)device bus address
> >+ETHA_DRVINFO_EROM_VER   (string)expansion ROM version
> >+ETHA_DRVINFO_N_PRIV_FLAGS   (u32)   number of private flags
> >+ETHA_DRVINFO_N_STATS(u32)   number of device stats
> >+ETHA_DRVINFO_TESTINFO_LEN   (u32)   number of test results
> >+ETHA_DRVINFO_EEDUMP_LEN (u32)   EEPROM dump size
> >+ETHA_DRVINFO_REGDUMP_LEN(u32)   register dump size
> 
> We are now working on providing various fw memory regions dump in
> devlink. It makes sense to have it in devlink for couple of reasons:
> 1) per-asic, not netdev specific, therefore does not really make sense
>to have netdev as handle, but rather devlink handle.
> 2) snapshot support - we need to provide support for getting snapshot
>(for example on failure), transferring to user and deleting it
>(remove from driver memory).
> 
> Also, driver name, version, fwversion, etc is per-asic. Would make sense
> to have it in devlink as well.
> 
> I think this is great opprotunity to move things where they should be to
> be alligned with the current world and kernel infrastructure.

IMHO this depends on the question whether we are going to rework also
the interface between kernel and NIC drivers (currently ethtool_ops).
If we are, it would make good sense to split the information in both
interfaces. If not, it doesn't seem to make userspace do two queries,
each getting one part of the same information provided by NIC.

Also, I must admit that one thing I dislike about the iw tool is that it
pushes me to distinguish between operations on "phy" and "dev". While
I understand that it makes perfect sense for someone familiar with the
internals, it's quite annoying for an occasional "consumer". It would be
sad if we created a set of perfectly logical and clean tools and
(almost) 20 years later, people still used old ioctl based ethtool
because "it's what they are used to and it works just fine for them"
(like they do with ifconfig, netstat or brctl).

Michal

Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

[Jiri Pirko]

Mon, Dec 11, 2017 at 02:54:01PM CET, mkube...@suse.cz wrote:
>Request the same information as ETHTOOL_GDRVINFO. This is read-only so that
>corresponding SET_DRVINFO exists but is only used in kernel replies. Rip
>the code to query the driver out of the legacy interface and move it to
>a new file ethtool_common.c so that both interfaces can use it.
>
>Signed-off-by: Michal Kubecek 
>---
> Documentation/networking/ethtool-netlink.txt | 30 ++-
> include/uapi/linux/ethtool_netlink.h | 21 
> net/core/Makefile|  2 +-
> net/core/ethtool.c   | 42 +++-
> net/core/ethtool_common.c| 46 +
> net/core/ethtool_common.h| 11 
> net/core/ethtool_netlink.c   | 75 
> 7 files changed, 189 insertions(+), 38 deletions(-)
> create mode 100644 net/core/ethtool_common.c
> create mode 100644 net/core/ethtool_common.h
>
>diff --git a/Documentation/networking/ethtool-netlink.txt 
>b/Documentation/networking/ethtool-netlink.txt
>index 893e5156f6a7..cb992180b211 100644
>--- a/Documentation/networking/ethtool-netlink.txt
>+++ b/Documentation/networking/ethtool-netlink.txt
>@@ -107,6 +107,9 @@ which the request applies.
> List of message types
> -
> 
>+ETHTOOL_CMD_GET_DRVINFO
>+ETHTOOL_CMD_SET_DRVINFO   response only
>+
> All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
> indicate the type.
> 
>@@ -119,6 +122,31 @@ messages marked as "response only" in the table above.
> Later sections describe the format and semantics of these request messages.
> 
> 
>+GET_DRVINFO
>+---
>+
>+GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
>+basic driver information. The request doesn't use any attributes and flags,
>+info_mask and index field in request header are ignored. Kernel response
>+contents:
>+
>+ETHA_DRVINFO_DRIVER   (string)driver name
>+ETHA_DRVINFO_VERSION  (string)driver version

You use 3 prefixes:
ETHTOOL_ for cmd
ETHA_ for attrs
ethnl_ for function

I suggest to sync this, perhaps to:
ETHNL_CMD_*
ETHNL_ATTR_*
ethnl_*
?


>+ETHA_DRVINFO_FWVERSION(string)firmware version
>+ETHA_DRVINFO_BUSINFO  (string)device bus address
>+ETHA_DRVINFO_EROM_VER (string)expansion ROM version
>+ETHA_DRVINFO_N_PRIV_FLAGS (u32)   number of private flags
>+ETHA_DRVINFO_N_STATS  (u32)   number of device stats
>+ETHA_DRVINFO_TESTINFO_LEN (u32)   number of test results
>+ETHA_DRVINFO_EEDUMP_LEN   (u32)   EEPROM dump size
>+ETHA_DRVINFO_REGDUMP_LEN  (u32)   register dump size

We are now working on providing various fw memory regions dump in
devlink. It makes sense to have it in devlink for couple of reasons:
1) per-asic, not netdev specific, therefore does not really make sense
   to have netdev as handle, but rather devlink handle.
2) snapshot support - we need to provide support for getting snapshot
   (for example on failure), transferring to user and deleting it
   (remove from driver memory).

Also, driver name, version, fwversion, etc is per-asic. Would make sense
to have it in devlink as well.

I think this is great opprotunity to move things where they should be to
be alligned with the current world and kernel infrastructure.