Re: [iwl-net PATCH] idpf: wait for the reset completed state in idpf_check_reset_complete()

[Brian Vazquez]

Thanks for the review, Paul.  I answered inline.

I will fold the spec reference, device and reproducer into v2 and
carry them forward.

the Reviewed-by tags.
On Thu, Oct 8, 2026 at 2:31 PM Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> Dear Brian,
>
>
> Thank you for your patch.
>
> Am 08.10.26 um 19:33 schrieb Brian Vazquez:
> > PFGEN_RSTAT[1:0] / VFGEN_RSTAT[1:0] (PFR_STATE / VFR_STATE) encode the
> > function reset progress as a 2-bit state, not a flag:
> >
> >    00b - reset in progress
> >    01b - reset completed, written by the control plane
> >    10b - function active, written by the control plane once the function
> >          has completed VIRTCHNL2_OP_VERSION
> >    11b - reserved
>
> For me it’d be helpful if you mentioned the datasheet.

The state encoding is in the public IDPF specification, "PF Reset
Status - PFGEN_RSTAT" [1]:

[1] https://github.com/oasis-tcs/idpf-specification/blob/main/idpf_specification.md
>
> > idpf_check_reset_complete() tests (reg_val & rstat_m) as a boolean, with
> > rstat_m = GENMASK(1, 0), so any non-zero state is taken as "completed",
> > although only 01b carries that meaning.
> >
> > That only works if hardware moves the field to 00b as soon as the reset
> > is triggered. On the devices tested it does not: PFSWR transitions
> > PFR_STATE to 00b only when the field currently reads 01b. When the
> > function is active (10b) the field keeps its value until the control
> > plane finishes the reset and writes 01b, which takes a few hundred
> > milliseconds.
>
> Please mention the device, so it’s easier to reproduce. Also, the
> command to trigger the reset.

Intel IPU E2100, 8086:1452. An FLR on an active PF is enough:

  # echo 1 > /sys/bus/pci/devices/0000:01:00.0/reset

  [  299.685030] idpf 0000:01:00.0: HW reset detected
  [  299.715710] idpf 0000:01:00.0: Device HW Reset initiated
  [  362.916087] idpf 0000:01:00.0: Device HW Reset initiated
  [  364.318246] eth0: (slave eth1): link status definitely up, ...

Nothing is logged during the 63 s; the VERSION timeout is silent on this path.

>
> > Consequently, on a hard reset of an active function
> > (IDPF_HR_FUNC_RESET) the first poll sees 10b, returns immediately, the
> > driver re-creates the mailbox and sends VIRTCHNL2_OP_VERSION while the
> > reset is still being processed. The control plane tears the mailbox
> > down underneath it, VERSION is never answered, and the driver sits in a
> > 60 second virtchnl timeout (-ETIME) before going through a second hard
> > reset which then succeeds.
> >
> > Make idpf_check_reset_complete() wait for the state to be exactly 01b.
> > This is the stricter reading of the register definition and it does not
> > depend on how the field got to its current value: functions that are
> > still in progress (00b) or active (10b) keep polling until the control
> > plane reports completion.
> >
> > Fixes: 8077c727561a ("idpf: add controlq init and reset checks")
> > Signed-off-by: Brian Vazquez <brianvv@xxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/intel/idpf/idpf.h     | 2 ++
> >   drivers/net/ethernet/intel/idpf/idpf_lib.c | 3 ++-
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
> > index 470bc23c844c..ce8a1bd0ec8b 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> > @@ -166,6 +166,8 @@ struct idpf_netdev_priv {
> >       spinlock_t stats_lock;
> >   };
> >
> > +#define IDPF_RSTAT_COMPLETE          0x01
> > +
> >   /**
> >    * struct idpf_reset_reg - Reset register offsets/masks
> >    * @rstat: Reset status register
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > index 2c148377540c..a827e0fa67c8 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -1870,7 +1870,8 @@ static int idpf_check_reset_complete(struct idpf_adapter *adapter,
> >                * register for us yet and 0xFFFFFFFF is not a valid value for
> >                * the register, so treat that as invalid.
> >                */
> > -             if (reg_val != 0xFFFFFFFF && (reg_val & reset_reg->rstat_m))
> > +             if (reg_val != 0xFFFFFFFF &&
> > +                 (reg_val & reset_reg->rstat_m) == IDPF_RSTAT_COMPLETE)
> >                       return 0;
> >
> >               usleep_range(5000, 10000);
>
> Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>
>
> Kind regards,
>
> Paul