[PATCH net-next 0/5] i40e: Add basic devlink support

[PATCH net-next 0/5] i40e: Add basic devlink support

[Ivan Vecera]

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

[PATCH net-next 1/5] i40e: Add initial devlink support

[Ivan Vecera]

Add an initial support for devlink interface to i40e driver.

Similarly to ice driver the implementation doe not enable devlink
to manage device-wide configuration and devlink instance is created
for each physical function of PCIe device.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/net/ethernet/intel/Kconfig            |   1 +
 drivers/net/ethernet/intel/i40e/Makefile      |   3 +-
 drivers/net/ethernet/intel/i40e/i40e.h        |   3 +
 .../net/ethernet/intel/i40e/i40e_devlink.c    | 118 ++++++++++++++++++
 .../net/ethernet/intel/i40e/i40e_devlink.h    |  18 +++
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  27 +++-
 6 files changed, 164 insertions(+), 6 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/i40e/i40e_devlink.c
 create mode 100644 drivers/net/ethernet/intel/i40e/i40e_devlink.h

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index e6684f3cc0ce..06ddd7147c7f 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -225,6 +225,7 @@ config I40E
 	depends on PTP_1588_CLOCK_OPTIONAL
 	depends on PCI
 	select AUXILIARY_BUS
+	select NET_DEVLINK
 	help
 	  This driver supports Intel(R) Ethernet Controller XL710 Family of
 	  devices.  For more information on how to identify your adapter, go
diff --git a/drivers/net/ethernet/intel/i40e/Makefile b/drivers/net/ethernet/intel/i40e/Makefile
index 2f21b3e89fd0..cad93f323bd5 100644
--- a/drivers/net/ethernet/intel/i40e/Makefile
+++ b/drivers/net/ethernet/intel/i40e/Makefile
@@ -24,6 +24,7 @@ i40e-objs := i40e_main.o \
 	i40e_ddp.o \
 	i40e_client.o   \
 	i40e_virtchnl_pf.o \
-	i40e_xsk.o
+	i40e_xsk.o	\
+	i40e_devlink.o
 
 i40e-$(CONFIG_I40E_DCB) += i40e_dcb.o i40e_dcb_nl.o
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 214744de120d..b7e20cea19c2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -9,10 +9,12 @@
 #include <linux/types.h>
 #include <linux/avf/virtchnl.h>
 #include <linux/net/intel/i40e_client.h>
+#include <net/devlink.h>
 #include <net/pkt_cls.h>
 #include <net/udp_tunnel.h>
 #include "i40e_dcb.h"
 #include "i40e_debug.h"
+#include "i40e_devlink.h"
 #include "i40e_io.h"
 #include "i40e_prototype.h"
 #include "i40e_register.h"
