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

Wed, Dec 13, 2017 at 12:54:39AM CET, mkubecek@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, 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.

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