[PATCH 0/3] Expose known poison in SPA ranges to the block layer

[PATCH 0/3] Expose known poison in SPA ranges to the block layer

[vishal]

From: Vishal Verma <vishal.l.verma@intel.com>

This series does a few things:
- Retrieve all known poison in the system physical address (SPA) space
  using ARS (Address Range Scrub) commands to firmware
- Store this poison in a new 'nd_poison' structure
- In pmem, consume the poison list and expose the ranges as bad sectors

This depends on the badblocks series sent out previously[1]

This was tested using nfit_test to add poison at specific address ranges.
More testing, specially on NVDIMM-N hardware is much appreciated!


[1]: https://lists.01.org/pipermail/linux-nvdimm/2015-December/003239.html

Vishal Verma (3):
  nfit_test: Enable DSMs for all test NFITs
  libnvdimm: Add a poison list
  pmem: Use the poison list to expose badblocks

 drivers/acpi/nfit.c              | 193 +++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/core.c            |  63 +++++++++++++
 drivers/nvdimm/nd-core.h         |   1 +
 drivers/nvdimm/nd.h              |   6 ++
 drivers/nvdimm/pmem.c            | 124 +++++++++++++++++++++++++
 include/linux/libnvdimm.h        |   2 +
 tools/testing/nvdimm/test/nfit.c |   9 ++
 7 files changed, 398 insertions(+)

-- 
2.5.0

[PATCH 1/3] nfit_test: Enable DSMs for all test NFITs

[vishal]

From: Vishal Verma <vishal.l.verma@intel.com>

In preparation for getting a poison list using ARS DSMs, enable DSMs for
all manufactured NFITs supplied by the test framework.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/nvdimm/test/nfit.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 51cf825..298805a 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -1088,6 +1088,8 @@ static void nfit_test1_setup(struct nfit_test *t)
 	struct acpi_nfit_memory_map *memdev;
 	struct acpi_nfit_control_region *dcr;
 	struct acpi_nfit_system_address *spa;
+	struct nvdimm_bus_descriptor *nd_desc;
+	struct acpi_nfit_desc *acpi_desc;
 
 	offset = 0;
 	/* spa0 (flat range with no bdw aliasing) */
@@ -1135,6 +1137,13 @@ static void nfit_test1_setup(struct nfit_test *t)
 	dcr->command_size = 0;
 	dcr->status_offset = 0;
 	dcr->status_size = 0;
