[PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Vincent Mailhol]

If ethtool_ops::get_drvinfo() callback isn't set,
ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
ethtool_drvinfo::bus_info fields.

However, if the driver provides the callback function, those two
fields are not touched. This means that the driver has to fill these
itself.

Allow the driver to leave those two fields empty and populate them in
such case. This way, the driver can rely on the default values for the
name and the bus_info. If the driver provides values, do nothing.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 net/ethtool/ioctl.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 57e7238a4136..546f931c3b6c 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -713,15 +713,22 @@ static int
 ethtool_get_drvinfo(struct net_device *dev, struct ethtool_devlink_compat *rsp)
 {
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct device *parent = dev->dev.parent;
 
 	rsp->info.cmd = ETHTOOL_GDRVINFO;
 	strscpy(rsp->info.version, UTS_RELEASE, sizeof(rsp->info.version));
 	if (ops->get_drvinfo) {
 		ops->get_drvinfo(dev, &rsp->info);
-	} else if (dev->dev.parent && dev->dev.parent->driver) {
-		strscpy(rsp->info.bus_info, dev_name(dev->dev.parent),
+		if (!rsp->info.bus_info[0] && parent)
+			strscpy(rsp->info.bus_info, dev_name(parent),
+				sizeof(rsp->info.bus_info));
+		if (!rsp->info.driver[0] && parent && parent->driver)
+			strscpy(rsp->info.driver, parent->driver->name,
+				sizeof(rsp->info.driver));
+	} else if (parent && parent->driver) {
+		strscpy(rsp->info.bus_info, dev_name(parent),
 			sizeof(rsp->info.bus_info));
-		strscpy(rsp->info.driver, dev->dev.parent->driver->name,
+		strscpy(rsp->info.driver, parent->driver->name,
 			sizeof(rsp->info.driver));
 	} else if (dev->rtnl_link_ops) {
 		strscpy(rsp->info.driver, dev->rtnl_link_ops->kind,
-- 
2.37.4

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> If ethtool_ops::get_drvinfo() callback isn't set,
> ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> ethtool_drvinfo::bus_info fields.
> 
> However, if the driver provides the callback function, those two
> fields are not touched. This means that the driver has to fill these
> itself.

Can you please point to such drivers? One can argue that they don't need
to touch these fields in a first place and ethtool_drvinfo should always
overwrite them.

Thanks

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Jakub Kicinski]

On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > If ethtool_ops::get_drvinfo() callback isn't set,
> > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > ethtool_drvinfo::bus_info fields.
> > 
> > However, if the driver provides the callback function, those two
> > fields are not touched. This means that the driver has to fill these
> > itself.  
> 
> Can you please point to such drivers?

What you mean by "such drivers" is not clear from the quoted context,
at least to me.

> One can argue that they don't need to touch these fields in a first
> place and ethtool_drvinfo should always overwrite them.

Quite likely most driver prints to .driver and .bus_info can be dropped
with this patch in place. Then again, I'm suspecting it's a bit of a
chicken and an egg problem with people adding new drivers not having 
an incentive to add the print in the core and people who want to add
the print in the core not having any driver that would benefit.
Therefore I'd lean towards accepting Vincent's patch as is even if
the submission can likely be more thorough and strict.

While I'm typing - I've used dev_driver_string() to get the driver
name in the past. Perhaps something to consider?

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Vincent MAILHOL]

On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > ethtool_drvinfo::bus_info fields.
> > >
> > > However, if the driver provides the callback function, those two
> > > fields are not touched. This means that the driver has to fill these
> > > itself.
> >
> > Can you please point to such drivers?
>
> What you mean by "such drivers" is not clear from the quoted context,
> at least to me.

An example:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041

This driver wants to set fw_version but needs to also fill the driver
name and bus_info. My patch will enable *such drivers* to only fill
the fw_version and delegate the rest to the core.

> > One can argue that they don't need to touch these fields in a first
> > place and ethtool_drvinfo should always overwrite them.
>
> Quite likely most driver prints to .driver and .bus_info can be dropped
> with this patch in place. Then again, I'm suspecting it's a bit of a
> chicken and an egg problem with people adding new drivers not having
> an incentive to add the print in the core and people who want to add
> the print in the core not having any driver that would benefit.
> Therefore I'd lean towards accepting Vincent's patch as is even if
> the submission can likely be more thorough and strict.

If we can agree that no drivers should ever print .driver and
.bus_info, then I am fine to send a clean-up patch to remove all this
after this one gets accepted. However, I am not willing to invest time
for nothing. So would one of you be ready to sign-off such a  clean-up
patch?

> While I'm typing - I've used dev_driver_string() to get the driver
> name in the past. Perhaps something to consider?

I am not sure of that one. If dev->dev.parent->driver is not set, it
defaults to dev_bus_name() which is .bus_info, isn't it?
https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2181

 For the end user, it might be better to display an empty driver name
in 'ethtool -i' rather than reporting the bus_info twice?

I mean, if you ask me for my opinion, then my answer is "I am not
sure". If you have confidence that dev_driver_string() is better, then
I will send a v2 right away.


Yours sincerely,
Vincent Mailhol

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > ethtool_drvinfo::bus_info fields.
> > > >
> > > > However, if the driver provides the callback function, those two
> > > > fields are not touched. This means that the driver has to fill these
> > > > itself.
> > >
> > > Can you please point to such drivers?
> >
> > What you mean by "such drivers" is not clear from the quoted context,
> > at least to me.
> 
> An example:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> 
> This driver wants to set fw_version but needs to also fill the driver
> name and bus_info. My patch will enable *such drivers* to only fill
> the fw_version and delegate the rest to the core.

Sorry for being misleading, It looks like I typed only part of the sentence
which I had in my mind. I wanted to see if any driver exists which prints
drv_name and bus_info different from default.

> 
> > > One can argue that they don't need to touch these fields in a first
> > > place and ethtool_drvinfo should always overwrite them.
> >
> > Quite likely most driver prints to .driver and .bus_info can be dropped
> > with this patch in place. Then again, I'm suspecting it's a bit of a
> > chicken and an egg problem with people adding new drivers not having
> > an incentive to add the print in the core and people who want to add
> > the print in the core not having any driver that would benefit.
> > Therefore I'd lean towards accepting Vincent's patch as is even if
> > the submission can likely be more thorough and strict.
> 
> If we can agree that no drivers should ever print .driver and
> .bus_info, then I am fine to send a clean-up patch to remove all this
> after this one gets accepted. However, I am not willing to invest time
> for nothing. So would one of you be ready to sign-off such a  clean-up
> patch?

I will be happy to see such patch and will review it, but can't add sign-off
as I'm not netdev maintainer.

Thanks

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Vincent MAILHOL]

On Thu. 10 Nov. 2022 at 18:11, Leon Romanovsky <leon@kernel.org> wrote:
> On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> > On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > > ethtool_drvinfo::bus_info fields.
> > > > >
> > > > > However, if the driver provides the callback function, those two
> > > > > fields are not touched. This means that the driver has to fill these
> > > > > itself.
> > > >
> > > > Can you please point to such drivers?
> > >
> > > What you mean by "such drivers" is not clear from the quoted context,
> > > at least to me.
> >
> > An example:
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> >
> > This driver wants to set fw_version but needs to also fill the driver
> > name and bus_info. My patch will enable *such drivers* to only fill
> > the fw_version and delegate the rest to the core.
>
> Sorry for being misleading, It looks like I typed only part of the sentence
> which I had in my mind. I wanted to see if any driver exists which prints
> drv_name and bus_info different from default.
>
> >
> > > > One can argue that they don't need to touch these fields in a first
> > > > place and ethtool_drvinfo should always overwrite them.
> > >
> > > Quite likely most driver prints to .driver and .bus_info can be dropped
> > > with this patch in place. Then again, I'm suspecting it's a bit of a
> > > chicken and an egg problem with people adding new drivers not having
> > > an incentive to add the print in the core and people who want to add
> > > the print in the core not having any driver that would benefit.
> > > Therefore I'd lean towards accepting Vincent's patch as is even if
> > > the submission can likely be more thorough and strict.
> >
> > If we can agree that no drivers should ever print .driver and
> > .bus_info, then I am fine to send a clean-up patch to remove all this
> > after this one gets accepted. However, I am not willing to invest time
> > for nothing. So would one of you be ready to sign-off such a  clean-up
> > patch?
>
> I will be happy to see such patch and will review it, but can't add sign-off
> as I'm not netdev maintainer.

Well, if you want to review, just have a look at:
  $ git grep -W "get_drvinfo(struct"

Unless some driver names their callback function in some non standard
way, that should be pretty it. Regardless, this will give you a fair
representation of the current situation.

There are a few nuances. For example, some drivers use dev_name() for
the .bus_info, while some others use pci_name(). I am not sure if this
makes a difference... Also, there are many hardcoded names for .driver
and I do not have the courage to check all of them. I am OK to blindly
remove all that mess but no more.


Yours sincerely,
Vincent Mailhol

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Thu, Nov 10, 2022 at 08:43:25PM +0900, Vincent MAILHOL wrote:
> On Thu. 10 Nov. 2022 at 18:11, Leon Romanovsky <leon@kernel.org> wrote:
> > On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> > > On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> > > > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > > > ethtool_drvinfo::bus_info fields.
> > > > > >
> > > > > > However, if the driver provides the callback function, those two
> > > > > > fields are not touched. This means that the driver has to fill these
> > > > > > itself.
> > > > >
> > > > > Can you please point to such drivers?
> > > >
> > > > What you mean by "such drivers" is not clear from the quoted context,
> > > > at least to me.
> > >
> > > An example:
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> > >
> > > This driver wants to set fw_version but needs to also fill the driver
> > > name and bus_info. My patch will enable *such drivers* to only fill
> > > the fw_version and delegate the rest to the core.
> >
> > Sorry for being misleading, It looks like I typed only part of the sentence
> > which I had in my mind. I wanted to see if any driver exists which prints
> > drv_name and bus_info different from default.
> >
> > >
> > > > > One can argue that they don't need to touch these fields in a first
> > > > > place and ethtool_drvinfo should always overwrite them.
> > > >
> > > > Quite likely most driver prints to .driver and .bus_info can be dropped
> > > > with this patch in place. Then again, I'm suspecting it's a bit of a
> > > > chicken and an egg problem with people adding new drivers not having
> > > > an incentive to add the print in the core and people who want to add
> > > > the print in the core not having any driver that would benefit.
> > > > Therefore I'd lean towards accepting Vincent's patch as is even if
> > > > the submission can likely be more thorough and strict.
> > >
> > > If we can agree that no drivers should ever print .driver and
> > > .bus_info, then I am fine to send a clean-up patch to remove all this
> > > after this one gets accepted. However, I am not willing to invest time
> > > for nothing. So would one of you be ready to sign-off such a  clean-up
> > > patch?
> >
> > I will be happy to see such patch and will review it, but can't add sign-off
> > as I'm not netdev maintainer.
> 
> Well, if you want to review, just have a look at:
>   $ git grep -W "get_drvinfo(struct"

BTW, in some of the callbacks, if driver doesn't exists, they print "N/A",
while in your patch it will be empty string.

Thanks

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Andrew Lunn]

> I will be happy to see such patch and will review it, but can't add sign-off
> as I'm not netdev maintainer.

Anybody can review any patch, and give an Acked-by or Reviewed-by. It
is up to the Maintainer doing the actual merge to decided if it has
any actual weight.

    Andrew

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Vincent MAILHOL]

