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

Hey Tony,
Thanks for the review.

On Wed, Jul 15, 2026 at 1:47 AM Tony Nguyen [off-list ref] wrote:

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

...

quoted

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

[RM] I do think that is intended behavior. Reads should not be
stateful and there is no open read sequence to interrupt.
Each ice_aq_read_nvm call takes an absolute offset from the starting point.

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?

[RM] I don't think there is a leak as there is no FW read sequence
state. As far as I know, lack of last_cmd does not leave anything
dangling in the FW.

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

[RM] I'm happy to adjust the patch once you have more details on it.

quoted

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

[RM] Agree, this can cause issues. We can't drop sq_last_status as
ice_discover_flash_size depends on it.
Proposed fix: drop the save/restore sq_last_status and propagate the
read AQ error through a real return path instead of the shared global.
I could add optional enum libie_aq_err *read_aq_err out param to
ice_read_flat_nvm(), capture sq_last_status before the
ice_release_nvm, and have ice_discover_flash_size() test that instead.
The log-only callers (devlink/ethtool) don't care about it.
Let me know what you think about it.

Thanks,
Tony

quoted

                      break;
+             }
+
+             ice_release_nvm(hw);

              bytes_read += read_size;
              offset += read_size;

Regards,
Robert