+
+	acpi_desc = &t->acpi_desc;
+	set_bit(ND_CMD_ARS_CAP, &acpi_desc->bus_dsm_force_en);
+	set_bit(ND_CMD_ARS_START, &acpi_desc->bus_dsm_force_en);
+	set_bit(ND_CMD_ARS_STATUS, &acpi_desc->bus_dsm_force_en);
+	nd_desc = &acpi_desc->nd_desc;
+	nd_desc->ndctl = nfit_test_ctl;
 }
 
 static int nfit_test_blk_do_io(struct nd_blk_region *ndbr, resource_size_t dpa,
-- 
2.5.0

[PATCH 2/3] libnvdimm: Add a poison list

[vishal]

From: Vishal Verma <vishal.l.verma@intel.com>

During region creation, perform Address Range Scrubs (ARS) for the SPA
(System Physical Address) ranges to retrieve known poison locations from
firmware. Add a new data structure 'nd_poison' which is used as a list
in nvdimm_bus to store these poison locations for subsequent consumption
by the pmem driver.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c       | 193 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/core.c     |  63 +++++++++++++++
 drivers/nvdimm/nd-core.h  |   1 +
 drivers/nvdimm/nd.h       |   6 ++
 include/linux/libnvdimm.h |   2 +
 5 files changed, 265 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index e7ed39b..bd8726d 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/ndctl.h>
+#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/acpi.h>
 #include <linux/sort.h>
@@ -1473,6 +1474,195 @@ static void acpi_nfit_blk_region_disable(struct nvdimm_bus *nvdimm_bus,
 	/* devm will free nfit_blk */
 }
 
+static int ars_get_cap(struct nvdimm_bus_descriptor *nd_desc,
+		struct nd_cmd_ars_cap *cmd, u64 addr, u64 length)
+{
+	cmd->address = addr;
+	cmd->length = length;
+
+	return nd_desc->ndctl(nd_desc, NULL, ND_CMD_ARS_CAP, cmd,
+			sizeof(*cmd));
+}
+
+static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc,
+		struct nd_cmd_ars_start *cmd, u64 addr, u64 length)
+{
+	int rc;
+
+	cmd->address = addr;
+	cmd->length = length;
+	cmd->type = ND_ARS_PERSISTENT;
+
+	while (1) {
+		rc = nd_desc->ndctl(nd_desc, NULL, ND_CMD_ARS_START, cmd,
+				sizeof(*cmd));
+		if (rc)
+			return rc;
+		switch (cmd->status) {
+		case 0:
+			return 0;
+		case 1:
+			/* ARS unsupported, but we should never get here */
+			return 0;
+		case 2:
+			return -EINVAL;
+		case 3:
+			/* ARS is in progress */
+			msleep(1000);
+			break;
+		default:
+			return -ENXIO;
+		}
+	}
+}
+
+static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc,
+		struct nd_cmd_ars_status *cmd)
+{
+	int rc;
+
+	while (1) {
+		rc = nd_desc->ndctl(nd_desc, NULL, ND_CMD_ARS_STATUS, cmd,
+			sizeof(*cmd));
+		if (rc || cmd->status & 0xffff)
+			return -ENXIO;
+
+		/* Check extended status (Upper two bytes) */
+		switch (cmd->status & 0xffff0000) {
+		case 0:
+			return 0;
+		case 1:
+			/* ARS is in progress */
+			msleep(1000);
+			break;
+		case 2:
+			/* No ARS performed for the current boot */
+		default:
+			return -ENXIO;
+		}
+	}
+}
+
+static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus,
+		struct nd_cmd_ars_status *ars_status, u64 start)
+{
+	int rc;
+	u32 i;
+
+	/*
+	 * The address field returned by ars_status should be either
+	 * less than or equal to the address we last started ARS for.
+	 * The (start, length) returned by ars_status should also have
+	 * non-zero overlap with the range we started ARS for.
+	 * If this is not the case, bail.
+	 */
+	if (ars_status->address > start ||
+			(ars_status->address + ars_status->length < start))
+		return -ENXIO;
+
+	for (i = 0; i < ars_status->num_records; i++) {
+		rc = nvdimm_bus_add_poison(nvdimm_bus,
+				ars_status->records[i].err_address,
+				ars_status->records[i].length);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
+static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
+		struct nd_region_desc *ndr_desc)
+{
+	struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
+	struct nvdimm_bus *nvdimm_bus = acpi_desc->nvdimm_bus;
+	struct nd_cmd_ars_status *ars_status = NULL;
+	struct nd_cmd_ars_start *ars_start = NULL;
+	struct nd_cmd_ars_cap *ars_cap = NULL;
+	u64 start, len, cur, remaining;
+	int rc;
+
+	ars_cap = kzalloc(sizeof(*ars_cap), GFP_KERNEL);
+	if (!ars_cap)
+		return -ENOMEM;
+
+	start = ndr_desc->res->start;
+	len = ndr_desc->res->end - ndr_desc->res->start + 1;
+
+	rc = ars_get_cap(nd_desc, ars_cap, start, len);
+	if (rc)
+		goto out;
+
+	/* If ARS is unsupported, skip this but continue initialization */
+	if (ars_cap->status & 0xffff) {
+		dev_warn(acpi_desc->dev,
+			"ARS unsupported, won't construct an error list\n");
+		goto out;
+	}
+
+	/*
+	 * Check if a full-range ARS has been run. If so, use those results
+	 * without having to start a new ARS.
+	 */
+	ars_status = kzalloc(ars_cap->max_ars_out + sizeof(*ars_status),
+			GFP_KERNEL);
+	if (!ars_status) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = ars_get_status(nd_desc, ars_status);
+	if (rc)
+		goto out;
+
+	if (ars_status->address <= start &&
+		(ars_status->address + ars_status->length >= start + len)) {
+		rc = ars_status_process_records(nvdimm_bus, ars_status, start);
+		goto out;
+	}
+
+	/*
+	 * ARS_STATUS can overflow if the number of poison entries found is
+	 * greater than the maximum buffer size (ars_cap->max_ars_out)
+	 * To detect overflow, check if the length field of ars_status
+	 * is less than the length we supplied. If so, process the
+	 * error entries we got, adjust the start point, and start again
+	 */
+	ars_start = kzalloc(sizeof(*ars_start), GFP_KERNEL);
+	if (!ars_start)
+		return -ENOMEM;
+
+	cur = start;
+	remaining = len;
+	do {
+		u64 done, end;
+
+		rc = ars_do_start(nd_desc, ars_start, cur, remaining);
+		if (rc)
+			goto out;
+
+		rc = ars_get_status(nd_desc, ars_status);
+		if (rc)
+			goto out;
+
+		rc = ars_status_process_records(nvdimm_bus, ars_status, cur);
+		if (rc)
+			goto out;
+
+		end = min(cur + remaining,
+			ars_status->address + ars_status->length);
+		done = end - cur;
+		cur += done;
+		remaining -= done;
+	} while (remaining);
+
+ out:
+	kfree(ars_cap);
+	kfree(ars_start);
+	kfree(ars_status);
+	return rc;
+}
+
 static int acpi_nfit_init_mapping(struct acpi_nfit_desc *acpi_desc,
 		struct nd_mapping *nd_mapping, struct nd_region_desc *ndr_desc,
 		struct acpi_nfit_memory_map *memdev,
@@ -1585,6 +1775,9 @@ 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_find_poison(acpi_desc, ndr_desc);
+		if (rc)
+			return rc;
 		if (!nvdimm_pmem_region_create(nvdimm_bus, ndr_desc))
 			return -ENOMEM;
 	} else if (nfit_spa_type(spa) == NFIT_SPA_VOLATILE) {
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 82c49bb..8c882c8 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -325,6 +325,7 @@ struct nvdimm_bus *__nvdimm_bus_register(struct device *parent,
 	if (!nvdimm_bus)
 		return NULL;
 	INIT_LIST_HEAD(&nvdimm_bus->list);
+	INIT_LIST_HEAD(&nvdimm_bus->poison_list);
 	init_waitqueue_head(&nvdimm_bus->probe_wait);
 	nvdimm_bus->id = ida_simple_get(&nd_ida, 0, 0, GFP_KERNEL);
 	mutex_init(&nvdimm_bus->reconfig_mutex);
@@ -359,6 +360,67 @@ struct nvdimm_bus *__nvdimm_bus_register(struct device *parent,
 }
 EXPORT_SYMBOL_GPL(__nvdimm_bus_register);
 
+struct list_head *nvdimm_bus_get_poison_list(struct nvdimm_bus *nvdimm_bus)
+{
+	return &nvdimm_bus->poison_list;
+}
+EXPORT_SYMBOL_GPL(nvdimm_bus_get_poison_list);
+
+static int __add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
+{
+	struct nd_poison *pl;
+
+	pl = kzalloc(sizeof(*pl), GFP_KERNEL);
+	if (!pl)
+		return -ENOMEM;
+
+	pl->start = addr;
+	pl->length = length;
+	list_add_tail(&pl->list, &nvdimm_bus->poison_list);
+
+	return 0;
+}
+
+int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
+{
+	struct nd_poison *pl;
+
+	if (list_empty(&nvdimm_bus->poison_list))
+		return __add_poison(nvdimm_bus, addr, length);
+
+	/*
+	 * There is a chance this is a duplicate, check for those first.
+	 * This will be the common case as ARS_STATUS returns all known
+	 * errors in the SPA space, and we can't query it per region
+	 */
+	list_for_each_entry(pl, &nvdimm_bus->poison_list, list)
+		if (pl->start == addr) {
+			/* If length has changed, update this list entry */
+			if (pl->length != length)
+				pl->length = length;
+			return 0;
+		}
+
+	/*
+	 * If not a duplicate or a simple length update, add the entry as is,
+	 * as any overlapping ranges will get resolved when the list is consumed
+	 * and converted to badblocks
+	 */
+	return __add_poison(nvdimm_bus, addr, length);
+}
+EXPORT_SYMBOL_GPL(nvdimm_bus_add_poison);
+
+static void free_poison_list(struct list_head *poison_list)
+{
+	struct nd_poison *pl, *next;
+
+	list_for_each_entry_safe(pl, next, poison_list, list) {
+		list_del(&pl->list);
+		kfree(pl);
+	}
+	list_del_init(poison_list);
+}
+
 static int child_unregister(struct device *dev, void *data)
 {
 	/*
@@ -385,6 +447,7 @@ void nvdimm_bus_unregister(struct nvdimm_bus *nvdimm_bus)
 
 	nd_synchronize();
 	device_for_each_child(&nvdimm_bus->dev, NULL, child_unregister);
+	free_poison_list(&nvdimm_bus->poison_list);
 	nvdimm_bus_destroy_ndctl(nvdimm_bus);
 
 	device_unregister(&nvdimm_bus->dev);
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 159aed5..29acdaa 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -30,6 +30,7 @@ struct nvdimm_bus {
 	struct list_head list;
 	struct device dev;
 	int id, probe_active;
+	struct list_head poison_list;
 	struct mutex reconfig_mutex;
 };
 
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 417e521..ba91fcd 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -38,6 +38,12 @@ enum {
 #endif
 };
 
+struct nd_poison {
+	u64 start;
+	u64 length;
+	struct list_head list;
+};
+
 struct nvdimm_drvdata {
 	struct device *dev;
 	int nsindex_size;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 3f021dc..43b82a0 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -116,6 +116,8 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
 
 }
 
+struct list_head *nvdimm_bus_get_poison_list(struct nvdimm_bus *nvdimm_bus);
+int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length);
 struct nvdimm_bus *__nvdimm_bus_register(struct device *parent,
 		struct nvdimm_bus_descriptor *nfit_desc, struct module *module);
 #define nvdimm_bus_register(parent, desc) \
-- 
2.5.0

[PATCH 3/3] pmem: Use the poison list to expose badblocks

[vishal]

From: Vishal Verma <vishal.l.verma@intel.com>

Enable the gendisk badblocks feature for pmem namespaces.
If the pmem namespace being created has any known poison associated with
its physical address space, convert the poison ranges to bad sectors
exposed using the badblocks interface.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/nvdimm/pmem.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 8ee7989..462570f 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -19,14 +19,18 @@
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/memory_hotplug.h>
 #include <linux/moduleparam.h>
+#include <linux/libnvdimm.h>
 #include <linux/vmalloc.h>
+#include <linux/blkdev.h>
 #include <linux/slab.h>
 #include <linux/pmem.h>
 #include <linux/nd.h>
+#include "nd-core.h"
 #include "pfn.h"
 #include "nd.h"
 
@@ -163,11 +167,122 @@ static void pmem_detach_disk(struct pmem_device *pmem)
 	blk_cleanup_queue(pmem->pmem_queue);
 }
 
+/**
+ * pmem_add_badblock_range() - Convert a physical address range to bad sectors
+ * @disk:	the disk associated with the pmem namespace
+ * @start:	start of the physical address range
+ * @length:	number of bytes of poison to be added
+ *
+ * This assumes that the range provided with (start, length) is already within
+ * the bounds of physical addresses for this namespace, i.e. lies in the
+ * interval [pmem->phys_addr, pmem->phys_addr + pmem->size)
+ */
+static int pmem_add_badblock_range(struct gendisk *disk, u64 start, u64 length)
+{
+	unsigned int sector_size = queue_logical_block_size(disk->queue);
+	struct pmem_device *pmem = disk->private_data;
+	sector_t start_sector;
+	u64 num_sectors;
+	u32 rem;
+
+	start_sector = div_u64(start - pmem->phys_addr, sector_size);
+	num_sectors = div_u64_rem(length, sector_size, &rem);
+	if (rem)
+		num_sectors++;
+
+	if (unlikely(num_sectors > (u64)INT_MAX)) {
+		u64 remaining = num_sectors;
+		sector_t s = start_sector;
+		int rc;
+
+		while (remaining) {
+			int done = min_t(u64, remaining, INT_MAX);
+
+			rc = disk_set_badblocks(disk, s, done);
+			if (rc)
+				return rc;
+			remaining -= done;
+			s += done;
+		}
+		return 0;
+	} else
+		return disk_set_badblocks(disk, start_sector, num_sectors);
+}
+
+/*
+ * The poison list generated during NFIT initialization may contain multiple,
+ * possibly overlapping ranges in the SPA (System Physical Address) space.
+ * Compare each of these ranges to the pmem namespace currently being
+ * initialized, and add badblocks for the sub-ranges that match
+ */
+static int pmem_add_poison(struct gendisk *disk, struct nvdimm_bus *nvdimm_bus)
+{
+	struct pmem_device *pmem = disk->private_data;
+	struct list_head *poison_list;
+	struct nd_poison *pl;
+	int rc;
+
+	poison_list = nvdimm_bus_get_poison_list(nvdimm_bus);
+	if (list_empty(poison_list))
+		return 0;
+
+	list_for_each_entry(pl, poison_list, list) {
+		u64 pl_end = pl->start + pl->length - 1;
+		u64 pmem_end = pmem->phys_addr + pmem->size - 1;
+
+		/* Discard intervals with no intersection */
+		if (pl_end < pmem->phys_addr)
+			continue;
+		if (pl->start > pmem_end)
+			continue;
+		/* Deal with any overlap after start of the pmem range */
+		if (pl->start >= pmem->phys_addr) {
+			u64 start = pl->start;
+			u64 len;
+
+			if (pl_end <= pmem_end)
+				len = pl->length;
+			else
+				len = pmem->phys_addr + pmem->size - pl->start;
+
+			rc = pmem_add_badblock_range(disk, start, len);
+			if (rc)
+				return rc;
+			dev_info(&nvdimm_bus->dev,
+				"Found a poison range (0x%llx, 0x%llx)\n",
+				start, len);
+			continue;
+		}
+		/* Deal with overlap for poison starting before pmem range */
+		if (pl->start < pmem->phys_addr) {
+			u64 start = pmem->phys_addr;
+			u64 len;
+
+			if (pl_end < pmem_end)
+				len = pl->start + pl->length - pmem->phys_addr;
+			else
+				len = pmem->size;
+
+			rc = pmem_add_badblock_range(disk, start, len);
+			if (rc)
+				return rc;
+			dev_info(&nvdimm_bus->dev,
+				"Found a poison range (0x%llx, 0x%llx)\n",
+				start, len);
+		}
+	}
+
+	return 0;
+}
+
 static int pmem_attach_disk(struct device *dev,
 		struct nd_namespace_common *ndns, struct pmem_device *pmem)
 {
+	struct nd_region *nd_region = to_nd_region(dev->parent);
+	struct nvdimm_bus *nvdimm_bus;
 	int nid = dev_to_node(dev);
 	struct gendisk *disk;
+	int ret;
 
 	pmem->pmem_queue = blk_alloc_queue_node(GFP_KERNEL, nid);
 	if (!pmem->pmem_queue)
@@ -196,6 +311,15 @@ static int pmem_attach_disk(struct device *dev,
 	set_capacity(disk, (pmem->size - pmem->data_offset) / 512);
 	pmem->pmem_disk = disk;
 
+	ret = disk_alloc_badblocks(disk);
+	if (ret)
+		return ret;
+
+	nvdimm_bus = to_nvdimm_bus(nd_region->dev.parent);
+	ret = pmem_add_poison(disk, nvdimm_bus);
+	if (ret)
+		return ret;
+
 	add_disk(disk);
 	revalidate_disk(disk);
 
-- 
2.5.0

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5391 bytes --]

Hi Vishal,

[auto build test ERROR on linux-nvdimm/libnvdimm-for-next]
[also build test ERROR on v4.4-rc5 next-20151218]

url:    https://github.com/0day-ci/linux/commits/vishal-kernel-org/Expose-known-poison-in-SPA-ranges-to-the-block-layer/20151220-172142
base:   https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm libnvdimm-for-next
config: x86_64-randconfig-x017-201551 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/nvdimm/pmem.c: In function 'pmem_add_badblock_range':
>> drivers/nvdimm/pmem.c:201:9: error: implicit declaration of function 'disk_set_badblocks' [-Werror=implicit-function-declaration]
       rc = disk_set_badblocks(disk, s, done);
            ^
   drivers/nvdimm/pmem.c: In function 'pmem_attach_disk':
>> drivers/nvdimm/pmem.c:314:8: error: implicit declaration of function 'disk_alloc_badblocks' [-Werror=implicit-function-declaration]
     ret = disk_alloc_badblocks(disk);
           ^
   cc1: some warnings being treated as errors

vim +/disk_set_badblocks +201 drivers/nvdimm/pmem.c

   195			sector_t s = start_sector;
   196			int rc;
   197	
   198			while (remaining) {
   199				int done = min_t(u64, remaining, INT_MAX);
   200	
 > 201				rc = disk_set_badblocks(disk, s, done);
   202				if (rc)
   203					return rc;
   204				remaining -= done;
   205				s += done;
   206			}
   207			return 0;
   208		} else
   209			return disk_set_badblocks(disk, start_sector, num_sectors);
   210	}
   211	
   212	/*
   213	 * The poison list generated during NFIT initialization may contain multiple,
   214	 * possibly overlapping ranges in the SPA (System Physical Address) space.
   215	 * Compare each of these ranges to the pmem namespace currently being
   216	 * initialized, and add badblocks for the sub-ranges that match
   217	 */
   218	static int pmem_add_poison(struct gendisk *disk, struct nvdimm_bus *nvdimm_bus)
   219	{
   220		struct pmem_device *pmem = disk->private_data;
   221		struct list_head *poison_list;
   222		struct nd_poison *pl;
   223		int rc;
   224	
   225		poison_list = nvdimm_bus_get_poison_list(nvdimm_bus);
   226		if (list_empty(poison_list))
   227			return 0;
   228	
   229		list_for_each_entry(pl, poison_list, list) {
   230			u64 pl_end = pl->start + pl->length - 1;
   231			u64 pmem_end = pmem->phys_addr + pmem->size - 1;
   232	
   233			/* Discard intervals with no intersection */
   234			if (pl_end < pmem->phys_addr)
   235				continue;
   236			if (pl->start > pmem_end)
   237				continue;
   238			/* Deal with any overlap after start of the pmem range */
   239			if (pl->start >= pmem->phys_addr) {
   240				u64 start = pl->start;
   241				u64 len;
   242	
   243				if (pl_end <= pmem_end)
   244					len = pl->length;
   245				else
   246					len = pmem->phys_addr + pmem->size - pl->start;
   247	
   248				rc = pmem_add_badblock_range(disk, start, len);
   249				if (rc)
   250					return rc;
   251				dev_info(&nvdimm_bus->dev,
   252					"Found a poison range (0x%llx, 0x%llx)\n",
   253					start, len);
   254				continue;
   255			}
   256			/* Deal with overlap for poison starting before pmem range */
   257			if (pl->start < pmem->phys_addr) {
   258				u64 start = pmem->phys_addr;
   259				u64 len;
   260	
   261				if (pl_end < pmem_end)
   262					len = pl->start + pl->length - pmem->phys_addr;
   263				else
   264					len = pmem->size;
   265	
   266				rc = pmem_add_badblock_range(disk, start, len);
   267				if (rc)
   268					return rc;
   269				dev_info(&nvdimm_bus->dev,
   270					"Found a poison range (0x%llx, 0x%llx)\n",
   271					start, len);
   272			}
   273		}
   274	
   275		return 0;
   276	}
   277	
   278	static int pmem_attach_disk(struct device *dev,
   279			struct nd_namespace_common *ndns, struct pmem_device *pmem)
   280	{
   281		struct nd_region *nd_region = to_nd_region(dev->parent);
   282		struct nvdimm_bus *nvdimm_bus;
   283		int nid = dev_to_node(dev);
   284		struct gendisk *disk;
   285		int ret;
   286	
   287		pmem->pmem_queue = blk_alloc_queue_node(GFP_KERNEL, nid);
   288		if (!pmem->pmem_queue)
   289			return -ENOMEM;
   290	
   291		blk_queue_make_request(pmem->pmem_queue, pmem_make_request);
   292		blk_queue_physical_block_size(pmem->pmem_queue, PAGE_SIZE);
   293		blk_queue_max_hw_sectors(pmem->pmem_queue, UINT_MAX);
   294		blk_queue_bounce_limit(pmem->pmem_queue, BLK_BOUNCE_ANY);
   295		queue_flag_set_unlocked(QUEUE_FLAG_NONROT, pmem->pmem_queue);
   296	
   297		disk = alloc_disk_node(0, nid);
   298		if (!disk) {
   299			blk_cleanup_queue(pmem->pmem_queue);
   300			return -ENOMEM;
   301		}
   302	
   303		disk->major		= pmem_major;
   304		disk->first_minor	= 0;
   305		disk->fops		= &pmem_fops;
   306		disk->private_data	= pmem;
   307		disk->queue		= pmem->pmem_queue;
   308		disk->flags		= GENHD_FL_EXT_DEVT;
   309		nvdimm_namespace_disk_name(ndns, disk->disk_name);
   310		disk->driverfs_dev = dev;
   311		set_capacity(disk, (pmem->size - pmem->data_offset) / 512);
   312		pmem->pmem_disk = disk;
   313	
 > 314		ret = disk_alloc_badblocks(disk);
   315		if (ret)
   316			return ret;
   317	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24096 bytes --]

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Dan Williams]

On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
> From: Vishal Verma <vishal.l.verma@intel.com>
>
> Enable the gendisk badblocks feature for pmem namespaces.
> If the pmem namespace being created has any known poison associated with
> its physical address space, convert the poison ranges to bad sectors
> exposed using the badblocks interface.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/nvdimm/pmem.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)

I think we should move this new functionality to the core because
there is not much pmem driver specific.  It's all generic nvdimm-core
and block-core functionality.  The only missing information the core
routine needs is the gendisk and a data offset (if sector-zero is at
an offset from the base address range of the namespace).  Something
like:

nvdimm_namespace_disk_poison(struct nd_namespace_common *ndns,
    resource_size_t offset, struct gendisk *disk)

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Verma, Vishal L]

