linux-kernel - Re: [PATCH v3] resource: add a simple test for walk_iomem_res

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 5 Jun 2024 14:32:03 -0700
From: Chia-I Wu <olvaffe@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	AKASHI Takahiro <takahiro.akashi@...aro.org>, Baoquan He <bhe@...hat.com>, 
	Dan Williams <dan.j.williams@...el.com>, Alison Schofield <alison.schofield@...el.com>, 
	linux-kernel@...r.kernel.org
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
<andriy.shevchenko@...ux.intel.com> 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
>
>