The series adds initial support for devlink to i40e driver.
Patch-set overview:
Patch 1: Adds initial devlink support (devlink and port registration)
Patch 2: Refactors and split i40e_nvm_version_str()
Patch 3: Adds support for 'devlink dev info'
Patch 4: Refactors existing helper function to read PBA ID
Patch 5: Adds 'board.id' to 'devlink dev info' using PBA ID
Ivan Vecera (5):
i40e: Add initial devlink support
i40e: Split and refactor i40e_nvm_version_str()
i40e: Add handler for devlink .info_get
i40e: Refactor and rename i40e_read_pba_string()
i40e: Add PBA as board id info to devlink .info_get
drivers/net/ethernet/intel/Kconfig | 1 +
drivers/net/ethernet/intel/i40e/Makefile | 3 +-
drivers/net/ethernet/intel/i40e/i40e.h | 136 ++++++++---
drivers/net/ethernet/intel/i40e/i40e_common.c | 58 +++--
.../net/ethernet/intel/i40e/i40e_devlink.c | 224 ++++++++++++++++++
.../net/ethernet/intel/i40e/i40e_devlink.h | 18 ++
.../net/ethernet/intel/i40e/i40e_ethtool.c | 4 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 40 +++-
.../net/ethernet/intel/i40e/i40e_prototype.h | 3 +-
drivers/net/ethernet/intel/i40e/i40e_type.h | 3 +
10 files changed, 414 insertions(+), 76 deletions(-)
create mode 100644 drivers/net/ethernet/intel/i40e/i40e_devlink.c
create mode 100644 drivers/net/ethernet/intel/i40e/i40e_devlink.h
--
2.41.0
Function i40e_read_pba_string() is currently unused but will be used
by subsequent patch to provide board ID via devlink device info.
The function reads PBA block from NVM so it cannot be called during
adapter reset and as we would like to provide PBA ID via devlink
info it is better to read the PBA ID during i40e_probe() and cache
it in i40e_hw structure to avoid a waiting for potential adapter
reset in devlink info callback.
So...
- Remove pba_num and pba_num_size arguments from the function,
allocate resource managed buffer to store PBA ID string and
save resulting pointer to i40e_hw->pba_id field
- Make the function void as the PBA ID can be missing and in this
case (or in case of NVM reading failure) the i40e_hw->pba_id
will be NULL
- Rename the function to i40e_get_pba_string() to align with other
functions like i40e_get_oem_version() i40e_get_port_mac_addr()...
- Call this function on init during i40e_probe()
Signed-off-by: Ivan Vecera <[email protected]>
---
drivers/net/ethernet/intel/i40e/i40e_common.c | 58 +++++++++++--------
drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
.../net/ethernet/intel/i40e/i40e_prototype.h | 3 +-
drivers/net/ethernet/intel/i40e/i40e_type.h | 3 +
4 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 6d1042ca0317..04db9cdc7d94 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -821,62 +821,72 @@ void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable)
}
/**
- * i40e_read_pba_string - Reads part number string from EEPROM
+ * i40e_get_pba_string - Reads part number string from EEPROM
* @hw: pointer to hardware structure
- * @pba_num: stores the part number string from the EEPROM
- * @pba_num_size: part number string buffer length
*
- * Reads the part number string from the EEPROM.
+ * Reads the part number string from the EEPROM and stores it
+ * into newly allocated buffer and saves resulting pointer
+ * to i40e_hw->pba_id field.
**/
-int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
- u32 pba_num_size)
+void i40e_get_pba_string(struct i40e_hw *hw)
{
+#define I40E_NVM_PBA_FLAGS_BLK_PRESENT 0xFAFA
u16 pba_word = 0;
u16 pba_size = 0;
u16 pba_ptr = 0;
- int status = 0;
- u16 i = 0;
+ int status;
+ char *ptr;
+ u16 i;
status = i40e_read_nvm_word(hw, I40E_SR_PBA_FLAGS, &pba_word);
- if (status || (pba_word != 0xFAFA)) {
- hw_dbg(hw, "Failed to read PBA flags or flag is invalid.\n");
- return status;
+ if (status) {
+ hw_dbg(hw, "Failed to read PBA flags.\n");
+ return;
+ }
+ if (pba_word != I40E_NVM_PBA_FLAGS_BLK_PRESENT) {
+ hw_dbg(hw, "PBA block is not present.\n");
+ return;
}
status = i40e_read_nvm_word(hw, I40E_SR_PBA_BLOCK_PTR, &pba_ptr);
if (status) {
hw_dbg(hw, "Failed to read PBA Block pointer.\n");
- return status;
+ return;
}
status = i40e_read_nvm_word(hw, pba_ptr, &pba_size);
if (status) {
hw_dbg(hw, "Failed to read PBA Block size.\n");
- return status;
+ return;
}
/* Subtract one to get PBA word count (PBA Size word is included in
- * total size)
+ * total size) and advance pointer to first PBA word.
*/
pba_size--;
- if (pba_num_size < (((u32)pba_size * 2) + 1)) {
- hw_dbg(hw, "Buffer too small for PBA data.\n");
- return -EINVAL;
+ pba_ptr++;
+ if (!pba_size) {
+ hw_dbg(hw, "PBA ID is empty.\n");
+ return;
}
+ ptr = devm_kzalloc(i40e_hw_to_dev(hw), pba_size * 2 + 1, GFP_KERNEL);
+ if (!ptr)
+ return;
+ hw->pba_id = ptr;
+
for (i = 0; i < pba_size; i++) {
- status = i40e_read_nvm_word(hw, (pba_ptr + 1) + i, &pba_word);
+ status = i40e_read_nvm_word(hw, pba_ptr + i, &pba_word);
if (status) {
hw_dbg(hw, "Failed to read PBA Block word %d.\n", i);
- return status;
+ devm_kfree(i40e_hw_to_dev(hw), hw->pba_id);
+ hw->pba_id = NULL;
+ return;
}
- pba_num[(i * 2)] = (pba_word >> 8) & 0xFF;
- pba_num[(i * 2) + 1] = pba_word & 0xFF;
+ *ptr++ = (pba_word >> 8) & 0xFF;
+ *ptr++ = pba_word & 0xFF;
}
- pba_num[(pba_size * 2)] = '\0';
-
- return status;
}
/**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index ba8fb0556216..3157d14d9b12 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -15846,6 +15846,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_pf_reset;
}
i40e_get_oem_version(hw);
+ i40e_get_pba_string(hw);
/* provide nvm, fw, api versions, vendor:device id, subsys vendor:device id */
i40e_nvm_version_str(hw, nvm_ver, sizeof(nvm_ver));
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 46b9a05ceb91..001162042050 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -341,8 +341,7 @@ i40e_aq_configure_partition_bw(struct i40e_hw *hw,
struct i40e_aqc_configure_partition_bw_data *bw_data,
struct i40e_asq_cmd_details *cmd_details);
int i40e_get_port_mac_addr(struct i40e_hw *hw, u8 *mac_addr);
-int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
- u32 pba_num_size);
+void i40e_get_pba_string(struct i40e_hw *hw);
void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable);
/* prototype for functions used for NVM access */
int i40e_init_nvm(struct i40e_hw *hw);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index dc7cd16ad8fb..aff6dc6afbe2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -493,6 +493,9 @@ struct i40e_hw {
struct i40e_nvm_info nvm;
struct i40e_fc_info fc;
+ /* PBA ID */
+ const char *pba_id;
+
/* pci info */
u16 device_id;
u16 vendor_id;
--
2.41.0
Expose stored PBA ID string as unique board identifier via
devlink's .info_get command.
Signed-off-by: Ivan Vecera <[email protected]>
---
drivers/net/ethernet/intel/i40e/i40e_devlink.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
index fb6144d74c98..9168ade8da47 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
@@ -29,7 +29,15 @@ static void i40e_info_fw_api(struct i40e_hw *hw, char *buf, size_t len)
snprintf(buf, len, "%u.%u", aq->api_maj_ver, aq->api_min_ver);
}
+static void i40e_info_pba(struct i40e_hw *hw, char *buf, size_t len)
+{
+ buf[0] = '\0';
+ if (hw->pba_id)
+ strscpy(buf, hw->pba_id, len);
+}
+
enum i40e_devlink_version_type {
+ I40E_DL_VERSION_FIXED,
I40E_DL_VERSION_RUNNING,
};
@@ -41,6 +49,8 @@ static int i40e_devlink_info_put(struct devlink_info_req *req,
return 0;
switch (type) {
+ case I40E_DL_VERSION_FIXED:
+ return devlink_info_version_fixed_put(req, key, value);
case I40E_DL_VERSION_RUNNING:
return devlink_info_version_running_put(req, key, value);
}
@@ -90,6 +100,12 @@ static int i40e_devlink_info_get(struct devlink *dl,
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);
+ if (err)
+ return err;
+
+ i40e_info_pba(hw, buf, sizeof(buf));
+ err = i40e_devlink_info_put(req, I40E_DL_VERSION_FIXED,
+ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, buf);
return err;
}
--
2.41.0
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 <[email protected]>
---
.../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;
+}
+
static const struct devlink_ops i40e_devlink_ops = {
+ .info_get = i40e_devlink_info_get,
};
/**
--
2.41.0
Hello:
This series was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:
On Fri, 13 Oct 2023 19:07:50 +0200 you wrote:
> The series adds initial support for devlink to i40e driver.
>
> Patch-set overview:
> Patch 1: Adds initial devlink support (devlink and port registration)
> Patch 2: Refactors and split i40e_nvm_version_str()
> Patch 3: Adds support for 'devlink dev info'
> Patch 4: Refactors existing helper function to read PBA ID
> Patch 5: Adds 'board.id' to 'devlink dev info' using PBA ID
>
> [...]
Here is the summary with links:
- [net-next,1/5] i40e: Add initial devlink support
https://git.kernel.org/netdev/net-next/c/9e479d64dc58
- [net-next,2/5] i40e: Split and refactor i40e_nvm_version_str()
https://git.kernel.org/netdev/net-next/c/7aabde397683
- [net-next,3/5] i40e: Add handler for devlink .info_get
https://git.kernel.org/netdev/net-next/c/5a423552e0d9
- [net-next,4/5] i40e: Refactor and rename i40e_read_pba_string()
https://git.kernel.org/netdev/net-next/c/df19ea696644
- [net-next,5/5] i40e: Add PBA as board id info to devlink .info_get
https://git.kernel.org/netdev/net-next/c/3e02480d5e38
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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/[email protected]/
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 <[email protected]>
> ---
> .../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 10/13/2023 10:07 AM, Ivan Vecera wrote:
> Function i40e_read_pba_string() is currently unused but will be used
> by subsequent patch to provide board ID via devlink device info.
>
> The function reads PBA block from NVM so it cannot be called during
> adapter reset and as we would like to provide PBA ID via devlink
> info it is better to read the PBA ID during i40e_probe() and cache
> it in i40e_hw structure to avoid a waiting for potential adapter
> reset in devlink info callback.
>
> So...
> - Remove pba_num and pba_num_size arguments from the function,
> allocate resource managed buffer to store PBA ID string and
> save resulting pointer to i40e_hw->pba_id field
> - Make the function void as the PBA ID can be missing and in this
> case (or in case of NVM reading failure) the i40e_hw->pba_id
> will be NULL
> - Rename the function to i40e_get_pba_string() to align with other
> functions like i40e_get_oem_version() i40e_get_port_mac_addr()...
> - Call this function on init during i40e_probe()
>
And the PBA value shouldn't be changing even with a new NVM image
flashed to the device, so its not something that is likely to have
updated at run time, thus saving during probe is sufficient.
Makes sense.
Reviewed-by: Jacob Keller <[email protected]>
> Signed-off-by: Ivan Vecera <[email protected]>
> ---
> drivers/net/ethernet/intel/i40e/i40e_common.c | 58 +++++++++++--------
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
> .../net/ethernet/intel/i40e/i40e_prototype.h | 3 +-
> drivers/net/ethernet/intel/i40e/i40e_type.h | 3 +
> 4 files changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 6d1042ca0317..04db9cdc7d94 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -821,62 +821,72 @@ void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable)
> }
>
> /**
> - * i40e_read_pba_string - Reads part number string from EEPROM
> + * i40e_get_pba_string - Reads part number string from EEPROM
> * @hw: pointer to hardware structure
> - * @pba_num: stores the part number string from the EEPROM
> - * @pba_num_size: part number string buffer length
> *
> - * Reads the part number string from the EEPROM.
> + * Reads the part number string from the EEPROM and stores it
> + * into newly allocated buffer and saves resulting pointer
> + * to i40e_hw->pba_id field.
> **/
> -int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
> - u32 pba_num_size)
> +void i40e_get_pba_string(struct i40e_hw *hw)
> {
> +#define I40E_NVM_PBA_FLAGS_BLK_PRESENT 0xFAFA
> u16 pba_word = 0;
> u16 pba_size = 0;
> u16 pba_ptr = 0;
> - int status = 0;
> - u16 i = 0;
> + int status;
> + char *ptr;
> + u16 i;
>
> status = i40e_read_nvm_word(hw, I40E_SR_PBA_FLAGS, &pba_word);
> - if (status || (pba_word != 0xFAFA)) {
> - hw_dbg(hw, "Failed to read PBA flags or flag is invalid.\n");
> - return status;
> + if (status) {
> + hw_dbg(hw, "Failed to read PBA flags.\n");
> + return;
> + }
> + if (pba_word != I40E_NVM_PBA_FLAGS_BLK_PRESENT) {
> + hw_dbg(hw, "PBA block is not present.\n");
> + return;
> }
>
> status = i40e_read_nvm_word(hw, I40E_SR_PBA_BLOCK_PTR, &pba_ptr);
> if (status) {
> hw_dbg(hw, "Failed to read PBA Block pointer.\n");
> - return status;
> + return;
> }
>
> status = i40e_read_nvm_word(hw, pba_ptr, &pba_size);
> if (status) {
> hw_dbg(hw, "Failed to read PBA Block size.\n");
> - return status;
> + return;
> }
>
> /* Subtract one to get PBA word count (PBA Size word is included in
> - * total size)
> + * total size) and advance pointer to first PBA word.
> */
> pba_size--;
> - if (pba_num_size < (((u32)pba_size * 2) + 1)) {
> - hw_dbg(hw, "Buffer too small for PBA data.\n");
> - return -EINVAL;
> + pba_ptr++;
> + if (!pba_size) {
> + hw_dbg(hw, "PBA ID is empty.\n");
> + return;
> }
>
> + ptr = devm_kzalloc(i40e_hw_to_dev(hw), pba_size * 2 + 1, GFP_KERNEL);
> + if (!ptr)
> + return;
> + hw->pba_id = ptr;
> +
> for (i = 0; i < pba_size; i++) {
> - status = i40e_read_nvm_word(hw, (pba_ptr + 1) + i, &pba_word);
> + status = i40e_read_nvm_word(hw, pba_ptr + i, &pba_word);
> if (status) {
> hw_dbg(hw, "Failed to read PBA Block word %d.\n", i);
> - return status;
> + devm_kfree(i40e_hw_to_dev(hw), hw->pba_id);
> + hw->pba_id = NULL;
> + return;
> }
>
> - pba_num[(i * 2)] = (pba_word >> 8) & 0xFF;
> - pba_num[(i * 2) + 1] = pba_word & 0xFF;
> + *ptr++ = (pba_word >> 8) & 0xFF;
> + *ptr++ = pba_word & 0xFF;
> }
> - pba_num[(pba_size * 2)] = '\0';
> -
> - return status;
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index ba8fb0556216..3157d14d9b12 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -15846,6 +15846,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_pf_reset;
> }
> i40e_get_oem_version(hw);
> + i40e_get_pba_string(hw);
>
> /* provide nvm, fw, api versions, vendor:device id, subsys vendor:device id */
> i40e_nvm_version_str(hw, nvm_ver, sizeof(nvm_ver));
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> index 46b9a05ceb91..001162042050 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> @@ -341,8 +341,7 @@ i40e_aq_configure_partition_bw(struct i40e_hw *hw,
> struct i40e_aqc_configure_partition_bw_data *bw_data,
> struct i40e_asq_cmd_details *cmd_details);
> int i40e_get_port_mac_addr(struct i40e_hw *hw, u8 *mac_addr);
> -int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
> - u32 pba_num_size);
> +void i40e_get_pba_string(struct i40e_hw *hw);
> void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable);
> /* prototype for functions used for NVM access */
> int i40e_init_nvm(struct i40e_hw *hw);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
> index dc7cd16ad8fb..aff6dc6afbe2 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_type.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
> @@ -493,6 +493,9 @@ struct i40e_hw {
> struct i40e_nvm_info nvm;
> struct i40e_fc_info fc;
>
> + /* PBA ID */
> + const char *pba_id;
> +
> /* pci info */
> u16 device_id;
> u16 vendor_id;
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 <[email protected]>
>> ---
>> .../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,
>> };
>>
>> /**
>