Re: [RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Pranjal Shrivastava <praan@google.com>]

On Sun, May 04, 2025 at 01:29:04PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> > @@ -139,6 +147,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> >                 .old_domain = iommu_get_domain_for_dev(dev),
> >                 .ssid = IOMMU_NO_PASID,
> >         };
> > +       struct arm_smmu_device *smmu = master->smmu;
> 
> Appears unused.
> 

Yes, left it by mistake, the kernel test robot complains about it too,
will remove this.

> > @@ -130,10 +130,10 @@ static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
> >         if (pm_runtime_enabled(smmu->dev))
> >                 return pm_runtime_get_if_in_use(smmu->dev);
> >
> > -       return 0;
> > +       return 1;
> 
> Can we fix this in the commit that added this function?
> 

Ugh, yes, my bad. Will move it to the appropriate commit.

> > @@ -2365,6 +2437,10 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> >         if (!size)
> >                 return;
> >
> > +       /* No need to invalidate TLBs if smmu is suspended */
> 
> The code doesn't check if the SMMU is suspended but rather whether the
> usage count is zero which is different. I'm concerned about the
> following sequence of events:
> 
>  * usage count drops to 0
>  * TLB invalidations are elided, stale TLB entries remain
>  * usage count becomes non-zero
> 
> This would result in stale TLB entries staying permanently. I propose
> adding a new member like "is_suspended" to struct arm_smmu_device that
> stores whether the SMMU device is suspended. If this value reads true
> (i.e. SMMU is suspended) then you can rely on the driver issuing
> TLBI_NSNH_ALL before re-enabling SMMU. Use proper acquire and release
> semantics when accessing this variable.

Hmm.. we aren't relying on the refcount alone but the rpm state as well.
Pasting the following lines from pm_runtime_get_if_in_use's doc strings:

 * Increment the runtime PM usage counter of @dev if its runtime PM
 * status is  %RPM_ACTIVE and its runtime PM usage counter is greater
 * than 0, in which case it returns 1.

Thus, we aren't only eliding the invalidations when the refcount == 0
but also when rpm state != RPM_ACTIVE which is checked first[1].

I believe you're suggesting a case where the rpm_state == RPM_ACTIVE
but the refcount bounces between 0 & 1 without changing the rpm state?

Skimming through the core runtime pm code[2], it seems it does nothing
to resume if current_state and/or last_state were RPM_ACTIVE. 

Let me dig deeper into the core runtime code to see if we're missing
something here.. otherwise, I agree with your suggestion to have a bool
to check if the smmu is really suspended.


Thanks,
Praan

[1]
https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/base/power/runtime.c#L1217

[2]
https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/base/power/runtime.c#L784