[PATCH net-next 00/15][pull request] 100GbE Intel Wired LAN Driver Updates 2021-01-28

[PATCH net-next 00/15][pull request] 100GbE Intel Wired LAN Driver Updates 2021-01-28

[Tony Nguyen]

This series contains updates to ice driver only.

Jake adds devlink reporting of security revision fields associated with
'fw.undi' and 'fw.mgmt'. Also implements support for displaying and
updating the minimum security revision fields for the device as
driver-specific devlink parameters. And adds reporting of timeout length
during devlink flash.

He also implements support to report devlink info regarding the version of
firmware that is stored (downloaded) to the device, but is not yet active.
This includes the UNDI Option ROM, the Netlist module, and the
fw.bundle_id.

Changes include:
   Refactoring version reporting to allow for a context structure.

   ice_read_flash_module is further abstracted to think in terms of
   "active" and "inactive" banks, rather than focusing on "read from
   the 1st or 2nd bank". Further, the function is extended to allow
   reading arbitrary sizes beyond just one word at a time.

   Extend the version function to allow requesting the flash bank to read
   from (active or inactive).

Gustavo A. R. Silva replaces a one-element array to flexible-array
member.

Bruce utilizes flex_array_size() helper and removes dead code on a check
for a condition that can't occur.

The following are changes since commit 32e31b78272ba0905c751a0f6ff6ab4c275a780e:
  Merge branch 'net-sfp-add-support-for-gpon-rtl8672-rtl9601c-and-ubiquiti-u-fiber'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE

Bruce Allan (2):
  ice: use flex_array_size where possible
  ice: remove dead code

Gustavo A. R. Silva (1):
  ice: Replace one-element array with flexible-array member

Jacob Keller (12):
  ice: create flash_info structure and separate NVM version
  ice: cache NVM module bank information
  ice: read security revision to ice_nvm_info and ice_orom_info
  ice: add devlink parameters to read and write minimum security
    revision
  ice: report timeout length for erasing during devlink flash
  ice: introduce context struct for info report
  ice: refactor interface for ice_read_flash_module
  ice: allow reading inactive flash security revision
  ice: allow reading arbitrary size data with read_flash_module
  ice: display some stored NVM versions via devlink info
  ice: display stored netlist versions via devlink info
  ice: display stored UNDI firmware version via devlink info

 Documentation/networking/devlink/ice.rst      |  43 +
 drivers/net/ethernet/intel/ice/ice.h          |   2 +-
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  40 +-
 drivers/net/ethernet/intel/ice/ice_common.c   |   2 +-
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 496 +++++++++-
 drivers/net/ethernet/intel/ice/ice_devlink.h  |   2 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |   8 +-
 .../net/ethernet/intel/ice/ice_flex_pipe.c    |   2 +-
 .../net/ethernet/intel/ice/ice_fw_update.c    |  10 +-
 drivers/net/ethernet/intel/ice/ice_main.c     |  19 +-
 drivers/net/ethernet/intel/ice/ice_nvm.c      | 876 +++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_nvm.h      |  18 +
 drivers/net/ethernet/intel/ice/ice_status.h   |   1 +
 drivers/net/ethernet/intel/ice/ice_type.h     | 141 ++-
 14 files changed, 1427 insertions(+), 233 deletions(-)

-- 
2.26.2

[PATCH net-next 01/15] ice: create flash_info structure and separate NVM version

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice_nvm_info structure has become somewhat of a dumping ground for
all of the fields related to flash version. It holds the NVM version and
EETRACK id, the OptionROM info structure, the flash size, the ShadowRAM
size, and more.

A future change is going to add the ability to read the NVM version and
EETRACK ID from the inactive NVM bank. To make this simpler, it is
useful to have these NVM version info fields extracted to their own
structure.

Rename ice_nvm_info into ice_flash_info, and create a separate
ice_nvm_info structure that will contain the eetrack and NVM map
version. Move the netlist_ver structure into ice_flash_info and rename it
ice_netlist_info for consistency.

Modify the static ice_get_orom_ver_info to take the option rom structure
as a pointer. This makes it more obvious what portion of the hw struct
is being modified. Do the same for ice_get_netlist_ver_info.

Introduce a new ice_get_nvm_ver_info function, which will be similar to
ice_get_orom_ver_info and ice_get_netlist_ver_info, used to keep the NVM
version extraction code co-located.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 16 ++--
 drivers/net/ethernet/intel/ice/ice_ethtool.c |  8 +-
 drivers/net/ethernet/intel/ice/ice_nvm.c     | 92 ++++++++++++--------
 drivers/net/ethernet/intel/ice/ice_type.h    | 37 ++++----
 4 files changed, 91 insertions(+), 62 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 29d6192b15f3..44b64524b1b8 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -58,7 +58,7 @@ static int ice_info_fw_build(struct ice_pf *pf, char *buf, size_t len)
 
 static int ice_info_orom_ver(struct ice_pf *pf, char *buf, size_t len)
 {
-	struct ice_orom_info *orom = &pf->hw.nvm.orom;
+	struct ice_orom_info *orom = &pf->hw.flash.orom;
 
 	snprintf(buf, len, "%u.%u.%u", orom->major, orom->build, orom->patch);
 
@@ -67,16 +67,16 @@ static int ice_info_orom_ver(struct ice_pf *pf, char *buf, size_t len)
 
 static int ice_info_nvm_ver(struct ice_pf *pf, char *buf, size_t len)
 {
-	struct ice_nvm_info *nvm = &pf->hw.nvm;
+	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
 
-	snprintf(buf, len, "%x.%02x", nvm->major_ver, nvm->minor_ver);
+	snprintf(buf, len, "%x.%02x", nvm->major, nvm->minor);
 
 	return 0;
 }
 
 static int ice_info_eetrack(struct ice_pf *pf, char *buf, size_t len)
 {
-	struct ice_nvm_info *nvm = &pf->hw.nvm;
+	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
 
 	snprintf(buf, len, "0x%08x", nvm->eetrack);
 
@@ -111,7 +111,7 @@ static int ice_info_ddp_pkg_bundle_id(struct ice_pf *pf, char *buf, size_t len)
 
 static int ice_info_netlist_ver(struct ice_pf *pf, char *buf, size_t len)
 {
-	struct ice_netlist_ver_info *netlist = &pf->hw.netlist_ver;
+	struct ice_netlist_info *netlist = &pf->hw.flash.netlist;
 
 	/* The netlist version fields are BCD formatted */
 	snprintf(buf, len, "%x.%x.%x-%x.%x.%x", netlist->major, netlist->minor,
@@ -123,7 +123,7 @@ static int ice_info_netlist_ver(struct ice_pf *pf, char *buf, size_t len)
 
 static int ice_info_netlist_build(struct ice_pf *pf, char *buf, size_t len)
 {
-	struct ice_netlist_ver_info *netlist = &pf->hw.netlist_ver;
+	struct ice_netlist_info *netlist = &pf->hw.flash.netlist;
 
 	snprintf(buf, len, "0x%08x", netlist->hash);
 
@@ -433,7 +433,7 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 	void *nvm_data;
 	u32 nvm_size;
 
-	nvm_size = hw->nvm.flash_size;
+	nvm_size = hw->flash.flash_size;
 	nvm_data = vzalloc(nvm_size);
 	if (!nvm_data)
 		return -ENOMEM;
@@ -533,7 +533,7 @@ void ice_devlink_init_regions(struct ice_pf *pf)
 	struct device *dev = ice_pf_to_dev(pf);
 	u64 nvm_size;
 
-	nvm_size = pf->hw.nvm.flash_size;
+	nvm_size = pf->hw.flash.flash_size;
 	pf->nvm_region = devlink_region_create(devlink, &ice_nvm_region_ops, 1,
 					       nvm_size);
 	if (IS_ERR(pf->nvm_region)) {
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 9e8e9531cd87..78698e84fe40 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -179,8 +179,8 @@ ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
 	struct ice_orom_info *orom;
 	struct ice_nvm_info *nvm;
 
-	nvm = &hw->nvm;
-	orom = &nvm->orom;
+	nvm = &hw->flash.nvm;
+	orom = &hw->flash.orom;
 
 	strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
 
@@ -188,7 +188,7 @@ ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
 	 * determined) which contains more pertinent information.
 	 */
 	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
-		 "%x.%02x 0x%x %d.%d.%d", nvm->major_ver, nvm->minor_ver,
+		 "%x.%02x 0x%x %d.%d.%d", nvm->major, nvm->minor,
 		 nvm->eetrack, orom->major, orom->build, orom->patch);
 
 	strscpy(drvinfo->bus_info, pci_name(pf->pdev),
@@ -250,7 +250,7 @@ static int ice_get_eeprom_len(struct net_device *netdev)
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_pf *pf = np->vsi->back;
 
-	return (int)pf->hw.nvm.flash_size;
+	return (int)pf->hw.flash.flash_size;
 }
 
 static int
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index f729cd0c6224..b0f0b4fc266b 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -72,7 +72,7 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
 	*length = 0;
 
 	/* Verify the length of the read if this is for the Shadow RAM */
-	if (read_shadow_ram && ((offset + inlen) > (hw->nvm.sr_words * 2u))) {
+	if (read_shadow_ram && ((offset + inlen) > (hw->flash.sr_words * 2u))) {
 		ice_debug(hw, ICE_DBG_NVM, "NVM error: requested offset is beyond Shadow RAM limit\n");
 		return ICE_ERR_PARAM;
 	}
@@ -213,7 +213,7 @@ ice_read_sr_word_aq(struct ice_hw *hw, u16 offset, u16 *data)
 enum ice_status
 ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access)
 {
-	if (hw->nvm.blank_nvm_mode)
+	if (hw->flash.blank_nvm_mode)
 		return 0;
 
 	return ice_acquire_res(hw, ICE_NVM_RES_ID, access, ICE_NVM_TIMEOUT);
@@ -227,7 +227,7 @@ ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access)
  */
 void ice_release_nvm(struct ice_hw *hw)
 {
-	if (hw->nvm.blank_nvm_mode)
+	if (hw->flash.blank_nvm_mode)
 		return;
 
 	ice_release_res(hw, ICE_NVM_RES_ID);
@@ -379,17 +379,56 @@ ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size)
 	return status;
 }
 
+/**
+ * ice_get_nvm_ver_info - Read NVM version information
+ * @hw: pointer to the HW struct
+ * @nvm: pointer to NVM info structure
+ *
+ * Read the NVM EETRACK ID and map version of the main NVM image bank, filling
+ * in the nvm info structure.
+ */
+static enum ice_status
+ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
+{
+	u16 eetrack_lo, eetrack_hi, ver;
+	enum ice_status status;
+
+	status = ice_read_sr_word(hw, ICE_SR_NVM_DEV_STARTER_VER, &ver);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read DEV starter version.\n");
+		return status;
+	}
+	nvm->major = (ver & ICE_NVM_VER_HI_MASK) >> ICE_NVM_VER_HI_SHIFT;
+	nvm->minor = (ver & ICE_NVM_VER_LO_MASK) >> ICE_NVM_VER_LO_SHIFT;
+
+	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_LO, &eetrack_lo);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read EETRACK lo.\n");
+		return status;
+	}
+	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_HI, &eetrack_hi);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read EETRACK hi.\n");
+		return status;
+	}
+
+	nvm->eetrack = (eetrack_hi << 16) | eetrack_lo;
+
+	return 0;
+}
+
 /**
  * ice_get_orom_ver_info - Read Option ROM version information
  * @hw: pointer to the HW struct
+ * @orom: pointer to Option ROM info structure
  *
  * Read the Combo Image version data from the Boot Configuration TLV and fill
  * in the option ROM version data.
  */
-static enum ice_status ice_get_orom_ver_info(struct ice_hw *hw)
+static enum ice_status
+ice_get_orom_ver_info(struct ice_hw *hw, struct ice_orom_info *orom)
 {
 	u16 combo_hi, combo_lo, boot_cfg_tlv, boot_cfg_tlv_len;
-	struct ice_orom_info *orom = &hw->nvm.orom;
 	enum ice_status status;
 	u32 combo_ver;
 
@@ -436,12 +475,13 @@ static enum ice_status ice_get_orom_ver_info(struct ice_hw *hw)
 /**
  * ice_get_netlist_ver_info
  * @hw: pointer to the HW struct
+ * @ver: pointer to netlist version info structure
  *
  * Get the netlist version information
  */
-static enum ice_status ice_get_netlist_ver_info(struct ice_hw *hw)
+static enum ice_status
+ice_get_netlist_ver_info(struct ice_hw *hw, struct ice_netlist_info *ver)
 {
-	struct ice_netlist_ver_info *ver = &hw->netlist_ver;
 	enum ice_status ret;
 	u32 id_blk_start;
 	__le16 raw_data;
@@ -555,7 +595,7 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
 
 	ice_debug(hw, ICE_DBG_NVM, "Predicted flash size is %u bytes\n", max_size);
 
-	hw->nvm.flash_size = max_size;
+	hw->flash.flash_size = max_size;
 
 err_read_flat_nvm:
 	ice_release_nvm(hw);
@@ -572,8 +612,7 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
  */
 enum ice_status ice_init_nvm(struct ice_hw *hw)
 {
-	struct ice_nvm_info *nvm = &hw->nvm;
-	u16 eetrack_lo, eetrack_hi, ver;
+	struct ice_flash_info *flash = &hw->flash;
 	enum ice_status status;
 	u32 fla, gens_stat;
 	u8 sr_size;
@@ -585,54 +624,39 @@ enum ice_status ice_init_nvm(struct ice_hw *hw)
 	sr_size = (gens_stat & GLNVM_GENS_SR_SIZE_M) >> GLNVM_GENS_SR_SIZE_S;
 
 	/* Switching to words (sr_size contains power of 2) */
-	nvm->sr_words = BIT(sr_size) * ICE_SR_WORDS_IN_1KB;
+	flash->sr_words = BIT(sr_size) * ICE_SR_WORDS_IN_1KB;
 
 	/* Check if we are in the normal or blank NVM programming mode */
 	fla = rd32(hw, GLNVM_FLA);
 	if (fla & GLNVM_FLA_LOCKED_M) { /* Normal programming mode */
-		nvm->blank_nvm_mode = false;
+		flash->blank_nvm_mode = false;
 	} else {
 		/* Blank programming mode */
-		nvm->blank_nvm_mode = true;
+		flash->blank_nvm_mode = true;
 		ice_debug(hw, ICE_DBG_NVM, "NVM init error: unsupported blank mode.\n");
 		return ICE_ERR_NVM_BLANK_MODE;
 	}
 
-	status = ice_read_sr_word(hw, ICE_SR_NVM_DEV_STARTER_VER, &ver);
-	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read DEV starter version.\n");
-		return status;
-	}
-	nvm->major_ver = (ver & ICE_NVM_VER_HI_MASK) >> ICE_NVM_VER_HI_SHIFT;
-	nvm->minor_ver = (ver & ICE_NVM_VER_LO_MASK) >> ICE_NVM_VER_LO_SHIFT;
-
-	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_LO, &eetrack_lo);
-	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read EETRACK lo.\n");
-		return status;
-	}
-	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_HI, &eetrack_hi);
+	status = ice_discover_flash_size(hw);
 	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read EETRACK hi.\n");
+		ice_debug(hw, ICE_DBG_NVM, "NVM init error: failed to discover flash size.\n");
 		return status;
 	}
 
-	nvm->eetrack = (eetrack_hi << 16) | eetrack_lo;
-
-	status = ice_discover_flash_size(hw);
+	status = ice_get_nvm_ver_info(hw, &flash->nvm);
 	if (status) {
-		ice_debug(hw, ICE_DBG_NVM, "NVM init error: failed to discover flash size.\n");
+		ice_debug(hw, ICE_DBG_INIT, "Failed to read NVM info.\n");
 		return status;
 	}
 
-	status = ice_get_orom_ver_info(hw);
+	status = ice_get_orom_ver_info(hw, &flash->orom);
 	if (status) {
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read Option ROM info.\n");
 		return status;
 	}
 
 	/* read the netlist version information */
-	status = ice_get_netlist_ver_info(hw);
+	status = ice_get_netlist_ver_info(hw, &flash->netlist);
 	if (status)
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read netlist info.\n");
 
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 2226a291a394..7af7758374d4 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -313,14 +313,30 @@ struct ice_orom_info {
 	u16 build;			/* Build version of OROM */
 };
 
-/* NVM Information */
+/* NVM version information */
 struct ice_nvm_info {
+	u32 eetrack;
+	u8 major;
+	u8 minor;
+};
+
+/* netlist version information */
+struct ice_netlist_info {
+	u32 major;			/* major high/low */
+	u32 minor;			/* minor high/low */
+	u32 type;			/* type high/low */
+	u32 rev;			/* revision high/low */
+	u32 hash;			/* SHA-1 hash word */
+	u16 cust_ver;			/* customer version */
+};
+
+/* Flash Chip Information */
+struct ice_flash_info {
 	struct ice_orom_info orom;	/* Option ROM version info */
-	u32 eetrack;			/* NVM data version */
+	struct ice_nvm_info nvm;	/* NVM version information */
+	struct ice_netlist_info netlist;/* Netlist version info */
 	u16 sr_words;			/* Shadow RAM size in words */
 	u32 flash_size;			/* Size of available flash in bytes */
-	u8 major_ver;			/* major version of NVM package */
-	u8 minor_ver;			/* minor version of dev starter */
 	u8 blank_nvm_mode;		/* is NVM empty (no FW present) */
 };
 
@@ -348,16 +364,6 @@ struct ice_link_default_override_tlv {
 
 #define ICE_NVM_VER_LEN	32
 
-/* netlist version information */
-struct ice_netlist_ver_info {
-	u32 major;			/* major high/low */
-	u32 minor;			/* minor high/low */
-	u32 type;			/* type high/low */
-	u32 rev;			/* revision high/low */
-	u32 hash;			/* SHA-1 hash word */
-	u16 cust_ver;			/* customer version */
-};
-
 /* Max number of port to queue branches w.r.t topology */
 #define ICE_MAX_TRAFFIC_CLASS 8
 #define ICE_TXSCHED_MAX_BRANCHES ICE_MAX_TRAFFIC_CLASS
@@ -605,10 +611,9 @@ struct ice_hw {
 	u8 evb_veb;		/* true for VEB, false for VEPA */
 	u8 reset_ongoing;	/* true if HW is in reset, false otherwise */
 	struct ice_bus_info bus;
-	struct ice_nvm_info nvm;
+	struct ice_flash_info flash;
 	struct ice_hw_dev_caps dev_caps;	/* device capabilities */
 	struct ice_hw_func_caps func_caps;	/* function capabilities */
-	struct ice_netlist_ver_info netlist_ver; /* netlist version info */
 
 	struct ice_switch_info *switch_info;	/* switch filter lists */
 
-- 
2.26.2

[PATCH net-next 02/15] ice: cache NVM module bank information

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice flash contains two copies of each of the NVM, Option ROM, and
Netlist modules. Each bank has a pointer word and a size word. In order
to correctly read from the active flash bank, the driver must calculate
the offset manually.

During NVM initialization, read the Shadow RAM control word and
determine which bank is active for each NVM module. Additionally, cache
the size and pointer values for use in calculating the correct offset.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_nvm.c  | 151 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_type.h |  37 ++++++
 2 files changed, 188 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index b0f0b4fc266b..308344045397 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -603,6 +603,151 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
 	return status;
 }
 
+/**
+ * ice_read_sr_pointer - Read the value of a Shadow RAM pointer word
+ * @hw: pointer to the HW structure
+ * @offset: the word offset of the Shadow RAM word to read
+ * @pointer: pointer value read from Shadow RAM
+ *
+ * Read the given Shadow RAM word, and convert it to a pointer value specified
+ * in bytes. This function assumes the specified offset is a valid pointer
+ * word.
+ *
+ * Each pointer word specifies whether it is stored in word size or 4KB
+ * sector size by using the highest bit. The reported pointer value will be in
+ * bytes, intended for flat NVM reads.
+ */
+static enum ice_status
+ice_read_sr_pointer(struct ice_hw *hw, u16 offset, u32 *pointer)
+{
+	enum ice_status status;
+	u16 value;
+
+	status = ice_read_sr_word(hw, offset, &value);
+	if (status)
+		return status;
+
+	/* Determine if the pointer is in 4KB or word units */
+	if (value & ICE_SR_NVM_PTR_4KB_UNITS)
+		*pointer = (value & ~ICE_SR_NVM_PTR_4KB_UNITS) * 4 * 1024;
+	else
+		*pointer = value * 2;
+
+	return 0;
+}
+
+/**
+ * ice_read_sr_area_size - Read an area size from a Shadow RAM word
+ * @hw: pointer to the HW structure
+ * @offset: the word offset of the Shadow RAM to read
+ * @size: size value read from the Shadow RAM
+ *
+ * Read the given Shadow RAM word, and convert it to an area size value
+ * specified in bytes. This function assumes the specified offset is a valid
+ * area size word.
+ *
+ * Each area size word is specified in 4KB sector units. This function reports
+ * the size in bytes, intended for flat NVM reads.
+ */
+static enum ice_status
+ice_read_sr_area_size(struct ice_hw *hw, u16 offset, u32 *size)
+{
+	enum ice_status status;
+	u16 value;
+
+	status = ice_read_sr_word(hw, offset, &value);
+	if (status)
+		return status;
+
+	/* Area sizes are always specified in 4KB units */
+	*size = value * 4 * 1024;
+
+	return 0;
+}
+
+/**
+ * ice_determine_active_flash_banks - Discover active bank for each module
+ * @hw: pointer to the HW struct
+ *
+ * Read the Shadow RAM control word and determine which banks are active for
+ * the NVM, OROM, and Netlist modules. Also read and calculate the associated
+ * pointer and size. These values are then cached into the ice_flash_info
+ * structure for later use in order to calculate the correct offset to read
+ * from the active module.
+ */
+static enum ice_status
+ice_determine_active_flash_banks(struct ice_hw *hw)
+{
+	struct ice_bank_info *banks = &hw->flash.banks;
+	enum ice_status status;
+	u16 ctrl_word;
+
+	status = ice_read_sr_word(hw, ICE_SR_NVM_CTRL_WORD, &ctrl_word);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read the Shadow RAM control word\n");
+		return status;
+	}
+
+	/* Check that the control word indicates validity */
+	if ((ctrl_word & ICE_SR_CTRL_WORD_1_M) >> ICE_SR_CTRL_WORD_1_S != ICE_SR_CTRL_WORD_VALID) {
+		ice_debug(hw, ICE_DBG_NVM, "Shadow RAM control word is invalid\n");
+		return ICE_ERR_CFG;
+	}
+
+	if (!(ctrl_word & ICE_SR_CTRL_WORD_NVM_BANK))
+		banks->nvm_bank = ICE_1ST_FLASH_BANK;
+	else
+		banks->nvm_bank = ICE_2ND_FLASH_BANK;
+
+	if (!(ctrl_word & ICE_SR_CTRL_WORD_OROM_BANK))
+		banks->orom_bank = ICE_1ST_FLASH_BANK;
+	else
+		banks->orom_bank = ICE_2ND_FLASH_BANK;
+
+	if (!(ctrl_word & ICE_SR_CTRL_WORD_NETLIST_BANK))
+		banks->netlist_bank = ICE_1ST_FLASH_BANK;
+	else
+		banks->netlist_bank = ICE_2ND_FLASH_BANK;
+
+	status = ice_read_sr_pointer(hw, ICE_SR_1ST_NVM_BANK_PTR, &banks->nvm_ptr);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read NVM bank pointer\n");
+		return status;
+	}
+
+	status = ice_read_sr_area_size(hw, ICE_SR_NVM_BANK_SIZE, &banks->nvm_size);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read NVM bank area size\n");
+		return status;
+	}
+
+	status = ice_read_sr_pointer(hw, ICE_SR_1ST_OROM_BANK_PTR, &banks->orom_ptr);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read OROM bank pointer\n");
+		return status;
+	}
+
+	status = ice_read_sr_area_size(hw, ICE_SR_OROM_BANK_SIZE, &banks->orom_size);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read OROM bank area size\n");
+		return status;
+	}
+
+	status = ice_read_sr_pointer(hw, ICE_SR_NETLIST_BANK_PTR, &banks->netlist_ptr);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read Netlist bank pointer\n");
+		return status;
+	}
+
+	status = ice_read_sr_area_size(hw, ICE_SR_NETLIST_BANK_SIZE, &banks->netlist_size);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read Netlist bank area size\n");
+		return status;
+	}
+
+	return 0;
+}
+
 /**
  * ice_init_nvm - initializes NVM setting
  * @hw: pointer to the HW struct
@@ -643,6 +788,12 @@ enum ice_status ice_init_nvm(struct ice_hw *hw)
 		return status;
 	}
 
+	status = ice_determine_active_flash_banks(hw);
+	if (status) {
+		ice_debug(hw, ICE_DBG_NVM, "Failed to determine active flash banks.\n");
+		return status;
+	}
+
 	status = ice_get_nvm_ver_info(hw, &flash->nvm);
 	if (status) {
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read NVM info.\n");
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 7af7758374d4..bc3be64cf3d9 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -330,11 +330,34 @@ struct ice_netlist_info {
 	u16 cust_ver;			/* customer version */
 };
 
