Received-SPF: pass (google.com: domain of linux-kernel+bounces-82432-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 26 Feb 2024 17:05:37 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Baolu Lu <baolu.lu@linux.intel.com>
Cc: Ethan Zhao <haifeng.zhao@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: <20240226230537.GA10383@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9c4637d5-6496-4c68-b2db-4be1e56ca746@linux.intel.com>

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 device may be removed after the pci_dev_is_disconnected() test and
before a TLB invalidate is sent.

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.

This series claims to avoid "continuous hard lockup warnings and
system hangs".  It may reduce the likelihood, but I don't think it can
completely avoid them.  

I don't see any acknowledgement of that in the commit logs of this
series.  E.g., it doesn't say "we can recover from ATS Invalidate
Completion Timeouts, but the timeouts are on the order of minutes, so
we want to avoid them when possible."

And given the "system hangs" language, I am not convinced that we
actually *can* recover from those timeouts.

Using pci_dev_is_disconnected() may make those timeouts less frequent
and give the illusion that the problem is "solved", but it just means
the problem is still there and harder to reproduce.

> - During the device driver lifecycle when a device TLB invalidation
>   timeout event is generated by the IOMMU hardware: This helps handle
>   situations where the device might have been hot-removed.
> 
> While there may be some adjustments needed in patch 3/3, I'd like to
> confirm with you whether it's feasible to expose this helper for general
> use within the iommu subsystem.
> 
> If you agree with this change, I can route this patch to Linus through
> the iommu tree with an "acked-by" or "reviewed-by" tag from you.
> 
> Best regards,
> baolu