Re: [PATCH 5/6] iommu/amd: Fail probe on ATS configuration failure

[Pranjal Shrivastava <praan@google.com>]

On Mon, Jun 01, 2026 at 03:01:25PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The newly added WARN_ON(ret) in pdev_enable_cap_ats() can be spuriously triggered for SR-IOV Virtual Functions (VFs) if the Physical Function (PF) was not properly configured for ATS.
> --
> 
> commit 40ae1c6d486f021eadc253f746299ffa4ca11dff
> Author: Pranjal Shrivastava <praan@google.com>
> 
> iommu/amd: Fail probe on ATS configuration failure
> 
> This commit updates the AMD IOMMU driver to call pci_prepare_ats() during
> device probe, failing the probe if it returns an error. It also adds a
> WARN_ON() in pdev_enable_cap_ats() to catch failures from pci_enable_ats().
> 
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index 4ef6024c5a4ea..783f53cb85996 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -566,10 +566,17 @@ static inline int pdev_enable_cap_ats(struct pci_dev *pdev)
> >  	if (amd_iommu_iotlb_sup &&
> >  	    (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_ATS_SUP)) {
> >  		ret = pci_enable_ats(pdev, PAGE_SHIFT);
> > -		if (!ret) {
> > -			dev_data->ats_enabled = 1;
> > -			dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
> > -		}
> > +
> > +		/*
> > +		 * pci_enable_ats() should not fail here because earlier
> > +		 * checks have already verified support & config.
> > +		 */
> > +		if (WARN_ON(ret))
> > +			return ret;
> 
> [Severity: High]
> Can this WARN_ON() trigger spuriously for SR-IOV Virtual Functions (VFs) if
> the Physical Function (PF) was not properly configured for ATS?
> 
> Looking at pci_prepare_ats(), it has a fast-path that returns 0 immediately
> for VFs without checking or configuring the PF's Smallest Translation Unit
> (STU):
> 
> int pci_prepare_ats(struct pci_dev *dev, int ps)
> {
>     ...
>     if (dev->is_virtfn)
>         return 0;
>     ...
> }
> 
> Later, when pdev_enable_cap_ats() calls pci_enable_ats() for the VF, the
> actual validation happens there:
> 
> int pci_enable_ats(struct pci_dev *dev, int ps)
> {
>     ...
>     if (pdev->ats_stu != ps)
>         return -EINVAL;
>     ...
> }
> 
> If the PF failed its IOMMU probe or was explicitly bypassed by the driver,
> its ats_stu would remain 0.
> 
> When VFs are subsequently probed, pci_prepare_ats() would blindly succeed,
> but pci_enable_ats() would fail with -EINVAL, triggering this new WARN_ON().
> 
> Could this lead to unnecessary bug reports or kernel crashes on systems
> configured with panic_on_warn?

No.. this error won't be hit because the path highlighted above is being
changed in the ATS series:

https://lore.kernel.org/all/20260601143644.2358771-3-praan@google.com/

Thanks,
Praan