[PATCH v3 0/2] Hotplug support for libnvdimm

[PATCH v3 0/2] Hotplug support for libnvdimm

[Vishal Verma]

This series adds support for hotplug of NVDIMMs. Upon hotplug, the ACPI
core calls the .notify callback we register. From this, we evaluate the
_FIT method which returns an updated NFIT. This is scanned for any new
tables, and any new regions found from it are registered and made
available for use.

The series is tested with nfit_test (tools/testing/nvdimm) only, which
means the parts of getting a notification from the acpi core, and calling
_FIT are untested.

Changes from v2->v3:
 - in acpi_nfit_init, splice off the old contents if to a "prev" list and
   only check for duplicates when "prev" is not empty (Dan)
 - in acpi_nfit_init, error out if tables are found to be deleted
 - locking changes: Use device_lock for .add and .notify. Check if
   dev->driver is valid during notify to protect against a prior
   removal (Dan)
 - Change IS_ERR_OR_NULL to IS_ERR for acpi_nfit_desc_init (Dan)
 - nfit_test: for the hot-plug DIMM, add a flush hint table too for
   completeness

Changes from v1->v2:
 - If a 0-length header is found in the nfit (patch 1), also spew a
   warning (Jeff)
 - Don't make a new acpi_evaluate_fit helper - open code a call to
   acpi_evaluate_object in nfit.c (Dan/Rafael)
 - Remove a warning for duplicate DCRs (Toshi)
 - Add an init_lock to protect the notify handler from racing with an
   'add' or 'remove' (Dan)
 - The only NVDIMM in a system *could* potentially come from a hotplug,
   esp in the virtualization case. Refactor how acpi_nfit_desc is
   initialized to account for this. For the same reason, don't fail when
   a valid NFIT is not found at driver load time. A by-product of this
   change is that we need to initialize lists and mutexes manually in
   nfit test. (Dan)
 - Remove acpi_nfit_merge (added in v1) as it is now essentially
   the same as acpi_nfit_init
 - Reword the commit message for patch 2/2 to say 'hot add' instead of
   hotplug, making it clearer that hot removal support is not being added

Vishal Verma (2):
  nfit: in acpi_nfit_init, break on a 0-length table
  acpi: nfit: Add support for hot-add

 drivers/acpi/nfit.c              | 307 +++++++++++++++++++++++++++++++--------
 drivers/acpi/nfit.h              |   2 +
 tools/testing/nvdimm/test/nfit.c | 164 ++++++++++++++++++++-
 3 files changed, 413 insertions(+), 60 deletions(-)

-- 
2.4.3

[PATCH v3 1/2] nfit: in acpi_nfit_init, break on a 0-length table

[Vishal Verma]

If acpi_nfit_init is called (such as from nfit_test), with an nfit table
that has more memory allocated than it needs (and a similarly large
'size' field, add_tables would happily keep adding null SPA Range tables
filling up all available memory.

Make it friendlier by breaking out if a 0-length header is found in any
of the tables.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..35b4b56 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -335,6 +335,12 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
 		return NULL;
 
 	hdr = table;
+	if (!hdr->length) {
+		dev_warn(dev, "found a zero length table '%d' parsing nfit\n",
+			hdr->type);
+		return NULL;
+	}
+
 	switch (hdr->type) {
 	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
 		if (!add_spa(acpi_desc, table))
-- 
2.4.3

[PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Vishal Verma]

Add a .notify callback to the acpi_nfit_driver that gets called on a
hotplug event. From this, evaluate the _FIT ACPI method which returns
the updated NFIT with handles for the hot-plugged NVDIMM.

Iterate over the new NFIT, and add any new tables found, and
register/enable the corresponding regions.

In the nfit test framework, after normal initialization, update the NFIT
with a new hot-plugged NVDIMM, and directly call into the driver to
update its view of the available regions.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Cc: Elliott, Robert <elliott@hpe.com>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: <linux-acpi@vger.kernel.org>
Cc: <linux-nvdimm@lists.01.org>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit.c              | 301 +++++++++++++++++++++++++++++++--------
 drivers/acpi/nfit.h              |   2 +
 tools/testing/nvdimm/test/nfit.c | 164 ++++++++++++++++++++-
 3 files changed, 407 insertions(+), 60 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 35b4b56..869f279 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -33,6 +33,15 @@ static bool force_enable_dimms;
 module_param(force_enable_dimms, bool, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(force_enable_dimms, "Ignore _STA (ACPI DIMM device) status");
 
+struct nfit_table_prev {
+	struct list_head spas;
+	struct list_head memdevs;
+	struct list_head dcrs;
+	struct list_head bdws;
+	struct list_head idts;
+	struct list_head flushes;
+};
+
 static u8 nfit_uuid[NFIT_UUID_MAX][16];
 
 const u8 *to_nfit_uuid(enum nfit_uuids id)
@@ -221,12 +230,20 @@ static int nfit_spa_type(struct acpi_nfit_system_address *spa)
 }
 
 static bool add_spa(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_system_address *spa)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_spa *nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa),
-			GFP_KERNEL);
+	struct nfit_spa *nfit_spa;
+
+	list_for_each_entry(nfit_spa, &prev->spas, list) {
+		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0) {
+			list_move_tail(&nfit_spa->list, &acpi_desc->spas);
+			return true;
+		}
+	}
 
+	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa), GFP_KERNEL);
 	if (!nfit_spa)
 		return false;
 	INIT_LIST_HEAD(&nfit_spa->list);
@@ -239,12 +256,19 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
 }
 
 static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_memory_map *memdev)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_memdev *nfit_memdev = devm_kzalloc(dev,
-			sizeof(*nfit_memdev), GFP_KERNEL);
+	struct nfit_memdev *nfit_memdev;
 
+	list_for_each_entry(nfit_memdev, &prev->memdevs, list)
+		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0) {
+			list_move_tail(&nfit_memdev->list, &acpi_desc->memdevs);
+			return true;
+		}
+
+	nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev), GFP_KERNEL);
 	if (!nfit_memdev)
 		return false;
 	INIT_LIST_HEAD(&nfit_memdev->list);
@@ -257,12 +281,19 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 }
 
 static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_control_region *dcr)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_dcr *nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr),
-			GFP_KERNEL);
+	struct nfit_dcr *nfit_dcr;
+
+	list_for_each_entry(nfit_dcr, &prev->dcrs, list)
+		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0) {
+			list_move_tail(&nfit_dcr->list, &acpi_desc->dcrs);
+			return true;
+		}
 