On Thu. 10 Nov. 2022 at 21:00, Leon Romanovsky <leon@kernel.org> wrote:
> On Thu, Nov 10, 2022 at 08:43:25PM +0900, Vincent MAILHOL wrote:
> > On Thu. 10 Nov. 2022 at 18:11, Leon Romanovsky <leon@kernel.org> wrote:
> > > On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> > > > On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > > > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > > > > ethtool_drvinfo::bus_info fields.
> > > > > > >
> > > > > > > However, if the driver provides the callback function, those two
> > > > > > > fields are not touched. This means that the driver has to fill these
> > > > > > > itself.
> > > > > >
> > > > > > Can you please point to such drivers?
> > > > >
> > > > > What you mean by "such drivers" is not clear from the quoted context,
> > > > > at least to me.
> > > >
> > > > An example:
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> > > >
> > > > This driver wants to set fw_version but needs to also fill the driver
> > > > name and bus_info. My patch will enable *such drivers* to only fill
> > > > the fw_version and delegate the rest to the core.
> > >
> > > Sorry for being misleading, It looks like I typed only part of the sentence
> > > which I had in my mind. I wanted to see if any driver exists which prints
> > > drv_name and bus_info different from default.
> > >
> > > >
> > > > > > One can argue that they don't need to touch these fields in a first
> > > > > > place and ethtool_drvinfo should always overwrite them.
> > > > >
> > > > > Quite likely most driver prints to .driver and .bus_info can be dropped
> > > > > with this patch in place. Then again, I'm suspecting it's a bit of a
> > > > > chicken and an egg problem with people adding new drivers not having
> > > > > an incentive to add the print in the core and people who want to add
> > > > > the print in the core not having any driver that would benefit.
> > > > > Therefore I'd lean towards accepting Vincent's patch as is even if
> > > > > the submission can likely be more thorough and strict.
> > > >
> > > > If we can agree that no drivers should ever print .driver and
> > > > .bus_info, then I am fine to send a clean-up patch to remove all this
> > > > after this one gets accepted. However, I am not willing to invest time
> > > > for nothing. So would one of you be ready to sign-off such a  clean-up
> > > > patch?
> > >
> > > I will be happy to see such patch and will review it, but can't add sign-off
> > > as I'm not netdev maintainer.
> >
> > Well, if you want to review, just have a look at:
> >   $ git grep -W "get_drvinfo(struct"
>
> BTW, in some of the callbacks, if driver doesn't exists, they print "N/A",
> while in your patch it will be empty string.