On Sun, 2015-12-20 at 17:20 -0800, Dan Williams wrote:
> On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
> > From: Vishal Verma <vishal.l.verma@intel.com>
> > 
> > Enable the gendisk badblocks feature for pmem namespaces.
> > If the pmem namespace being created has any known poison associated
> > with
> > its physical address space, convert the poison ranges to bad sectors
> > exposed using the badblocks interface.
> > 
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/nvdimm/pmem.c | 124
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> 
> I think we should move this new functionality to the core because
> there is not much pmem driver specific.  It's all generic nvdimm-core
> and block-core functionality.  The only missing information the core
> routine needs is the gendisk and a data offset (if sector-zero is at
> an offset from the base address range of the namespace).  Something
> like:
> 
> nvdimm_namespace_disk_poison(struct nd_namespace_common *ndns,
>     resource_size_t offset, struct gendisk *disk)

This should be easy to do, however isn't it a bit counter-intuitive to
move this into core? The lower level drivers pmem/blk/btt are all owners
of their respective gendisks, and so doesn't it make more sense for them
to be in control of manipulating their gendisk data. Also, I can see
moving them if this was a common operation, but only pmem will ever need
to do this..

I'm not too strongly opposed to this however - the one thing that did
feel a bit awkward being in pmem was that we ask core for a struct
list_head and then walk it ourselves - pmem doesn't normally know about
the internals of nvdimm_bus, but with this we implicitly make it aware.

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Dan Williams]