+	nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr), GFP_KERNEL);
 	if (!nfit_dcr)
 		return false;
 	INIT_LIST_HEAD(&nfit_dcr->list);
@@ -274,12 +305,19 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
 }
 
 static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_data_region *bdw)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_bdw *nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw),
-			GFP_KERNEL);
+	struct nfit_bdw *nfit_bdw;
+
+	list_for_each_entry(nfit_bdw, &prev->bdws, list)
+		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0) {
+			list_move_tail(&nfit_bdw->list, &acpi_desc->bdws);
+			return true;
+		}
 
+	nfit_bdw = devm_kzalloc(dev, sizeof(*nfit_bdw), GFP_KERNEL);
 	if (!nfit_bdw)
 		return false;
 	INIT_LIST_HEAD(&nfit_bdw->list);
@@ -291,12 +329,19 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
 }
 
 static bool add_idt(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_interleave *idt)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_idt *nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt),
-			GFP_KERNEL);
+	struct nfit_idt *nfit_idt;
+
+	list_for_each_entry(nfit_idt, &prev->idts, list)
+		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0) {
+			list_move_tail(&nfit_idt->list, &acpi_desc->idts);
+			return true;
+		}
 
+	nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt), GFP_KERNEL);
 	if (!nfit_idt)
 		return false;
 	INIT_LIST_HEAD(&nfit_idt->list);
@@ -308,12 +353,19 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 }
 
 static bool add_flush(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev,
 		struct acpi_nfit_flush_address *flush)
 {
 	struct device *dev = acpi_desc->dev;
-	struct nfit_flush *nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush),
-			GFP_KERNEL);
+	struct nfit_flush *nfit_flush;
 
+	list_for_each_entry(nfit_flush, &prev->flushes, list)
+		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0) {
+			list_move_tail(&nfit_flush->list, &acpi_desc->flushes);
+			return true;
+		}
+
+	nfit_flush = devm_kzalloc(dev, sizeof(*nfit_flush), GFP_KERNEL);
 	if (!nfit_flush)
 		return false;
 	INIT_LIST_HEAD(&nfit_flush->list);
@@ -324,8 +376,8 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
 	return true;
 }
 
