On 1/31/2020 10:07 AM, Jakub Kicinski wrote:

On Thu, 30 Jan 2020 14:59:05 -0800, Jacob Keller wrote:

quoted

The devlink .info_get callback allows the driver to report detailed
version information. The following devlink versions are reported with
this initial implementation:

 "driver.version" -> device driver version, to match ethtool -i version
 "fw" -> firmware version as reported by ethtool -i firmware-version
 "fw.mgmt" -> The version of the firmware that controls PHY, link, etc
 "fw.api" -> API version of interface exposed over the AdminQ
 "fw.build" -> Unique build identifier for the management firmware
 "nvm.version" -> Version of the NVM parameters section
 "nvm.oem" -> OEM-specific version information
 "nvm.eetrack" -> Unique identifier for the combined NVM image

These all need documentation.

There's a patch at the end of the series, but it should probably just be
squashed in here.

quoted

With this, devlink can now report at least the same information as
reported by the older ethtool interface. Each section of the
"firmware-version" is also reported independently so that it is easier
to understand the meaning.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 103 +++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 0b0f936122de..493c2c2986f2 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c

@@ -2,9 +2,112 @@
 /* Copyright (c) 2019, Intel Corporation. */
 
 #include "ice.h"
+#include "ice_lib.h"
 #include "ice_devlink.h"
 
+/**
+ * ice_devlink_info_get - .info_get devlink handler
+ * @devlink: devlink instance structure
+ * @req: the devlink info request
+ * @extack: extended netdev ack structure
+ *
+ * Callback for the devlink .info_get operation. Reports information about the
+ * device.
+ *
+ * @returns zero on success or an error code on failure.
+ */
+static int ice_devlink_info_get(struct devlink *devlink,
+				struct devlink_info_req *req,
+				struct netlink_ext_ack *extack)
+{
+	u8 oem_ver, oem_patch, nvm_ver_hi, nvm_ver_lo;
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct ice_hw *hw = &pf->hw;
+	u16 oem_build;
+	char buf[32]; /* TODO: size this properly */
+	int err;
+
+	ice_get_nvm_version(hw, &oem_ver, &oem_build, &oem_patch, &nvm_ver_hi,
+			    &nvm_ver_lo);
+
+	err = devlink_info_driver_name_put(req, KBUILD_MODNAME);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set driver name");
+		return err;
+	}
+
+	/* driver.version */
+	err = devlink_info_version_fixed_put(req, "driver.version",
+					     ice_drv_ver);

Hard no. You should really try to follow the discussions on netdev.

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set driver version");
+		return err;
+	}
+
+	/* fw (match exact output of ethtool -i firmware-version) */

That's generally a bad idea, the whole point of info was that people
were stuffing multiple things into ethtool -i fw. Is this only one item
referring to one single entity?

Right. I can just remove this entirely.

quoted

+	err = devlink_info_version_running_put(req,
+					       DEVLINK_INFO_VERSION_GENERIC_FW,
+					       ice_nvm_version_str(hw));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set combined fw version");
+		return err;
+	}
+
+	/* fw.mgmt (DEVLINK_INFO_VERSION_GENERIC_FW_MGMT) */
+	snprintf(buf, sizeof(buf), "%u.%u.%u", hw->fw_maj_ver, hw->fw_min_ver,
+		 hw->fw_patch);
+	err = devlink_info_version_running_put(req, "fw.mgmt", buf);

why not use the define?

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set fw version data");
+		return err;
+	}
+
+	/* fw.api */
+	snprintf(buf, sizeof(buf), "%u.%u", hw->api_maj_ver,
+		 hw->api_min_ver);
+	err = devlink_info_version_running_put(req, "fw.api", buf);

Is this the API version of the management FW? I'd go for "fw.mgmt.api".
Datapath, RoCE and other bits may have APIs which evolve independently
for complex chips.

I'm not 100% sure, but probably. Will check and update.

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set fw API data");
+		return err;
+	}
+
+	/* fw.build */
+	snprintf(buf, sizeof(buf), "%08x", hw->fw_build);
+	err = devlink_info_version_running_put(req, "fw.build", buf);

Huh? Why is this not part of the version?

Maybe you want to use fw.bundle? Naming is hard, at Netronome added
that as a unique identifier for the FW in its entirety / the entire
build as it is passed from Eng to QA and released externally.

fw.bundle is probably more appropriate, yes.

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set fw build data");
+		return err;
+	}
+
+	/* nvm.version */
+	snprintf(buf, sizeof(buf), "%x.%02x", nvm_ver_hi, nvm_ver_lo);
+	err = devlink_info_version_running_put(req, "nvm.version", buf);

Please us the psid

Ok.

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set NVM version data");
+		return err;
+	}
+
+	/* nvm.oem */
+	snprintf(buf, sizeof(buf), "%u.%u.%u", oem_ver, oem_build, oem_patch);
+	err = devlink_info_version_running_put(req, "nvm.oem", buf);

This looks like free form catch all. Let's not.

I'm not actually sure what these represent either. Will try to figure
that out and update.

quoted

+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set oem version data");
+		return err;
+	}
+
+	/* nvm.eetrack */
+	snprintf(buf, sizeof(buf), "0x%0X", hw->nvm.eetrack);

Mm. maybe this is bundle? Or psid. Hm. Please explain what this is and
what it's supposed to be used for. I should probably add more docs to
the existing items :S

It's probably closer to a bundle or psid. Will fix.

quoted

+	err = devlink_info_version_running_put(req, "nvm.eetrack", buf);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set NVM eetrack data");
+		return err;
+	}
+
+	return 0;
+}
+
 const struct devlink_ops ice_devlink_ops = {
+	.info_get = ice_devlink_info_get,
 };
 
 /**