From: Bjorn Helgaas <helgaas@kernel.org>
To: Ethan Zhao <haifeng.zhao@linux.intel.com>
Cc: Baolu Lu <baolu.lu@linux.intel.com>,
bhelgaas@google.com, robin.murphy@arm.com, jgg@ziepe.ca,
kevin.tian@intel.com, dwmw2@infradead.org, will@kernel.org,
lukas@wunner.de, yi.l.liu@intel.com, dan.carpenter@linaro.org,
iommu@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-pci@vger.kernel.org, Haorong Ye <yehaorong@bytedance.com>
Subject: Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers
Date: Thu, 29 Feb 2024 16:33:02 -0600 [thread overview]
Message-ID: <20240229223302.GA363505@bhelgaas> (raw)
In-Reply-To: <c3585084-5587-49ca-bc2d-db92714a557b@linux.intel.com>
On Thu, Feb 29, 2024 at 09:58:43AM +0800, Ethan Zhao wrote:
> On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
> > On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
> > > On 2024/2/22 17:02, Ethan Zhao wrote:
> > > > Make pci_dev_is_disconnected() public so that it can be called from
> > > > Intel VT-d driver to quickly fix/workaround the surprise removal
> > > > unplug hang issue for those ATS capable devices on PCIe switch downstream
> > > > hotplug capable ports.
> > > >
> > > > Beside pci_device_is_present() function, this one has no config space
> > > > space access, so is light enough to optimize the normal pure surprise
> > > > removal and safe removal flow.
> > > >
> > > > Tested-by: Haorong Ye<yehaorong@bytedance.com>
> > > > Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> > > > ---
> > > > drivers/pci/pci.h | 5 -----
> > > > include/linux/pci.h | 5 +++++
> > > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > > Hi PCI subsystem maintainers,
> > >
> > > The iommu drivers (including, but not limited to, the Intel VT-d driver)
> > > require a helper to check the physical presence of a PCI device in two
> > > scenarios:
> > >
> > > - During the iommu_release_device() path: This ensures the device is
> > > physically present before sending device TLB invalidation to device.
> > This wording is fundamentally wrong. Testing
> > pci_dev_is_disconnected() can never ensure the device will still be
> > present by the time a TLB invalidation is sent.
>
> The logic of testing pci_dev_is_disconnected() in patch [2/3] works
> in the opposite:
>
> 1. if pci_dev_is_disconnected() return true, means the device is in
> the process of surprise removal handling, adapter already been
> removed from the slot.
>
> 2. for removed device, no need to send ATS invalidation request to it.
> removed device lost power, its devTLB wouldn't be valid anymore.
>
> 3. if pci_dev_is_disconnected() return false, the device is *likely*
> to be removed at any momoment after this function called.
> such case will be treated in the iommu ITE fault handling, not to
> retry the timeout request if device isn't present (patch [3/3]).
>
> > The device may be removed after the pci_dev_is_disconnected() test and
> > before a TLB invalidate is sent.
>
> even in the process while TLB is invalidating.
>
> > This is why I hesitate to expose pci_dev_is_disconnected() (and
> > pci_device_is_present(), which we already export) outside
> > drivers/pci/. They both lead to terrible mistakes like relying on the
> > false assumption that the result will remain valid after the functions
> > return, without any recognition that we MUST be able to deal with the
> > cases where that assumption is broken.
>
> Yup, your concern is worthy ,but isn't happening within this patchset.
OK, I acked the patch.
I guess my complaint is really with pci_device_is_present() because
that's even harder to use correctly.
pci_device_is_present():
slow (may do config access to device)
true => device *was* present in the recent past, may not be now
false => device is not accessible
pci_dev_is_disconnected():
fast (doesn't touch device)
true => device is not accessible
false => basically means nothing
I guess they're both hard ;)
Bjorn
next prev parent reply other threads:[~2024-02-29 22:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 9:02 [PATCH v13 0/3] fix vt-d hard lockup when hotplug ATS capable device Ethan Zhao
2024-02-22 9:02 ` [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers Ethan Zhao
2024-02-22 12:54 ` Baolu Lu
2024-02-23 6:47 ` Dan Carpenter
2024-02-23 7:35 ` Ethan Zhao
2024-02-26 23:05 ` Bjorn Helgaas
2024-02-29 1:58 ` Ethan Zhao
2024-02-29 22:33 ` Bjorn Helgaas [this message]
2024-03-01 2:03 ` Ethan Zhao
2024-02-29 22:26 ` Bjorn Helgaas
2024-02-22 9:02 ` [PATCH v13 2/3] iommu/vt-d: don't issue ATS Invalidation request when device is disconnected Ethan Zhao
2024-02-22 9:02 ` [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid Ethan Zhao
2024-02-22 11:24 ` Dan Carpenter
2024-02-23 2:29 ` Ethan Zhao
2024-02-23 6:08 ` Dan Carpenter
2024-02-23 7:32 ` Ethan Zhao
2024-02-23 8:19 ` Dan Carpenter
2024-02-26 2:48 ` Ethan Zhao
2024-02-26 20:00 ` Ethan Zhao
2024-02-27 4:54 ` Dan Carpenter
2024-02-26 22:52 ` Bjorn Helgaas
2024-02-27 2:30 ` Ethan Zhao
2024-02-27 16:29 ` Bjorn Helgaas
2024-02-28 9:00 ` Ethan Zhao
2024-02-26 11:48 ` kernel test robot
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=20240229223302.GA363505@bhelgaas \
--to=helgaas@kernel.org \
--cc=baolu.lu@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=dan.carpenter@linaro.org \
--cc=dwmw2@infradead.org \
--cc=haifeng.zhao@linux.intel.com \
--cc=iommu@lists.linux.dev \
--cc=jgg@ziepe.ca \
--cc=kevin.tian@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
--cc=yehaorong@bytedance.com \
--cc=yi.l.liu@intel.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).