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 <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > > > Kind regards, > > Paul
