Re: [PATCH iwl] ice: acquire NVM lock around each flash read
From: Tony Nguyen <anthony.l.nguyen@intel.com>
Date: 2026-07-14 23:47:48
Also in:
intel-wired-lan, lkml
On 7/3/2026 3:32 AM, Robert Malz wrote: ...
quoted hunk ↗ jump to hunk
@@ -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;