[PATCH iwl] ice: acquire NVM lock around each flash read

[PATCH iwl] ice: acquire NVM lock around each flash read

[Robert Malz]

FW caps the NVM read lock at a maximum of 3000ms regardless of the timeout
requested via ice_acquire_nvm(). ice_read_flat_nvm() splits a read into
multiple ice_aq_read_nvm() commands, one per 4KB sector, all issued under a
single lock taken by the caller. Reading a large region can exceed 3000ms,
so FW reclaims the lock mid-read and the remaining commands might fail.

Move the lock acquire/release into ice_read_flat_nvm() so it brackets each
individual ice_aq_read_nvm() command, ensuring the lock is never held
across more than one FW read. ice_release_nvm() issues its own AQ command
and would overwrite sq_last_status, so the read's AQ error is preserved
across the release for callers such as ice_discover_flash_size() that
inspect it.

Callers that previously took the lock around ice_read_flat_nvm(),
ice_read_sr_word() or ice_read_flash_module() now call them without it.
The per-block locking in ice_devlink_nvm_snapshot() is now redundant
and dropped.

Fixes: e94509906d6b ("ice: create function to read a section of the NVM and Shadow RAM")
Signed-off-by: Robert Malz <robert.malz@canonical.com>
---
 .../net/ethernet/intel/ice/devlink/devlink.c  | 21 -------
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 11 +---
 drivers/net/ethernet/intel/ice/ice_nvm.c      | 58 ++++++++++---------
 3 files changed, 32 insertions(+), 58 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
index 22b7d8e6bd9e..7c7fe3d2e9d9 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
@@ -1891,26 +1891,15 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 	for (i = 0; i < num_blks; i++) {
 		u32 read_sz = min_t(u32, ICE_DEVLINK_READ_BLK_SIZE, left);
 
-		status = ice_acquire_nvm(hw, ICE_RES_READ);
-		if (status) {
-			dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
-				status, hw->adminq.sq_last_status);
-			NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
-			vfree(nvm_data);
-			return -EIO;
-		}
-
 		status = ice_read_flat_nvm(hw, i * ICE_DEVLINK_READ_BLK_SIZE,
 					   &read_sz, tmp, read_shadow_ram);
 		if (status) {
 			dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
 				read_sz, status, hw->adminq.sq_last_status);
 			NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM contents");
-			ice_release_nvm(hw);
 			vfree(nvm_data);
 			return -EIO;
 		}
-		ice_release_nvm(hw);
 
 		tmp += read_sz;
 		left -= read_sz;
@@ -1966,24 +1955,14 @@ static int ice_devlink_nvm_read(struct devlink *devlink,
 		return -ERANGE;
 	}
 
-	status = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (status) {
-		dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
-			status, hw->adminq.sq_last_status);
-		NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
-		return -EIO;
-	}
-
 	status = ice_read_flat_nvm(hw, (u32)offset, &size, data,
 				   read_shadow_ram);
 	if (status) {
 		dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
 			size, status, hw->adminq.sq_last_status);
 		NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM contents");
-		ice_release_nvm(hw);
 		return -EIO;
 	}
-	ice_release_nvm(hw);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 49371b065845..ddeca39d822b 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -869,24 +869,15 @@ ice_get_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom,
 	if (!buf)
 		return -ENOMEM;
 
-	ret = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (ret) {
-		dev_err(dev, "ice_acquire_nvm failed, err %d aq_err %s\n",
-			ret, libie_aq_str(hw->adminq.sq_last_status));
-		goto out;
-	}
-
 	ret = ice_read_flat_nvm(hw, eeprom->offset, &eeprom->len, buf,
 				false);
 	if (ret) {
 		dev_err(dev, "ice_read_flat_nvm failed, err %d aq_err %s\n",
 			ret, libie_aq_str(hw->adminq.sq_last_status));
-		goto release;
+		goto out;
 	}
 
 	memcpy(bytes, buf, eeprom->len);