+/* Enumeration of possible flash banks for the NVM, OROM, and Netlist modules
+ * of the flash image.
+ */
+enum ice_flash_bank {
+	ICE_INVALID_FLASH_BANK,
+	ICE_1ST_FLASH_BANK,
+	ICE_2ND_FLASH_BANK,
+};
+
+/* information for accessing NVM, OROM, and Netlist flash banks */
+struct ice_bank_info {
+	u32 nvm_ptr;				/* Pointer to 1st NVM bank */
+	u32 nvm_size;				/* Size of NVM bank */
+	u32 orom_ptr;				/* Pointer to 1st OROM bank */
+	u32 orom_size;				/* Size of OROM bank */
+	u32 netlist_ptr;			/* Pointer to 1st Netlist bank */
+	u32 netlist_size;			/* Size of Netlist bank */
+	enum ice_flash_bank nvm_bank;		/* Active NVM bank */
+	enum ice_flash_bank orom_bank;		/* Active OROM bank */
+	enum ice_flash_bank netlist_bank;	/* Active Netlist bank */
+};
+
 /* Flash Chip Information */
 struct ice_flash_info {
 	struct ice_orom_info orom;	/* Option ROM version info */
 	struct ice_nvm_info nvm;	/* NVM version information */
 	struct ice_netlist_info netlist;/* Netlist version info */
+	struct ice_bank_info banks;	/* Flash Bank information */
 	u16 sr_words;			/* Shadow RAM size in words */
 	u32 flash_size;			/* Size of available flash in bytes */
 	u8 blank_nvm_mode;		/* is NVM empty (no FW present) */
@@ -770,6 +793,7 @@ struct ice_hw_port_stats {
 };
 
 /* Checksum and Shadow RAM pointers */
+#define ICE_SR_NVM_CTRL_WORD		0x00
 #define ICE_SR_BOOT_CFG_PTR		0x132
 #define ICE_SR_NVM_WOL_CFG		0x19
 #define ICE_NVM_OROM_VER_OFF		0x02
@@ -789,10 +813,23 @@ struct ice_hw_port_stats {
 #define ICE_OROM_VER_MASK		(0xff << ICE_OROM_VER_SHIFT)
 #define ICE_SR_PFA_PTR			0x40
 #define ICE_SR_1ST_NVM_BANK_PTR		0x42
+#define ICE_SR_NVM_BANK_SIZE		0x43
 #define ICE_SR_1ST_OROM_BANK_PTR	0x44
+#define ICE_SR_OROM_BANK_SIZE		0x45
 #define ICE_SR_NETLIST_BANK_PTR		0x46
+#define ICE_SR_NETLIST_BANK_SIZE	0x47
 #define ICE_SR_SECTOR_SIZE_IN_WORDS	0x800
 
+/* Auxiliary field, mask, and shift definition for Shadow RAM and NVM Flash */
+#define ICE_SR_CTRL_WORD_1_S		0x06
+#define ICE_SR_CTRL_WORD_1_M		(0x03 << ICE_SR_CTRL_WORD_1_S)
+#define ICE_SR_CTRL_WORD_VALID		0x1
+#define ICE_SR_CTRL_WORD_OROM_BANK	BIT(3)
+#define ICE_SR_CTRL_WORD_NETLIST_BANK	BIT(4)
+#define ICE_SR_CTRL_WORD_NVM_BANK	BIT(5)
+
+#define ICE_SR_NVM_PTR_4KB_UNITS	BIT(15)
+
 /* Link override related */
 #define ICE_SR_PFA_LINK_OVERRIDE_WORDS		10
 #define ICE_SR_PFA_LINK_OVERRIDE_PHY_WORDS	4
-- 
2.26.2

Re: [PATCH net-next 02/15] ice: cache NVM module bank information

[Willem de Bruijn]

On Thu, Jan 28, 2021 at 7:46 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> From: Jacob Keller <jacob.e.keller@intel.com>
>
> The ice flash contains two copies of each of the NVM, Option ROM, and
> Netlist modules. Each bank has a pointer word and a size word. In order
> to correctly read from the active flash bank, the driver must calculate
> the offset manually.
>
> During NVM initialization, read the Shadow RAM control word and
> determine which bank is active for each NVM module. Additionally, cache
> the size and pointer values for use in calculating the correct offset.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_nvm.c  | 151 ++++++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_type.h |  37 ++++++
>  2 files changed, 188 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index b0f0b4fc266b..308344045397 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -603,6 +603,151 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
>         return status;
>  }
>
> +/**
> + * ice_read_sr_pointer - Read the value of a Shadow RAM pointer word
> + * @hw: pointer to the HW structure
> + * @offset: the word offset of the Shadow RAM word to read
> + * @pointer: pointer value read from Shadow RAM
> + *
> + * Read the given Shadow RAM word, and convert it to a pointer value specified
> + * in bytes. This function assumes the specified offset is a valid pointer
> + * word.
> + *
> + * Each pointer word specifies whether it is stored in word size or 4KB
> + * sector size by using the highest bit. The reported pointer value will be in
> + * bytes, intended for flat NVM reads.
> + */
> +static enum ice_status
> +ice_read_sr_pointer(struct ice_hw *hw, u16 offset, u32 *pointer)
> +{
> +       enum ice_status status;
> +       u16 value;
> +
> +       status = ice_read_sr_word(hw, offset, &value);
> +       if (status)
> +               return status;
> +
> +       /* Determine if the pointer is in 4KB or word units */
> +       if (value & ICE_SR_NVM_PTR_4KB_UNITS)
> +               *pointer = (value & ~ICE_SR_NVM_PTR_4KB_UNITS) * 4 * 1024;
> +       else
> +               *pointer = value * 2;

Should this be << 2, for 4B words?

Re: [PATCH net-next 02/15] ice: cache NVM module bank information

[Willem de Bruijn]

On Fri, Jan 29, 2021 at 4:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 7:46 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
> >
> > From: Jacob Keller <jacob.e.keller@intel.com>
> >
> > The ice flash contains two copies of each of the NVM, Option ROM, and
> > Netlist modules. Each bank has a pointer word and a size word. In order
> > to correctly read from the active flash bank, the driver must calculate
> > the offset manually.
> >
> > During NVM initialization, read the Shadow RAM control word and
> > determine which bank is active for each NVM module. Additionally, cache
> > the size and pointer values for use in calculating the correct offset.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_nvm.c  | 151 ++++++++++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_type.h |  37 ++++++
> >  2 files changed, 188 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> > index b0f0b4fc266b..308344045397 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> > @@ -603,6 +603,151 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
> >         return status;
> >  }
> >
> > +/**
> > + * ice_read_sr_pointer - Read the value of a Shadow RAM pointer word
> > + * @hw: pointer to the HW structure
> > + * @offset: the word offset of the Shadow RAM word to read
> > + * @pointer: pointer value read from Shadow RAM
> > + *
> > + * Read the given Shadow RAM word, and convert it to a pointer value specified
> > + * in bytes. This function assumes the specified offset is a valid pointer
> > + * word.
> > + *
> > + * Each pointer word specifies whether it is stored in word size or 4KB
> > + * sector size by using the highest bit. The reported pointer value will be in
> > + * bytes, intended for flat NVM reads.
> > + */
> > +static enum ice_status
> > +ice_read_sr_pointer(struct ice_hw *hw, u16 offset, u32 *pointer)
> > +{
> > +       enum ice_status status;
> > +       u16 value;
> > +
> > +       status = ice_read_sr_word(hw, offset, &value);
> > +       if (status)
> > +               return status;
> > +
> > +       /* Determine if the pointer is in 4KB or word units */
> > +       if (value & ICE_SR_NVM_PTR_4KB_UNITS)
> > +               *pointer = (value & ~ICE_SR_NVM_PTR_4KB_UNITS) * 4 * 1024;
> > +       else
> > +               *pointer = value * 2;
>
> Should this be << 2, for 4B words?

Never mind, sorry. I gather from patch 3 that wordsize is 16b.

Re: [PATCH net-next 02/15] ice: cache NVM module bank information

[Jacob Keller]

On 1/29/2021 1:04 PM, Willem de Bruijn wrote:
> On Fri, Jan 29, 2021 at 4:01 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>>
>> On Thu, Jan 28, 2021 at 7:46 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>>>
>>> From: Jacob Keller <jacob.e.keller@intel.com>
>>>
>>> The ice flash contains two copies of each of the NVM, Option ROM, and
>>> Netlist modules. Each bank has a pointer word and a size word. In order
>>> to correctly read from the active flash bank, the driver must calculate
>>> the offset manually.
>>>
>>> During NVM initialization, read the Shadow RAM control word and
>>> determine which bank is active for each NVM module. Additionally, cache
>>> the size and pointer values for use in calculating the correct offset.
>>>
>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>> Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>> ---
>>>  drivers/net/ethernet/intel/ice/ice_nvm.c  | 151 ++++++++++++++++++++++
>>>  drivers/net/ethernet/intel/ice/ice_type.h |  37 ++++++
>>>  2 files changed, 188 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> index b0f0b4fc266b..308344045397 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> @@ -603,6 +603,151 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
>>>         return status;
>>>  }
>>>
>>> +/**
>>> + * ice_read_sr_pointer - Read the value of a Shadow RAM pointer word
>>> + * @hw: pointer to the HW structure
>>> + * @offset: the word offset of the Shadow RAM word to read
>>> + * @pointer: pointer value read from Shadow RAM
>>> + *
>>> + * Read the given Shadow RAM word, and convert it to a pointer value specified
>>> + * in bytes. This function assumes the specified offset is a valid pointer
>>> + * word.
>>> + *
>>> + * Each pointer word specifies whether it is stored in word size or 4KB
>>> + * sector size by using the highest bit. The reported pointer value will be in
>>> + * bytes, intended for flat NVM reads.
>>> + */
>>> +static enum ice_status
>>> +ice_read_sr_pointer(struct ice_hw *hw, u16 offset, u32 *pointer)
>>> +{
>>> +       enum ice_status status;
>>> +       u16 value;
>>> +
>>> +       status = ice_read_sr_word(hw, offset, &value);
>>> +       if (status)
>>> +               return status;
>>> +
>>> +       /* Determine if the pointer is in 4KB or word units */
>>> +       if (value & ICE_SR_NVM_PTR_4KB_UNITS)
>>> +               *pointer = (value & ~ICE_SR_NVM_PTR_4KB_UNITS) * 4 * 1024;
>>> +       else
>>> +               *pointer = value * 2;
>>
>> Should this be << 2, for 4B words?
> 
> Never mind, sorry. I gather from patch 3 that wordsize is 16b.
> 


Ah, yes that could have been explained a bit better. In this context, a
word is indeed 2 bytes.

Perhaps we could have used "<< 1" and "<< 12" or similar instead of the
multiplication, but I felt this was a bit more clear.

Thanks,
Jake

Re: [PATCH net-next 02/15] ice: cache NVM module bank information

[Willem de Bruijn]

On Fri, Jan 29, 2021 at 4:32 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 1/29/2021 1:04 PM, Willem de Bruijn wrote:
> > On Fri, Jan 29, 2021 at 4:01 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> >>
> >> On Thu, Jan 28, 2021 at 7:46 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
> >>>
> >>> From: Jacob Keller <jacob.e.keller@intel.com>
> >>>
> >>> The ice flash contains two copies of each of the NVM, Option ROM, and
> >>> Netlist modules. Each bank has a pointer word and a size word. In order
> >>> to correctly read from the active flash bank, the driver must calculate
> >>> the offset manually.
> >>>
> >>> During NVM initialization, read the Shadow RAM control word and
> >>> determine which bank is active for each NVM module. Additionally, cache
> >>> the size and pointer values for use in calculating the correct offset.
> >>>
> >>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >>> Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
> >>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >>> ---
> >>>  drivers/net/ethernet/intel/ice/ice_nvm.c  | 151 ++++++++++++++++++++++
> >>>  drivers/net/ethernet/intel/ice/ice_type.h |  37 ++++++
> >>>  2 files changed, 188 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> >>> index b0f0b4fc266b..308344045397 100644
> >>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> >>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> >>> @@ -603,6 +603,151 @@ static enum ice_status ice_discover_flash_size(struct ice_hw *hw)
> >>>         return status;
> >>>  }
> >>>
> >>> +/**
> >>> + * ice_read_sr_pointer - Read the value of a Shadow RAM pointer word
> >>> + * @hw: pointer to the HW structure
> >>> + * @offset: the word offset of the Shadow RAM word to read
> >>> + * @pointer: pointer value read from Shadow RAM
> >>> + *
> >>> + * Read the given Shadow RAM word, and convert it to a pointer value specified
> >>> + * in bytes. This function assumes the specified offset is a valid pointer
> >>> + * word.
> >>> + *
> >>> + * Each pointer word specifies whether it is stored in word size or 4KB
> >>> + * sector size by using the highest bit. The reported pointer value will be in
> >>> + * bytes, intended for flat NVM reads.
> >>> + */
> >>> +static enum ice_status
> >>> +ice_read_sr_pointer(struct ice_hw *hw, u16 offset, u32 *pointer)
> >>> +{
> >>> +       enum ice_status status;
> >>> +       u16 value;
> >>> +
> >>> +       status = ice_read_sr_word(hw, offset, &value);
> >>> +       if (status)
> >>> +               return status;
> >>> +
> >>> +       /* Determine if the pointer is in 4KB or word units */
> >>> +       if (value & ICE_SR_NVM_PTR_4KB_UNITS)
> >>> +               *pointer = (value & ~ICE_SR_NVM_PTR_4KB_UNITS) * 4 * 1024;
> >>> +       else
> >>> +               *pointer = value * 2;
> >>
> >> Should this be << 2, for 4B words?
> >
> > Never mind, sorry. I gather from patch 3 that wordsize is 16b.
> >
>
>
> Ah, yes that could have been explained a bit better. In this context, a
> word is indeed 2 bytes.
>
> Perhaps we could have used "<< 1" and "<< 12" or similar instead of the
> multiplication, but I felt this was a bit more clear.

Thanks. That doesn't matter (for me). I just wrongly assumed wordsize to be 4B.

[PATCH net-next 03/15] ice: read security revision to ice_nvm_info and ice_orom_info

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The main NVM module and the Option ROM module contain a security
revision in their CSS header. This security revision is used to
determine whether or not the signed module should be loaded at bootup.
If the module security revision is lower than the associated minimum
security revision, it will not be loaded.

The CSS header does not have a module id associated with it, and thus
requires flat NVM reads in order to access it. To do this, take
advantage of the cached bank information. Introduce a new
"ice_read_flash_module" function that takes the module and bank to read.
Implement both ice_read_active_nvm_module and
ice_read_active_orom_module. These functions will use the cached values
to determine the active bank and calculate the appropriate offset.

Using these new access functions, extract the security revision for both
the main NVM bank and the Option ROM into the associated info structure.

Add the security revisions to the devlink info output. Report the main
NVM bank security revision as "fw.mgmt.srev". Report the Option ROM
security revision as "fw.undi.srev".

A future patch will add the associated minimum security revisions as
devlink flash parameters.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 Documentation/networking/devlink/ice.rst     |   9 +
 drivers/net/ethernet/intel/ice/ice_devlink.c |  20 +++
 drivers/net/ethernet/intel/ice/ice_nvm.c     | 172 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_type.h    |   9 +
 4 files changed, 210 insertions(+)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index a432dc419fa4..78707970ee62 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -38,6 +38,11 @@ The ``ice`` driver reports the following versions
       - running
       - 0x305d955f
       - Unique identifier of the source for the management firmware.
+    * - ``fw.mgmt.srev``
+      - running
+      - 2
+      - Security revision of the management firmware and associated NVM
+        contents.
     * - ``fw.undi``
       - running
       - 1.2581.0
@@ -48,6 +53,10 @@ The ``ice`` driver reports the following versions
         non-breaking changes and reset to 1 when the major version is
         incremented. The patch version is normally 0 but is incremented when
         a fix is delivered as a patch against an older base Option ROM.
+    * - ``fw.undi.srev``
+      - running
+      - 2
+      - Security revision of the Option ROM containing the UEFI driver.
     * - ``fw.psid.api``
       - running
       - 0.80
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 44b64524b1b8..4b08bf6dd0b0 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -56,6 +56,15 @@ static int ice_info_fw_build(struct ice_pf *pf, char *buf, size_t len)
 	return 0;
 }
 
+static int ice_info_fw_srev(struct ice_pf *pf, char *buf, size_t len)
+{
+	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
+
+	snprintf(buf, len, "%u", nvm->srev);
+
+	return 0;
+}
+
 static int ice_info_orom_ver(struct ice_pf *pf, char *buf, size_t len)
 {
 	struct ice_orom_info *orom = &pf->hw.flash.orom;
@@ -65,6 +74,15 @@ static int ice_info_orom_ver(struct ice_pf *pf, char *buf, size_t len)
 	return 0;
 }
 
+static int ice_info_orom_srev(struct ice_pf *pf, char *buf, size_t len)
+{
+	struct ice_orom_info *orom = &pf->hw.flash.orom;
+
+	snprintf(buf, len, "%u", orom->srev);
+
+	return 0;
+}
+
 static int ice_info_nvm_ver(struct ice_pf *pf, char *buf, size_t len)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
@@ -148,7 +166,9 @@ static const struct ice_devlink_version {
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, ice_info_fw_mgmt),
 	running("fw.mgmt.api", ice_info_fw_api),
 	running("fw.mgmt.build", ice_info_fw_build),
+	running("fw.mgmt.srev", ice_info_fw_srev),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_UNDI, ice_info_orom_ver),
+	running("fw.undi.srev", ice_info_orom_srev),
 	running("fw.psid.api", ice_info_nvm_ver),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, ice_info_eetrack),
 	running("fw.app.name", ice_info_ddp_pkg_name),
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 308344045397..21eef3d037d6 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -233,6 +233,105 @@ void ice_release_nvm(struct ice_hw *hw)
 	ice_release_res(hw, ICE_NVM_RES_ID);
 }
 
+/**
+ * ice_read_flash_module - Read a word from one of the main NVM modules
+ * @hw: pointer to the HW structure
+ * @bank: which bank of the module to read
+ * @module: the module to read
+ * @offset: the offset into the module in words
+ * @data: storage for the word read from the flash
+ *
+ * Read a word from the specified bank of the module. The bank must be either
+ * the 1st or 2nd bank. The word will be read using flat NVM access, and
+ * relies on the hw->flash.banks data being setup by
+ * ice_determine_active_flash_banks() during initialization.
+ */
+static enum ice_status
+ice_read_flash_module(struct ice_hw *hw, enum ice_flash_bank bank, u16 module,
+		      u32 offset, u16 *data)
+{
+	struct ice_bank_info *banks = &hw->flash.banks;
+	u32 bytes = sizeof(u16);
+	enum ice_status status;
+	__le16 data_local;
+	bool second_bank;
+	u32 start;
+
+	switch (bank) {
+	case ICE_1ST_FLASH_BANK:
+		second_bank = false;
+		break;
+	case ICE_2ND_FLASH_BANK:
+		second_bank = true;
+		break;
+	case ICE_INVALID_FLASH_BANK:
+	default:
+		ice_debug(hw, ICE_DBG_NVM, "Unexpected flash bank %u\n", bank);
+		return ICE_ERR_PARAM;
+	}
+
+	switch (module) {
+	case ICE_SR_1ST_NVM_BANK_PTR:
+		start = banks->nvm_ptr + (second_bank ? banks->nvm_size : 0);
+		break;
+	case ICE_SR_1ST_OROM_BANK_PTR:
+		start = banks->orom_ptr + (second_bank ? banks->orom_size : 0);
+		break;
+	case ICE_SR_NETLIST_BANK_PTR:
+		start = banks->netlist_ptr + (second_bank ? banks->netlist_size : 0);
+		break;
+	default:
+		ice_debug(hw, ICE_DBG_NVM, "Unexpected flash module 0x%04x\n", module);
+		return ICE_ERR_PARAM;
+	}
+
+	status = ice_acquire_nvm(hw, ICE_RES_READ);
+	if (status)
+		return status;
+
+	status = ice_read_flat_nvm(hw, start + offset * sizeof(u16), &bytes,
+				   (__force u8 *)&data_local, false);
+	if (!status)
+		*data = le16_to_cpu(data_local);
+
+	ice_release_nvm(hw);
+
+	return status;
+}
+
+/**
+ * ice_read_active_nvm_module - Read from the active main NVM module
+ * @hw: pointer to the HW structure
+ * @offset: offset into the NVM module to read, in words
+ * @data: storage for returned word value
+ *
+ * Read the specified word from the active NVM module. This includes the CSS
+ * header at the start of the NVM module.
+ */
+static enum ice_status
+ice_read_active_nvm_module(struct ice_hw *hw, u32 offset, u16 *data)
+{
+	return ice_read_flash_module(hw, hw->flash.banks.nvm_bank,
+				     ICE_SR_1ST_NVM_BANK_PTR, offset, data);
+}
+
+/**
+ * ice_read_active_orom_module - Read from the active Option ROM module
+ * @hw: pointer to the HW structure
+ * @offset: offset into the OROM module to read, in words
+ * @data: storage for returned word value
+ *
+ * Read the specified word from the active Option ROM module of the flash.
+ * Note that unlike the NVM module, the CSS data is stored at the end of the
+ * module instead of at the beginning.
+ */
+static enum ice_status
+ice_read_active_orom_module(struct ice_hw *hw, u32 offset, u16 *data)
+{
+	return ice_read_flash_module(hw, hw->flash.banks.orom_bank,
+				     ICE_SR_1ST_OROM_BANK_PTR, offset, data);
+}
+
 /**
  * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
  * @hw: pointer to the HW structure
@@ -379,6 +478,32 @@ ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size)
 	return status;
 }
 
+/**
+ * ice_get_nvm_srev - Read the security revision from the NVM CSS header
+ * @hw: pointer to the HW struct
+ * @srev: storage for security revision
+ *
+ * Read the security revision out of the CSS header of the active NVM module
+ * bank.
+ */
+static enum ice_status ice_get_nvm_srev(struct ice_hw *hw, u32 *srev)
+{
+	enum ice_status status;
+	u16 srev_l, srev_h;
+
+	status = ice_read_active_nvm_module(hw, ICE_NVM_CSS_SREV_L, &srev_l);
+	if (status)
+		return status;
+
+	status = ice_read_active_nvm_module(hw, ICE_NVM_CSS_SREV_H, &srev_h);
+	if (status)
+		return status;
+
+	*srev = srev_h << 16 | srev_l;
+
+	return 0;
+}
+
 /**
  * ice_get_nvm_ver_info - Read NVM version information
  * @hw: pointer to the HW struct
@@ -414,6 +539,49 @@ ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
 
 	nvm->eetrack = (eetrack_hi << 16) | eetrack_lo;
 
+	status = ice_get_nvm_srev(hw, &nvm->srev);
+	if (status)
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read NVM security revision.\n");
+
+	return 0;
+}
+
+/**
+ * ice_get_orom_srev - Read the security revision from the OROM CSS header
+ * @hw: pointer to the HW struct
+ * @srev: storage for security revision
+ *
+ * Read the security revision out of the CSS header of the active OROM module
+ * bank.
+ */
+static enum ice_status ice_get_orom_srev(struct ice_hw *hw, u32 *srev)
+{
+	enum ice_status status;
+	u16 srev_l, srev_h;
+	u32 css_start;
+
+	if (hw->flash.banks.orom_size < ICE_NVM_OROM_TRAILER_LENGTH) {
+		ice_debug(hw, ICE_DBG_NVM, "Unexpected Option ROM Size of %u\n",
+			  hw->flash.banks.orom_size);
+		return ICE_ERR_CFG;
+	}
+
+	/* calculate how far into the Option ROM the CSS header starts. Note
+	 * that ice_read_active_orom_module takes a word offset so we need to
+	 * divide by 2 here.
+	 */
+	css_start = (hw->flash.banks.orom_size - ICE_NVM_OROM_TRAILER_LENGTH) / 2;
+
+	status = ice_read_active_orom_module(hw, css_start + ICE_NVM_CSS_SREV_L, &srev_l);
+	if (status)
+		return status;
+
+	status = ice_read_active_orom_module(hw, css_start + ICE_NVM_CSS_SREV_H, &srev_h);
+	if (status)
+		return status;
+
+	*srev = srev_h << 16 | srev_l;
+
 	return 0;
 }
 