-static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
-		const void *end)
+static void *add_table(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev, void *table, const void *end)
 {
 	struct device *dev = acpi_desc->dev;
 	struct acpi_nfit_header *hdr;
@@ -343,27 +395,27 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, void *table,
 
 	switch (hdr->type) {
 	case ACPI_NFIT_TYPE_SYSTEM_ADDRESS:
-		if (!add_spa(acpi_desc, table))
+		if (!add_spa(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_MEMORY_MAP:
-		if (!add_memdev(acpi_desc, table))
+		if (!add_memdev(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_CONTROL_REGION:
-		if (!add_dcr(acpi_desc, table))
+		if (!add_dcr(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_DATA_REGION:
-		if (!add_bdw(acpi_desc, table))
+		if (!add_bdw(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_INTERLEAVE:
-		if (!add_idt(acpi_desc, table))
+		if (!add_idt(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_FLUSH_ADDRESS:
-		if (!add_flush(acpi_desc, table))
+		if (!add_flush(acpi_desc, prev, table))
 			return err;
 		break;
 	case ACPI_NFIT_TYPE_SMBIOS:
@@ -808,12 +860,7 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		device_handle = __to_nfit_memdev(nfit_mem)->device_handle;
 		nvdimm = acpi_nfit_dimm_by_handle(acpi_desc, device_handle);
 		if (nvdimm) {
-			/*
-			 * If for some reason we find multiple DCRs the
-			 * first one wins
-			 */
-			dev_err(acpi_desc->dev, "duplicate DCR detected: %s\n",
-					nvdimm_name(nvdimm));
+			dimm_count++;
 			continue;
 		}
 
@@ -1482,6 +1529,9 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 	struct resource res;
 	int count = 0, rc;
 
+	if (nfit_spa->is_registered)
+		return 0;
+
 	if (spa->range_index == 0) {
 		dev_dbg(acpi_desc->dev, "%s: detected invalid spa index\n",
 				__func__);
@@ -1535,6 +1585,8 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 		if (!nvdimm_volatile_region_create(nvdimm_bus, ndr_desc))
 			return -ENOMEM;
 	}
+
+	nfit_spa->is_registered = 1;
 	return 0;
 }
 
@@ -1551,71 +1603,101 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc)
 	return 0;
 }
 
+static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc,
+		struct nfit_table_prev *prev)
+{
+	struct device *dev = acpi_desc->dev;
+
+	if (!list_empty(&prev->spas) ||
+			!list_empty(&prev->memdevs) ||
+			!list_empty(&prev->dcrs) ||
+			!list_empty(&prev->bdws) ||
+			!list_empty(&prev->idts) ||
+			!list_empty(&prev->flushes)) {
+		dev_err(dev, "new nfit deletes entries (unsupported)\n");
+		return -ENXIO;
+	}
+	return 0;
+}
+
 int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 {
 	struct device *dev = acpi_desc->dev;
+	struct nfit_table_prev prev;
 	const void *end;
 	u8 *data;
 	int rc;
 
-	INIT_LIST_HEAD(&acpi_desc->spa_maps);
-	INIT_LIST_HEAD(&acpi_desc->spas);
-	INIT_LIST_HEAD(&acpi_desc->dcrs);
-	INIT_LIST_HEAD(&acpi_desc->bdws);
-	INIT_LIST_HEAD(&acpi_desc->idts);
-	INIT_LIST_HEAD(&acpi_desc->flushes);
-	INIT_LIST_HEAD(&acpi_desc->memdevs);
-	INIT_LIST_HEAD(&acpi_desc->dimms);
-	mutex_init(&acpi_desc->spa_map_mutex);
+	mutex_lock(&acpi_desc->init_mutex);
+
+	INIT_LIST_HEAD(&prev.spas);
+	INIT_LIST_HEAD(&prev.memdevs);
+	INIT_LIST_HEAD(&prev.dcrs);
+	INIT_LIST_HEAD(&prev.bdws);
+	INIT_LIST_HEAD(&prev.idts);
+	INIT_LIST_HEAD(&prev.flushes);
+
+	list_cut_position(&prev.spas, &acpi_desc->spas,
+				acpi_desc->spas.prev);
+	list_cut_position(&prev.memdevs, &acpi_desc->memdevs,
+				acpi_desc->memdevs.prev);
+	list_cut_position(&prev.dcrs, &acpi_desc->dcrs,
+				acpi_desc->dcrs.prev);
+	list_cut_position(&prev.bdws, &acpi_desc->bdws,
+				acpi_desc->bdws.prev);
+	list_cut_position(&prev.idts, &acpi_desc->idts,
+				acpi_desc->idts.prev);
+	list_cut_position(&prev.flushes, &acpi_desc->flushes,
+				acpi_desc->flushes.prev);
 
 	data = (u8 *) acpi_desc->nfit;
 	end = data + sz;
 	data += sizeof(struct acpi_table_nfit);
 	while (!IS_ERR_OR_NULL(data))
-		data = add_table(acpi_desc, data, end);
+		data = add_table(acpi_desc, &prev, data, end);
 
 	if (IS_ERR(data)) {
 		dev_dbg(dev, "%s: nfit table parsing error: %ld\n", __func__,
 				PTR_ERR(data));
-		return PTR_ERR(data);
+		rc = PTR_ERR(data);
+		goto out_unlock;
 	}
 
-	if (nfit_mem_init(acpi_desc) != 0)
-		return -ENOMEM;
+	rc = acpi_nfit_check_deletions(acpi_desc, &prev);
+	if (rc)
+		goto out_unlock;
+
+	if (nfit_mem_init(acpi_desc) != 0) {
+		rc = -ENOMEM;
+		goto out_unlock;
+	}
 
 	acpi_nfit_init_dsms(acpi_desc);
 
 	rc = acpi_nfit_register_dimms(acpi_desc);
 	if (rc)
-		return rc;
+		goto out_unlock;
+
+	rc = acpi_nfit_register_regions(acpi_desc);
 
-	return acpi_nfit_register_regions(acpi_desc);
+ out_unlock:
+	mutex_unlock(&acpi_desc->init_mutex);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(acpi_nfit_init);
 
-static int acpi_nfit_add(struct acpi_device *adev)
+static struct acpi_nfit_desc *acpi_nfit_desc_init(struct acpi_device *adev)
 {
 	struct nvdimm_bus_descriptor *nd_desc;
 	struct acpi_nfit_desc *acpi_desc;
 	struct device *dev = &adev->dev;
-	struct acpi_table_header *tbl;
-	acpi_status status = AE_OK;
-	acpi_size sz;
-	int rc;
-
-	status = acpi_get_table_with_size("NFIT", 0, &tbl, &sz);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "failed to find NFIT\n");
-		return -ENXIO;
-	}
 
 	acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
 	if (!acpi_desc)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dev_set_drvdata(dev, acpi_desc);
 	acpi_desc->dev = dev;
-	acpi_desc->nfit = (struct acpi_table_nfit *) tbl;
 	acpi_desc->blk_do_io = acpi_nfit_blk_region_do_io;
 	nd_desc = &acpi_desc->nd_desc;
 	nd_desc->provider_name = "ACPI.NFIT";
@@ -1623,15 +1705,69 @@ static int acpi_nfit_add(struct acpi_device *adev)
 	nd_desc->attr_groups = acpi_nfit_attribute_groups;
 
 	acpi_desc->nvdimm_bus = nvdimm_bus_register(dev, nd_desc);
-	if (!acpi_desc->nvdimm_bus)
-		return -ENXIO;
+	if (!acpi_desc->nvdimm_bus) {
+		devm_kfree(dev, acpi_desc);
+		return ERR_PTR(-ENXIO);
+	}
+
+	INIT_LIST_HEAD(&acpi_desc->spa_maps);
+	INIT_LIST_HEAD(&acpi_desc->spas);
+	INIT_LIST_HEAD(&acpi_desc->dcrs);
+	INIT_LIST_HEAD(&acpi_desc->bdws);
+	INIT_LIST_HEAD(&acpi_desc->idts);
+	INIT_LIST_HEAD(&acpi_desc->flushes);
+	INIT_LIST_HEAD(&acpi_desc->memdevs);
+	INIT_LIST_HEAD(&acpi_desc->dimms);
+	mutex_init(&acpi_desc->spa_map_mutex);
+	mutex_init(&acpi_desc->init_mutex);
+
+	return acpi_desc;
+}
+
+static int acpi_nfit_add(struct acpi_device *adev)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_nfit_desc *acpi_desc;
+	struct device *dev = &adev->dev;
+	struct acpi_table_header *tbl;
+	acpi_status status = AE_OK;
+	acpi_size sz;
+	int rc = 0;
+
+	device_lock(dev);
+	status = acpi_get_table_with_size("NFIT", 0, &tbl, &sz);
+	if (ACPI_FAILURE(status)) {
+		/* This is ok, we could have an nvdimm hotplugged later */
+		dev_dbg(dev, "failed to find NFIT at startup\n");
+		goto out_unlock;
+	}
+
+	acpi_desc = acpi_nfit_desc_init(adev);
+	if (IS_ERR(acpi_desc)) {
+		dev_err(dev, "%s: error initializing acpi_desc: %ld\n",
+				__func__, PTR_ERR(acpi_desc));
+		rc = PTR_ERR(acpi_desc);
+		goto out_unlock;
+	}
+
+	acpi_desc->nfit = (struct acpi_table_nfit *) tbl;
+
+	/* Evaluate _FIT and override with that if present */
+	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
+	if (ACPI_SUCCESS(status) && buf.length > 0) {
+		acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
+		sz = buf.length;
+	}
 
 	rc = acpi_nfit_init(acpi_desc, sz);
 	if (rc) {
 		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
-		return rc;
+		goto out_unlock;
 	}
-	return 0;
+
+ out_unlock:
+	device_unlock(dev);
+	return rc;
 }
 
 static int acpi_nfit_remove(struct acpi_device *adev)
@@ -1642,6 +1778,54 @@ static int acpi_nfit_remove(struct acpi_device *adev)
 	return 0;
 }
 
+static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
+{
+	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_table_nfit *nfit_saved;
+	struct device *dev = &adev->dev;
+	acpi_status status;
+	int ret;
+
+	dev_dbg(dev, "%s: event: %d\n", __func__, event);
+
+	device_lock(dev);
+	if (!dev->driver) {
+		/* dev->driver may be null if we're being removed */
+		dev_dbg(dev, "%s: no driver found for dev\n", __func__);
+		return;
+	}
+
+	if (!acpi_desc) {
+		acpi_desc = acpi_nfit_desc_init(adev);
+		if (IS_ERR(acpi_desc)) {
+			dev_err(dev, "%s: error initializing acpi_desc: %ld\n",
+				__func__, PTR_ERR(acpi_desc));
+			goto out_unlock;
+		}
+	}
+
+	/* Evaluate _FIT */
+	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "failed to evaluate _FIT\n");
+		goto out_unlock;
+	}
+
+	nfit_saved = acpi_desc->nfit;
+	acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
+	ret = acpi_nfit_init(acpi_desc, buf.length);
+	if (!ret) {
+		/* Merge failed, restore old nfit, and exit */
+		acpi_desc->nfit = nfit_saved;
+		dev_err(dev, "failed to merge updated NFIT\n");
+	}
+	kfree(buf.pointer);
+
+ out_unlock:
+	device_unlock(dev);
+}
+
 static const struct acpi_device_id acpi_nfit_ids[] = {
 	{ "ACPI0012", 0 },
 	{ "", 0 },
@@ -1654,6 +1838,7 @@ static struct acpi_driver acpi_nfit_driver = {
 	.ops = {
 		.add = acpi_nfit_add,
 		.remove = acpi_nfit_remove,
+		.notify = acpi_nfit_notify,
 	},
 };
 
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 7e74015..beb4632 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -48,6 +48,7 @@ enum {
 struct nfit_spa {
 	struct acpi_nfit_system_address *spa;
 	struct list_head list;
+	int is_registered;
 };
 
 struct nfit_dcr {
@@ -97,6 +98,7 @@ struct acpi_nfit_desc {
 	struct nvdimm_bus_descriptor nd_desc;
 	struct acpi_table_nfit *nfit;
 	struct mutex spa_map_mutex;
+	struct mutex init_mutex;
 	struct list_head spa_maps;
 	struct list_head memdevs;
 	struct list_head flushes;
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 021e6f9..dce346a 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -17,8 +17,10 @@
 #include <linux/vmalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/ndctl.h>
 #include <linux/sizes.h>
+#include <linux/list.h>
 #include <linux/slab.h>
 #include <nfit.h>
 #include <nd.h>
@@ -44,6 +46,15 @@
  * +------+  |                 blk5.0             |  pm1.0  |    3      region5
  *           +-------------------------+----------+-+-------+
  *
+ * +--+---+
+ * | cpu1 |
+ * +--+---+                   (Hotplug DIMM)
+ *    |      +----------------------------------------------+
+ * +--+---+  |                 blk6.0/pm7.0                 |    4      region6/7
+ * | imc0 +--+----------------------------------------------+
+ * +------+
+ *
+ *
  * *) In this layout we have four dimms and two memory controllers in one
  *    socket.  Each unique interface (BLK or PMEM) to DPA space
  *    is identified by a region device with a dynamically assigned id.
@@ -85,8 +96,8 @@
  *    reference an NVDIMM.
  */
 enum {
-	NUM_PM  = 2,
-	NUM_DCR = 4,
+	NUM_PM  = 3,
+	NUM_DCR = 5,
 	NUM_BDW = NUM_DCR,
 	NUM_SPA = NUM_PM + NUM_DCR + NUM_BDW,
 	NUM_MEM = NUM_DCR + NUM_BDW + 2 /* spa0 iset */ + 4 /* spa1 iset */,
@@ -115,6 +126,7 @@ static u32 handle[NUM_DCR] = {
 	[1] = NFIT_DIMM_HANDLE(0, 0, 0, 0, 1),
 	[2] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 0),
 	[3] = NFIT_DIMM_HANDLE(0, 0, 1, 0, 1),
+	[4] = NFIT_DIMM_HANDLE(0, 1, 0, 0, 0),
 };
 
 struct nfit_test {
@@ -138,6 +150,7 @@ struct nfit_test {
 	dma_addr_t *dcr_dma;
 	int (*alloc)(struct nfit_test *t);
 	void (*setup)(struct nfit_test *t);
+	int setup_hotplug;
 };
 
 static struct nfit_test *to_nfit_test(struct device *dev)
@@ -428,6 +441,10 @@ static int nfit_test0_alloc(struct nfit_test *t)
 	if (!t->spa_set[1])
 		return -ENOMEM;
 
+	t->spa_set[2] = test_alloc_coherent(t, SPA0_SIZE, &t->spa_set_dma[2]);
+	if (!t->spa_set[2])
+		return -ENOMEM;
+
 	for (i = 0; i < NUM_DCR; i++) {
 		t->dimm[i] = test_alloc(t, DIMM_SIZE, &t->dimm_dma[i]);
 		if (!t->dimm[i])
@@ -950,6 +967,126 @@ static void nfit_test0_setup(struct nfit_test *t)
 	flush->hint_count = 1;
 	flush->hint_address[0] = t->flush_dma[3];
 
+	if (t->setup_hotplug) {
+		offset = offset + sizeof(struct acpi_nfit_flush_address) * 4;
+		/* dcr-descriptor4 */
+		dcr = nfit_buf + offset;
+		dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
+		dcr->header.length = sizeof(struct acpi_nfit_control_region);
+		dcr->region_index = 4+1;
+		dcr->vendor_id = 0xabcd;
+		dcr->device_id = 0;
+		dcr->revision_id = 1;
+		dcr->serial_number = ~handle[4];
+		dcr->windows = 1;
+		dcr->window_size = DCR_SIZE;
+		dcr->command_offset = 0;
+		dcr->command_size = 8;
+		dcr->status_offset = 8;
+		dcr->status_size = 4;
+
+		offset = offset + sizeof(struct acpi_nfit_control_region);
+		/* bdw4 (spa/dcr4, dimm4) */
+		bdw = nfit_buf + offset;
+		bdw->header.type = ACPI_NFIT_TYPE_DATA_REGION;
+		bdw->header.length = sizeof(struct acpi_nfit_data_region);
+		bdw->region_index = 4+1;
+		bdw->windows = 1;
+		bdw->offset = 0;
+		bdw->size = BDW_SIZE;
+		bdw->capacity = DIMM_SIZE;
+		bdw->start_address = 0;
+
+		offset = offset + sizeof(struct acpi_nfit_data_region);
+		/* spa10 (dcr4) dimm4 */
+		spa = nfit_buf + offset;
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);
+		spa->range_index = 10+1;
+		spa->address = t->dcr_dma[4];
+		spa->length = DCR_SIZE;
+
+		/*
+		 * spa11 (single-dimm interleave for hotplug, note storage
+		 * does not actually alias the related block-data-window
+		 * regions)
+		 */
+		spa = nfit_buf + offset + sizeof(*spa);
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);
+		spa->range_index = 11+1;
+		spa->address = t->spa_set_dma[2];
+		spa->length = SPA0_SIZE;
+
+		/* spa12 (bdw for dcr4) dimm4 */
+		spa = nfit_buf + offset + sizeof(*spa) * 2;
+		spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
+		spa->header.length = sizeof(*spa);
+		memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);
+		spa->range_index = 12+1;
+		spa->address = t->dimm_dma[4];
+		spa->length = DIMM_SIZE;
+
+		offset = offset + sizeof(*spa) * 3;
+		/* mem-region14 (spa/dcr4, dimm4) */
+		memdev = nfit_buf + offset;
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 10+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = 0;
+		memdev->region_offset = 0;
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+		/* mem-region15 (spa0, dimm4) */
+		memdev = nfit_buf + offset +
+				sizeof(struct acpi_nfit_memory_map);
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 11+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = SPA0_SIZE;
+		memdev->region_offset = t->spa_set_dma[2];
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+		/* mem-region16 (spa/dcr4, dimm4) */
+		memdev = nfit_buf + offset +
+				sizeof(struct acpi_nfit_memory_map) * 2;
+		memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
+		memdev->header.length = sizeof(*memdev);
+		memdev->device_handle = handle[4];
+		memdev->physical_id = 4;
+		memdev->region_id = 0;
+		memdev->range_index = 12+1;
+		memdev->region_index = 4+1;
+		memdev->region_size = 0;
+		memdev->region_offset = 0;
+		memdev->address = 0;
+		memdev->interleave_index = 0;
+		memdev->interleave_ways = 1;
+
+		offset = offset + sizeof(struct acpi_nfit_memory_map) * 3;
+		/* flush3 (dimm4) */
+		flush = nfit_buf + offset;
+		flush->header.type = ACPI_NFIT_TYPE_FLUSH_ADDRESS;
+		flush->header.length = sizeof(struct acpi_nfit_flush_address);
+		flush->device_handle = handle[4];
+		flush->hint_count = 1;
+		flush->hint_address[0] = t->flush_dma[4];
+	}
+
 	acpi_desc = &t->acpi_desc;
 	set_bit(ND_CMD_GET_CONFIG_SIZE, &acpi_desc->dimm_dsm_force_en);
 	set_bit(ND_CMD_GET_CONFIG_DATA, &acpi_desc->dimm_dsm_force_en);
@@ -1108,6 +1245,29 @@ static int nfit_test_probe(struct platform_device *pdev)
 	if (!acpi_desc->nvdimm_bus)
 		return -ENXIO;
 
+	INIT_LIST_HEAD(&acpi_desc->spa_maps);
+	INIT_LIST_HEAD(&acpi_desc->spas);
+	INIT_LIST_HEAD(&acpi_desc->dcrs);
+	INIT_LIST_HEAD(&acpi_desc->bdws);
+	INIT_LIST_HEAD(&acpi_desc->idts);
+	INIT_LIST_HEAD(&acpi_desc->flushes);
+	INIT_LIST_HEAD(&acpi_desc->memdevs);
+	INIT_LIST_HEAD(&acpi_desc->dimms);
+	mutex_init(&acpi_desc->spa_map_mutex);
+	mutex_init(&acpi_desc->init_mutex);
+
+	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size);
+	if (rc) {
+		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
+		return rc;
+	}
+
+	if (nfit_test->setup != nfit_test0_setup)
+		return 0;
+
+	nfit_test->setup_hotplug = 1;
+	nfit_test->setup(nfit_test);
+
 	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size);
 	if (rc) {
 		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
-- 
2.4.3

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Dan Williams]

Rafael, awaiting your ack...

Vishal, one thing I'll fix up on applying...

On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> Add a .notify callback to the acpi_nfit_driver that gets called on a
> hotplug event. From this, evaluate the _FIT ACPI method which returns
> the updated NFIT with handles for the hot-plugged NVDIMM.
>
> Iterate over the new NFIT, and add any new tables found, and
> register/enable the corresponding regions.
>
> In the nfit test framework, after normal initialization, update the NFIT
> with a new hot-plugged NVDIMM, and directly call into the driver to
> update its view of the available regions.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Cc: Elliott, Robert <elliott@hpe.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: <linux-acpi@vger.kernel.org>
> Cc: <linux-nvdimm@lists.01.org>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
[..]

> +static int acpi_nfit_add(struct acpi_device *adev)
> +{
> +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +       struct acpi_nfit_desc *acpi_desc;
> +       struct device *dev = &adev->dev;
> +       struct acpi_table_header *tbl;
> +       acpi_status status = AE_OK;
> +       acpi_size sz;
> +       int rc = 0;
> +
> +       device_lock(dev);

The device is already locked by the time we reach this point.  The
unit tests don't trip this path which might be why this wasn't seen
earlier.

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Dan Williams]

On Sat, Nov 7, 2015 at 10:57 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> Rafael, awaiting your ack...
>
> Vishal, one thing I'll fix up on applying...
>
> On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
>> Add a .notify callback to the acpi_nfit_driver that gets called on a
>> hotplug event. From this, evaluate the _FIT ACPI method which returns
>> the updated NFIT with handles for the hot-plugged NVDIMM.
>>
>> Iterate over the new NFIT, and add any new tables found, and
>> register/enable the corresponding regions.
>>
>> In the nfit test framework, after normal initialization, update the NFIT
>> with a new hot-plugged NVDIMM, and directly call into the driver to
>> update its view of the available regions.
>>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Toshi Kani <toshi.kani@hpe.com>
>> Cc: Elliott, Robert <elliott@hpe.com>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> Cc: <linux-acpi@vger.kernel.org>
>> Cc: <linux-nvdimm@lists.01.org>
>> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> [..]
>
>> +static int acpi_nfit_add(struct acpi_device *adev)
>> +{
>> +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>> +       struct acpi_nfit_desc *acpi_desc;
>> +       struct device *dev = &adev->dev;
>> +       struct acpi_table_header *tbl;
>> +       acpi_status status = AE_OK;
>> +       acpi_size sz;
>> +       int rc = 0;
>> +
>> +       device_lock(dev);
>
> The device is already locked by the time we reach this point.  The
> unit tests don't trip this path which might be why this wasn't seen
> earlier.

The lockup call trace if you're interested:

dracut:/# [  376.030455] INFO: task systemd-udevd:278 blocked for more
than 120 seconds.
[  376.032561]       Tainted: G           O    4.3.0-rc6+ #1747
[  376.034376] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[  376.036704] systemd-udevd   D 0000000000000000     0   278    262 0x00000004
[  376.039758]  ffff880352a0ba60 0000000000000092 ffff880352a0ba88
ffff880361e57b98
[  376.043779]  ffff88035f150000 ffff880352ea5dc0 ffff880352a0c000
0000000000000246
[  376.047800]  ffff880359a43148 ffff880352ea5dc0 00000000ffffffff
ffff880352a0ba78
[  376.051821] Call Trace:
[  376.052882]  [<ffffffff818e8b43>] schedule+0x33/0x80
[  376.054527]  [<ffffffff818e8eae>] schedule_preempt_disabled+0xe/0x10
[  376.056493]  [<ffffffff818eaf2f>] mutex_lock_nested+0x14f/0x3a0
[  376.058361]  [<ffffffffa003140e>] ? acpi_nfit_add+0x4e/0x150 [nfit]
[  376.060310]  [<ffffffffa003140e>] acpi_nfit_add+0x4e/0x150 [nfit]
[  376.062283]  [<ffffffff81584310>] ? devices_kset_move_last+0x60/0x90
[  376.064250]  [<ffffffff814d61c0>] acpi_device_probe+0x4f/0xf5
[  376.066076]  [<ffffffff81588244>] driver_probe_device+0x224/0x480
[  376.067983]  [<ffffffff81588528>] __driver_attach+0x88/0x90
[  376.069769]  [<ffffffff815884a0>] ? driver_probe_device+0x480/0x480
[  376.071716]  [<ffffffff81585d83>] bus_for_each_dev+0x73/0xc0
[  376.073522]  [<ffffffff81587b9e>] driver_attach+0x1e/0x20
[  376.075267]  [<ffffffff815876de>] bus_add_driver+0x1ee/0x280
[  376.077072]  [<ffffffffa0038000>] ? 0xffffffffa0038000
[  376.078757]  [<ffffffff81588f10>] driver_register+0x60/0xe0
[  376.080543]  [<ffffffff814d6095>] acpi_bus_register_driver+0x40/0x42
[  376.082511]  [<ffffffffa00380ce>] nfit_init+0xce/0x1000 [nfit]


...fixed by the following incremental change:

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 869f279fde95..a2e99ccf0480 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1732,22 +1732,19 @@ static int acpi_nfit_add(struct acpi_device *adev)
       struct acpi_table_header *tbl;
       acpi_status status = AE_OK;
       acpi_size sz;
-       int rc = 0;

-       device_lock(dev);
       status = acpi_get_table_with_size("NFIT", 0, &tbl, &sz);
       if (ACPI_FAILURE(status)) {
               /* This is ok, we could have an nvdimm hotplugged later */
               dev_dbg(dev, "failed to find NFIT at startup\n");
-               goto out_unlock;
+               return 0;
       }

       acpi_desc = acpi_nfit_desc_init(adev);
       if (IS_ERR(acpi_desc)) {
               dev_err(dev, "%s: error initializing acpi_desc: %ld\n",
                               __func__, PTR_ERR(acpi_desc));
-               rc = PTR_ERR(acpi_desc);
-               goto out_unlock;
+               return PTR_ERR(acpi_desc);
       }

       acpi_desc->nfit = (struct acpi_table_nfit *) tbl;
@@ -1762,12 +1759,9 @@ static int acpi_nfit_add(struct acpi_device *adev)
       rc = acpi_nfit_init(acpi_desc, sz);
       if (rc) {
               nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
-               goto out_unlock;
+               return rc;
       }
-
- out_unlock:
-       device_unlock(dev);
-       return rc;
+       return 0;
}

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Verma, Vishal L]

On Sat, 2015-11-07 at 13:20 -0800, Dan Williams wrote:
> On Sat, Nov 7, 2015 at 10:57 AM, Dan Williams <dan.j.williams@intel.co
> m> wrote:
> > Rafael, awaiting your ack...
> > 
> > Vishal, one thing I'll fix up on applying...
> > 
> > On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel.
> > com> wrote:
> > > Add a .notify callback to the acpi_nfit_driver that gets called on
> > > a
> > > hotplug event. From this, evaluate the _FIT ACPI method which
> > > returns
> > > the updated NFIT with handles for the hot-plugged NVDIMM.
> > > 
> > > Iterate over the new NFIT, and add any new tables found, and
> > > register/enable the corresponding regions.
> > > 
> > > In the nfit test framework, after normal initialization, update
> > > the NFIT
> > > with a new hot-plugged NVDIMM, and directly call into the driver
> > > to
> > > update its view of the available regions.
> > > 
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Toshi Kani <toshi.kani@hpe.com>
> > > Cc: Elliott, Robert <elliott@hpe.com>
> > > Cc: Jeff Moyer <jmoyer@redhat.com>
> > > Cc: <linux-acpi@vger.kernel.org>
> > > Cc: <linux-nvdimm@lists.01.org>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > [..]
> > 
> > > +static int acpi_nfit_add(struct acpi_device *adev)
> > > +{
> > > +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> > > +       struct acpi_nfit_desc *acpi_desc;
> > > +       struct device *dev = &adev->dev;
> > > +       struct acpi_table_header *tbl;
> > > +       acpi_status status = AE_OK;
> > > +       acpi_size sz;
> > > +       int rc = 0;
> > > +
> > > +       device_lock(dev);
> > 
> > The device is already locked by the time we reach this point.  The
> > unit tests don't trip this path which might be why this wasn't seen
> > earlier.
> 
> The lockup call trace if you're interested:
> 
> dracut:/# [  376.030455] INFO: task systemd-udevd:278 blocked for more
> than 120 seconds.
> [  376.032561]       Tainted: G           O    4.3.0-rc6+ #1747
> [  376.034376] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [  376.036704] systemd-udevd   D 0000000000000000     0   278    262
> 0x00000004
> [  376.039758]  ffff880352a0ba60 0000000000000092 ffff880352a0ba88
> ffff880361e57b98
> [  376.043779]  ffff88035f150000 ffff880352ea5dc0 ffff880352a0c000
> 0000000000000246
> [  376.047800]  ffff880359a43148 ffff880352ea5dc0 00000000ffffffff
> ffff880352a0ba78
> [  376.051821] Call Trace:
> [  376.052882]  [<ffffffff818e8b43>] schedule+0x33/0x80
> [  376.054527]  [<ffffffff818e8eae>]
> schedule_preempt_disabled+0xe/0x10
> [  376.056493]  [<ffffffff818eaf2f>] mutex_lock_nested+0x14f/0x3a0
> [  376.058361]  [<ffffffffa003140e>] ? acpi_nfit_add+0x4e/0x150 [nfit]
> [  376.060310]  [<ffffffffa003140e>] acpi_nfit_add+0x4e/0x150 [nfit]
> [  376.062283]  [<ffffffff81584310>] ?
> devices_kset_move_last+0x60/0x90
> [  376.064250]  [<ffffffff814d61c0>] acpi_device_probe+0x4f/0xf5
> [  376.066076]  [<ffffffff81588244>] driver_probe_device+0x224/0x480
> [  376.067983]  [<ffffffff81588528>] __driver_attach+0x88/0x90
> [  376.069769]  [<ffffffff815884a0>] ? driver_probe_device+0x480/0x480
> [  376.071716]  [<ffffffff81585d83>] bus_for_each_dev+0x73/0xc0
> [  376.073522]  [<ffffffff81587b9e>] driver_attach+0x1e/0x20
> [  376.075267]  [<ffffffff815876de>] bus_add_driver+0x1ee/0x280
> [  376.077072]  [<ffffffffa0038000>] ? 0xffffffffa0038000
> [  376.078757]  [<ffffffff81588f10>] driver_register+0x60/0xe0
> [  376.080543]  [<ffffffff814d6095>]
> acpi_bus_register_driver+0x40/0x42
> [  376.082511]  [<ffffffffa00380ce>] nfit_init+0xce/0x1000 [nfit]
> 
> 

Thanks for the fixup, Dan. The change looks good - I think I got too
reliant on lockdep not complaining - should've taken a closer look.

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Dan Williams]

On Mon, Nov 9, 2015 at 10:12 AM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Sat, 2015-11-07 at 13:20 -0800, Dan Williams wrote:
>> On Sat, Nov 7, 2015 at 10:57 AM, Dan Williams <dan.j.williams@intel.co
>> m> wrote:
>> > Rafael, awaiting your ack...
>> >
>> > Vishal, one thing I'll fix up on applying...
>> >
>> > On Tue, Oct 27, 2015 at 3:58 PM, Vishal Verma <vishal.l.verma@intel.
>> > com> wrote:
>> > > Add a .notify callback to the acpi_nfit_driver that gets called on
>> > > a
>> > > hotplug event. From this, evaluate the _FIT ACPI method which
>> > > returns
>> > > the updated NFIT with handles for the hot-plugged NVDIMM.
>> > >
>> > > Iterate over the new NFIT, and add any new tables found, and
>> > > register/enable the corresponding regions.
>> > >
>> > > In the nfit test framework, after normal initialization, update
>> > > the NFIT
>> > > with a new hot-plugged NVDIMM, and directly call into the driver
>> > > to
>> > > update its view of the available regions.
>> > >
>> > > Cc: Dan Williams <dan.j.williams@intel.com>
>> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > > Cc: Toshi Kani <toshi.kani@hpe.com>
>> > > Cc: Elliott, Robert <elliott@hpe.com>
>> > > Cc: Jeff Moyer <jmoyer@redhat.com>
>> > > Cc: <linux-acpi@vger.kernel.org>
>> > > Cc: <linux-nvdimm@lists.01.org>
>> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
>> > [..]
>> >
>> > > +static int acpi_nfit_add(struct acpi_device *adev)
>> > > +{
>> > > +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>> > > +       struct acpi_nfit_desc *acpi_desc;
>> > > +       struct device *dev = &adev->dev;
>> > > +       struct acpi_table_header *tbl;
>> > > +       acpi_status status = AE_OK;
>> > > +       acpi_size sz;
>> > > +       int rc = 0;
>> > > +
>> > > +       device_lock(dev);
>> >
>> > The device is already locked by the time we reach this point.  The
>> > unit tests don't trip this path which might be why this wasn't seen
>> > earlier.
>>
>> The lockup call trace if you're interested:
>>
>> dracut:/# [  376.030455] INFO: task systemd-udevd:278 blocked for more
>> than 120 seconds.
>> [  376.032561]       Tainted: G           O    4.3.0-rc6+ #1747
>> [  376.034376] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
>> disables this message.
>> [  376.036704] systemd-udevd   D 0000000000000000     0   278    262
>> 0x00000004
>> [  376.039758]  ffff880352a0ba60 0000000000000092 ffff880352a0ba88
>> ffff880361e57b98
>> [  376.043779]  ffff88035f150000 ffff880352ea5dc0 ffff880352a0c000
>> 0000000000000246
>> [  376.047800]  ffff880359a43148 ffff880352ea5dc0 00000000ffffffff
>> ffff880352a0ba78
>> [  376.051821] Call Trace:
>> [  376.052882]  [<ffffffff818e8b43>] schedule+0x33/0x80
>> [  376.054527]  [<ffffffff818e8eae>]
>> schedule_preempt_disabled+0xe/0x10
>> [  376.056493]  [<ffffffff818eaf2f>] mutex_lock_nested+0x14f/0x3a0
>> [  376.058361]  [<ffffffffa003140e>] ? acpi_nfit_add+0x4e/0x150 [nfit]
>> [  376.060310]  [<ffffffffa003140e>] acpi_nfit_add+0x4e/0x150 [nfit]
>> [  376.062283]  [<ffffffff81584310>] ?
>> devices_kset_move_last+0x60/0x90
>> [  376.064250]  [<ffffffff814d61c0>] acpi_device_probe+0x4f/0xf5
>> [  376.066076]  [<ffffffff81588244>] driver_probe_device+0x224/0x480
>> [  376.067983]  [<ffffffff81588528>] __driver_attach+0x88/0x90
>> [  376.069769]  [<ffffffff815884a0>] ? driver_probe_device+0x480/0x480
>> [  376.071716]  [<ffffffff81585d83>] bus_for_each_dev+0x73/0xc0
>> [  376.073522]  [<ffffffff81587b9e>] driver_attach+0x1e/0x20
>> [  376.075267]  [<ffffffff815876de>] bus_add_driver+0x1ee/0x280
>> [  376.077072]  [<ffffffffa0038000>] ? 0xffffffffa0038000
>> [  376.078757]  [<ffffffff81588f10>] driver_register+0x60/0xe0
>> [  376.080543]  [<ffffffff814d6095>]
>> acpi_bus_register_driver+0x40/0x42
>> [  376.082511]  [<ffffffffa00380ce>] nfit_init+0xce/0x1000 [nfit]
>>
>>
>
> Thanks for the fixup, Dan. The change looks good - I think I got too
> reliant on lockdep not complaining - should've taken a closer look.

Unfortunately device_lock() has this by default in device_initialize():

void device_initialize(struct device *dev)
{
...
       lockdep_set_novalidate_class(&dev->mutex);
...
}

...so it hides these issues by default.

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Rafael J. Wysocki]

On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
> Rafael, awaiting your ack...

Well, this seems to be entirely NFIT-related.

Is there anything in particular you want me to look at?

Rafael

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Dan Williams]

On Sun, Nov 8, 2015 at 4:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
>> Rafael, awaiting your ack...
>
> Well, this seems to be entirely NFIT-related.
>
> Is there anything in particular you want me to look at?
>

This might be more relevant for a follow-on patch, but I wonder if the
core should be guaranteeing that the ->notify() callback occurs under
device_lock().  In the case of NFIT it seems possible for the notify
event to race ->add() or ->remove(), but maybe I missed some other
guarantee?

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Dan Williams]

