Re: [RFC PATCH 3/5] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Pranjal Shrivastava <praan@google.com>]

On Tue, Mar 25, 2025 at 09:52:37PM -0700, Daniel Mentz wrote:
> On Tue, Mar 18, 2025 at 5:43 PM 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);
> > +
> > +       /* We might get the vcmdq */
> > +       struct arm_smmu_cmdq_ent cmd = {
> > +               .opcode = smmu->features & ARM_SMMU_FEAT_E2H ?
> > +                       CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA,
> > +       };
> > +
> > +       struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu, &cmd);
> > +       struct arm_smmu_ll_queue *llq = &cmdq->q.llq;
> > +
> > +       /*
> > +        * Since suspend is invoked when all clients have been
> > +        * we don't expect more commands to be added to the cmdq.
> > +        * Thus, wait for all existing commands to complete.
> > +        */
> > +       arm_smmu_cmdq_shared_lock(cmdq);
> > +       arm_smmu_cmdq_poll_until_empty(smmu, cmdq, llq);
> 
> llq is pointing to cmdq->q.llq which means that
> arm_smmu_cmdq_poll_until_empty will write to cmdq->q.llq.cons which I
> understand is not allowed unless you are holding the exclusive lock or
> are the only thread holding a shared lock.
> 
> How about the following:
> 
> 1. Call arm_smmu_cmdq_exclusive_trylock_irqsave(). If this fails,
> print an error message.
> 2. Call __arm_smmu_cmdq_poll_until_consumed on the current producer index.
> 3. Update cmdq->q.llq.cons
> 4. Call arm_smmu_cmdq_exclusive_unlock_irqrestore()
> 

As mentioned in the other thread, __arm_smmu_cmdq_poll_until_consumed
relies on `queue_consumed` which waits for the prod to cross cons idx.
We could potentially call poll_until_consumed on `prod - 1 but it feels`
hacky..relying on `queue_empty` should be better.

About locking, that is right, but I was assuming since we are in the
suspend callback here, no other client is operating the SMMU. Due to the
devlinks, I don't see a path that could be potentially racy here but if
we *somehow* fail to get the lock, I guess we should stop the suspend
along with logging the error message.

Do you see a case where we might not be the only owner of the cmdq while
suspending?

> 
> > +       arm_smmu_cmdq_shared_unlock(cmdq);
> > +
> > +       /* Disable all queues */
> > +       arm_smmu_device_disable(smmu);
> > +
> > +       /* Abort all transactions to avoid spurious bypass */
> > +       arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > +
> > +       /* Free all MSIs (re-allocated on resume) */
> > +       arm_smmu_free_msis(dev);
> > +
> > +       dev_dbg(dev, "Suspending smmu\n");
> > +       return 0;
> > +}

Thanks,
Praan