@@ -469,6 +637,10 @@ ice_get_orom_ver_info(struct ice_hw *hw, struct ice_orom_info *orom)
 	orom->build = (u16)((combo_ver & ICE_OROM_VER_BUILD_MASK) >>
 			    ICE_OROM_VER_BUILD_SHIFT);
 
+	status = ice_get_orom_srev(hw, &orom->srev);
+	if (status)
+		ice_debug(hw, ICE_DBG_NVM, "Failed to read Option ROM security revision.\n");
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index bc3be64cf3d9..0e0cbf90c431 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -311,11 +311,13 @@ struct ice_orom_info {
 	u8 major;			/* Major version of OROM */
 	u8 patch;			/* Patch version of OROM */
 	u16 build;			/* Build version of OROM */
+	u32 srev;			/* Security revision */
 };
 
 /* NVM version information */
 struct ice_nvm_info {
 	u32 eetrack;
+	u32 srev;
 	u8 major;
 	u8 minor;
 };
@@ -820,6 +822,13 @@ struct ice_hw_port_stats {
 #define ICE_SR_NETLIST_BANK_SIZE	0x47
 #define ICE_SR_SECTOR_SIZE_IN_WORDS	0x800
 
+/* CSS Header words */
+#define ICE_NVM_CSS_SREV_L			0x14
+#define ICE_NVM_CSS_SREV_H			0x15
+
+/* Size in bytes of Option ROM trailer */
+#define ICE_NVM_OROM_TRAILER_LENGTH		660
+
 /* Auxiliary field, mask, and shift definition for Shadow RAM and NVM Flash */
 #define ICE_SR_CTRL_WORD_1_S		0x06
 #define ICE_SR_CTRL_WORD_1_M		(0x03 << ICE_SR_CTRL_WORD_1_S)
-- 
2.26.2

Re: [PATCH net-next 03/15] ice: read security revision to ice_nvm_info and ice_orom_info

[Jakub Kicinski]

On Thu, 28 Jan 2021 16:43:20 -0800 Tony Nguyen wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The main NVM module and the Option ROM module contain a security
> revision in their CSS header. This security revision is used to
> determine whether or not the signed module should be loaded at bootup.
> If the module security revision is lower than the associated minimum
> security revision, it will not be loaded.
> 
> The CSS header does not have a module id associated with it, and thus
> requires flat NVM reads in order to access it. To do this, take
> advantage of the cached bank information. Introduce a new
> "ice_read_flash_module" function that takes the module and bank to read.
> Implement both ice_read_active_nvm_module and
> ice_read_active_orom_module. These functions will use the cached values
> to determine the active bank and calculate the appropriate offset.
> 
> Using these new access functions, extract the security revision for both
> the main NVM bank and the Option ROM into the associated info structure.
> 
> Add the security revisions to the devlink info output. Report the main
> NVM bank security revision as "fw.mgmt.srev". Report the Option ROM
> security revision as "fw.undi.srev".
> 
> A future patch will add the associated minimum security revisions as
> devlink flash parameters.

This needs a wider discussion. Hopefully we can agree on a reasonably
uniform way of handling this across vendors. Having to fish out
_particular_ version keys out and then target _particular_ parameters
for each vendor is not great.

First off - is there a standard around the version management that we
can base the interface on? What about key management? There's gotta be
a way of revoking keys too, right?


I'd recommend separating the srev patches out of the series so the
other ones can land.

RE: [PATCH net-next 03/15] ice: read security revision to ice_nvm_info and ice_orom_info

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, January 29, 2021 10:44 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; Keller, Jacob E <jacob.e.keller@intel.com>;
> netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX
> <tonyx.brelinski@intel.com>
> Subject: Re: [PATCH net-next 03/15] ice: read security revision to ice_nvm_info
> and ice_orom_info
> 
> On Thu, 28 Jan 2021 16:43:20 -0800 Tony Nguyen wrote:
> > From: Jacob Keller <jacob.e.keller@intel.com>
> >
> > The main NVM module and the Option ROM module contain a security
> > revision in their CSS header. This security revision is used to
> > determine whether or not the signed module should be loaded at bootup.
> > If the module security revision is lower than the associated minimum
> > security revision, it will not be loaded.
> >
> > The CSS header does not have a module id associated with it, and thus
> > requires flat NVM reads in order to access it. To do this, take
> > advantage of the cached bank information. Introduce a new
> > "ice_read_flash_module" function that takes the module and bank to read.
> > Implement both ice_read_active_nvm_module and
> > ice_read_active_orom_module. These functions will use the cached values
> > to determine the active bank and calculate the appropriate offset.
> >
> > Using these new access functions, extract the security revision for both
> > the main NVM bank and the Option ROM into the associated info structure.
> >
> > Add the security revisions to the devlink info output. Report the main
> > NVM bank security revision as "fw.mgmt.srev". Report the Option ROM
> > security revision as "fw.undi.srev".
> >
> > A future patch will add the associated minimum security revisions as
> > devlink flash parameters.
> 
> This needs a wider discussion. Hopefully we can agree on a reasonably
> uniform way of handling this across vendors. Having to fish out
> _particular_ version keys out and then target _particular_ parameters
> for each vendor is not great.
> 

Yea, I can see how that would be problematic. It does seem like some sort of tied interface would make sense.

> First off - is there a standard around the version management that we
> can base the interface on? What about key management? There's gotta be
> a way of revoking keys too, right?
> 

I am not sure. None of the implementation I've written deals with key management and it wasn't an ask.

> 
> I'd recommend separating the srev patches out of the series so the
> other ones can land.

Sure, We can do that.

[PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice NVM flash has a security revision field for the main NVM bank
and the Option ROM bank. In addition to the revision within the module,
the device also has a minimum security revision TLV area. This minimum
security revision field indicates the minimum value that will be
accepted for the associated security revision when loading the NVM bank.

Add functions to read and update the minimum security revisions. Use
these functions to implement devlink parameters, "fw.undi.minsrev" and
"fw.mgmt.minsrev".

These parameters are permanent (i.e. stored in flash), and are used to
indicate the minimum security revision of the associated NVM bank. If
the image in the bank has a lower security revision, then the flash
loader will not continue loading that flash bank.

The new parameters allow for opting in to update the minimum security
revision to ensure that a flash image with a known security flaw cannot
be loaded.

Note that the minimum security revision cannot be reduced, only
increased. The driver also refuses to allow an update if the currently
active image revision is lower than the requested value. This is done to
avoid potentially updating the value such that the device can no longer
start.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 Documentation/networking/devlink/ice.rst      |  34 +++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  17 ++
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 218 +++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_devlink.h  |   2 +
 drivers/net/ethernet/intel/ice/ice_main.c     |   3 +
 drivers/net/ethernet/intel/ice/ice_nvm.c      | 112 +++++++++
 drivers/net/ethernet/intel/ice/ice_nvm.h      |   4 +
 drivers/net/ethernet/intel/ice/ice_type.h     |   8 +
 8 files changed, 397 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 78707970ee62..efa366ae633a 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -96,6 +96,40 @@ The ``ice`` driver reports the following versions
       - 0xee16ced7
       - The first 4 bytes of the hash of the netlist module contents.
 
+Parameters
+==========
+
+The minimum security revision fields of the ice device control whether the
+associated flash section can be loaded. If the security revision field of
+the section -- ``fw.mgmt.srev`` for the main firmware section and
+``fw.undi.srev`` for the Option ROM -- is lower than the associated minimum
+security revision, then the device will not load that section of firmware.
+
+The ``ice`` driver implements driver-specific parameters for updating the
+minimum security revision fields associated those two sections of the device
+flash. Note that the device will not allow lowering a minimum security
+revision, nor will it allow increasing the security revision higher than the
+associated security revision of the active flash image.
+
+.. list-table:: Minimum security revision parameters
+      :widths: 5 5 5 85
+
+   * - Name
+     - Type
+     - Mode
+     - Description
+   * - ``fw.undi.minsrev``
+     - u32
+     - permanent
+     - The device's minimum security revision for the ``fw.undi`` section of
+       the flash.
+   * - ``fw.mgmt.minsrev``
+     - u32
+     - permanent
+     - The device's minimum security revision for the ``fw.mgmt`` section of
+       the flash.
+
+
 Flash Update
 ============
 
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index b06fbe99d8e9..40c96662458a 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1334,6 +1334,8 @@ struct ice_aqc_nvm_checksum {
 	u8 rsvd2[12];
 };
 
+#define ICE_AQC_NVM_MINSREV_MOD_ID		0x130
+
 /* The result of netlist NVM read comes in a TLV format. The actual data
  * (netlist header) starts from word offset 1 (byte 2). The FW strips
  * out the type field from the TLV header so all the netlist fields
@@ -1361,6 +1363,21 @@ struct ice_aqc_nvm_checksum {
 #define ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH		0xA
 #define ICE_AQC_NVM_NETLIST_ID_BLK_CUST_VER		0x2F
 
+/* Used for reading and writing MinSRev using 0x0701 and 0x0703. Note that the
+ * type field is excluded from the section when reading and writing from
+ * a module using the module_typeid field with these AQ commands.
+ */
+struct ice_aqc_nvm_minsrev {
+	__le16 length;
+	__le16 validity;
+#define ICE_AQC_NVM_MINSREV_NVM_VALID		BIT(0)
+#define ICE_AQC_NVM_MINSREV_OROM_VALID		BIT(1)
+	__le16 nvm_minsrev_l;
+	__le16 nvm_minsrev_h;
+	__le16 orom_minsrev_l;
+	__le16 orom_minsrev_h;
+};
+
 /* Used for NVM Set Package Data command - 0x070A */
 struct ice_aqc_nvm_pkg_data {
 	u8 reserved[3];
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 4b08bf6dd0b0..8af33ef70f76 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -250,6 +250,193 @@ static int ice_devlink_info_get(struct devlink *devlink,
 	return 0;
 }
 
+enum ice_devlink_param_id {
+	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
+	ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
+};
+
+/**
+ * ice_devlink_minsrev_get - Get the current minimum security revision
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to get
+ * @ctx: context to return the parameter value
+ *
+ * Returns: zero on success, or an error code on failure.
+ */
+static int
+ice_devlink_minsrev_get(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_minsrev_info minsrevs = {};
+	enum ice_status status;
+
+	if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV &&
+	    id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV)
+		return -EINVAL;
+
+	status = ice_get_nvm_minsrevs(&pf->hw, &minsrevs);
+	if (status) {
+		dev_warn(dev, "Failed to read minimum security revision data from flash\n");
+		return -EIO;
+	}
+
+	/* We report zero if the device has not yet had a valid minimum
+	 * security revision programmed for the associated module. This makes
+	 * sense because it is not possible to have a security revision of
+	 * less than zero. Thus, all images will be able to load if the
+	 * minimum security revision is zero, the same as the case where the
+	 * minimum value is indicated as invalid.
+	 */
+	switch (id) {
+	case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV:
+		if (minsrevs.nvm_valid)
+			ctx->val.vu32 = minsrevs.nvm;
+		else
+			ctx->val.vu32 = 0;
+		break;
+	case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV:
+		if (minsrevs.orom_valid)
+			ctx->val.vu32 = minsrevs.orom;
+		else
+			ctx->val.vu32 = 0;
+		break;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_devlink_minsrev_set - Set the minimum security revision
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to set
+ * @ctx: context to return the parameter value
+ *
+ * Set the minimum security revision value for fw.mgmt or fw.undi. The kernel
+ * calls the validate handler before calling this, so we do not need to
+ * duplicate those checks here.
+ *
+ * Returns: zero on success, or an error code on failure.
+ */
+static int
+ice_devlink_minsrev_set(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_minsrev_info minsrevs = {};
+	enum ice_status status;
+
+	switch (id) {
+	case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV:
+		minsrevs.nvm_valid = true;
+		minsrevs.nvm = ctx->val.vu32;
+		break;
+	case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV:
+		minsrevs.orom_valid = true;
+		minsrevs.orom = ctx->val.vu32;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	status = ice_update_nvm_minsrevs(&pf->hw, &minsrevs);
+	if (status) {
+		dev_warn(dev, "Failed to update minimum security revision data\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_devlink_minsrev_validate - Validate a minimum security revision update
+ * @devlink: unused pointer to devlink instance
+ * @id: the parameter ID to validate
+ * @val: value to validate
+ * @extack: netlink extended ACK structure
+ *
+ * Check that a proposed update to a minimum security revision field is valid.
+ * Each minimum security revision can only be increased, not decreased.
+ * Additionally, we verify that the value is never set higher than the
+ * security revision of the active flash component.
+ *
+ * Returns: zero if the value is valid, -ERANGE if it is out of range, and
+ * -EINVAL if this function is called with the wrong ID.
+ */
+static int
+ice_devlink_minsrev_validate(struct devlink *devlink, u32 id, union devlink_param_value val,
+			     struct netlink_ext_ack *extack)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_minsrev_info minsrevs = {};
+	enum ice_status status;
+
+	if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV &&
+	    id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV)
+		return -EINVAL;
+
+	status = ice_get_nvm_minsrevs(&pf->hw, &minsrevs);
+	if (status) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to read minimum security revision data from flash");
+		return -EIO;
+	}
+
+	switch (id) {
+	case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV:
+		if (val.vu32 > pf->hw.flash.nvm.srev) {
+			NL_SET_ERR_MSG_MOD(extack, "Cannot update fw.mgmt minimum security revision higher than the currently running firmware");
+			dev_dbg(dev, "Attempted to set fw.mgmt.minsrev to %u, but running firmware has srev %u\n",
+				val.vu32, pf->hw.flash.nvm.srev);
+			return -EPERM;
+		}
+
+		if (minsrevs.nvm_valid && val.vu32 < minsrevs.nvm) {
+			NL_SET_ERR_MSG_MOD(extack, "Cannot lower the minimum security revision for fw.mgmt flash section");
+			dev_dbg(dev, "Attempted  to set fw.mgmt.minsrev to %u, but current minsrev is %u\n",
+				val.vu32, minsrevs.nvm);
+			return -EPERM;
+		}
+		break;
+	case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV:
+		if (val.vu32 > pf->hw.flash.orom.srev) {
+			NL_SET_ERR_MSG_MOD(extack, "Cannot update fw.undi minimum security revision higher than the currently running firmware");
+			dev_dbg(dev, "Attempted to set fw.undi.minsrev to %u, but running firmware has srev %u\n",
+				val.vu32, pf->hw.flash.orom.srev);
+			return -EPERM;
+		}
+
+		if (minsrevs.orom_valid && val.vu32 < minsrevs.orom) {
+			NL_SET_ERR_MSG_MOD(extack, "Cannot lower the minimum security revision for fw.undi flash section");
+			dev_dbg(dev, "Attempted  to set fw.undi.minsrev to %u, but current minsrev is %u\n",
+				val.vu32, minsrevs.orom);
+			return -EPERM;
+		}
+		break;
+	}
+
+	return 0;
+}
+
+/* devlink parameters for the ice driver */
+static const struct devlink_param ice_devlink_params[] = {
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
+			     "fw.mgmt.minsrev",
+			     DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     ice_devlink_minsrev_get,
+			     ice_devlink_minsrev_set,
+			     ice_devlink_minsrev_validate),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
+			     "fw.undi.minsrev",
+			     DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     ice_devlink_minsrev_get,
+			     ice_devlink_minsrev_set,
+			     ice_devlink_minsrev_validate),
+};
+
 /**
  * ice_devlink_flash_update - Update firmware stored in flash on the device
  * @devlink: pointer to devlink associated with device to update
@@ -356,6 +543,13 @@ int ice_devlink_register(struct ice_pf *pf)
 		return err;
 	}
 
+	err = devlink_params_register(devlink, ice_devlink_params,
+				      ARRAY_SIZE(ice_devlink_params));
+	if (err) {
+		dev_err(dev, "devlink params registration failed: %d\n", err);
+		return err;
+	}
+
 	return 0;
 }
 
@@ -367,7 +561,29 @@ int ice_devlink_register(struct ice_pf *pf)
  */
 void ice_devlink_unregister(struct ice_pf *pf)
 {
-	devlink_unregister(priv_to_devlink(pf));
+	struct devlink *devlink = priv_to_devlink(pf);
+
+	devlink_params_unregister(devlink, ice_devlink_params,
+				  ARRAY_SIZE(ice_devlink_params));
+	devlink_unregister(devlink);
+}
+
+/**
+ * ice_devlink_params_publish - Publish parameters to allow user access.
+ * @pf: the PF structure pointer
+ */
+void ice_devlink_params_publish(struct ice_pf *pf)
+{
+	devlink_params_publish(priv_to_devlink(pf));
+}
+
+/**
+ * ice_devlink_params_unpublish - Unpublish parameters to prevent user access.
+ * @pf: the PF structure pointer
+ */
+void ice_devlink_params_unpublish(struct ice_pf *pf)
+{
+	devlink_params_unpublish(priv_to_devlink(pf));
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.h b/drivers/net/ethernet/intel/ice/ice_devlink.h
index e07e74426bde..e3363ea5c7ac 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.h
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.h
@@ -8,6 +8,8 @@ struct ice_pf *ice_allocate_pf(struct device *dev);
 
 int ice_devlink_register(struct ice_pf *pf);
 void ice_devlink_unregister(struct ice_pf *pf);
+void ice_devlink_params_publish(struct ice_pf *pf);
+void ice_devlink_params_unpublish(struct ice_pf *pf);
 int ice_devlink_create_port(struct ice_vsi *vsi);
 void ice_devlink_destroy_port(struct ice_vsi *vsi);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 6e251dfffc91..66a40dfadb6a 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4071,6 +4071,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	}
 
 	ice_devlink_init_regions(pf);
+	ice_devlink_params_publish(pf);
 
 	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
 	pf->hw.udp_tunnel_nic.unset_port = ice_udp_tunnel_unset_port;
@@ -4259,6 +4260,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	devm_kfree(dev, pf->vsi);
 err_init_pf_unroll:
 	ice_deinit_pf(pf);
+	ice_devlink_params_unpublish(pf);
 	ice_devlink_destroy_regions(pf);
 	ice_deinit_hw(hw);
 err_exit_unroll:
@@ -4371,6 +4373,7 @@ static void ice_remove(struct pci_dev *pdev)
 		ice_vsi_free_q_vectors(pf->vsi[i]);
 	}
 	ice_deinit_pf(pf);
+	ice_devlink_params_unpublish(pf);
 	ice_devlink_destroy_regions(pf);
 	ice_deinit_hw(&pf->hw);
 	ice_devlink_unregister(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 21eef3d037d6..ed4d6058a90d 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -1038,6 +1038,118 @@ enum ice_status ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags)
 	return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
 }
 
+/**
+ * ice_get_nvm_minsrevs - Get the Minimum Security Revision values from flash
+ * @hw: pointer to the HW struct
+ * @minsrevs: structure to store NVM and OROM minsrev values
+ *
+ * Read the Minimum Security Revision TLV and extract the revision values from
+ * the flash image into a readable structure for processing.
+ */
+enum ice_status
+ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs)
+{
+	struct ice_aqc_nvm_minsrev data;
+	enum ice_status status;
+	u16 valid;
+
+	status = ice_acquire_nvm(hw, ICE_RES_READ);
+	if (status)
+		return status;
+
+	status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data),
+				 &data, true, false, NULL);
+
+	ice_release_nvm(hw);
+
+	if (status)
+		return status;
+
+	valid = le16_to_cpu(data.validity);
+
+	/* Extract NVM minimum security revision */
+	if (valid & ICE_AQC_NVM_MINSREV_NVM_VALID) {
+		u16 minsrev_l, minsrev_h;
+
+		minsrev_l = le16_to_cpu(data.nvm_minsrev_l);
+		minsrev_h = le16_to_cpu(data.nvm_minsrev_h);
+
+		minsrevs->nvm = minsrev_h << 16 | minsrev_l;
+		minsrevs->nvm_valid = true;
+	}
+
+	/* Extract the OROM minimum security revision */
+	if (valid & ICE_AQC_NVM_MINSREV_OROM_VALID) {
+		u16 minsrev_l, minsrev_h;
+
+		minsrev_l = le16_to_cpu(data.orom_minsrev_l);
+		minsrev_h = le16_to_cpu(data.orom_minsrev_h);
+
+		minsrevs->orom = minsrev_h << 16 | minsrev_l;
+		minsrevs->orom_valid = true;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_update_nvm_minsrevs - Update minimum security revision TLV data in flash
+ * @hw: pointer to the HW struct
+ * @minsrevs: minimum security revision information
+ *
+ * Update the NVM or Option ROM minimum security revision fields in the PFA
+ * area of the flash. Reads the minsrevs->nvm_valid and minsrevs->orom_valid
+ * fields to determine what update is being requested. If the valid bit is not
+ * set for that module, then the associated minsrev will be left as is.
+ */
+enum ice_status
+ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs)
+{
+	struct ice_aqc_nvm_minsrev data;
+	enum ice_status status;
+
+	if (!minsrevs->nvm_valid && !minsrevs->orom_valid) {
+		ice_debug(hw, ICE_DBG_NVM, "At least one of NVM and OROM MinSrev must be valid");
+		return ICE_ERR_PARAM;
+	}
+
+	status = ice_acquire_nvm(hw, ICE_RES_WRITE);
+	if (status)
+		return status;
+
+	/* Get current data */
+	status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data),
+				 &data, true, false, NULL);
+	if (status)
+		goto exit_release_res;
+
+	if (minsrevs->nvm_valid) {
+		data.nvm_minsrev_l = cpu_to_le16(minsrevs->nvm & 0xFFFF);
+		data.nvm_minsrev_h = cpu_to_le16(minsrevs->nvm >> 16);
+		data.validity |= cpu_to_le16(ICE_AQC_NVM_MINSREV_NVM_VALID);
+	}
+
+	if (minsrevs->orom_valid) {
+		data.orom_minsrev_l = cpu_to_le16(minsrevs->orom & 0xFFFF);
+		data.orom_minsrev_h = cpu_to_le16(minsrevs->orom >> 16);
+		data.validity |= cpu_to_le16(ICE_AQC_NVM_MINSREV_OROM_VALID);
+	}
+
+	/* Update flash data */
+	status = ice_aq_update_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0, sizeof(data), &data,
+				   true, ICE_AQC_NVM_SPECIAL_UPDATE, NULL);
+	if (status)
+		goto exit_release_res;
+
+	/* Dump the Shadow RAM to the flash */
+	status = ice_nvm_write_activate(hw, 0);
+
+exit_release_res:
+	ice_release_nvm(hw);
+
+	return status;
+}
+
 /**
  * ice_aq_nvm_update_empr
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 8d430909f846..8cfb9b9ac638 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -14,6 +14,10 @@ enum ice_status
 ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
 		       u16 module_type);
 enum ice_status
+ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
+enum ice_status
+ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
+enum ice_status
 ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size);
 enum ice_status ice_init_nvm(struct ice_hw *hw);
 enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data);
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 0e0cbf90c431..f387641195a9 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -322,6 +322,14 @@ struct ice_nvm_info {
 	u8 minor;
 };
 
+/* Minimum Security Revision information */
+struct ice_minsrev_info {
+	u32 nvm;
+	u32 orom;
+	u8 nvm_valid : 1;
+	u8 orom_valid : 1;
+};
+
 /* netlist version information */
 struct ice_netlist_info {
 	u32 major;			/* major high/low */
-- 
2.26.2

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jakub Kicinski]

