> From: Lukas Wunner <lukas@xxxxxxxxx> > Sent: Wednesday, November 16, 2022 6:16 AM > > [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). > pci_device_is_present() is calling pci_dev_is_disconnected(). pci_dev_is_disconnected() avoids reading the vendor id. So pci_dev_is_disconnected() looks less strong check. I see that it can return a valid value on recoverable error case. In that case, is pci_channel_offline() a more precise way to check that covers transient and permanent error? And if that is the right check, we need to fix all the callers, mainly widely used nvme driver [1]. [1] https://elixir.bootlin.com/linux/v6.1-rc5/source/drivers/nvme/host/pci.c#L3228 Also, we need to add API documentation on when to use this API in context of hotplug, so that all related drivers can consistently use single API. > 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
