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

[Jacob Keller]

On 7/15/2026 10:04 AM, Tony Nguyen wrote:
> 
> 
> 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

I concur as well. In theory we should probably try to fully refactor the
send_sq commands to propagate the error out from the lowest level
without sq_last_status at all.. but thats a bigger ask and I don't think
Robert needs to be asked to do that.

The approach described here seems like a reasonable compromise fornow.

Thanks,
Jake