On Wed, 22 Jul 2026 00:15:36 +0000
Josh Hilke <[email protected]> wrote:
> +
> +static void igb_reset(struct igb *igb)
> +{
> + igb_write32(igb, E1000_CTRL, igb_read32(igb, E1000_CTRL) |
> E1000_CTRL_RST);
> + /*
> + * Must wait at least 1 millisecond after setting the reset bit before
> + * checking if this device is ready to be used (82576 datasheet section
> + * 4.2.1.6.1).
> + */
> + usleep(2000);
> + VFIO_ASSERT_EQ(igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST, 0);
> + igb_write32(igb, E1000_IMC, 0xFFFFFFFF);
> +}
It looks like we have a regression introduced starting in v5. This was
previously a delay loop:
usleep(1000);
while (igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST)
usleep(10);
Sashiko complained[1] about it being unbounded and it was changed to
this fixed delay and assert, that probably works with QEMU, but not on
my physical NIC.
The real driver seems to wait 10-20ms and actually tests
E1000_EECD_AUTO_RD, which seems a bit more robust, so I'd suggest the
following:
diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index 25cac663de1d..064842b2cd59 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -191,14 +191,28 @@ static int igb_probe(struct vfio_pci_device *device)
static void igb_reset(struct igb *igb)
{
+ int retries = 20;
+
igb_write32(igb, E1000_CTRL, igb_read32(igb, E1000_CTRL) |
E1000_CTRL_RST);
/*
* Must wait at least 1 millisecond after setting the reset bit before
* checking if this device is ready to be used (82576 datasheet section
- * 4.2.1.6.1).
+ * 4.2.1.6.1). The delay also ensures the reset has taken effect and
+ * cleared EECD.AUTO_RD before it is polled below.
+ */
+ usleep(1000);
+
+ /*
+ * Poll NVM Auto Read Done rather than CTRL.RST, matching
+ * igb_get_auto_rd_done() in the igb driver: AUTO_RD implies both that
+ * the reset completed and that the device finished re-reading its
+ * configuration from NVM, which is what actually makes it usable.
*/
- usleep(2000);
- VFIO_ASSERT_EQ(igb_read32(igb, E1000_CTRL) & E1000_CTRL_RST, 0);
+ while (retries-- > 0 && !(igb_read32(igb, E1000_EECD) &
E1000_EECD_AUTO_RD))
+ usleep(1000);
+
+ VFIO_ASSERT_GE(retries, 0, "Device reset did not complete");
+
igb_write32(igb, E1000_IMC, 0xFFFFFFFF);
}
I get a full pass on a physical NIC with that change. Does it still
work on QEMU?
Also...
> diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk
> b/tools/testing/selftests/vfio/lib/libvfio.mk
> index 9f47bceed16f..1f13cca04348 100644
> --- a/tools/testing/selftests/vfio/lib/libvfio.mk
> +++ b/tools/testing/selftests/vfio/lib/libvfio.mk
> @@ -12,6 +12,7 @@ LIBVFIO_C += vfio_pci_driver.c
> ifeq ($(ARCH:x86_64=x86),x86)
> LIBVFIO_C += drivers/ioat/ioat.c
> LIBVFIO_C += drivers/dsa/dsa.c
> +LIBVFIO_C += drivers/igb/igb.c
> endif
>
> LIBVFIO_OUTPUT := $(OUTPUT)/libvfio
> diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
> b/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
> index 6827f4a6febe..a5d0547132c4 100644
> --- a/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
> +++ b/tools/testing/selftests/vfio/lib/vfio_pci_driver.c
> @@ -5,12 +5,14 @@
> #ifdef __x86_64__
> extern struct vfio_pci_driver_ops dsa_ops;
> extern struct vfio_pci_driver_ops ioat_ops;
> +extern struct vfio_pci_driver_ops igb_ops;
> #endif
>
> static struct vfio_pci_driver_ops *driver_ops[] = {
> #ifdef __x86_64__
> &dsa_ops,
> &ioat_ops,
> + &igb_ops,
> #endif
> };
Why are we restricting this driver to x86_64? As a plugin device, and
especially with QEMU emulation support, this should have no
architecture restriction. Follow the lead of nv_falcon, which is
currently in the next branch. Thanks,
Alex
[1]https://lore.kernel.org/all/[email protected]/