On Sun, Nov 8, 2015 at 5:26 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sun, Nov 8, 2015 at 4:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
>>> Rafael, awaiting your ack...
>>
>> Well, this seems to be entirely NFIT-related.
>>
>> Is there anything in particular you want me to look at?
>>
>
> This might be more relevant for a follow-on patch, but I wonder if the
> core should be guaranteeing that the ->notify() callback occurs under
> device_lock().  In the case of NFIT it seems possible for the notify
> event to race ->add() or ->remove(), but maybe I missed some other
> guarantee?

...and no worries if you don't see anything worth commenting on, the
bulk of this is indeed NFIT specific.

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Rafael J. Wysocki]

On Monday, November 09, 2015 10:25:10 AM Dan Williams wrote:
> On Sun, Nov 8, 2015 at 5:26 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Sun, Nov 8, 2015 at 4:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
> >>> Rafael, awaiting your ack...
> >>
> >> Well, this seems to be entirely NFIT-related.
> >>
> >> Is there anything in particular you want me to look at?
> >>
> >
> > This might be more relevant for a follow-on patch, but I wonder if the
> > core should be guaranteeing that the ->notify() callback occurs under
> > device_lock().  In the case of NFIT it seems possible for the notify
> > event to race ->add() or ->remove(), but maybe I missed some other
> > guarantee?
> 
> ...and no worries if you don't see anything worth commenting on, the
> bulk of this is indeed NFIT specific.

