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

[Jacob Keller <jacob.e.keller@intel.com>]

On 7/3/2026 6:34 AM, Przemek Kitszel wrote:
> On 7/3/26 12:32, Robert Malz wrote:
>> FW caps the NVM read lock at a maximum of 3000ms regardless of the
>> timeout
>> requested via ice_acquire_nvm(). ice_read_flat_nvm() splits a read into
>> multiple ice_aq_read_nvm() commands, one per 4KB sector, all issued
>> under a
>> single lock taken by the caller. Reading a large region can exceed
>> 3000ms,
>> so FW reclaims the lock mid-read and the remaining commands might fail.
>>

Yikes.

>> Move the lock acquire/release into ice_read_flat_nvm() so it brackets
>> each
>> individual ice_aq_read_nvm() command, ensuring the lock is never held
>> across more than one FW read. ice_release_nvm() issues its own AQ command
>> and would overwrite sq_last_status, so the read's AQ error is preserved
>> across the release for callers such as ice_discover_flash_size() that
>> inspect it.
>>
>> Callers that previously took the lock around ice_read_flat_nvm(),
>> ice_read_sr_word() or ice_read_flash_module() now call them without it.
>> The per-block locking in ice_devlink_nvm_snapshot() is now redundant
>> and dropped.
>>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>


>> Fixes: e94509906d6b ("ice: create function to read a section of the
>> NVM and Shadow RAM")
>> Signed-off-by: Robert Malz <robert.malz@canonical.com>
> 
> thank you for extra effort [1]
> current fix looks elegant!
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> [1] for reference, this is previous attempt for the fix:
> https://lore.kernel.org/intel-wired-lan/CADcc-
> bysA531q2Wh=TD_oFqxivLLdnCRNY5jy7mkZuO0cwJwvg@mail.gmail.com
> 
> [...]
> 
>>   /**
>> - * ice_read_sr_word - Reads Shadow RAM word and acquire NVM if necessary
>> + * ice_read_sr_word - Reads Shadow RAM word
>>    * @hw: pointer to the HW structure
>>    * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
>>    * @data: word read from the Shadow RAM
>>    *
>> - * Reads one 16 bit word from the Shadow RAM using the
>> ice_read_sr_word_aq.
>> + * Reads one 16 bit word from the Shadow RAM using ice_read_sr_word_aq.
>> + *
>> + * The NVM lock is acquired and released internally by
>> ice_read_flat_nvm()
>> + * around the FW read, so this function must be called without the
>> lock held.
>>    */
> 
> for future submissions would be great to "fix" kdoc warnings of touched
> functions, here "Return: " section is missing.
> I do not ask to fix this particular one (given there will be no ask for
> v2 otherwise).
> 
>>   int ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
>>   {
>