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

[Robert Malz]

Hey Aleksandr,
Thanks for the review.
Added debug logs instead of warning as some callers are already
logging errors in case of ice_read_flat_nvm failure.
Let me know if that works for you.

v3 for reference:
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20260727/056245.html

Regards,
Robert


On Mon, Jul 27, 2026 at 10:27 AM Loktionov, Aleksandr
<aleksandr.loktionov@xxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf
> > Of Robert Malz via Intel-wired-lan
> > Sent: Friday, July 17, 2026 2:12 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn
> > <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
> > Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> > Abeni <pabeni@xxxxxxxxxx>; Lobakin, Aleksander
> > <aleksander.lobakin@xxxxxxxxx>; Keller, Jacob E
> > <jacob.e.keller@xxxxxxxxx>; Jesse Brandeburg <jbrandeb@xxxxxxxxxx>
> > Cc: Robert Malz <robert.malz@xxxxxxxxxxxxx>; intel-wired-
> > lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > Subject: [Intel-wired-lan] [PATCH iwl v2] ice: acquire NVM lock around
> > each flash read
> >
> > 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.
> >
> > 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 overwrites
> > hw->adminq.sq_last_status, which some callers inspect after a failed
> > read.
> > Add an optional read_aq_err output parameter to ice_read_flat_nvm() to
> > capture the failing read's AQ error before the release; callers that
> > need it (ice_discover_flash_size() and the ethtool/devlink log paths)
> > use it instead of sq_last_status, others pass NULL.
> >
> > 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 now-redundant per-block locking in ice_devlink_nvm_snapshot() is
> > dropped.
> >
> > Fixes: e94509906d6b ("ice: create function to read a section of the
> > NVM and Shadow RAM")
> > Signed-off-by: Robert Malz <robert.malz@xxxxxxxxxxxxx>
> > ---
> > v2:
> > - Replace the save/restore of sq_last_status across ice_release_nvm(),
> >   which could race with a concurrent AdminQ command, with a new
> > optional
> >   read_aq_err output parameter.
> > - Add missing "Return:" kdoc to ice_read_sr_word().
> > ---
> >  .../net/ethernet/intel/ice/devlink/devlink.c  | 32 ++------
> > drivers/net/ethernet/intel/ice/ice_ethtool.c  | 16 +---
> >  drivers/net/ethernet/intel/ice/ice_nvm.c      | 76 +++++++++++-------
> > -
> >  drivers/net/ethernet/intel/ice/ice_nvm.h      |  2 +-
> >  4 files changed, 56 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c
> > b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> > index 22b7d8e6bd9e..5a1ab9654fb8 100644
> > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
> > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> > @@ -1856,6 +1856,7 @@ static int ice_devlink_nvm_snapshot(struct
> > devlink *devlink,  {
> >       struct ice_pf *pf = devlink_priv(devlink);
> >       struct device *dev = ice_pf_to_dev(pf);
> > +     enum libie_aq_err read_aq_err = LIBIE_AQ_RC_OK;
> >       struct ice_hw *hw = &pf->hw;
> >       bool read_shadow_ram;
> >       u8 *nvm_data, *tmp, i;
> > @@ -1891,26 +1892,16 @@ static int ice_devlink_nvm_snapshot(struct
> > devlink *devlink,
> >       for (i = 0; i < num_blks; i++) {
> >               u32 read_sz = min_t(u32, ICE_DEVLINK_READ_BLK_SIZE,
> > left);
> >
>
> ...
>
> >  {
> >       u32 inlen = *length;
> >       u32 bytes_read = 0;
> > @@ -92,12 +102,27 @@ 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)
> Silent lock failure without any traces in dmesg looks scary!
> I'm pretty sure we need at least log an warning or error.
> > +                     break;
> > +
>
> ...
>
> >  int
> >  ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16
> > *module_tlv_len,
> >                      u16 module_type);
> > --
> > 2.34.1
>