On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The ice NVM flash has a security revision field for the main NVM bank
> and the Option ROM bank. In addition to the revision within the module,
> the device also has a minimum security revision TLV area. This minimum
> security revision field indicates the minimum value that will be
> accepted for the associated security revision when loading the NVM bank.
> 
> Add functions to read and update the minimum security revisions. Use
> these functions to implement devlink parameters, "fw.undi.minsrev" and
> "fw.mgmt.minsrev".
> 
> These parameters are permanent (i.e. stored in flash), and are used to
> indicate the minimum security revision of the associated NVM bank. If
> the image in the bank has a lower security revision, then the flash
> loader will not continue loading that flash bank.
> 
> The new parameters allow for opting in to update the minimum security
> revision to ensure that a flash image with a known security flaw cannot
> be loaded.
> 
> Note that the minimum security revision cannot be reduced, only
> increased. The driver also refuses to allow an update if the currently
> active image revision is lower than the requested value. This is done to
> avoid potentially updating the value such that the device can no longer
> start.

Hi Jake, I had a couple of conversations with people from operations
and I'm struggling to find interest in writing this parameter. 

It seems like the expectation is that the min sec revision will go up
automatically after a new firmware with a higher number is flashed.

Do you have a user scenario where the manual bumping is needed?

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jacob Keller]

On 2/3/2021 12:41 PM, Jakub Kicinski wrote:
> On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote:
>> From: Jacob Keller <jacob.e.keller@intel.com>
>>
>> The ice NVM flash has a security revision field for the main NVM bank
>> and the Option ROM bank. In addition to the revision within the module,
>> the device also has a minimum security revision TLV area. This minimum
>> security revision field indicates the minimum value that will be
>> accepted for the associated security revision when loading the NVM bank.
>>
>> Add functions to read and update the minimum security revisions. Use
>> these functions to implement devlink parameters, "fw.undi.minsrev" and
>> "fw.mgmt.minsrev".
>>
>> These parameters are permanent (i.e. stored in flash), and are used to
>> indicate the minimum security revision of the associated NVM bank. If
>> the image in the bank has a lower security revision, then the flash
>> loader will not continue loading that flash bank.
>>
>> The new parameters allow for opting in to update the minimum security
>> revision to ensure that a flash image with a known security flaw cannot
>> be loaded.
>>
>> Note that the minimum security revision cannot be reduced, only
>> increased. The driver also refuses to allow an update if the currently
>> active image revision is lower than the requested value. This is done to
>> avoid potentially updating the value such that the device can no longer
>> start.

Hi Jakub,

> 
> Hi Jake, I had a couple of conversations with people from operations
> and I'm struggling to find interest in writing this parameter. 
>> It seems like the expectation is that the min sec revision will go up
> automatically after a new firmware with a higher number is flashed.
> 

I believe the intention is that the update is not automatic, and
requires the user to opt-in to enforcing the new minimum value. This is
because once you update this value it is not possible to lower it
without physical access to reflash the chip directly. It's intended as a
mechanism to allow a system administrator to ensure that the board is
unable to downgrade below a given minimum security revision.

> Do you have a user scenario where the manual bumping is needed?
> 

In our case, we have tools which would use this interface and would
perform the update upon request i.e. because the tool is configured to
perform the update.

We don't want this field to be updated every time the board is flashed,
as it is supposed to be an optional "opt-in", and not forced.

The flow is something like:

a) device is as firmware version with SREV of 1
b) new firmware is flashed with SREV 2
c) system administrator confirms that new firmware is working and that
no issues have occurred
d) system administrator then decides to enforce new srev by updating the
minimum srev value.

If there was an issue at step (c), we want to still be able to roll back
to the old firmware. If the minimum srev is updated automatically, this
would not be possible.

I've asked for further details from some of our firmware folks, and can
try to provide further information. The key is that making it automatic
is bad because it prevents rollback, so we want to ensure that it is
only updated after the system administrator is ready to opt-in.

Ofcourse, it is plausible that most won't actually update this ever,
because preventing the ability to use an old firmware might not be desired.

The goal with this series was to provide a mechanism to allow the
update, because existing tools based on direct flash access have support
for this, and we want to ensure that these tools can be ported to
devlink without the direct flash access that we were (ab)using before.

Thanks,
Jake

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jakub Kicinski]

On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote:
> On 2/3/2021 12:41 PM, Jakub Kicinski wrote:
> > On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote:  
> >> From: Jacob Keller <jacob.e.keller@intel.com>
> >>
> >> The ice NVM flash has a security revision field for the main NVM bank
> >> and the Option ROM bank. In addition to the revision within the module,
> >> the device also has a minimum security revision TLV area. This minimum
> >> security revision field indicates the minimum value that will be
> >> accepted for the associated security revision when loading the NVM bank.
> >>
> >> Add functions to read and update the minimum security revisions. Use
> >> these functions to implement devlink parameters, "fw.undi.minsrev" and
> >> "fw.mgmt.minsrev".
> >>
> >> These parameters are permanent (i.e. stored in flash), and are used to
> >> indicate the minimum security revision of the associated NVM bank. If
> >> the image in the bank has a lower security revision, then the flash
> >> loader will not continue loading that flash bank.
> >>
> >> The new parameters allow for opting in to update the minimum security
> >> revision to ensure that a flash image with a known security flaw cannot
> >> be loaded.
> >>
> >> Note that the minimum security revision cannot be reduced, only
> >> increased. The driver also refuses to allow an update if the currently
> >> active image revision is lower than the requested value. This is done to
> >> avoid potentially updating the value such that the device can no longer
> >> start.  
> >
> > Hi Jake, I had a couple of conversations with people from operations
> > and I'm struggling to find interest in writing this parameter.   
> >> It seems like the expectation is that the min sec revision will go up  
> > automatically after a new firmware with a higher number is flashed.
> 
> I believe the intention is that the update is not automatic, and
> requires the user to opt-in to enforcing the new minimum value. This is
> because once you update this value it is not possible to lower it
> without physical access to reflash the chip directly. It's intended as a
> mechanism to allow a system administrator to ensure that the board is
> unable to downgrade below a given minimum security revision.
> 
> > Do you have a user scenario where the manual bumping is needed?
> >   
> 
> In our case, we have tools which would use this interface and would
> perform the update upon request i.e. because the tool is configured to
> perform the update.
> 
> We don't want this field to be updated every time the board is flashed,
> as it is supposed to be an optional "opt-in", and not forced.
> 
> The flow is something like:
> 
> a) device is as firmware version with SREV of 1
> b) new firmware is flashed with SREV 2
> c) system administrator confirms that new firmware is working and that
> no issues have occurred
> d) system administrator then decides to enforce new srev by updating the
> minimum srev value.

Dunno, seems to me secure by default is a better approach. If admin 
is worried you can always ship an eval build which does not bump the
version. Or address the issues with the next release rather than roll
back.

> If there was an issue at step (c), we want to still be able to roll back
> to the old firmware. If the minimum srev is updated automatically, this
> would not be possible.
> 
> I've asked for further details from some of our firmware folks, and can
> try to provide further information. The key is that making it automatic
> is bad because it prevents rollback, so we want to ensure that it is
> only updated after the system administrator is ready to opt-in.
> 
> Ofcourse, it is plausible that most won't actually update this ever,
> because preventing the ability to use an old firmware might not be desired.

Well, if there is a point to secure boot w/ NICs people better prevent
replay attacks. Not every FW update is a security update, tho, so it's
not like "going to the old version" would never be possible.

> The goal with this series was to provide a mechanism to allow the
> update, because existing tools based on direct flash access have support
> for this, and we want to ensure that these tools can be ported to
> devlink without the direct flash access that we were (ab)using before.

I'm not completely opposed to this mechanism (although you may want 
to communicate the approach to your customers clearly, because many
may be surprised) - but let's be clear - the goal of devlink is to
create a replacement for vendor tooling, not be their underlying
mechanism.

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jacob Keller]

On 2/3/2021 6:08 PM, Jakub Kicinski wrote:
> On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote:
>> On 2/3/2021 12:41 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote:  
>>>> From: Jacob Keller <jacob.e.keller@intel.com>
>>>>
>>>> The ice NVM flash has a security revision field for the main NVM bank
>>>> and the Option ROM bank. In addition to the revision within the module,
>>>> the device also has a minimum security revision TLV area. This minimum
>>>> security revision field indicates the minimum value that will be
>>>> accepted for the associated security revision when loading the NVM bank.
>>>>
>>>> Add functions to read and update the minimum security revisions. Use
>>>> these functions to implement devlink parameters, "fw.undi.minsrev" and
>>>> "fw.mgmt.minsrev".
>>>>
>>>> These parameters are permanent (i.e. stored in flash), and are used to
>>>> indicate the minimum security revision of the associated NVM bank. If
>>>> the image in the bank has a lower security revision, then the flash
>>>> loader will not continue loading that flash bank.
>>>>
>>>> The new parameters allow for opting in to update the minimum security
>>>> revision to ensure that a flash image with a known security flaw cannot
>>>> be loaded.
>>>>
>>>> Note that the minimum security revision cannot be reduced, only
>>>> increased. The driver also refuses to allow an update if the currently
>>>> active image revision is lower than the requested value. This is done to
>>>> avoid potentially updating the value such that the device can no longer
>>>> start.  
>>>
>>> Hi Jake, I had a couple of conversations with people from operations
>>> and I'm struggling to find interest in writing this parameter.   
>>>> It seems like the expectation is that the min sec revision will go up  
>>> automatically after a new firmware with a higher number is flashed.
>>
>> I believe the intention is that the update is not automatic, and
>> requires the user to opt-in to enforcing the new minimum value. This is
>> because once you update this value it is not possible to lower it
>> without physical access to reflash the chip directly. It's intended as a
>> mechanism to allow a system administrator to ensure that the board is
>> unable to downgrade below a given minimum security revision.
>>
>>> Do you have a user scenario where the manual bumping is needed?
>>>   
>>
>> In our case, we have tools which would use this interface and would
>> perform the update upon request i.e. because the tool is configured to
>> perform the update.
>>
>> We don't want this field to be updated every time the board is flashed,
>> as it is supposed to be an optional "opt-in", and not forced.
>>
>> The flow is something like:
>>
>> a) device is as firmware version with SREV of 1
>> b) new firmware is flashed with SREV 2
>> c) system administrator confirms that new firmware is working and that
>> no issues have occurred
>> d) system administrator then decides to enforce new srev by updating the
>> minimum srev value.
> 
> Dunno, seems to me secure by default is a better approach. If admin 
> is worried you can always ship an eval build which does not bump the
> version. Or address the issues with the next release rather than roll
> back.
> 
>> If there was an issue at step (c), we want to still be able to roll back
>> to the old firmware. If the minimum srev is updated automatically, this
>> would not be possible.
>>
>> I've asked for further details from some of our firmware folks, and can
>> try to provide further information. The key is that making it automatic
>> is bad because it prevents rollback, so we want to ensure that it is
>> only updated after the system administrator is ready to opt-in.
>>
>> Ofcourse, it is plausible that most won't actually update this ever,
>> because preventing the ability to use an old firmware might not be desired.
> 
> Well, if there is a point to secure boot w/ NICs people better prevent
> replay attacks. Not every FW update is a security update, tho, so it's
> not like "going to the old version" would never be possible.
> 
Correct. The issue only comes up for firmware versions which have an
updated security revision, which we do not expect to occur every release.

>> The goal with this series was to provide a mechanism to allow the
>> update, because existing tools based on direct flash access have support
>> for this, and we want to ensure that these tools can be ported to
>> devlink without the direct flash access that we were (ab)using before.
> 
> I'm not completely opposed to this mechanism (although you may want 
> to communicate the approach to your customers clearly, because many
> may be surprised)
I do admit the primary motivation for this implementation was to enable
the existing tooling to function.

I'd rather see the right solution designed here, so if this isn't the
right direction I want to work with the list to figure out what makes
the most sense. (Even if that's "minimum security should update
automatically").

Perhaps some extension of the info mechanism or other solution is also
preferable so that it is more generic?

But I don't have any example from other vendors that matches this so I'm
not sure what makes the most sense here.

> but let's be clear - the goal of devlink is to
> create a replacement for vendor tooling, not be their underlying
> mechanism.
> 

The intention of modifying our tools is to help ensure existing
practices and tools can continue to work. This is primarily being done
to appease folks who "just want things to keep working as is".

I agree with the goal of devlink. But convincing everyone here to move
is a slow process that mostly involves educating folks one by one.

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jacob Keller]

On 2/4/2021 11:10 AM, Jacob Keller wrote:
> I'd rather see the right solution designed here, so if this isn't the
> right direction I want to work with the list to figure out what makes
> the most sense. (Even if that's "minimum security should update
> automatically").
>
I want to clarify here based on feedback I received from customer
support engineers: We believe it is not acceptable to update this
automatically, because not all customers want that behavior and would
prefer to have control over when to lock in the minimum security revision.

Previous products have behaved this way and we had significant feedback
when this occurred that many of our customers were unhappy about this,
even after we explained the reasoning.

I do not believe that we can accept an automatic/default update of
minimum security revision.

RE: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Brelinski, TonyX]

From: Jacob Keller <jacob.e.keller@intel.com> 
Sent: Thursday, February 4, 2021 1:54 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com>
Subject: Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision



On 2/4/2021 11:10 AM, Jacob Keller wrote:
> I'd rather see the right solution designed here, so if this isn't the 
> right direction I want to work with the list to figure out what makes 
> the most sense. (Even if that's "minimum security should update 
> automatically").
>
I want to clarify here based on feedback I received from customer support engineers: We believe it is not acceptable to update this automatically, because not all customers want that behavior and would prefer to have control over when to lock in the minimum security revision.

Previous products have behaved this way and we had significant feedback when this occurred that many of our customers were unhappy about this, even after we explained the reasoning.

I do not believe that we can accept an automatic/default update of minimum security revision.

----------------------------------------------
I tested this revision:
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com> A Contingent Worker at Intel

RE: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Brelinski, TonyX]

From: Brelinski, TonyX 
Sent: Friday, February 5, 2021 6:32 PM
To: Jacob Keller <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com
Subject: RE: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

From: Jacob Keller <jacob.e.keller@intel.com> 
Sent: Thursday, February 4, 2021 1:54 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com>
Subject: Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision



On 2/4/2021 11:10 AM, Jacob Keller wrote:
> I'd rather see the right solution designed here, so if this isn't the 
> right direction I want to work with the list to figure out what makes 
> the most sense. (Even if that's "minimum security should update 
> automatically").
>
I want to clarify here based on feedback I received from customer support engineers: We believe it is not acceptable to update this automatically, because not all customers want that behavior and would prefer to have control over when to lock in the minimum security revision.

Previous products have behaved this way and we had significant feedback when this occurred that many of our customers were unhappy about this, even after we explained the reasoning.

I do not believe that we can accept an automatic/default update of minimum security revision.

----------------------------------------------

Scratch that.  I replied to the wrong email.  Sorry about that.

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jakub Kicinski]

On Thu, 4 Feb 2021 13:53:34 -0800 Jacob Keller wrote:
> On 2/4/2021 11:10 AM, Jacob Keller wrote:
> > I'd rather see the right solution designed here, so if this isn't the
> > right direction I want to work with the list to figure out what makes
> > the most sense. (Even if that's "minimum security should update
> > automatically").
> >  
> I want to clarify here based on feedback I received from customer
> support engineers: We believe it is not acceptable to update this
> automatically, because not all customers want that behavior and would
> prefer to have control over when to lock in the minimum security revision.
> 
> Previous products have behaved this way and we had significant feedback
> when this occurred that many of our customers were unhappy about this,
> even after we explained the reasoning.
> 
> I do not believe that we can accept an automatic/default update of
> minimum security revision.

I spent some time reading through various docs, and my main concern 
now is introduction of an API which does not have any cryptographic
guarantees.

An attacker who has infiltrated the OS but did not manage to crack 
the device yet, can fake the SEV responses and keep the counter from
ever being bumped until they successfully expoit the device. Is the 
min SEV counter included in device measurements?

I'm starting to think that distributing separate FW builds with and
without auto-SEV bump is the best way to fit into the SecBoot infra,
without additional wrinkles and attack vectors.

WDYT?

Re: [PATCH net-next 04/15] ice: add devlink parameters to read and write minimum security revision

[Jacob Keller]

On 2/3/2021 6:08 PM, Jakub Kicinski wrote:
> On Wed, 3 Feb 2021 17:34:24 -0800 Jacob Keller wrote:
>> On 2/3/2021 12:41 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jan 2021 16:43:21 -0800 Tony Nguyen wrote:  
>>>> From: Jacob Keller <jacob.e.keller@intel.com>
>>>>
>>>> The ice NVM flash has a security revision field for the main NVM bank
>>>> and the Option ROM bank. In addition to the revision within the module,
>>>> the device also has a minimum security revision TLV area. This minimum
>>>> security revision field indicates the minimum value that will be
>>>> accepted for the associated security revision when loading the NVM bank.
>>>>
>>>> Add functions to read and update the minimum security revisions. Use
>>>> these functions to implement devlink parameters, "fw.undi.minsrev" and
>>>> "fw.mgmt.minsrev".
>>>>
>>>> These parameters are permanent (i.e. stored in flash), and are used to
>>>> indicate the minimum security revision of the associated NVM bank. If
>>>> the image in the bank has a lower security revision, then the flash
>>>> loader will not continue loading that flash bank.
>>>>
>>>> The new parameters allow for opting in to update the minimum security
>>>> revision to ensure that a flash image with a known security flaw cannot
>>>> be loaded.
>>>>
>>>> Note that the minimum security revision cannot be reduced, only
>>>> increased. The driver also refuses to allow an update if the currently
>>>> active image revision is lower than the requested value. This is done to
>>>> avoid potentially updating the value such that the device can no longer
>>>> start.  
>>>
>>> Hi Jake, I had a couple of conversations with people from operations
>>> and I'm struggling to find interest in writing this parameter.   
>>>> It seems like the expectation is that the min sec revision will go up  
>>> automatically after a new firmware with a higher number is flashed.
>>
>> I believe the intention is that the update is not automatic, and
>> requires the user to opt-in to enforcing the new minimum value. This is
>> because once you update this value it is not possible to lower it
>> without physical access to reflash the chip directly. It's intended as a
>> mechanism to allow a system administrator to ensure that the board is
>> unable to downgrade below a given minimum security revision.
>>
>>> Do you have a user scenario where the manual bumping is needed?
>>>   


I've spoken with some of our customer support engineers. Feedback I've
received is that this was implemented as an automatic/default/enforced
update in past products. Several customers have indicated that they want
to be in control of when this update happens, and not to have it happen
automatically.

Specifically I've been asked to ensure this update is something that
must be "opt in" and not by default, because of the issues we've
received from vendors.


> 
> Dunno, seems to me secure by default is a better approach. If admin 
> is worried you can always ship an eval build which does not bump the
> version. Or address the issues with the next release rather than roll
> back.
> 

This is how we had it implemented in previous products, but we got
significant feedback that it should be a step that can be controlled by
the admin, so that they can decide when it is appropriate.

Making this the default behavior that the driver automatically occurs
after an update is not something we want, based on the feedback we've
received in our previous products.

The feedback that we've received is that a "one size fits all" automatic
update of the minimum value is not acceptable to all of our customers.

>>
>> Ofcourse, it is plausible that most won't actually update this ever,
>> because preventing the ability to use an old firmware might not be desired.
> 
> Well, if there is a point to secure boot w/ NICs people better prevent
> replay attacks. Not every FW update is a security update, tho, so it's
> not like "going to the old version" would never be possible.
> 

After I spoke to some folks internally I believe I misstated above: it's
not about never updating this, but about being in control of when to
perform the update.

>> The goal with this series was to provide a mechanism to allow the
>> update, because existing tools based on direct flash access have support
>> for this, and we want to ensure that these tools can be ported to
>> devlink without the direct flash access that we were (ab)using before.
> 
> I'm not completely opposed to this mechanism (although you may want 
> to communicate the approach to your customers clearly, because many
> may be surprised) - but let's be clear - the goal of devlink is to
> create a replacement for vendor tooling, not be their underlying
> mechanism.
> 

