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

[Robert Malz]

Hey Jacob, Tony, Przemek
Thanks again for the review.
I have submitted the v2 patch with the mentioned changes.
ref: https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20260713/056079.html

Regards,
Robert

On Wed, Jul 15, 2026 at 8:22 PM Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
>
> 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