From: Parav Pandit <parav@nvidia.com>
To: Lukas Wunner <lukas@wunner.de>, "Michael S. Tsirkin" <mst@redhat.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
Wei Gong <gongwei833x@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: RE: [PATCH v2] pci: fix device presence detection for VFs
Date: Thu, 17 Nov 2022 05:36:48 +0000 [thread overview]
Message-ID: <PH0PR12MB54811F4658F068C46E071E81DC069@PH0PR12MB5481.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20221116111619.GA5804@wunner.de>
> From: Lukas Wunner <lukas@wunner.de>
> 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
next prev parent reply other threads:[~2022-11-17 5:37 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-26 6:11 [PATCH v2] pci: fix device presence detection for VFs Michael S. Tsirkin
2022-10-26 13:46 ` Wei Gong
2022-11-08 4:52 ` Wei Gong
2022-11-08 4:58 ` Michael S. Tsirkin
2022-11-08 5:06 ` Bjorn Helgaas
2022-11-10 19:35 ` Bjorn Helgaas
2022-11-10 20:15 ` Michael S. Tsirkin
2022-11-11 23:42 ` Bjorn Helgaas
2022-11-13 8:46 ` Michael S. Tsirkin
2022-11-15 16:24 ` Bjorn Helgaas
2022-11-16 11:16 ` Lukas Wunner
2022-11-17 5:36 ` Parav Pandit [this message]
2022-12-19 5:56 ` Michael S. Tsirkin
2022-12-19 8:22 ` Lukas Wunner
2022-11-11 4:00 ` Wei Gong
2022-11-08 14:53 ` Bjorn Helgaas
2022-11-08 15:02 ` Bjorn Helgaas
2022-11-08 15:19 ` Michael S. Tsirkin
2022-11-08 17:58 ` Bjorn Helgaas
2022-11-08 18:02 ` Michael S. Tsirkin
2022-11-09 4:36 ` Wei Gong
2022-11-09 5:12 ` Bjorn Helgaas
2022-11-09 7:00 ` Wei Gong
2022-11-09 7:10 ` Michael S. Tsirkin
2022-11-09 17:30 ` Bjorn Helgaas
2022-11-09 17:49 ` Michael S. Tsirkin
2022-11-11 23:39 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PH0PR12MB54811F4658F068C46E071E81DC069@PH0PR12MB5481.namprd12.prod.outlook.com \
--to=parav@nvidia.com \
--cc=bhelgaas@google.com \
--cc=gongwei833x@gmail.com \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mst@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).