On Sun, 05 Feb 2023 17:03:53 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> 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.
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Seems sensible to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> 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
>
