Re: [PATCH v5 5/5] vfio: selftests: igb: Recover after DMA-read faults

[Alex Williamson <alex@shazbot.org>]

On Mon, 20 Jul 2026 15:42:27 -0600
Alex Williamson <alex.williamson@nvidia.com> wrote:

> On Mon, 20 Jul 2026 14:27:12 -0700
> Josh Hilke <jrhilke@google.com> wrote:
> 
> > On Tue, Jul 14, 2026 at 2:12 PM <sashiko-bot@kernel.org> wrote:  
> > >
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [Medium] Test flakiness due to fixed heuristic delay instead of polling on EAGAIN during VFIO_DEVICE_RESET
> > > --
> > >
> > > --- Patch [5]: [PATCH v5 5/5] vfio: selftests: igb: Recover after DMA-read faults ---
> > > commit a631c99ce1be37dbbdda0314b730b2c6bd947b50
> > > Author: Alex Williamson <alex.williamson@nvidia.com>
> > >
> > > vfio: selftests: igb: Recover after DMA-read faults
> > >
> > > This commit adds recovery logic to the igb selftest when an intentional DMA
> > > read fault causes the device to hang. It introduces a reset and re-init
> > > sequence using VFIO_DEVICE_RESET, including a 10ms delay to allow kernel
> > > error handlers to release the device lock.
> > >    
> > > > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > > > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > > > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c    
> > >
> > > [ ... ]
> > >    
> > > > @@ -500,7 +522,28 @@ static int igb_memcpy_wait(struct vfio_pci_device *device)
> > > >
> > > >       igb_irq_enable(igb);
> > > >
> > > > -     return (status & 1) ? 0 : -ETIMEDOUT;
> > > > +     if (status & 1)
> > > > +             return 0;
> > > > +
> > > > +     /*
> > > > +      * The descriptor never completed.  On real 82576 hardware this
> > > > +      * typically follows a DMA-read fault from one of the intentional
> > > > +      * unmapped-IOVA tests; the fault leaves the descriptor engine
> > > > +      * unable to service subsequent valid descriptors.  CTRL.RST alone
> > > > +      * reinitializes the queue registers but leaves the engine wedged
> > > > +      * for the current process, so a broader VFIO_DEVICE_RESET (FLR)
> > > > +      * is required.
> > > > +      *
> > > > +      * Delay before requesting reset so PCIe/IOMMU/AER error handling
> > > > +      * triggered by the just-observed DMA fault can release the device
> > > > +      * lock VFIO_DEVICE_RESET contends for.  The 10 ms value is
> > > > +      * heuristic.  The current memcpy still fails with -ETIMEDOUT;
> > > > +      * recovery only ensures the next memcpy starts from a usable state.
> > > > +      */
> > > > +     usleep(10000);
> > > > +     igb_error_reset_and_reinit(device);    
> > >
> > > [Severity: Medium]
> > > Will this fixed heuristic delay cause test flakiness under heavy load?
> > >
> > > If the kernel's PCIe/IOMMU/AER error handling takes longer than 10ms to
> > > complete (for example, when KASAN or other heavy debugging options are
> > > enabled), the device lock might still be held.
> > >
> > > Looking at the kernel side in drivers/vfio/pci/vfio_pci_core.c:
> > > vfio_pci_ioctl_reset(), lock contention causes pci_try_reset_function() to
> > > return -EAGAIN:
> > >
> > >         ret = pci_try_reset_function(vdev->pdev);
> > >         if (__vfio_pci_memory_enabled(vdev))
> > >                 vfio_pci_dma_buf_move(vdev, false);
> > >         up_write(&vdev->memory_lock);
> > >
> > >         return ret;
> > >
> > > Since igb_error_reset_and_reinit() calls vfio_pci_device_reset(), which
> > > asserts that the ioctl returns 0, would this lock contention cause an
> > > immediate test suite failure?
> > >
> > > Should this code explicitly poll for -EAGAIN from the ioctl instead of
> > > relying entirely on a fixed sleep?
> > >    
> > > > +     return -ETIMEDOUT;    
> > 
> > Alex, I'm guessing 10ms is what worked for you when testing with the
> > physical hardware? If it's working fine, I don't think it needs to be
> > changed.  
> 
> Yes, the delay was found empirically and indeed we could make this more
> robust if we retried on -EAGAIN.  I'd be in favor of doing that as a
> follow-up.  Thanks,

Oh, some further context, vfio_pci_device_reset() asserts the return is
zero, which IIRC, is why I accepted the heuristic delay rather than
expand the scope with a new reset callback within the framework.
Thanks,

Alex