[PATCH 0/5] Adding clear poison path for device DAX

[PATCH 0/5] Adding clear poison path for device DAX

[Dave Jiang]

The following series implements a way to clear poison for device DAX
through ndctl from user space. The application can acquire the region's
base address through the sysfs region resource attribute. From that an
ioctl can be used to send ND_CMD_CLEAR_ERROR for clearing the poison and
related badblocks.

---

Dave Jiang (5):
      libnvdimm: Add mechanism to publish badblocks at nd region level
      libnvdimm: Add resource sysfs attrib to nd region
      acpi: cleanup acpi_nfit_ctl calling xlat_status
      acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks
      libnvdimm: providing dax support for nvdimm testing


 drivers/acpi/nfit/core.c         |   78 ++++++++++++++++++++++++++++++++++----
 drivers/acpi/nfit/nfit.h         |    2 +
 drivers/dax/dax-private.h        |   40 +++++++++++++++++++
 drivers/dax/dax.c                |   29 ++------------
 drivers/nvdimm/bus.c             |   55 +++++++++++++++++++++++----
 drivers/nvdimm/core.c            |   17 ++++++--
 drivers/nvdimm/nd.h              |    1 
 drivers/nvdimm/region.c          |   33 ++++++++++++++++
 drivers/nvdimm/region_devs.c     |   32 ++++++++++++++++
 include/linux/libnvdimm.h        |    6 ++-
 tools/testing/nvdimm/Kbuild      |    3 +
 tools/testing/nvdimm/dax-dev.c   |   44 +++++++++++++++++++++
 tools/testing/nvdimm/test/nfit.c |   23 +++++++----
 13 files changed, 309 insertions(+), 54 deletions(-)
 create mode 100644 drivers/dax/dax-private.h
 create mode 100644 tools/testing/nvdimm/dax-dev.c

--
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/5] libnvdimm: Add mechanism to publish badblocks at nd region level

[Dave Jiang]

badblocks sysfs file will be export at nd_region level. When nvdimm event
notifier happens for NVDIMM_REVALIATE_POISON, the badblocks in the nd
region will be updated.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/nd.h          |    1 +
 drivers/nvdimm/region.c      |   24 ++++++++++++++++++++++++
 drivers/nvdimm/region_devs.c |   19 +++++++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 35dd750..d713bf9 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -154,6 +154,7 @@ struct nd_region {
 	u64 ndr_start;
 	int id, num_lanes, ro, numa_node;
 	void *provider_data;
+	struct badblocks bb;
 	struct nd_interleave_set *nd_set;
 	struct nd_percpu_lane __percpu *lane;
 	struct nd_mapping mapping[0];
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 8f24177..869a886 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/nd.h>
+#include "nd-core.h"
 #include "nd.h"
 
 static int nd_region_probe(struct device *dev)
@@ -52,6 +53,17 @@ static int nd_region_probe(struct device *dev)
 	if (rc && err && rc == err)
 		return -ENODEV;
 
+	if (is_nd_pmem(&nd_region->dev)) {
+		struct resource ndr_res;
+
+		if (devm_init_badblocks(dev, &nd_region->bb))
+			return -ENODEV;
+		ndr_res.start = nd_region->ndr_start;
+		ndr_res.end = nd_region->ndr_start + nd_region->ndr_size - 1;
+		nvdimm_badblocks_populate(nd_region,
+				&nd_region->bb, &ndr_res);
+	}
+
 	nd_region->btt_seed = nd_btt_create(nd_region);
 	nd_region->pfn_seed = nd_pfn_create(nd_region);
 	nd_region->dax_seed = nd_dax_create(nd_region);
@@ -104,6 +116,18 @@ static int child_notify(struct device *dev, void *data)
 
 static void nd_region_notify(struct device *dev, enum nvdimm_event event)
 {
+	if (event == NVDIMM_REVALIDATE_POISON) {
+		struct nd_region *nd_region = to_nd_region(dev);
+		struct resource res;
+
+		if (is_nd_pmem(&nd_region->dev)) {
+			res.start = nd_region->ndr_start;
+			res.end = nd_region->ndr_start +
+				nd_region->ndr_size - 1;
+			nvdimm_badblocks_populate(nd_region,
+					&nd_region->bb, &res);
+		}
+	}
 	device_for_each_child(dev, &event, child_notify);
 }
 
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 7cd705f..4d6db84 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -448,6 +448,21 @@ static ssize_t read_only_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(read_only);
 
+static ssize_t nd_badblocks_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nd_region *nd_region = to_nd_region(dev);
+
+	return badblocks_show(&nd_region->bb, buf, 0);
+}
+static struct device_attribute dev_attr_nd_badblocks = {
+	.attr = {
+		.name = "badblocks",
+		.mode = S_IRUGO
+	},
+	.show = nd_badblocks_show,
+};
+
 static struct attribute *nd_region_attributes[] = {
 	&dev_attr_size.attr,
 	&dev_attr_nstype.attr,
@@ -460,6 +475,7 @@ static struct attribute *nd_region_attributes[] = {
 	&dev_attr_available_size.attr,
 	&dev_attr_namespace_seed.attr,
 	&dev_attr_init_namespaces.attr,
+	&dev_attr_nd_badblocks.attr,
 	NULL,
 };
 
@@ -476,6 +492,9 @@ static umode_t region_visible(struct kobject *kobj, struct attribute *a, int n)
 	if (!is_nd_pmem(dev) && a == &dev_attr_dax_seed.attr)
 		return 0;
 
+	if (!is_nd_pmem(dev) && a == &dev_attr_nd_badblocks.attr)
+		return 0;
+
 	if (a != &dev_attr_set_cookie.attr
 			&& a != &dev_attr_available_size.attr)
 		return a->mode;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/5] libnvdimm: Add resource sysfs attrib to nd region

