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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] ATC Invalidation is sent before ATS is enabled on the PCIe endpoint, violating PCIe specifications and causing Unsupported Request (UR) completions.
--

commit a735f1337ddc38ee7c9d344d6b05b83e1d9050f4
Author: Pranjal Shrivastava <praan@google.com>

iommu/arm-smmu-v3: Standardize ATS enablement failure reporting

The SMMUv3 driver currently has a two-phase commit in its ATS enablement
flow. This commit updates 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.

Link: https://lore.kernel.org/all/cover.1779392420.git.nicolinc@nvidia.com/

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index a10affb483a4f..aaebd72bc48dc 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2956,8 +2956,14 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
>  	 * ATC invalidation of PASID 0 causes the entire ATC to be flushed.
>  	 */
>  	arm_smmu_atc_inv_master(master, IOMMU_NO_PASID);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to send an ATC
invalidation before ATS is enabled on the PCIe endpoint?

According to the PCIe Base Specification, if the Enable bit in the ATS
Control Register is clear, the Function must respond to Invalidate Requests
with an Unsupported Request (UR) Completion status.

Since the SMMUv3 architecture treats a UR completion to an ATC Invalidate as
an ATC_INV CERROR, doesn't this halt the command queue and raise a Global
Error (GERROR) interrupt?

Looking at the driver's GERROR handler, it handles this by returning
without advancing the CONS pointer past the waiting CMD_SYNC. Because the
UR is a hard error rather than a transient timeout, wouldn't retrying the
CMD_SYNC continuously fail, resulting in an infinite GERROR interrupt loop
and a completely stalled SMMU command queue?

Should pci_enable_ats() be called before sending the ATC invalidate command?

> -	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.
> +	 */
> +	WARN(pci_enable_ats(pdev, stu),
> +	     "Failed to enable ATS (STU %zu)\n", stu);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528202353.3422206-1-praan@google.com?part=4