Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <901e8187-097e-457c-a467-e878afbc3504@redhat.com>
Date:   Wed, 18 Oct 2023 13:58:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 3/5] i40e: Add handler for devlink .info_get
To:     Jacob Keller <jacob.e.keller@intel.com>, netdev@vger.kernel.org
Cc:     Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Tony Nguyen <anthony.l.nguyen@intel.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        intel-wired-lan@lists.osuosl.org
References: <20231013170755.2367410-1-ivecera@redhat.com>
 <20231013170755.2367410-4-ivecera@redhat.com>
 <93fa7e66-a4fc-47f5-84c8-e26551eb3204@intel.com>
Content-Language: en-US
From:   Ivan Vecera <ivecera@redhat.com>
In-Reply-To: <93fa7e66-a4fc-47f5-84c8-e26551eb3204@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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