linux-kernel - Re: [PATCH v2 4/6] ACPI: HMAT: Refactor hmat_register_target_device to hmem_register

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f964eb62-5bc9-7e85-5c44-9027a6c08d4c@oracle.com>
Date:   Tue, 24 Mar 2020 19:40:56 +0000
From:   Joao Martins <joao.m.martins@...cle.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-acpi@...r.kernel.org,
        "Rafael J. Wysocki" <rjw@...ysocki.net>, peterz@...radead.org,
        dave.hansen@...ux.intel.com, ard.biesheuvel@...aro.org,
        linux-nvdimm@...ts.01.org, linux-kernel@...r.kernel.org,
        x86@...nel.org
Subject: Re: [PATCH v2 4/6] ACPI: HMAT: Refactor hmat_register_target_device
 to hmem_register_device


On 3/22/20 4:12 PM, Dan Williams wrote:
> In preparation for exposing "Soft Reserved" memory ranges without an
> HMAT, move the hmem device registration to its own compilation unit and
> make the implementation generic.
> 
> The generic implementation drops usage acpi_map_pxm_to_online_node()
> that was translating ACPI proximity domain values and instead relies on
> numa_map_to_online_node() to determine the numa node for the device.
> 
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> Link: https://lore.kernel.org/r/158318761484.2216124.2049322072599482736.stgit@dwillia2-desk3.amr.corp.intel.com
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  drivers/acpi/numa/hmat.c  |   68 ++++-----------------------------------------
>  drivers/dax/Kconfig       |    4 +++
>  drivers/dax/Makefile      |    3 +-
>  drivers/dax/hmem.c        |   56 -------------------------------------
>  drivers/dax/hmem/Makefile |    5 +++
>  drivers/dax/hmem/device.c |   64 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/dax/hmem/hmem.c   |   56 +++++++++++++++++++++++++++++++++++++
>  include/linux/dax.h       |    8 +++++
>  8 files changed, 144 insertions(+), 120 deletions(-)
>  delete mode 100644 drivers/dax/hmem.c
>  create mode 100644 drivers/dax/hmem/Makefile
>  create mode 100644 drivers/dax/hmem/device.c
>  create mode 100644 drivers/dax/hmem/hmem.c
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index a12e36a12618..134bcb40b2af 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -24,6 +24,7 @@
>  #include <linux/mutex.h>
>  #include <linux/node.h>
>  #include <linux/sysfs.h>
> +#include <linux/dax.h>
>  
>  static u8 hmat_revision;
>  static int hmat_disable __initdata;
> @@ -640,66 +641,6 @@ static void hmat_register_target_perf(struct memory_target *target)
>  	node_set_perf_attrs(mem_nid, &target->hmem_attrs, 0);
>  }
>  
> -static void hmat_register_target_device(struct memory_target *target,
          ^^^^ int ?

> -		struct resource *r)
> -{
> -	/* define a clean / non-busy resource for the platform device */
> -	struct resource res = {
> -		.start = r->start,
> -		.end = r->end,
> -		.flags = IORESOURCE_MEM,
> -	};
> -	struct platform_device *pdev;
> -	struct memregion_info info;
> -	int rc, id;
> -
> -	rc = region_intersects(res.start, resource_size(&res), IORESOURCE_MEM,
> -			IORES_DESC_SOFT_RESERVED);
> -	if (rc != REGION_INTERSECTS)
> -		return;
		^ return -ENXIO;

> -
> -	id = memregion_alloc(GFP_KERNEL);
> -	if (id < 0) {
> -		pr_err("memregion allocation failure for %pr\n", &res);
> -		return;
		^ return -ENOMEM;

> -	}
> -
> -	pdev = platform_device_alloc("hmem", id);
> -	if (!pdev) {

		rc = -ENOMEM;

> -		pr_err("hmem device allocation failure for %pr\n", &res);
> -		goto out_pdev;
> -	}
> -
> -	pdev->dev.numa_node = acpi_map_pxm_to_online_node(target->memory_pxm);
> -	info = (struct memregion_info) {
> -		.target_node = acpi_map_pxm_to_node(target->memory_pxm),
> -	};
> -	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_pdev;
> -	}
> -
> -	rc = platform_device_add_resources(pdev, &res, 1);
> -	if (rc < 0) {
> -		pr_err("hmem resource 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);
> -		goto out_resource;
> -	}
> -
> -	return;
	^^^^^^ return 0;
> -
> -out_resource:
> -	put_device(&pdev->dev);
> -out_pdev:
> -	memregion_free(id);

	return rc;

> -}
> -
>  static void hmat_register_target_devices(struct memory_target *target)
>  {
>  	struct resource *res;
> @@ -711,8 +652,11 @@ static void hmat_register_target_devices(struct memory_target *target)
>  	if (!IS_ENABLED(CONFIG_DEV_DAX_HMEM))
>  		return;
>  
> -	for (res = target->memregions.child; res; res = res->sibling)
> -		hmat_register_target_device(target, res);
> +	for (res = target->memregions.child; res; res = res->sibling) {
> +		int target_nid = acpi_map_pxm_to_node(target->memory_pxm);
> +
> +		hmem_register_device(target_nid, res);
> +	}
>  }
>  

If it makes sense to propagate error to hmem_register_device (as noted above),
then here perhaps a pr_err() if hmem registration fails mainly for reporting
purposes?

Regardless of the error nits, looks good overall. So, for what is worth:

  Reviewed-by: Joao Martins <joao.m.martins@...cle.com>

	Joao