Received-SPF: pass (google.com: domain of linux-kernel+bounces-203298-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240605195324.3213954-1-olvaffe@gmail.com> <ZmDLm1xN68f_6Odg@smile.fi.intel.com>
In-Reply-To: <ZmDLm1xN68f_6Odg@smile.fi.intel.com>
From: Chia-I Wu <olvaffe@gmail.com>
Date: Wed, 5 Jun 2024 14:32:03 -0700
Message-ID: <CAPaKu7Qsvjq31LJJPMnpT64r7-5MTXaDQ7bNStK8V_QHw_fjTw@mail.gmail.com>
Subject: Re: [PATCH v3] resource: add a simple test for walk_iomem_res_desc()
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	AKASHI Takahiro <takahiro.akashi@linaro.org>, Baoquan He <bhe@redhat.com>, 
	Dan Williams <dan.j.williams@intel.com>, Alison Schofield <alison.schofield@intel.com>, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 5, 2024 at 1:33=E2=80=AFPM Andy Shevchenko
<andriy.shevchenko@linux.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 =3D data;
>
> + Blank line.
>
> > +     (*count)++;
> > +     return 0;
> > +}
>
> ...
>
> > +static void resource_test_walk_iomem_res_desc(struct kunit *test)
> > +{
> > +     struct resource root =3D {
> > +             .name =3D "Resource Walk Test",
> > +     };
> > +     struct resource res[8];
> > +     int count;
> > +
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     allocate_resource(&iomem_resource, &root, 0x10000=
0,
> > +                             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] =3D DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYSRAM =
1",
> > +                     IORESOURCE_SYSTEM_RAM);
> > +     res[1] =3D DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTHER",=
 0);
> > +
> > +     res[2] =3D DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NESTED"=
, 0);
> > +     res[3] =3D DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYSRAM =
2",
> > +                     IORESOURCE_SYSTEM_RAM);
> > +
> > +     res[4] =3D 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 =3D 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 =3D 0;
> > +     walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > +                     res[1].start, res[1].end, &count, resource_walk_c=
ount);
> > +     KUNIT_EXPECT_EQ(test, count, 0);
> > +
> > +     /* walk the region requested by res[2] */
> > +     count =3D 0;
> > +     walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > +                     res[2].start, res[2].end, &count, resource_walk_c=
ount);
> > +     KUNIT_EXPECT_EQ(test, count, 1);
> > +
> > +     /* walk the region requested by res[4] */
> > +     count =3D 0;
> > +     walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_SYSTEM_RAM,
> > +                     res[4].start, res[4].end, &count, resource_walk_c=
ount);
> > +     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
>
>