Re: [PATCH net-next 3/5] i40e: Add handler for devlink .info_get

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/13/2023 10:07 AM, Ivan Vecera wrote:
> Provide devlink .info_get callback to allow the driver to report
> detailed version information. The following info is reported:
> 
>  "serial_number" -> The PCI DSN of the adapter
>  "fw.mgmt" -> The version of the firmware
>  "fw.mgmt.api" -> The API version of interface exposed over the AdminQ
>  "fw.psid" -> The version of the NVM image
>  "fw.bundle_id" -> Unique identifier for the combined flash image
>  "fw.undi" -> The combo image version
> 
> With this, 'devlink dev info' provides at least the same amount
> information as is reported by ETHTOOL_GDRVINFO:
> 
> $ ethtool -i enp2s0f0 | egrep '(driver|firmware)'
> driver: i40e
> firmware-version: 9.30 0x8000e5f3 1.3429.0
> 
> $ devlink dev info pci/0000:02:00.0
> pci/0000:02:00.0:
>   driver i40e
>   serial_number c0-de-b7-ff-ff-ef-ec-3c
>   versions:
>       running:
>         fw.mgmt 9.130.73618

The ice driver used fw.mgmt.build for the fw_build value, rather than
combining it into the fw.mgmt value.

>         fw.mgmt.api 1.15
>         fw.psid 9.30

As discussed in the other thread, ice used fw.psid.api

>         fw.bundle_id 0x8000e5f3
>         fw.undi 1.3429.0
> 

Does i40e have a netlist? The ice driver reports netlist versions as
well. It also reports the DDP version information, but I don't think
i40e supports that either if I recall..

> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  .../net/ethernet/intel/i40e/i40e_devlink.c    | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> index 66b7f5be45ae..fb6144d74c98 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> @@ -5,7 +5,97 @@
>  #include "i40e.h"
>  #include "i40e_devlink.h"
>  
> +static void i40e_info_get_dsn(struct i40e_pf *pf, char *buf, size_t len)
> +{
> +	u8 dsn[8];
> +
> +	put_unaligned_be64(pci_get_dsn(pf->pdev), dsn);
> +
> +	snprintf(buf, len, "%8phD", dsn);
> +}
> +
> +static void i40e_info_fw_mgmt(struct i40e_hw *hw, char *buf, size_t len)
> +{
> +	struct i40e_adminq_info *aq = &hw->aq;
> +
> +	snprintf(buf, len, "%u.%u.%05d",
> +		 aq->fw_maj_ver, aq->fw_min_ver, aq->fw_build);
> +}
> +
> +static void i40e_info_fw_api(struct i40e_hw *hw, char *buf, size_t len)
> +{
> +	struct i40e_adminq_info *aq = &hw->aq;
> +
> +	snprintf(buf, len, "%u.%u", aq->api_maj_ver, aq->api_min_ver);
> +}
> +
> +enum i40e_devlink_version_type {
> +	I40E_DL_VERSION_RUNNING,
> +};
> +
> +static int i40e_devlink_info_put(struct devlink_info_req *req,
> +				 enum i40e_devlink_version_type type,
> +				 const char *key, const char *value)
> +{
> +	if (!strlen(value))
> +		return 0;
> +
> +	switch (type) {
> +	case I40E_DL_VERSION_RUNNING:
> +		return devlink_info_version_running_put(req, key, value);
> +	}
> +	return 0;
> +}
> +
> +static int i40e_devlink_info_get(struct devlink *dl,
> +				 struct devlink_info_req *req,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct i40e_pf *pf = devlink_priv(dl);
> +	struct i40e_hw *hw = &pf->hw;
> +	char buf[32];
> +	int err;
> +
> +	i40e_info_get_dsn(pf, buf, sizeof(buf));
> +	err = devlink_info_serial_number_put(req, buf);
> +	if (err)
> +		return err;
> +
> +	i40e_info_fw_mgmt(hw, buf, sizeof(buf));
> +	err = i40e_devlink_info_put(req, I40E_DL_VERSION_RUNNING,
> +				    DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, buf);
> +	if (err)
> +		return err;
> +
> +	i40e_info_fw_api(hw, buf, sizeof(buf));
> +	err = i40e_devlink_info_put(req, I40E_DL_VERSION_RUNNING,
> +				    DEVLINK_INFO_VERSION_GENERIC_FW_MGMT_API,
> +				    buf);
> +	if (err)
> +		return err;
> +
> +	i40e_info_nvm_ver(hw, buf, sizeof(buf));
> +	err = i40e_devlink_info_put(req, I40E_DL_VERSION_RUNNING,
> +				    DEVLINK_INFO_VERSION_GENERIC_FW_PSID, buf);
> +	if (err)
> +		return err;
> +
> +	i40e_info_eetrack(hw, buf, sizeof(buf));
> +	err = i40e_devlink_info_put(req, I40E_DL_VERSION_RUNNING,
> +				    DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID,
> +				    buf);
> +	if (err)
> +		return err;
> +
> +	i40e_info_civd_ver(hw, buf, sizeof(buf));
> +	err = i40e_devlink_info_put(req, I40E_DL_VERSION_RUNNING,
> +				    DEVLINK_INFO_VERSION_GENERIC_FW_UNDI, buf);
> +
> +	return err;
> +}

The ice driver created a struct list and loop flow to iterate this. I'm
wondering if it could make sense to extract that logic into devlink
core, so that drivers just need to implement a map between version names
and functions which extract the name.

It seems like it would be straight forward to implement with a setup,
the list mapping info names to version getters, and a teardown.

Hmm...

> +
>  static const struct devlink_ops i40e_devlink_ops = {
> +	.info_get = i40e_devlink_info_get,
>  };
>  
>  /**