Received-SPF: pass (google.com: domain of linux-kernel+bounces-87644-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 29 Feb 2024 16:33:02 -0600
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
Message-ID: <20240229223302.GA363505@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
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