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

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.

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

+	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?

+	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?

+	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.

+	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.

+	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 

+	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.

+	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

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