Re: [PATCH rc v2] iommu/arm-smmu-v3: Fix inconsistent ATS state tracking

[Pranjal Shrivastava <praan@google.com>]

On Mon, May 04, 2026 at 01:23:06PM -0700, Nicolin Chen wrote:
> On Mon, May 04, 2026 at 07:33:07PM +0000, Pranjal Shrivastava wrote:
> > On Mon, May 04, 2026 at 11:01:42AM -0700, Nicolin Chen wrote:
> > > On Mon, May 04, 2026 at 04:38:42PM +0000, Pranjal Shrivastava wrote:
> > > I am thinking, maybe the call sites of pci_enable/disable_ats() can
> > > check to_pci_dev(dev)->ats_enabled instead of master->ats_enabled?
> > > 
> > > Then, we keep master->ats_enabled as-is, so detach() can revert the
> > > nr_ats_masters and ATS invalidation entry in domain->invs.
> > 
> > My suggestion in that case would be to update arm_smmu_ats_supported to
> > check if the client is a VF and check if it's PF enables ATS:
> > 
> > static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
> > {
> >     struct device *dev = master->dev;
> >     struct pci_dev *pdev;
> > 
> >     if (!dev_is_pci(dev))
> >         return false;
> > 
> >     pdev = to_pci_dev(dev);
> >     if (!pci_ats_supported(pdev))
> >         return false;
> > 
> >     /*
> >      * If this is a VF, ATS only works if the PF has already enabled it
> >      * with a valid STU.
> >      */
> >     if (pdev->is_virtfn && !pci_physfn(pdev)->ats_enabled)
> >         return false;
> > 
> >     return true;
> > }
> 
> I think that's okay to address the issue for now. But it assumes
> that pci_enable_ats() will never change so it won't fail for any
> other reason.
> 
> > and then in attach_prepare we can add the ats check for safety:
> > 
> >         if (!state->ats_enabled && master->ats_enabled) {
> >                 pci_disable_ats(to_pci_dev(master->dev));
> > 
> > +               if (pdev->ats_enabled)
> > +                       pci_disable_ats(to_pci_dev(master->dev));
> 
> master->ats_enabled is redundant if we check pdev->ats_enabled.
> 

BTW, is it really redundant? I assume it represents the driver's state 
machine? I believe the condition:

if (!state->ats_enabled && master->ats_enabled) 

translates to: The current state has ATS active, but the new target
state is to have them inactive. Isn't that the case?


> If we add a gate here, should add to pci_enable_ats() too?
> 

I see that pci_enable_ats already checks for it:

	if (WARN_ON(dev->ats_enabled))
		return -EBUSY;

Praan