[Dave Jiang]

Adding sysfs attribute in order to export the physical address of the
ND region. This is for supporting of user app poison clear via
device dax.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/region_devs.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 4d6db84..56f8123 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -463,6 +463,15 @@ static struct device_attribute dev_attr_nd_badblocks = {
 	.show = nd_badblocks_show,
 };
 
+static ssize_t resource_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nd_region *nd_region = to_nd_region(dev);
+
+	return sprintf(buf, "%#llx\n", nd_region->ndr_start);
+}
+static DEVICE_ATTR_RO(resource);
+
 static struct attribute *nd_region_attributes[] = {
 	&dev_attr_size.attr,
 	&dev_attr_nstype.attr,
@@ -476,6 +485,7 @@ static struct attribute *nd_region_attributes[] = {
 	&dev_attr_namespace_seed.attr,
 	&dev_attr_init_namespaces.attr,
 	&dev_attr_nd_badblocks.attr,
+	&dev_attr_resource.attr,
 	NULL,
 };
 
@@ -495,6 +505,9 @@ static umode_t region_visible(struct kobject *kobj, struct attribute *a, int n)
 	if (!is_nd_pmem(dev) && a == &dev_attr_nd_badblocks.attr)
 		return 0;
 
+	if (!is_nd_pmem(dev) && a == &dev_attr_resource.attr)
+		return 0;
+
 	if (a != &dev_attr_set_cookie.attr
 			&& a != &dev_attr_available_size.attr)
 		return a->mode;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

[Dave Jiang]

Make sure that xlat_status is unconditionally called.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/core.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7361d00..9d4f461 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 	acpi_handle handle;
 	unsigned int func;
 	const u8 *uuid;
-	int rc, i;
+	int rc = 0, xlat_rc, i;
 
 	func = cmd;
 	if (cmd == ND_CMD_CALL) {
@@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 			 * unfilled in the output buffer
 			 */
 			rc = buf_len - offset - in_buf.buffer.length;
-			if (cmd_rc)
-				*cmd_rc = xlat_status(nvdimm, buf, cmd,
-						fw_status);
 		} else {
 			dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
 					__func__, dimm_name, cmd_name, buf_len,
 					offset);
 			rc = -ENXIO;
+			goto out;
 		}
-	} else {
-		rc = 0;
-		if (cmd_rc)
-			*cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
 	}
 
+	xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
+
+	if (cmd_rc)
+		*cmd_rc = xlat_rc;
+
  out:
 	ACPI_FREE(out_obj);
 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 4/5] acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks

[Dave Jiang]

Providing mechanism to clear poison list via the ndctl ND_CMD_CLEAR_ERROR
call. We will update the poison list and also the badblocks at region level
if the region is in dax mode or in pmem mode and not active.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/core.c         |   63 ++++++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/nfit.h         |    2 +
 drivers/nvdimm/bus.c             |   55 +++++++++++++++++++++++++++++----
 drivers/nvdimm/core.c            |   17 ++++++++--
 drivers/nvdimm/region.c          |    9 +++++
 include/linux/libnvdimm.h        |    6 +++-
 tools/testing/nvdimm/test/nfit.c |   23 ++++++++------
 7 files changed, 154 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9d4f461..9ca246f 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -94,6 +94,63 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
 	return to_acpi_device(acpi_desc->dev);
 }
 