On Mon, Dec 21, 2015 at 10:50 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Sun, 2015-12-20 at 17:20 -0800, Dan Williams wrote:
>> On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
>> > From: Vishal Verma <vishal.l.verma@intel.com>
>> >
>> > Enable the gendisk badblocks feature for pmem namespaces.
>> > If the pmem namespace being created has any known poison associated
>> > with
>> > its physical address space, convert the poison ranges to bad sectors
>> > exposed using the badblocks interface.
>> >
>> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
>> > ---
>> >  drivers/nvdimm/pmem.c | 124
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 124 insertions(+)
>>
>> I think we should move this new functionality to the core because
>> there is not much pmem driver specific.  It's all generic nvdimm-core
>> and block-core functionality.  The only missing information the core
>> routine needs is the gendisk and a data offset (if sector-zero is at
>> an offset from the base address range of the namespace).  Something
>> like:
>>
>> nvdimm_namespace_disk_poison(struct nd_namespace_common *ndns,
>>     resource_size_t offset, struct gendisk *disk)
>
> This should be easy to do, however isn't it a bit counter-intuitive to
> move this into core? The lower level drivers pmem/blk/btt are all owners
> of their respective gendisks, and so doesn't it make more sense for them
> to be in control of manipulating their gendisk data. Also, I can see
> moving them if this was a common operation, but only pmem will ever need
> to do this..
>
> I'm not too strongly opposed to this however - the one thing that did
> feel a bit awkward being in pmem was that we ask core for a struct
> list_head and then walk it ourselves - pmem doesn't normally know about
> the internals of nvdimm_bus, but with this we implicitly make it aware.