Indeed and this is inconsistent. For example, no one sets
.erom_version to "N/A". So you will have some of the fields set to
"N/A" and some others set to an empty string. The "N/A" thing was a
mistake to begin with. I will not change my patch.

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Jakub Kicinski]

On Thu, 10 Nov 2022 17:34:55 +0900 Vincent MAILHOL wrote:
> > While I'm typing - I've used dev_driver_string() to get the driver
> > name in the past. Perhaps something to consider?  
> 
> I am not sure of that one. If dev->dev.parent->driver is not set, it
> defaults to dev_bus_name() which is .bus_info, isn't it?
> https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2181

I don't think so?  We put dev_name() into bus_info, which is usually
the address of the device on the bus (e.g. (D)BDF for PCI, like
0000:00:14.3). The name of the bus is pci. dev_driver_string() will
also fall back to printing class name.

>  For the end user, it might be better to display an empty driver name
> in 'ethtool -i' rather than reporting the bus_info twice?
> 
> I mean, if you ask me for my opinion, then my answer is "I am not
> sure". If you have confidence that dev_driver_string() is better, then
> I will send a v2 right away.

Well, it doesn't matter. I asked because handful of popular drivers 
use dev_driver_string(). But.. these are drivers so parent->driver 
will be set and it ends up not making any difference.

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Jakub Kicinski]

