LinuxLists.cc - [PATCH] ethtool: doc: clarify what drivers can implement in their get

2022-11-11 03:41:00

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

2022-11-11 06:46:14

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH net-next v2] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

2022-11-13 09:14:52

by Leon Romanovsky

[permalink] [raw]

Subject: Re: [PATCH net-next v2] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Fri, Nov 11, 2022 at 03:40:54PM +0900, Vincent Mailhol wrote:
> Many of the drivers which implement ethtool_ops::get_drvinfo() will
> prints the .driver, .version or .bus_info of struct ethtool_drvinfo.
> To have a glance of current state, do:
>
> $ git grep -W "get_drvinfo(struct"
>
> Printing in those three fields is useless because:
>
> - since [1], the driver version should be the kernel version (at
> least for upstream drivers). Arguably, out of tree drivers might
> still want to set a custom version, but out of tree is not our
> focus.
>
> - since [2], the core is able to provide default values for .driver
> and .bus_info.
>
> In summary, drivers may provide @fw_version and @erom_version, the
> rest is expected to be done by the core. Update the doc to reflect the
> facts.
>
> Also update the dummy driver and simply remove the callback in order
> not to confuse the newcomers: most of the drivers will not need this
> callback function any more.
>
> [1] commit 6a7e25c7fb48 ("net/core: Replace driver version to be
> kernel version")
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a7e25c7fb482dba3e80fec953f1907bcb24d52c
>
> [2] commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
> drvinfo fields even if callback exits")
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=edaf5df22cb8e7e849773ce69fcc9bc20ca92160
>
> CC: Leon Romanovsky <[email protected]>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
> Arguably, dummy.c is code and not documentation, but for me, it makes
> sense to treat it as documentation, thus I am putting everything in
> one single patch.

If to judge by newcomers submissions, many of them don't read documentation.

Thanks,
Reviewed-by: Leon Romanovsky <[email protected]>

2022-11-13 09:18:11

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

2022-11-15 05:35:53

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Sun, 13 Nov 2022 17:34:04 +0900 Vincent Mailhol wrote:
> - * Drivers should set at most @driver, @version, @fw_version and
> - * @bus_info in their get_drvinfo() implementation. The ethtool
> - * core fills in the other fields using other driver operations.
> + * Drivers should set at most @fw_version and @erom_version in their
> + * get_drvinfo() implementation. The ethtool core fills in the other
> + * fields using other driver operations.

Can I still nit pick the working on v3? :)

Almost half of the fields are not filled in by other operations,
so how about we cut deeper? Even @erom_version is only filled in by
a single driver, and pretty much deprecated (devlink is much more
flexible for all FW version reporting and flashing).

How about:

* Majority of the drivers should no longer implement this callback.
* Most fields are correctly filled in by the core using system
* information, or populated using other driver operations.

2022-11-15 08:05:14

by Vincent MAILHOL

[permalink] [raw]

Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Tue. 15 Nov. 2022 at 14:27, Jakub Kicinski <[email protected]> wrote:
> On Sun, 13 Nov 2022 17:34:04 +0900 Vincent Mailhol wrote:
> > - * Drivers should set at most @driver, @version, @fw_version and
> > - * @bus_info in their get_drvinfo() implementation. The ethtool
> > - * core fills in the other fields using other driver operations.
> > + * Drivers should set at most @fw_version and @erom_version in their
> > + * get_drvinfo() implementation. The ethtool core fills in the other
> > + * fields using other driver operations.
>
> Can I still nit pick the working on v3? :)

Nitpicks are welcome!

> Almost half of the fields are not filled in by other operations,
> so how about we cut deeper? Even @erom_version is only filled in by
> a single driver, and pretty much deprecated

I do not like to say that it is "pretty much deprecated". Either it is
deprecated and we can start to clean things up, or it is not and
people can continue to use it. It is good to have a clear position,
whatever it is.

