Re: [PATCH v5 07/10] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Pranjal Shrivastava <praan@google.com>]

On Mon, Mar 09, 2026 at 06:45:09PM -0700, Daniel Mentz wrote:
> On Mon, Jan 26, 2026 at 7:12 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> > +{
> > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +       int timeout = ARM_SMMU_SUSPEND_TIMEOUT_US;
> > +       u32 enables;
> > +       int ret;
> > +
> > +       /* Try to suspend the device, wait for in-flight submissions */
> > +       do {
> > +               if (atomic_cmpxchg(&smmu->nr_cmdq_users, 1, 0) == 1)
> 
> 
> I understand we must follow the rule: do not elide any invalidations
> while SMMU is still enabled. Otherwise, the following sequence of
> events might occur:
>  * The driver clears a table descriptor
>  * The TLBI command is skipped
>  * The driver frees the page that backed the translation table
>  * The page is re-allocated for some unrelated purpose
>  * SMMU performs a table walk and then descends into the page that has
> been reallocated, misinterpreting its contents.
> 
> Can you disable SMMU before setting nr_cmdq_users to 0?
> 

No, that'd be racy, if I disable the smmu but nr_cmdq_users != 0 an
"owner" thread might believe that the SMMU is still functional and poll
for consumption.

I don't think that the SMMU would perform a walk as we first set the
nr_cmdq_users == 0 to stop command submissions and then immediately set
GBPA to abort. IF a command submission raced with this, we wait for the
CMDQ to be drained before doing anything, the owner thread would poll
till sync in this case only then the driver would return from the unmap
call and get a chance to free the page.

> >
> > +       /* Disable everything */
> > +       arm_smmu_device_disable(smmu);
> > +       dev_dbg(dev, "Suspended smmu\n");
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> > +{
> > +       int ret;
> > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +
> > +       dev_dbg(dev, "Resuming device\n");
> 
> 
> Try to be consistent with these messages.  Here, it says "Resuming
> device" whereas above, it says "Suspended smmu". Also, try to align
> new messages with existing messages in the driver. I believe most
> other messages start with lowercase characters.

Ack. I'll update it in v6.

Thanks
Praan