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

  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
        fw.mgmt.api 1.15
        fw.psid 9.30
        fw.bundle_id 0x8000e5f3
        fw.undi 1.3429.0

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 .../net/ethernet/intel/i40e/i40e_devlink.c    | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
  

On Fri, 13 Oct 2023 19:07:53 +0200 Ivan Vecera wrote:
>  "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

Your board reports "fw.psid 9.30", this may not be right,
PSID is more of a board+customer ID, IIUC. 9.30 looks like
a version, not an ID.

>  "fw.bundle_id" -> Unique identifier for the combined flash image
>  "fw.undi" -> The combo image version

UNDI means PXE. Is that whave "combo image" means for Intel?
  

On 16. 10. 23 16:56, Jakub Kicinski wrote:
> On Fri, 13 Oct 2023 19:07:53 +0200 Ivan Vecera wrote:
>>   "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
> 
> Your board reports "fw.psid 9.30", this may not be right,
> PSID is more of a board+customer ID, IIUC. 9.30 looks like
> a version, not an ID.

Maybe plain 'fw' should be used for this '9.30' as this is a version
of the whole software package provided by Intel for these adapters
(e.g. 
https://www.intel.com/content/www/us/en/download/18190/non-volatile-memory-nvm-update-utility-for-intel-ethernet-network-adapter-700-series.html).

Thoughts?

>>   "fw.bundle_id" -> Unique identifier for the combined flash image
>>   "fw.undi" -> The combo image version
> 
> UNDI means PXE. Is that whave "combo image" means for Intel?

Combo image version (aka CIVD) is reported by nvmupdate tool and this
should be version of OROM that contains PXE, EFI images that each of
them can have specific version but this CIVD should be overall OROM 
version for this combination of PXE and EFI. I hope I'm right.

Thanks,
Ivan
  

On Tue, 17 Oct 2023 11:56:20 +0200 Ivan Vecera wrote:
> > Your board reports "fw.psid 9.30", this may not be right,
> > PSID is more of a board+customer ID, IIUC. 9.30 looks like
> > a version, not an ID.  
> 
> Maybe plain 'fw' should be used for this '9.30' as this is a version
> of the whole software package provided by Intel for these adapters
> (e.g. 
> https://www.intel.com/content/www/us/en/download/18190/non-volatile-memory-nvm-update-utility-for-intel-ethernet-network-adapter-700-series.html).
> 
> Thoughts?

Hm, that could be better, yes.

Jake, any guidance?

> > UNDI means PXE. Is that whave "combo image" means for Intel?  
> 
> Combo image version (aka CIVD) is reported by nvmupdate tool and this
> should be version of OROM that contains PXE, EFI images that each of
> them can have specific version but this CIVD should be overall OROM 
> version for this combination of PXE and EFI. I hope I'm right.

Sounds good then!
  

On 10/17/2023 8:21 AM, Jakub Kicinski wrote:
> On Tue, 17 Oct 2023 11:56:20 +0200 Ivan Vecera wrote:
>>> Your board reports "fw.psid 9.30", this may not be right,
>>> PSID is more of a board+customer ID, IIUC. 9.30 looks like
>>> a version, not an ID.  
>>
>> Maybe plain 'fw' should be used for this '9.30' as this is a version
>> of the whole software package provided by Intel for these adapters
>> (e.g. 
>> https://www.intel.com/content/www/us/en/download/18190/non-volatile-memory-nvm-update-utility-for-intel-ethernet-network-adapter-700-series.html).
>>
>> Thoughts?
> 
> Hm, that could be better, yes.
> 
> Jake, any guidance?

Hm. The ice driver has 'fw.psid.api' which is documented as:

    * - ``fw.psid.api``
      - running
      - 0.80
      - Version defining the format of the flash contents.

I think we settled on this as well back when I was working on the ice
version.

See
https://lore.kernel.org/netdev/70001e87-b369-bab4-f318-ad4514e7dcfb@intel.com/

However, looking at the code more closely, this does appear to match the
ice driver's implementation if you use "fw.psid.api". I believe the
intent for this is a version that indicates the format or layout of the
NVM contents.

Given that ice uses fw.psid.api for what appears to be the same purpose
I would propose that here as well.

> 
>>> UNDI means PXE. Is that whave "combo image" means for Intel?  
>>
>> Combo image version (aka CIVD) is reported by nvmupdate tool and this
>> should be version of OROM that contains PXE, EFI images that each of
>> them can have specific version but this CIVD should be overall OROM 
>> version for this combination of PXE and EFI. I hope I'm right.
> 
> Sounds good then!

Yes that sounds correct. That's what we do in ice as well.

I'm going to review the whole patch now since I hadn't noticed this
previously.

Thanks,
Jake
  

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,
>  };
>  
>  /**
  

On 17. 10. 23 19:17, Jacob Keller wrote:
> 
> 
> 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.

OK, will fix by follow up.

>>          fw.mgmt.api 1.15
>>          fw.psid 9.30
> 
> As discussed in the other thread, ice used fw.psid.api

OK, will change it to 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..

i40e supports to load DDP in runtime by ethtool flash function and the
name and version of DDP package could be provided IMHO.


>> 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,
>>   };
>>   
>>   /**
>