Received-SPF: pass (google.com: domain of linux-kernel+bounces-203183-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 5 Jun 2024 23:33:31 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Chia-I Wu <olvaffe@gmail.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
Subject: Re: [PATCH v3] resource: add a simple test for walk_iomem_res_desc()
Message-ID: <ZmDLm1xN68f_6Odg@smile.fi.intel.com>
References: <20240605195324.3213954-1-olvaffe@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240605195324.3213954-1-olvaffe@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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