-release:
-	ice_release_nvm(hw);
 out:
 	kfree(buf);
 	return ret;
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 7e187a804dfa..cb1541844242 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -58,6 +58,11 @@ int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
  * breaks read requests across Shadow RAM sectors and ensures that no single
  * read request exceeds the maximum 4KB read for a single AdminQ command.
  *
+ * FW caps the read lock at a maximum of 3000ms, so a read spanning multiple
+ * 4KB sectors cannot be done under a single lock without FW reclaiming it
+ * mid-read. The NVM lock is therefore acquired and released around each AQ
+ * read, so this function must be called without the lock held.
+ *
  * Returns a status code on failure. Note that the data pointer may be
  * partially updated if some reads succeed before a failure.
  */
@@ -65,6 +70,7 @@ int
 ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
 		  bool read_shadow_ram)
 {
+	enum libie_aq_err aq_err;
 	u32 inlen = *length;
 	u32 bytes_read = 0;
 	bool last_cmd;
@@ -92,12 +98,28 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
 
 		last_cmd = !(bytes_read + read_size < inlen);
 
+		status = ice_acquire_nvm(hw, ICE_RES_READ);
+		if (status)
+			break;
+
 		status = ice_aq_read_nvm(hw, ICE_AQC_NVM_START_POINT,
 					 offset, read_size,
 					 data + bytes_read, last_cmd,
 					 read_shadow_ram, NULL);
-		if (status)
+		if (status) {
+			/* ice_release_nvm() issues an AQ command that would
+			 * overwrite sq_last_status, which some callers
+			 * inspect after a failed read. Preserve the read's
+			 * AQ error across the release.
+			 */
+			aq_err = hw->adminq.sq_last_status;
+
+			ice_release_nvm(hw);
+			hw->adminq.sq_last_status = aq_err;
 			break;
+		}
+
+		ice_release_nvm(hw);
 
 		bytes_read += read_size;
 		offset += read_size;
@@ -330,14 +352,8 @@ ice_read_flash_module(struct ice_hw *hw, enum ice_bank_select bank, u16 module,
 		return -EINVAL;
 	}
 
-	status = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (status)
-		return status;
-
 	status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
 
-	ice_release_nvm(hw);
-
 	return status;
 }
 
@@ -419,24 +435,19 @@ ice_read_netlist_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset
 }
 
 /**
- * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
+ * ice_read_sr_word - Reads Shadow RAM word
  * @hw: pointer to the HW structure
  * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
  * @data: word read from the Shadow RAM
  *
- * Reads one 16 bit word from the Shadow RAM using the ice_read_sr_word_aq.
+ * Reads one 16 bit word from the Shadow RAM using ice_read_sr_word_aq.
+ *
+ * The NVM lock is acquired and released internally by ice_read_flat_nvm()
+ * around the FW read, so this function must be called without the lock held.
  */
 int ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
 {
-	int status;
-
-	status = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (!status) {
-		status = ice_read_sr_word_aq(hw, offset, data);
-		ice_release_nvm(hw);
-	}
-
-	return status;
+	return ice_read_sr_word_aq(hw, offset, data);
 }
 
 /**
@@ -856,11 +867,7 @@ int ice_get_inactive_netlist_ver(struct ice_hw *hw, struct ice_netlist_info *net
 static int ice_discover_flash_size(struct ice_hw *hw)
 {
 	u32 min_size = 0, max_size = ICE_AQC_NVM_MAX_OFFSET + 1;
-	int status;
-
-	status = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (status)
-		return status;
+	int status = 0;
 
 	while ((max_size - min_size) > 1) {
 		u32 offset = (max_size + min_size) / 2;
@@ -880,7 +887,7 @@ static int ice_discover_flash_size(struct ice_hw *hw)
 			min_size = offset;
 		} else {
 			/* an unexpected error occurred */