We want some mechanism to allow administrators to decide when to update
this value. We do not want to have a "default" be to update because that
means the system administrators who want control over the process might
accidentally perform a non-reversible  operation. (Once you update the
value you can't lower it without physically accessing the flash chip).

I understand if the use of vendor-specific parameters here isn't
acceptable. I'm happy to help design a more generic solution that avoids
these and potentially integrates better into the flash update process.

There is also the technical question of when the update can be
performed. As is, it's a software controlled update, and must occur
after the new flash image is booted, in order to ensure we do not update
the value in a way that bricks the currently running firmware from booting.

[PATCH net-next 05/15] ice: report timeout length for erasing during devlink flash

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

When erasing, notify userspace of how long we will potentially take to
erase a module. Doing so allows userspace to report the timeout, giving
a clear indication of the upper time bound of the operation.

Since we're re-using the erase timeout value, make it a macro rather
than a magic number.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Shannon Nelson <snelson@pensando.io>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_fw_update.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
index 8f81b95e679c..dcec0360ce55 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
@@ -417,6 +417,11 @@ ice_write_nvm_module(struct ice_pf *pf, u16 module, const char *component,
 	return err;
 }
 
+/* Length in seconds to wait before timing out when erasing a flash module.
+ * Yes, erasing really can take minutes to complete.
+ */
+#define ICE_FW_ERASE_TIMEOUT 300
+
 /**
  * ice_erase_nvm_module - Erase an NVM module and await firmware completion
  * @pf: the PF data structure
@@ -449,7 +454,7 @@ ice_erase_nvm_module(struct ice_pf *pf, u16 module, const char *component,
 
 	devlink = priv_to_devlink(pf);
 
-	devlink_flash_update_status_notify(devlink, "Erasing", component, 0, 0);
+	devlink_flash_update_timeout_notify(devlink, "Erasing", component, ICE_FW_ERASE_TIMEOUT);
 
 	status = ice_aq_erase_nvm(hw, module, NULL);
 	if (status) {
@@ -461,8 +466,7 @@ ice_erase_nvm_module(struct ice_pf *pf, u16 module, const char *component,
 		goto out_notify_devlink;
 	}
 
-	/* Yes, this really can take minutes to complete */
-	err = ice_aq_wait_for_event(pf, ice_aqc_opc_nvm_erase, 300 * HZ, &event);
+	err = ice_aq_wait_for_event(pf, ice_aqc_opc_nvm_erase, ICE_FW_ERASE_TIMEOUT * HZ, &event);
 	if (err) {
 		dev_err(dev, "Timed out waiting for firmware to respond with erase completion for %s (module 0x%02x), err %d\n",
 			component, module, err);
-- 
2.26.2

[PATCH net-next 06/15] ice: introduce context struct for info report

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice driver uses an array of structures which link an info name with
a function that formats the associated version data into a string.

All existing format functions simply format already captured static data
from the driver hw structure. Future changes will introduce format
functions for reporting the versions of flash sections stored but not
yet applied. This type of version data is not stored as a member of the
hw structure. This is because (a) it might not yet exist in the case
there is no pending flash update, and (b) even if it does, it might
change such as if an update is canceled or replaced by a new update
before finalizing.

We could simply have each format function gather its own data upon being
called. However, in some cases the raw binary version data is
a combination of multiple different reported fields. Additionally, the
current interface doesn't have a way for the function to indicate that
the version doesn't exist.

Refactor this function interface to take a new ice_info_ctx structure
instead of the buffer pointer and length. This context structure allows
for future extensions to pre-gather version data that is stored within
the context struct instead of the hw struct.

Allocate this context structure initially at the start of
ice_devlink_info_get. We use dynamic allocation instead of a local stack
variable in order to avoid using too much kernel stack once we extend it
with additional data structures.

Modify the main loop that drives the info reporting so that the version
buffer string is always cleared between each format. Explicitly check
that the format function actually filled in a version string of non-zero
length. If the string is not provided, simply skip this version without
reporting an error. This allows for introducing format functions of
versions which may or may not be present, such as the version of
a pending update that has not yet been activated.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 117 ++++++++++++-------
 1 file changed, 72 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 8af33ef70f76..917fdbb20b7b 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -6,144 +6,159 @@
 #include "ice_devlink.h"
 #include "ice_fw_update.h"
 
-static void ice_info_get_dsn(struct ice_pf *pf, char *buf, size_t len)
+/* context for devlink info version reporting */
+struct ice_info_ctx {
+	char buf[128];
+};
+
+/* The following functions are used to format specific strings for various
+ * devlink info versions. The ctx parameter is used to provide the storage
+ * buffer, as well as any ancillary information calculated when the info
+ * request was made.
+ *
+ * If a version does not exist, for example a "stored" version that does not
+ * exist because no update is pending, the function should leave the buffer in
+ * the ctx structure empty and return 0.
+ */
+
+static void ice_info_get_dsn(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	u8 dsn[8];
 
 	/* Copy the DSN into an array in Big Endian format */
 	put_unaligned_be64(pci_get_dsn(pf->pdev), dsn);
 
-	snprintf(buf, len, "%8phD", dsn);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%8phD", dsn);
 }
 
-static int ice_info_pba(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_pba(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
 	enum ice_status status;
 
-	status = ice_read_pba_string(hw, (u8 *)buf, len);
+	status = ice_read_pba_string(hw, (u8 *)ctx->buf, sizeof(ctx->buf));
 	if (status)
 		return -EIO;
 
 	return 0;
 }
 
-static int ice_info_fw_mgmt(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_fw_mgmt(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
 
-	snprintf(buf, len, "%u.%u.%u", hw->fw_maj_ver, hw->fw_min_ver,
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u.%u.%u", hw->fw_maj_ver, hw->fw_min_ver,
 		 hw->fw_patch);
 
 	return 0;
 }
 
-static int ice_info_fw_api(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_fw_api(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
 
-	snprintf(buf, len, "%u.%u", hw->api_maj_ver, hw->api_min_ver);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u.%u", hw->api_maj_ver, hw->api_min_ver);
 
 	return 0;
 }
 
-static int ice_info_fw_build(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_fw_build(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
 
-	snprintf(buf, len, "0x%08x", hw->fw_build);
+	snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", hw->fw_build);
 
 	return 0;
 }
 
-static int ice_info_fw_srev(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_fw_srev(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
 
-	snprintf(buf, len, "%u", nvm->srev);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u", nvm->srev);
 
 	return 0;
 }
 
-static int ice_info_orom_ver(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_orom_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_orom_info *orom = &pf->hw.flash.orom;
 
-	snprintf(buf, len, "%u.%u.%u", orom->major, orom->build, orom->patch);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u.%u.%u", orom->major, orom->build, orom->patch);
 
 	return 0;
 }
 
-static int ice_info_orom_srev(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_orom_srev(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_orom_info *orom = &pf->hw.flash.orom;
 
-	snprintf(buf, len, "%u", orom->srev);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u", orom->srev);
 
 	return 0;
 }
 
-static int ice_info_nvm_ver(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_nvm_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
 
-	snprintf(buf, len, "%x.%02x", nvm->major, nvm->minor);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%x.%02x", nvm->major, nvm->minor);
 
 	return 0;
 }
 
-static int ice_info_eetrack(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_eetrack(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
 
-	snprintf(buf, len, "0x%08x", nvm->eetrack);
+	snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", nvm->eetrack);
 
 	return 0;
 }
 
-static int ice_info_ddp_pkg_name(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_ddp_pkg_name(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
 
-	snprintf(buf, len, "%s", hw->active_pkg_name);
+	snprintf(ctx->buf, sizeof(ctx->buf), "%s", hw->active_pkg_name);
 
 	return 0;
 }
 
-static int ice_info_ddp_pkg_version(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_ddp_pkg_version(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_pkg_ver *pkg = &pf->hw.active_pkg_ver;
 
-	snprintf(buf, len, "%u.%u.%u.%u", pkg->major, pkg->minor, pkg->update,
+	snprintf(ctx->buf, sizeof(ctx->buf), "%u.%u.%u.%u", pkg->major, pkg->minor, pkg->update,
 		 pkg->draft);
 
 	return 0;
 }
 
-static int ice_info_ddp_pkg_bundle_id(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_ddp_pkg_bundle_id(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
-	snprintf(buf, len, "0x%08x", pf->hw.active_track_id);
+	snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", pf->hw.active_track_id);
 
 	return 0;
 }
 
-static int ice_info_netlist_ver(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_netlist_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_netlist_info *netlist = &pf->hw.flash.netlist;
 
 	/* The netlist version fields are BCD formatted */
-	snprintf(buf, len, "%x.%x.%x-%x.%x.%x", netlist->major, netlist->minor,
+	snprintf(ctx->buf, sizeof(ctx->buf), "%x.%x.%x-%x.%x.%x", netlist->major, netlist->minor,
 		 netlist->type >> 16, netlist->type & 0xFFFF, netlist->rev,
 		 netlist->cust_ver);
 
 	return 0;
 }
 
-static int ice_info_netlist_build(struct ice_pf *pf, char *buf, size_t len)
+static int ice_info_netlist_build(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_netlist_info *netlist = &pf->hw.flash.netlist;
 
-	snprintf(buf, len, "0x%08x", netlist->hash);
+	snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", netlist->hash);
 
 	return 0;
 }
@@ -160,7 +175,7 @@ enum ice_version_type {
 static const struct ice_devlink_version {
 	enum ice_version_type type;
 	const char *key;
-	int (*getter)(struct ice_pf *pf, char *buf, size_t len);
+	int (*getter)(struct ice_pf *pf, struct ice_info_ctx *ctx);
 } ice_devlink_versions[] = {
 	fixed(DEVLINK_INFO_VERSION_GENERIC_BOARD_ID, ice_info_pba),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_MGMT, ice_info_fw_mgmt),
@@ -194,60 +209,72 @@ static int ice_devlink_info_get(struct devlink *devlink,
 				struct netlink_ext_ack *extack)
 {
 	struct ice_pf *pf = devlink_priv(devlink);
-	char buf[100];
+	struct ice_info_ctx *ctx;
 	size_t i;
 	int err;
 
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
 	err = devlink_info_driver_name_put(req, KBUILD_MODNAME);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to set driver name");
-		return err;
+		goto out_free_ctx;
 	}
 
-	ice_info_get_dsn(pf, buf, sizeof(buf));
+	ice_info_get_dsn(pf, ctx);
 
-	err = devlink_info_serial_number_put(req, buf);
+	err = devlink_info_serial_number_put(req, ctx->buf);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to set serial number");
-		return err;
+		goto out_free_ctx;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(ice_devlink_versions); i++) {
 		enum ice_version_type type = ice_devlink_versions[i].type;
 		const char *key = ice_devlink_versions[i].key;
 
-		err = ice_devlink_versions[i].getter(pf, buf, sizeof(buf));
+		memset(ctx->buf, 0, sizeof(ctx->buf));
+
+		err = ice_devlink_versions[i].getter(pf, ctx);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack, "Unable to obtain version info");
-			return err;
+			goto out_free_ctx;
 		}
 
+		/* Do not report missing versions */
+		if (ctx->buf[0] == '\0')
+			continue;
+
 		switch (type) {
 		case ICE_VERSION_FIXED:
-			err = devlink_info_version_fixed_put(req, key, buf);
+			err = devlink_info_version_fixed_put(req, key, ctx->buf);
 			if (err) {
 				NL_SET_ERR_MSG_MOD(extack, "Unable to set fixed version");
-				return err;
+				goto out_free_ctx;
 			}
 			break;
 		case ICE_VERSION_RUNNING:
-			err = devlink_info_version_running_put(req, key, buf);
+			err = devlink_info_version_running_put(req, key, ctx->buf);
 			if (err) {
 				NL_SET_ERR_MSG_MOD(extack, "Unable to set running version");
-				return err;
+				goto out_free_ctx;
 			}
 			break;
 		case ICE_VERSION_STORED:
-			err = devlink_info_version_stored_put(req, key, buf);
+			err = devlink_info_version_stored_put(req, key, ctx->buf);
 			if (err) {
 				NL_SET_ERR_MSG_MOD(extack, "Unable to set stored version");
-				return err;
+				goto out_free_ctx;
 			}
 			break;
 		}
 	}
 
-	return 0;
+out_free_ctx:
+	kfree(ctx);
+	return err;
 }
 
 enum ice_devlink_param_id {
-- 
2.26.2

[PATCH net-next 07/15] ice: refactor interface for ice_read_flash_module

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice_read_flash_module interface for reading from the various NVM
modules was introduced in commit 682fa08580ac ("ice: read security
revision to ice_nvm_info and ice_orom_info")

It's purpose is two-fold. First, it enables reading data from the CSS
header, used to allow accessing the image security revisions. Second, it
allowed reading from either the 1st or the 2nd NVM bank. This interface
was necessary because the device has two copies of each module. Only one
bank is active at a time, but it could be different for each module. The
driver had to determine which bank was active and then use that to
calculate the offset into the flash to read.

Future plans include allowing access to read not just from the active
flash bank, but also the inactive bank. This will be useful for enabling
display of the version information for a pending flash update.

The current abstraction in ice_read_flash_module is to specify the exact
bank to read. This requires callers to know whether to read from the 1st
or 2nd flash bank. This is the wrong abstraction level, since in most
cases the decision point from a caller's perspective is whether to read
from the active bank or the inactive bank.

Add a new ice_bank_select enumeration, used to indicate whether a flow
wants to read from the active, or inactive flash bank. Refactor
ice_read_flash_module to take this new enumeration instead of a raw
flash bank.

Have ice_read_flash_module select which bank to read from based on the
cached data we load during NVM initialization. With this change, it will
be come easier to implement reading version data from the inactive flash
banks in a future change.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_nvm.c  | 116 +++++++++++++++-------
 drivers/net/ethernet/intel/ice/ice_type.h |   9 ++
 2 files changed, 91 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index ed4d6058a90d..0e949114359c 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -233,6 +233,74 @@ void ice_release_nvm(struct ice_hw *hw)
 	ice_release_res(hw, ICE_NVM_RES_ID);
 }
 
+/**
+ * ice_get_flash_bank_offset - Get offset into requested flash bank
+ * @hw: pointer to the HW structure
+ * @bank: whether to read from the active or inactive flash bank
+ * @module: the module to read from
+ *
+ * Based on the module, lookup the module offset from the beginning of the
+ * flash.
+ *
+ * Returns the flash offset. Note that a value of zero is invalid and must be
+ * treated as an error.
+ */
+static u32 ice_get_flash_bank_offset(struct ice_hw *hw, enum ice_bank_select bank, u16 module)
+{
+	struct ice_bank_info *banks = &hw->flash.banks;
+	enum ice_flash_bank active_bank;
+	bool second_bank_active;
+	u32 offset, size;
+
+	switch (module) {
+	case ICE_SR_1ST_NVM_BANK_PTR:
+		offset = banks->nvm_ptr;
+		size = banks->nvm_size;
+		active_bank = banks->nvm_bank;
+		break;
+	case ICE_SR_1ST_OROM_BANK_PTR:
+		offset = banks->orom_ptr;
+		size = banks->orom_size;
+		active_bank = banks->orom_bank;
+		break;
+	case ICE_SR_NETLIST_BANK_PTR:
+		offset = banks->netlist_ptr;
+		size = banks->netlist_size;
+		active_bank = banks->netlist_bank;
+		break;
+	default:
+		ice_debug(hw, ICE_DBG_NVM, "Unexpected value for flash module: 0x%04x\n", module);
+		return 0;
+	}
+
+	switch (active_bank) {
+	case ICE_1ST_FLASH_BANK:
+		second_bank_active = false;
+		break;
+	case ICE_2ND_FLASH_BANK:
+		second_bank_active = true;
+		break;
+	default:
+		ice_debug(hw, ICE_DBG_NVM, "Unexpected value for active flash bank: %u\n",
+			  active_bank);
+		return 0;
+	}
+
+	/* The second flash bank is stored immediately following the first
+	 * bank. Based on whether the 1st or 2nd bank is active, and whether
+	 * we want the active or inactive bank, calculate the desired offset.
+	 */
+	switch (bank) {
+	case ICE_ACTIVE_FLASH_BANK:
+		return offset + (second_bank_active ? size : 0);
+	case ICE_INACTIVE_FLASH_BANK:
+		return offset + (second_bank_active ? 0 : size);
+	}
+
+	ice_debug(hw, ICE_DBG_NVM, "Unexpected value for flash bank selection: %u\n", bank);
+	return 0;
+}
+
 /**
  * ice_read_flash_module - Read a word from one of the main NVM modules
  * @hw: pointer to the HW structure
@@ -241,47 +309,27 @@ void ice_release_nvm(struct ice_hw *hw)
  * @offset: the offset into the module in words
  * @data: storage for the word read from the flash
  *
- * Read a word from the specified bank of the module. The bank must be either
- * the 1st or 2nd bank. The word will be read using flat NVM access, and
- * relies on the hw->flash.banks data being setup by
- * ice_determine_active_flash_banks() during initialization.
+ * Read a word from the specified flash module. The bank parameter indicates
+ * whether or not to read from the active bank or the inactive bank of that
+ * module.
+ *
+ * The word will be read using flat NVM access, and relies on the
+ * hw->flash.banks data being setup by ice_determine_active_flash_banks()
+ * during initialization.
  */
 static enum ice_status
-ice_read_flash_module(struct ice_hw *hw, enum ice_flash_bank bank, u16 module,
+ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
 		      u32 offset, u16 *data)
 {
-	struct ice_bank_info *banks = &hw->flash.banks;
 	u32 bytes = sizeof(u16);
 	enum ice_status status;
 	__le16 data_local;
-	bool second_bank;
 	u32 start;
 
-	switch (bank) {
-	case ICE_1ST_FLASH_BANK:
-		second_bank = false;
-		break;
-	case ICE_2ND_FLASH_BANK:
-		second_bank = true;
-		break;
-	case ICE_INVALID_FLASH_BANK:
-	default:
-		ice_debug(hw, ICE_DBG_NVM, "Unexpected flash bank %u\n", bank);
-		return ICE_ERR_PARAM;
-	}
-
-	switch (module) {
-	case ICE_SR_1ST_NVM_BANK_PTR:
-		start = banks->nvm_ptr + (second_bank ? banks->nvm_size : 0);
-		break;
-	case ICE_SR_1ST_OROM_BANK_PTR:
-		start = banks->orom_ptr + (second_bank ? banks->orom_size : 0);
-		break;
-	case ICE_SR_NETLIST_BANK_PTR:
-		start = banks->netlist_ptr + (second_bank ? banks->netlist_size : 0);
-		break;
-	default:
-		ice_debug(hw, ICE_DBG_NVM, "Unexpected flash module 0x%04x\n", module);
+	start = ice_get_flash_bank_offset(hw, bank, module);
+	if (!start) {
+		ice_debug(hw, ICE_DBG_NVM, "Unable to calculate flash bank offset for module 0x%04x\n",
+			  module);
 		return ICE_ERR_PARAM;
 	}
 
@@ -311,7 +359,7 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_flash_bank bank, u16 module,
 static enum ice_status
 ice_read_active_nvm_module(struct ice_hw *hw, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, hw->flash.banks.nvm_bank,
+	return ice_read_flash_module(hw, ICE_ACTIVE_FLASH_BANK,
 				     ICE_SR_1ST_NVM_BANK_PTR, offset, data);
 }
 
@@ -328,7 +376,7 @@ ice_read_active_nvm_module(struct ice_hw *hw, u32 offset, u16 *data)
 static enum ice_status
 ice_read_active_orom_module(struct ice_hw *hw, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, hw->flash.banks.orom_bank,
+	return ice_read_flash_module(hw, ICE_ACTIVE_FLASH_BANK,
 				     ICE_SR_1ST_OROM_BANK_PTR, offset, data);
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index f387641195a9..c5a9a6ad2907 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -349,6 +349,15 @@ enum ice_flash_bank {
 	ICE_2ND_FLASH_BANK,
 };
 
+/* Enumeration of which flash bank is desired to read from, either the active
+ * bank or the inactive bank. Used to abstract 1st and 2nd bank notion from
+ * code which just wants to read the active or inactive flash bank.
+ */
+enum ice_bank_select {
+	ICE_ACTIVE_FLASH_BANK,
+	ICE_INACTIVE_FLASH_BANK,
+};
+
 /* information for accessing NVM, OROM, and Netlist flash banks */
 struct ice_bank_info {
 	u32 nvm_ptr;				/* Pointer to 1st NVM bank */
-- 
2.26.2

[PATCH net-next 08/15] ice: allow reading inactive flash security revision

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

Modify ice_get_nvm_srev and ice_get_orom_srev to take the
ice_flash_bank enumeration that specifies whether to read from the
active or the inactive flash module. Rename and refactor the
ice_read_active_nvm_module and ice_read_active_orom_module functions to
take the bank enum value as well.

With this change, ice_get_nvm_srev and ice_get_orom_srev will be usable
in a future change to implement reading the version data for a pending
flash image.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_nvm.c | 36 +++++++++++++-----------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 0e949114359c..88a9e17744f3 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -348,8 +348,9 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
 }
 
 /**
- * ice_read_active_nvm_module - Read from the active main NVM module
+ * ice_read_nvm_module - Read from the active main NVM module
  * @hw: pointer to the HW structure
+ * @bank: whether to read from active or inactive NVM module
  * @offset: offset into the NVM module to read, in words
  * @data: storage for returned word value
  *
@@ -357,15 +358,15 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
  * header at the start of the NVM module.
  */
 static enum ice_status
-ice_read_active_nvm_module(struct ice_hw *hw, u32 offset, u16 *data)
+ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, ICE_ACTIVE_FLASH_BANK,
-				     ICE_SR_1ST_NVM_BANK_PTR, offset, data);
+	return ice_read_flash_module(hw, bank, ICE_SR_1ST_NVM_BANK_PTR, offset, data);
 }
 
 /**
- * ice_read_active_orom_module - Read from the active Option ROM module
+ * ice_read_orom_module - Read from the active Option ROM module
  * @hw: pointer to the HW structure
+ * @bank: whether to read from active or inactive OROM module
  * @offset: offset into the OROM module to read, in words
  * @data: storage for returned word value
  *
@@ -374,10 +375,9 @@ ice_read_active_nvm_module(struct ice_hw *hw, u32 offset, u16 *data)
  * module instead of at the beginning.
  */
 static enum ice_status
-ice_read_active_orom_module(struct ice_hw *hw, u32 offset, u16 *data)
+ice_read_orom_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, ICE_ACTIVE_FLASH_BANK,
-				     ICE_SR_1ST_OROM_BANK_PTR, offset, data);
+	return ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR, offset, data);
 }
 
 /**
@@ -529,21 +529,22 @@ ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size)
 /**
  * ice_get_nvm_srev - Read the security revision from the NVM CSS header
  * @hw: pointer to the HW struct
+ * @bank: whether to read from the active or inactive flash bank
  * @srev: storage for security revision
  *
  * Read the security revision out of the CSS header of the active NVM module
  * bank.
  */
-static enum ice_status ice_get_nvm_srev(struct ice_hw *hw, u32 *srev)
+static enum ice_status ice_get_nvm_srev(struct ice_hw *hw, enum ice_bank_select bank, u32 *srev)
 {
 	enum ice_status status;
 	u16 srev_l, srev_h;
 
-	status = ice_read_active_nvm_module(hw, ICE_NVM_CSS_SREV_L, &srev_l);
+	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_SREV_L, &srev_l);
 	if (status)
 		return status;
 
-	status = ice_read_active_nvm_module(hw, ICE_NVM_CSS_SREV_H, &srev_h);
+	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_SREV_H, &srev_h);
 	if (status)
 		return status;
 
@@ -587,7 +588,7 @@ ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
 
 	nvm->eetrack = (eetrack_hi << 16) | eetrack_lo;
 
-	status = ice_get_nvm_srev(hw, &nvm->srev);
+	status = ice_get_nvm_srev(hw, ICE_ACTIVE_FLASH_BANK, &nvm->srev);
 	if (status)
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read NVM security revision.\n");
 
@@ -597,12 +598,13 @@ ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
 /**
  * ice_get_orom_srev - Read the security revision from the OROM CSS header
  * @hw: pointer to the HW struct
+ * @bank: whether to read from active or inactive flash module
  * @srev: storage for security revision
  *
  * Read the security revision out of the CSS header of the active OROM module
  * bank.
  */
-static enum ice_status ice_get_orom_srev(struct ice_hw *hw, u32 *srev)
+static enum ice_status ice_get_orom_srev(struct ice_hw *hw, enum ice_bank_select bank, u32 *srev)
 {
 	enum ice_status status;
 	u16 srev_l, srev_h;
@@ -615,16 +617,16 @@ static enum ice_status ice_get_orom_srev(struct ice_hw *hw, u32 *srev)
 	}
 
 	/* calculate how far into the Option ROM the CSS header starts. Note
-	 * that ice_read_active_orom_module takes a word offset so we need to
+	 * that ice_read_orom_module takes a word offset so we need to
 	 * divide by 2 here.
 	 */
 	css_start = (hw->flash.banks.orom_size - ICE_NVM_OROM_TRAILER_LENGTH) / 2;
 
-	status = ice_read_active_orom_module(hw, css_start + ICE_NVM_CSS_SREV_L, &srev_l);
+	status = ice_read_orom_module(hw, bank, css_start + ICE_NVM_CSS_SREV_L, &srev_l);
 	if (status)
 		return status;
 
-	status = ice_read_active_orom_module(hw, css_start + ICE_NVM_CSS_SREV_H, &srev_h);
+	status = ice_read_orom_module(hw, bank, css_start + ICE_NVM_CSS_SREV_H, &srev_h);
 	if (status)
 		return status;
 
@@ -685,7 +687,7 @@ ice_get_orom_ver_info(struct ice_hw *hw, struct ice_orom_info *orom)
 	orom->build = (u16)((combo_ver & ICE_OROM_VER_BUILD_MASK) >>
 			    ICE_OROM_VER_BUILD_SHIFT);
 
-	status = ice_get_orom_srev(hw, &orom->srev);
+	status = ice_get_orom_srev(hw, ICE_ACTIVE_FLASH_BANK, &orom->srev);
 	if (status)
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read Option ROM security revision.\n");
 
-- 
2.26.2

[PATCH net-next 09/15] ice: allow reading arbitrary size data with read_flash_module

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

Refactor ice_read_flash_module so that it takes a size and a length
value, rather than always reading in 2-byte increments. The
ice_read_nvm_module and ice_read_orom_module wrapper functions will
still read a u16 with the byte-swapping enabled.

This will be used in a future change to implement reading of the CIVD
data from the Option ROM module.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_nvm.c | 34 ++++++++++++++++--------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 88a9e17744f3..ff99815402d1 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -306,10 +306,11 @@ static u32 ice_get_flash_bank_offset(struct ice_hw *hw, enum ice_bank_select ban
  * @hw: pointer to the HW structure
  * @bank: which bank of the module to read
  * @module: the module to read
- * @offset: the offset into the module in words
+ * @offset: the offset into the module in bytes
  * @data: storage for the word read from the flash
+ * @length: bytes of data to read
  *
- * Read a word from the specified flash module. The bank parameter indicates
+ * Read data from the specified flash module. The bank parameter indicates
  * whether or not to read from the active bank or the inactive bank of that
  * module.
  *
@@ -319,11 +320,9 @@ static u32 ice_get_flash_bank_offset(struct ice_hw *hw, enum ice_bank_select ban
  */
 static enum ice_status
 ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
-		      u32 offset, u16 *data)
+		      u32 offset, u8 *data, u32 length)
 {
-	u32 bytes = sizeof(u16);
 	enum ice_status status;
-	__le16 data_local;
 	u32 start;
 
 	start = ice_get_flash_bank_offset(hw, bank, module);
@@ -337,10 +336,7 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
 	if (status)
 		return status;
 
-	status = ice_read_flat_nvm(hw, start + offset * sizeof(u16), &bytes,
-				   (__force u8 *)&data_local, false);
-	if (!status)
-		*data = le16_to_cpu(data_local);
+	status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
 
 	ice_release_nvm(hw);
 
@@ -360,7 +356,15 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
 static enum ice_status
 ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, bank, ICE_SR_1ST_NVM_BANK_PTR, offset, data);
+	enum ice_status status;
+	__le16 data_local;
+
+	status = ice_read_flash_module(hw, bank, ICE_SR_1ST_NVM_BANK_PTR, offset * sizeof(u16),
+				       (__force u8 *)&data_local, sizeof(u16));
+	if (!status)
+		*data = le16_to_cpu(data_local);
+
+	return status;
 }
 
 /**
@@ -377,7 +381,15 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
 static enum ice_status
 ice_read_orom_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
 {
-	return ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR, offset, data);
+	enum ice_status status;
+	__le16 data_local;
+
+	status = ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR, offset * sizeof(u16),
+				       (__force u8 *)&data_local, sizeof(u16));
+	if (!status)
+		*data = le16_to_cpu(data_local);
+
+	return status;
 }
 
 /**
-- 
2.26.2

[PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The devlink info interface supports drivers reporting "stored" versions.
These versions indicate the version of an update that has been
downloaded to the device, but is not yet active.

Add a new function to read some of the fw.mgmt version data from the
inactive flash section. This function, ice_get_inactive_nvm_ver, will
read the NVM version data from the inactive section of flash.

To avoid code duplication, we refactor ice_get_nvm_ver_info so that it
takes the bank parameter for specifying which flash bank to read from.
Instead of reading from the copy stored in the Shadow RAM, always read
from the copy of the specified flash bank.

Note that the start of the Shadow RAM copy is not directly following the
CSS header, but is actually aligned to the next 64-byte boundary. The
correct word offset must be rounded up to 32-bytes.

When reporting the versions via devlink info, first read the device
capabilities. If there is a pending flash update, use this new function
to extract the inactive flash versions. Add the stored fields to the
flash version map structure so that they will be displayed when
available.

It should be noted that it is not currently feasible to extract all of
the related versions for the management firmware. This patch adds
support for displaying "fw.mgmt.srev", "fw.psid.api", and
"fw.bundle_id". The management firmware versions are more difficult to
extract from the binary and have not been implemented in this change.

Future changes will introduce support for reading the UNDI Option ROM
version and the version associated with the Netlist module.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 60 ++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_nvm.c     | 44 ++++++++++++--
 drivers/net/ethernet/intel/ice/ice_nvm.h     |  2 +
 drivers/net/ethernet/intel/ice/ice_type.h    |  8 ++-
 4 files changed, 107 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 917fdbb20b7b..377095774ddb 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -9,6 +9,8 @@
 /* context for devlink info version reporting */
 struct ice_info_ctx {
 	char buf[128];
+	struct ice_nvm_info pending_nvm;
+	struct ice_hw_dev_caps dev_caps;
 };
 
 /* The following functions are used to format specific strings for various
@@ -80,6 +82,17 @@ static int ice_info_fw_srev(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_fw_srev(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_nvm_info *nvm = &ctx->pending_nvm;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm)
+		snprintf(ctx->buf, sizeof(ctx->buf), "%u", nvm->srev);
+
+	return 0;
+}
+
 static int ice_info_orom_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_orom_info *orom = &pf->hw.flash.orom;
@@ -107,6 +120,17 @@ static int ice_info_nvm_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_nvm_ver(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_nvm_info *nvm = &ctx->pending_nvm;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm)
+		snprintf(ctx->buf, sizeof(ctx->buf), "%x.%02x", nvm->major, nvm->minor);
+
+	return 0;
+}
+
 static int ice_info_eetrack(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
@@ -116,6 +140,17 @@ static int ice_info_eetrack(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_eetrack(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_nvm_info *nvm = &ctx->pending_nvm;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm)
+		snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", nvm->eetrack);
+
+	return 0;
+}
+
 static int ice_info_ddp_pkg_name(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_hw *hw = &pf->hw;
@@ -165,6 +200,7 @@ static int ice_info_netlist_build(struct ice_pf *pf, struct ice_info_ctx *ctx)
 
 #define fixed(key, getter) { ICE_VERSION_FIXED, key, getter }
 #define running(key, getter) { ICE_VERSION_RUNNING, key, getter }
+#define stored(key, getter) { ICE_VERSION_STORED, key, getter }
 
 enum ice_version_type {
 	ICE_VERSION_FIXED,
@@ -182,10 +218,13 @@ static const struct ice_devlink_version {
 	running("fw.mgmt.api", ice_info_fw_api),
 	running("fw.mgmt.build", ice_info_fw_build),
 	running("fw.mgmt.srev", ice_info_fw_srev),
+	stored("fw.mgmt.srev", ice_info_pending_fw_srev),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_UNDI, ice_info_orom_ver),
 	running("fw.undi.srev", ice_info_orom_srev),
 	running("fw.psid.api", ice_info_nvm_ver),
+	stored("fw.psid.api", ice_info_pending_nvm_ver),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, ice_info_eetrack),
+	stored(DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, ice_info_pending_eetrack),
 	running("fw.app.name", ice_info_ddp_pkg_name),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_APP, ice_info_ddp_pkg_version),
 	running("fw.app.bundle_id", ice_info_ddp_pkg_bundle_id),
@@ -209,7 +248,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
 				struct netlink_ext_ack *extack)
 {
 	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_hw *hw = &pf->hw;
 	struct ice_info_ctx *ctx;
+	enum ice_status status;
 	size_t i;
 	int err;
 
@@ -217,6 +259,24 @@ static int ice_devlink_info_get(struct devlink *devlink,
 	if (!ctx)
 		return -ENOMEM;
 
+	/* discover capabilities first */
+	status = ice_discover_dev_caps(hw, &ctx->dev_caps);
+	if (status) {
+		err = -EIO;
+		goto out_free_ctx;
+	}
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm) {
+		status = ice_get_inactive_nvm_ver(hw, &ctx->pending_nvm);
+		if (status) {
+			dev_dbg(dev, "Unable to read inactive NVM version data, status %s aq_err %s\n",
+				ice_stat_str(status), ice_aq_str(hw->adminq.sq_last_status));
+
+			/* disable display of pending Option ROM */
+			ctx->dev_caps.common_cap.nvm_update_pending_nvm = false;
+		}
+	}
+
 	err = devlink_info_driver_name_put(req, KBUILD_MODNAME);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to set driver name");
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index ff99815402d1..9613d24eaa06 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -367,6 +367,22 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
 	return status;
 }
 
+/**
+ * ice_read_nvm_sr_copy - Read a word from the Shadow RAM copy in the NVM bank
+ * @hw: pointer to the HW structure
+ * @bank: whether to read from the active or inactive NVM module
+ * @offset: offset into the Shadow RAM copy to read, in words
+ * @data: storage for returned word value
+ *
+ * Read the specified word from the copy of the Shadow RAM found in the
+ * specified NVM module.
+ */
+static enum ice_status
+ice_read_nvm_sr_copy(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
+{
+	return ice_read_nvm_module(hw, bank, ICE_NVM_SR_COPY_WORD_OFFSET + offset, data);
+}
+
 /**
  * ice_read_orom_module - Read from the active Option ROM module
  * @hw: pointer to the HW structure
@@ -568,31 +584,33 @@ static enum ice_status ice_get_nvm_srev(struct ice_hw *hw, enum ice_bank_select
 /**
  * ice_get_nvm_ver_info - Read NVM version information
  * @hw: pointer to the HW struct
+ * @bank: whether to read from the active or inactive flash bank
  * @nvm: pointer to NVM info structure
  *
  * Read the NVM EETRACK ID and map version of the main NVM image bank, filling
  * in the nvm info structure.
  */
 static enum ice_status
-ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
+ice_get_nvm_ver_info(struct ice_hw *hw, enum ice_bank_select bank, struct ice_nvm_info *nvm)
 {
 	u16 eetrack_lo, eetrack_hi, ver;
 	enum ice_status status;
 
-	status = ice_read_sr_word(hw, ICE_SR_NVM_DEV_STARTER_VER, &ver);
+	status = ice_read_nvm_sr_copy(hw, bank, ICE_SR_NVM_DEV_STARTER_VER, &ver);
 	if (status) {
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read DEV starter version.\n");
 		return status;
 	}
+
 	nvm->major = (ver & ICE_NVM_VER_HI_MASK) >> ICE_NVM_VER_HI_SHIFT;
 	nvm->minor = (ver & ICE_NVM_VER_LO_MASK) >> ICE_NVM_VER_LO_SHIFT;
 
-	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_LO, &eetrack_lo);
+	status = ice_read_nvm_sr_copy(hw, bank, ICE_SR_NVM_EETRACK_LO, &eetrack_lo);
 	if (status) {
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read EETRACK lo.\n");
 		return status;
 	}
-	status = ice_read_sr_word(hw, ICE_SR_NVM_EETRACK_HI, &eetrack_hi);
+	status = ice_read_nvm_sr_copy(hw, bank, ICE_SR_NVM_EETRACK_HI, &eetrack_hi);
 	if (status) {
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read EETRACK hi.\n");
 		return status;
@@ -600,13 +618,27 @@ ice_get_nvm_ver_info(struct ice_hw *hw, struct ice_nvm_info *nvm)
 
 	nvm->eetrack = (eetrack_hi << 16) | eetrack_lo;
 
-	status = ice_get_nvm_srev(hw, ICE_ACTIVE_FLASH_BANK, &nvm->srev);
+	status = ice_get_nvm_srev(hw, bank, &nvm->srev);
 	if (status)
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read NVM security revision.\n");
 
 	return 0;
 }
 
+/**
+ * ice_get_inactive_nvm_ver - Read Option ROM version from the inactive bank
+ * @hw: pointer to the HW structure
+ * @nvm: storage for Option ROM version information
+ *
+ * Reads the NVM EETRACK ID, Map version, and security revision of the
+ * inactive NVM bank. Used to access version data for a pending update that
+ * has not yet been activated.
+ */
+enum ice_status ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm)
+{
+	return ice_get_nvm_ver_info(hw, ICE_INACTIVE_FLASH_BANK, nvm);
+}
+
 /**
  * ice_get_orom_srev - Read the security revision from the OROM CSS header
  * @hw: pointer to the HW struct
@@ -1028,7 +1060,7 @@ enum ice_status ice_init_nvm(struct ice_hw *hw)
 		return status;
 	}
 
-	status = ice_get_nvm_ver_info(hw, &flash->nvm);
+	status = ice_get_nvm_ver_info(hw, ICE_ACTIVE_FLASH_BANK, &flash->nvm);
 	if (status) {
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read NVM info.\n");
 		return status;
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 8cfb9b9ac638..c5c737b7b062 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -18,6 +18,8 @@ ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 enum ice_status
 ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 enum ice_status
+ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm);
+enum ice_status
 ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size);
 enum ice_status ice_init_nvm(struct ice_hw *hw);
 enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data);
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index c5a9a6ad2907..c2fc12681bb3 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -843,8 +843,14 @@ struct ice_hw_port_stats {
 #define ICE_NVM_CSS_SREV_L			0x14
 #define ICE_NVM_CSS_SREV_H			0x15
 
+/* Length of CSS header section in words */
+#define ICE_CSS_HEADER_LENGTH			330
+
+/* Offset of Shadow RAM copy in the NVM bank area. */
+#define ICE_NVM_SR_COPY_WORD_OFFSET		roundup(ICE_CSS_HEADER_LENGTH, 32)
+
 /* Size in bytes of Option ROM trailer */
-#define ICE_NVM_OROM_TRAILER_LENGTH		660
+#define ICE_NVM_OROM_TRAILER_LENGTH		(2 * ICE_CSS_HEADER_LENGTH)
 
 /* Auxiliary field, mask, and shift definition for Shadow RAM and NVM Flash */
 #define ICE_SR_CTRL_WORD_1_S		0x06
-- 
2.26.2

Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Jakub Kicinski]

On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:
> When reporting the versions via devlink info, first read the device
> capabilities. If there is a pending flash update, use this new function
> to extract the inactive flash versions. Add the stored fields to the
> flash version map structure so that they will be displayed when
> available.

Why only report them when there is an update pending?

The expectation was that you'd always report what you can and user 
can tell the update is pending by comparing the fields.

RE: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, January 29, 2021 10:38 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; Keller, Jacob E <jacob.e.keller@intel.com>;
> netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX
> <tonyx.brelinski@intel.com>
> Subject: Re: [PATCH net-next 10/15] ice: display some stored NVM versions via
> devlink info
> 
> On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:
> > When reporting the versions via devlink info, first read the device
> > capabilities. If there is a pending flash update, use this new function
> > to extract the inactive flash versions. Add the stored fields to the
> > flash version map structure so that they will be displayed when
> > available.
> 
> Why only report them when there is an update pending?
> 
> The expectation was that you'd always report what you can and user
> can tell the update is pending by comparing the fields.

The data in the inactive bank might not be a valid image. There's no straightforward way to verify this except by detecting that we're about to switch banks on the next reboot. If we report this information all the time, in some cases it would be reporting numbers which are meaningless and not actually valid version information. I had assumed this would lead to more confusion than only reporting the data when the bank has data we know is going to be activated soon

Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Jacob Keller]

On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
> On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:
>> When reporting the versions via devlink info, first read the device
>> capabilities. If there is a pending flash update, use this new function
>> to extract the inactive flash versions. Add the stored fields to the
>> flash version map structure so that they will be displayed when
>> available.
> 
> Why only report them when there is an update pending?
> 
> The expectation was that you'd always report what you can and user 
> can tell the update is pending by comparing the fields.
> 

If there is no pending update, what is the expected behavior? We report
the currently active image version as both stored and running?

In our case, the device has 2 copies of each of the 3 modules: NVM,
Netlist, and UNDI/OptionROM.

For each module, the device has a bit that indicates whether it will
boot from the first or second bank of the image. When we update,
whichever bank is not active is erased, and then populated with the new
image contents. The bit indicating which bank to load is flipped. Once
the device is rebooted (EMP reset), then the new bank is loaded, and the
firmware performs some onetime initialization.

So for us, in theory we have up to 2 versions within the device for each
bank: the version in the currently active bank, and a version in the
inactive bank. In the inactive case, it may or may not be valid
depending on if that banks contents were ever a valid image. On a fresh
card, this might be empty or filled with garbage.

Presumably we do not want to report that we have "stored" a version
which is not going to be activated next time that we boot?

The documentation indicated that stored should be the version which
*will* be activated.

If I just blindly always reported what was inactive, then the following
scenarios exist:

# Brand new card:

running:
  fw.bundle_id: Version
stored
  fw.bundle_id: <zero or garbage>

# Do an update:

running:
  fw.bundle_id: Version
stored
  fw.bundle_id: NewVersion

# reset/reboot

running:
  fw.bundle_id: NewVersion
stored:
  fw.bundle_id: Version


I could get behind that if we do not have a pending update we report the
stored value as the same as the running value (i.e. from the active
bank), where as if we have a pending update that will be triggered we
would report the inactive bank. I didn't see the value in that before
because it seemed like "if you don't have a pending update, you don't
have a stored value, so just report the active version in the running
category")

It's also plausibly useful to report the stored but not pending value in
some cases, but I really don't want to report zeros or garbage data on
accident. This would almost certainly lead to confusing support
conversations.

Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Jakub Kicinski]

On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:
> On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
> > On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  
> >> When reporting the versions via devlink info, first read the device
> >> capabilities. If there is a pending flash update, use this new function
> >> to extract the inactive flash versions. Add the stored fields to the
> >> flash version map structure so that they will be displayed when
> >> available.  
> > 
> > Why only report them when there is an update pending?
> > 
> > The expectation was that you'd always report what you can and user 
> > can tell the update is pending by comparing the fields.
> 
> If there is no pending update, what is the expected behavior? We report
> the currently active image version as both stored and running?
> 
> In our case, the device has 2 copies of each of the 3 modules: NVM,
> Netlist, and UNDI/OptionROM.
> 
> For each module, the device has a bit that indicates whether it will
> boot from the first or second bank of the image. When we update,
> whichever bank is not active is erased, and then populated with the new
> image contents. The bit indicating which bank to load is flipped. Once
> the device is rebooted (EMP reset), then the new bank is loaded, and the
> firmware performs some onetime initialization.
> 
> So for us, in theory we have up to 2 versions within the device for each
> bank: the version in the currently active bank, and a version in the
> inactive bank. In the inactive case, it may or may not be valid
> depending on if that banks contents were ever a valid image. On a fresh
> card, this might be empty or filled with garbage.
> 
> Presumably we do not want to report that we have "stored" a version
> which is not going to be activated next time that we boot?
> 
> The documentation indicated that stored should be the version which
> *will* be activated.
> 
> If I just blindly always reported what was inactive, then the following
> scenarios exist:
> 
> # Brand new card:
> 
> running:
>   fw.bundle_id: Version
> stored
>   fw.bundle_id: <zero or garbage>
> 
> # Do an update:
> 
> running:
>   fw.bundle_id: Version
> stored
>   fw.bundle_id: NewVersion
> 
> # reset/reboot
> 
> running:
>   fw.bundle_id: NewVersion
> stored:
>   fw.bundle_id: Version
> 
> 
> I could get behind that if we do not have a pending update we report the
> stored value as the same as the running value (i.e. from the active
> bank), where as if we have a pending update that will be triggered we
> would report the inactive bank. I didn't see the value in that before
> because it seemed like "if you don't have a pending update, you don't
> have a stored value, so just report the active version in the running
> category")
> 
> It's also plausibly useful to report the stored but not pending value in
> some cases, but I really don't want to report zeros or garbage data on
> accident. This would almost certainly lead to confusing support
> conversations.

Very good points. Please see the documentation for example workflow:

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management

The FW update agent should be able to rely on 'stored' for checking if
flash update is needed.

If the FW update is not pending just report the same values as running.
You should not report old version after 2 flashings (3rd output in your
example) - that'd confuse the flow - as you said - the stored versions
would not be what will get activated.

Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Jacob Keller]

On 2/1/2021 2:34 PM, Jakub Kicinski wrote:
> On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:
>> On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  
>>>> When reporting the versions via devlink info, first read the device
>>>> capabilities. If there is a pending flash update, use this new function
>>>> to extract the inactive flash versions. Add the stored fields to the
>>>> flash version map structure so that they will be displayed when
>>>> available.  
>>>
>>> Why only report them when there is an update pending?
>>>
>>> The expectation was that you'd always report what you can and user 
>>> can tell the update is pending by comparing the fields.
>>
>> If there is no pending update, what is the expected behavior? We report
>> the currently active image version as both stored and running?
>>
>> In our case, the device has 2 copies of each of the 3 modules: NVM,
>> Netlist, and UNDI/OptionROM.
>>
>> For each module, the device has a bit that indicates whether it will
>> boot from the first or second bank of the image. When we update,
>> whichever bank is not active is erased, and then populated with the new
>> image contents. The bit indicating which bank to load is flipped. Once
>> the device is rebooted (EMP reset), then the new bank is loaded, and the
>> firmware performs some onetime initialization.
>>
>> So for us, in theory we have up to 2 versions within the device for each
>> bank: the version in the currently active bank, and a version in the
>> inactive bank. In the inactive case, it may or may not be valid
>> depending on if that banks contents were ever a valid image. On a fresh
>> card, this might be empty or filled with garbage.
>>
>> Presumably we do not want to report that we have "stored" a version
>> which is not going to be activated next time that we boot?
>>
>> The documentation indicated that stored should be the version which
>> *will* be activated.
>>
>> If I just blindly always reported what was inactive, then the following
>> scenarios exist:
>>
>> # Brand new card:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: <zero or garbage>
>>
>> # Do an update:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: NewVersion
>>
>> # reset/reboot
>>
>> running:
>>   fw.bundle_id: NewVersion
>> stored:
>>   fw.bundle_id: Version
>>
>>
>> I could get behind that if we do not have a pending update we report the
>> stored value as the same as the running value (i.e. from the active
>> bank), where as if we have a pending update that will be triggered we
>> would report the inactive bank. I didn't see the value in that before
>> because it seemed like "if you don't have a pending update, you don't
>> have a stored value, so just report the active version in the running
>> category")
>>
>> It's also plausibly useful to report the stored but not pending value in
>> some cases, but I really don't want to report zeros or garbage data on
>> accident. This would almost certainly lead to confusing support
>> conversations.
> 
> Very good points. Please see the documentation for example workflow:
> 
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
> 
> The FW update agent should be able to rely on 'stored' for checking if
> flash update is needed.
> 
> If the FW update is not pending just report the same values as running.
> You should not report old version after 2 flashings (3rd output in your
> example) - that'd confuse the flow - as you said - the stored versions
> would not be what will get activated.
> 

Sure, ok. I can add that when implementing this in the v2 submission
(along with extracting the security revision patches to a separate series).

Thanks,
Jake

RE: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Brelinski, TonyX]

From: Jacob Keller <jacob.e.keller@intel.com> 
Sent: Monday, February 1, 2021 3:09 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com>
Subject: Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info



On 2/1/2021 2:34 PM, Jakub Kicinski wrote:
> On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:
>> On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  
>>>> When reporting the versions via devlink info, first read the device 
>>>> capabilities. If there is a pending flash update, use this new 
>>>> function to extract the inactive flash versions. Add the stored 
>>>> fields to the flash version map structure so that they will be 
>>>> displayed when available.
>>>
>>> Why only report them when there is an update pending?
>>>
>>> The expectation was that you'd always report what you can and user 
>>> can tell the update is pending by comparing the fields.
>>
>> If there is no pending update, what is the expected behavior? We 
>> report the currently active image version as both stored and running?
>>
>> In our case, the device has 2 copies of each of the 3 modules: NVM, 
>> Netlist, and UNDI/OptionROM.
>>
>> For each module, the device has a bit that indicates whether it will 
>> boot from the first or second bank of the image. When we update, 
>> whichever bank is not active is erased, and then populated with the 
>> new image contents. The bit indicating which bank to load is flipped. 
>> Once the device is rebooted (EMP reset), then the new bank is loaded, 
>> and the firmware performs some onetime initialization.
>>
>> So for us, in theory we have up to 2 versions within the device for 
>> each
>> bank: the version in the currently active bank, and a version in the 
>> inactive bank. In the inactive case, it may or may not be valid 
>> depending on if that banks contents were ever a valid image. On a 
>> fresh card, this might be empty or filled with garbage.
>>
>> Presumably we do not want to report that we have "stored" a version 
>> which is not going to be activated next time that we boot?
>>
>> The documentation indicated that stored should be the version which
>> *will* be activated.
>>
>> If I just blindly always reported what was inactive, then the 
>> following scenarios exist:
>>
>> # Brand new card:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: <zero or garbage>
>>
>> # Do an update:
>>
>> running:
>>   fw.bundle_id: Version
>> stored
>>   fw.bundle_id: NewVersion
>>
>> # reset/reboot
>>
>> running:
>>   fw.bundle_id: NewVersion
>> stored:
>>   fw.bundle_id: Version
>>
>>
>> I could get behind that if we do not have a pending update we report 
>> the stored value as the same as the running value (i.e. from the 
>> active bank), where as if we have a pending update that will be 
>> triggered we would report the inactive bank. I didn't see the value 
>> in that before because it seemed like "if you don't have a pending 
>> update, you don't have a stored value, so just report the active 
>> version in the running
>> category")
>>
>> It's also plausibly useful to report the stored but not pending value 
>> in some cases, but I really don't want to report zeros or garbage 
>> data on accident. This would almost certainly lead to confusing 
>> support conversations.
> 
> Very good points. Please see the documentation for example workflow:
> 
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flas
> h.html#firmware-version-management
> 
> The FW update agent should be able to rely on 'stored' for checking if 
> flash update is needed.
> 
> If the FW update is not pending just report the same values as running.
> You should not report old version after 2 flashings (3rd output in 
> your
> example) - that'd confuse the flow - as you said - the stored versions 
> would not be what will get activated.
> 

Sure, ok. I can add that when implementing this in the v2 submission (along with extracting the security revision patches to a separate series).

Thanks,
Jake

------------------------------------------------------------------
I tested this revision:
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com> A Contingent Worker at Intel

[PATCH net-next 11/15] ice: display stored netlist versions via devlink info

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

Add a function to read the inactive netlist bank for version
information. To support this, refactor how we read the netlist version
data. Instead of using the firmware AQ interface with a module ID, read
from the flash as a flat NVM, using ice_read_flash_module.

This change requires a slight adjustment to the offset values used, as
reading from the flat NVM includes the type field (which was stripped by
firmware previously). Cleanup the macro names and move them to
ice_type.h. For clarity in how we calculate the offsets and so that
programmers can easily map the offset value to the data sheet, use
a wrapper macro to account for the offset adjustments.

Use the newly added ice_get_inactive_netlist_ver function to extract the
version data from the pending netlist module update. Add the stored
variants of "fw.netlist", and "fw.netlist.build" to the info version map
array.

With this change, we now report the pending netlist module version if we
detect a pending but not complete netlist update when reporting firmware
versions.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  27 ---
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  40 +++++
 drivers/net/ethernet/intel/ice/ice_main.c     |   2 +
 drivers/net/ethernet/intel/ice/ice_nvm.c      | 160 +++++++++++-------
 drivers/net/ethernet/intel/ice/ice_nvm.h      |   2 +
 drivers/net/ethernet/intel/ice/ice_status.h   |   1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |  35 ++++
 7 files changed, 176 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 40c96662458a..78d9b96cc743 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1336,33 +1336,6 @@ struct ice_aqc_nvm_checksum {
 
 #define ICE_AQC_NVM_MINSREV_MOD_ID		0x130
 
-/* The result of netlist NVM read comes in a TLV format. The actual data
- * (netlist header) starts from word offset 1 (byte 2). The FW strips
- * out the type field from the TLV header so all the netlist fields
- * should adjust their offset value by 1 word (2 bytes) in order to map
- * their correct location.
- */
-#define ICE_AQC_NVM_LINK_TOPO_NETLIST_MOD_ID		0x11B
-#define ICE_AQC_NVM_LINK_TOPO_NETLIST_LEN_OFFSET	1
-#define ICE_AQC_NVM_LINK_TOPO_NETLIST_LEN		2 /* In bytes */
-#define ICE_AQC_NVM_NETLIST_NODE_COUNT_OFFSET		2
-#define ICE_AQC_NVM_NETLIST_NODE_COUNT_LEN		2 /* In bytes */
-#define ICE_AQC_NVM_NETLIST_NODE_COUNT_M		ICE_M(0x3FF, 0)
-#define ICE_AQC_NVM_NETLIST_ID_BLK_START_OFFSET		5
-#define ICE_AQC_NVM_NETLIST_ID_BLK_LEN			0x30 /* In words */
-
-/* netlist ID block field offsets (word offsets) */
-#define ICE_AQC_NVM_NETLIST_ID_BLK_MAJOR_VER_LOW	2
-#define ICE_AQC_NVM_NETLIST_ID_BLK_MAJOR_VER_HIGH	3
-#define ICE_AQC_NVM_NETLIST_ID_BLK_MINOR_VER_LOW	4
-#define ICE_AQC_NVM_NETLIST_ID_BLK_MINOR_VER_HIGH	5
-#define ICE_AQC_NVM_NETLIST_ID_BLK_TYPE_LOW		6
-#define ICE_AQC_NVM_NETLIST_ID_BLK_TYPE_HIGH		7
-#define ICE_AQC_NVM_NETLIST_ID_BLK_REV_LOW		8
-#define ICE_AQC_NVM_NETLIST_ID_BLK_REV_HIGH		9
-#define ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH		0xA
-#define ICE_AQC_NVM_NETLIST_ID_BLK_CUST_VER		0x2F
-
 /* Used for reading and writing MinSRev using 0x0701 and 0x0703. Note that the
  * type field is excluded from the section when reading and writing from
  * a module using the module_typeid field with these AQ commands.
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 377095774ddb..2b47e1fbccfb 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -10,6 +10,7 @@
 struct ice_info_ctx {
 	char buf[128];
 	struct ice_nvm_info pending_nvm;
+	struct ice_netlist_info pending_netlist;
 	struct ice_hw_dev_caps dev_caps;
 };
 
@@ -198,6 +199,32 @@ static int ice_info_netlist_build(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_netlist_ver(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_netlist_info *netlist = &ctx->pending_netlist;
+
+	/* The netlist version fields are BCD formatted */
+	if (ctx->dev_caps.common_cap.nvm_update_pending_netlist)
+		snprintf(ctx->buf, sizeof(ctx->buf), "%x.%x.%x-%x.%x.%x",
+			 netlist->major, netlist->minor,
+			 netlist->type >> 16, netlist->type & 0xFFFF, netlist->rev,
+			 netlist->cust_ver);
+
+	return 0;
+}
+
+static int
+ice_info_pending_netlist_build(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_netlist_info *netlist = &ctx->pending_netlist;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_netlist)
+		snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", netlist->hash);
+
+	return 0;
+}
+
 #define fixed(key, getter) { ICE_VERSION_FIXED, key, getter }
 #define running(key, getter) { ICE_VERSION_RUNNING, key, getter }
 #define stored(key, getter) { ICE_VERSION_STORED, key, getter }
