[PATCH v2 0/4] Support persistent memory as reserved type in e820/EFI

[PATCH v2 0/4] Support persistent memory as reserved type in e820/EFI

[Toshi Kani]

ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
firmware interfaces, e820, EFI, and ACPI NFIT table.  This EFI
change, however, leads to hit a bug in the grub bootloader, which
treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
stored user data [1].

Therefore, BIOS may set generic reserved type in e820 and EFI
to cover PMEM ranges.  The kernel can initialize PMEM ranges
from ACPI NFIT table alone.

This scheme cause a problem in the iomem table, though.  On x86,
for instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table at early boot-time.
This creates "reserved" entry for a PMEM range, which does not allow
region_intersects() to check with PMEM type.

This patch-set introduces devm_insert/remove_resource(), and changes
the NFIT driver to insert a PMEM entry to the iomem table.

[1] https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html

Patch 1 fixes __request_region() to handle inheritance of attribute
properly.  This patch is independent from this series.

Patch 2 adds remove_resource() interface.

Patch 3 adds devm_insert_resource() and devm_remove_resouce().  

Patch 4 changes the ACPI nfit driver to call devm_insert_resource() to
insert a PMEM range from NFIT.

---
v2:
 - Change the NFIT driver to call insert_resource() to insert a PMEM entry
   when necessary. (Dan Williams)
 - The NFIT driver still needs to allow unloading. (Dan Williams)

---
This patch-set is based on the tip tree, which has the following patchset.
https://lkml.org/lkml/2016/1/26/886

---
Toshi Kani (4):
 1/4 resource: Change __request_region to inherit from immediate parent
 2/4 resource: Add remove_resource interface
 3/4 resource: Add device-managed insert/remove_resource()
 4/4 ACPI: Change NFIT driver to insert new resource

---
 drivers/acpi/nfit.c    |  30 ++++++++++++
 include/linux/ioport.h |   6 +++
 kernel/resource.c      | 127 +++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 155 insertions(+), 8 deletions(-)

[PATCH v2 1/4] resource: Change __request_region to inherit from immediate parent

[Toshi Kani]

__request_region() sets 'flags' of a new resource from @parent
as it inherits the parent's attribute.  When a target resource
has a conflict, this function inserts the new resource entry
under the conflicted entry by updating @parent.  In this case,
the new resource entry needs to inherit attribute from the updated
parent.  This conflict is a typical case since __request_region()
is used to allocate a new resource from a specific resource range.

For instance, request_mem_region() calls __request_region() with
@parent set to &iomem_resource, which is the root entry of the
whole iomem range.  When this request results in inserting a new
entry "DEV-A" under "BUS-1", "DEV-A" needs to inherit from the
immediate parent "BUS-1" as it holds specific attribute for the
range.

root (&iomem_resource)
 :
 + "BUS-1"
    + "DEV-A"