+int acpi_nfit_forget_poison(struct nvdimm_bus_descriptor *nd_desc,
+		unsigned int cmd, void *buf)
+{
+	struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
+	struct nvdimm_bus *nvdimm_bus = acpi_desc->nvdimm_bus;
+	struct nfit_spa *nfit_spa;
+	struct nd_cmd_clear_error *clear_err = buf;
+	int found_match = 0;
+
+	if ((cmd != ND_CMD_CLEAR_ERROR) || !nvdimm_bus || !clear_err->cleared)
+		return 0;
+
+	/* clearing the poison list we keep track of */
+	__nvdimm_forget_poison(nvdimm_bus, clear_err->address,
+			clear_err->cleared);
+
+	mutex_lock(&acpi_desc->init_mutex);
+	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
+		struct acpi_nfit_system_address *spa = nfit_spa->spa;
+		resource_size_t spa_begin, spa_end, clear_begin, clear_end;
+
+		if (nfit_spa_type(spa) != NFIT_SPA_PM)
+			continue;
+
+		spa_begin = spa->address;
+		spa_end = spa->address + spa->length - 1;
+		clear_begin = clear_err->address;
+		clear_end = clear_err->address + clear_err->cleared - 1;
+
+		/* make sure clear_err range is within a SPA range */
+		if (((clear_begin >= spa_begin) &&
+					(clear_begin < (spa_end))) &&
+				((clear_end > spa_begin) &&
+				 (clear_end <= spa_end))) {
+			found_match = 1;
+			break;
+		}
+	}
+	mutex_unlock(&acpi_desc->init_mutex);
+
+	/* now sync the badblocks lists from the poison list */
+	if (found_match) {
+		struct resource res;
+
+		res.start = clear_err->address;
+		res.end = clear_err->address + clear_err->cleared - 1;
+		nvdimm_region_badblocks_clear(nfit_spa->nd_region, &res);
+	} else {
+		dev_dbg(acpi_desc->dev,
+			"Unable to find NFIT SPA that matches error addr\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_nfit_forget_poison);
+
 static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
 {
 	struct nd_cmd_clear_error *clear_err;
@@ -353,6 +410,12 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 	}
 
 	xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
+	if (xlat_rc >= 0) {
+		xlat_rc = acpi_nfit_forget_poison(nd_desc, cmd, buf);
+		if (xlat_rc)
+			dev_err(dev, "%s:%s %s unable to forget poison\n",
+					__func__, dimm_name, cmd_name);
+	}
 
 	if (cmd_rc)
 		*cmd_rc = xlat_rc;
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index fc29c2e..e881780 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -243,4 +243,6 @@ void __acpi_nvdimm_notify(struct device *dev, u32 event);
 int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc);
 void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev);
+int acpi_nfit_forget_poison(struct nvdimm_bus_descriptor *nd_desc,
+		unsigned int cmd, void *buf);
 #endif /* __NFIT_H__ */
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 23d4a17..9e76143 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -27,6 +27,7 @@
 #include <linux/nd.h>
 #include "nd-core.h"
 #include "nd.h"
+#include "pfn.h"
 
 int nvdimm_major;
 static int nvdimm_bus_major;
@@ -218,7 +219,8 @@ long nvdimm_clear_poison(struct device *dev, phys_addr_t phys,
 	if (cmd_rc < 0)
 		return cmd_rc;
 
-	nvdimm_clear_from_poison_list(nvdimm_bus, phys, len);
+	nvdimm_forget_poison(nvdimm_bus, phys, len);
+
 	return clear_err.cleared;
 }
 EXPORT_SYMBOL_GPL(nvdimm_clear_poison);
@@ -769,16 +771,55 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
 	} while (true);
 }
 
-static int pmem_active(struct device *dev, void *data)
+static int nd_pmem_forget_poison_check(struct device *dev, void *data)
 {
-	if (is_nd_pmem(dev) && dev->driver)
+	struct nd_cmd_clear_error *clear_err =
+		(struct nd_cmd_clear_error *)data;
+	struct nd_btt *nd_btt = is_nd_btt(dev) ? to_nd_btt(dev) : NULL;
+	struct nd_pfn *nd_pfn = is_nd_pfn(dev) ? to_nd_pfn(dev) : NULL;
+	struct nd_dax *nd_dax = is_nd_dax(dev) ? to_nd_dax(dev) : NULL;
+	struct nd_namespace_common *ndns = NULL;
+	struct nd_namespace_io *nsio;
+	resource_size_t offset = 0, end_trunc = 0, start, end, pstart, pend;
+
+	if (nd_dax || !dev->driver)
+		return 0;
+
+	start = clear_err->address;
+	end = clear_err->address + clear_err->cleared - 1;
+
+	if (nd_btt || nd_pfn || nd_dax) {
+		if (nd_btt)
+			ndns = nd_btt->ndns;
+		else if (nd_pfn)
+			ndns = nd_pfn->ndns;
+		else if (nd_dax)
+			ndns = nd_dax->nd_pfn.ndns;
+
+		if (!ndns)
+			return 0;
+	} else
+		ndns = to_ndns(dev);
+
+	nsio = to_nd_namespace_io(&ndns->dev);
+	pstart = nsio->res.start + offset;
+	pend = nsio->res.end - end_trunc;
+
+	if ((pstart >= start) && (pend <= end))
 		return -EBUSY;
+
 	return 0;
+
+}
+
+static int nd_ns_forget_poison_check(struct device *dev, void *data)
+{
+	return device_for_each_child(dev, data, nd_pmem_forget_poison_check);
 }
 
 /* set_config requires an idle interleave set */
 static int nd_cmd_clear_to_send(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm, unsigned int cmd)
+		struct nvdimm *nvdimm, unsigned int cmd, void *data)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
 
@@ -792,8 +833,8 @@ static int nd_cmd_clear_to_send(struct nvdimm_bus *nvdimm_bus,
 
 	/* require clear error to go through the pmem driver */
 	if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR)
-		return device_for_each_child(&nvdimm_bus->dev, NULL,
-				pmem_active);
+		return device_for_each_child(&nvdimm_bus->dev, data,
+				nd_ns_forget_poison_check);
 
 	if (!nvdimm || cmd != ND_CMD_SET_CONFIG_DATA)
 		return 0;
