RE: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@xxxxxxxxx>
> Sent: 01 September 2026 14:14
> To: Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx>;
> kvm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: alex@xxxxxxxxxxx; jgg@xxxxxxxx; kevin.tian@xxxxxxxxx;
> kbusch@xxxxxxxx; michal.winiarski@xxxxxxxxx; Sonang Patel
> <sonangp@xxxxxxxxxx>; Nathan Chen <nathanc@xxxxxxxxxx>; Matt Ochs
> <mochs@xxxxxxxxxx>
> Subject: Re: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with
> recovery
> 
> External email: Use caution opening links or attachments
> 
> 
> On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> > vfio_pci_core_disable() frees vconfig while holding only the vfio
> > device_set mutex. The PCI error callbacks never take that one. They
> > run under the PCI device_lock instead, and vfio's close path does not
> > hold that. So a callback still running when close starts can walk into
> > state which is being freed.
> >
> > Publish a device_open flag under recovery_lock. enable() clears it
> > before it touches the device, finish_enable() sets it once
> > vfio_config_init() has allocated vconfig, and prepare_close() clears
> > it again before the teardown frees vconfig. All three take
> > recovery_lock for writing, so a callback either gets there first and
> > close waits for it, or it finds the flag clear and does nothing. The
> > access guards added later test the same flag.
> >
> > recovery_lock is not held across vfio_pci_core_disable(). A later
> > patch has error_detected() take it from under pci_bus_sem, and
> > disable() gets to pci_reset_bus(), which takes pci_bus_sem the other way
> round.
> >
> > access_blocked is only ever set while device_open is set. Nothing sets
> > it without testing device_open first, and close clears access_blocked
> > before it clears device_open. If close left it set, nothing could
> > clear it afterwards.
> > The transaction which set it cannot clear it once device_open is gone,
> > and every path which refuses work on a blocked device would go on
> > refusing. Clear it before device_open so a lock-free reader never sees
> > it set on a device which is closed.
> >
> > open() now refuses a disconnected device with -ENODEV. That is new.
> >
> > Signed-off-by: Shameer Kolothum <skolothumtho@xxxxxxxxxx>
> > ---
> >   drivers/vfio/pci/vfio_pci_core.c | 69 +++++++++++++++++++++++++++++++-
> >   1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index e0be5ddf7039..8de586e4bb73 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -591,10 +591,23 @@ static const struct dev_pm_ops
> vfio_pci_core_pm_ops = {
> >   int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> >   {
> >       struct pci_dev *pdev = vdev->pdev;
> > +     bool supported = vdev->pci_recovery_supported;
> >       int ret;
> >       u16 cmd;
> >       u8 msix_pos;
> >
> > +     if (supported) {
> > +             down_write(&vdev->recovery_lock);
> > +             if (pci_dev_is_disconnected(pdev)) {
> > +                     up_write(&vdev->recovery_lock);
> > +                     return -ENODEV;
> > +             }
> > +
> > +             vdev->pci_recovery_command_valid = false;
> > +             WRITE_ONCE(vdev->pci_recovery_device_open, false);
> > +             up_write(&vdev->recovery_lock);
> > +     }
> > +
> 
> Can we use scoped_guard()/guard() instead of down_write()/up_write()?
> The same comment applies across all similar sections.

Sure, thanks. I will convert these in the next respin wherever it
works out.

Most of the sections take the lock and drop it in the same block, so
they convert cleanly. A few will not, I guess. Some of the callers
drop the lock part way through on purpose, so that the work which
follows runs outside the lock and keeps recovery_lock below pci_bus_sem.
I will double check if there is a way to convert those as well.

Thanks,
Shameer