diff mbox series

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

Message ID 20210129004332.3004826-11-anthony.l.nguyen@intel.com
State New
Headers show
Series 100GbE Intel Wired LAN Driver Updates 2021-01-28 | expand

Commit Message

Tony Nguyen Jan. 29, 2021, 12:43 a.m. UTC
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(-)

Comments

Jakub Kicinski Jan. 30, 2021, 6:37 a.m. UTC | #1
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.
Jacob Keller Feb. 1, 2021, 6:15 p.m. UTC | #2
> -----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
Jacob Keller Feb. 1, 2021, 9:40 p.m. UTC | #3
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.
Jakub Kicinski Feb. 1, 2021, 10:34 p.m. UTC | #4
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.
Jacob Keller Feb. 1, 2021, 11:09 p.m. UTC | #5
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
Brelinski, TonyX Feb. 6, 2021, 2:35 a.m. UTC | #6
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
diff mbox series

Patch

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