@@ -927,7 +968,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	}
 
 	nvdimm_bus_lock(&nvdimm_bus->dev);
-	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, cmd);
+	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, cmd, buf);
 	if (rc)
 		goto out_unlock;
 
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 9303cfe..40a3da0 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -574,14 +574,15 @@ int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
 }
 EXPORT_SYMBOL_GPL(nvdimm_bus_add_poison);
 
-void nvdimm_clear_from_poison_list(struct nvdimm_bus *nvdimm_bus,
-		phys_addr_t start, unsigned int len)
+void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t start,
+		unsigned int len)
 {
 	struct list_head *poison_list = &nvdimm_bus->poison_list;
 	u64 clr_end = start + len - 1;
 	struct nd_poison *pl, *next;
 
-	nvdimm_bus_lock(&nvdimm_bus->dev);
+	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
+
 	WARN_ON_ONCE(list_empty(poison_list));
 
 	/*
@@ -634,9 +635,17 @@ void nvdimm_clear_from_poison_list(struct nvdimm_bus *nvdimm_bus,
 			continue;
 		}
 	}
+}
+EXPORT_SYMBOL_GPL(__nvdimm_forget_poison);
+
+void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
+		phys_addr_t start, unsigned int len)
+{
+	nvdimm_bus_lock(&nvdimm_bus->dev);
+	__nvdimm_forget_poison(nvdimm_bus, start, len);
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
 }
-EXPORT_SYMBOL_GPL(nvdimm_clear_from_poison_list);
+EXPORT_SYMBOL_GPL(nvdimm_forget_poison);
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 int nd_integrity_init(struct gendisk *disk, unsigned long meta_size)
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 869a886..211435c 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -131,6 +131,15 @@ static void nd_region_notify(struct device *dev, enum nvdimm_event event)
 	device_for_each_child(dev, &event, child_notify);
 }
 
+void nvdimm_region_badblocks_clear(struct nd_region *nd_region,
+		struct resource *res)
+{
+	sector_t sector = (res->start - nd_region->ndr_start) >> 9;
+
+	badblocks_clear(&nd_region->bb, sector, resource_size(res) >> 9);
+}
+EXPORT_SYMBOL_GPL(nvdimm_region_badblocks_clear);
+
 static struct nd_device_driver nd_region_driver = {
 	.probe = nd_region_probe,
 	.remove = nd_region_remove,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 8458c53..97101ef 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -118,7 +118,9 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
 }
 
 int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length);
-void nvdimm_clear_from_poison_list(struct nvdimm_bus *nvdimm_bus,
+void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
+		phys_addr_t start, unsigned int len);
+void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
 		phys_addr_t start, unsigned int len);
 struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 		struct nvdimm_bus_descriptor *nfit_desc);
@@ -160,4 +162,6 @@ void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane);
 u64 nd_fletcher64(void *addr, size_t len, bool le);
 void nvdimm_flush(struct nd_region *nd_region);
 int nvdimm_has_flush(struct nd_region *nd_region);
+void nvdimm_region_badblocks_clear(struct nd_region *nd_region,
+		struct resource *res);
 #endif /* __LIBNVDIMM_H__ */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 45be8b5..096f6dc 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -319,7 +319,8 @@ static int nfit_test_cmd_ars_status(struct ars_state *ars_state,
 	return 0;
 }
 
-static int nfit_test_cmd_clear_error(struct nd_cmd_clear_error *clear_err,
+static int nfit_test_cmd_setup_clear_error(
+		struct nd_cmd_clear_error *clear_err,
 		unsigned int buf_len, int *cmd_rc)
 {
 	const u64 mask = NFIT_TEST_CLEAR_ERR_UNIT - 1;
@@ -330,14 +331,11 @@ static int nfit_test_cmd_clear_error(struct nd_cmd_clear_error *clear_err,
 		return -EINVAL;
 
 	/*
-	 * Report 'all clear' success for all commands even though a new
-	 * scrub will find errors again.  This is enough to have the
-	 * error removed from the 'badblocks' tracking in the pmem
-	 * driver.
+	 * since this is a test and there's no actual _DSM command being
+	 * issued, we need to fake the result.
 	 */
-	clear_err->status = 0;
-	clear_err->cleared = clear_err->length;
-	*cmd_rc = 0;
+	*cmd_rc = clear_err->cleared = clear_err->length;
+
 	return 0;
 }
 
@@ -465,7 +463,14 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 					cmd_rc);
 			break;
 		case ND_CMD_CLEAR_ERROR:
-			rc = nfit_test_cmd_clear_error(buf, buf_len, cmd_rc);
+			rc = nfit_test_cmd_setup_clear_error(buf,
+					buf_len, cmd_rc);
+			if (rc < 0)
+				return rc;
+
+			rc = acpi_nfit_forget_poison(nd_desc, cmd, buf);
+			if (rc < 0)
+				return rc;
 			break;
 		default:
 			return -ENOTTY;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 5/5] libnvdimm: providing dax support for nvdimm testing

[Dave Jiang]

Adding dax support to the nvdimm testing in tools/testing/nvdimm. The
memory allocated by the tool is via vmalloc and non-contiguous.
Overriding pgoff_to_phys() call to support the vmalloc memory.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dax/dax-private.h      |   40 ++++++++++++++++++++++++++++++++++++
 drivers/dax/dax.c              |   29 +++++---------------------
 tools/testing/nvdimm/Kbuild    |    3 ++-
 tools/testing/nvdimm/dax-dev.c |   44 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 25 deletions(-)
 create mode 100644 drivers/dax/dax-private.h
 create mode 100644 tools/testing/nvdimm/dax-dev.c

diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
new file mode 100644
index 0000000..1567ea4
--- /dev/null
+++ b/drivers/dax/dax-private.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#ifndef __DAX_PRIVATE_H__
+#define __DAX_PRIVATE_H__
+
+#include <linux/device.h>
+#include <linux/cdev.h>
+
+/**
+ * struct dax_dev - subdivision of a dax region
+ * @region - parent region
+ * @inode - inode
+ * @dev - device backing the character device
+ * @cdev - core chardev data
+ * @alive - !alive + rcu grace period == no new mappings can be established
+ * @id - child id in the region
+ * @num_resources - number of physical address extents in this device
+ * @res - array of physical address ranges
+ */
+struct dax_dev {
+	struct dax_region *region;
+	struct inode *inode;
+	struct device dev;
+	struct cdev cdev;
+	bool alive;
+	int id;
+	int num_resources;
+	struct resource res[0];
+};
+#endif
diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index ed758b7..2755ef6 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -21,6 +21,7 @@
 #include <linux/dax.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include "dax-private.h"
 #include "dax.h"
 
 static dev_t dax_devt;
@@ -54,27 +55,6 @@ struct dax_region {
 	unsigned long pfn_flags;
 };
 
-/**
- * struct dax_dev - subdivision of a dax region
- * @region - parent region
- * @dev - device backing the character device
- * @cdev - core chardev data
- * @alive - !alive + rcu grace period == no new mappings can be established
- * @id - child id in the region
- * @num_resources - number of physical address extents in this device
- * @res - array of physical address ranges
- */
-struct dax_dev {
-	struct dax_region *region;
-	struct inode *inode;
-	struct device dev;
-	struct cdev cdev;
-	bool alive;
-	int id;
-	int num_resources;
-	struct resource res[0];
-};
-
 static ssize_t id_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -395,7 +375,8 @@ static int check_vma(struct dax_dev *dax_dev, struct vm_area_struct *vma,
 	return 0;
 }
 
-static phys_addr_t pgoff_to_phys(struct dax_dev *dax_dev, pgoff_t pgoff,
+/* see "strong" declaration in tools/testing/nvdimm/dax-dev.c */
+__weak phys_addr_t dax_pgoff_to_phys(struct dax_dev *dax_dev, pgoff_t pgoff,
 		unsigned long size)
 {
 	struct resource *res;
@@ -437,7 +418,7 @@ static int __dax_dev_fault(struct dax_dev *dax_dev, struct vm_area_struct *vma,
 		return VM_FAULT_SIGBUS;
 	}
 
-	phys = pgoff_to_phys(dax_dev, vmf->pgoff, PAGE_SIZE);
+	phys = dax_pgoff_to_phys(dax_dev, vmf->pgoff, PAGE_SIZE);
 	if (phys == -1) {
 		dev_dbg(dev, "%s: phys_to_pgoff(%#lx) failed\n", __func__,
 				vmf->pgoff);
@@ -499,7 +480,7 @@ static int __dax_dev_pmd_fault(struct dax_dev *dax_dev,
 	}
 
 	pgoff = linear_page_index(vma, pmd_addr);
-	phys = pgoff_to_phys(dax_dev, pgoff, PMD_SIZE);
+	phys = dax_pgoff_to_phys(dax_dev, pgoff, PMD_SIZE);
 	if (phys == -1) {
 		dev_dbg(dev, "%s: phys_to_pgoff(%#lx) failed\n", __func__,
 				pgoff);
diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild
index 405212b..6dcb3c4 100644
--- a/tools/testing/nvdimm/Kbuild
+++ b/tools/testing/nvdimm/Kbuild
@@ -28,7 +28,7 @@ obj-$(CONFIG_ND_BTT) += nd_btt.o
 obj-$(CONFIG_ND_BLK) += nd_blk.o
 obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
 obj-$(CONFIG_ACPI_NFIT) += nfit.o
-obj-$(CONFIG_DEV_DAX) += dax.o
+obj-$(CONFIG_DEV_DAX) += dax.o dax-dev.o
 obj-$(CONFIG_DEV_DAX_PMEM) += dax_pmem.o
 
 nfit-y := $(ACPI_SRC)/core.o
@@ -49,6 +49,7 @@ nd_e820-y := $(NVDIMM_SRC)/e820.o
 nd_e820-y += config_check.o
 
 dax-y := $(DAX_SRC)/dax.o
+dax-y += dax-dev.o
 dax-y += config_check.o
 
 dax_pmem-y := $(DAX_SRC)/pmem.o
diff --git a/tools/testing/nvdimm/dax-dev.c b/tools/testing/nvdimm/dax-dev.c
new file mode 100644
index 0000000..1763688
--- /dev/null
+++ b/tools/testing/nvdimm/dax-dev.c
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include "test/nfit_test.h"
+#include <linux/mm.h>
+#include "../../../drivers/dax/dax-private.h"
+
+phys_addr_t dax_pgoff_to_phys(struct dax_dev *dax_dev, pgoff_t pgoff,
+		unsigned long size)
+{
+
+	struct resource *res;
+	phys_addr_t virt;
+	int i;
+
+	for (i = 0; i < dax_dev->num_resources; i++) {
+		res = &dax_dev->res[i];
+		virt = pgoff * PAGE_SIZE + res->start;
+		if (virt >= res->start && virt <= res->end)
+			break;
+		pgoff -= PHYS_PFN(resource_size(res));
+	}
+
+	if (i < dax_dev->num_resources) {
+		res = &dax_dev->res[i];
+		if (virt + size - 1 <= res->end) {
+			struct page *page;
+
+			page = vmalloc_to_page((void *)virt);
+			return PFN_PHYS(page_to_pfn(page));
+		}
+	}
+
+	return -1;
+}

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/5] libnvdimm: Add mechanism to publish badblocks at nd region level

[Johannes Thumshirn]

On 03/06/2017 09:32 PM, Dave Jiang wrote:
> badblocks sysfs file will be export at nd_region level. When nvdimm event
> notifier happens for NVDIMM_REVALIATE_POISON, the badblocks in the nd
                       ^~ NVDIMM_REVALIDATE_POISON

> region will be updated.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

Otherwise looks good to me,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/5] libnvdimm: Add resource sysfs attrib to nd region

[Johannes Thumshirn]

On 03/06/2017 09:32 PM, Dave Jiang wrote:
> Adding sysfs attribute in order to export the physical address of the
> ND region. This is for supporting of user app poison clear via
> device dax.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 4/5] acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks

[Johannes Thumshirn]

On 03/06/2017 09:32 PM, Dave Jiang wrote:
> Providing mechanism to clear poison list via the ndctl ND_CMD_CLEAR_ERROR
> call. We will update the poison list and also the badblocks at region level
> if the region is in dax mode or in pmem mode and not active.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

[...]

> +	if ((cmd != ND_CMD_CLEAR_ERROR) || !nvdimm_bus || !clear_err->cleared)
             ^~ Unnecessary parenthesis?

[...]

> +
> +		/* make sure clear_err range is within a SPA range */
> +		if (((clear_begin >= spa_begin) &&
> +					(clear_begin < (spa_end))) &&
> +				((clear_end > spa_begin) &&
> +				 (clear_end <= spa_end))) {

Indentation looks a bit odd here and the superfluous parenthesis aren't
improving the situation.

[...]


> +	if (nd_btt || nd_pfn || nd_dax) {
> +		if (nd_btt)
> +			ndns = nd_btt->ndns;
> +		else if (nd_pfn)
> +			ndns = nd_pfn->ndns;
> +		else if (nd_dax)
> +			ndns = nd_dax->nd_pfn.ndns;
> +
> +		if (!ndns)
> +			return 0;

How can this (the !ndns case) ever happen?

And anyways isn't this sufficient, or am I missing something:

	if (nd_btt)
		ndns = nd_btt->ndns;
	else if (nd_pfn)
		ndns = nd_pfn->ndns;
	else if (nd_dax)
		ndns = nd_dax->nd_pfn.ndns;
	else
		ndns = to_ndns(dev);

> +	} else
> +		ndns = to_ndns(dev);
> +

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 5/5] libnvdimm: providing dax support for nvdimm testing

[Johannes Thumshirn]

On 03/06/2017 09:32 PM, Dave Jiang wrote:
> Adding dax support to the nvdimm testing in tools/testing/nvdimm. The
> memory allocated by the tool is via vmalloc and non-contiguous.
> Overriding pgoff_to_phys() call to support the vmalloc memory.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

Thanks,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 4/5] acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks

[Dave Jiang]

On 03/07/2017 02:30 AM, Johannes Thumshirn wrote:
> On 03/06/2017 09:32 PM, Dave Jiang wrote:
>> Providing mechanism to clear poison list via the ndctl ND_CMD_CLEAR_ERROR
>> call. We will update the poison list and also the badblocks at region level
>> if the region is in dax mode or in pmem mode and not active.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
> 
> [...]
> 
>> +	if ((cmd != ND_CMD_CLEAR_ERROR) || !nvdimm_bus || !clear_err->cleared)
>              ^~ Unnecessary parenthesis?
> 
> [...]
> 
>> +
>> +		/* make sure clear_err range is within a SPA range */
>> +		if (((clear_begin >= spa_begin) &&
>> +					(clear_begin < (spa_end))) &&
>> +				((clear_end > spa_begin) &&
>> +				 (clear_end <= spa_end))) {
> 
> Indentation looks a bit odd here and the superfluous parenthesis aren't
> improving the situation.
> 
> [...]

I'll fix that.

> 
> 
>> +	if (nd_btt || nd_pfn || nd_dax) {
>> +		if (nd_btt)
>> +			ndns = nd_btt->ndns;
>> +		else if (nd_pfn)
>> +			ndns = nd_pfn->ndns;
>> +		else if (nd_dax)
>> +			ndns = nd_dax->nd_pfn.ndns;
>> +
>> +		if (!ndns)
>> +			return 0;
> 
> How can this (the !ndns case) ever happen?

So if nd_btt->ndns or nd_pfn->ndns or nd_dax->nd_pfn.ndns happens to be
NULL, then this case can happen. I don't know how likely that is to
happen however. The code was lifted from nvdimm_namespace_common_probe().

> 
> And anyways isn't this sufficient, or am I missing something:
> 
> 	if (nd_btt)
> 		ndns = nd_btt->ndns;
> 	else if (nd_pfn)
> 		ndns = nd_pfn->ndns;
> 	else if (nd_dax)
> 		ndns = nd_dax->nd_pfn.ndns;
> 	else
> 		ndns = to_ndns(dev);
> 
>> +	} else
>> +		ndns = to_ndns(dev);
>> +
> 
> Thanks,
> 	Johannes
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 4/5] acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks

[Johannes Thumshirn]

On 03/07/2017 05:00 PM, Dave Jiang wrote:
> So if nd_btt->ndns or nd_pfn->ndns or nd_dax->nd_pfn.ndns happens to be
> NULL, then this case can happen. I don't know how likely that is to
> happen however. The code was lifted from nvdimm_namespace_common_probe().

Ah right, I just looked at the 'if (nd_btt || nd_pfn || nd_dax)' and
then missed out the *->ndns assignments.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

[Linda Knippers]

On 03/06/2017 03:32 PM, Dave Jiang wrote:
> Make sure that xlat_status is unconditionally called.

Is this just code cleanup or is it fixing something?

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/nfit/core.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 7361d00..9d4f461 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  	acpi_handle handle;
>  	unsigned int func;
>  	const u8 *uuid;
> -	int rc, i;
> +	int rc = 0, xlat_rc, i;
>  
>  	func = cmd;
>  	if (cmd == ND_CMD_CALL) {
> @@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  			 * unfilled in the output buffer
>  			 */
>  			rc = buf_len - offset - in_buf.buffer.length;
> -			if (cmd_rc)
> -				*cmd_rc = xlat_status(nvdimm, buf, cmd,
> -						fw_status);
>  		} else {
>  			dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
>  					__func__, dimm_name, cmd_name, buf_len,
>  					offset);
>  			rc = -ENXIO;
> +			goto out;
>  		}
> -	} else {
> -		rc = 0;
> -		if (cmd_rc)
> -			*cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>  	}
>  
> +	xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
> +
> +	if (cmd_rc)
> +		*cmd_rc = xlat_rc;

