Re: [PATCH v2 17/20] dax/hmem: Convey the dax range via memregion_info()

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 2/10/23 2:07 AM, Dan Williams wrote:
> In preparation for hmem platform devices to be unregistered, stop using
> platform_device_add_resources() to convey the address range. The
> platform_device_add_resources() API causes an existing "Soft Reserved"
> iomem resource to be re-parented under an inserted platform device
> resource. When that platform device is deleted it removes the platform
> device resource and all children.
>
> Instead, it is sufficient to convey just the address range and let
> request_mem_region() insert resources to indicate the devices active in
> the range. This allows the "Soft Reserved" resource to be re-enumerated
> upon the next probe event.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Tested-by: Fan Ni <fan.ni@xxxxxxxxxxx>
> Link: https://lore.kernel.org/r/167564543303.847146.11045895213318648441.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>

> ---
>   drivers/dax/hmem/device.c |   37 ++++++++++++++-----------------------
>   drivers/dax/hmem/hmem.c   |   14 +++-----------
>   include/linux/memregion.h |    2 ++
>   3 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/dax/hmem/device.c b/drivers/dax/hmem/device.c
> index 20749c7fab81..b1b339bccfe5 100644
> --- a/drivers/dax/hmem/device.c
> +++ b/drivers/dax/hmem/device.c
> @@ -15,15 +15,8 @@ static struct resource hmem_active = {
>   	.flags = IORESOURCE_MEM,
>   };
>   
> -void hmem_register_device(int target_nid, struct resource *r)
> +void hmem_register_device(int target_nid, struct resource *res)
>   {
> -	/* define a clean / non-busy resource for the platform device */
> -	struct resource res = {
> -		.start = r->start,
> -		.end = r->end,
> -		.flags = IORESOURCE_MEM,
> -		.desc = IORES_DESC_SOFT_RESERVED,
> -	};
>   	struct platform_device *pdev;
>   	struct memregion_info info;
>   	int rc, id;
> @@ -31,55 +24,53 @@ void hmem_register_device(int target_nid, struct resource *r)
>   	if (nohmem)
>   		return;
>   
> -	rc = region_intersects(res.start, resource_size(&res), IORESOURCE_MEM,
> -			IORES_DESC_SOFT_RESERVED);
> +	rc = region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
> +			       IORES_DESC_SOFT_RESERVED);
>   	if (rc != REGION_INTERSECTS)
>   		return;
>   
>   	id = memregion_alloc(GFP_KERNEL);
>   	if (id < 0) {
> -		pr_err("memregion allocation failure for %pr\n", &res);
> +		pr_err("memregion allocation failure for %pr\n", res);
>   		return;
>   	}
>   
>   	pdev = platform_device_alloc("hmem", id);
>   	if (!pdev) {
> -		pr_err("hmem device allocation failure for %pr\n", &res);
> +		pr_err("hmem device allocation failure for %pr\n", res);
>   		goto out_pdev;
>   	}
>   
> -	if (!__request_region(&hmem_active, res.start, resource_size(&res),
> +	if (!__request_region(&hmem_active, res->start, resource_size(res),
>   			      dev_name(&pdev->dev), 0)) {
> -		dev_dbg(&pdev->dev, "hmem range %pr already active\n", &res);
> +		dev_dbg(&pdev->dev, "hmem range %pr already active\n", res);
>   		goto out_active;
>   	}
>   
>   	pdev->dev.numa_node = numa_map_to_online_node(target_nid);
>   	info = (struct memregion_info) {
>   		.target_node = target_nid,
> +		.range = {
> +			.start = res->start,
> +			.end = res->end,
> +		},
>   	};
>   	rc = platform_device_add_data(pdev, &info, sizeof(info));
>   	if (rc < 0) {
> -		pr_err("hmem memregion_info allocation failure for %pr\n", &res);
> -		goto out_resource;
> -	}
> -
> -	rc = platform_device_add_resources(pdev, &res, 1);
> -	if (rc < 0) {
> -		pr_err("hmem resource allocation failure for %pr\n", &res);
> +		pr_err("hmem memregion_info allocation failure for %pr\n", res);
>   		goto out_resource;
>   	}
>   
>   	rc = platform_device_add(pdev);
>   	if (rc < 0) {
> -		dev_err(&pdev->dev, "device add failed for %pr\n", &res);
> +		dev_err(&pdev->dev, "device add failed for %pr\n", res);
>   		goto out_resource;
>   	}
>   
>   	return;
>   
>   out_resource:
> -	__release_region(&hmem_active, res.start, resource_size(&res));
> +	__release_region(&hmem_active, res->start, resource_size(res));
>   out_active:
>   	platform_device_put(pdev);
>   out_pdev:
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index c7351e0dc8ff..5025a8c9850b 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -15,25 +15,17 @@ static int dax_hmem_probe(struct platform_device *pdev)
>   	struct memregion_info *mri;
>   	struct dev_dax_data data;
>   	struct dev_dax *dev_dax;
> -	struct resource *res;
> -	struct range range;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return -ENOMEM;
>   
>   	mri = dev->platform_data;
> -	range.start = res->start;
> -	range.end = res->end;
> -	dax_region = alloc_dax_region(dev, pdev->id, &range, mri->target_node,
> -			PMD_SIZE, 0);
> +	dax_region = alloc_dax_region(dev, pdev->id, &mri->range,
> +				      mri->target_node, PMD_SIZE, 0);
>   	if (!dax_region)
>   		return -ENOMEM;
>   
>   	data = (struct dev_dax_data) {
>   		.dax_region = dax_region,
>   		.id = -1,
> -		.size = region_idle ? 0 : resource_size(res),
> +		.size = region_idle ? 0 : range_len(&mri->range),
>   	};
>   	dev_dax = devm_create_dev_dax(&data);
>   	if (IS_ERR(dev_dax))
> diff --git a/include/linux/memregion.h b/include/linux/memregion.h
> index bf83363807ac..c01321467789 100644
> --- a/include/linux/memregion.h
> +++ b/include/linux/memregion.h
> @@ -3,10 +3,12 @@
>   #define _MEMREGION_H_
>   #include <linux/types.h>
>   #include <linux/errno.h>
> +#include <linux/range.h>
>   #include <linux/bug.h>
>   
>   struct memregion_info {
>   	int target_node;
> +	struct range range;
>   };
>   
>   #ifdef CONFIG_MEMREGION