2024-06-05 19:53:41

by Chia-I Wu

Subject: [PATCH v3] resource: add a simple test for walk_iomem_res_desc()

This mainly tests that find_next_iomem_res() does not miss resources.

Signed-off-by: Chia-I Wu <[email protected]>

---
v2: update subject, use DEFINE_RES_NAMED and hardcoded offsets
v3: really hardcode offsets
---
kernel/resource_kunit.c | 65 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)

diff --git a/kernel/resource_kunit.c b/kernel/resource_kunit.c
index 58ab9f914602b..1a562d0978477 100644
--- a/kernel/resource_kunit.c
+++ b/kernel/resource_kunit.c
@@ -137,9 +137,74 @@ static void resource_test_intersection(struct kunit *test)
} while (++i < ARRAY_SIZE(results_for_intersection));
}

+static int resource_walk_count(struct resource *res, void *data)
+{
+ int *count = data;
+ (*count)++;
+ return 0;
+}
+
+static void resource_test_walk_iomem_res_desc(struct kunit *test)
+{
+ struct resource root = {
+ .name = "Resource Walk Test",
+ };
+ struct resource res[8];
+ int count;
+
+ KUNIT_ASSERT_EQ(test, 0,
+ allocate_resource(&iomem_resource, &root, 0x100000,
+ 0, ~0, 0x100000, NULL, NULL));
+
+ /* build the resource tree */
+ res[0] = DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYSRAM 1",
+ IORESOURCE_SYSTEM_RAM);
+ res[1] = DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTHER", 0);
+
+ res[2] = DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NESTED", 0);
+ res[3] = DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYSRAM 2",
+ IORESOURCE_SYSTEM_RAM);
+
+ res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
+ IORESOURCE_SYSTEM_RAM);
+
+ KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[0]));
+ KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[1]));
+ KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[2]));
+ KUNIT_EXPECT_EQ(test, 0, request_resource(&res[2], &res[3]));
+ KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[4]));
+
+ /* walk the entire region */
+ count = 0;
+ walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
+ root.start, root.end, &count, resource_walk_count);
+ KUNIT_EXPECT_EQ(test, count, 3);
+
+ /* walk the region requested by res[1] */
+ count = 0;
+ walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
+ res[1].start, res[1].end, &count, resource_walk_count);
+ KUNIT_EXPECT_EQ(test, count, 0);
+
+ /* walk the region requested by res[2] */
+ count = 0;
+ walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
+ res[2].start, res[2].end, &count, resource_walk_count);
+ KUNIT_EXPECT_EQ(test, count, 1);
+
+ /* walk the region requested by res[4] */
+ count = 0;
+ walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
+ res[4].start, res[4].end, &count, resource_walk_count);
+ KUNIT_EXPECT_EQ(test, count, 1);
+
+ release_resource(&root);
+}
+
static struct kunit_case resource_test_cases[] = {
KUNIT_CASE(resource_test_union),
KUNIT_CASE(resource_test_intersection),
+ KUNIT_CASE(resource_test_walk_iomem_res_desc),
{}
};

--
2.45.1.467.gbab1589fc0-goog

2024-06-05 20:33:48

by Andy Shevchenko

Subject: Re: [PATCH v3] resource: add a simple test for walk_iomem_res_desc()

On Wed, Jun 05, 2024 at 12:53:10PM -0700, Chia-I Wu wrote:
> This mainly tests that find_next_iomem_res() does not miss resources.

...

> v2: update subject, use DEFINE_RES_NAMED and hardcoded offsets
> v3: really hardcode offsets

This doesn't explain why you multiplied previous values.

...

> +static int resource_walk_count(struct resource *res, void *data)
> +{
> + int *count = data;

+ Blank line.

> + (*count)++;
> + return 0;
> +}

...

