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

[Jiri Pirko]

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