On Thu, 10 Nov 2022 11:11:38 +0200 Leon Romanovsky wrote:
> I will be happy to see such patch and will review it, but can't add sign-off
> as I'm not netdev maintainer.

Did we finish the version removal work? :S

Personally I'd rather direct any effort towards writing a checkpatch /
cocci / python check that catches new cases than cleaning up the pile
of drivers we have. A lot of which are not actively used..

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Thu, Nov 10, 2022 at 09:01:27AM -0800, Jakub Kicinski wrote:
> On Thu, 10 Nov 2022 11:11:38 +0200 Leon Romanovsky wrote:
> > I will be happy to see such patch and will review it, but can't add sign-off
> > as I'm not netdev maintainer.
> 
> Did we finish the version removal work? :S

Good point :)

> 
> Personally I'd rather direct any effort towards writing a checkpatch /
> cocci / python check that catches new cases than cleaning up the pile
> of drivers we have. A lot of which are not actively used..

I thought about this too, but came to conclusion what first step is to
remove "version" from in-kernel API. It will reduce drastically desire
to add driver version to new drivers.

"There is no shame in doing the right thing, even if its too late.".

BTW, I hope that we all learned that allowing drivers to return free
string to users will always lead to disaster.

Thanks

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Fri, Nov 11, 2022 at 12:53:16AM +0900, Vincent MAILHOL wrote:
> On Thu. 10 Nov. 2022 at 21:00, Leon Romanovsky <leon@kernel.org> wrote:
> > On Thu, Nov 10, 2022 at 08:43:25PM +0900, Vincent MAILHOL wrote:
> > > On Thu. 10 Nov. 2022 at 18:11, Leon Romanovsky <leon@kernel.org> wrote:
> > > > On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> > > > > On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > > > > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > > > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > > > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > > > > > ethtool_drvinfo::bus_info fields.
> > > > > > > >
> > > > > > > > However, if the driver provides the callback function, those two
> > > > > > > > fields are not touched. This means that the driver has to fill these
> > > > > > > > itself.
> > > > > > >
> > > > > > > Can you please point to such drivers?
> > > > > >
> > > > > > What you mean by "such drivers" is not clear from the quoted context,
> > > > > > at least to me.
> > > > >
> > > > > An example:
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> > > > >
> > > > > This driver wants to set fw_version but needs to also fill the driver
> > > > > name and bus_info. My patch will enable *such drivers* to only fill
> > > > > the fw_version and delegate the rest to the core.
> > > >
> > > > Sorry for being misleading, It looks like I typed only part of the sentence
> > > > which I had in my mind. I wanted to see if any driver exists which prints
> > > > drv_name and bus_info different from default.
> > > >
> > > > >
> > > > > > > One can argue that they don't need to touch these fields in a first
> > > > > > > place and ethtool_drvinfo should always overwrite them.
> > > > > >
> > > > > > Quite likely most driver prints to .driver and .bus_info can be dropped
> > > > > > with this patch in place. Then again, I'm suspecting it's a bit of a
> > > > > > chicken and an egg problem with people adding new drivers not having
> > > > > > an incentive to add the print in the core and people who want to add
> > > > > > the print in the core not having any driver that would benefit.
> > > > > > Therefore I'd lean towards accepting Vincent's patch as is even if
> > > > > > the submission can likely be more thorough and strict.
> > > > >
> > > > > If we can agree that no drivers should ever print .driver and
> > > > > .bus_info, then I am fine to send a clean-up patch to remove all this
> > > > > after this one gets accepted. However, I am not willing to invest time
> > > > > for nothing. So would one of you be ready to sign-off such a  clean-up
> > > > > patch?
> > > >
> > > > I will be happy to see such patch and will review it, but can't add sign-off
> > > > as I'm not netdev maintainer.
> > >
> > > Well, if you want to review, just have a look at:
> > >   $ git grep -W "get_drvinfo(struct"
> >
> > BTW, in some of the callbacks, if driver doesn't exists, they print "N/A",
> > while in your patch it will be empty string.
> 
> Indeed and this is inconsistent. For example, no one sets
> .erom_version to "N/A". So you will have some of the fields set to
> "N/A" and some others set to an empty string. The "N/A" thing was a
> mistake to begin with. I will not change my patch.

