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

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Lukas; you can probably give a better answer here :)]

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?
> > > 
> > > It's completed anyway - nothing special happened at the device
> > > level - but driver does not detect it.
> > > 
> > > Calling virtio_break_device will mark all queues as broken, as
> > > a result attempts to check whether operation completed
> > > will return false.
> > > 
> > > This probably means we need to work on handling surprise removal
> > > better in virtio blk - since it looks like actual suprise
> > > removal will hang too. But that I think is a separate issue.
> > 
> > Yeah, this situation doesn't seem like it's inherently special for
> > virtio or VFs, so it's a little surprising to see
> > pci_device_is_present() used there.
> 
> Just making sure - pci_device_is_present *is* the suggested way
> to distinguish between graceful and surprise removal, isn't it?

I'm not quite sure what you're asking here.  A driver would learn
about a graceful removal when its .remove() method is called before
the device is physically removed.  The device is still accessible and
everything should just work.

A driver would learn about a surprise removal either by a read result
that is PCI_POSSIBLE_ERROR() or possibly when its .error_detected()
callback is called.  The .remove() method will eventually be called
when we destroy the pci_dev.

I guess .remove() might use pci_device_is_present() and assume that if
it returns "true", this is a graceful removal.  But that's not
reliable since the device could be physically removed between the
pci_device_is_present() call and the driver's next access to it.

I think the best thing would be for .remove() to just do whatever it
needs to do and look for errors, e.g., using PCI_POSSIBLE_ERROR(),
without relying on pci_device_is_present().

If .remove() wants to avoid doing something that might cause an error,
maybe we should expose pci_dev_is_disconnected().  That's set by the
hotplug remove paths before .remove() is called and feels a little
less racy.

Bjorn