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;