@@ -229,7 +256,9 @@ static const struct ice_devlink_version {
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_APP, ice_info_ddp_pkg_version),
 	running("fw.app.bundle_id", ice_info_ddp_pkg_bundle_id),
 	running("fw.netlist", ice_info_netlist_ver),
+	stored("fw.netlist", ice_info_pending_netlist_ver),
 	running("fw.netlist.build", ice_info_netlist_build),
+	stored("fw.netlist.build", ice_info_pending_netlist_build),
 };
 
 /**
@@ -277,6 +306,17 @@ static int ice_devlink_info_get(struct devlink *devlink,
 		}
 	}
 
+	if (ctx->dev_caps.common_cap.nvm_update_pending_netlist) {
+		status = ice_get_inactive_netlist_ver(hw, &ctx->pending_netlist);
+		if (status) {
+			dev_dbg(dev, "Unable to read inactive Netlist version data, status %s aq_err %s\n",
+				ice_stat_str(status), ice_aq_str(hw->adminq.sq_last_status));
+
+			/* disable display of pending Option ROM */
+			ctx->dev_caps.common_cap.nvm_update_pending_netlist = false;
+		}
+	}
+
 	err = devlink_info_driver_name_put(req, KBUILD_MODNAME);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to set driver name");
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 66a40dfadb6a..5219aa70b530 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -6251,6 +6251,8 @@ const char *ice_stat_str(enum ice_status stat_err)
 		return "ICE_ERR_OUT_OF_RANGE";
 	case ICE_ERR_ALREADY_EXISTS:
 		return "ICE_ERR_ALREADY_EXISTS";
