Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Dec 2022 09:22:40 +0100
From:   Lukas Wunner <lukas@wunner.de>
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     Parav Pandit <parav@nvidia.com>,
        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
Message-ID: <20221219082240.GA5176@wunner.de>
References: <20221110144700-mutt-send-email-mst@kernel.org>
 <20221111234219.GA763705@bhelgaas>
 <20221113034519-mutt-send-email-mst@kernel.org>
 <20221116111619.GA5804@wunner.de>
 <PH0PR12MB54811F4658F068C46E071E81DC069@PH0PR12MB5481.namprd12.prod.outlook.com>
 <20221219005553-mutt-send-email-mst@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221219005553-mutt-send-email-mst@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Mon, Dec 19, 2022 at 12:56:15AM -0500, Michael S. Tsirkin wrote:
> On Thu, Nov 17, 2022 at 05:36:48AM +0000, Parav Pandit wrote:
> > > 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.
> 
> Bjorn, Lukas, what's your take on this idea?

I don't really know what to add to my e-mail of Nov 16
(quoted here in full).

Yes, pci_channel_offline() returns true on transient and permanent
failure.  Whether that's what you want, depends on your use case.
If you want to check for a surprise-removed device, then you only
want to check for permanent failure, so pci_channel_offline() is
not correct and you should rather check for
"pdev->error_state == pci_channel_io_perm_failure" or move
pci_dev_is_disconnected() to include/linux/pci.h.  But again,
I've already explained this in my e-mail ov Nov 16, so I don't
know what's unclear.

Thanks,

Lukas

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