Is there some benefit of calling xlat_status and then throwing away the result?

> +
>   out:
>  	ACPI_FREE(out_obj);
>  
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

[Dave Jiang]

On 03/07/2017 05:20 PM, Linda Knippers wrote:
> On 03/06/2017 03:32 PM, Dave Jiang wrote:
>> Make sure that xlat_status is unconditionally called.
> 
> Is this just code cleanup or is it fixing something?

This is code clean up requested by Dan.

> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/acpi/nfit/core.c |   15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 7361d00..9d4f461 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>  	acpi_handle handle;
>>  	unsigned int func;
>>  	const u8 *uuid;
>> -	int rc, i;
>> +	int rc = 0, xlat_rc, i;
>>  
>>  	func = cmd;
>>  	if (cmd == ND_CMD_CALL) {
>> @@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>  			 * unfilled in the output buffer
>>  			 */
>>  			rc = buf_len - offset - in_buf.buffer.length;
>> -			if (cmd_rc)
>> -				*cmd_rc = xlat_status(nvdimm, buf, cmd,
>> -						fw_status);
>>  		} else {
>>  			dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
>>  					__func__, dimm_name, cmd_name, buf_len,
>>  					offset);
>>  			rc = -ENXIO;
>> +			goto out;
>>  		}
>> -	} else {
>> -		rc = 0;
>> -		if (cmd_rc)
>> -			*cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>  	}
>>  
>> +	xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>> +
>> +	if (cmd_rc)
>> +		*cmd_rc = xlat_rc;
> 
> Is there some benefit of calling xlat_status and then throwing away the result?

