Date: Wed, 13 Dec 2017 00:54:39 +0100
From: Michal Kubecek <mkubecek@suse.cz>
To: Jiri Pirko <jiri@resnulli.us>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message
Message-ID: <20171212235439.z6tdj5nzzydwldkd@unicorn.suse.cz>
References: <cover.1513000306.git.mkubecek@suse.cz>
 <219f457821c4cbca64ead6a8f8a92246ed7f32a3.1513000306.git.mkubecek@suse.cz>
 <20171211161601.GB1885@nanopsycho>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171211161601.GB1885@nanopsycho>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2690
Lines: 61

On Mon, Dec 11, 2017 at 05:16:01PM +0100, Jiri Pirko wrote:
> Mon, Dec 11, 2017 at 02:54:01PM CET, mkubecek@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