[net-next] net/core: Replace driver version to be kernel version
diff mbox series

Message ID 20200123130541.30473-1-leon@kernel.org
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • [net-next] net/core: Replace driver version to be kernel version
Related show

Commit Message

Leon Romanovsky Jan. 23, 2020, 1:05 p.m. UTC
From: Leon Romanovsky <leonro@mellanox.com>

In order to stop useless driver version bumps and unify output
presented by ethtool -i, let's overwrite the version string.

Before this change:
[leonro@erver ~]$ ethtool -i eth0
driver: virtio_net
version: 1.0.0
After this change:
[leonro@server ~]$ ethtool -i eth0
driver: virtio_net
version: 5.5.0-rc6+

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
I wanted to change to VERMAGIC_STRING, but the output doesn't
look pleasant to my taste and on my system is truncated to be
"version: 5.5.0-rc6+ SMP mod_unload modve".

After this patch, we can drop all those version assignments
from the drivers.

Inspired by nfp and hns code.
---
 net/core/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

--
2.20.1

Comments

Jakub Kicinski Jan. 23, 2020, 2:40 p.m. UTC | #1
On Thu, 23 Jan 2020 15:05:41 +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> In order to stop useless driver version bumps and unify output
> presented by ethtool -i, let's overwrite the version string.
> 
> Before this change:
> [leonro@erver ~]$ ethtool -i eth0
> driver: virtio_net
> version: 1.0.0
> After this change:
> [leonro@server ~]$ ethtool -i eth0
> driver: virtio_net
> version: 5.5.0-rc6+
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

I wonder how hard it would be to tell Coccinelle to remove all prints
in the drivers.  Removing related defines would probably be a little
painful/manual. Hm..

Anyway, you gotta rebase on net-next, the ethtool code got moved around
:)
Leon Romanovsky Jan. 23, 2020, 2:54 p.m. UTC | #2
On Thu, Jan 23, 2020 at 06:40:06AM -0800, Jakub Kicinski wrote:
> On Thu, 23 Jan 2020 15:05:41 +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > In order to stop useless driver version bumps and unify output
> > presented by ethtool -i, let's overwrite the version string.
> >
> > Before this change:
> > [leonro@erver ~]$ ethtool -i eth0
> > driver: virtio_net
> > version: 1.0.0
> > After this change:
> > [leonro@server ~]$ ethtool -i eth0
> > driver: virtio_net
> > version: 5.5.0-rc6+
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>
> I wonder how hard it would be to tell Coccinelle to remove all prints
> in the drivers.  Removing related defines would probably be a little
> painful/manual. Hm..

It is also unclear how can we automatically catch new comers.

>
> Anyway, you gotta rebase on net-next, the ethtool code got moved around
> :)

I tried it now and It applies cleanly on top of commit
6d9f6e6790e7 Merge branch 'net-sched-add-Flow-Queue-PIE-packet-scheduler'

Thanks
Jakub Kicinski Jan. 23, 2020, 3:17 p.m. UTC | #3
On January 23, 2020 6:54:42 AM PST, Leon Romanovsky <leon@kernel.org> wrote:
>> Anyway, you gotta rebase on net-next, the ethtool code got moved
>around
>> :)
>
>I tried it now and It applies cleanly on top of commit
>6d9f6e6790e7 Merge branch
>'net-sched-add-Flow-Queue-PIE-packet-scheduler'

Git is magic.
David Miller Jan. 25, 2020, 9:13 a.m. UTC | #4
From: Leon Romanovsky <leon@kernel.org>
Date: Thu, 23 Jan 2020 15:05:41 +0200

> From: Leon Romanovsky <leonro@mellanox.com>
> 
> In order to stop useless driver version bumps and unify output
> presented by ethtool -i, let's overwrite the version string.
> 
> Before this change:
> [leonro@erver ~]$ ethtool -i eth0
> driver: virtio_net
> version: 1.0.0
> After this change:
> [leonro@server ~]$ ethtool -i eth0
> driver: virtio_net
> version: 5.5.0-rc6+
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