It's not exactly thrown away. I tried to preserve the original code path
as much as possible. So in the current case, if xlat_status() fails,
then *cmd_rc gets it like in the original code. If it's succeeds, then
the forget_poison provides the status instead.

> 
>> +
>>   out:
>>  	ACPI_FREE(out_obj);
>>  
>>
>> _______________________________________________
>> Linux-nvdimm mailing list
>> Linux-nvdimm@lists.01.org
>> https://lists.01.org/mailman/listinfo/linux-nvdimm
>>
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

[Linda Knippers]

On 03/08/2017 11:29 AM, Dave Jiang wrote:
> On 03/07/2017 05:20 PM, Linda Knippers wrote:
>> On 03/06/2017 03:32 PM, Dave Jiang wrote:
>>> Make sure that xlat_status is unconditionally called.
>>
>> Is this just code cleanup or is it fixing something?
> 
> This is code clean up requested by Dan.

Ok, then it seems like the commit message above should match
the subject.  As it is, it makes it sound like it's important
to call xlat_status unconditionally, but I still have a question
about that below.

> 
>>
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>  drivers/acpi/nfit/core.c |   15 +++++++--------
>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>> index 7361d00..9d4f461 100644
>>> --- a/drivers/acpi/nfit/core.c
>>> +++ b/drivers/acpi/nfit/core.c
>>> @@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>>  	acpi_handle handle;
>>>  	unsigned int func;
>>>  	const u8 *uuid;
>>> -	int rc, i;
>>> +	int rc = 0, xlat_rc, i;
>>>  
>>>  	func = cmd;
>>>  	if (cmd == ND_CMD_CALL) {
>>> @@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>>  			 * unfilled in the output buffer
>>>  			 */
>>>  			rc = buf_len - offset - in_buf.buffer.length;
>>> -			if (cmd_rc)
>>> -				*cmd_rc = xlat_status(nvdimm, buf, cmd,
>>> -						fw_status);
>>>  		} else {
>>>  			dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
>>>  					__func__, dimm_name, cmd_name, buf_len,
>>>  					offset);
>>>  			rc = -ENXIO;
>>> +			goto out;
>>>  		}
>>> -	} else {
>>> -		rc = 0;
>>> -		if (cmd_rc)
>>> -			*cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>>  	}
>>>  
>>> +	xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>> +
>>> +	if (cmd_rc)
>>> +		*cmd_rc = xlat_rc;
>>
>> Is there some benefit of calling xlat_status and then throwing away the result?
> 
> It's not exactly thrown away. I tried to preserve the original code path
> as much as possible. 