+	case ICE_ERR_NVM:
+		return "ICE_ERR_NVM";
 	case ICE_ERR_NVM_CHECKSUM:
 		return "ICE_ERR_NVM_CHECKSUM";
 	case ICE_ERR_BUF_TOO_SHORT:
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 9613d24eaa06..8bc2df09a11a 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -408,6 +408,29 @@ ice_read_orom_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u
 	return status;
 }
 
+/**
+ * ice_read_netlist_module - Read data from the netlist module area
+ * @hw: pointer to the HW structure
+ * @bank: whether to read from the active or inactive module
+ * @offset: offset into the netlist to read from
+ * @data: storage for returned word value
+ *
+ * Read a word from the specified netlist bank.
+ */
+static enum ice_status
+ice_read_netlist_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
+{
+	enum ice_status status;
+	__le16 data_local;
+
+	status = ice_read_flash_module(hw, bank, ICE_SR_NETLIST_BANK_PTR, offset * sizeof(u16),
+				       (__force u8 *)&data_local, sizeof(u16));
+	if (!status)
+		*data = le16_to_cpu(data_local);
+
+	return status;
+}
+
 /**
  * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
  * @hw: pointer to the HW structure
@@ -739,85 +762,94 @@ ice_get_orom_ver_info(struct ice_hw *hw, struct ice_orom_info *orom)
 }
 
 /**
- * ice_get_netlist_ver_info
+ * ice_get_netlist_info
  * @hw: pointer to the HW struct
- * @ver: pointer to netlist version info structure
+ * @bank: whether to read from the active or inactive flash bank
+ * @netlist: pointer to netlist version info structure
  *
- * Get the netlist version information
+ * Get the netlist version information from the requested bank. Reads the Link
+ * Topology section to find the Netlist ID block and extract the relevant
+ * information into the netlist version structure.
  */
 static enum ice_status
-ice_get_netlist_ver_info(struct ice_hw *hw, struct ice_netlist_info *ver)
+ice_get_netlist_info(struct ice_hw *hw, enum ice_bank_select bank,
+		     struct ice_netlist_info *netlist)
 {
-	enum ice_status ret;
-	u32 id_blk_start;
-	__le16 raw_data;
-	u16 data, i;
-	u16 *buff;
-
-	ret = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (ret)
-		return ret;
-	buff = kcalloc(ICE_AQC_NVM_NETLIST_ID_BLK_LEN, sizeof(*buff),
-		       GFP_KERNEL);
-	if (!buff) {
-		ret = ICE_ERR_NO_MEMORY;
-		goto exit_no_mem;
+	u16 module_id, length, node_count, i;
+	enum ice_status status;
+	u16 *id_blk;
+
+	status = ice_read_netlist_module(hw, bank, ICE_NETLIST_TYPE_OFFSET, &module_id);
+	if (status)
+		return status;
+
+	if (module_id != ICE_NETLIST_LINK_TOPO_MOD_ID) {
+		ice_debug(hw, ICE_DBG_NVM, "Expected netlist module_id ID of 0x%04x, but got 0x%04x\n",
+			  ICE_NETLIST_LINK_TOPO_MOD_ID, module_id);
+		return ICE_ERR_NVM;
 	}
 
-	/* read module length */
-	ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_LINK_TOPO_NETLIST_MOD_ID,
-			      ICE_AQC_NVM_LINK_TOPO_NETLIST_LEN_OFFSET * 2,
-			      ICE_AQC_NVM_LINK_TOPO_NETLIST_LEN, &raw_data,
-			      false, false, NULL);
-	if (ret)
-		goto exit_error;
+	status = ice_read_netlist_module(hw, bank, ICE_LINK_TOPO_MODULE_LEN, &length);
+	if (status)
+		return status;
 
-	data = le16_to_cpu(raw_data);
-	/* exit if length is = 0 */
-	if (!data)
-		goto exit_error;
+	/* sanity check that we have at least enough words to store the netlist ID block */
+	if (length < ICE_NETLIST_ID_BLK_SIZE) {
+		ice_debug(hw, ICE_DBG_NVM, "Netlist Link Topology module too small. Expected at least %u words, but got %u words.\n",
+			  ICE_NETLIST_ID_BLK_SIZE, length);
+		return ICE_ERR_NVM;
+	}
 
-	/* read node count */
-	ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_LINK_TOPO_NETLIST_MOD_ID,
-			      ICE_AQC_NVM_NETLIST_NODE_COUNT_OFFSET * 2,
-			      ICE_AQC_NVM_NETLIST_NODE_COUNT_LEN, &raw_data,
-			      false, false, NULL);
-	if (ret)
-		goto exit_error;
-	data = le16_to_cpu(raw_data) & ICE_AQC_NVM_NETLIST_NODE_COUNT_M;
+	status = ice_read_netlist_module(hw, bank, ICE_LINK_TOPO_NODE_COUNT, &node_count);
+	if (status)
+		return status;
+	node_count &= ICE_LINK_TOPO_NODE_COUNT_M;
 
-	/* netlist ID block starts from offset 4 + node count * 2 */
-	id_blk_start = ICE_AQC_NVM_NETLIST_ID_BLK_START_OFFSET + data * 2;
+	id_blk = kcalloc(ICE_NETLIST_ID_BLK_SIZE, sizeof(*id_blk), GFP_KERNEL);
+	if (!id_blk)
+		return ICE_ERR_NO_MEMORY;
 
-	/* read the entire netlist ID block */
-	ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_LINK_TOPO_NETLIST_MOD_ID,
-			      id_blk_start * 2,
-			      ICE_AQC_NVM_NETLIST_ID_BLK_LEN * 2, buff, false,
-			      false, NULL);
-	if (ret)
+	/* Read out the entire Netlist ID Block at once. */
+	status = ice_read_flash_module(hw, bank, ICE_SR_NETLIST_BANK_PTR,
+				       ICE_NETLIST_ID_BLK_OFFSET(node_count) * sizeof(u16),
+				       (u8 *)id_blk, ICE_NETLIST_ID_BLK_SIZE * sizeof(u16));
+	if (status)
 		goto exit_error;
 
-	for (i = 0; i < ICE_AQC_NVM_NETLIST_ID_BLK_LEN; i++)
-		buff[i] = le16_to_cpu(((__force __le16 *)buff)[i]);
-
-	ver->major = (buff[ICE_AQC_NVM_NETLIST_ID_BLK_MAJOR_VER_HIGH] << 16) |
-		buff[ICE_AQC_NVM_NETLIST_ID_BLK_MAJOR_VER_LOW];
-	ver->minor = (buff[ICE_AQC_NVM_NETLIST_ID_BLK_MINOR_VER_HIGH] << 16) |
-		buff[ICE_AQC_NVM_NETLIST_ID_BLK_MINOR_VER_LOW];
-	ver->type = (buff[ICE_AQC_NVM_NETLIST_ID_BLK_TYPE_HIGH] << 16) |
-		buff[ICE_AQC_NVM_NETLIST_ID_BLK_TYPE_LOW];
-	ver->rev = (buff[ICE_AQC_NVM_NETLIST_ID_BLK_REV_HIGH] << 16) |
-		buff[ICE_AQC_NVM_NETLIST_ID_BLK_REV_LOW];
-	ver->cust_ver = buff[ICE_AQC_NVM_NETLIST_ID_BLK_CUST_VER];
+	for (i = 0; i < ICE_NETLIST_ID_BLK_SIZE; i++)
+		id_blk[i] = le16_to_cpu(((__force __le16 *)id_blk)[i]);
+
+	netlist->major = id_blk[ICE_NETLIST_ID_BLK_MAJOR_VER_HIGH] << 16 |
+			 id_blk[ICE_NETLIST_ID_BLK_MAJOR_VER_LOW];
+	netlist->minor = id_blk[ICE_NETLIST_ID_BLK_MINOR_VER_HIGH] << 16 |
+			 id_blk[ICE_NETLIST_ID_BLK_MINOR_VER_LOW];
+	netlist->type = id_blk[ICE_NETLIST_ID_BLK_TYPE_HIGH] << 16 |
+			id_blk[ICE_NETLIST_ID_BLK_TYPE_LOW];
+	netlist->rev = id_blk[ICE_NETLIST_ID_BLK_REV_HIGH] << 16 |
+		       id_blk[ICE_NETLIST_ID_BLK_REV_LOW];
+	netlist->cust_ver = id_blk[ICE_NETLIST_ID_BLK_CUST_VER];
 	/* Read the left most 4 bytes of SHA */
-	ver->hash = buff[ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH + 15] << 16 |
-		buff[ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH + 14];
+	netlist->hash = id_blk[ICE_NETLIST_ID_BLK_SHA_HASH_WORD(15)] << 16 |
+			id_blk[ICE_NETLIST_ID_BLK_SHA_HASH_WORD(14)];
 
 exit_error:
-	kfree(buff);
-exit_no_mem:
-	ice_release_nvm(hw);
-	return ret;
+	kfree(id_blk);
+
+	return status;
+}
+
+/**
+ * ice_get_inactive_netlist_ver
+ * @hw: pointer to the HW struct
+ * @netlist: pointer to netlist version info structure
+ *
+ * Read the netlist version data from the inactive netlist bank. Used to
+ * extract version data of a pending flash update in order to display the
+ * version data.
+ */
+enum ice_status ice_get_inactive_netlist_ver(struct ice_hw *hw, struct ice_netlist_info *netlist)
+{
+	return ice_get_netlist_info(hw, ICE_INACTIVE_FLASH_BANK, netlist);
 }
 
 /**
@@ -1073,7 +1105,7 @@ enum ice_status ice_init_nvm(struct ice_hw *hw)
 	}
 
 	/* read the netlist version information */
-	status = ice_get_netlist_ver_info(hw, &flash->netlist);
+	status = ice_get_netlist_info(hw, ICE_ACTIVE_FLASH_BANK, &flash->netlist);
 	if (status)
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read netlist info.\n");
 
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index c5c737b7b062..34b5d9589ecc 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -20,6 +20,8 @@ ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 enum ice_status
 ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm);
 enum ice_status
+ice_get_inactive_netlist_ver(struct ice_hw *hw, struct ice_netlist_info *netlist);
+enum ice_status
 ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size);
 enum ice_status ice_init_nvm(struct ice_hw *hw);
 enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data);
