Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Fix ATS state tracking via ats_prepared gate

[Samiullah Khawaja <skhawaja@google.com>]

On Tue, May 19, 2026 at 11:59:47AM -0300, Jason Gunthorpe wrote:
>On Tue, May 19, 2026 at 02:55:54PM +0000, Pranjal Shrivastava wrote:
>> On Tue, May 19, 2026 at 11:44:30AM -0300, Jason Gunthorpe wrote:
>> > On Tue, May 19, 2026 at 01:53:22PM +0000, Pranjal Shrivastava wrote:
>> > > @@ -4450,7 +4450,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>> > >  	if (dev_is_pci(dev)) {
>> > >  		unsigned int stu = __ffs(smmu->pgsize_bitmap);
>> > >
>> > > -		pci_prepare_ats(to_pci_dev(dev), stu);
>> > > +		if (!pci_prepare_ats(to_pci_dev(dev), stu))
>> > > +			master->ats_prepared = true;
>> > >  	}
>> >
>> > This should fail not keep going, it is a kernel bug if
>> > pci_prepare_ats() fails.

I agree with this approach, as it captures the bug at right place and we
can fix it. This maybe changes the simantics of prepare_ats(), but I
agreed with this.
>>
>> Are you suggesting it's a kernel bug if the STU doesn't match?
>> IIUC, it can fail because of the following cases (including the PF STU):
>>
>> 	if (!pci_ats_supported(dev))
>> 		return -EINVAL;
>
>It should not be called if ats is not supported, iommu driver bug.

So basically iommu drivers should call pci_ats_supported(dev) before
calling prepare_ats() and probe fails if prepare_ats() fails.
>
>> 	if (WARN_ON(dev->ats_enabled))
>> 		return -EBUSY;
>
>It should not be called if ats is already enabled, iommu driver bug.
>
>> 	if (ps < PCI_ATS_MIN_STU)
>> 		return -EINVAL;
>
>PAGE_SIZE is illegal somehow, build bug.
>
>> 	if (dev->is_virtfn) {
>> 		struct pci_dev *pdev = pci_physfn(dev);
>>
>> 		if (pdev->ats_stu != ps)
>> 			return -EINVAL;
>
>The kernel failed to setup the PF, iommu driver bug.
>
>> If ats isn't supported or if there's a STU mismatch, shuoldn't we just
>> ensure that we don't try enabling ATS for that device?
>
>No, the kernel is broken somehow.
>
>> Returning an error here would fail the device probe.. which seems like a
>> big hammer for devices that can't reliably use ATS..
>
>It has nothing to do with devices, the kernel is broken if it fails.
>
>> We have a special case in SMMUv3 where we pre-allocate stuff in prepare
>> but actually enable ATS in attach_commit, which IMO neeeds to be gated
>> based on client device's ATS capabilities?
>
>Yes, all of this should be gated by pci_ats_supported()
>
>But also failing to enable should translate into a failed attach.
>
>Jason

Sami