Yeah, it's borderline, but teaching pmem how to walk a list that the
core built up is what triggered the suggestion.  It's true that the
pmem driver owns its gendisk, but that's why it gets to make the
decision to call the library helper or not.

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Dan Williams]

On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
> From: Vishal Verma <vishal.l.verma@intel.com>
>
> Enable the gendisk badblocks feature for pmem namespaces.
> If the pmem namespace being created has any known poison associated with
> its physical address space, convert the poison ranges to bad sectors
> exposed using the badblocks interface.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/nvdimm/pmem.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 8ee7989..462570f 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
[..]
> @@ -196,6 +311,15 @@ static int pmem_attach_disk(struct device *dev,
>         set_capacity(disk, (pmem->size - pmem->data_offset) / 512);
>         pmem->pmem_disk = disk;
>
> +       ret = disk_alloc_badblocks(disk);
> +       if (ret)
> +               return ret;

I think we should skip allocating a bad block list in the case we find
no poison.  Then we can do a simple "if (disk->bb)" to check if there
are any bad blocks.

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Verma, Vishal L]

On Wed, 2015-12-23 at 12:28 -0800, Dan Williams wrote:
> On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
> > From: Vishal Verma <vishal.l.verma@intel.com>
> > 
> > Enable the gendisk badblocks feature for pmem namespaces.
> > If the pmem namespace being created has any known poison associated
> > with
> > its physical address space, convert the poison ranges to bad sectors
> > exposed using the badblocks interface.
> > 
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/nvdimm/pmem.c | 124
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> > 
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index 8ee7989..462570f 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> [..]
> > @@ -196,6 +311,15 @@ static int pmem_attach_disk(struct device *dev,
> >         set_capacity(disk, (pmem->size - pmem->data_offset) / 512);
> >         pmem->pmem_disk = disk;
> > 
> > +       ret = disk_alloc_badblocks(disk);
> > +       if (ret)
> > +               return ret;
> 
> I think we should skip allocating a bad block list in the case we find
> no poison.  Then we can do a simple "if (disk->bb)" to check if there
> are any bad blocks.

Sounds good - I'll move this allocation into the (what will be a core)
routine that traverses the list, and make it such that we allocate
disk->bb iff a poison entry is found for _this_ pmem range.

It will mean a check at every list node, but that shouldn't matter as
this is just init-time overhead.

	-Vishal

Re: [PATCH 3/3] pmem: Use the poison list to expose badblocks

[Dan Williams]

On Wed, Dec 23, 2015 at 12:32 PM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Wed, 2015-12-23 at 12:28 -0800, Dan Williams wrote:
>> On Sun, Dec 20, 2015 at 1:18 AM,  <vishal@kernel.org> wrote:
>> > From: Vishal Verma <vishal.l.verma@intel.com>
>> >
>> > Enable the gendisk badblocks feature for pmem namespaces.
>> > If the pmem namespace being created has any known poison associated
>> > with
>> > its physical address space, convert the poison ranges to bad sectors
>> > exposed using the badblocks interface.
>> >
>> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
>> > ---
>> >  drivers/nvdimm/pmem.c | 124
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 124 insertions(+)
>> >
>> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
>> > index 8ee7989..462570f 100644
>> > --- a/drivers/nvdimm/pmem.c
>> > +++ b/drivers/nvdimm/pmem.c
>> [..]
>> > @@ -196,6 +311,15 @@ static int pmem_attach_disk(struct device *dev,
>> >         set_capacity(disk, (pmem->size - pmem->data_offset) / 512);
>> >         pmem->pmem_disk = disk;
>> >
>> > +       ret = disk_alloc_badblocks(disk);
>> > +       if (ret)
>> > +               return ret;
>>
>> I think we should skip allocating a bad block list in the case we find
>> no poison.  Then we can do a simple "if (disk->bb)" to check if there
>> are any bad blocks.
>
> Sounds good - I'll move this allocation into the (what will be a core)
> routine that traverses the list, and make it such that we allocate
> disk->bb iff a poison entry is found for _this_ pmem range.
>
> It will mean a check at every list node, but that shouldn't matter as
> this is just init-time overhead.
>

...or free it on "no poison found".

Re: [PATCH 0/3] Expose known poison in SPA ranges to the block layer

[Verma, Vishal L]

On Sun, 2015-12-20 at 02:18 -0700, vishal@kernel.org wrote:
> From: Vishal Verma <vishal.l.verma@intel.com>
> 
> This series does a few things:
> - Retrieve all known poison in the system physical address (SPA) space
>   using ARS (Address Range Scrub) commands to firmware
> - Store this poison in a new 'nd_poison' structure
> - In pmem, consume the poison list and expose the ranges as bad
> sectors
> 
> This depends on the badblocks series sent out previously[1]
> 
> This was tested using nfit_test to add poison at specific address
> ranges.
> More testing, specially on NVDIMM-N hardware is much appreciated!
> 
> 
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2015-December/003239.
> html
> 
Thanks to Linda for pointing this out -- the above link is to an older
version of the badblocks patchset.
The latest version (v4) is at:
https://lists.01.org/pipermail/linux-nvdimm/2015-December/003255.html

The 0-day failures are, of course, due to this missing dependency.

Thanks,
	-Vishal

Re: [PATCH 0/3] Expose known poison in SPA ranges to the block layer

[Linda Knippers]

On 12/20/2015 04:18 AM, vishal@kernel.org wrote:
> From: Vishal Verma <vishal.l.verma@intel.com>
> 
> This series does a few things:
> - Retrieve all known poison in the system physical address (SPA) space
>   using ARS (Address Range Scrub) commands to firmware
> - Store this poison in a new 'nd_poison' structure
> - In pmem, consume the poison list and expose the ranges as bad sectors
> 
> This depends on the badblocks series sent out previously[1]
> 
> This was tested using nfit_test to add poison at specific address ranges.
> More testing, specially on NVDIMM-N hardware is much appreciated!

All I can report on my NVDIMM-N hardware is that these patches work
properly when the firmware reports that ARS is not supported.  I
can see the capability query and status for each device. Since
there's no ARS support, the rest of the code gets skipped, at it should.

I'll take a look at the test code too but as for testing on my real
NVDIMM-N hardware, this is probably as far as I can go for a while.

-- ljk
> 
> 
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2015-December/003239.html
> 
> Vishal Verma (3):
>   nfit_test: Enable DSMs for all test NFITs
>   libnvdimm: Add a poison list
>   pmem: Use the poison list to expose badblocks
> 
>  drivers/acpi/nfit.c              | 193 +++++++++++++++++++++++++++++++++++++++
>  drivers/nvdimm/core.c            |  63 +++++++++++++
>  drivers/nvdimm/nd-core.h         |   1 +
>  drivers/nvdimm/nd.h              |   6 ++
>  drivers/nvdimm/pmem.c            | 124 +++++++++++++++++++++++++
>  include/linux/libnvdimm.h        |   2 +
>  tools/testing/nvdimm/test/nfit.c |   9 ++
>  7 files changed, 398 insertions(+)
>