I actually don't see anything objectionable in it, although admittedly I've
just had a cursorly look at it.

Thanks,
Rafael

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Rafael J. Wysocki]

On Sunday, November 08, 2015 05:26:03 PM Dan Williams wrote:
> On Sun, Nov 8, 2015 at 4:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
> >> Rafael, awaiting your ack...
> >
> > Well, this seems to be entirely NFIT-related.
> >
> > Is there anything in particular you want me to look at?
> >
> 
> This might be more relevant for a follow-on patch, but I wonder if the
> core should be guaranteeing that the ->notify() callback occurs under
> device_lock().  In the case of NFIT it seems possible for the notify
> event to race ->add() or ->remove(), but maybe I missed some other
> guarantee?

No, you didn't.

I'd rather drop the ->notify callback completely, because it's kind of
confusing, as there are other (non-ACPI) drivers that need to receive
ACPI notifications too and they have to use the raw ACPICA's
acpi_install_notify_handler() interface.  Having a special interface for
that for ACPI drivers only doesn't make much sense to me.

That said, the ACPICA's notify handling code is inherently racy with respect to
module unload, so potentially dangerous in any modular code.

IMO, a generic module-safe wrapper around that code is needed, but since I
haven't had the time to introduce one for quite a while, I guess it would be
fine to add device locking around ->notify in acpi_bus_notify(), at least for
the time being.

