[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <901e8187-097e-457c-a467-e878afbc3504@redhat.com>
Date: Wed, 18 Oct 2023 13:58:49 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Jacob Keller <jacob.e.keller@...el.com>, netdev@...r.kernel.org
Cc: Jesse Brandeburg <jesse.brandeburg@...el.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
intel-wired-lan@...ts.osuosl.org
Subject: Re: [PATCH net-next 3/5] i40e: Add handler for 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@...hat.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,
>> };
>>
>> /**
>
Powered by blists - more mailing lists