From: Pranjal Shrivastava <praan@google•com>
To: Nicolin Chen <nicolinc@nvidia•com>
Cc: iommu@lists•linux.dev, Will Deacon <will@kernel•org>,
Joerg Roedel <joro@8bytes•org>,
Robin Murphy <robin.murphy@arm•com>,
Jason Gunthorpe <jgg@ziepe•ca>,
Mostafa Saleh <smostafa@google•com>,
Daniel Mentz <danielmentz@google•com>,
Ashish Mhetre <amhetre@nvidia•com>,
linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH v7 08/11] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops
Date: Thu, 28 May 2026 21:21:22 +0000 [thread overview]
Message-ID: <ahix0vUm03lxDwAC@google.com> (raw)
In-Reply-To: <ahiaApzK1/nBwr2R@Asurada-Nvidia>
On Thu, May 28, 2026 at 12:39:46PM -0700, Nicolin Chen wrote:
> On Wed, May 27, 2026 at 10:14:04PM +0000, Pranjal Shrivastava wrote:
> > +/* Runtime PM helpers */
> > +__maybe_unused static int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> > +{
> > + int ret;
> > +
> > + if (pm_runtime_enabled(smmu->dev)) {
> > + ret = pm_runtime_resume_and_get(smmu->dev);
> > + if (ret < 0) {
> > + dev_err(smmu->dev, "failed to resume device: %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
>
> Nit: ret is used within the first if.
>
> Yet, I wouldn't like it to move. So, maybe do an early return:
>
> if (!pm_runtime_enabled(smmu->dev))
> return 0;
>
Ack. I'll add an early return.
> > +__maybe_unused static void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> > +{
> > + int ret;
> > +
> > + if (pm_runtime_enabled(smmu->dev)) {
> > + ret = pm_runtime_put_autosuspend(smmu->dev);
> > + if (ret < 0)
> > + dev_err(smmu->dev, "failed to suspend device: %d\n", ret);
> > + }
> > +}
>
> Ditto
>
Same here.
> > +
> > +static inline u32 arm_smmu_cmdq_owner_prod_idx(struct arm_smmu_cmdq *cmdq)
> > +{
> > + return atomic_read(&cmdq->owner_prod) & CMDQ_PROD_IDX_MASK;
> > +}
> > +
> > static void parse_driver_options(struct arm_smmu_device *smmu)
> > {
> > int i = 0;
> > @@ -789,7 +822,8 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > /* b. Stop gathering work by clearing the owned flag */
> > prod = atomic_fetch_andnot_relaxed(CMDQ_PROD_OWNED_FLAG,
> > &cmdq->q.llq.atomic.prod);
> > - prod &= ~CMDQ_PROD_OWNED_FLAG;
> > + /* Strip all metadata flags */
> > + prod &= CMDQ_PROD_IDX_MASK;
>
> Should its prior atomic_fetch_andnot_relaxed() call do something
> about the CMDQ_PROD_STOP_FLAG as well?
Umm.. No, the atomic_fetch_andnot_relaxed() call must leave the STOP_FLAG.
This block is the owner-publish phase, which occurs *after* the Point of
Commitment. If a submission successfully reserved its indices before the
gate closed, it shall be allowed to finish.
If the owner thread cleared the STOP_FLAG here in the global memory, it
would prematurely re-open the gate, allowing new racing submissions to
leak in during the suspend sequence.
The algorithm works mainly because the RPM callbacks are the only ones
that are allowed to manipulate this flag.
>
> > +/*
> > + * Lockless pre-check to elide invalidations if SMMU is suspended.
> > + * Races with concurrent suspend are benign: the cmpxchg loop in
> > + * arm_smmu_cmdq_issue_cmdlist() acts as the true commit point.
> > + * If we lose the race, that loop observes Q_STOP == 1 and safely
> > + * drops the command. If we win, the suspend thread waits for us.
> > + */
> > +static inline bool arm_smmu_can_elide(struct arm_smmu_device *smmu)
> > +{
> > + return !!Q_STOP(READ_ONCE(smmu->cmdq.q.llq.prod));
> > +}
>
> arm_smmu_cmdq_can_elide()
Nice name! I'll update it.
>
> Should it handle a secondary_cmdq?
No, I don't think we need to check secondary queues here. The STOP_FLAG
being set on the primary CMDQ during the suspend should suffice to
indicate the entire SMMU's power state. That's why the function takes in
the smmu ptr and not the cmdq.
Altough, thanks for discussing this, I noticed that while rebasing I
missed the elision check at one place: arm_smmu_write_ste()
I'll take add a check in that too in the next version.
Did you have another scenario / use-case in mind where this might not
work?
Thanks,
Praan
next prev parent reply other threads:[~2026-05-28 21:21 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-27 22:13 [PATCH v7 00/11] iommu/arm-smmu-v3: Implement Runtime/System Sleep ops Pranjal Shrivastava
2026-05-27 22:13 ` [PATCH v7 01/11] iommu/arm-smmu-v3: Refactor arm_smmu_setup_irqs Pranjal Shrivastava
2026-05-27 22:13 ` [PATCH v7 02/11] iommu/arm-smmu-v3: Add a helper to drain cmd queues Pranjal Shrivastava
2026-05-28 1:35 ` Nicolin Chen
2026-05-28 10:34 ` Pranjal Shrivastava
2026-05-28 22:09 ` Nicolin Chen
2026-05-29 14:32 ` Pranjal Shrivastava
2026-05-27 22:13 ` [PATCH v7 03/11] iommu/tegra241-cmdqv: Add a helper to drain VCMDQs Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 04/11] iommu/tegra241-cmdqv: Restore PROD and CONS after resume Pranjal Shrivastava
2026-05-28 18:14 ` Nicolin Chen
2026-05-27 22:14 ` [PATCH v7 05/11] iommu/arm-smmu-v3: Cache and restore MSI config Pranjal Shrivastava
2026-05-28 18:36 ` Nicolin Chen
2026-05-28 21:57 ` Pranjal Shrivastava
2026-05-28 22:03 ` Nicolin Chen
2026-05-27 22:14 ` [PATCH v7 06/11] iommu/arm-smmu-v3: Handle gerror during suspend Pranjal Shrivastava
2026-05-28 18:53 ` Nicolin Chen
2026-05-28 21:59 ` Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 07/11] iommu/arm-smmu-v3: Add CMDQ_PROD_STOP_FLAG to gate CMDQ submissions Pranjal Shrivastava
2026-05-28 19:41 ` Nicolin Chen
2026-05-28 21:57 ` Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 08/11] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops Pranjal Shrivastava
2026-05-28 19:39 ` Nicolin Chen
2026-05-28 21:21 ` Pranjal Shrivastava [this message]
2026-05-28 22:13 ` Nicolin Chen
2026-05-28 23:30 ` Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 09/11] iommu/arm-smmu-v3: Enable pm_runtime and setup devlinks Pranjal Shrivastava
2026-05-28 20:13 ` Nicolin Chen
2026-05-28 21:36 ` Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 10/11] iommu/arm-smmu-v3: Invoke pm_runtime before hw access Pranjal Shrivastava
2026-05-28 20:28 ` Nicolin Chen
2026-05-28 21:46 ` Pranjal Shrivastava
2026-05-28 22:01 ` Nicolin Chen
2026-05-28 22:25 ` Pranjal Shrivastava
2026-05-28 23:18 ` Nicolin Chen
2026-05-29 14:48 ` Pranjal Shrivastava
2026-06-03 22:18 ` Daniel Mentz
2026-06-04 7:18 ` Pranjal Shrivastava
2026-05-27 22:14 ` [PATCH v7 11/11] iommu/arm-smmu-v3: Add KUnit unit tests for Runtime PM Pranjal Shrivastava
2026-05-28 21:43 ` Nicolin Chen
2026-05-28 23:10 ` Pranjal Shrivastava
2026-05-28 23:21 ` Nicolin Chen
2026-05-28 23:33 ` Pranjal Shrivastava
2026-05-28 18:05 ` [PATCH v7 00/11] iommu/arm-smmu-v3: Implement Runtime/System Sleep ops Nicolin Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ahix0vUm03lxDwAC@google.com \
--to=praan@google$(echo .)com \
--cc=amhetre@nvidia$(echo .)com \
--cc=danielmentz@google$(echo .)com \
--cc=iommu@lists$(echo .)linux.dev \
--cc=jgg@ziepe$(echo .)ca \
--cc=joro@8bytes$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=nicolinc@nvidia$(echo .)com \
--cc=robin.murphy@arm$(echo .)com \
--cc=smostafa@google$(echo .)com \
--cc=will@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox