Re: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries

[Alex Williamson <alex@shazbot.org>]

On Wed, 08 Jul 2026 23:31:23 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing SW/FW synchronization semaphore when accessing PHY registers via MDIO.
> - [Medium] Missing DMA read memory barrier (dma_rmb) after descriptor completion polling.
> --
> 
> --- Patch [6]: [PATCH v3 6/9] vfio: selftests: igb: Disable PCIe completion timeout retries ---
> commit b47012b45a463be841715f383fb27408df2c020e
> Author: Alex Williamson <alex.williamson@nvidia.com>
> 
> vfio: selftests: igb: Disable PCIe completion timeout retries
> 
> The mix_and_match test intentionally submits TX descriptors with an
> unmapped source IOVA so that the DMA read fails.  By default the 82576
> re-sends the request after a PCIe completion timeout (datasheet section
> 8.6.1, GCR.Completion_Timeout_Resend, bit 16, initial value 1b).  On
> real hardware this turns a single fault into a stream of retried reads,
> keeping PCIe AER and IOMMU error handling busy and interfering with
> reset recovery.
>     
> Clear GCR.Completion_Timeout_Resend during device initialization so a
> failed read fails once and stays failed.
> 
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 1848be6cd46c9..a7a30660b23ed 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -219,6 +219,18 @@ static void igb_init(struct vfio_pci_device *device)
> >  		vfio_pci_config_writew(device, PCI_COMMAND, cmd_reg);
> >  	}
> >  
> > +	/*
> > +	 * Disable DMA re-send on PCIe completion timeout (82576 datasheet
> > +	 * section 8.6.1, GCR.Completion_Timeout_Resend, bit 16).  The
> > +	 * mix_and_match test intentionally submits descriptors targeting
> > +	 * unmapped IOVAs; with the default (set) value, the device keeps
> > +	 * retrying the failed read indefinitely, which keeps PCIe AER and
> > +	 * IOMMU error handling busy and interferes with reset recovery.
> > +	 */
> > +	ctrl = igb_read32(igb, E1000_GCR);
> > +	ctrl &= ~E1000_GCR_CMPL_TMOUT_RESEND;
> > +	igb_write32(igb, E1000_GCR, ctrl);
> > +
> >  	/* Configure PHY internal loopback for testing. */
> >  	igb_setup_loopback(igb);  
> 
> [Severity: Medium]
> Does writing directly to the MDIC register on real 82576 hardware risk
> colliding with the firmware or BMC management engine?
> 
> When running tests on physical devices, accessing PHY registers without
> acquiring the E1000_SW_FW_SYNC semaphore can cause corrupted MDIO
> transactions or hardware hangs.  For example, in igb_write_phy():
> 
> 	mdic = (((u32)data) |
> 		(offset << E1000_MDIC_REG_SHIFT) |
> 		(1 << E1000_MDIC_PHY_SHIFT) |
> 		E1000_MDIC_OP_WRITE);
> 
> 	igb_write32(igb, E1000_MDIC, mdic);

Technically yes, but it's non-trivial and we assume an isolated
environment.  I test on a plugin dual-port NIC, potentially an
integrated NIC with manageability on the platform might encounter
something like this, but it's a narrow window that's probably
acceptable for a selftest driver.  We could add the following above
igb_write_phy() to note the deferral:

+/*
+ * Write a PHY register over MDIO.
+ *
+ * A production driver would hold the SW/FW semaphore (SWSM.SWESMBI + the
+ * SW_FW_SYNC PHY bit) across the MDIO transaction to serialize against the
+ * device's management firmware.  The selftest owns the assigned function
+ * exclusively on a dedicated test device with no active manageability
+ * contending for the PHY, so the sync is omitted; it should be added here
+ * if this ever needs to run on a manageability-enabled NIC.
+ */

> [Severity: Medium]
> Can the CPU speculatively read the destination DMA buffer before the hardware
> descriptor writeback is architecturally complete?
> 
> In igb_memcpy_wait(), the code polls the status bit via READ_ONCE() but
> returns without issuing a dma_rmb():
> 
> 		status = le32_to_cpu(READ_ONCE(rx->wb.status_error));
> 		if (status & 1)
> 			break;
> 		usleep(1000);
> 	}
> 
> 	igb_irq_clear(igb);
> 
> On weakly-ordered architectures like ARM64, the test runner's subsequent
> memcmp() reads on the DMA destination buffer could be speculatively
> reordered before the loop's status poll finishes, resulting in stale
> data reads and sporadic test failures.

igb_memcpy_wait() is implemented in patch 1, and could indeed make use
of an rmb().  This function changes through the series, created in 1,
touched in 5 and 9.  Before we get here we could include
<asm/barrier.h> and call rmb() just before return 0;

Re-tested all the suggested code changes, reorder in igb_hw_init() and
rmb() in igb_memcpy_wait() against physical 82576 NIC, 35/35 pass.
Thanks,

Alex