The original code only calls xlat_status if cmd_rc is set, but you call it,
then don't use the result if cmd_rc isn't set.  Why call it if you know
ahead of time you're not going to use the result?  That's what I don't
understand.  Why isn't the call made only 'if (cmd_rc)', like the original
code?

> So in the current case, if xlat_status() fails,
> then *cmd_rc gets it like in the original code. If it's succeeds, then
> the forget_poison provides the status instead.
> 
>>
>>> +
>>>   out:
>>>  	ACPI_FREE(out_obj);
>>>  
>>>
>>> _______________________________________________
>>> Linux-nvdimm mailing list
>>> Linux-nvdimm@lists.01.org
>>> https://lists.01.org/mailman/listinfo/linux-nvdimm
>>>
>>

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

[Dan Williams]

On Wed, Mar 8, 2017 at 8:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 03/08/2017 11:29 AM, Dave Jiang wrote:
>> On 03/07/2017 05:20 PM, Linda Knippers wrote:
>>> On 03/06/2017 03:32 PM, Dave Jiang wrote:
>>>> Make sure that xlat_status is unconditionally called.
>>>
>>> Is this just code cleanup or is it fixing something?
>>
>> This is code clean up requested by Dan.
>
> Ok, then it seems like the commit message above should match
> the subject.  As it is, it makes it sound like it's important
> to call xlat_status unconditionally, but I still have a question
> about that below.
>
>>
>>>
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>  drivers/acpi/nfit/core.c |   15 +++++++--------
>>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>> index 7361d00..9d4f461 100644
>>>> --- a/drivers/acpi/nfit/core.c
>>>> +++ b/drivers/acpi/nfit/core.c
>>>> @@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>>>     acpi_handle handle;
>>>>     unsigned int func;
>>>>     const u8 *uuid;
>>>> -   int rc, i;
>>>> +   int rc = 0, xlat_rc, i;
>>>>
>>>>     func = cmd;
>>>>     if (cmd == ND_CMD_CALL) {
>>>> @@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>>>                      * unfilled in the output buffer
>>>>                      */
>>>>                     rc = buf_len - offset - in_buf.buffer.length;
>>>> -                   if (cmd_rc)
>>>> -                           *cmd_rc = xlat_status(nvdimm, buf, cmd,
>>>> -                                           fw_status);
>>>>             } else {
>>>>                     dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
>>>>                                     __func__, dimm_name, cmd_name, buf_len,
>>>>                                     offset);
>>>>                     rc = -ENXIO;
>>>> +                   goto out;
>>>>             }
>>>> -   } else {
>>>> -           rc = 0;
>>>> -           if (cmd_rc)
>>>> -                   *cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>>>     }
>>>>
>>>> +   xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>>> +
>>>> +   if (cmd_rc)
>>>> +           *cmd_rc = xlat_rc;
>>>
>>> Is there some benefit of calling xlat_status and then throwing away the result?
>>
>> It's not exactly thrown away. I tried to preserve the original code path
>> as much as possible.
>
> The original code only calls xlat_status if cmd_rc is set, but you call it,
> then don't use the result if cmd_rc isn't set.  Why call it if you know
> ahead of time you're not going to use the result?  That's what I don't
> understand.  Why isn't the call made only 'if (cmd_rc)', like the original
> code?
>

In a following patch we use the xlat_rc result even in cases when
cmd_rc is NULL.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm