[+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