Thanks,
Rafael

Re: [PATCH v3 2/2] acpi: nfit: Add support for hot-add

[Rafael J. Wysocki]

On Monday, November 09, 2015 10:24:00 PM Rafael J. Wysocki wrote:
> On Sunday, November 08, 2015 05:26:03 PM Dan Williams wrote:
> > On Sun, Nov 8, 2015 at 4:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Saturday, November 07, 2015 10:57:17 AM Dan Williams wrote:
> > >> Rafael, awaiting your ack...
> > >
> > > Well, this seems to be entirely NFIT-related.
> > >
> > > Is there anything in particular you want me to look at?
> > >
> > 
> > This might be more relevant for a follow-on patch, but I wonder if the
> > core should be guaranteeing that the ->notify() callback occurs under
> > device_lock().  In the case of NFIT it seems possible for the notify
> > event to race ->add() or ->remove(), but maybe I missed some other
> > guarantee?
> 
> No, you didn't.
> 
> I'd rather drop the ->notify callback completely, because it's kind of
> confusing, as there are other (non-ACPI) drivers that need to receive
> ACPI notifications too and they have to use the raw ACPICA's
> acpi_install_notify_handler() interface.  Having a special interface for
> that for ACPI drivers only doesn't make much sense to me.
> 
> That said, the ACPICA's notify handling code is inherently racy with respect to
> module unload, so potentially dangerous in any modular code.
> 
> IMO, a generic module-safe wrapper around that code is needed, but since I
> haven't had the time to introduce one for quite a while, I guess it would be
> fine to add device locking around ->notify in acpi_bus_notify(), at least for
> the time being.

And in acpi_device_notify() for that matter.

Thanks,
Rafael