Re: [PATCH v4 3/5] iommu/arm-smmu-v3: Fix ATS state tracking

[Pranjal Shrivastava <praan@google.com>]

On Mon, May 25, 2026 at 01:05:36PM -0700, Nicolin Chen wrote:
> On Mon, May 25, 2026 at 06:43:45PM +0000, Pranjal Shrivastava wrote:
> > @@ -3065,8 +3065,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);
> > -	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.
> 
> Nits:
>  - WARN usually indicates a kernel bug already.
>  - pci_prepare_ats() covers pci_ats_supported().

Ack. For the pci_ats_supported.. since we're failing probe if
pci_prepare_ats() fails, the idea was to call prepare only if ATS was
supported [1] i.e. not penalise non-ATS callers of pci_prepare_ats. But
I agree.. maybe a better way would be to factor our pci_ats_supported().

> 
> 	/*
> 	 * As pci_prepare_ats() have already verified the hardware capability
> 	 * and programmed the STE, pci_enable_ats() should not fail here.
> 	 */
> 
> > @@ -4264,9 +4270,16 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
> >  		master->stall_enabled = true;
> >  
> >  	if (dev_is_pci(dev)) {
> > -		unsigned int stu = __ffs(smmu->pgsize_bitmap);
> > +		struct pci_dev *pdev = to_pci_dev(dev);
> >  
> > -		pci_prepare_ats(to_pci_dev(dev), stu);
> > +		if (pci_ats_supported(pdev)) {
> > +			unsigned int stu = __ffs(smmu->pgsize_bitmap);
> > +			int ret;
> > +
> > +			ret = pci_prepare_ats(pdev, stu);
> > +			if (ret)
> > +				return ERR_PTR(ret);
> > +		}
> 
> Again, pci_prepare_ats() covers pci_ats_supported(). So, the check
> is redundant. Instead, it should check arm_smmu_ats_supported().
> 

Ack, I'll add a check with arm_smmu_ats_supported

> By the way, this would conflict into my series:
> https://lore.kernel.org/linux-iommu/18bb6f421b3be891caa8f1fb50f3a4d56b52d5be.1779392420.git.nicolinc@nvidia.com/
> 
> It would be nicer to have an arm_smmu_master_prepare_ats() so that
> both series would have a common ground; mine would be just adding
> some additional lines if your series goes in first.

Ahh yes, I was thinking about the conflict to (I'm yet to review the
series though). That makes sense, we could add 
arm_smmu_master_prepare_ats() for clarity

Thanks,
Praan

[1] https://lore.kernel.org/all/20260519145947.GK7702@ziepe.ca/