@@ -411,6 +413,7 @@ static inline const u8 *i40e_channel_mac(struct i40e_channel *ch)
 /* struct that defines the Ethernet device */
 struct i40e_pf {
 	struct pci_dev *pdev;
+	struct devlink_port devlink_port;
 	struct i40e_hw hw;
 	DECLARE_BITMAP(state, __I40E_STATE_SIZE__);
 	struct msix_entry *msix_entries;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
new file mode 100644
index 000000000000..66b7f5be45ae
--- /dev/null
+++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2023 Intel Corporation. */
+
+#include <net/devlink.h>
+#include "i40e.h"
+#include "i40e_devlink.h"
+
+static const struct devlink_ops i40e_devlink_ops = {
+};
+
+/**
+ * i40e_alloc_pf - Allocate devlink and return i40e_pf structure pointer
+ * @dev: the device to allocate for
+ *
+ * Allocate a devlink instance for this device and return the private
+ * area as the i40e_pf structure.
+ **/
+struct i40e_pf *i40e_alloc_pf(struct device *dev)
+{
+	struct devlink *devlink;
+
+	devlink = devlink_alloc(&i40e_devlink_ops, sizeof(struct i40e_pf), dev);
+	if (!devlink)
+		return NULL;
+
+	return devlink_priv(devlink);
+}
+
+/**
+ * i40e_free_pf - Free i40e_pf structure and associated devlink
+ * @pf: the PF structure
+ *
+ * Free i40e_pf structure and devlink allocated by devlink_alloc.
+ **/
+void i40e_free_pf(struct i40e_pf *pf)
+{
+	struct devlink *devlink = priv_to_devlink(pf);
+
+	devlink_free(devlink);
+}
+
+/**
+ * i40e_devlink_register - Register devlink interface for this PF
+ * @pf: the PF to register the devlink for.
+ *
+ * Register the devlink instance associated with this physical function.
+ **/
+void i40e_devlink_register(struct i40e_pf *pf)
+{
+	devlink_register(priv_to_devlink(pf));
+}
+
+/**
+ * i40e_devlink_unregister - Unregister devlink resources for this PF.
+ * @pf: the PF structure to cleanup
+ *
+ * Releases resources used by devlink and cleans up associated memory.
+ **/
+void i40e_devlink_unregister(struct i40e_pf *pf)
+{
+	devlink_unregister(priv_to_devlink(pf));
+}
+
+/**
+ * i40e_devlink_set_switch_id - Set unique switch id based on pci dsn
+ * @pf: the PF to create a devlink port for
+ * @ppid: struct with switch id information
+ */
+static void i40e_devlink_set_switch_id(struct i40e_pf *pf,
+				       struct netdev_phys_item_id *ppid)
+{
+	u64 id = pci_get_dsn(pf->pdev);
+
+	ppid->id_len = sizeof(id);
+	put_unaligned_be64(id, &ppid->id);
+}
+
+/**
+ * i40e_devlink_create_port - Create a devlink port for this PF
+ * @pf: the PF to create a port for
+ *
+ * Create and register a devlink_port for this PF. Note that although each
+ * physical function is connected to a separate devlink instance, the port
+ * will still be numbered according to the physical function id.
+ *
+ * Return: zero on success or an error code on failure.
+ **/
+int i40e_devlink_create_port(struct i40e_pf *pf)
+{
+	struct devlink *devlink = priv_to_devlink(pf);
+	struct devlink_port_attrs attrs = {};
+	struct device *dev = &pf->pdev->dev;
+	int err;
+
+	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+	attrs.phys.port_number = pf->hw.pf_id;
+	i40e_devlink_set_switch_id(pf, &attrs.switch_id);
+	devlink_port_attrs_set(&pf->devlink_port, &attrs);
+	err = devlink_port_register(devlink, &pf->devlink_port, pf->hw.pf_id);
+	if (err) {
+		dev_err(dev, "devlink_port_register failed: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
+ * i40e_devlink_destroy_port - Destroy the devlink_port for this PF
+ * @pf: the PF to cleanup
+ *
+ * Unregisters the devlink_port structure associated with this PF.
+ **/
+void i40e_devlink_destroy_port(struct i40e_pf *pf)
+{
+	devlink_port_type_clear(&pf->devlink_port);
+	devlink_port_unregister(&pf->devlink_port);
+}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.h b/drivers/net/ethernet/intel/i40e/i40e_devlink.h
new file mode 100644
index 000000000000..469fb3d2ee25
--- /dev/null
+++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2023, Intel Corporation. */
+
+#ifndef _I40E_DEVLINK_H_
+#define _I40E_DEVLINK_H_
+
+#include <linux/device.h>
+
+struct i40e_pf;
+
+struct i40e_pf *i40e_alloc_pf(struct device *dev);
+void i40e_free_pf(struct i40e_pf *pf);
+void i40e_devlink_register(struct i40e_pf *pf);
+void i40e_devlink_unregister(struct i40e_pf *pf);
+int i40e_devlink_create_port(struct i40e_pf *pf);
+void i40e_devlink_destroy_port(struct i40e_pf *pf);
+
+#endif /* _I40E_DEVLINK_H_ */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 1e52e1debf7c..f0e563a7f7b3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -14211,6 +14211,8 @@ int i40e_vsi_release(struct i40e_vsi *vsi)
 	}
 	set_bit(__I40E_VSI_RELEASING, vsi->state);
 	uplink_seid = vsi->uplink_seid;
+	if (vsi->type == I40E_VSI_MAIN)
+		i40e_devlink_destroy_port(pf);
 	if (vsi->type != I40E_VSI_SRIOV) {
 		if (vsi->netdev_registered) {
 			vsi->netdev_registered = false;
@@ -14398,6 +14400,8 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
 
 err_rings:
 	i40e_vsi_free_q_vectors(vsi);
+	if (vsi->type == I40E_VSI_MAIN)
+		i40e_devlink_destroy_port(pf);
 	if (vsi->netdev_registered) {
 		vsi->netdev_registered = false;
 		unregister_netdev(vsi->netdev);
@@ -14544,9 +14548,15 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
 		ret = i40e_netif_set_realnum_tx_rx_queues(vsi);
 		if (ret)
 			goto err_netdev;
+		if (vsi->type == I40E_VSI_MAIN) {
+			ret = i40e_devlink_create_port(pf);
+			if (ret)
+				goto err_netdev;
+			SET_NETDEV_DEVLINK_PORT(vsi->netdev, &pf->devlink_port);
+		}
 		ret = register_netdev(vsi->netdev);
 		if (ret)
-			goto err_netdev;
+			goto err_dl_port;
 		vsi->netdev_registered = true;
 		netif_carrier_off(vsi->netdev);
 #ifdef CONFIG_I40E_DCB
@@ -14589,6 +14599,9 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
 		free_netdev(vsi->netdev);
 		vsi->netdev = NULL;
 	}
+err_dl_port:
+	if (vsi->type == I40E_VSI_MAIN)
+		i40e_devlink_destroy_port(pf);
 err_netdev:
 	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
 err_vsi:
@@ -15619,7 +15632,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw)
 	iounmap(hw->hw_addr);
 	pci_release_mem_regions(pf->pdev);
 	pci_disable_device(pf->pdev);
-	kfree(pf);
+	i40e_free_pf(pf);
 
 	return err;
 }
@@ -15696,7 +15709,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * the Admin Queue structures and then querying for the
 	 * device's current profile information.
 	 */
-	pf = kzalloc(sizeof(*pf), GFP_KERNEL);
+	pf = i40e_alloc_pf(&pdev->dev);
 	if (!pf) {
 		err = -ENOMEM;
 		goto err_pf_alloc;
@@ -16223,6 +16236,8 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* print a string summarizing features */
 	i40e_print_features(pf);
 
+	i40e_devlink_register(pf);
+
 	return 0;
 
 	/* Unwind what we've done if something failed in the setup */
@@ -16243,7 +16258,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_pf_reset:
 	iounmap(hw->hw_addr);
 err_ioremap:
-	kfree(pf);
+	i40e_free_pf(pf);
 err_pf_alloc:
 	pci_release_mem_regions(pdev);
 err_pci_reg:
@@ -16268,6 +16283,8 @@ static void i40e_remove(struct pci_dev *pdev)
 	int ret_code;
 	int i;
 
+	i40e_devlink_unregister(pf);
+
 	i40e_dbg_pf_exit(pf);
 
 	i40e_ptp_stop(pf);
@@ -16393,7 +16410,7 @@ static void i40e_remove(struct pci_dev *pdev)
 	kfree(pf->vsi);
 
 	iounmap(hw->hw_addr);
-	kfree(pf);
+	i40e_free_pf(pf);
 	pci_release_mem_regions(pdev);
 
 	pci_disable_device(pdev);
-- 
2.41.0

[PATCH net-next 2/5] i40e: Split and refactor i40e_nvm_version_str()

[Ivan Vecera]

The function formats NVM version string according adapter's
EETrackID value. If this value OEM specific (0xffffffff) then
the reported version is with format:
"<gen>.<snap>.<release>"
and in other case
"<nvm_maj>.<nvm_min> <eetrackid> <cvid_maj>.<cvid_bld>.<cvid_min>"

These versions are reported in the subsequent patch in this series
that implements devlink .info_get but separately.

So split the function into separate ones, refactor it to use them
and remove ugly static string buffer.
Additionally convert NVM/OEM version mask macros to use GENMASK and
use FIELD_GET/FIELD_PREP for them in i40e_nvm_version_str() and
i40e_get_oem_version(). This makes code more readable and allows
us to remove related shift macros.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h        | 133 +++++++++++++-----
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |   4 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  12 +-
 3 files changed, 105 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index b7e20cea19c2..c4cd54aa4dd5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -49,23 +49,19 @@
 #define I40E_QUEUE_WAIT_RETRY_LIMIT	10
 #define I40E_INT_NAME_STR_LEN		(IFNAMSIZ + 16)
 
-#define I40E_NVM_VERSION_LO_SHIFT	0
-#define I40E_NVM_VERSION_LO_MASK	(0xff << I40E_NVM_VERSION_LO_SHIFT)
-#define I40E_NVM_VERSION_HI_SHIFT	12
-#define I40E_NVM_VERSION_HI_MASK	(0xf << I40E_NVM_VERSION_HI_SHIFT)
-#define I40E_OEM_VER_BUILD_MASK		0xffff
-#define I40E_OEM_VER_PATCH_MASK		0xff
-#define I40E_OEM_VER_BUILD_SHIFT	8
-#define I40E_OEM_VER_SHIFT		24
 #define I40E_PHY_DEBUG_ALL \
 	(I40E_AQ_PHY_DEBUG_DISABLE_LINK_FW | \
 	I40E_AQ_PHY_DEBUG_DISABLE_ALL_LINK_FW)
 
 #define I40E_OEM_EETRACK_ID		0xffffffff
-#define I40E_OEM_GEN_SHIFT		24
-#define I40E_OEM_SNAP_MASK		0x00ff0000
-#define I40E_OEM_SNAP_SHIFT		16
-#define I40E_OEM_RELEASE_MASK		0x0000ffff
+#define I40E_NVM_VERSION_LO_MASK	GENMASK(7, 0)
+#define I40E_NVM_VERSION_HI_MASK	GENMASK(15, 12)
+#define I40E_OEM_VER_BUILD_MASK		GENMASK(23, 8)
+#define I40E_OEM_VER_PATCH_MASK		GENMASK(7, 0)
+#define I40E_OEM_VER_MASK		GENMASK(31, 24)
+#define I40E_OEM_GEN_MASK		GENMASK(31, 24)
+#define I40E_OEM_SNAP_MASK		GENMASK(23, 16)
+#define I40E_OEM_RELEASE_MASK		GENMASK(15, 0)
 
 #define I40E_RX_DESC(R, i)	\
 	(&(((union i40e_rx_desc *)((R)->desc))[i]))
@@ -954,43 +950,104 @@ struct i40e_device {
 };
 
 /**
- * i40e_nvm_version_str - format the NVM version strings
+ * i40e_info_nvm_ver - format the NVM version string
  * @hw: ptr to the hardware info
+ * @buf: string buffer to store
+ * @len: buffer size
+ *
+ * Formats NVM version string as:
+ * <gen>.<snap>.<release> when eetrackid == I40E_OEM_EETRACK_ID
+ * <nvm_major>.<nvm_minor> otherwise
  **/
-static inline char *i40e_nvm_version_str(struct i40e_hw *hw)
+static inline void i40e_info_nvm_ver(struct i40e_hw *hw, char *buf, size_t len)
 {
-	static char buf[32];
-	u32 full_ver;
+	struct i40e_nvm_info *nvm = &hw->nvm;
 
-	full_ver = hw->nvm.oem_ver;
-
-	if (hw->nvm.eetrack == I40E_OEM_EETRACK_ID) {
+	if (nvm->eetrack == I40E_OEM_EETRACK_ID) {
+		u32 full_ver = nvm->oem_ver;
 		u8 gen, snap;
 		u16 release;
 
-		gen = (u8)(full_ver >> I40E_OEM_GEN_SHIFT);
-		snap = (u8)((full_ver & I40E_OEM_SNAP_MASK) >>
-			I40E_OEM_SNAP_SHIFT);
-		release = (u16)(full_ver & I40E_OEM_RELEASE_MASK);
-
-		snprintf(buf, sizeof(buf), "%x.%x.%x", gen, snap, release);
+		gen = FIELD_GET(I40E_OEM_GEN_MASK, full_ver);
+		snap = FIELD_GET(I40E_OEM_SNAP_MASK, full_ver);
+		release = FIELD_GET(I40E_OEM_RELEASE_MASK, full_ver);
+		snprintf(buf, len, "%x.%x.%x", gen, snap, release);
 	} else {
-		u8 ver, patch;
+		u8 major, minor;
+
+		major = FIELD_GET(I40E_NVM_VERSION_HI_MASK, nvm->version);
+		minor = FIELD_GET(I40E_NVM_VERSION_LO_MASK, nvm->version);
+		snprintf(buf, len, "%x.%02x", major, minor);
+	}
+}
+
+/**
+ * i40e_info_eetrack - format the EETrackID string
+ * @hw: ptr to the hardware info
+ * @buf: string buffer to store
+ * @len: buffer size
+ *
+ * Returns hexadecimally formated EETrackID if it is
+ * different from I40E_OEM_EETRACK_ID or empty string.
+ **/
+static inline void i40e_info_eetrack(struct i40e_hw *hw, char *buf, size_t len)
+{
+	struct i40e_nvm_info *nvm = &hw->nvm;
+
+	buf[0] = '\0';
+	if (nvm->eetrack != I40E_OEM_EETRACK_ID)
+		snprintf(buf, len, "0x%08x", nvm->eetrack);
+}
+
+/**
+ * i40e_info_civd_ver - format the NVM version strings
+ * @hw: ptr to the hardware info
+ * @buf: string buffer to store
+ * @len: buffer size
+ *
+ * Returns formated combo image version if adapter's EETrackID is
+ * different from I40E_OEM_EETRACK_ID or empty string.
+ **/
+static inline void i40e_info_civd_ver(struct i40e_hw *hw, char *buf, size_t len)
+{
+	struct i40e_nvm_info *nvm = &hw->nvm;
+
+	buf[0] = '\0';
+	if (nvm->eetrack != I40E_OEM_EETRACK_ID) {
+		u32 full_ver = nvm->oem_ver;
+		u8 major, minor;
 		u16 build;
 
-		ver = (u8)(full_ver >> I40E_OEM_VER_SHIFT);
-		build = (u16)((full_ver >> I40E_OEM_VER_BUILD_SHIFT) &
-			 I40E_OEM_VER_BUILD_MASK);
-		patch = (u8)(full_ver & I40E_OEM_VER_PATCH_MASK);
-
-		snprintf(buf, sizeof(buf),
-			 "%x.%02x 0x%x %d.%d.%d",
-			 (hw->nvm.version & I40E_NVM_VERSION_HI_MASK) >>
-				I40E_NVM_VERSION_HI_SHIFT,
-			 (hw->nvm.version & I40E_NVM_VERSION_LO_MASK) >>
-				I40E_NVM_VERSION_LO_SHIFT,
-			 hw->nvm.eetrack, ver, build, patch);
+		major = FIELD_GET(I40E_OEM_VER_MASK, full_ver);
+		build = FIELD_GET(I40E_OEM_VER_BUILD_MASK, full_ver);
+		minor = FIELD_GET(I40E_OEM_VER_PATCH_MASK, full_ver);
+		snprintf(buf, len, "%d.%d.%d", major, build, minor);
 	}
+}
+
+/**
+ * i40e_nvm_version_str - format the NVM version strings
+ * @hw: ptr to the hardware info
+ * @buf: string buffer to store
+ * @len: buffer size
+ **/
+static inline char *i40e_nvm_version_str(struct i40e_hw *hw, char *buf,
+					 size_t len)
+{
+	char ver[16] = " ";
+
+	/* Get NVM version */
+	i40e_info_nvm_ver(hw, buf, len);
+
+	/* Append EETrackID if provided */
+	i40e_info_eetrack(hw, &ver[1], sizeof(ver) - 1);
+	if (strlen(ver) > 1)
+		strlcat(buf, ver, len);
+
+	/* Append combo image version if provided */
+	i40e_info_civd_ver(hw, &ver[1], sizeof(ver) - 1);
+	if (strlen(ver) > 1)
+		strlcat(buf, ver, len);
 
 	return buf;
 }
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index a89f7ca510fd..ebf36f76c0d7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2006,8 +2006,8 @@ static void i40e_get_drvinfo(struct net_device *netdev,
 	struct i40e_pf *pf = vsi->back;
 
 	strscpy(drvinfo->driver, i40e_driver_name, sizeof(drvinfo->driver));
-	strscpy(drvinfo->fw_version, i40e_nvm_version_str(&pf->hw),
-		sizeof(drvinfo->fw_version));
+	i40e_nvm_version_str(&pf->hw, drvinfo->fw_version,
+			     sizeof(drvinfo->fw_version));
 	strscpy(drvinfo->bus_info, pci_name(pf->pdev),
 		sizeof(drvinfo->bus_info));
 	drvinfo->n_priv_flags = I40E_PRIV_FLAGS_STR_LEN;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index f0e563a7f7b3..ba8fb0556216 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10798,7 +10798,9 @@ static void i40e_get_oem_version(struct i40e_hw *hw)
 			   &gen_snap);
 	i40e_read_nvm_word(hw, block_offset + I40E_NVM_OEM_RELEASE_OFFSET,
 			   &release);
-	hw->nvm.oem_ver = (gen_snap << I40E_OEM_SNAP_SHIFT) | release;
+	hw->nvm.oem_ver =
+		FIELD_PREP(I40E_OEM_GEN_MASK | I40E_OEM_SNAP_MASK, gen_snap) |
+		FIELD_PREP(I40E_OEM_RELEASE_MASK, release);
 	hw->nvm.eetrack = I40E_OEM_EETRACK_ID;
 }
 
@@ -15674,6 +15676,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct i40e_hw *hw;
 	static u16 pfs_found;
 	u16 wol_nvm_bits;
+	char nvm_ver[32];
 	u16 link_status;
 #ifdef CONFIG_I40E_DCB
 	int status;
@@ -15845,11 +15848,12 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	i40e_get_oem_version(hw);
 
 	/* provide nvm, fw, api versions, vendor:device id, subsys vendor:device id */
+	i40e_nvm_version_str(hw, nvm_ver, sizeof(nvm_ver));
 	dev_info(&pdev->dev, "fw %d.%d.%05d api %d.%d nvm %s [%04x:%04x] [%04x:%04x]\n",
 		 hw->aq.fw_maj_ver, hw->aq.fw_min_ver, hw->aq.fw_build,
-		 hw->aq.api_maj_ver, hw->aq.api_min_ver,
-		 i40e_nvm_version_str(hw), hw->vendor_id, hw->device_id,
-		 hw->subsystem_vendor_id, hw->subsystem_device_id);
+		 hw->aq.api_maj_ver, hw->aq.api_min_ver, nvm_ver,
+		 hw->vendor_id, hw->device_id, hw->subsystem_vendor_id,
+		 hw->subsystem_device_id);
 
 	if (hw->aq.api_maj_ver == I40E_FW_API_VERSION_MAJOR &&
 	    hw->aq.api_min_ver > I40E_FW_MINOR_VERSION(hw))
-- 
2.41.0

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

[Ivan Vecera]

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(+)

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

[PATCH net-next 4/5] i40e: Refactor and rename i40e_read_pba_string()

[Ivan Vecera]

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 <ivecera@redhat.com>
---
 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

[PATCH net-next 5/5] i40e: Add PBA as board id info to devlink .info_get

[Ivan Vecera]

Expose stored PBA ID string as unique board identifier via
devlink's .info_get command.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 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

Re: [PATCH net-next 0/5] i40e: Add basic devlink support

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

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

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

[Jakub Kicinski]

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?

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

[Ivan Vecera]

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

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

[Jakub Kicinski]

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!

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

[Jacob Keller]

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

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

[Jacob Keller]

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

Re: [PATCH net-next 4/5] i40e: Refactor and rename i40e_read_pba_string()

[Jacob Keller]

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 <jacob.e.keller@intel.com>

> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  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;

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

[Ivan Vecera]

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