From: Alex Williamson <alex@shazbot.org>
To: Josh Hilke <jrhilke@google.com>
Cc: David Matlack <dmatlack@google.com>,
Shuah Khan <shuah@kernel.org>,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
linux-kselftest@vger.kernel.org,
Vipin Sharma <vipinsh@google.com>,
Alex Williamson <alex.williamson@nvidia.com>,
alex@shazbot.org
Subject: Re: [PATCH v6 1/6] vfio: selftests: igb: Add driver for Intel 82576 device
Date: Tue, 28 Jul 2026 12:14:27 -0600 [thread overview]
Message-ID: <20260728121427.7d70e598@shazbot.org> (raw)
In-Reply-To: <20260722-igb_v3_b4-v6-1-0be1d29918d4@google.com>
On Wed, 22 Jul 2026 00:15:36 +0000
Josh Hilke <jrhilke@google.com> 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/20260710221939.CFE741F000E9@smtp.kernel.org/
next prev parent reply other threads:[~2026-07-28 18:14 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-22 0:15 [PATCH v6 0/6] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) Josh Hilke
2026-07-22 0:15 ` [PATCH v6 1/6] vfio: selftests: igb: Add driver for Intel 82576 device Josh Hilke
2026-07-28 18:14 ` Alex Williamson [this message]
2026-07-22 0:15 ` [PATCH v6 2/6] vfio: selftests: igb: Use PHY internal loopback on 82576 Josh Hilke
2026-07-22 0:15 ` [PATCH v6 3/6] vfio: selftests: Add helpers to re-enable interrupts Josh Hilke
2026-07-22 0:15 ` [PATCH v6 4/6] vfio: selftests: igb: Factor hardware programming into igb_hw_init() Josh Hilke
2026-07-22 0:15 ` [PATCH v6 5/6] vfio: selftests: Retry on EAGAIN during device reset Josh Hilke
2026-07-22 0:15 ` [PATCH v6 6/6] vfio: selftests: igb: Recover after DMA-read faults Josh Hilke
2026-07-24 21:42 ` [PATCH v6 0/6] vfio: selftests: Add driver for Intel Ethernet Gigabit Controller (IGB) David Matlack
2026-07-27 20:57 ` Josh Hilke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260728121427.7d70e598@shazbot.org \
--to=alex@shazbot.org \
--cc=alex.williamson@nvidia.com \
--cc=dmatlack@google.com \
--cc=jrhilke@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=shuah@kernel.org \
--cc=vipinsh@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.