Per the PCIe spec, behavior of the PASID capability is undefined if the
value of the PASID Enable bit changes while the Enable bit of the
function's ATS control register is Set. Unfortunately,
pdev_enable_caps() does exactly that by ordering enabling ATS for the
device before enabling PASID.

Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Vasant Hegde <vasant.hegde@amd.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Fixes: eda8c2860ab679 ("iommu/amd: Enable device ATS/PASID/PRI capabilities independently")
Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com>
---
 drivers/iommu/amd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3117d99cf83d..8b8d3e843743 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -634,8 +634,8 @@ static inline void pdev_disable_cap_pasid(struct pci_dev *pdev)
 
 static void pdev_enable_caps(struct pci_dev *pdev)
 {
-	pdev_enable_cap_ats(pdev);
 	pdev_enable_cap_pasid(pdev);
+	pdev_enable_cap_ats(pdev);
 	pdev_enable_cap_pri(pdev);
 }
 
-- 
2.43.0

On 7/3/2025 9:24 PM, Easwar Hariharan wrote:
> Per the PCIe spec, behavior of the PASID capability is undefined if the
> value of the PASID Enable bit changes while the Enable bit of the
> function's ATS control register is Set. Unfortunately,
> pdev_enable_caps() does exactly that by ordering enabling ATS for the
> device before enabling PASID.
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Vasant Hegde <vasant.hegde@amd.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: eda8c2860ab679 ("iommu/amd: Enable device ATS/PASID/PRI capabilities independently")
> Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com>

Patch looks good.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


-Vasant



> ---
>  drivers/iommu/amd/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 3117d99cf83d..8b8d3e843743 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -634,8 +634,8 @@ static inline void pdev_disable_cap_pasid(struct pci_dev *pdev)
>  
>  static void pdev_enable_caps(struct pci_dev *pdev)
>  {
> -	pdev_enable_cap_ats(pdev);
>  	pdev_enable_cap_pasid(pdev);
> +	pdev_enable_cap_ats(pdev);
>  	pdev_enable_cap_pri(pdev);
>  }
>

On Thu, Jul 03, 2025 at 08:54:33AM -0700, Easwar Hariharan wrote:
> Per the PCIe spec, behavior of the PASID capability is undefined if the
> value of the PASID Enable bit changes while the Enable bit of the
> function's ATS control register is Set. Unfortunately,
> pdev_enable_caps() does exactly that by ordering enabling ATS for the
> device before enabling PASID.
> 
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Vasant Hegde <vasant.hegde@amd.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Fixes: eda8c2860ab679 ("iommu/amd: Enable device ATS/PASID/PRI capabilities independently")
> Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com>
> ---
>  drivers/iommu/amd/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

The other two drivers look like they do it Ok.

Jason