I don't see anyone asking you to change your patch drastically. This
discussion is more about next steps.

Thanks

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  8 Nov 2022 12:57:54 +0900 you wrote:
> If ethtool_ops::get_drvinfo() callback isn't set,
> ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> ethtool_drvinfo::bus_info fields.
> 
> However, if the driver provides the callback function, those two
> fields are not touched. This means that the driver has to fill these
> itself.
> 
> [...]

Here is the summary with links:
  - [net-next,v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits
    https://git.kernel.org/netdev/net-next/c/edaf5df22cb8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Vincent MAILHOL]

On Fri. 11 Nov. 2022 à 02:01, Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 10 Nov 2022 11:11:38 +0200 Leon Romanovsky wrote:
> > I will be happy to see such patch and will review it, but can't add sign-off
> > as I'm not netdev maintainer.
>
> Did we finish the version removal work? :S
>
> Personally I'd rather direct any effort towards writing a checkpatch /
> cocci / python check that catches new cases than cleaning up the pile
> of drivers we have. A lot of which are not actively used..

Agree, but I will not work on that (because of other personal
priorities). If someone else wants to do it, go ahead :)

What I can do is update the documentation:
https://lore.kernel.org/netdev/20221111064054.371965-1-mailhol.vincent@wanadoo.fr/


Yours sincerely,
Vincent Mailhol

Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

[Leon Romanovsky]

On Fri, Nov 11, 2022 at 03:43:36PM +0900, Vincent MAILHOL wrote:
> On Fri. 11 Nov. 2022 à 02:01, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 10 Nov 2022 11:11:38 +0200 Leon Romanovsky wrote:
> > > I will be happy to see such patch and will review it, but can't add sign-off
> > > as I'm not netdev maintainer.
> >
> > Did we finish the version removal work? :S
> >
> > Personally I'd rather direct any effort towards writing a checkpatch /
> > cocci / python check that catches new cases than cleaning up the pile
> > of drivers we have. A lot of which are not actively used..
> 
> Agree, but I will not work on that (because of other personal
> priorities). If someone else wants to do it, go ahead :)

It's ok, Jakub referred to me. I sent a lot of patches like this.
https://lore.kernel.org/netdev/5d76f3116ee795071ec044eabb815d6c2bdc7dbd.1649922731.git.leonro@nvidia.com/

> 
> What I can do is update the documentation:
> https://lore.kernel.org/netdev/20221111064054.371965-1-mailhol.vincent@wanadoo.fr/
> 
> 
> Yours sincerely,
> Vincent Mailhol