From: Nicolin Chen <nicolinc@nvidia•com>
To: Pranjal Shrivastava <praan@google•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 11/11] iommu/arm-smmu-v3: Add KUnit unit tests for Runtime PM
Date: Thu, 28 May 2026 14:43:57 -0700 [thread overview]
Message-ID: <ahi3HYssUY9mjjbg@Asurada-Nvidia> (raw)
In-Reply-To: <20260527221407.1756491-12-praan@google.com>
On Wed, May 27, 2026 at 10:14:07PM +0000, Pranjal Shrivastava wrote:
> Introduce a kunit selftests to verify the Runtime PM elision gating,
> post-suspend elisions and progress on resumption under active
> invalidation load. Simulate concurrent HW suspension using a timer.
> Mock all HW registers and CMDQ buffers by allocating them on RAM.
> Make the mock CMDQ self-consuming to avoid hitting queue_full scenarios.
>
> Signed-off-by: Pranjal Shrivastava <praan@google•com>
Maybe I am wrong, yet this reads like an entirely AI-generated code.
So, should it have an Assisted-by?
Documentation/process/submitting-patches.rst:637:Using Assisted-by:
Documentation/process/submitting-patches.rst:641:you need to acknowledge that use by adding an Assisted-by tag. Failure to
Documentation/process/coding-assistants.rst:44:Contributions should include an Assisted-by tag in the following format::
Documentation/process/coding-assistants.rst:46: Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Documentation/process/coding-assistants.rst:59: Assisted-by: Claude:claude-3-opus coccinelle sparse
> +struct arm_smmu_test_pm_context {
> + u32 *mock_prod_reg;
> + u64 *mock_base;
> + unsigned long *mock_valid_map;
> +};
It seems all about cmdq only, so maybe:
struct arm_smmu_mock_cmdq
?
> +/*
> + * Shared helper to allocate and map a fully functional, self-consuming mock
Is it really a "Shared" helper? I see only one caller..
> + * CMDQ. By redirecting both prod_reg and cons_reg to the same memory location,
> + * every producer write is instantly reflected as a consumer completion.
> + */
> +static struct arm_smmu_test_pm_context *
> +arm_smmu_v3_test_init_mock_cmdq(struct kunit *test, struct arm_smmu_device *smmu)
> +{
> + struct arm_smmu_test_pm_context *ctx;
> + struct arm_smmu_cmdq *cmdq = &smmu->cmdq;
> +
> + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_NULL(test, ctx);
This ctx struct isn't that large. Maybe the test function (caller)
can put on the stack and pass its point int than allocation?
> + /* Use a standard 1024-entry queue for safe word bitmask alignment */
> + ctx->mock_prod_reg = kunit_kzalloc(test, sizeof(u32), GFP_KERNEL);
Is a u32 allocation necessary?
> + ctx->mock_base = kunit_kzalloc(test, 1024 * 16, GFP_KERNEL);
s/16/sizeof(struct arm_smmu_cmd)
> + ctx->mock_valid_map = kunit_kzalloc(test, BITS_TO_LONGS(1024) * sizeof(long), GFP_KERNEL);
> +
> + KUNIT_ASSERT_NOT_NULL(test, ctx->mock_prod_reg);
> + KUNIT_ASSERT_NOT_NULL(test, ctx->mock_base);
> + KUNIT_ASSERT_NOT_NULL(test, ctx->mock_valid_map);
> +
> + smmu->features = 0;
> + /* 1024 entries */
> + cmdq->q.llq.max_n_shift = 10;
> + /* SMMUv3 commands are 2 dwords (16 bytes) */
> + cmdq->q.ent_dwords = 2;
CMDQ_ENT_DWORDS?
> + cmdq->q.base = (__le64 *)ctx->mock_base;
ctx->mock_base doesn't seem to have any other use? Why does it have
to be in the ctx structure?
> + cmdq->valid_map = (atomic_long_t *)ctx->mock_valid_map;
Same questions.
> + /* Self-Consuming, prod == cons always ensures queue empty */
> + cmdq->q.prod_reg = (__force u32 __iomem *)ctx->mock_prod_reg;
> + cmdq->q.cons_reg = (__force u32 __iomem *)ctx->mock_prod_reg;
> +
> + /* Initialize memory indices */
> + atomic_set(&cmdq->q.llq.atomic.prod, 0);
> + atomic_set(&cmdq->q.llq.atomic.cons, 0);
> + atomic_set(&cmdq->owner_prod, 0);
> + *ctx->mock_prod_reg = 0;
Arguably, mock_prod_reg is only used to test against a prod snapshot
so it doesn't seem necessary to be in a structure.
> + return ctx;
> +}
> +
> +struct arm_smmu_test_timer_context {
> + struct kunit *test;
> + struct arm_smmu_device *smmu;
> + struct arm_smmu_test_pm_context *pm_ctx;
pm_ctx doesn't seem to be used by the timer callback function?
Is it necessary in this structure? Even "test" isn't used.
>. + struct timer_list timer;
> + bool stopped;
> +};
> +
> +/* Asynchronous timer callback running in softirq context */
Would be nicer to replace this with the inline comments:
/* Simulate a concurrent runtime suspend event interrupting the invalidations */
> +static void arm_smmu_v3_test_rpm_timer_callback(struct timer_list *t)
> +{
> + struct arm_smmu_test_timer_context *ctx =
> + timer_container_of(ctx, t, timer);
> + struct arm_smmu_cmdq *cmdq = &ctx->smmu->cmdq;
> +
> + /*
> + * Asynchronously set the STOP_FLAG (Close the gate).
> + * Simulate a concurrent runtime suspend event interrupting
> + * the invalidations happening under active load.
> + */
> + atomic_or(CMDQ_PROD_STOP_FLAG, &cmdq->q.llq.atomic.prod);
> +
> + /* Signal to the main storm loop that suspend has triggered */
> + WRITE_ONCE(ctx->stopped, true);
Then, it should be named "suspended".
> +}
> +
> +/*
> + * Verify SMMU PM Runtime gating, elision, and post-suspend resumption
> + * safety sequentially under active stress.
> + */
> +static void arm_smmu_v3_test_rpm_stress_race(struct kunit *test)
> +{
> + struct arm_smmu_device *smmu = kunit_kzalloc(test, sizeof(*smmu), GFP_KERNEL);
arm-smmu-v3-test has a global arm_smmu_device to use.
> + struct arm_smmu_test_pm_context *pm_ctx;
> + struct arm_smmu_test_timer_context *timer_ctx;
> + struct arm_smmu_cmdq *cmdq = &smmu->cmdq;
> + struct arm_smmu_cmd cmd = arm_smmu_make_cmd_cfgi_all();
Can we organize this a bit? Maybe in an inverted xmas tree?
Also, it would be nicer to run git-clang-format.
> + u32 stopped_prod;
> + int i, ret;
> +
> + KUNIT_ASSERT_NOT_NULL(test, smmu);
> + pm_ctx = arm_smmu_v3_test_init_mock_cmdq(test, smmu);
> +
> + timer_ctx = kunit_kmalloc(test, sizeof(*timer_ctx), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_NULL(test, timer_ctx);
The allocation doesn't seem necessary either. We use kunit_kzalloc
in this file only when the memory (usually an array) is too large.
> + timer_ctx->stopped = false;
Then, should have been kunit_kzalloc() at least. Or " = {0};", if
keep it on the stack.
> + /* Setup the kernel software timer */
> + timer_setup(&timer_ctx->timer, arm_smmu_v3_test_rpm_timer_callback, 0);
> +
> + /* Start the timer to fire in 10 jiffies */
> + mod_timer(&timer_ctx->timer, jiffies + msecs_to_jiffies(10));
Both lines reads pretty clear without that verbose comments...
> +
> + /* Execute the unmap storm until the timer triggers */
> + while (!READ_ONCE(timer_ctx->stopped)) {
> + ret = arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, &cmd, 1, false);
> + if (ret != 0) {
> + KUNIT_FAIL(test, "Unmap Storm loop failed to submit cmd list: %d\n", ret);
> + break;
> + }
As of now, arm_smmu_cmdq_issue_cmdlist() fails only on -ETIMEDOUT
and spits a print already. So, maybe simplify it:
if (arm_smmu_cmdq_issue_cmdlist())
break;
> static struct kunit_case arm_smmu_v3_test_cases[] = {
> + KUNIT_CASE(arm_smmu_v3_test_rpm_stress_race),
> KUNIT_CASE(arm_smmu_v3_write_ste_test_bypass_to_abort),
> KUNIT_CASE(arm_smmu_v3_write_ste_test_abort_to_bypass),
> KUNIT_CASE(arm_smmu_v3_write_ste_test_cdtable_to_abort),
Usually we append to the end. And following the naming convention:
arm_smmu_v3_rpm_test_stress_race
Nicolin
next prev parent reply other threads:[~2026-05-28 21:44 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
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 [this message]
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=ahi3HYssUY9mjjbg@Asurada-Nvidia \
--to=nicolinc@nvidia$(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=praan@google$(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