Re: [Intel-wired-lan] [PATCH iwl] ice: acquire NVM lock around each flash read

Hey Tony,
Thanks for the review.

On Wed, Jul 15, 2026 at 1:47 AM Tony Nguyen <[email protected]> wrote:
>
>
>
> 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?

[RM] I do think that is intended behavior. Reads should not be
stateful and there is no open read sequence to interrupt.
Each ice_aq_read_nvm call takes an absolute offset from the starting point.

> 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?

[RM] I don't think there is a leak as there is no FW read sequence
state. As far as I know, lack of last_cmd does not leave anything
dangling in the FW.

>
> [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.

[RM] I'm happy to adjust the patch once you have more details on it.

>
> > -             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.

>
> Thanks,
> Tony
>
> >                       break;
> > +             }
> > +
> > +             ice_release_nvm(hw);
> >
> >               bytes_read += read_size;
> >               offset += read_size;
>
>

Regards,
Robert