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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 7/15/2026 3:22 AM, Robert Malz wrote:

...

>>> -             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.
This sounds good to me.

Thanks,
Tony