> +static void resource_test_walk_iomem_res_desc(struct kunit *test)
> +{
> + struct resource root = {
> + .name = "Resource Walk Test",
> + };
> + struct resource res[8];
> + int count;
> +
> + KUNIT_ASSERT_EQ(test, 0,
> + allocate_resource(&iomem_resource, &root, 0x100000,
> + 0, ~0, 0x100000, NULL, NULL));

Shouldn't this use RESOURCE_SIZE_MAX?

Please, split the assertion and allocate_resource() call, so it becomes
readable what exactly you checked against.

> + /* build the resource tree */
> + res[0] = DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYSRAM 1",
> + IORESOURCE_SYSTEM_RAM);
> + res[1] = DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTHER", 0);
> +
> + res[2] = DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NESTED", 0);
> + res[3] = DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYSRAM 2",
> + IORESOURCE_SYSTEM_RAM);
> +
> + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> + IORESOURCE_SYSTEM_RAM);
> +
> + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[0]));
> + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[1]));
> + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[2]));
> + KUNIT_EXPECT_EQ(test, 0, request_resource(&res[2], &res[3]));
> + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[4]));
> +
> + /* walk the entire region */
> + count = 0;
> + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> + root.start, root.end, &count, resource_walk_count);
> + KUNIT_EXPECT_EQ(test, count, 3);
> +
> + /* walk the region requested by res[1] */
> + count = 0;
> + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> + res[1].start, res[1].end, &count, resource_walk_count);
> + KUNIT_EXPECT_EQ(test, count, 0);
> +
> + /* walk the region requested by res[2] */
> + count = 0;
> + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> + res[2].start, res[2].end, &count, resource_walk_count);
> + KUNIT_EXPECT_EQ(test, count, 1);
> +
> + /* walk the region requested by res[4] */
> + count = 0;
> + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> + res[4].start, res[4].end, &count, resource_walk_count);
> + KUNIT_EXPECT_EQ(test, count, 1);
> +
> + release_resource(&root);
> +}

Other than the above, LGTM. Hopefully next version will be ready to apply.

--
With Best Regards,
Andy Shevchenko

2024-06-05 21:51:03

by Chia-I Wu

Subject: Re: [PATCH v3] resource: add a simple test for walk_iomem_res_desc()

On Wed, Jun 5, 2024 at 1:33 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Jun 05, 2024 at 12:53:10PM -0700, Chia-I Wu wrote:
> > This mainly tests that find_next_iomem_res() does not miss resources.
>
> ...
>
> > v2: update subject, use DEFINE_RES_NAMED and hardcoded offsets
> > v3: really hardcode offsets
>
> This doesn't explain why you multiplied previous values.
They were quite randomly picked. They were changed because multiples
of 0x1000 are easier to read than multiples of 0x400.

All suggestions have been incorporated in v4.
>
> ...
>
> > +static int resource_walk_count(struct resource *res, void *data)
> > +{
> > + int *count = data;
>
> + Blank line.
>
> > + (*count)++;
> > + return 0;
> > +}
>
> ...
>
> > +static void resource_test_walk_iomem_res_desc(struct kunit *test)
> > +{
> > + struct resource root = {
> > + .name = "Resource Walk Test",
> > + };
> > + struct resource res[8];
> > + int count;
> > +
> > + KUNIT_ASSERT_EQ(test, 0,
> > + allocate_resource(&iomem_resource, &root, 0x100000,
> > + 0, ~0, 0x100000, NULL, NULL));
>
> Shouldn't this use RESOURCE_SIZE_MAX?
>
> Please, split the assertion and allocate_resource() call, so it becomes
> readable what exactly you checked against.
>
> > + /* build the resource tree */
> > + res[0] = DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYSRAM 1",
> > + IORESOURCE_SYSTEM_RAM);
> > + res[1] = DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTHER", 0);
> > +
> > + res[2] = DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NESTED", 0);
> > + res[3] = DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYSRAM 2",
> > + IORESOURCE_SYSTEM_RAM);
> > +
> > + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> > + IORESOURCE_SYSTEM_RAM);
> > +
> > + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[0]));
> > + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[1]));
> > + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[2]));
> > + KUNIT_EXPECT_EQ(test, 0, request_resource(&res[2], &res[3]));
> > + KUNIT_EXPECT_EQ(test, 0, request_resource(&root, &res[4]));
> > +
> > + /* walk the entire region */
> > + count = 0;
> > + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > + root.start, root.end, &count, resource_walk_count);
> > + KUNIT_EXPECT_EQ(test, count, 3);
> > +
> > + /* walk the region requested by res[1] */
> > + count = 0;
> > + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > + res[1].start, res[1].end, &count, resource_walk_count);
> > + KUNIT_EXPECT_EQ(test, count, 0);
> > +
> > + /* walk the region requested by res[2] */
> > + count = 0;
> > + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > + res[2].start, res[2].end, &count, resource_walk_count);
> > + KUNIT_EXPECT_EQ(test, count, 1);
> > +
> > + /* walk the region requested by res[4] */
> > + count = 0;
> > + walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > + res[4].start, res[4].end, &count, resource_walk_count);
> > + KUNIT_EXPECT_EQ(test, count, 1);
> > +
> > + release_resource(&root);
> > +}
>
> Other than the above, LGTM. Hopefully next version will be ready to apply.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>