-			goto err_read_flat_nvm;
+			return status;
 		}
 	}
 
@@ -888,9 +895,6 @@ static int ice_discover_flash_size(struct ice_hw *hw)
 
 	hw->flash.flash_size = max_size;
 
-err_read_flat_nvm:
-	ice_release_nvm(hw);
-
 	return status;
 }
 
-- 
2.34.1

Re: [PATCH iwl] ice: acquire NVM lock around each flash read

[Przemek Kitszel]

On 7/3/26 12:32, Robert Malz wrote:
> FW caps the NVM read lock at a maximum of 3000ms regardless of the timeout
> requested via ice_acquire_nvm(). ice_read_flat_nvm() splits a read into
> multiple ice_aq_read_nvm() commands, one per 4KB sector, all issued under a
> single lock taken by the caller. Reading a large region can exceed 3000ms,
> so FW reclaims the lock mid-read and the remaining commands might fail.
> 
> Move the lock acquire/release into ice_read_flat_nvm() so it brackets each
> individual ice_aq_read_nvm() command, ensuring the lock is never held
> across more than one FW read. ice_release_nvm() issues its own AQ command
> and would overwrite sq_last_status, so the read's AQ error is preserved
> across the release for callers such as ice_discover_flash_size() that
> inspect it.
> 
> Callers that previously took the lock around ice_read_flat_nvm(),
> ice_read_sr_word() or ice_read_flash_module() now call them without it.
> The per-block locking in ice_devlink_nvm_snapshot() is now redundant
> and dropped.
> 
> Fixes: e94509906d6b ("ice: create function to read a section of the NVM and Shadow RAM")
> Signed-off-by: Robert Malz <robert.malz@canonical.com>

thank you for extra effort [1]
current fix looks elegant!
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

[1] for reference, this is previous attempt for the fix:
https://lore.kernel.org/intel-wired-lan/CADcc-bysA531q2Wh=TD_oFqxivLLdnCRNY5jy7mkZuO0cwJwvg@mail.gmail.com

[...]

>   /**
> - * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
> + * ice_read_sr_word - Reads Shadow RAM word
>    * @hw: pointer to the HW structure
>    * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
>    * @data: word read from the Shadow RAM
>    *
> - * Reads one 16 bit word from the Shadow RAM using the ice_read_sr_word_aq.
> + * Reads one 16 bit word from the Shadow RAM using ice_read_sr_word_aq.
> + *
> + * The NVM lock is acquired and released internally by ice_read_flat_nvm()
> + * around the FW read, so this function must be called without the lock held.
>    */

for future submissions would be great to "fix" kdoc warnings of touched
functions, here "Return: " section is missing.
I do not ask to fix this particular one (given there will be no ask for
v2 otherwise).

>   int ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
>   {

Re: [PATCH iwl] ice: acquire NVM lock around each flash read

[Jacob Keller]

On 7/3/2026 6:34 AM, Przemek Kitszel wrote:
> On 7/3/26 12:32, Robert Malz wrote:
>> FW caps the NVM read lock at a maximum of 3000ms regardless of the
>> timeout
>> requested via ice_acquire_nvm(). ice_read_flat_nvm() splits a read into
>> multiple ice_aq_read_nvm() commands, one per 4KB sector, all issued
>> under a
>> single lock taken by the caller. Reading a large region can exceed
>> 3000ms,
>> so FW reclaims the lock mid-read and the remaining commands might fail.
>>

Yikes.

>> Move the lock acquire/release into ice_read_flat_nvm() so it brackets
>> each
>> individual ice_aq_read_nvm() command, ensuring the lock is never held
>> across more than one FW read. ice_release_nvm() issues its own AQ command
>> and would overwrite sq_last_status, so the read's AQ error is preserved
>> across the release for callers such as ice_discover_flash_size() that
>> inspect it.
>>
>> Callers that previously took the lock around ice_read_flat_nvm(),
>> ice_read_sr_word() or ice_read_flash_module() now call them without it.
>> The per-block locking in ice_devlink_nvm_snapshot() is now redundant
>> and dropped.
>>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>


>> Fixes: e94509906d6b ("ice: create function to read a section of the
>> NVM and Shadow RAM")
>> Signed-off-by: Robert Malz <robert.malz@canonical.com>
> 
> thank you for extra effort [1]
> current fix looks elegant!
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> [1] for reference, this is previous attempt for the fix:
> https://lore.kernel.org/intel-wired-lan/CADcc-
> bysA531q2Wh=TD_oFqxivLLdnCRNY5jy7mkZuO0cwJwvg@mail.gmail.com
> 
> [...]
> 
>>   /**
>> - * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
>> + * ice_read_sr_word - Reads Shadow RAM word
>>    * @hw: pointer to the HW structure
>>    * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
>>    * @data: word read from the Shadow RAM
>>    *
>> - * Reads one 16 bit word from the Shadow RAM using the
>> ice_read_sr_word_aq.
>> + * Reads one 16 bit word from the Shadow RAM using ice_read_sr_word_aq.
>> + *
>> + * The NVM lock is acquired and released internally by
>> ice_read_flat_nvm()
>> + * around the FW read, so this function must be called without the
>> lock held.
>>    */
> 
> for future submissions would be great to "fix" kdoc warnings of touched
> functions, here "Return: " section is missing.
> I do not ask to fix this particular one (given there will be no ask for
> v2 otherwise).
> 
>>   int ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
>>   {
> 

Re: [PATCH iwl] ice: acquire NVM lock around each flash read

[Tony Nguyen]

On 7/3/2026 3:32 AM, Robert Malz wrote:

...

> @@ -92,12 +98,28 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>   
>   		last_cmd = !(bytes_read + read_size < inlen);
>   
> +		status = ice_acquire_nvm(hw, ICE_RES_READ);
> +		if (status)
> +			break;
> +
>   		status = ice_aq_read_nvm(hw, ICE_AQC_NVM_START_POINT,
>   					 offset, read_size,
>   					 data + bytes_read, last_cmd,
>   					 read_shadow_ram, NULL);

Sashiko says:

If this chunk has last_cmd = false, doesn't releasing the NVM lock 
immediately
after ice_aq_read_nvm() allow other entities to acquire the lock and issue
their own NVM commands in the middle of our open read sequence?
Also, if ice_acquire_nvm() fails on the next loop iteration, the loop breaks
and we never send a command with last_cmd = true. Will this permanently leak
the sequence state in the firmware?

[TN] I'm seeing conflicting documentation on whether this bit matters 
for the read command. I'm working on getting clarification. If it does 
matter, we'll likely need to adjust this.

> -		if (status)
> +		if (status) {
> +			/* ice_release_nvm() issues an AQ command that would
> +			 * overwrite sq_last_status, which some callers
> +			 * inspect after a failed read. Preserve the read's
> +			 * AQ error across the release.
> +			 */
> +			aq_err = hw->adminq.sq_last_status;
> +
> +			ice_release_nvm(hw);
> +			hw->adminq.sq_last_status = aq_err;

Does restoring hw->adminq.sq_last_status here without holding the Admin 
Queue
lock risk overwriting the status of a concurrent AdminQ command?
If another thread acquires the lock and sends a command immediately after
ice_release_nvm(hw), this lockless write could corrupt its error status.

[TN] I don't believe the AQ lock will work as desired we can't guarantee 
that we will have the lock directly following the release in order to 
restore the AQ error. Similar to the NVM lock issue, I think this is a 
small window but wanted to bring this here in case others had 
thoughts/comments on this.

Thanks,
Tony

>   			break;
> +		}
> +
> +		ice_release_nvm(hw);
>   
>   		bytes_read += read_size;
>   		offset += read_size;