> (devlink is much more flexible for all FW version reporting and flashing).

Side note, I just started to study devlink.

> How about:
>
> * Majority of the drivers should no longer implement this callback.
> * Most fields are correctly filled in by the core using system
> * information, or populated using other driver operations.

> * Majority of the drivers should no longer implement this callback.
^^^^
In this context, "this callback" is not defined (there is no prior mention in
this struct doc). I will replace it with "the drv_info() callback".

I am fine to try to discourage the developper from implementing the
callback. But I still want a small note to state the facts (c.f. above
comment, unless we explicitly deprecate the drv_info(), I do not think
we should try to completely hide its existence).

What about this:

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index dc2aa3d75b39..fe4d8dddb0a7 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -159,8 +159,10 @@ static inline __u32 ethtool_cmd_speed(const
struct ethtool_cmd *ep)
* in its bus driver structure (e.g. pci_driver::name). Must
* not be an empty string.
* @version: Driver version string; may be an empty string
- * @fw_version: Firmware version string; may be an empty string
- * @erom_version: Expansion ROM version string; may be an empty string
+ * @fw_version: Firmware version string; drivers can set it; may be an
+ * empty string
+ * @erom_version: Expansion ROM version string; drivers can set it;
+ * may be an empty string
* @bus_info: Device bus address. This should match the dev_name()
* string for the underlying bus device, if there is one. May be
* an empty string.
@@ -180,9 +182,10 @@ static inline __u32 ethtool_cmd_speed(const
struct ethtool_cmd *ep)
* Users can use the %ETHTOOL_GSSET_INFO command to get the number of
* strings in any string set (from Linux 2.6.34).
*
- * Drivers should set at most @driver, @version, @fw_version and
- * @bus_info in their get_drvinfo() implementation. The ethtool
- * core fills in the other fields using other driver operations.
+ * Majority of the drivers should no longer implement the
+ * get_drvinfo() callback. Most fields are correctly filled in by the
+ * core using system information, or populated using other driver
+ * operations.
*/
struct ethtool_drvinfo {
__u32 cmd;

Yours sincerely,
Vincent Mailhol

2022-11-15 16:43:25

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Tue, 15 Nov 2022 16:52:39 +0900 Vincent MAILHOL wrote:
> - * @fw_version: Firmware version string; may be an empty string
> - * @erom_version: Expansion ROM version string; may be an empty string
> + * @fw_version: Firmware version string; drivers can set it; may be an
> + * empty string
> + * @erom_version: Expansion ROM version string; drivers can set it;
> + * may be an empty string

"drivers can set it" rings a little odd to my non-native-English-
-speaker's ear. Perhaps "driver-defined;" ? Either way is fine, tho.

> * @bus_info: Device bus address. This should match the dev_name()
> * string for the underlying bus device, if there is one. May be
> * an empty string.
> @@ -180,9 +182,10 @@ static inline __u32 ethtool_cmd_speed(const
> struct ethtool_cmd *ep)
> * Users can use the %ETHTOOL_GSSET_INFO command to get the number of
> * strings in any string set (from Linux 2.6.34).
> *
> - * Drivers should set at most @driver, @version, @fw_version and
> - * @bus_info in their get_drvinfo() implementation. The ethtool
> - * core fills in the other fields using other driver operations.
> + * Majority of the drivers should no longer implement the
> + * get_drvinfo() callback. Most fields are correctly filled in by the
> + * core using system information, or populated using other driver
> + * operations.

SG! Good point on the doc being for the struct. We can make the notice
even stronger if you want by saying s/Majority of the/Modern/

2022-11-15 23:41:21

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH net-next v4] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

Many of the drivers which implement ethtool_ops::get_drvinfo() will
prints the .driver, .version or .bus_info of struct ethtool_drvinfo.
To have a glance of current state, do:

$ git grep -W "get_drvinfo(struct"

Printing in those three fields is useless because:

- since [1], the driver version should be the kernel version (at
least for upstream drivers). Arguably, out of tree drivers might
still want to set a custom version, but out of tree is not our
focus.

- since [2], the core is able to provide default values for .driver
and .bus_info.

In summary, drivers may provide .fw_version and .erom_version, the
rest is expected to be done by the core. Update the doc to reflect the
facts and discourage developers from implementing the get_drvinfo()
callback.

Also update the dummy driver and simply remove the callback in order
not to confuse the newcomers: most of the drivers will not need this
callback function any more.

[1] commit 6a7e25c7fb48 ("net/core: Replace driver version to be
kernel version")
Link: https://git.kernel.org/torvalds/linux/c/6a7e25c7fb48

[2] commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
drvinfo fields even if callback exits")
Link: https://git.kernel.org/netdev/net-next/c/edaf5df22cb8

Reviewed-by: Leon Romanovsky <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
* Changelog *

v3 -> v4:

* rephrasing of the documentation according to Jakub's comments.

v2 -> v3:

* add Reviewed-by: Leon Romanovsky <[email protected]> tag.
* use shorter links.

v1 -> v2:

* forgot the net-next prefix in the patch subject... Sorry for the
noise.
---
drivers/net/dummy.c | 7 -------
include/uapi/linux/ethtool.h | 12 +++++++-----
2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index aa0fc00faecb..c4b1b0aa438a 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -99,14 +99,7 @@ static const struct net_device_ops dummy_netdev_ops = {
.ndo_change_carrier = dummy_change_carrier,
};

-static void dummy_get_drvinfo(struct net_device *dev,
- struct ethtool_drvinfo *info)
-{
- strscpy(info->driver, DRV_NAME, sizeof(info->driver));
-}
-
static const struct ethtool_ops dummy_ethtool_ops = {
- .get_drvinfo = dummy_get_drvinfo,
.get_ts_info = ethtool_op_get_ts_info,
};

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index dc2aa3d75b39..e801bd4bd6c7 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -159,8 +159,10 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
* in its bus driver structure (e.g. pci_driver::name). Must
* not be an empty string.
* @version: Driver version string; may be an empty string
- * @fw_version: Firmware version string; may be an empty string
- * @erom_version: Expansion ROM version string; may be an empty string
+ * @fw_version: Firmware version string; driver defined; may be an
+ * empty string
+ * @erom_version: Expansion ROM version string; driver defined; may be
+ * an empty string
* @bus_info: Device bus address. This should match the dev_name()
* string for the underlying bus device, if there is one. May be
* an empty string.
@@ -180,9 +182,9 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
* Users can use the %ETHTOOL_GSSET_INFO command to get the number of
* strings in any string set (from Linux 2.6.34).
*
- * Drivers should set at most @driver, @version, @fw_version and
- * @bus_info in their get_drvinfo() implementation. The ethtool
- * core fills in the other fields using other driver operations.
+ * Modern drivers no longer have to implement the get_drvinfo()
+ * callback. Most fields are correctly filled in by the core using
+ * system information, or populated using other driver operations.
*/
struct ethtool_drvinfo {
__u32 cmd;
--
2.25.1

2022-11-16 00:47:48

by Vincent MAILHOL

[permalink] [raw]

Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Tue. 16 Nov. 2022 at 01:28, Jakub Kicinski <[email protected]> wrote:
> On Tue, 15 Nov 2022 16:52:39 +0900 Vincent MAILHOL wrote:
> > - * @fw_version: Firmware version string; may be an empty string
> > - * @erom_version: Expansion ROM version string; may be an empty string
> > + * @fw_version: Firmware version string; drivers can set it; may be an
> > + * empty string
> > + * @erom_version: Expansion ROM version string; drivers can set it;
> > + * may be an empty string
>
> "drivers can set it" rings a little odd to my non-native-English-
> -speaker's ear. Perhaps "driver-defined;" ? Either way is fine, tho.

I am not a native speaker either so I won't argue here. Changed to
"driver defined" in v4.

> > * @bus_info: Device bus address. This should match the dev_name()
> > * string for the underlying bus device, if there is one. May be
> > * an empty string.
> > @@ -180,9 +182,10 @@ static inline __u32 ethtool_cmd_speed(const
> > struct ethtool_cmd *ep)
> > * Users can use the %ETHTOOL_GSSET_INFO command to get the number of
> > * strings in any string set (from Linux 2.6.34).
> > *
> > - * Drivers should set at most @driver, @version, @fw_version and
> > - * @bus_info in their get_drvinfo() implementation. The ethtool
> > - * core fills in the other fields using other driver operations.
> > + * Majority of the drivers should no longer implement the
> > + * get_drvinfo() callback. Most fields are correctly filled in by the
> > + * core using system information, or populated using other driver
> > + * operations.
>
> SG! Good point on the doc being for the struct. We can make the notice
> even stronger if you want by saying s/Majority of the/Modern/

"Modern drivers should no longer..." sounds like an unconditional
interdiction. I changed it to "Modern drivers no longer have to..." to
nuance the terms and suggest that they remain some edge cases.

I sent the v4 but messed up the In-Reply-To tag so it became a new thread:
https://lore.kernel.org/netdev/[email protected]/T/#u

Yours sincerely,
Vincent Mailhol

2022-11-16 04:41:41

by Vincent MAILHOL

[permalink] [raw]

Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

On Wed. 16 Nov. 2022 at 09:30, Vincent MAILHOL
<[email protected]> wrote:
> On Tue. 16 Nov. 2022 at 01:28, Jakub Kicinski <[email protected]> wrote:
> > On Tue, 15 Nov 2022 16:52:39 +0900 Vincent MAILHOL wrote:
> > SG! Good point on the doc being for the struct.

That comment just made me realize that actually, struct ethtool_ops is
also outdated:
https://elixir.bootlin.com/linux/latest/source/include/linux/ethtool.h#L462

Please ignore the v4, I will send another patch to also fix the struct
ethtool_ops doc.

Yours sincerely,
Vincent Mailhol

2022-11-16 18:02:11

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

Many of the drivers which implement ethtool_ops::get_drvinfo() will
prints the .driver, .version or .bus_info of struct ethtool_drvinfo.
To have a glance of current state, do:

$ git grep -W "get_drvinfo(struct"

Printing in those three fields is useless because:

- since [1], the driver version should be the kernel version (at
least for upstream drivers). Arguably, out of tree drivers might
still want to set a custom version, but out of tree is not our
focus.

- since [2], the core is able to provide default values for .driver
and .bus_info.

In summary, drivers may provide .fw_version and .erom_version, the
rest is expected to be done by the core.

In struct ethtool_ops doc from linux/ethtool: rephrase field
get_drvinfo() doc to discourage developers from implementing this
callback.

In struct ethtool_drvinfo doc from uapi/linux/ethtool.h: remove the
paragraph mentioning what drivers should do. Rationale: no need to
repeat what is already written in struct ethtool_ops doc. But add a
note that .fw_version and .erom_version are driver defined.

Also update the dummy driver and simply remove the callback in order
not to confuse the newcomers: most of the drivers will not need this
callback function any more.

[1] commit 6a7e25c7fb48 ("net/core: Replace driver version to be
kernel version")
Link: https://git.kernel.org/torvalds/linux/c/6a7e25c7fb48

[2] commit edaf5df22cb8 ("ethtool: ethtool_get_drvinfo: populate
drvinfo fields even if callback exits")
Link: https://git.kernel.org/netdev/net-next/c/edaf5df22cb8

Reviewed-by: Leon Romanovsky <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
* Changelog *

v4 -> v5:

* update struct ethtool_drvinfo doc in uapi/linux/ethtool.h as well.

* remove the paragraph mentioning what drivers should do from struct
ethtool_drvinfo in uapi/linux/ethtool.h. Rationale: no need to
repeat what is already written in struct ethtool_ops doc.

v3 -> v4:

* rephrasing of the documentation according to Jakub's comments.

v2 -> v3:

* add Reviewed-by: Leon Romanovsky <[email protected]> tag.

* use shorter links.

v1 -> v2:

* forgot the net-next prefix in the patch subject... Sorry for the
noise.
---
drivers/net/dummy.c | 7 -------
include/linux/ethtool.h | 8 ++++----
include/uapi/linux/ethtool.h | 10 ++++------
3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index aa0fc00faecb..c4b1b0aa438a 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -99,14 +99,7 @@ static const struct net_device_ops dummy_netdev_ops = {
.ndo_change_carrier = dummy_change_carrier,
};

-static void dummy_get_drvinfo(struct net_device *dev,
- struct ethtool_drvinfo *info)
-{
- strscpy(info->driver, DRV_NAME, sizeof(info->driver));
-}
-
static const struct ethtool_ops dummy_ethtool_ops = {
- .get_drvinfo = dummy_get_drvinfo,
.get_ts_info = ethtool_op_get_ts_info,
};

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5c51c7fda32a..9e0a76fc7de9 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -473,10 +473,10 @@ struct ethtool_module_power_mode_params {
* parameter.
* @supported_coalesce_params: supported types of interrupt coalescing.
* @supported_ring_params: supported ring params.
- * @get_drvinfo: Report driver/device information. Should only set the
- * @driver, @version, @fw_version and @bus_info fields. If not
- * implemented, the @driver and @bus_info fields will be filled in
- * according to the netdev's parent device.
+ * @get_drvinfo: Report driver/device information. Modern drivers no
+ * longer have to implement this callback. Most fields are
+ * correctly filled in by the core using system information, or
+ * populated using other driver operations.
* @get_regs_len: Get buffer length required for @get_regs
* @get_regs: Get device registers
* @get_wol: Report whether Wake-on-Lan is enabled
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f341de2ae612..58e587ba0450 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -159,8 +159,10 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
* in its bus driver structure (e.g. pci_driver::name). Must
* not be an empty string.
* @version: Driver version string; may be an empty string
- * @fw_version: Firmware version string; may be an empty string
- * @erom_version: Expansion ROM version string; may be an empty string
+ * @fw_version: Firmware version string; driver defined; may be an
+ * empty string
+ * @erom_version: Expansion ROM version string; driver defined; may be
+ * an empty string
* @bus_info: Device bus address. This should match the dev_name()
* string for the underlying bus device, if there is one. May be
* an empty string.
@@ -179,10 +181,6 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
*
* Users can use the %ETHTOOL_GSSET_INFO command to get the number of
* strings in any string set (from Linux 2.6.34).
- *
- * Drivers should set at most @driver, @version, @fw_version and
- * @bus_info in their get_drvinfo() implementation. The ethtool
- * core fills in the other fields using other driver operations.
*/
struct ethtool_drvinfo {
__u32 cmd;
--
2.37.4

2022-11-18 04:57:05

by patchwork-bot+netdevbpf

[permalink] [raw]

Subject: Re: [PATCH v5] ethtool: doc: clarify what drivers can implement in their get_drvinfo()

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:

On Thu, 17 Nov 2022 02:18:28 +0900 you wrote:
> Many of the drivers which implement ethtool_ops::get_drvinfo() will
> prints the .driver, .version or .bus_info of struct ethtool_drvinfo.
> To have a glance of current state, do:
>
> $ git grep -W "get_drvinfo(struct"
>
> Printing in those three fields is useless because:
>
> [...]

Here is the summary with links:
- [v5] ethtool: doc: clarify what drivers can implement in their get_drvinfo()
https://git.kernel.org/netdev/net-next/c/f20a0a0519f3

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