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, with 4KB intervals since 0x1000 is easier
to read than 0x400
v4: use RESOURCE_SIZE_MAX, split allocate_resource and KUNIT_ASSERT_EQ,
and other cosmetic changes
---
kernel/resource_kunit.c | 67 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/kernel/resource_kunit.c b/kernel/resource_kunit.c
index 58ab9f914602b..580c9ff6940f8 100644
--- a/kernel/resource_kunit.c
+++ b/kernel/resource_kunit.c
@@ -137,9 +137,76 @@ 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;
+ int ret;
+
+ ret = allocate_resource(&iomem_resource, &root, 0x100000,
+ 0, RESOURCE_SIZE_MAX, 0x100000, NULL, NULL);
+ KUNIT_ASSERT_EQ(test, 0, ret);
+
+ /* 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
On Wed, Jun 5, 2024 at 3:10 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Jun 05, 2024 at 02:28:26PM -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, with 4KB intervals since 0x1000 is easier
> > to read than 0x400
>
> Right, but...
>
> > v4: use RESOURCE_SIZE_MAX, split allocate_resource and KUNIT_ASSERT_EQ,
> > and other cosmetic changes
>
> ...
>
> > + ret = allocate_resource(&iomem_resource, &root, 0x100000,
> > + 0, RESOURCE_SIZE_MAX, 0x100000, NULL, NULL);
>
> Just double check that limits.h is included.
>
> ...
>
> > + /* 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);
>
> ...here is overlap with the previous resource.
>
> And here is the gap to the next one, in case we make that overlapping gone.
>
> > + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> > + IORESOURCE_SYSTEM_RAM);
>
> It wasn't the case in previous data. Please, elaborate what's going on here?
The test data is chosen to be
first interval: a matching resource (res[0])
second interval: a non-matching resource (res[1])
third interval: a hole
fourth interval: a matching resource (res[3]) nested in a
non-matching resource (res[2])
fifth interval: a matching resource (res[4])
The idea hasn't changed between revisions.
res[3] went from a half of res[2] to a quarter of res[2] in v4. I
guess it causes confusion if it is not viewed as a nested resource.
>
> ...
>
> And rather sending one version per 12h, take your time and think more about
> test data. What are we testing? Are the testing data correct? Shouldn't we also
> have negative test cases?
The current choice of test data covers the most common patterns. Do
you have other patterns you want to cover? I am new to the resource
code and that's why I am largely reactive to review feedback.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Jun 05, 2024 at 03:52:26PM -0700, Chia-I Wu wrote:
> On Wed, Jun 5, 2024 at 3:10 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Jun 05, 2024 at 02:28:26PM -0700, Chia-I Wu wrote:
> > > This mainly tests that find_next_iomem_res() does not miss resources.
...
> > > + /* 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);
> >
> > ...here is overlap with the previous resource.
> >
> > And here is the gap to the next one, in case we make that overlapping gone.
> >
> > > + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> > > + IORESOURCE_SYSTEM_RAM);
> >
> > It wasn't the case in previous data. Please, elaborate what's going on here?
> The test data is chosen to be
>
> first interval: a matching resource (res[0])
> second interval: a non-matching resource (res[1])
> third interval: a hole
> fourth interval: a matching resource (res[3]) nested in a
> non-matching resource (res[2])
> fifth interval: a matching resource (res[4])
>
> The idea hasn't changed between revisions.
>
> res[3] went from a half of res[2] to a quarter of res[2] in v4. I
> guess it causes confusion if it is not viewed as a nested resource.
Okay, so far it's correct data from testing p.o.v.
Maybe you can add a comment on top explaining this layout?
...
> > And rather sending one version per 12h, take your time and think more about
> > test data. What are we testing? Are the testing data correct? Shouldn't we also
> > have negative test cases?
> The current choice of test data covers the most common patterns. Do
> you have other patterns you want to cover? I am new to the resource
> code and that's why I am largely reactive to review feedback.
Nope, seems okay to have what is there for the starter. Later on we might add
more if required. Just got confused.
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 5, 2024 at 4:36 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Jun 05, 2024 at 03:52:26PM -0700, Chia-I Wu wrote:
> > On Wed, Jun 5, 2024 at 3:10 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Jun 05, 2024 at 02:28:26PM -0700, Chia-I Wu wrote:
> > > > This mainly tests that find_next_iomem_res() does not miss resources.
>
> ...
>
> > > > + /* 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);
> > >
> > > ...here is overlap with the previous resource.
> > >
> > > And here is the gap to the next one, in case we make that overlapping gone.
> > >
> > > > + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> > > > + IORESOURCE_SYSTEM_RAM);
> > >
> > > It wasn't the case in previous data. Please, elaborate what's going on here?
> > The test data is chosen to be
> >
> > first interval: a matching resource (res[0])
> > second interval: a non-matching resource (res[1])
> > third interval: a hole
> > fourth interval: a matching resource (res[3]) nested in a
> > non-matching resource (res[2])
> > fifth interval: a matching resource (res[4])
> >
> > The idea hasn't changed between revisions.
> >
> > res[3] went from a half of res[2] to a quarter of res[2] in v4. I
> > guess it causes confusion if it is not viewed as a nested resource.
>
> Okay, so far it's correct data from testing p.o.v.
>
> Maybe you can add a comment on top explaining this layout?
Done in v5. I also added negative tests for all holes in the test
data. Hope it's better now.
>
> ...
>
> > > And rather sending one version per 12h, take your time and think more about
> > > test data. What are we testing? Are the testing data correct? Shouldn't we also
> > > have negative test cases?
> > The current choice of test data covers the most common patterns. Do
> > you have other patterns you want to cover? I am new to the resource
> > code and that's why I am largely reactive to review feedback.
>
> Nope, seems okay to have what is there for the starter. Later on we might add
> more if required. Just got confused.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Jun 05, 2024 at 02:28:26PM -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, with 4KB intervals since 0x1000 is easier
> to read than 0x400
Right, but...
> v4: use RESOURCE_SIZE_MAX, split allocate_resource and KUNIT_ASSERT_EQ,
> and other cosmetic changes
...
> + ret = allocate_resource(&iomem_resource, &root, 0x100000,
> + 0, RESOURCE_SIZE_MAX, 0x100000, NULL, NULL);
Just double check that limits.h is included.
...
> + /* 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);
...here is overlap with the previous resource.
And here is the gap to the next one, in case we make that overlapping gone.
> + res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> + IORESOURCE_SYSTEM_RAM);
It wasn't the case in previous data. Please, elaborate what's going on here?
...
And rather sending one version per 12h, take your time and think more about
test data. What are we testing? Are the testing data correct? Shouldn't we also
have negative test cases?
--
With Best Regards,
Andy Shevchenko