diff --git a/drivers/net/ethernet/intel/ice/ice_status.h b/drivers/net/ethernet/intel/ice/ice_status.h
index 4028c6365172..dbf66057371d 100644
--- a/drivers/net/ethernet/intel/ice/ice_status.h
+++ b/drivers/net/ethernet/intel/ice/ice_status.h
@@ -29,6 +29,7 @@ enum ice_status {
 	ICE_ERR_HW_TABLE			= -19,
 	ICE_ERR_FW_DDP_MISMATCH			= -20,
 
+	ICE_ERR_NVM				= -50,
 	ICE_ERR_NVM_CHECKSUM			= -51,
 	ICE_ERR_BUF_TOO_SHORT			= -52,
 	ICE_ERR_NVM_BLANK_MODE			= -53,
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index c2fc12681bb3..92560f324138 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -852,6 +852,41 @@ struct ice_hw_port_stats {
 /* Size in bytes of Option ROM trailer */
 #define ICE_NVM_OROM_TRAILER_LENGTH		(2 * ICE_CSS_HEADER_LENGTH)
 
+/* The Link Topology Netlist section is stored as a series of words. It is
+ * stored in the NVM as a TLV, with the first two words containing the type
+ * and length.
+ */
+#define ICE_NETLIST_LINK_TOPO_MOD_ID		0x011B
+#define ICE_NETLIST_TYPE_OFFSET			0x0000
+#define ICE_NETLIST_LEN_OFFSET			0x0001
+
+/* The Link Topology section follows the TLV header. When reading the netlist
+ * using ice_read_netlist_module, we need to account for the 2-word TLV
+ * header.
+ */
+#define ICE_NETLIST_LINK_TOPO_OFFSET(n)		((n) + 2)
+
+#define ICE_LINK_TOPO_MODULE_LEN		ICE_NETLIST_LINK_TOPO_OFFSET(0x0000)
+#define ICE_LINK_TOPO_NODE_COUNT		ICE_NETLIST_LINK_TOPO_OFFSET(0x0001)
+
+#define ICE_LINK_TOPO_NODE_COUNT_M		ICE_M(0x3FF, 0)
+
+/* The Netlist ID Block is located after all of the Link Topology nodes. */
+#define ICE_NETLIST_ID_BLK_SIZE			0x30
+#define ICE_NETLIST_ID_BLK_OFFSET(n)		ICE_NETLIST_LINK_TOPO_OFFSET(0x0004 + 2 * (n))
+
+/* netlist ID block field offsets (word offsets) */
+#define ICE_NETLIST_ID_BLK_MAJOR_VER_LOW	0x02
+#define ICE_NETLIST_ID_BLK_MAJOR_VER_HIGH	0x03
+#define ICE_NETLIST_ID_BLK_MINOR_VER_LOW	0x04
+#define ICE_NETLIST_ID_BLK_MINOR_VER_HIGH	0x05
+#define ICE_NETLIST_ID_BLK_TYPE_LOW		0x06
+#define ICE_NETLIST_ID_BLK_TYPE_HIGH		0x07
+#define ICE_NETLIST_ID_BLK_REV_LOW		0x08
+#define ICE_NETLIST_ID_BLK_REV_HIGH		0x09
+#define ICE_NETLIST_ID_BLK_SHA_HASH_WORD(n)	(0x0A + (n))
+#define ICE_NETLIST_ID_BLK_CUST_VER		0x2F
+
 /* Auxiliary field, mask, and shift definition for Shadow RAM and NVM Flash */
 #define ICE_SR_CTRL_WORD_1_S		0x06
 #define ICE_SR_CTRL_WORD_1_M		(0x03 << ICE_SR_CTRL_WORD_1_S)
-- 
2.26.2

[PATCH net-next 12/15] ice: display stored UNDI firmware version via devlink info

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

Just as we recently added support for other stored firmware flash
versions, support display of the stored UNDI Option ROM version via
devlink info.

To do this, we need to introduce a new ice_get_inactive_orom_ver
function. This is a little trickier than with other flash versions. The
Option ROM version data was being read from a special "Boot
Configuration" block of the NVM Preserved Field Area. This block only
contains the *active* Option ROM version data. It is populated when the
device firmware finishes updating the Option ROM.

This method is ineffective at reading the stored Option ROM version
data. Instead of reading from this section of the flash, replace this
version extraction with one which locates the Combo Version information
from within the Option ROM binary.

This data is stored within the Option ROM at a 512 byte offset, in
a simple structured format. The structure uses a simple modulo 256
checksum for integrity verification. Scan through the Option ROM to
locate the CIVD data section, and extract the Combo Version.

Refactor ice_get_orom_ver_info so that it takes the bank select
enumeration parameter. Use this to implement ice_get_inactive_orom_ver.

Although all ice devices have a Boot Configuration block in the NVM PFA,
not all devices have a valid Option ROM. In this case, the old
ice_get_orom_ver_info would "succeed" but report a version of all
zeros. The new implementation would fail to locate the $CIV section in
the Option ROM and report an error. Thus, we must ensure that
ice_init_nvm does not fail if ice_get_orom_ver_info fails.

Use the new ice_get_inactive_orom_ver to allow reporting the Option ROM
versions for a pending update via devlink info.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c |  37 ++++++
 drivers/net/ethernet/intel/ice/ice_nvm.c     | 121 +++++++++++++------
 drivers/net/ethernet/intel/ice/ice_nvm.h     |  10 ++
 3 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 2b47e1fbccfb..9276f5abc63d 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -9,6 +9,7 @@
 /* context for devlink info version reporting */
 struct ice_info_ctx {
 	char buf[128];
+	struct ice_orom_info pending_orom;
 	struct ice_nvm_info pending_nvm;
 	struct ice_netlist_info pending_netlist;
 	struct ice_hw_dev_caps dev_caps;
@@ -103,6 +104,18 @@ static int ice_info_orom_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_orom_ver(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_orom_info *orom = &ctx->pending_orom;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_orom)
+		snprintf(ctx->buf, sizeof(ctx->buf), "%u.%u.%u",
+			 orom->major, orom->build, orom->patch);
+
+	return 0;
+}
+
 static int ice_info_orom_srev(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_orom_info *orom = &pf->hw.flash.orom;
@@ -112,6 +125,17 @@ static int ice_info_orom_srev(struct ice_pf *pf, struct ice_info_ctx *ctx)
 	return 0;
 }
 
+static int
+ice_info_pending_orom_srev(struct ice_pf __always_unused *pf, struct ice_info_ctx *ctx)
+{
+	struct ice_orom_info *orom = &ctx->pending_orom;
+
+	if (ctx->dev_caps.common_cap.nvm_update_pending_orom)
+		snprintf(ctx->buf, sizeof(ctx->buf), "%u", orom->srev);
+
+	return 0;
+}
+
 static int ice_info_nvm_ver(struct ice_pf *pf, struct ice_info_ctx *ctx)
 {
 	struct ice_nvm_info *nvm = &pf->hw.flash.nvm;
@@ -247,7 +271,9 @@ static const struct ice_devlink_version {
 	running("fw.mgmt.srev", ice_info_fw_srev),
 	stored("fw.mgmt.srev", ice_info_pending_fw_srev),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_UNDI, ice_info_orom_ver),
+	stored(DEVLINK_INFO_VERSION_GENERIC_FW_UNDI, ice_info_pending_orom_ver),
 	running("fw.undi.srev", ice_info_orom_srev),
+	stored("fw.undi.srev", ice_info_pending_orom_srev),
 	running("fw.psid.api", ice_info_nvm_ver),
 	stored("fw.psid.api", ice_info_pending_nvm_ver),
 	running(DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID, ice_info_eetrack),
@@ -295,6 +321,17 @@ static int ice_devlink_info_get(struct devlink *devlink,
 		goto out_free_ctx;
 	}
 
+	if (ctx->dev_caps.common_cap.nvm_update_pending_orom) {
+		status = ice_get_inactive_orom_ver(hw, &ctx->pending_orom);
+		if (status) {
+			dev_dbg(dev, "Unable to read inactive Option ROM version data, status %s aq_err %s\n",
+				ice_stat_str(status), ice_aq_str(hw->adminq.sq_last_status));
+
+			/* disable display of pending Option ROM */
+			ctx->dev_caps.common_cap.nvm_update_pending_orom = false;
+		}
+	}
+
 	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm) {
 		status = ice_get_inactive_nvm_ver(hw, &ctx->pending_nvm);
 		if (status) {
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 8bc2df09a11a..916ab2504a07 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -703,64 +703,109 @@ static enum ice_status ice_get_orom_srev(struct ice_hw *hw, enum ice_bank_select
 }
 
 /**
- * ice_get_orom_ver_info - Read Option ROM version information
+ * ice_get_orom_civd_data - Get the combo version information from Option ROM
  * @hw: pointer to the HW struct
- * @orom: pointer to Option ROM info structure
+ * @bank: whether to read from the active or inactive flash module
+ * @civd: storage for the Option ROM CIVD data.
  *
- * Read the Combo Image version data from the Boot Configuration TLV and fill
- * in the option ROM version data.
+ * Searches through the Option ROM flash contents to locate the CIVD data for
+ * the image.
  */
 static enum ice_status
-ice_get_orom_ver_info(struct ice_hw *hw, struct ice_orom_info *orom)
+ice_get_orom_civd_data(struct ice_hw *hw, enum ice_bank_select bank,
+		       struct ice_orom_civd_info *civd)
 {
-	u16 combo_hi, combo_lo, boot_cfg_tlv, boot_cfg_tlv_len;
+	struct ice_orom_civd_info tmp;
 	enum ice_status status;
-	u32 combo_ver;
-
-	status = ice_get_pfa_module_tlv(hw, &boot_cfg_tlv, &boot_cfg_tlv_len,
-					ICE_SR_BOOT_CFG_PTR);
-	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read Boot Configuration Block TLV.\n");
-		return status;
-	}
+	u32 offset;
 
-	/* Boot Configuration Block must have length at least 2 words
-	 * (Combo Image Version High and Combo Image Version Low)
+	/* The CIVD section is located in the Option ROM aligned to 512 bytes.
+	 * The first 4 bytes must contain the ASCII characters "$CIV".
+	 * A simple modulo 256 sum of all of the bytes of the structure must
+	 * equal 0.
 	 */
-	if (boot_cfg_tlv_len < 2) {
-		ice_debug(hw, ICE_DBG_INIT, "Invalid Boot Configuration Block TLV size.\n");
-		return ICE_ERR_INVAL_SIZE;
-	}
+	for (offset = 0; (offset + 512) <= hw->flash.banks.orom_size; offset += 512) {
+		u8 sum = 0, i;
 
-	status = ice_read_sr_word(hw, (boot_cfg_tlv + ICE_NVM_OROM_VER_OFF),
-				  &combo_hi);
-	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read OROM_VER hi.\n");
-		return status;
+		status = ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR,
+					       offset, (u8 *)&tmp, sizeof(tmp));
+		if (status) {
+			ice_debug(hw, ICE_DBG_NVM, "Unable to read Option ROM CIVD data\n");
+			return status;
+		}
+
+		/* Skip forward until we find a matching signature */
+		if (memcmp("$CIV", tmp.signature, sizeof(tmp.signature)) != 0)
+			continue;
+
+		/* Verify that the simple checksum is zero */
+		for (i = 0; i < sizeof(tmp); i++)
+			sum += ((u8 *)&tmp)[i];
+
+		if (sum) {
+			ice_debug(hw, ICE_DBG_NVM, "Found CIVD data with invalid checksum of %u\n",
+				  sum);
+			return ICE_ERR_NVM;
+		}
+
+		*civd = tmp;
+		return 0;
 	}
 
-	status = ice_read_sr_word(hw, (boot_cfg_tlv + ICE_NVM_OROM_VER_OFF + 1),
-				  &combo_lo);
+	return ICE_ERR_NVM;
+}
+
+/**
+ * ice_get_orom_ver_info - Read Option ROM version information
+ * @hw: pointer to the HW struct
+ * @bank: whether to read from the active or inactive flash module
+ * @orom: pointer to Option ROM info structure
+ *
+ * Read Option ROM version and security revision from the Option ROM flash
+ * section.
+ */
+static enum ice_status
+ice_get_orom_ver_info(struct ice_hw *hw, enum ice_bank_select bank, struct ice_orom_info *orom)
+{
+	struct ice_orom_civd_info civd;
+	enum ice_status status;
+	u32 combo_ver;
+
+	status = ice_get_orom_civd_data(hw, bank, &civd);
 	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed to read OROM_VER lo.\n");
+		ice_debug(hw, ICE_DBG_NVM, "Failed to locate valid Option ROM CIVD data\n");
 		return status;
 	}
 
-	combo_ver = ((u32)combo_hi << 16) | combo_lo;
+	combo_ver = le32_to_cpu(civd.combo_ver);
 
-	orom->major = (u8)((combo_ver & ICE_OROM_VER_MASK) >>
-			   ICE_OROM_VER_SHIFT);
+	orom->major = (u8)((combo_ver & ICE_OROM_VER_MASK) >> ICE_OROM_VER_SHIFT);
 	orom->patch = (u8)(combo_ver & ICE_OROM_VER_PATCH_MASK);
-	orom->build = (u16)((combo_ver & ICE_OROM_VER_BUILD_MASK) >>
-			    ICE_OROM_VER_BUILD_SHIFT);
+	orom->build = (u16)((combo_ver & ICE_OROM_VER_BUILD_MASK) >> ICE_OROM_VER_BUILD_SHIFT);
 
-	status = ice_get_orom_srev(hw, ICE_ACTIVE_FLASH_BANK, &orom->srev);
-	if (status)
+	status = ice_get_orom_srev(hw, bank, &orom->srev);
+	if (status) {
 		ice_debug(hw, ICE_DBG_NVM, "Failed to read Option ROM security revision.\n");
+		return status;
+	}
 
 	return 0;
 }
 
+/**
+ * ice_get_inactive_orom_ver - Read Option ROM version from the inactive bank
+ * @hw: pointer to the HW structure
+ * @orom: storage for Option ROM version information
+ *
+ * Reads the Option ROM version and security revision data for the inactive
+ * section of flash. Used to access version data for a pending update that has
+ * not yet been activated.
+ */
+enum ice_status ice_get_inactive_orom_ver(struct ice_hw *hw, struct ice_orom_info *orom)
+{
+	return ice_get_orom_ver_info(hw, ICE_INACTIVE_FLASH_BANK, orom);
+}
+
 /**
  * ice_get_netlist_info
  * @hw: pointer to the HW struct
@@ -1098,11 +1143,9 @@ enum ice_status ice_init_nvm(struct ice_hw *hw)
 		return status;
 	}
 
-	status = ice_get_orom_ver_info(hw, &flash->orom);
-	if (status) {
+	status = ice_get_orom_ver_info(hw, ICE_ACTIVE_FLASH_BANK, &flash->orom);
+	if (status)
 		ice_debug(hw, ICE_DBG_INIT, "Failed to read Option ROM info.\n");
-		return status;
-	}
 
 	/* read the netlist version information */
 	status = ice_get_netlist_info(hw, ICE_ACTIVE_FLASH_BANK, &flash->netlist);
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 34b5d9589ecc..905f6893e0b1 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -4,6 +4,14 @@
 #ifndef _ICE_NVM_H_
 #define _ICE_NVM_H_
 
+struct ice_orom_civd_info {
+	u8 signature[4];	/* Must match ASCII '$CIV' characters */
+	u8 checksum;		/* Simple modulo 256 sum of all structure bytes must equal 0 */
+	__le32 combo_ver;	/* Combo Image Version number */
+	u8 combo_name_len;	/* Length of the unicode combo image version string, max of 32 */
+	__le16 combo_name[32];	/* Unicode string representing the Combo Image version */
+} __packed;
+
 enum ice_status
 ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
 void ice_release_nvm(struct ice_hw *hw);
@@ -18,6 +26,8 @@ ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 enum ice_status
 ice_update_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 enum ice_status
+ice_get_inactive_orom_ver(struct ice_hw *hw, struct ice_orom_info *orom);
+enum ice_status
 ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm);
 enum ice_status
 ice_get_inactive_netlist_ver(struct ice_hw *hw, struct ice_netlist_info *netlist);
-- 
2.26.2

[PATCH net-next 13/15] ice: Replace one-element array with flexible-array member

[Tony Nguyen]

From: "Gustavo A. R. Silva" <gustavoars@kernel.org>

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code according to the use of a flexible-array member in
struct ice_res_tracker, instead of a one-element array and use the
struct_size() helper to calculate the size for the allocations.

Also, notice that the code below suggests that, currently, two too many
bytes are being allocated with devm_kzalloc(), as the total number of
entries (pf->irq_tracker->num_entries) for pf->irq_tracker->list[] is
_vectors_ and sizeof(*pf->irq_tracker) also includes the size of the
one-element array _list_ in struct ice_res_tracker.

drivers/net/ethernet/intel/ice/ice_main.c:3511:
3511         /* populate SW interrupts pool with number of OS granted IRQs. */
3512         pf->num_avail_sw_msix = (u16)vectors;
3513         pf->irq_tracker->num_entries = (u16)vectors;
3514         pf->irq_tracker->end = pf->irq_tracker->num_entries;

With this change, the right amount of dynamic memory is now allocated
because, contrary to one-element arrays which occupy at least as much
space as a single object of the type, flexible-array members don't
occupy such space in the containing structure.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays

Built-tested-by: kernel test robot <lkp@intel.com>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h      | 2 +-
 drivers/net/ethernet/intel/ice/ice_main.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 56725356a17b..45bbae68b014 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -164,7 +164,7 @@ struct ice_tc_cfg {
 struct ice_res_tracker {
 	u16 num_entries;
 	u16 end;
-	u16 list[1];
+	u16 list[];
 };
 
 struct ice_qs_cfg {
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 5219aa70b530..ff6190bffff6 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3499,9 +3499,9 @@ static int ice_init_interrupt_scheme(struct ice_pf *pf)
 		return vectors;
 
 	/* set up vector assignment tracking */
-	pf->irq_tracker =
-		devm_kzalloc(ice_pf_to_dev(pf), sizeof(*pf->irq_tracker) +
-			     (sizeof(u16) * vectors), GFP_KERNEL);
+	pf->irq_tracker = devm_kzalloc(ice_pf_to_dev(pf),
+				       struct_size(pf->irq_tracker, list, vectors),
+				       GFP_KERNEL);
 	if (!pf->irq_tracker) {
 		ice_dis_msix(pf);
 		return -ENOMEM;
-- 
2.26.2

[PATCH net-next 14/15] ice: use flex_array_size where possible

[Tony Nguyen]

From: Bruce Allan <bruce.w.allan@intel.com>

Use the flex_array_size() helper with the recently added flexible array
members in structures.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c    | 2 +-
 drivers/net/ethernet/intel/ice/ice_flex_pipe.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 6d7e7dd0ebe2..607d33d05a0c 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1653,7 +1653,7 @@ ice_aq_alloc_free_res(struct ice_hw *hw, u16 num_entries,
 	if (!buf)
 		return ICE_ERR_PARAM;
 
-	if (buf_size < (num_entries * sizeof(buf->elem[0])))
+	if (buf_size < flex_array_size(buf, elem, num_entries))
 		return ICE_ERR_PARAM;
 
 	ice_fill_dflt_direct_cmd_desc(&desc, opc);
diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
index f5e81b555353..cf5b717b9293 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
@@ -1525,7 +1525,7 @@ ice_pkg_buf_reserve_section(struct ice_buf_build *bld, u16 count)
 	bld->reserved_section_table_entries += count;
 
 	data_end = le16_to_cpu(buf->data_end) +
-		   (count * sizeof(buf->section_entry[0]));
+		flex_array_size(buf, section_entry, count);
 	buf->data_end = cpu_to_le16(data_end);
 
 	return 0;
-- 
2.26.2

[PATCH net-next 15/15] ice: remove dead code

[Tony Nguyen]

From: Bruce Allan <bruce.w.allan@intel.com>

The check for a NULL pf pointer is moot since the earlier declaration and
assignment of struct device *dev already de-referenced the pointer.  Also,
the only caller of ice_set_dflt_mib() already ensures pf is not NULL.

Cc: Dave Ertman <david.m.ertman@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index ff6190bffff6..3ff4f0d2ff1b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -785,15 +785,9 @@ static void ice_set_dflt_mib(struct ice_pf *pf)
 	u8 mib_type, *buf, *lldpmib = NULL;
 	u16 len, typelen, offset = 0;
 	struct ice_lldp_org_tlv *tlv;
-	struct ice_hw *hw;
+	struct ice_hw *hw = &pf->hw;
 	u32 ouisubtype;
 
-	if (!pf) {
-		dev_dbg(dev, "%s NULL pf pointer\n", __func__);
-		return;
-	}
-
-	hw = &pf->hw;
 	mib_type = SET_LOCAL_MIB_TYPE_LOCAL_MIB;
 	lldpmib = kzalloc(ICE_LLDPDU_SIZE, GFP_KERNEL);
 	if (!lldpmib) {
-- 
2.26.2

Re: [PATCH net-next 00/15][pull request] 100GbE Intel Wired LAN Driver Updates 2021-01-28

[Willem de Bruijn]

On Thu, Jan 28, 2021 at 7:44 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> This series contains updates to ice driver only.
>
> Jake adds devlink reporting of security revision fields associated with
> 'fw.undi' and 'fw.mgmt'. Also implements support for displaying and
> updating the minimum security revision fields for the device as
> driver-specific devlink parameters. And adds reporting of timeout length
> during devlink flash.
>
> He also implements support to report devlink info regarding the version of
> firmware that is stored (downloaded) to the device, but is not yet active.
> This includes the UNDI Option ROM, the Netlist module, and the
> fw.bundle_id.
>
> Changes include:
>    Refactoring version reporting to allow for a context structure.
>
>    ice_read_flash_module is further abstracted to think in terms of
>    "active" and "inactive" banks, rather than focusing on "read from
>    the 1st or 2nd bank". Further, the function is extended to allow
>    reading arbitrary sizes beyond just one word at a time.
>
>    Extend the version function to allow requesting the flash bank to read
>    from (active or inactive).
>
> Gustavo A. R. Silva replaces a one-element array to flexible-array
> member.
>
> Bruce utilizes flex_array_size() helper and removes dead code on a check
> for a condition that can't occur.
>
> The following are changes since commit 32e31b78272ba0905c751a0f6ff6ab4c275a780e:
>   Merge branch 'net-sfp-add-support-for-gpon-rtl8672-rtl9601c-and-ubiquiti-u-fiber'
> and are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE
>
> Bruce Allan (2):
>   ice: use flex_array_size where possible
>   ice: remove dead code
>
> Gustavo A. R. Silva (1):
>   ice: Replace one-element array with flexible-array member
>
> Jacob Keller (12):
>   ice: create flash_info structure and separate NVM version
>   ice: cache NVM module bank information
>   ice: read security revision to ice_nvm_info and ice_orom_info
>   ice: add devlink parameters to read and write minimum security
>     revision
>   ice: report timeout length for erasing during devlink flash
>   ice: introduce context struct for info report
>   ice: refactor interface for ice_read_flash_module
>   ice: allow reading inactive flash security revision
>   ice: allow reading arbitrary size data with read_flash_module
>   ice: display some stored NVM versions via devlink info
>   ice: display stored netlist versions via devlink info
>   ice: display stored UNDI firmware version via devlink info
>
>  Documentation/networking/devlink/ice.rst      |  43 +
>  drivers/net/ethernet/intel/ice/ice.h          |   2 +-
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  40 +-
>  drivers/net/ethernet/intel/ice/ice_common.c   |   2 +-
>  drivers/net/ethernet/intel/ice/ice_devlink.c  | 496 +++++++++-
>  drivers/net/ethernet/intel/ice/ice_devlink.h  |   2 +
>  drivers/net/ethernet/intel/ice/ice_ethtool.c  |   8 +-
>  .../net/ethernet/intel/ice/ice_flex_pipe.c    |   2 +-
>  .../net/ethernet/intel/ice/ice_fw_update.c    |  10 +-
>  drivers/net/ethernet/intel/ice/ice_main.c     |  19 +-
>  drivers/net/ethernet/intel/ice/ice_nvm.c      | 876 +++++++++++++++---
>  drivers/net/ethernet/intel/ice/ice_nvm.h      |  18 +
>  drivers/net/ethernet/intel/ice/ice_status.h   |   1 +
>  drivers/net/ethernet/intel/ice/ice_type.h     | 141 ++-
>  14 files changed, 1427 insertions(+), 233 deletions(-)

For netdrv

Acked-by: Willem de Bruijn <willemb@google.com>

Very clear code and documentation, thanks!