Re: [PATCH v6 4/6] iommu/arm-smmu-v3: Standardize ATS enablement failure reporting

[Nicolin Chen]

On Fri, May 29, 2026 at 11:12:06AM +0000, Pranjal Shrivastava wrote:
> The SMMUv3 driver currently has a two-phase commit in its ATS enablement
> flow. During arm_smmu_attach_prepare(), it predicts whether ATS will be
> enabled using arm_smmu_ats_supported() and accordingly increments
> nr_ats_masters and merges ATS invalidations into the domain's invs array.
> 
> However, the actual hardware enablement via pci_enable_ats() happens
> later in arm_smmu_attach_commit(). If this call to pci_enable_ats fails,
> the SMMU driver's ATS state tracking remains polluted, i.e., the driver
> tracks ATS as enabled on a master that is not actually using it. This
> leads to an incorrect nr_ats_masters and triggers a warning in the PCI
> core during detach:
> 
>  1 [  127.925080] ------------[ cut here ]------------
>  2 [  127.925084] WARNING: drivers/pci/ats.c:132 at pci_disable_ats+0x94/0xa8
>  3 ...
>  4 [  128.068169] Call trace:
>  5 [  128.070603]  pci_disable_ats+0x94/0xa8 (P)
>  6 [  128.074688]  arm_smmu_attach_prepare+0x104/0x310
>  7 [  128.079292]  arm_smmu_attach_dev_ste+0x128/0x1e0
> 
> The issue was exposed under heavy load when running a VFIO-based DMA
> map stress test (iova_stress).
> 
> Following the addition of the arm_smmu_master_prepare_ats() [1] helper during
> device probe, failable ATS configuration (STU setup) is now handled early
> during probe. This ensures that any master reaching the attach phase is
> guaranteed to have a valid ATS configuration.
> 
> Update arm_smmu_enable_ats() to use the WARN() macro for any
> subsequent enablement failures during the commit phase. Since probe
> checks now preclude software configuration errors, any failure here is
> considered a kernel bug.

The commit message feels like mixing a stale background and the
real requirement (based on the latest code line). Could that DMA
map stress test still trigger the WARN_ON in pci_disable_ats(),
after having arm_smmu_master_prepare_ats()?

It'd be nicer if the writing can be simplified a bit.

>  	arm_smmu_atc_inv_master(master, IOMMU_NO_PASID);
> -	if (pci_enable_ats(pdev, stu))
> -		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
> +
> +	/*
> +	 * Any failure at this point is a kernel bug. pci_ats_supported()
> +	 * and pci_prepare_ats() have already verified the hardware capability
> +	 * and programmed the STU. Thus, pci_enable_ats() should not fail here.
> +	 */

The patch that removes pci_ats_supported() from pci_prepare_ats()
is dropped in this v6. So, my previous comments may stay true and
the two lines can be enough?

 	/*
 	 * As pci_prepare_ats() have already verified the hardware capability
 	 * and programmed the STE, pci_enable_ats() should not fail here.
 	 */

> +	WARN(pci_enable_ats(pdev, stu),
> +	     "Failed to enable ATS (STU %zu)\n", stu);

https://sashiko.dev/#/patchset/20260529111208.387412-1-praan%40google.com
Please check Sashiko review (for other patches in this series too).

I think it'd be cleaner to just have:

-	if (pci_enable_ats(pdev, stu))
+	if (WARN_ON(pci_enable_ats(pdev, stu)))

Nicolin