Please respin on current net-next, thank you :)
Shannon Nelson Jan. 26, 2020, 6:56 p.m. UTC | #5
On 1/23/20 5:05 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky<leonro@mellanox.com>
>
> In order to stop useless driver version bumps and unify output
> presented by ethtool -i, let's overwrite the version string.
>
> Before this change:
> [leonro@erver ~]$ ethtool -i eth0
> driver: virtio_net
> version: 1.0.0
> After this change:
> [leonro@server ~]$ ethtool -i eth0
> driver: virtio_net
> version: 5.5.0-rc6+
>
> Signed-off-by: Leon Romanovsky<leonro@mellanox.com>
> ---
> I wanted to change to VERMAGIC_STRING, but the output doesn't
> look pleasant to my taste and on my system is truncated to be
> "version: 5.5.0-rc6+ SMP mod_unload modve".
>
> After this patch, we can drop all those version assignments
> from the drivers.
>
> Inspired by nfp and hns code.
> ---
>   net/core/ethtool.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index cd9bc67381b2..3c6fb13a78bf 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -17,6 +17,7 @@
>   #include <linux/phy.h>
>   #include <linux/bitops.h>
>   #include <linux/uaccess.h>
> +#include <linux/vermagic.h>
>   #include <linux/vmalloc.h>
>   #include <linux/sfp.h>
>   #include <linux/slab.h>
> @@ -776,6 +777,8 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
>   		return -EOPNOTSUPP;
>   	}
>
> +	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
> +
>   	/*
>   	 * this method of obtaining string set info is deprecated;
>   	 * Use ETHTOOL_GSSET_INFO instead.
> --
> 2.20.1
>

First of all, although I've seen some of the arguments about distros and 
their backporting, I still believe that the driver version number is 
useful.  In most cases it at least gets us in the ballpark of what 
generation the driver happens to be and is still useful. I'd really 
prefer that it is just left alone for the device manufactures and their 
support folks to deal with.

Fine, I'm sure I lose that argument since there's already been plenty of 
discussion about it.

Meanwhile, there is some non-zero number of support scripts and 
processes, possibly internal testing chains, that use that driver/vendor 
specific version information and will be broken by this change.  Small 
number?  Large number?  I don't know, but we're breaking them.

Sure, I probably easily lose that argument too, but it still should be 
stated.

This will end up affecting out-of-tree drivers as well, where it is 
useful to know what the version number is, most especially since it is 
different from what the kernel provided driver is.  How else are we to 
get this information out to the user?  If this feature gets squashed, 
we'll end up having to abuse some other mechanism so we can get the live 
information from the driver, and probably each vendor will find a 
different way to sneak it out, giving us more chaos than where we 
started.  At least the ethtool version field is a known and consistent 
place for the version info.

Of course, out-of-tree drivers are not first class citizens, so I 
probably lose that argument as well.

So if you are so all fired up about not allowing the drivers to report 
their own version number, then why report anything at all? Maybe just 
report a blank field.  As some have said, the uname info is already 
available else where, why are we sticking it here?

Personally, I think this is a rather arbitrary, heavy handed and 
unnecessary slam on the drivers, and will make support more difficult in 
the long run.

sln
Leon Romanovsky Jan. 26, 2020, 7:41 p.m. UTC | #6
On Sun, Jan 26, 2020 at 10:56:17AM -0800, Shannon Nelson wrote:
> On 1/23/20 5:05 AM, Leon Romanovsky wrote:
> > From: Leon Romanovsky<leonro@mellanox.com>
> >
> > In order to stop useless driver version bumps and unify output
> > presented by ethtool -i, let's overwrite the version string.
> >
> > Before this change:
> > [leonro@erver ~]$ ethtool -i eth0
> > driver: virtio_net
> > version: 1.0.0
> > After this change:
> > [leonro@server ~]$ ethtool -i eth0
> > driver: virtio_net
> > version: 5.5.0-rc6+
> >
> > Signed-off-by: Leon Romanovsky<leonro@mellanox.com>
> > ---
> > I wanted to change to VERMAGIC_STRING, but the output doesn't
> > look pleasant to my taste and on my system is truncated to be
> > "version: 5.5.0-rc6+ SMP mod_unload modve".
> >
> > After this patch, we can drop all those version assignments
> > from the drivers.
> >
> > Inspired by nfp and hns code.
> > ---
> >   net/core/ethtool.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index cd9bc67381b2..3c6fb13a78bf 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -17,6 +17,7 @@
> >   #include <linux/phy.h>
> >   #include <linux/bitops.h>
> >   #include <linux/uaccess.h>
> > +#include <linux/vermagic.h>
> >   #include <linux/vmalloc.h>
> >   #include <linux/sfp.h>
> >   #include <linux/slab.h>
> > @@ -776,6 +777,8 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
> >   		return -EOPNOTSUPP;
> >   	}
> >
> > +	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
> > +
> >   	/*
> >   	 * this method of obtaining string set info is deprecated;
> >   	 * Use ETHTOOL_GSSET_INFO instead.
> > --
> > 2.20.1
> >
>
> First of all, although I've seen some of the arguments about distros and
> their backporting, I still believe that the driver version number is
> useful.  In most cases it at least gets us in the ballpark of what
> generation the driver happens to be and is still useful. I'd really prefer
> that it is just left alone for the device manufactures and their support
> folks to deal with.
>
> Fine, I'm sure I lose that argument since there's already been plenty of
> discussion about it.
>
> Meanwhile, there is some non-zero number of support scripts and processes,
> possibly internal testing chains, that use that driver/vendor specific
> version information and will be broken by this change.  Small number?  Large
> number?  I don't know, but we're breaking them.
>
> Sure, I probably easily lose that argument too, but it still should be
> stated.
>
> This will end up affecting out-of-tree drivers as well, where it is useful
> to know what the version number is, most especially since it is different
> from what the kernel provided driver is.  How else are we to get this
> information out to the user?  If this feature gets squashed, we'll end up
> having to abuse some other mechanism so we can get the live information from
> the driver, and probably each vendor will find a different way to sneak it
> out, giving us more chaos than where we started.  At least the ethtool
> version field is a known and consistent place for the version info.
>
> Of course, out-of-tree drivers are not first class citizens, so I probably
> lose that argument as well.
>
> So if you are so all fired up about not allowing the drivers to report their
> own version number, then why report anything at all? Maybe just report a
> blank field.  As some have said, the uname info is already available else
> where, why are we sticking it here?
>
> Personally, I think this is a rather arbitrary, heavy handed and unnecessary
> slam on the drivers, and will make support more difficult in the long run.

The thing is that leaving this field as empty, for sure will break all
applications. I have a feeling that it can be close to 100% hit rate.
So, kernel version was chosen as an option, because it is already
successfully in use by at least two drivers (nfp and hns).

Leaving to deal with driver version to vendors is not an option too,
because they prove for more than once that they are not capable to
define user visible interfaces. It comes due to their natural believe
that their company is alone in the world and user visible interface
should be suitable for them only.

It is already impossible for users to distinguish properly versions
of different vendors, because they use arbitrary strings with some
numbers.

Thanks

>
> sln
>
Jakub Kicinski Jan. 26, 2020, 8:49 p.m. UTC | #7
On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:
> > This will end up affecting out-of-tree drivers as well, where it is useful
> > to know what the version number is, most especially since it is different
> > from what the kernel provided driver is.  How else are we to get this
> > information out to the user?  If this feature gets squashed, we'll end up
> > having to abuse some other mechanism so we can get the live information from
> > the driver, and probably each vendor will find a different way to sneak it
> > out, giving us more chaos than where we started.  At least the ethtool
> > version field is a known and consistent place for the version info.
> >
> > Of course, out-of-tree drivers are not first class citizens, so I probably
> > lose that argument as well.
> >
> > So if you are so all fired up about not allowing the drivers to report their
> > own version number, then why report anything at all? Maybe just report a
> > blank field.  As some have said, the uname info is already available else
> > where, why are we sticking it here?
> >
> > Personally, I think this is a rather arbitrary, heavy handed and unnecessary
> > slam on the drivers, and will make support more difficult in the long run.  
> 
> The thing is that leaving this field as empty, for sure will break all
> applications. I have a feeling that it can be close to 100% hit rate.
> So, kernel version was chosen as an option, because it is already
> successfully in use by at least two drivers (nfp and hns).

Shannon does have a point that out of tree drivers still make use of
this field. Perhaps it would be a more suitable first step to print the
kernel version as default and add a comment saying upstream modules
shouldn't overwrite it (perhaps one day CI can catch new violators).

The NFP reports the git hash of the driver source plus the string
"(oot)" for out-of-tree:

https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315

> Leaving to deal with driver version to vendors is not an option too,
> because they prove for more than once that they are not capable to
> define user visible interfaces. It comes due to their natural believe
> that their company is alone in the world and user visible interface
> should be suitable for them only.
> 
> It is already impossible for users to distinguish properly versions
> of different vendors, because they use arbitrary strings with some
> numbers.

That is true. But reporting the kernel version without even as much as
in-tree/out-of-tree indication makes the field entirely meaningless.
Shannon Nelson Jan. 26, 2020, 8:52 p.m. UTC | #8
On 1/26/20 11:41 AM, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 10:56:17AM -0800, Shannon Nelson wrote:
>> On 1/23/20 5:05 AM, Leon Romanovsky wrote:
>>> From: Leon Romanovsky<leonro@mellanox.com>
>>>
>>> In order to stop useless driver version bumps and unify output
>>> presented by ethtool -i, let's overwrite the version string.
>>>
>>> Before this change:
>>> [leonro@erver ~]$ ethtool -i eth0
>>> driver: virtio_net
>>> version: 1.0.0
>>> After this change:
>>> [leonro@server ~]$ ethtool -i eth0
>>> driver: virtio_net
>>> version: 5.5.0-rc6+
>>>
>>> Signed-off-by: Leon Romanovsky<leonro@mellanox.com>
>>> ---
>>> I wanted to change to VERMAGIC_STRING, but the output doesn't
>>> look pleasant to my taste and on my system is truncated to be
>>> "version: 5.5.0-rc6+ SMP mod_unload modve".
>>>
>>> After this patch, we can drop all those version assignments
>>> from the drivers.
>>>
>>> Inspired by nfp and hns code.
>>> ---
>>>    net/core/ethtool.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>>> index cd9bc67381b2..3c6fb13a78bf 100644
>>> --- a/net/core/ethtool.c
>>> +++ b/net/core/ethtool.c
>>> @@ -17,6 +17,7 @@
>>>    #include <linux/phy.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/uaccess.h>
>>> +#include <linux/vermagic.h>
>>>    #include <linux/vmalloc.h>
>>>    #include <linux/sfp.h>
>>>    #include <linux/slab.h>
>>> @@ -776,6 +777,8 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
>>>    		return -EOPNOTSUPP;
>>>    	}
>>>
>>> +	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
>>> +
>>>    	/*
>>>    	 * this method of obtaining string set info is deprecated;
>>>    	 * Use ETHTOOL_GSSET_INFO instead.
>>> --
>>> 2.20.1
>>>
>> First of all, although I've seen some of the arguments about distros and
>> their backporting, I still believe that the driver version number is
>> useful.  In most cases it at least gets us in the ballpark of what
>> generation the driver happens to be and is still useful. I'd really prefer
>> that it is just left alone for the device manufactures and their support
>> folks to deal with.
>>
>> Fine, I'm sure I lose that argument since there's already been plenty of
>> discussion about it.
>>
>> Meanwhile, there is some non-zero number of support scripts and processes,
>> possibly internal testing chains, that use that driver/vendor specific
>> version information and will be broken by this change.  Small number?  Large
>> number?  I don't know, but we're breaking them.
>>
>> Sure, I probably easily lose that argument too, but it still should be
>> stated.
>>
>> This will end up affecting out-of-tree drivers as well, where it is useful
>> to know what the version number is, most especially since it is different
>> from what the kernel provided driver is.  How else are we to get this
>> information out to the user?  If this feature gets squashed, we'll end up
>> having to abuse some other mechanism so we can get the live information from
>> the driver, and probably each vendor will find a different way to sneak it
>> out, giving us more chaos than where we started.  At least the ethtool
>> version field is a known and consistent place for the version info.
>>
>> Of course, out-of-tree drivers are not first class citizens, so I probably
>> lose that argument as well.
>>
>> So if you are so all fired up about not allowing the drivers to report their
>> own version number, then why report anything at all? Maybe just report a
>> blank field.  As some have said, the uname info is already available else
>> where, why are we sticking it here?
>>
>> Personally, I think this is a rather arbitrary, heavy handed and unnecessary
>> slam on the drivers, and will make support more difficult in the long run.
> The thing is that leaving this field as empty, for sure will break all
> applications. I have a feeling that it can be close to 100% hit rate.
> So, kernel version was chosen as an option, because it is already
> successfully in use by at least two drivers (nfp and hns).

I'm glad that works for those drivers.

> Leaving to deal with driver version to vendors is not an option too,
> because they prove for more than once that they are not capable to
> define user visible interfaces. It comes due to their natural believe
> that their company is alone in the world and user visible interface
> should be suitable for them only.

So you want to remove the one reliable place to put some information we 
find useful, thus forcing us to come up with creative new places to put 
it?  Shall we remove the firmware version number as well when we start 
abusing it by adding driver version information?

There has been a lot of work over the years to try to corral and unify 
various bits of information so that everyone can access it the same way, 
and now you're trying to remove one of those methods. This will only 
force driver writers to get "creative" on how to get the info they need 
out to the user.

> It is already impossible for users to distinguish properly versions
> of different vendors, because they use arbitrary strings with some
> numbers.

Shall we also fix it so those pesky distros can't add their own 
arbitrary version numbers to the kernel?

Again, I think you are trying to remove a useful bit of information.  
Just because it isn't useful to you doesn't mean it is useless to others.

sln
Leon Romanovsky Jan. 26, 2020, 9:08 p.m. UTC | #9
On Sun, Jan 26, 2020 at 12:49:57PM -0800, Jakub Kicinski wrote:
> On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:
> > > This will end up affecting out-of-tree drivers as well, where it is useful
> > > to know what the version number is, most especially since it is different
> > > from what the kernel provided driver is.  How else are we to get this
> > > information out to the user?  If this feature gets squashed, we'll end up
> > > having to abuse some other mechanism so we can get the live information from
> > > the driver, and probably each vendor will find a different way to sneak it
> > > out, giving us more chaos than where we started.  At least the ethtool
> > > version field is a known and consistent place for the version info.
> > >
> > > Of course, out-of-tree drivers are not first class citizens, so I probably
> > > lose that argument as well.
> > >
> > > So if you are so all fired up about not allowing the drivers to report their
> > > own version number, then why report anything at all? Maybe just report a
> > > blank field.  As some have said, the uname info is already available else
> > > where, why are we sticking it here?
> > >
> > > Personally, I think this is a rather arbitrary, heavy handed and unnecessary
> > > slam on the drivers, and will make support more difficult in the long run.
> >
> > The thing is that leaving this field as empty, for sure will break all
> > applications. I have a feeling that it can be close to 100% hit rate.
> > So, kernel version was chosen as an option, because it is already
> > successfully in use by at least two drivers (nfp and hns).
>
> Shannon does have a point that out of tree drivers still make use of
> this field. Perhaps it would be a more suitable first step to print the
> kernel version as default and add a comment saying upstream modules
> shouldn't overwrite it (perhaps one day CI can catch new violators).

Jakub,

Shannon proposed to remove this field and it was me who said no :)
My plan is to overwrite ->version, delete all users and add
WARN_ONEC(strcpy(..->version_)...) inside net/ethtool/ to catch
abusers.

>
> The NFP reports the git hash of the driver source plus the string
> "(oot)" for out-of-tree:
>
> https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
> https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315

I was inspired by upstream code.
https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c#L184

>
> > Leaving to deal with driver version to vendors is not an option too,
> > because they prove for more than once that they are not capable to
> > define user visible interfaces. It comes due to their natural believe
> > that their company is alone in the world and user visible interface
> > should be suitable for them only.
> >
> > It is already impossible for users to distinguish properly versions
> > of different vendors, because they use arbitrary strings with some
> > numbers.
>
> That is true. But reporting the kernel version without even as much as
> in-tree/out-of-tree indication makes the field entirely meaningless.

The long-standing policy in kernel that we don't really care about
out-of-tree code.

Thanks
Shannon Nelson Jan. 26, 2020, 9:17 p.m. UTC | #10
On 1/26/20 1:08 PM, Leon Romanovsky wrote:
> The long-standing policy in kernel that we don't really care about
> out-of-tree code.

That doesn't mean we need to be aggressively against out-of-tree code.  
One of the positive points about Linux and loadable modules has always 
been the flexibility that allows and encourages innovation, and helps 
enable more work and testing before a driver can become a fully-fledged 
part of the kernel.  This move actively discourages part of that 
flexibility and I think it is breaking part of the usefulness of modules.

sln
Leon Romanovsky Jan. 26, 2020, 9:19 p.m. UTC | #11
On Sun, Jan 26, 2020 at 12:52:05PM -0800, Shannon Nelson wrote:
> On 1/26/20 11:41 AM, Leon Romanovsky wrote:
> > On Sun, Jan 26, 2020 at 10:56:17AM -0800, Shannon Nelson wrote:
> > > On 1/23/20 5:05 AM, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky<leonro@mellanox.com>
> > > >
> > > > In order to stop useless driver version bumps and unify output
> > > > presented by ethtool -i, let's overwrite the version string.
> > > >
> > > > Before this change:
> > > > [leonro@erver ~]$ ethtool -i eth0
> > > > driver: virtio_net
> > > > version: 1.0.0
> > > > After this change:
> > > > [leonro@server ~]$ ethtool -i eth0
> > > > driver: virtio_net
> > > > version: 5.5.0-rc6+
> > > >
> > > > Signed-off-by: Leon Romanovsky<leonro@mellanox.com>
> > > > ---
> > > > I wanted to change to VERMAGIC_STRING, but the output doesn't
> > > > look pleasant to my taste and on my system is truncated to be
> > > > "version: 5.5.0-rc6+ SMP mod_unload modve".
> > > >
> > > > After this patch, we can drop all those version assignments
> > > > from the drivers.
> > > >
> > > > Inspired by nfp and hns code.
> > > > ---
> > > >    net/core/ethtool.c | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > > > index cd9bc67381b2..3c6fb13a78bf 100644
> > > > --- a/net/core/ethtool.c
> > > > +++ b/net/core/ethtool.c
> > > > @@ -17,6 +17,7 @@
> > > >    #include <linux/phy.h>
> > > >    #include <linux/bitops.h>
> > > >    #include <linux/uaccess.h>
> > > > +#include <linux/vermagic.h>
> > > >    #include <linux/vmalloc.h>
> > > >    #include <linux/sfp.h>
> > > >    #include <linux/slab.h>
> > > > @@ -776,6 +777,8 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
> > > >    		return -EOPNOTSUPP;
> > > >    	}
> > > >
> > > > +	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
> > > > +
> > > >    	/*
> > > >    	 * this method of obtaining string set info is deprecated;
> > > >    	 * Use ETHTOOL_GSSET_INFO instead.
> > > > --
> > > > 2.20.1
> > > >
> > > First of all, although I've seen some of the arguments about distros and
> > > their backporting, I still believe that the driver version number is
> > > useful.  In most cases it at least gets us in the ballpark of what
> > > generation the driver happens to be and is still useful. I'd really prefer
> > > that it is just left alone for the device manufactures and their support
> > > folks to deal with.
> > >
> > > Fine, I'm sure I lose that argument since there's already been plenty of
> > > discussion about it.
> > >
> > > Meanwhile, there is some non-zero number of support scripts and processes,
> > > possibly internal testing chains, that use that driver/vendor specific
> > > version information and will be broken by this change.  Small number?  Large
> > > number?  I don't know, but we're breaking them.
> > >
> > > Sure, I probably easily lose that argument too, but it still should be
> > > stated.
> > >
> > > This will end up affecting out-of-tree drivers as well, where it is useful
> > > to know what the version number is, most especially since it is different
> > > from what the kernel provided driver is.  How else are we to get this
> > > information out to the user?  If this feature gets squashed, we'll end up
> > > having to abuse some other mechanism so we can get the live information from
> > > the driver, and probably each vendor will find a different way to sneak it
> > > out, giving us more chaos than where we started.  At least the ethtool
> > > version field is a known and consistent place for the version info.
> > >
> > > Of course, out-of-tree drivers are not first class citizens, so I probably
> > > lose that argument as well.
> > >
> > > So if you are so all fired up about not allowing the drivers to report their
> > > own version number, then why report anything at all? Maybe just report a
> > > blank field.  As some have said, the uname info is already available else
> > > where, why are we sticking it here?
> > >
> > > Personally, I think this is a rather arbitrary, heavy handed and unnecessary
> > > slam on the drivers, and will make support more difficult in the long run.
> > The thing is that leaving this field as empty, for sure will break all
> > applications. I have a feeling that it can be close to 100% hit rate.
> > So, kernel version was chosen as an option, because it is already
> > successfully in use by at least two drivers (nfp and hns).
>
> I'm glad that works for those drivers.
>
> > Leaving to deal with driver version to vendors is not an option too,
> > because they prove for more than once that they are not capable to
> > define user visible interfaces. It comes due to their natural believe
> > that their company is alone in the world and user visible interface
> > should be suitable for them only.
>
> So you want to remove the one reliable place to put some information we find
> useful, thus forcing us to come up with creative new places to put it? 

What exactly do you find reliable in random string printed by ethtool?

> Shall we remove the firmware version number as well when we start abusing it
> by adding driver version information?

It is unrelated and I have hard time to understand your question, firmware
is not part of the kernel and firmware version needed to tight specific driver
to specific version. It doesn't need to be general by definition.

>
> There has been a lot of work over the years to try to corral and unify
> various bits of information so that everyone can access it the same way, and
> now you're trying to remove one of those methods. This will only force
> driver writers to get "creative" on how to get the info they need out to the
> user.

I'm not removing, but fixing something that was broken for a long time
already. The version thing comes from pre-git era and it doesn't make
sense for a long time already.

>
> > It is already impossible for users to distinguish properly versions
> > of different vendors, because they use arbitrary strings with some
> > numbers.
>
> Shall we also fix it so those pesky distros can't add their own arbitrary
> version numbers to the kernel?
>
> Again, I think you are trying to remove a useful bit of information.  Just
> because it isn't useful to you doesn't mean it is useless to others.
>

Again, I'm not removing it.

Feel free to start discussion in ksummit mailing list on why it is so
useful that we need to update driver version independently from the
main kernel version.

If you success to convince Linus and Greg, we will be happy to update
our driver version every week like a clock.

Thanks

> sln
>
Leon Romanovsky Jan. 26, 2020, 9:24 p.m. UTC | #12
On Sun, Jan 26, 2020 at 01:17:52PM -0800, Shannon Nelson wrote:
> On 1/26/20 1:08 PM, Leon Romanovsky wrote:
> > The long-standing policy in kernel that we don't really care about
> > out-of-tree code.
>
> That doesn't mean we need to be aggressively against out-of-tree code.  One
> of the positive points about Linux and loadable modules has always been the
> flexibility that allows and encourages innovation, and helps enable more
> work and testing before a driver can become a fully-fledged part of the
> kernel.  This move actively discourages part of that flexibility and I think
> it is breaking part of the usefulness of modules.

You are mixing definitions, nothing stops those people to innovate and
develop their code inside kernel and as standalone modules too.

It just stops them to put useless driver version string inside ethtool.
If they feel that their life can't be without something from 90s, they
have venerable MODULE_VERSION() macro to print anything they want.

Thanks

>
> sln
>
Jakub Kicinski Jan. 26, 2020, 9:33 p.m. UTC | #13
On Sun, 26 Jan 2020 23:08:50 +0200, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 12:49:57PM -0800, Jakub Kicinski wrote:
> > On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:  
> > > > This will end up affecting out-of-tree drivers as well, where it is useful
> > > > to know what the version number is, most especially since it is different
> > > > from what the kernel provided driver is.  How else are we to get this
> > > > information out to the user?  If this feature gets squashed, we'll end up
> > > > having to abuse some other mechanism so we can get the live information from
> > > > the driver, and probably each vendor will find a different way to sneak it
> > > > out, giving us more chaos than where we started.  At least the ethtool
> > > > version field is a known and consistent place for the version info.

> > Shannon does have a point that out of tree drivers still make use of
> > this field. Perhaps it would be a more suitable first step to print the
> > kernel version as default and add a comment saying upstream modules
> > shouldn't overwrite it (perhaps one day CI can catch new violators).  
> 
> Shannon proposed to remove this field and it was me who said no :)

Obviously, we can't remove fields from UAPI structs.

> My plan is to overwrite ->version, delete all users and add
> WARN_ONEC(strcpy(..->version_)...) inside net/ethtool/ to catch
> abusers.

What I was thinking just now was: initialize ->version to utsname
before drivers are called, delete all upstream users, add a coccicheck
for upstream drivers which try to report the version.

> > The NFP reports the git hash of the driver source plus the string
> > "(oot)" for out-of-tree:
> >
> > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
> > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315  
> 
> I was inspired by upstream code.
> https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c#L184

Right, upstream nfp reports kernel version (both in modinfo and ethtool)
GitHub/compat/backport/out-of-tree reports kernel version in which the
code was expected to appear in modinfo:

https://github.com/Netronome/nfp-drv-kmods/commit/7ec15c47caf5dbdf1f9806410535ad5b7373ec34#diff-492d7fa4004d885a38cfa889ed1adbe7L1284

And git hash of the driver source plus out of tree marker in ethtool.

That means it's out-of-tree driver which has to carry the extra code
and require extra feeding. As backport should IMHO.

> > > Leaving to deal with driver version to vendors is not an option too,
> > > because they prove for more than once that they are not capable to
> > > define user visible interfaces. It comes due to their natural believe
> > > that their company is alone in the world and user visible interface
> > > should be suitable for them only.
> > >
> > > It is already impossible for users to distinguish properly versions
> > > of different vendors, because they use arbitrary strings with some
> > > numbers.  
> >
> > That is true. But reporting the kernel version without even as much as
> > in-tree/out-of-tree indication makes the field entirely meaningless.  
> 
> The long-standing policy in kernel that we don't really care about
> out-of-tree code.

Yeah... we all know it's not that simple :)

The in-tree driver versions are meaningless and cause annoying churn
when people arbitrarily bump them. If we can get people to stop doing
that we'll be happy, that's all there is to it. 

Out of tree the field is useful, so we don't have to take it away just
as a matter of principle. If we can't convince people to stop bringing
the versions into the tree that'll be another story...
Shannon Nelson Jan. 26, 2020, 10:12 p.m. UTC | #14
On 1/26/20 1:24 PM, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 01:17:52PM -0800, Shannon Nelson wrote:
>> On 1/26/20 1:08 PM, Leon Romanovsky wrote:
>>> The long-standing policy in kernel that we don't really care about
>>> out-of-tree code.
>> That doesn't mean we need to be aggressively against out-of-tree code.  One
>> of the positive points about Linux and loadable modules has always been the
>> flexibility that allows and encourages innovation, and helps enable more
>> work and testing before a driver can become a fully-fledged part of the
>> kernel.  This move actively discourages part of that flexibility and I think
>> it is breaking part of the usefulness of modules.
> You are mixing definitions, nothing stops those people to innovate and
> develop their code inside kernel and as standalone modules too.
>
> It just stops them to put useless driver version string inside ethtool.
> If they feel that their life can't be without something from 90s, they
> have venerable MODULE_VERSION() macro to print anything they want.
>
Part of the pain of supporting our users is getting them to give us 
useful information about their problem.  The more commands I need them 
to run to get information about the environment, the less likely I will 
get anything useful.  We've been training our users over the years to 
use "ethtool -i" to get a good chunk of that info, with the knowledge 
that the driver version is only a hint, based upon the distro involved.  
I don't want to lose that hint.  If anything, I'd prefer that we added a 
field for UTS_RELEASE in the ethtool output, but I know that's too much 
to ask.

If the driver can put its "useless" version info into the 
MODULE_VERSION, why is it not acceptable for the ethtool driver version 
field?

... and as beauty is in the eye of the beholder, a judgement of 
"useless" is a personal thing.  Personally, I find it the driver version 
useful.

sln
Shannon Nelson Jan. 26, 2020, 10:21 p.m. UTC | #15
On 1/26/20 1:33 PM, Jakub Kicinski wrote
>> The long-standing policy in kernel that we don't really care about
>> out-of-tree code.
> Yeah... we all know it's not that simple :)
>
> The in-tree driver versions are meaningless and cause annoying churn
> when people arbitrarily bump them. If we can get people to stop doing
> that we'll be happy, that's all there is to it.
>
Perhaps it would be helpful if this standard was applied to all the 
drivers equally?  For example, I see that this week's ice driver update 
from Intel was accepted with no comment on their driver version bump.

Look, if we want to stamp all in-kernel drivers with the kernel version, 
fine.  But let's do it in a way that doesn't break the out-of-tree 
driver ability to report something else.  Can we set up a macro for 
in-kernel drivers to use in their get_drvinfo callback and require 
drivers to use that macro?  Then the out-of-tree drivers are able to 
replace that macro with whatever they need.  Just don't forcibly bash 
the value from higher up in the stack.

sln
Michal Kubecek Jan. 26, 2020, 10:22 p.m. UTC | #16
On Sun, Jan 26, 2020 at 02:12:38PM -0800, Shannon Nelson wrote:
> On 1/26/20 1:24 PM, Leon Romanovsky wrote:
> > On Sun, Jan 26, 2020 at 01:17:52PM -0800, Shannon Nelson wrote:
> > > On 1/26/20 1:08 PM, Leon Romanovsky wrote:
> > > > The long-standing policy in kernel that we don't really care about
> > > > out-of-tree code.
> > > That doesn't mean we need to be aggressively against out-of-tree code.  One
> > > of the positive points about Linux and loadable modules has always been the
> > > flexibility that allows and encourages innovation, and helps enable more
> > > work and testing before a driver can become a fully-fledged part of the
> > > kernel.  This move actively discourages part of that flexibility and I think
> > > it is breaking part of the usefulness of modules.
> > You are mixing definitions, nothing stops those people to innovate and
> > develop their code inside kernel and as standalone modules too.
> > 
> > It just stops them to put useless driver version string inside ethtool.
> > If they feel that their life can't be without something from 90s, they
> > have venerable MODULE_VERSION() macro to print anything they want.
> > 
> Part of the pain of supporting our users is getting them to give us useful
> information about their problem.  The more commands I need them to run to
> get information about the environment, the less likely I will get anything
> useful.  We've been training our users over the years to use "ethtool -i" to
> get a good chunk of that info, with the knowledge that the driver version is
> only a hint, based upon the distro involved.  I don't want to lose that
> hint.  If anything, I'd prefer that we added a field for UTS_RELEASE in the
> ethtool output, but I know that's too much to ask.

At the same time, I've been trying to explain both our L1/L2 support
guys and our customers that "driver version" information reported by
"ethtool -i" is almost useless and that if they really want to identify
driver version, they should rather use srcversion as reported by modinfo
or sysfs.

Michal
Shannon Nelson Jan. 26, 2020, 10:57 p.m. UTC | #17
On 1/26/20 2:22 PM, Michal Kubecek wrote:
> On Sun, Jan 26, 2020 at 02:12:38PM -0800, Shannon Nelson wrote:
>> On 1/26/20 1:24 PM, Leon Romanovsky wrote:
>>> On Sun, Jan 26, 2020 at 01:17:52PM -0800, Shannon Nelson wrote:
>>>> On 1/26/20 1:08 PM, Leon Romanovsky wrote:
>>>>> The long-standing policy in kernel that we don't really care about
>>>>> out-of-tree code.
>>>> That doesn't mean we need to be aggressively against out-of-tree code.  One
>>>> of the positive points about Linux and loadable modules has always been the
>>>> flexibility that allows and encourages innovation, and helps enable more
>>>> work and testing before a driver can become a fully-fledged part of the
>>>> kernel.  This move actively discourages part of that flexibility and I think
>>>> it is breaking part of the usefulness of modules.
>>> You are mixing definitions, nothing stops those people to innovate and
>>> develop their code inside kernel and as standalone modules too.
>>>
>>> It just stops them to put useless driver version string inside ethtool.
>>> If they feel that their life can't be without something from 90s, they
>>> have venerable MODULE_VERSION() macro to print anything they want.
>>>
>> Part of the pain of supporting our users is getting them to give us useful
>> information about their problem.  The more commands I need them to run to
>> get information about the environment, the less likely I will get anything
>> useful.  We've been training our users over the years to use "ethtool -i" to
>> get a good chunk of that info, with the knowledge that the driver version is
>> only a hint, based upon the distro involved.  I don't want to lose that
>> hint.  If anything, I'd prefer that we added a field for UTS_RELEASE in the
>> ethtool output, but I know that's too much to ask.
> At the same time, I've been trying to explain both our L1/L2 support
> guys and our customers that "driver version" information reported by
> "ethtool -i" is almost useless and that if they really want to identify
> driver version, they should rather use srcversion as reported by modinfo
> or sysfs.
>
> Michal

So as I suggested elsewhere, can we compromise by not bashing the driver 
string in the caller stack, but require the in-kernel drivers to use a 
particular macro that will put the kernel/git version into the string?  
This allows out-of-tree drivers the option of overriding the version 
with some other string that can be meaningful in any other given old or 
new distro kernel.  This should be easy to enforce mechanically with 
checkpatch, and easy enough to do a sweeping coccinelle change on the 
existing drivers.

sln
Leon Romanovsky Jan. 27, 2020, 5:18 a.m. UTC | #18
On Sun, Jan 26, 2020 at 11:22:53PM +0100, Michal Kubecek wrote:
> On Sun, Jan 26, 2020 at 02:12:38PM -0800, Shannon Nelson wrote:
> > On 1/26/20 1:24 PM, Leon Romanovsky wrote:
> > > On Sun, Jan 26, 2020 at 01:17:52PM -0800, Shannon Nelson wrote:
> > > > On 1/26/20 1:08 PM, Leon Romanovsky wrote:
> > > > > The long-standing policy in kernel that we don't really care about
> > > > > out-of-tree code.
> > > > That doesn't mean we need to be aggressively against out-of-tree code.  One
> > > > of the positive points about Linux and loadable modules has always been the
> > > > flexibility that allows and encourages innovation, and helps enable more
> > > > work and testing before a driver can become a fully-fledged part of the
> > > > kernel.  This move actively discourages part of that flexibility and I think
> > > > it is breaking part of the usefulness of modules.
> > > You are mixing definitions, nothing stops those people to innovate and
> > > develop their code inside kernel and as standalone modules too.
> > >
> > > It just stops them to put useless driver version string inside ethtool.
> > > If they feel that their life can't be without something from 90s, they
> > > have venerable MODULE_VERSION() macro to print anything they want.
> > >
> > Part of the pain of supporting our users is getting them to give us useful
> > information about their problem.  The more commands I need them to run to
> > get information about the environment, the less likely I will get anything
> > useful.  We've been training our users over the years to use "ethtool -i" to
> > get a good chunk of that info, with the knowledge that the driver version is
> > only a hint, based upon the distro involved.  I don't want to lose that
> > hint.  If anything, I'd prefer that we added a field for UTS_RELEASE in the
> > ethtool output, but I know that's too much to ask.
>
> At the same time, I've been trying to explain both our L1/L2 support
> guys and our customers that "driver version" information reported by
> "ethtool -i" is almost useless and that if they really want to identify
> driver version, they should rather use srcversion as reported by modinfo
> or sysfs.

We went even farther and wrote simple bash script that collects all
needed information from the system.

Thanks

>
> Michal
Leon Romanovsky Jan. 27, 2020, 5:34 a.m. UTC | #19
On Sun, Jan 26, 2020 at 02:21:58PM -0800, Shannon Nelson wrote:
> On 1/26/20 1:33 PM, Jakub Kicinski wrote
> > > The long-standing policy in kernel that we don't really care about
> > > out-of-tree code.
> > Yeah... we all know it's not that simple :)
> >
> > The in-tree driver versions are meaningless and cause annoying churn
> > when people arbitrarily bump them. If we can get people to stop doing
> > that we'll be happy, that's all there is to it.
> >
> Perhaps it would be helpful if this standard was applied to all the drivers
> equally?  For example, I see that this week's ice driver update from Intel
> was accepted with no comment on their driver version bump.

Thanks, it is another great example of why trusting driver authors,
even experienced, on specific topics is not an option.

>
> Look, if we want to stamp all in-kernel drivers with the kernel version,
> fine.  But let's do it in a way that doesn't break the out-of-tree driver
> ability to report something else.  Can we set up a macro for in-kernel
> drivers to use in their get_drvinfo callback and require drivers to use that
> macro?  Then the out-of-tree drivers are able to replace that macro with
> whatever they need.  Just don't forcibly bash the value from higher up in
> the stack.

The thing is that we don't consider in-kernel API as stable one, so
addition of new field which is not in use in upstream looks sketchy to
me, but I have an idea how to solve it.

Thanks

>
> sln
>
Leon Romanovsky Jan. 27, 2020, 5:49 a.m. UTC | #20
On Sun, Jan 26, 2020 at 01:33:53PM -0800, Jakub Kicinski wrote:
> On Sun, 26 Jan 2020 23:08:50 +0200, Leon Romanovsky wrote:
> > On Sun, Jan 26, 2020 at 12:49:57PM -0800, Jakub Kicinski wrote:
> > > On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:
> > > > > This will end up affecting out-of-tree drivers as well, where it is useful
> > > > > to know what the version number is, most especially since it is different
> > > > > from what the kernel provided driver is.  How else are we to get this
> > > > > information out to the user?  If this feature gets squashed, we'll end up
> > > > > having to abuse some other mechanism so we can get the live information from
> > > > > the driver, and probably each vendor will find a different way to sneak it
> > > > > out, giving us more chaos than where we started.  At least the ethtool
> > > > > version field is a known and consistent place for the version info.
>
> > > Shannon does have a point that out of tree drivers still make use of
> > > this field. Perhaps it would be a more suitable first step to print the
> > > kernel version as default and add a comment saying upstream modules
> > > shouldn't overwrite it (perhaps one day CI can catch new violators).
> >
> > Shannon proposed to remove this field and it was me who said no :)
>
> Obviously, we can't remove fields from UAPI structs.
>
> > My plan is to overwrite ->version, delete all users and add
> > WARN_ONEC(strcpy(..->version_)...) inside net/ethtool/ to catch
> > abusers.
>
> What I was thinking just now was: initialize ->version to utsname
> before drivers are called, delete all upstream users, add a coccicheck
> for upstream drivers which try to report the version.
>
> > > The NFP reports the git hash of the driver source plus the string
> > > "(oot)" for out-of-tree:
> > >
> > > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
> > > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315
> >
> > I was inspired by upstream code.
> > https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c#L184
>
> Right, upstream nfp reports kernel version (both in modinfo and ethtool)
> GitHub/compat/backport/out-of-tree reports kernel version in which the
> code was expected to appear in modinfo:
>
> https://github.com/Netronome/nfp-drv-kmods/commit/7ec15c47caf5dbdf1f9806410535ad5b7373ec34#diff-492d7fa4004d885a38cfa889ed1adbe7L1284
>
> And git hash of the driver source plus out of tree marker in ethtool.
>
> That means it's out-of-tree driver which has to carry the extra code
> and require extra feeding. As backport should IMHO.
>
> > > > Leaving to deal with driver version to vendors is not an option too,
> > > > because they prove for more than once that they are not capable to
> > > > define user visible interfaces. It comes due to their natural believe
> > > > that their company is alone in the world and user visible interface
> > > > should be suitable for them only.
> > > >
> > > > It is already impossible for users to distinguish properly versions
> > > > of different vendors, because they use arbitrary strings with some
> > > > numbers.
> > >
> > > That is true. But reporting the kernel version without even as much as
> > > in-tree/out-of-tree indication makes the field entirely meaningless.
> >
> > The long-standing policy in kernel that we don't really care about
> > out-of-tree code.
>
> Yeah... we all know it's not that simple :)

It is simple, unfortunately netdev people like to complicate things
by declaring ABI in very vague way which sometimes goes so far that
it ends more strict than anyone would imagine.

We, RDMA and many other subsystems mentioned in that ksummit thread,
removed MODULE_VERSION() a long time ago and got zero complains from
the real users.

>
> The in-tree driver versions are meaningless and cause annoying churn
> when people arbitrarily bump them. If we can get people to stop doing
> that we'll be happy, that's all there is to it.
>
> Out of tree the field is useful, so we don't have to take it away just
> as a matter of principle. If we can't convince people to stop bringing
> the versions into the tree that'll be another story...

As Shannon pointed, even experienced people will try to sneak those
changes. I assume that it is mainly because they are pushed to do it
by the people who doesn't understand Linux kernel process.

Thanks
Michal Kubecek Jan. 27, 2020, 6:08 a.m. UTC | #21
On Sun, Jan 26, 2020 at 02:57:21PM -0800, Shannon Nelson wrote:
> On 1/26/20 2:22 PM, Michal Kubecek wrote:
> > On Sun, Jan 26, 2020 at 02:12:38PM -0800, Shannon Nelson wrote:
> > > Part of the pain of supporting our users is getting them to give us useful
> > > information about their problem.  The more commands I need them to run to
> > > get information about the environment, the less likely I will get anything
> > > useful.  We've been training our users over the years to use "ethtool -i" to
> > > get a good chunk of that info, with the knowledge that the driver version is
> > > only a hint, based upon the distro involved.  I don't want to lose that
> > > hint.  If anything, I'd prefer that we added a field for UTS_RELEASE in the
> > > ethtool output, but I know that's too much to ask.
> > 
> > At the same time, I've been trying to explain both our L1/L2 support
> > guys and our customers that "driver version" information reported by
> > "ethtool -i" is almost useless and that if they really want to identify
> > driver version, they should rather use srcversion as reported by modinfo
> > or sysfs.
> 
> So as I suggested elsewhere, can we compromise by not bashing the driver
> string in the caller stack, but require the in-kernel drivers to use a
> particular macro that will put the kernel/git version into the string?  This
> allows out-of-tree drivers the option of overriding the version with some
> other string that can be meaningful in any other given old or new distro
> kernel.  This should be easy to enforce mechanically with checkpatch, and
> easy enough to do a sweeping coccinelle change on the existing drivers.

Personally, I rather liked what Jakub suggested earlier: set
ethtool_drvinfo::version to kernel version before ops->get_drvinfo() is called
in ethtool_get_drvinfo() (and its netlink counterpart once we get some
consensus about what information should be in the message), clean up in-tree
drivers so that they don't touch it and add a coccinelle check so that we keep
in-tree drivers compliant; this would allow out-of-tree drivers to overwrite
ethtool_drvinfo::version with whatever they want.

Michal
Leon Romanovsky Jan. 27, 2020, 6:42 a.m. UTC | #22
On Mon, Jan 27, 2020 at 07:08:35AM +0100, Michal Kubecek wrote:
> On Sun, Jan 26, 2020 at 02:57:21PM -0800, Shannon Nelson wrote:
> > On 1/26/20 2:22 PM, Michal Kubecek wrote:
> > > On Sun, Jan 26, 2020 at 02:12:38PM -0800, Shannon Nelson wrote:
> > > > Part of the pain of supporting our users is getting them to give us useful
> > > > information about their problem.  The more commands I need them to run to
> > > > get information about the environment, the less likely I will get anything
> > > > useful.  We've been training our users over the years to use "ethtool -i" to
> > > > get a good chunk of that info, with the knowledge that the driver version is
> > > > only a hint, based upon the distro involved.  I don't want to lose that
> > > > hint.  If anything, I'd prefer that we added a field for UTS_RELEASE in the
> > > > ethtool output, but I know that's too much to ask.
> > >
> > > At the same time, I've been trying to explain both our L1/L2 support
> > > guys and our customers that "driver version" information reported by
> > > "ethtool -i" is almost useless and that if they really want to identify
> > > driver version, they should rather use srcversion as reported by modinfo
> > > or sysfs.
> >
> > So as I suggested elsewhere, can we compromise by not bashing the driver
> > string in the caller stack, but require the in-kernel drivers to use a
> > particular macro that will put the kernel/git version into the string?  This
> > allows out-of-tree drivers the option of overriding the version with some
> > other string that can be meaningful in any other given old or new distro
> > kernel.  This should be easy to enforce mechanically with checkpatch, and
> > easy enough to do a sweeping coccinelle change on the existing drivers.
>
> Personally, I rather liked what Jakub suggested earlier: set
> ethtool_drvinfo::version to kernel version before ops->get_drvinfo() is called
> in ethtool_get_drvinfo() (and its netlink counterpart once we get some
> consensus about what information should be in the message), clean up in-tree
> drivers so that they don't touch it and add a coccinelle check so that we keep
> in-tree drivers compliant; this would allow out-of-tree drivers to overwrite
> ethtool_drvinfo::version with whatever they want.

It works for MODULE_VERSION(), so I don't see any reason to have different
solution for the same value for ethtool.

For example, ib_core module doesn't have MODULE_VERSION() string.
[leonro@server ~]$ modinfo ib_core
filename:       /lib/modules/5.5.0-rc6/modules/ib_core.ko
<...>
intree:         Y
name:           ib_core
vermagic:       5.5.0-rc6 SMP mod_unload modversions
<...>

Thanks

>
> Michal
Leon Romanovsky Jan. 27, 2020, 6:45 a.m. UTC | #23
On Mon, Jan 27, 2020 at 07:34:33AM +0200, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 02:21:58PM -0800, Shannon Nelson wrote:
> > On 1/26/20 1:33 PM, Jakub Kicinski wrote
> > > > The long-standing policy in kernel that we don't really care about
> > > > out-of-tree code.
> > > Yeah... we all know it's not that simple :)
> > >
> > > The in-tree driver versions are meaningless and cause annoying churn
> > > when people arbitrarily bump them. If we can get people to stop doing
> > > that we'll be happy, that's all there is to it.
> > >
> > Perhaps it would be helpful if this standard was applied to all the drivers
> > equally?  For example, I see that this week's ice driver update from Intel
> > was accepted with no comment on their driver version bump.
>
> Thanks, it is another great example of why trusting driver authors,
> even experienced, on specific topics is not an option.
>
> >
> > Look, if we want to stamp all in-kernel drivers with the kernel version,
> > fine.  But let's do it in a way that doesn't break the out-of-tree driver
> > ability to report something else.  Can we set up a macro for in-kernel
> > drivers to use in their get_drvinfo callback and require drivers to use that
> > macro?  Then the out-of-tree drivers are able to replace that macro with
> > whatever they need.  Just don't forcibly bash the value from higher up in
> > the stack.
>
> The thing is that we don't consider in-kernel API as stable one, so
> addition of new field which is not in use in upstream looks sketchy to
> me, but I have an idea how to solve it.

Actually, it looks like my idea is Jakub's and Michal's idea. I will use
this opportunity and remove MODULE_VERSION() too.

Thanks

>
> Thanks
>
> >
> > sln
> >
David Miller Jan. 27, 2020, 12:21 p.m. UTC | #24
From: Leon Romanovsky <leon@kernel.org>
Date: Mon, 27 Jan 2020 07:49:55 +0200

> We, RDMA and many other subsystems mentioned in that ksummit thread,
> removed MODULE_VERSION() a long time ago and got zero complains from
> the real users.

Changes to RDMA have a disproportionate level of impact compared to
all of netdev.

So comparing the level of real or perceived potential impact is quite
intellectually dishonest.
Leon Romanovsky Jan. 27, 2020, 12:42 p.m. UTC | #25
On Mon, Jan 27, 2020 at 01:21:14PM +0100, David Miller wrote:
> From: Leon Romanovsky <leon@kernel.org>
> Date: Mon, 27 Jan 2020 07:49:55 +0200
>
> > We, RDMA and many other subsystems mentioned in that ksummit thread,
> > removed MODULE_VERSION() a long time ago and got zero complains from
> > the real users.
>
> Changes to RDMA have a disproportionate level of impact compared to
> all of netdev.
>
> So comparing the level of real or perceived potential impact is quite
> intellectually dishonest.

This whole discussion was more emotional than intellectual :).

Anyway, I sent v4 https://lore.kernel.org/linux-rdma/20200127072028.19123-1-leon@kernel.org
and that variant provides default version value without harming
out-of-tree modules.

I have a plan to start and remove ethtool version and MODULE_VERSION()
calls from the drivers/net/* modules after merge window completes.

Does this plan sound right to you?

Thanks
David Miller Jan. 27, 2020, 12:47 p.m. UTC | #26
From: Leon Romanovsky <leon@kernel.org>
Date: Mon, 27 Jan 2020 14:42:05 +0200

> Anyway, I sent v4 https://lore.kernel.org/linux-rdma/20200127072028.19123-1-leon@kernel.org
> and that variant provides default version value without harming
> out-of-tree modules.
> 
> I have a plan to start and remove ethtool version and MODULE_VERSION()
> calls from the drivers/net/* modules after merge window completes.
> 
> Does this plan sound right to you?

Yes it does, I was just reviewing the patch now.
Jakub Kicinski Jan. 27, 2020, 2:21 p.m. UTC | #27
On Mon, 27 Jan 2020 08:45:34 +0200, Leon Romanovsky wrote:
> > The thing is that we don't consider in-kernel API as stable one, so
> > addition of new field which is not in use in upstream looks sketchy to
> > me, but I have an idea how to solve it.  
> 
> Actually, it looks like my idea is Jakub's and Michal's idea. I will use
> this opportunity and remove MODULE_VERSION() too.

If you do please make sure DKMS works. I remember it was looking at
that value.
Leon Romanovsky Jan. 27, 2020, 3:39 p.m. UTC | #28
On Mon, Jan 27, 2020 at 06:21:08AM -0800, Jakub Kicinski wrote:
> On Mon, 27 Jan 2020 08:45:34 +0200, Leon Romanovsky wrote:
> > > The thing is that we don't consider in-kernel API as stable one, so
> > > addition of new field which is not in use in upstream looks sketchy to
> > > me, but I have an idea how to solve it.
> >
> > Actually, it looks like my idea is Jakub's and Michal's idea. I will use
> > this opportunity and remove MODULE_VERSION() too.
>
> If you do please make sure DKMS works. I remember it was looking at
> that value.

I'll check, thanks.

However from reading the source, it will be unlikely to see any
issues there.

DKMS uses modversion for two things. First is to manage internal tree
where those modules will be built and it knows how to install and clean
automatically modules. Second is to list modaliases, and it handles them
perfectly with and without version [1].

In our case, modules version will be available, it just going to change
from 1.0.0. to something like 5.5.0-rc6.

[1] https://github.com/dell/dkms/blob/master/dkms_find-provides#L48

Thanks
Shannon Nelson Jan. 27, 2020, 5:57 p.m. UTC | #29
On 1/26/20 9:49 PM, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 01:33:53PM -0800, Jakub Kicinski wrote:
>> On Sun, 26 Jan 2020 23:08:50 +0200, Leon Romanovsky wrote:
>>> On Sun, Jan 26, 2020 at 12:49:57PM -0800, Jakub Kicinski wrote:
>>>> On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:
>>>>>> This will end up affecting out-of-tree drivers as well, where it is useful
>>>>>> to know what the version number is, most especially since it is different
>>>>>> from what the kernel provided driver is.  How else are we to get this
>>>>>> information out to the user?  If this feature gets squashed, we'll end up
>>>>>> having to abuse some other mechanism so we can get the live information from
>>>>>> the driver, and probably each vendor will find a different way to sneak it
>>>>>> out, giving us more chaos than where we started.  At least the ethtool
>>>>>> version field is a known and consistent place for the version info.
>>>> Shannon does have a point that out of tree drivers still make use of
>>>> this field. Perhaps it would be a more suitable first step to print the
>>>> kernel version as default and add a comment saying upstream modules
>>>> shouldn't overwrite it (perhaps one day CI can catch new violators).
>>> Shannon proposed to remove this field and it was me who said no :)
>> Obviously, we can't remove fields from UAPI structs.
>>
>>> My plan is to overwrite ->version, delete all users and add
>>> WARN_ONEC(strcpy(..->version_)...) inside net/ethtool/ to catch
>>> abusers.
>> What I was thinking just now was: initialize ->version to utsname
>> before drivers are called, delete all upstream users, add a coccicheck
>> for upstream drivers which try to report the version.
>>
>>>> The NFP reports the git hash of the driver source plus the string
>>>> "(oot)" for out-of-tree:
>>>>
>>>> https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
>>>> https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315
>>> I was inspired by upstream code.
>>> https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c#L184
>> Right, upstream nfp reports kernel version (both in modinfo and ethtool)
>> GitHub/compat/backport/out-of-tree reports kernel version in which the
>> code was expected to appear in modinfo:
>>
>> https://github.com/Netronome/nfp-drv-kmods/commit/7ec15c47caf5dbdf1f9806410535ad5b7373ec34#diff-492d7fa4004d885a38cfa889ed1adbe7L1284
>>
>> And git hash of the driver source plus out of tree marker in ethtool.
>>
>> That means it's out-of-tree driver which has to carry the extra code
>> and require extra feeding. As backport should IMHO.
>>
>>>>> Leaving to deal with driver version to vendors is not an option too,
>>>>> because they prove for more than once that they are not capable to
>>>>> define user visible interfaces. It comes due to their natural believe
>>>>> that their company is alone in the world and user visible interface
>>>>> should be suitable for them only.
>>>>>
>>>>> It is already impossible for users to distinguish properly versions
>>>>> of different vendors, because they use arbitrary strings with some
>>>>> numbers.
>>>> That is true. But reporting the kernel version without even as much as
>>>> in-tree/out-of-tree indication makes the field entirely meaningless.
>>> The long-standing policy in kernel that we don't really care about
>>> out-of-tree code.
>> Yeah... we all know it's not that simple :)
> It is simple, unfortunately netdev people like to complicate things
> by declaring ABI in very vague way which sometimes goes so far that
> it ends more strict than anyone would imagine.
>
> We, RDMA and many other subsystems mentioned in that ksummit thread,
> removed MODULE_VERSION() a long time ago and got zero complains from
> the real users.
>
>> The in-tree driver versions are meaningless and cause annoying churn
>> when people arbitrarily bump them. If we can get people to stop doing
>> that we'll be happy, that's all there is to it.
>>
>> Out of tree the field is useful, so we don't have to take it away just
>> as a matter of principle. If we can't convince people to stop bringing
>> the versions into the tree that'll be another story...
> As Shannon pointed, even experienced people will try to sneak those
> changes. I assume that it is mainly because they are pushed to do it
> by the people who doesn't understand Linux kernel process.
>
I don't think that the Intel Networking folks were trying to "sneak" 
something through, I think they have simply been continuing the process 
that they've been following for years.  When we have groups such as 
them, with a long history of contributions to drivers and stack, not 
following the new rules, perhaps we need to take a look at how we're 
publicizing these changes.

sln

Patch
diff mbox series

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index cd9bc67381b2..3c6fb13a78bf 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -17,6 +17,7 @@ 
 #include <linux/phy.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/vermagic.h>
 #include <linux/vmalloc.h>
 #include <linux/sfp.h>
 #include <linux/slab.h>
@@ -776,6 +777,8 @@  static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 		return -EOPNOTSUPP;
 	}

+	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
+
 	/*
 	 * this method of obtaining string set info is deprecated;
 	 * Use ETHTOOL_GSSET_INFO instead.