Change __request_region() to set 'flags' and 'desc' of a new entry
from the immediate parent.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 kernel/resource.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 4d46605..5a56e8f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1085,15 +1085,16 @@ struct resource * __request_region(struct resource *parent,
 	res->name = name;
 	res->start = start;
 	res->end = start + n - 1;
-	res->flags = resource_type(parent) | resource_ext_type(parent);
-	res->flags |= IORESOURCE_BUSY | flags;
-	res->desc = IORES_DESC_NONE;
 
 	write_lock(&resource_lock);
 
 	for (;;) {
 		struct resource *conflict;
 
+		res->flags = resource_type(parent) | resource_ext_type(parent);
+		res->flags |= IORESOURCE_BUSY | flags;
+		res->desc = parent->desc;
+
 		conflict = __request_resource(parent, res);
 		if (!conflict)
 			break;

[PATCH v2 2/4] resource: Add remove_resource interface

[Toshi Kani]

insert_resource() and insert_resource_conflict() are called
by resource producers to insert a new resource.  When there
is any conflict, they move conflicting resources down to the
children of the new resource.  There is no destructor of these
interfaces, however.

Add remove_resource(), which removes a resource previously
inserted by insert_resource() or insert_resource_conflict(),
and moves the children up to where they were before.

__release_resource() is changed to have @release_child, so
that this function can be used for remove_resource() as well.

Also add comments to clarify that these functions are intended
for producers of resources to avoid any confusion with
request/release_resource() for consumers.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/ioport.h |    1 +
 kernel/resource.c      |   51 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index afb4559..8017b8b 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -174,6 +174,7 @@ extern void reserve_region_with_split(struct resource *root,
 extern struct resource *insert_resource_conflict(struct resource *parent, struct resource *new);
 extern int insert_resource(struct resource *parent, struct resource *new);
 extern void insert_resource_expand_to_fit(struct resource *root, struct resource *new);
+extern int remove_resource(struct resource *old);
 extern void arch_remove_reservations(struct resource *avail);
 extern int allocate_resource(struct resource *root, struct resource *new,
 			     resource_size_t size, resource_size_t min,
diff --git a/kernel/resource.c b/kernel/resource.c
index 5a56e8f..effb6ee 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -233,9 +233,9 @@ static struct resource * __request_resource(struct resource *root, struct resour
 	}
 }
 
-static int __release_resource(struct resource *old)
+static int __release_resource(struct resource *old, bool release_child)
 {
-	struct resource *tmp, **p;
+	struct resource *tmp, **p, *chd;
 
 	p = &old->parent->child;
 	for (;;) {
@@ -243,7 +243,17 @@ static int __release_resource(struct resource *old)
 		if (!tmp)
 			break;
 		if (tmp == old) {
-			*p = tmp->sibling;
+			if (release_child || !(tmp->child)) {
+				*p = tmp->sibling;
+			} else {
+				for (chd = tmp->child;; chd = chd->sibling) {
+					chd->parent = tmp->parent;
+					if (!(chd->sibling))
+						break;
+				}
+				*p = tmp->child;
+				chd->sibling = tmp->sibling;
+			}
 			old->parent = NULL;
 			return 0;
 		}
@@ -325,7 +335,7 @@ int release_resource(struct resource *old)
 	int retval;
 
 	write_lock(&resource_lock);
-	retval = __release_resource(old);
+	retval = __release_resource(old, true);
 	write_unlock(&resource_lock);
 	return retval;
 }
@@ -679,7 +689,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
 		old->start = new.start;
 		old->end = new.end;
 	} else {
-		__release_resource(old);
+		__release_resource(old, true);
 		*old = new;
 		conflict = __request_resource(root, old);
 		BUG_ON(conflict);
@@ -825,6 +835,9 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
  * entirely fit within the range of the new resource, then the new
  * resource is inserted and the conflicting resources become children of
  * the new resource.
+ *
+ * This function is intended for producers of resources, such as FW modules
+ * and bus drivers.
  */
 struct resource *insert_resource_conflict(struct resource *parent, struct resource *new)
 {
@@ -842,6 +855,9 @@ struct resource *insert_resource_conflict(struct resource *parent, struct resour
  * @new: new resource to insert
  *
  * Returns 0 on success, -EBUSY if the resource can't be inserted.
+ *
+ * This function is intended for producers of resources, such as FW modules
+ * and bus drivers.
  */
 int insert_resource(struct resource *parent, struct resource *new)
 {
@@ -885,6 +901,31 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
 	write_unlock(&resource_lock);
 }
 
+/**
+ * remove_resource - Remove a resource in the resource tree
+ * @old: resource to remove
+ *
+ * Returns 0 on success, -EINVAL if the resource is not valid.
+ *
+ * This function removes a resource previously inserted by insert_resource()
+ * or insert_resource_conflict(), and moves the children (if any) up to
+ * where they were before.  insert_resource() and insert_resource_conflict()
+ * insert a new resource, and move any conflicting resources down to the
+ * children of the new resource.
+ *
+ * insert_resource(), insert_resource_conflict() and remove_resource() are
+ * intended for producers of resources, such as FW modules and bus drivers.
+ */
+int remove_resource(struct resource *old)
+{
+	int retval;
+
+	write_lock(&resource_lock);
+	retval = __release_resource(old, false);
+	write_unlock(&resource_lock);
+	return retval;
+}
+
 static int __adjust_resource(struct resource *res, resource_size_t start,
 				resource_size_t size)
 {

[PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

[Toshi Kani]

insert_resource() and remove_resouce() are called by producers
of resources, such as FW modules and bus drivers.  These modules
may be implemented as loadable modules.

Add device-managed implementaions of insert_resource() and
remove_resouce() functions.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/ioport.h |    5 +++
 kernel/resource.c      |   69 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8017b8b..3580038 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev,
 
 extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
+
+extern int devm_insert_resource(struct device *dev, struct resource *root,
+				 struct resource *new);
+extern void devm_remove_resource(struct device *dev, struct resource *old);
+
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
 extern int iomem_is_exclusive(u64 addr);
 
diff --git a/kernel/resource.c b/kernel/resource.c
index effb6ee..b1a3394 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent,
 EXPORT_SYMBOL(__devm_release_region);
 
 /*
+ * Remove function for devm_insert_resource() and devm_remove_resource().
+ */
+static void devm_resource_remove(struct device *dev, void *ptr)
+{
+	struct resource **r = ptr;
+
+	remove_resource(*r);
+}
+
+/**
+ * devm_insert_resource() - insert an I/O or memory resource
+ * @dev: device for which to produce the resource
+ * @root: root of the resource tree
+ * @new: descriptor of the new resource
+ *
+ * This is a device-managed version of insert_resource(). There is usually
+ * no need to release resources requested by this function explicitly since
+ * that will be taken care of when the device is unbound from its bus driver.
+ * If for some reason the resource needs to be released explicitly, because
+ * of ordering issues for example, bus drivers must call devm_remove_resource()
+ * rather than the regular remove_resource().
+ *
+ * devm_insert_resource() is intended for producers of resources, such as
+ * FW modules and bus drivers.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int devm_insert_resource(struct device *dev, struct resource *root,
+			  struct resource *new)
+{
+	struct resource **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_resource_remove, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	*ptr = new;
+
+	ret = insert_resource(root, new);
+	if (ret) {
+		dev_err(dev, "unable to insert resource: %pR (%d)\n", new, ret);
+		devres_free(ptr);
+		return -EBUSY;
+	}
+
+	devres_add(dev, ptr);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_insert_resource);
+
+/**
+ * devm_remove_resource() - remove a previously inserted resource
+ * @dev: device for which to remove the resource
+ * @old: descriptor of the resource
+ *
+ * Remove a resource previously inserted using devm_insert_resource().
+ *
+ * devm_remove_resource() is intended for producers of resources, such as
+ * FW modules and bus drivers.
+ */
+void devm_remove_resource(struct device *dev, struct resource *old)
+{
+	WARN_ON(devres_release(dev, devm_resource_remove, devm_resource_match,
+			       old));
+}
+EXPORT_SYMBOL_GPL(devm_remove_resource);
+
+/*
  * Called from init/main.c to reserve IO ports.
  */
 #define MAXRESERVE 4

Re: [PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

[Dan Williams]

On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> insert_resource() and remove_resouce() are called by producers
> of resources, such as FW modules and bus drivers.  These modules
> may be implemented as loadable modules.
>
> Add device-managed implementaions of insert_resource() and
> remove_resouce() functions.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> ---
>  include/linux/ioport.h |    5 +++
>  kernel/resource.c      |   69 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 8017b8b..3580038 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev,
>
>  extern void __devm_release_region(struct device *dev, struct resource *parent,
>                                   resource_size_t start, resource_size_t n);
> +
> +extern int devm_insert_resource(struct device *dev, struct resource *root,
> +                                struct resource *new);
> +extern void devm_remove_resource(struct device *dev, struct resource *old);
> +
>  extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
>  extern int iomem_is_exclusive(u64 addr);
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index effb6ee..b1a3394 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent,
>  EXPORT_SYMBOL(__devm_release_region);
>
>  /*
> + * Remove function for devm_insert_resource() and devm_remove_resource().
> + */
> +static void devm_resource_remove(struct device *dev, void *ptr)
> +{
> +       struct resource **r = ptr;
> +
> +       remove_resource(*r);
> +}
> +

Maybe call this __devm_remove_resource?  I think this makes it clearer
that it is a private helper function.  I was initially confused about
the difference between devm_resource_remove and devm_remove_resource.

Re: [PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

[Toshi Kani]

On Thu, 2016-03-03 at 14:45 -0800, Dan Williams wrote:
> On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
 :
> > 
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index effb6ee..b1a3394 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev,
> > struct resource *parent,
> >  EXPORT_SYMBOL(__devm_release_region);
> > 
> >  /*
> > + * Remove function for devm_insert_resource() and
> > devm_remove_resource().
> > + */
> > +static void devm_resource_remove(struct device *dev, void *ptr)
> > +{
> > +       struct resource **r = ptr;
> > +
> > +       remove_resource(*r);
> > +}
> > +
> 
> Maybe call this __devm_remove_resource?  I think this makes it clearer
> that it is a private helper function.  I was initially confused about
> the difference between devm_resource_remove and devm_remove_resource.

Will do.  I followed the naming convention of devm_resource_release() in
the same file, but I agree that this is confusing.

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

[Toshi Kani]

ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
firmware interfaces, e820, EFI, and ACPI NFIT table.  This EFI
change, however, leads to hit a bug in the grub bootloader, which
treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
stored user data [1].

Therefore, BIOS may set generic reserved type in e820 and EFI
to cover PMEM ranges.  The kernel can initialize PMEM ranges
from ACPI NFIT table alone.

This scheme causes a problem in the iomem table, though.  On x86,
for instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table at early boot-time.
This creates "reserved" entry for a PMEM range, which does not allow
region_intersects() to check with PMEM type.

Change acpi_nfit_register_region() to call acpi_nfit_insert_resource(),
which calls devm_insert_resource() to insert a PMEM entry from NFIT
when the iomem table does not have a PMEM entry already.  That is,
when a PMEM range is marked as reserved type in e820, it inserts
"Persistent Memory" entry, which results as follows.

 + "Persistent Memory"
    + "reserved"

This allows the EINJ driver, which calls region_intersects() to
check PMEM ranges, to work continuously even if BIOS sets reserved
type (or sets nothing) to PMEM ranges in e820 and EFI.

[1]: https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/acpi/nfit.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index fb53db1..d97b53f 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus,
 	return 0;
 }
 
+static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
+		struct nd_region_desc *ndr_desc)
+{
+	struct resource *res, *nd_res = ndr_desc->res;
+	size_t size = nd_res->end - nd_res->start + 1;
+
+	/* No operation if the region is already registered as PMEM */
+	if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
+			IORES_DESC_PERSISTENT_MEMORY) == REGION_INTERSECTS)
+		return 0;
+
+	res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	res->name = "Persistent Memory";
+	res->start = nd_res->start;
+	res->end = nd_res->end;
+	res->flags = IORESOURCE_MEM;
+	res->desc = IORES_DESC_PERSISTENT_MEMORY;
+
+	return devm_insert_resource(acpi_desc->dev, &iomem_resource, res);
+}
+
 static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
 		struct nd_region_desc *ndr_desc)
 {
@@ -1781,6 +1805,12 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 
 	nvdimm_bus = acpi_desc->nvdimm_bus;
 	if (nfit_spa_type(spa) == NFIT_SPA_PM) {
+		rc = acpi_nfit_insert_resource(acpi_desc, ndr_desc);
+		if (rc)
+			dev_warn(acpi_desc->dev,
+				"failed to insert pmem resource to iomem: %d\n",
+				rc);
+
 		rc = acpi_nfit_find_poison(acpi_desc, ndr_desc);
 		if (rc) {
 			dev_err(acpi_desc->dev,

Re: [PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

[Dan Williams]

On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
> firmware interfaces, e820, EFI, and ACPI NFIT table.  This EFI
> change, however, leads to hit a bug in the grub bootloader, which
> treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
> stored user data [1].
>
> Therefore, BIOS may set generic reserved type in e820 and EFI
> to cover PMEM ranges.  The kernel can initialize PMEM ranges
> from ACPI NFIT table alone.
>
> This scheme causes a problem in the iomem table, though.  On x86,
> for instance, e820_reserve_resources() initializes top-level entries
> (iomem_resource.child) from the e820 table at early boot-time.
> This creates "reserved" entry for a PMEM range, which does not allow
> region_intersects() to check with PMEM type.
>
> Change acpi_nfit_register_region() to call acpi_nfit_insert_resource(),
> which calls devm_insert_resource() to insert a PMEM entry from NFIT
> when the iomem table does not have a PMEM entry already.  That is,
> when a PMEM range is marked as reserved type in e820, it inserts
> "Persistent Memory" entry, which results as follows.
>
>  + "Persistent Memory"
>     + "reserved"
>
> This allows the EINJ driver, which calls region_intersects() to
> check PMEM ranges, to work continuously even if BIOS sets reserved
> type (or sets nothing) to PMEM ranges in e820 and EFI.
>
> [1]: https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  drivers/acpi/nfit.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index fb53db1..d97b53f 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus,
>         return 0;
>  }
>
> +static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
> +               struct nd_region_desc *ndr_desc)
> +{
> +       struct resource *res, *nd_res = ndr_desc->res;
> +       size_t size = nd_res->end - nd_res->start + 1;
> +
> +       /* No operation if the region is already registered as PMEM */
> +       if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
> +                       IORES_DESC_PERSISTENT_MEMORY) == REGION_INTERSECTS)
> +               return 0;
> +
> +       res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);

How about allocating this resource on the stack and then have
devm_insert_resource handle the dynamic allocation (memdup) so we have
one less failure point to handle in the driver.

Re: [PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

[Toshi Kani]

On Thu, 2016-03-03 at 14:49 -0800, Dan Williams wrote:
> On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
 :
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index fb53db1..d97b53f 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct
> > nvdimm_bus *nvdimm_bus,
> >         return 0;
> >  }
> > 
> > +static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
> > +               struct nd_region_desc *ndr_desc)
> > +{
> > +       struct resource *res, *nd_res = ndr_desc->res;
> > +       size_t size = nd_res->end - nd_res->start + 1;
> > +
> > +       /* No operation if the region is already registered as PMEM */
> > +       if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
> > +                       IORES_DESC_PERSISTENT_MEMORY) ==
> > REGION_INTERSECTS)
> > +               return 0;
> > +
> > +       res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);
> 
> How about allocating this resource on the stack and then have
> devm_insert_resource handle the dynamic allocation (memdup) so we have
> one less failure point to handle in the driver.

I like the idea, but existing callers of insert_resource() allocate a
resource either statically or dynamically.  It may be contained by other
structure as well.  So, I think devm_insert_resource() should be consistent
with insert_resource() on this regard. 

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html