Re: [PATCH v2] pci: fix device presence detection for VFs

[Lukas Wunner]

[cc += Parav Pandit, author of 43bb40c5b926]

On Sun, Nov 13, 2022 at 03:46:06AM -0500, Michael S. Tsirkin wrote:
> On Fri, Nov 11, 2022 at 05:42:19PM -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 10, 2022 at 03:15:55PM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Nov 10, 2022 at 01:35:47PM -0600, Bjorn Helgaas wrote:
> > > > Prior to this change pci_device_is_present(VF) returned "false"
> > > > (because the VF Vendor ID is 0xffff); after the change it will return
> > > > "true" (because it will look at the PF Vendor ID instead).
> > > > 
> > > > Previously virtio_pci_remove() called virtio_break_device().  I guess
> > > > that meant the virtio I/O operation will never be completed?
> > > > 
> > > > But if we don't call virtio_break_device(), the virtio I/O operation
> > > > *will* be completed?
> 
> Just making sure - pci_device_is_present *is* the suggested way
> to distinguish between graceful and surprise removal, isn't it?

No, it's not.  Instead of !pci_device_is_present() you really want to
call pci_dev_is_disconnected() instead.

While the fix Bjorn applied for v6.2 may solve the issue and may make
sense on it's own, it's not the solution you're looking for.  You want
to swap the call to !pci_device_is_present() with pci_dev_is_disconnected(),
move pci_dev_is_disconnected() from drivers/pci/pci.h to include/linux/pci.h
and add a Fixes tag referencing 43bb40c5b926.

If you don't want to move pci_dev_is_disconnected(), you can alternatively
check for "pdev->error_state == pci_channel_io_perm_failure" or call
pci_channel_offline().  The latter will also return true though on
transient inaccessibility of the device (e.g. if it's being reset).

The theory of operation is as follows:  The PCI layer does indeed know
whether the device was surprise removed or gracefully removed and that
information is passed in the "presence" flag to pciehp_unconfigure_device()
(in drivers/pci/hotplug/pciehp_pci.c).  That function does the following:

	if (!presence)
		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);

In other words, pdev->error_state is set to pci_channel_io_perm_failure
on the entire hierarchy below the hotplug port.  And pci_dev_is_disconnected()
simply checks whether that's the device's error_state.

pci_dev_is_disconnected() makes sense if you definitely know the device
is gone and want to skip certain steps or delays on device teardown.
However be aware that the device may be hot-removed after graceful
removal was initiated.  In such a situation, pci_dev_is_disconnected()
may return false and you'll try to access the device as normal, even
though it was yanked from the slot after the pci_dev_is_disconnected()
call was performed.  Ideally you should be able to cope with such
scenarios as well.

For some more background info, refer to this LWN article (scroll down
to the "Surprise removal" section):
https://lwn.net/Articles/767885/

Thanks,

Lukas