| Message ID | 20200123130541.30473-1-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next] net/core: Replace driver version to be kernel version | expand |
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 :)
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
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.
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 :)
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
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 >
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.
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
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
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
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 >
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 >
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...
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
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
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
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
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
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 >
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
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
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
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 > >
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.
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
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.
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.
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
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
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.