Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221111030838.1059-1-mailhol.vincent@wanadoo.fr>
 <20221113083404.86983-1-mailhol.vincent@wanadoo.fr> <20221114212718.76bd6c8b@kernel.org>
In-Reply-To: <20221114212718.76bd6c8b@kernel.org>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Tue, 15 Nov 2022 16:52:39 +0900
Message-ID: <CAMZ6RqJ-2_ymLiGuObmBLRDpNNy0ZpMCeRU2qgNPvq2oArnX8A@mail.gmail.com>
Subject: Re: [PATCH net-next v3] ethtool: doc: clarify what drivers can
 implement in their get_drvinfo()
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Petr Machata <petrm@nvidia.com>,
        Hao Chen <chenhao288@hisilicon.com>,
        Amit Cohen <amcohen@nvidia.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Sean Anderson <sean.anderson@seco.com>,
        linux-kernel@vger.kernel.org,
        Leon Romanovsky <leonro@mellanox.com>,
        Leon Romanovsky <leonro@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue. 15 Nov. 2022 at 14:27, Jakub Kicinski <kuba@kernel.org> 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