[PATCH v2 0/7] libnvdimm: Fix async operations and locking

[PATCH v2 0/7] libnvdimm: Fix async operations and locking

[Dan Williams]

Changes since v1 [1]:

- Fix an ioctl command corruption regression that manifested as an
  intermittent failure of the monitor.sh unit test. This is handled in
  the patch4 prep patch that makes it safe for nd_ioctl() to be
  re-entrant. (Vishal)

- Update the changelog for the driver-core 'lockdep_lock' hack to
  indicate Greg's non-NAK.

[1]: https://lore.kernel.org/lkml/156029554317.419799.1324389595953183385.stgit@dwillia2-desk3.amr.corp.intel.com/

---

The libnvdimm subsystem uses async operations to parallelize device
probing operations and to allow sysfs to trigger device_unregister() on
deleted namepsaces. A multithreaded stress test of the libnvdimm sysfs
interface uncovered a case where device_unregister() is triggered
multiple times, and the subsequent investigation uncovered a broken
locking scenario.

The lack of lockdep coverage for device_lock() stymied the debug. That
is, until patch6 "driver-core, libnvdimm: Let device subsystems add
local lockdep coverage" solved that with a shadow lock, with lockdep
coverage, to mirror device_lock() operations. Given the time saved with
shadow-lock debug-hack, patch6 attempts to generalize device_lock()
debug facility that might be able to be carried upstream. Patch6 is
staged at the end of this fix series in case it is contentious and needs
to be dropped.

Patch1 "drivers/base: Introduce kill_device()" could be achieved with
local libnvdimm infrastructure. However, the existing 'dead' flag in
'struct device_private' aims to solve similar async register/unregister
races so the fix in patch2 "libnvdimm/bus: Prevent duplicate
device_unregister() calls" can be implemented with existing driver-core
infrastructure.

Patch3 is a rare lockdep warning that is intermittent based on
namespaces racing ahead of the completion of probe of their parent
region. It is not related to the other fixes, it just happened to
trigger as a result of the async stress test.

Patch5 and patch6 address an ABBA deadlock tripped by the stress test.

These patches pass the failing stress test and the existing libnvdimm
unit tests with CONFIG_PROVE_LOCKING=y and the new "dev->lockdep_mutex"
shadow lock with no lockdep warnings.

---

Dan Williams (7):
      drivers/base: Introduce kill_device()
      libnvdimm/bus: Prevent duplicate device_unregister() calls
      libnvdimm/region: Register badblocks before namespaces
      libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant
      libnvdimm/bus: Stop holding nvdimm_bus_list_mutex over __nd_ioctl()
      libnvdimm/bus: Fix wait_nvdimm_bus_probe_idle() ABBA deadlock
      driver-core, libnvdimm: Let device subsystems add local lockdep coverage


 drivers/acpi/nfit/core.c        |   28 +++--
 drivers/acpi/nfit/nfit.h        |   24 ++++
 drivers/base/core.c             |   30 ++++--
 drivers/nvdimm/btt_devs.c       |   16 +--
 drivers/nvdimm/bus.c            |  210 ++++++++++++++++++++++++++-------------
 drivers/nvdimm/core.c           |   10 +-
 drivers/nvdimm/dimm_devs.c      |    4 -
 drivers/nvdimm/namespace_devs.c |   36 +++----
 drivers/nvdimm/nd-core.h        |   71 +++++++++++++
 drivers/nvdimm/pfn_devs.c       |   24 ++--
 drivers/nvdimm/pmem.c           |    4 -
 drivers/nvdimm/region.c         |   24 ++--
 drivers/nvdimm/region_devs.c    |   12 +-
 include/linux/device.h          |    6 +
 14 files changed, 343 insertions(+), 156 deletions(-)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 1/7] drivers/base: Introduce kill_device()

[Dan Williams]

The libnvdimm subsystem arranges for devices to be destroyed as a result
of a sysfs operation. Since device_unregister() cannot be called from
an actively running sysfs attribute of the same device libnvdimm
arranges for device_unregister() to be performed in an out-of-line async
context.

The driver core maintains a 'dead' state for coordinating its own racing
async registration / de-registration requests. Rather than add local
'dead' state tracking infrastructure to libnvdimm device objects, export
the existing state tracking via a new kill_device() helper.

The kill_device() helper simply marks the device as dead, i.e. that it
is on its way to device_del(), or returns that the device was already
dead. This can be used in advance of calling device_unregister() for
subsystems like libnvdimm that might need to handle multiple user
threads racing to delete a device.

This refactoring does not change any behavior, but it is a pre-requisite
for follow-on fixes and therefore marked for -stable.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Fixes: 4d88a97aa9e8 ("libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver...")
Cc: <stable@vger.kernel.org>
Tested-by: Jane Chu <jane.chu@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/base/core.c    |   27 +++++++++++++++++++--------
 include/linux/device.h |    1 +
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..eaf3aa0cb803 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2211,6 +2211,24 @@ void put_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(put_device);
 
+bool kill_device(struct device *dev)
+{
+	/*
+	 * Require the device lock and set the "dead" flag to guarantee that
+	 * the update behavior is consistent with the other bitfields near
+	 * it and that we cannot have an asynchronous probe routine trying
+	 * to run while we are tearing out the bus/class/sysfs from
+	 * underneath the device.
+	 */
+	lockdep_assert_held(&dev->mutex);
+
+	if (dev->p->dead)
+		return false;
+	dev->p->dead = true;
+	return true;
+}
+EXPORT_SYMBOL_GPL(kill_device);
+
 /**
  * device_del - delete device from system.
  * @dev: device.
@@ -2230,15 +2248,8 @@ void device_del(struct device *dev)
 	struct kobject *glue_dir = NULL;
 	struct class_interface *class_intf;
 
-	/*
-	 * Hold the device lock and set the "dead" flag to guarantee that
-	 * the update behavior is consistent with the other bitfields near
-	 * it and that we cannot have an asynchronous probe routine trying
-	 * to run while we are tearing out the bus/class/sysfs from
-	 * underneath the device.
-	 */
 	device_lock(dev);
-	dev->p->dead = true;
+	kill_device(dev);
 	device_unlock(dev);
 
 	/* Notify clients of device removal.  This call must come
diff --git a/include/linux/device.h b/include/linux/device.h
index e85264fb6616..0da5c67f6be1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1373,6 +1373,7 @@ extern int (*platform_notify_remove)(struct device *dev);
  */
 extern struct device *get_device(struct device *dev);
 extern void put_device(struct device *dev);
+extern bool kill_device(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
 extern int devtmpfs_create_node(struct device *dev);

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

Re: [PATCH v2 1/7] drivers/base: Introduce kill_device()

[Greg Kroah-Hartman]

On Wed, Jul 17, 2019 at 06:07:53PM -0700, Dan Williams wrote:
> The libnvdimm subsystem arranges for devices to be destroyed as a result
> of a sysfs operation. Since device_unregister() cannot be called from
> an actively running sysfs attribute of the same device libnvdimm
> arranges for device_unregister() to be performed in an out-of-line async
> context.
> 
> The driver core maintains a 'dead' state for coordinating its own racing
> async registration / de-registration requests. Rather than add local
> 'dead' state tracking infrastructure to libnvdimm device objects, export
> the existing state tracking via a new kill_device() helper.
> 
> The kill_device() helper simply marks the device as dead, i.e. that it
> is on its way to device_del(), or returns that the device was already
> dead. This can be used in advance of calling device_unregister() for
> subsystems like libnvdimm that might need to handle multiple user
> threads racing to delete a device.
> 
> This refactoring does not change any behavior, but it is a pre-requisite
> for follow-on fixes and therefore marked for -stable.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Fixes: 4d88a97aa9e8 ("libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver...")
> Cc: <stable@vger.kernel.org>
> Tested-by: Jane Chu <jane.chu@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 1/7] drivers/base: Introduce kill_device()

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 4d88a97aa9e8 libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure.

The bot has tested the following trees: v5.2.1, v5.1.18, v4.19.59, v4.14.133, v4.9.185, v4.4.185.

v5.2.1: Build OK!
v5.1.18: Build OK!
v4.19.59: Failed to apply! Possible dependencies:
    3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag")

v4.14.133: Failed to apply! Possible dependencies:
    3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag")

v4.9.185: Failed to apply! Possible dependencies:
    3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag")

v4.4.185: Failed to apply! Possible dependencies:
    3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag")
    656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from bus_type.match()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha

[PATCH v2 2/7] libnvdimm/bus: Prevent duplicate device_unregister() calls

[Dan Williams]

A multithreaded namespace creation/destruction stress test currently
fails with signatures like the following:

    sysfs group 'power' not found for kobject 'dax1.1'
    RIP: 0010:sysfs_remove_group+0x76/0x80
    Call Trace:
     device_del+0x73/0x370
     device_unregister+0x16/0x50
     nd_async_device_unregister+0x1e/0x30 [libnvdimm]
     async_run_entry_fn+0x39/0x160
     process_one_work+0x23c/0x5e0
     worker_thread+0x3c/0x390

    BUG: kernel NULL pointer dereference, address: 0000000000000020
    RIP: 0010:klist_put+0x1b/0x6c
    Call Trace:
     klist_del+0xe/0x10
     device_del+0x8a/0x2c9
     ? __switch_to_asm+0x34/0x70
     ? __switch_to_asm+0x40/0x70
     device_unregister+0x44/0x4f
     nd_async_device_unregister+0x22/0x2d [libnvdimm]
     async_run_entry_fn+0x47/0x15a
     process_one_work+0x1a2/0x2eb
     worker_thread+0x1b8/0x26e

Use the kill_device() helper to atomically resolve the race of multiple
threads issuing kill, device_unregister(), requests.

Reported-by: Jane Chu <jane.chu@oracle.com>
Reported-by: Erwin Tsaur <erwin.tsaur@oracle.com>
Fixes: 4d88a97aa9e8 ("libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver...")
Cc: <stable@vger.kernel.org>
Link: https://github.com/pmem/ndctl/issues/96
Tested-by: Tested-by: Jane Chu <jane.chu@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 2dca3034fee0..42713b210f51 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -547,13 +547,38 @@ EXPORT_SYMBOL(nd_device_register);
 
 void nd_device_unregister(struct device *dev, enum nd_async_mode mode)
 {
+	bool killed;
+
 	switch (mode) {
 	case ND_ASYNC:
+		/*
+		 * In the async case this is being triggered with the
+		 * device lock held and the unregistration work needs to
+		 * be moved out of line iff this is thread has won the
+		 * race to schedule the deletion.
+		 */
+		if (!kill_device(dev))
+			return;
+
 		get_device(dev);
 		async_schedule_domain(nd_async_device_unregister, dev,
 				&nd_async_domain);
 		break;
 	case ND_SYNC:
+		/*
+		 * In the sync case the device is being unregistered due
+		 * to a state change of the parent. Claim the kill state
+		 * to synchronize against other unregistration requests,
+		 * or otherwise let the async path handle it if the
+		 * unregistration was already queued.
+		 */
+		device_lock(dev);
+		killed = kill_device(dev);
+		device_unlock(dev);
+
+		if (!killed)
+			return;
+
 		nd_synchronize();
 		device_unregister(dev);
 		break;

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

[PATCH v2 3/7] libnvdimm/region: Register badblocks before namespaces

[Dan Williams]

Namespace activation expects to be able to reference region badblocks.
The following warning sometimes triggers when asynchronous namespace
activation races in front of the completion of namespace probing. Move
all possible namespace probing after region badblocks initialization.

Otherwise, lockdep sometimes catches the uninitialized state of the
badblocks seqlock with stack trace signatures like:

    INFO: trying to register non-static key.
    pmem2: detected capacity change from 0 to 136365211648
    the code is fine but needs lockdep annotation.
    turning off the locking correctness validator.
    CPU: 9 PID: 358 Comm: kworker/u80:5 Tainted: G           OE     5.2.0-rc4+ #3382
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
    Workqueue: events_unbound async_run_entry_fn
    Call Trace:
     dump_stack+0x85/0xc0
    pmem1.12: detected capacity change from 0 to 8589934592
     register_lock_class+0x56a/0x570
     ? check_object+0x140/0x270
     __lock_acquire+0x80/0x1710
     ? __mutex_lock+0x39d/0x910
     lock_acquire+0x9e/0x180
     ? nd_pfn_validate+0x28f/0x440 [libnvdimm]
     badblocks_check+0x93/0x1f0
     ? nd_pfn_validate+0x28f/0x440 [libnvdimm]
     nd_pfn_validate+0x28f/0x440 [libnvdimm]
     ? lockdep_hardirqs_on+0xf0/0x180
     nd_dax_probe+0x9a/0x120 [libnvdimm]
     nd_pmem_probe+0x6d/0x180 [nd_pmem]
     nvdimm_bus_probe+0x90/0x2c0 [libnvdimm]

Fixes: 48af2f7e52f4 ("libnvdimm, pfn: during init, clear errors...")
Cc: <stable@vger.kernel.org>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/region.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index ef46cc3a71ae..488c47ac4c4a 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -34,17 +34,6 @@ static int nd_region_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	rc = nd_region_register_namespaces(nd_region, &err);
-	if (rc < 0)
-		return rc;
-
-	ndrd = dev_get_drvdata(dev);
-	ndrd->ns_active = rc;
-	ndrd->ns_count = rc + err;
-
-	if (rc && err && rc == err)
-		return -ENODEV;
-
 	if (is_nd_pmem(&nd_region->dev)) {
 		struct resource ndr_res;
 
@@ -60,6 +49,17 @@ static int nd_region_probe(struct device *dev)
 		nvdimm_badblocks_populate(nd_region, &nd_region->bb, &ndr_res);
 	}
 
+	rc = nd_region_register_namespaces(nd_region, &err);
+	if (rc < 0)
+		return rc;
+
+	ndrd = dev_get_drvdata(dev);
+	ndrd->ns_active = rc;
+	ndrd->ns_count = rc + err;
+
+	if (rc && err && rc == err)
+		return -ENODEV;
+
 	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);

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

Re: [PATCH v2 3/7] libnvdimm/region: Register badblocks before namespaces

[Verma, Vishal L]

On Wed, 2019-07-17 at 18:08 -0700, Dan Williams wrote:
> Namespace activation expects to be able to reference region badblocks.
> The following warning sometimes triggers when asynchronous namespace
> activation races in front of the completion of namespace probing. Move
> all possible namespace probing after region badblocks initialization.
> 
> Otherwise, lockdep sometimes catches the uninitialized state of the
> badblocks seqlock with stack trace signatures like:
> 
>     INFO: trying to register non-static key.
>     pmem2: detected capacity change from 0 to 136365211648
>     the code is fine but needs lockdep annotation.
>     turning off the locking correctness validator.
>     CPU: 9 PID: 358 Comm: kworker/u80:5 Tainted:
> G           OE     5.2.0-rc4+ #3382
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0
> 02/06/2015
>     Workqueue: events_unbound async_run_entry_fn
>     Call Trace:
>      dump_stack+0x85/0xc0
>     pmem1.12: detected capacity change from 0 to 8589934592
>      register_lock_class+0x56a/0x570
>      ? check_object+0x140/0x270
>      __lock_acquire+0x80/0x1710
>      ? __mutex_lock+0x39d/0x910
>      lock_acquire+0x9e/0x180
>      ? nd_pfn_validate+0x28f/0x440 [libnvdimm]
>      badblocks_check+0x93/0x1f0
>      ? nd_pfn_validate+0x28f/0x440 [libnvdimm]
>      nd_pfn_validate+0x28f/0x440 [libnvdimm]
>      ? lockdep_hardirqs_on+0xf0/0x180
>      nd_dax_probe+0x9a/0x120 [libnvdimm]
>      nd_pmem_probe+0x6d/0x180 [nd_pmem]
>      nvdimm_bus_probe+0x90/0x2c0 [libnvdimm]
> 
> Fixes: 48af2f7e52f4 ("libnvdimm, pfn: during init, clear errors...")
> Cc: <stable@vger.kernel.org>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/nvdimm/region.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

This looks good to me,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index ef46cc3a71ae..488c47ac4c4a 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -34,17 +34,6 @@ static int nd_region_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	rc = nd_region_register_namespaces(nd_region, &err);
> -	if (rc < 0)
> -		return rc;
> -
> -	ndrd = dev_get_drvdata(dev);
> -	ndrd->ns_active = rc;
> -	ndrd->ns_count = rc + err;
> -
> -	if (rc && err && rc == err)
> -		return -ENODEV;
> -
>  	if (is_nd_pmem(&nd_region->dev)) {
>  		struct resource ndr_res;
>  
> @@ -60,6 +49,17 @@ static int nd_region_probe(struct device *dev)
>  		nvdimm_badblocks_populate(nd_region, &nd_region->bb,
> &ndr_res);
>  	}
>  
> +	rc = nd_region_register_namespaces(nd_region, &err);
> +	if (rc < 0)
> +		return rc;
> +
> +	ndrd = dev_get_drvdata(dev);
> +	ndrd->ns_active = rc;
> +	ndrd->ns_count = rc + err;
> +
> +	if (rc && err && rc == err)
> +		return -ENODEV;
> +
>  	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);
> 

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

[PATCH v2 4/7] libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant

[Dan Williams]

In preparation for not holding a lock over the execution of nd_ioctl(),
update the implementation to allow multiple threads to be attempting
ioctls at the same time. The bus lock still prevents multiple in-flight
->ndctl() invocations from corrupting each other's state, but static
global staging buffers are moved to the heap.

Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c |   59 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 42713b210f51..a3180c28fb2b 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -970,20 +970,19 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		int read_only, unsigned int ioctl_cmd, unsigned long arg)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
-	static char out_env[ND_CMD_MAX_ENVELOPE];
-	static char in_env[ND_CMD_MAX_ENVELOPE];
 	const struct nd_cmd_desc *desc = NULL;
 	unsigned int cmd = _IOC_NR(ioctl_cmd);
 	struct device *dev = &nvdimm_bus->dev;
 	void __user *p = (void __user *) arg;
+	char *out_env = NULL, *in_env = NULL;
 	const char *cmd_name, *dimm_name;
 	u32 in_len = 0, out_len = 0;
 	unsigned int func = cmd;
 	unsigned long cmd_mask;
 	struct nd_cmd_pkg pkg;
 	int rc, i, cmd_rc;
+	void *buf = NULL;
 	u64 buf_len = 0;
-	void *buf;
 
 	if (nvdimm) {
 		desc = nd_cmd_dimm_desc(cmd);
@@ -1023,6 +1022,9 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		}
 
 	/* process an input envelope */
+	in_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
+	if (!in_env)
+		return -ENOMEM;
 	for (i = 0; i < desc->in_num; i++) {
 		u32 in_size, copy;
 
@@ -1030,14 +1032,17 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		if (in_size == UINT_MAX) {
 			dev_err(dev, "%s:%s unknown input size cmd: %s field: %d\n",
 					__func__, dimm_name, cmd_name, i);
-			return -ENXIO;
+			rc = -ENXIO;
+			goto out;
 		}
-		if (in_len < sizeof(in_env))
-			copy = min_t(u32, sizeof(in_env) - in_len, in_size);
+		if (in_len < ND_CMD_MAX_ENVELOPE)
+			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - in_len, in_size);
 		else
 			copy = 0;
-		if (copy && copy_from_user(&in_env[in_len], p + in_len, copy))
-			return -EFAULT;
+		if (copy && copy_from_user(&in_env[in_len], p + in_len, copy)) {
+			rc = -EFAULT;
+			goto out;
+		}
 		in_len += in_size;
 	}
 
@@ -1049,6 +1054,12 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	}
 
 	/* process an output envelope */
+	out_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
+	if (!out_env) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
 	for (i = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
 				(u32 *) in_env, (u32 *) out_env, 0);
@@ -1057,15 +1068,18 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		if (out_size == UINT_MAX) {
 			dev_dbg(dev, "%s unknown output size cmd: %s field: %d\n",
 					dimm_name, cmd_name, i);
-			return -EFAULT;
+			rc = -EFAULT;
+			goto out;
 		}
-		if (out_len < sizeof(out_env))
-			copy = min_t(u32, sizeof(out_env) - out_len, out_size);
+		if (out_len < ND_CMD_MAX_ENVELOPE)
+			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - out_len, out_size);
 		else
 			copy = 0;
 		if (copy && copy_from_user(&out_env[out_len],
-					p + in_len + out_len, copy))
-			return -EFAULT;
+					p + in_len + out_len, copy)) {
+			rc = -EFAULT;
+			goto out;
+		}
 		out_len += out_size;
 	}
 
@@ -1073,12 +1087,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
 		dev_dbg(dev, "%s cmd: %s buf_len: %llu > %d\n", dimm_name,
 				cmd_name, buf_len, ND_IOCTL_MAX_BUFLEN);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 
 	buf = vmalloc(buf_len);
-	if (!buf)
-		return -ENOMEM;
+	if (!buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	if (copy_from_user(buf, p, buf_len)) {
 		rc = -EFAULT;
@@ -1100,17 +1117,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		nvdimm_account_cleared_poison(nvdimm_bus, clear_err->address,
 				clear_err->cleared);
 	}
-	nvdimm_bus_unlock(&nvdimm_bus->dev);
 
 	if (copy_to_user(p, buf, buf_len))
 		rc = -EFAULT;
 
-	vfree(buf);
-	return rc;
-
- out_unlock:
+out_unlock:
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
- out:
+out:
+	kfree(in_env);
+	kfree(out_env);
 	vfree(buf);
 	return rc;
 }

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

Re: [PATCH v2 4/7] libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant

[Verma, Vishal L]

On Wed, 2019-07-17 at 18:08 -0700, Dan Williams wrote:
> In preparation for not holding a lock over the execution of
> nd_ioctl(),
> update the implementation to allow multiple threads to be attempting
> ioctls at the same time. The bus lock still prevents multiple in-
> flight
> ->ndctl() invocations from corrupting each other's state, but static
> global staging buffers are moved to the heap.
> 
> Reported-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/nvdimm/bus.c |   59 +++++++++++++++++++++++++++++++--------
> -----------
>  1 file changed, 37 insertions(+), 22 deletions(-)

Ran tens of iterations of the unit tests with this, and couldn't
reproduce the failure.

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Tested-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 42713b210f51..a3180c28fb2b 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -970,20 +970,19 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		int read_only, unsigned int ioctl_cmd, unsigned long
> arg)
>  {
>  	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
> -	static char out_env[ND_CMD_MAX_ENVELOPE];
> -	static char in_env[ND_CMD_MAX_ENVELOPE];
>  	const struct nd_cmd_desc *desc = NULL;
>  	unsigned int cmd = _IOC_NR(ioctl_cmd);
>  	struct device *dev = &nvdimm_bus->dev;
>  	void __user *p = (void __user *) arg;
> +	char *out_env = NULL, *in_env = NULL;
>  	const char *cmd_name, *dimm_name;
>  	u32 in_len = 0, out_len = 0;
>  	unsigned int func = cmd;
>  	unsigned long cmd_mask;
>  	struct nd_cmd_pkg pkg;
>  	int rc, i, cmd_rc;
> +	void *buf = NULL;
>  	u64 buf_len = 0;
> -	void *buf;
>  
>  	if (nvdimm) {
>  		desc = nd_cmd_dimm_desc(cmd);
> @@ -1023,6 +1022,9 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		}
>  
>  	/* process an input envelope */
> +	in_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
> +	if (!in_env)
> +		return -ENOMEM;
>  	for (i = 0; i < desc->in_num; i++) {
>  		u32 in_size, copy;
>  
> @@ -1030,14 +1032,17 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		if (in_size == UINT_MAX) {
>  			dev_err(dev, "%s:%s unknown input size cmd: %s
> field: %d\n",
>  					__func__, dimm_name, cmd_name,
> i);
> -			return -ENXIO;
> +			rc = -ENXIO;
> +			goto out;
>  		}
> -		if (in_len < sizeof(in_env))
> -			copy = min_t(u32, sizeof(in_env) - in_len,
> in_size);
> +		if (in_len < ND_CMD_MAX_ENVELOPE)
> +			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - in_len,
> in_size);
>  		else
>  			copy = 0;
> -		if (copy && copy_from_user(&in_env[in_len], p + in_len,
> copy))
> -			return -EFAULT;
> +		if (copy && copy_from_user(&in_env[in_len], p + in_len,
> copy)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
>  		in_len += in_size;
>  	}
>  
> @@ -1049,6 +1054,12 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  	}
>  
>  	/* process an output envelope */
> +	out_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
> +	if (!out_env) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
>  	for (i = 0; i < desc->out_num; i++) {
>  		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
>  				(u32 *) in_env, (u32 *) out_env, 0);
> @@ -1057,15 +1068,18 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		if (out_size == UINT_MAX) {
>  			dev_dbg(dev, "%s unknown output size cmd: %s
> field: %d\n",
>  					dimm_name, cmd_name, i);
> -			return -EFAULT;
> +			rc = -EFAULT;
> +			goto out;
>  		}
> -		if (out_len < sizeof(out_env))
> -			copy = min_t(u32, sizeof(out_env) - out_len,
> out_size);
> +		if (out_len < ND_CMD_MAX_ENVELOPE)
> +			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - out_len,
> out_size);
>  		else
>  			copy = 0;
>  		if (copy && copy_from_user(&out_env[out_len],
> -					p + in_len + out_len, copy))
> -			return -EFAULT;
> +					p + in_len + out_len, copy)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
>  		out_len += out_size;
>  	}
>  
> @@ -1073,12 +1087,15 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
>  		dev_dbg(dev, "%s cmd: %s buf_len: %llu > %d\n",
> dimm_name,
>  				cmd_name, buf_len, ND_IOCTL_MAX_BUFLEN);
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto out;
>  	}
>  
>  	buf = vmalloc(buf_len);
> -	if (!buf)
> -		return -ENOMEM;
> +	if (!buf) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  
>  	if (copy_from_user(buf, p, buf_len)) {
>  		rc = -EFAULT;
> @@ -1100,17 +1117,15 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		nvdimm_account_cleared_poison(nvdimm_bus, clear_err-
> >address,
>  				clear_err->cleared);
>  	}
> -	nvdimm_bus_unlock(&nvdimm_bus->dev);
>  
>  	if (copy_to_user(p, buf, buf_len))
>  		rc = -EFAULT;
>  
> -	vfree(buf);
> -	return rc;
> -
> - out_unlock:
> +out_unlock:
>  	nvdimm_bus_unlock(&nvdimm_bus->dev);
> - out:
> +out:
> +	kfree(in_env);
> +	kfree(out_env);
>  	vfree(buf);
>  	return rc;
>  }
> 

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

[PATCH v2 5/7] libnvdimm/bus: Stop holding nvdimm_bus_list_mutex over __nd_ioctl()

[Dan Williams]

In preparation for fixing a deadlock between wait_for_bus_probe_idle()
and the nvdimm_bus_list_mutex arrange for __nd_ioctl() without
nvdimm_bus_list_mutex held. This also unifies the 'dimm' and 'bus' level
ioctls into a common nd_ioctl() preamble implementation.

Marked for -stable as it is a pre-requisite for a follow-on fix.

Cc: <stable@vger.kernel.org>
Fixes: bf9bccc14c05 ("libnvdimm: pmem label sets and namespace instantiation")
Cc: Vishal Verma <vishal.l.verma@intel.com>
Tested-by: Jane Chu <jane.chu@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c     |   96 ++++++++++++++++++++++++++++------------------
 drivers/nvdimm/nd-core.h |    3 +
 2 files changed, 60 insertions(+), 39 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index a3180c28fb2b..a38572bf486b 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -73,7 +73,7 @@ static void nvdimm_bus_probe_end(struct nvdimm_bus *nvdimm_bus)
 {
 	nvdimm_bus_lock(&nvdimm_bus->dev);
 	if (--nvdimm_bus->probe_active == 0)
-		wake_up(&nvdimm_bus->probe_wait);
+		wake_up(&nvdimm_bus->wait);
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
 }
 
@@ -341,7 +341,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 		return NULL;
 	INIT_LIST_HEAD(&nvdimm_bus->list);
 	INIT_LIST_HEAD(&nvdimm_bus->mapping_list);
-	init_waitqueue_head(&nvdimm_bus->probe_wait);
+	init_waitqueue_head(&nvdimm_bus->wait);
 	nvdimm_bus->id = ida_simple_get(&nd_ida, 0, 0, GFP_KERNEL);
 	if (nvdimm_bus->id < 0) {
 		kfree(nvdimm_bus);
@@ -426,6 +426,9 @@ static int nd_bus_remove(struct device *dev)
 	list_del_init(&nvdimm_bus->list);
 	mutex_unlock(&nvdimm_bus_list_mutex);
 
+	wait_event(nvdimm_bus->wait,
+			atomic_read(&nvdimm_bus->ioctl_active) == 0);
+
 	nd_synchronize();
 	device_for_each_child(&nvdimm_bus->dev, NULL, child_unregister);
 
@@ -885,7 +888,7 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
 		if (nvdimm_bus->probe_active == 0)
 			break;
 		nvdimm_bus_unlock(&nvdimm_bus->dev);
-		wait_event(nvdimm_bus->probe_wait,
+		wait_event(nvdimm_bus->wait,
 				nvdimm_bus->probe_active == 0);
 		nvdimm_bus_lock(&nvdimm_bus->dev);
 	} while (true);
@@ -1130,24 +1133,10 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	return rc;
 }
 
-static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	long id = (long) file->private_data;
-	int rc = -ENXIO, ro;
-	struct nvdimm_bus *nvdimm_bus;
-
-	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
-	mutex_lock(&nvdimm_bus_list_mutex);
-	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
-		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
-			break;
-		}
-	}
-	mutex_unlock(&nvdimm_bus_list_mutex);
-
-	return rc;
-}
+enum nd_ioctl_mode {
+	BUS_IOCTL,
+	DIMM_IOCTL,
+};
 
 static int match_dimm(struct device *dev, void *data)
 {
@@ -1162,31 +1151,62 @@ static int match_dimm(struct device *dev, void *data)
 	return 0;
 }
 
-static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
+		enum nd_ioctl_mode mode)
+
 {
-	int rc = -ENXIO, ro;
-	struct nvdimm_bus *nvdimm_bus;
+	struct nvdimm_bus *nvdimm_bus, *found = NULL;
+	long id = (long) file->private_data;
+	struct nvdimm *nvdimm = NULL;
+	int rc, ro;
 
 	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
-		struct device *dev = device_find_child(&nvdimm_bus->dev,
-				file->private_data, match_dimm);
-		struct nvdimm *nvdimm;
-
-		if (!dev)
-			continue;
+		if (mode == DIMM_IOCTL) {
+			struct device *dev;
+
+			dev = device_find_child(&nvdimm_bus->dev,
+					file->private_data, match_dimm);
+			if (!dev)
+				continue;
+			nvdimm = to_nvdimm(dev);
+			found = nvdimm_bus;
+		} else if (nvdimm_bus->id == id) {
+			found = nvdimm_bus;
+		}
 
-		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
-		put_device(dev);
-		break;
+		if (found) {
+			atomic_inc(&nvdimm_bus->ioctl_active);
+			break;
+		}
 	}
 	mutex_unlock(&nvdimm_bus_list_mutex);
 
+	if (!found)
+		return -ENXIO;
+
+	nvdimm_bus = found;
+	rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
+
+	if (nvdimm)
+		put_device(&nvdimm->dev);
+	if (atomic_dec_and_test(&nvdimm_bus->ioctl_active))
+		wake_up(&nvdimm_bus->wait);
+
 	return rc;
 }
 
+static long bus_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return nd_ioctl(file, cmd, arg, BUS_IOCTL);
+}
+
+static long dimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return nd_ioctl(file, cmd, arg, DIMM_IOCTL);
+}
+
 static int nd_open(struct inode *inode, struct file *file)
 {
 	long minor = iminor(inode);
@@ -1198,16 +1218,16 @@ static int nd_open(struct inode *inode, struct file *file)
 static const struct file_operations nvdimm_bus_fops = {
 	.owner = THIS_MODULE,
 	.open = nd_open,
-	.unlocked_ioctl = nd_ioctl,
-	.compat_ioctl = nd_ioctl,
+	.unlocked_ioctl = bus_ioctl,
+	.compat_ioctl = bus_ioctl,
 	.llseek = noop_llseek,
 };
 
 static const struct file_operations nvdimm_fops = {
 	.owner = THIS_MODULE,
 	.open = nd_open,
-	.unlocked_ioctl = nvdimm_ioctl,
-	.compat_ioctl = nvdimm_ioctl,
+	.unlocked_ioctl = dimm_ioctl,
+	.compat_ioctl = dimm_ioctl,
 	.llseek = noop_llseek,
 };
 
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 391e88de3a29..6cd470547106 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -17,10 +17,11 @@ extern struct workqueue_struct *nvdimm_wq;
 
 struct nvdimm_bus {
 	struct nvdimm_bus_descriptor *nd_desc;
-	wait_queue_head_t probe_wait;
+	wait_queue_head_t wait;
 	struct list_head list;
 	struct device dev;
 	int id, probe_active;
+	atomic_t ioctl_active;
 	struct list_head mapping_list;
 	struct mutex reconfig_mutex;
 	struct badrange badrange;

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

[PATCH v2 6/7] libnvdimm/bus: Fix wait_nvdimm_bus_probe_idle() ABBA deadlock

[Dan Williams]

A multithreaded namespace creation/destruction stress test currently
deadlocks with the following lockup signature:

    INFO: task ndctl:2924 blocked for more than 122 seconds.
          Tainted: G           OE     5.2.0-rc4+ #3382
    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
    ndctl           D    0  2924   1176 0x00000000
    Call Trace:
     ? __schedule+0x27e/0x780
     schedule+0x30/0xb0
     wait_nvdimm_bus_probe_idle+0x8a/0xd0 [libnvdimm]
     ? finish_wait+0x80/0x80
     uuid_store+0xe6/0x2e0 [libnvdimm]
     kernfs_fop_write+0xf0/0x1a0
     vfs_write+0xb7/0x1b0
     ksys_write+0x5c/0xd0
     do_syscall_64+0x60/0x240

     INFO: task ndctl:2923 blocked for more than 122 seconds.
           Tainted: G           OE     5.2.0-rc4+ #3382
     "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
     ndctl           D    0  2923   1175 0x00000000
     Call Trace:
      ? __schedule+0x27e/0x780
      ? __mutex_lock+0x489/0x910
      schedule+0x30/0xb0
      schedule_preempt_disabled+0x11/0x20
      __mutex_lock+0x48e/0x910
      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
      ? __lock_acquire+0x23f/0x1710
      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
      nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
      __dax_pmem_probe+0x5e/0x210 [dax_pmem_core]
      ? nvdimm_bus_probe+0x1d0/0x2c0 [libnvdimm]
      dax_pmem_probe+0xc/0x20 [dax_pmem]
      nvdimm_bus_probe+0x90/0x2c0 [libnvdimm]
      really_probe+0xef/0x390
      driver_probe_device+0xb4/0x100

In this sequence an 'nd_dax' device is being probed and trying to take
the lock on its backing namespace to validate that the 'nd_dax' device
indeed has exclusive access to the backing namespace. Meanwhile, another
thread is trying to update the uuid property of that same backing
namespace. So one thread is in the probe path trying to acquire the
lock, and the other thread has acquired the lock and tries to flush the
probe path.

Fix this deadlock by not holding the namespace device_lock over the
wait_nvdimm_bus_probe_idle() synchronization step. In turn this requires
the device_lock to be held on entry to wait_nvdimm_bus_probe_idle() and
subsequently dropped internally to wait_nvdimm_bus_probe_idle().

Cc: <stable@vger.kernel.org>
Fixes: bf9bccc14c05 ("libnvdimm: pmem label sets and namespace instantiation")
Cc: Vishal Verma <vishal.l.verma@intel.com>
Tested-by: Jane Chu <jane.chu@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c         |   14 +++++++++-----
 drivers/nvdimm/region_devs.c |    4 ++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index a38572bf486b..df41f3571dc9 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -887,10 +887,12 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
 	do {
 		if (nvdimm_bus->probe_active == 0)
 			break;
-		nvdimm_bus_unlock(&nvdimm_bus->dev);
+		nvdimm_bus_unlock(dev);
+		device_unlock(dev);
 		wait_event(nvdimm_bus->wait,
 				nvdimm_bus->probe_active == 0);
-		nvdimm_bus_lock(&nvdimm_bus->dev);
+		device_lock(dev);
+		nvdimm_bus_lock(dev);
 	} while (true);
 }
 
@@ -1016,7 +1018,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		case ND_CMD_ARS_START:
 		case ND_CMD_CLEAR_ERROR:
 		case ND_CMD_CALL:
-			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
+			dev_dbg(dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
 			return -EPERM;
@@ -1105,7 +1107,8 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		goto out;
 	}
 
-	nvdimm_bus_lock(&nvdimm_bus->dev);
+	device_lock(dev);
+	nvdimm_bus_lock(dev);
 	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
 	if (rc)
 		goto out_unlock;
@@ -1125,7 +1128,8 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		rc = -EFAULT;
 
 out_unlock:
-	nvdimm_bus_unlock(&nvdimm_bus->dev);
+	nvdimm_bus_unlock(dev);
+	device_unlock(dev);
 out:
 	kfree(in_env);
 	kfree(out_env);
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 4fed9ce9c2fe..a15276cdec7d 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -422,10 +422,12 @@ static ssize_t available_size_show(struct device *dev,
 	 * memory nvdimm_bus_lock() is dropped, but that's userspace's
 	 * problem to not race itself.
 	 */
+	device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	available = nd_region_available_dpa(nd_region);
 	nvdimm_bus_unlock(dev);
+	device_unlock(dev);
 
 	return sprintf(buf, "%llu\n", available);
 }
@@ -437,10 +439,12 @@ static ssize_t max_available_extent_show(struct device *dev,
 	struct nd_region *nd_region = to_nd_region(dev);
 	unsigned long long available = 0;
 
+	device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	available = nd_region_allocatable_dpa(nd_region);
 	nvdimm_bus_unlock(dev);
+	device_unlock(dev);
 
 	return sprintf(buf, "%llu\n", available);
 }

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

Re: [PATCH v2 6/7] libnvdimm/bus: Fix wait_nvdimm_bus_probe_idle() ABBA deadlock

[Sasha Levin]

On Wed, Jul 17, 2019 at 06:08:21PM -0700, Dan Williams wrote:
>A multithreaded namespace creation/destruction stress test currently
>deadlocks with the following lockup signature:
>
>    INFO: task ndctl:2924 blocked for more than 122 seconds.
>          Tainted: G           OE     5.2.0-rc4+ #3382
>    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>    ndctl           D    0  2924   1176 0x00000000
>    Call Trace:
>     ? __schedule+0x27e/0x780
>     schedule+0x30/0xb0
>     wait_nvdimm_bus_probe_idle+0x8a/0xd0 [libnvdimm]
>     ? finish_wait+0x80/0x80
>     uuid_store+0xe6/0x2e0 [libnvdimm]
>     kernfs_fop_write+0xf0/0x1a0
>     vfs_write+0xb7/0x1b0
>     ksys_write+0x5c/0xd0
>     do_syscall_64+0x60/0x240
>
>     INFO: task ndctl:2923 blocked for more than 122 seconds.
>           Tainted: G           OE     5.2.0-rc4+ #3382
>     "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>     ndctl           D    0  2923   1175 0x00000000
>     Call Trace:
>      ? __schedule+0x27e/0x780
>      ? __mutex_lock+0x489/0x910
>      schedule+0x30/0xb0
>      schedule_preempt_disabled+0x11/0x20
>      __mutex_lock+0x48e/0x910
>      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
>      ? __lock_acquire+0x23f/0x1710
>      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
>      nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
>      __dax_pmem_probe+0x5e/0x210 [dax_pmem_core]
>      ? nvdimm_bus_probe+0x1d0/0x2c0 [libnvdimm]
>      dax_pmem_probe+0xc/0x20 [dax_pmem]
>      nvdimm_bus_probe+0x90/0x2c0 [libnvdimm]
>      really_probe+0xef/0x390
>      driver_probe_device+0xb4/0x100
>
>In this sequence an 'nd_dax' device is being probed and trying to take
>the lock on its backing namespace to validate that the 'nd_dax' device
>indeed has exclusive access to the backing namespace. Meanwhile, another
>thread is trying to update the uuid property of that same backing
>namespace. So one thread is in the probe path trying to acquire the
>lock, and the other thread has acquired the lock and tries to flush the
>probe path.
>
>Fix this deadlock by not holding the namespace device_lock over the
>wait_nvdimm_bus_probe_idle() synchronization step. In turn this requires
>the device_lock to be held on entry to wait_nvdimm_bus_probe_idle() and
>subsequently dropped internally to wait_nvdimm_bus_probe_idle().
>
>Cc: <stable@vger.kernel.org>
>Fixes: bf9bccc14c05 ("libnvdimm: pmem label sets and namespace instantiation")
>Cc: Vishal Verma <vishal.l.verma@intel.com>
>Tested-by: Jane Chu <jane.chu@oracle.com>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

The way these patches are split, when we take them to stable this patch
won't apply because it wants "libnvdimm/bus: Prepare the nd_ioctl() path
to be re-entrant".

If you were to send another iteration of this patchset, could you please
re-order the patches so they will apply cleanly to stable? this will
help with reducing mail exchanges later on and possibly a mis-merge into
stable.

If not, this should serve as a reference for future us to double check
the backport.

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

Re: [PATCH v2 6/7] libnvdimm/bus: Fix wait_nvdimm_bus_probe_idle() ABBA deadlock

[Dan Williams]

On Wed, Jul 17, 2019 at 7:05 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Wed, Jul 17, 2019 at 06:08:21PM -0700, Dan Williams wrote:
> >A multithreaded namespace creation/destruction stress test currently
> >deadlocks with the following lockup signature:
> >
> >    INFO: task ndctl:2924 blocked for more than 122 seconds.
> >          Tainted: G           OE     5.2.0-rc4+ #3382
> >    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> >    ndctl           D    0  2924   1176 0x00000000
> >    Call Trace:
> >     ? __schedule+0x27e/0x780
> >     schedule+0x30/0xb0
> >     wait_nvdimm_bus_probe_idle+0x8a/0xd0 [libnvdimm]
> >     ? finish_wait+0x80/0x80
> >     uuid_store+0xe6/0x2e0 [libnvdimm]
> >     kernfs_fop_write+0xf0/0x1a0
> >     vfs_write+0xb7/0x1b0
> >     ksys_write+0x5c/0xd0
> >     do_syscall_64+0x60/0x240
> >
> >     INFO: task ndctl:2923 blocked for more than 122 seconds.
> >           Tainted: G           OE     5.2.0-rc4+ #3382
> >     "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> >     ndctl           D    0  2923   1175 0x00000000
> >     Call Trace:
> >      ? __schedule+0x27e/0x780
> >      ? __mutex_lock+0x489/0x910
> >      schedule+0x30/0xb0
> >      schedule_preempt_disabled+0x11/0x20
> >      __mutex_lock+0x48e/0x910
> >      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
> >      ? __lock_acquire+0x23f/0x1710
> >      ? nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
> >      nvdimm_namespace_common_probe+0x95/0x4d0 [libnvdimm]
> >      __dax_pmem_probe+0x5e/0x210 [dax_pmem_core]
> >      ? nvdimm_bus_probe+0x1d0/0x2c0 [libnvdimm]
> >      dax_pmem_probe+0xc/0x20 [dax_pmem]
> >      nvdimm_bus_probe+0x90/0x2c0 [libnvdimm]
> >      really_probe+0xef/0x390
> >      driver_probe_device+0xb4/0x100
> >
> >In this sequence an 'nd_dax' device is being probed and trying to take
> >the lock on its backing namespace to validate that the 'nd_dax' device
> >indeed has exclusive access to the backing namespace. Meanwhile, another
> >thread is trying to update the uuid property of that same backing
> >namespace. So one thread is in the probe path trying to acquire the
> >lock, and the other thread has acquired the lock and tries to flush the
> >probe path.
> >
> >Fix this deadlock by not holding the namespace device_lock over the
> >wait_nvdimm_bus_probe_idle() synchronization step. In turn this requires
> >the device_lock to be held on entry to wait_nvdimm_bus_probe_idle() and
> >subsequently dropped internally to wait_nvdimm_bus_probe_idle().
> >
> >Cc: <stable@vger.kernel.org>
> >Fixes: bf9bccc14c05 ("libnvdimm: pmem label sets and namespace instantiation")
> >Cc: Vishal Verma <vishal.l.verma@intel.com>
> >Tested-by: Jane Chu <jane.chu@oracle.com>
> >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Hi Dan,
>
> The way these patches are split, when we take them to stable this patch
> won't apply because it wants "libnvdimm/bus: Prepare the nd_ioctl() path
> to be re-entrant".
>
> If you were to send another iteration of this patchset, could you please
> re-order the patches so they will apply cleanly to stable? this will
> help with reducing mail exchanges later on and possibly a mis-merge into
> stable.
>
> If not, this should serve as a reference for future us to double check
> the backport.

Oh we should backport all of them. I'll tag that one for -stable as
well. It's a hard pre-requisite for the fix.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 7/7] driver-core, libnvdimm: Let device subsystems add local lockdep coverage

[Dan Williams]

For good reason, the standard device_lock() is marked
lockdep_set_novalidate_class() because there is simply no sane way to
describe the myriad ways the device_lock() ordered with other locks.
However, that leaves subsystems that know their own local device_lock()
ordering rules to find lock ordering mistakes manually. Instead,
introduce an optional / additional lockdep-enabled lock that a subsystem
can acquire in all the same paths that the device_lock() is acquired.

A conversion of the NFIT driver and NVDIMM subsystem to a
lockdep-validate device_lock() scheme is included. The
debug_nvdimm_lock() implementation implements the correct lock-class and
stacking order for the libnvdimm device topology hierarchy.

Yes, this is a hack, but hopefully it is a useful hack for other
subsystems device_lock() debug sessions. Quoting Greg:

    "Yeah, it feels a bit hacky but it's really up to a subsystem to mess up
     using it as much as anything else, so user beware :)

     I don't object to it if it makes things easier for you to debug."

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c        |   28 ++++++++--------
 drivers/acpi/nfit/nfit.h        |   24 ++++++++++++++
 drivers/base/core.c             |    3 ++
 drivers/nvdimm/btt_devs.c       |   16 +++++----
 drivers/nvdimm/bus.c            |   28 ++++++++++------
 drivers/nvdimm/core.c           |   10 +++---
 drivers/nvdimm/dimm_devs.c      |    4 +-
 drivers/nvdimm/namespace_devs.c |   36 ++++++++++-----------
 drivers/nvdimm/nd-core.h        |   68 +++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/pfn_devs.c       |   24 +++++++-------
 drivers/nvdimm/pmem.c           |    4 +-
 drivers/nvdimm/region.c         |    2 +
 drivers/nvdimm/region_devs.c    |   16 +++++----
 include/linux/device.h          |    5 +++
 14 files changed, 187 insertions(+), 81 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 23022cf20d26..f22139458ce1 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1282,7 +1282,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
 	if (rc)
 		return rc;
 
-	device_lock(dev);
+	nfit_device_lock(dev);
 	nd_desc = dev_get_drvdata(dev);
 	if (nd_desc) {
 		struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
@@ -1299,7 +1299,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
 			break;
 		}
 	}
-	device_unlock(dev);
+	nfit_device_unlock(dev);
 	if (rc)
 		return rc;
 	return size;
@@ -1319,7 +1319,7 @@ static ssize_t scrub_show(struct device *dev,
 	ssize_t rc = -ENXIO;
 	bool busy;
 
-	device_lock(dev);
+	nfit_device_lock(dev);
 	nd_desc = dev_get_drvdata(dev);
 	if (!nd_desc) {
 		device_unlock(dev);
@@ -1339,7 +1339,7 @@ static ssize_t scrub_show(struct device *dev,
 	}
 
 	mutex_unlock(&acpi_desc->init_mutex);
-	device_unlock(dev);
+	nfit_device_unlock(dev);
 	return rc;
 }
 
@@ -1356,14 +1356,14 @@ static ssize_t scrub_store(struct device *dev,
 	if (val != 1)
 		return -EINVAL;
 
-	device_lock(dev);
+	nfit_device_lock(dev);
 	nd_desc = dev_get_drvdata(dev);
 	if (nd_desc) {
 		struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
 
 		rc = acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_LONG);
 	}
-	device_unlock(dev);
+	nfit_device_unlock(dev);
 	if (rc)
 		return rc;
 	return size;
@@ -1749,9 +1749,9 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
 	struct acpi_device *adev = data;
 	struct device *dev = &adev->dev;
 
-	device_lock(dev->parent);
+	nfit_device_lock(dev->parent);
 	__acpi_nvdimm_notify(dev, event);
-	device_unlock(dev->parent);
+	nfit_device_unlock(dev->parent);
 }
 
 static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
@@ -3457,8 +3457,8 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
 	struct device *dev = acpi_desc->dev;
 
 	/* Bounce the device lock to flush acpi_nfit_add / acpi_nfit_notify */
-	device_lock(dev);
-	device_unlock(dev);
+	nfit_device_lock(dev);
+	nfit_device_unlock(dev);
 
 	/* Bounce the init_mutex to complete initial registration */
 	mutex_lock(&acpi_desc->init_mutex);
@@ -3602,8 +3602,8 @@ void acpi_nfit_shutdown(void *data)
 	 * acpi_nfit_ars_rescan() submissions have had a chance to
 	 * either submit or see ->cancel set.
 	 */
-	device_lock(bus_dev);
-	device_unlock(bus_dev);
+	nfit_device_lock(bus_dev);
+	nfit_device_unlock(bus_dev);
 
 	flush_workqueue(nfit_wq);
 }
@@ -3746,9 +3746,9 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
 
 static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 {
-	device_lock(&adev->dev);
+	nfit_device_lock(&adev->dev);
 	__acpi_nfit_notify(&adev->dev, adev->handle, event);
-	device_unlock(&adev->dev);
+	nfit_device_unlock(&adev->dev);
 }
 
 static const struct acpi_device_id acpi_nfit_ids[] = {
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 6ee2b02af73e..24241941181c 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -312,6 +312,30 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
 	return container_of(nd_desc, struct acpi_nfit_desc, nd_desc);
 }
 
+#ifdef CONFIG_PROVE_LOCKING
+static inline void nfit_device_lock(struct device *dev)
+{
+	device_lock(dev);
+	mutex_lock(&dev->lockdep_mutex);
+}
+
+static inline void nfit_device_unlock(struct device *dev)
+{
+	mutex_unlock(&dev->lockdep_mutex);
+	device_unlock(dev);
+}
+#else
+static inline void nfit_device_lock(struct device *dev)
+{
+	device_lock(dev);
+}
+
+static inline void nfit_device_unlock(struct device *dev)
+{
+	device_unlock(dev);
+}
+#endif
+
 const guid_t *to_nfit_uuid(enum nfit_uuids id);
 int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz);
 void acpi_nfit_shutdown(void *data);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index eaf3aa0cb803..4825949d6547 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1663,6 +1663,9 @@ void device_initialize(struct device *dev)
 	kobject_init(&dev->kobj, &device_ktype);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
+#ifdef CONFIG_PROVE_LOCKING
+	mutex_init(&dev->lockdep_mutex);
+#endif
 	lockdep_set_novalidate_class(&dev->mutex);
 	spin_lock_init(&dev->devres_lock);
 	INIT_LIST_HEAD(&dev->devres_head);
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 62d00fffa4af..3508a79110c7 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -62,14 +62,14 @@ static ssize_t sector_size_store(struct device *dev,
 	struct nd_btt *nd_btt = to_nd_btt(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	rc = nd_size_select_store(dev, buf, &nd_btt->lbasize,
 			btt_lbasize_supported);
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -91,11 +91,11 @@ static ssize_t uuid_store(struct device *dev,
 	struct nd_btt *nd_btt = to_nd_btt(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	rc = nd_uuid_store(dev, &nd_btt->uuid, buf, len);
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -120,13 +120,13 @@ static ssize_t namespace_store(struct device *dev,
 	struct nd_btt *nd_btt = to_nd_btt(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	rc = nd_namespace_store(dev, &nd_btt->ndns, buf, len);
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -138,14 +138,14 @@ static ssize_t size_show(struct device *dev,
 	struct nd_btt *nd_btt = to_nd_btt(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (dev->driver)
 		rc = sprintf(buf, "%llu\n", nd_btt->size);
 	else {
 		/* no size to convey if the btt instance is disabled */
 		rc = -ENXIO;
 	}
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index df41f3571dc9..798c5c4aea9c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -26,7 +26,7 @@
 
 int nvdimm_major;
 static int nvdimm_bus_major;
-static struct class *nd_class;
+struct class *nd_class;
 static DEFINE_IDA(nd_ida);
 
 static int to_nd_device_type(struct device *dev)
@@ -91,7 +91,10 @@ static int nvdimm_bus_probe(struct device *dev)
 			dev->driver->name, dev_name(dev));
 
 	nvdimm_bus_probe_start(nvdimm_bus);
+	debug_nvdimm_lock(dev);
 	rc = nd_drv->probe(dev);
+	debug_nvdimm_unlock(dev);
+
 	if (rc == 0)
 		nd_region_probe_success(nvdimm_bus, dev);
 	else
@@ -113,8 +116,11 @@ static int nvdimm_bus_remove(struct device *dev)
 	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 	int rc = 0;
 
-	if (nd_drv->remove)
+	if (nd_drv->remove) {
+		debug_nvdimm_lock(dev);
 		rc = nd_drv->remove(dev);
+		debug_nvdimm_unlock(dev);
+	}
 	nd_region_disable(nvdimm_bus, dev);
 
 	dev_dbg(&nvdimm_bus->dev, "%s.remove(%s) = %d\n", dev->driver->name,
@@ -140,7 +146,7 @@ static void nvdimm_bus_shutdown(struct device *dev)
 
 void nd_device_notify(struct device *dev, enum nvdimm_event event)
 {
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (dev->driver) {
 		struct nd_device_driver *nd_drv;
 
@@ -148,7 +154,7 @@ void nd_device_notify(struct device *dev, enum nvdimm_event event)
 		if (nd_drv->notify)
 			nd_drv->notify(dev, event);
 	}
-	device_unlock(dev);
+	nd_device_unlock(dev);
 }
 EXPORT_SYMBOL(nd_device_notify);
 
@@ -296,7 +302,7 @@ static void nvdimm_bus_release(struct device *dev)
 	kfree(nvdimm_bus);
 }
 
-static bool is_nvdimm_bus(struct device *dev)
+bool is_nvdimm_bus(struct device *dev)
 {
 	return dev->release == nvdimm_bus_release;
 }
@@ -575,9 +581,9 @@ void nd_device_unregister(struct device *dev, enum nd_async_mode mode)
 		 * or otherwise let the async path handle it if the
 		 * unregistration was already queued.
 		 */
-		device_lock(dev);
+		nd_device_lock(dev);
 		killed = kill_device(dev);
-		device_unlock(dev);
+		nd_device_unlock(dev);
 
 		if (!killed)
 			return;
@@ -888,10 +894,10 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
 		if (nvdimm_bus->probe_active == 0)
 			break;
 		nvdimm_bus_unlock(dev);
-		device_unlock(dev);
+		nd_device_unlock(dev);
 		wait_event(nvdimm_bus->wait,
 				nvdimm_bus->probe_active == 0);
-		device_lock(dev);
+		nd_device_lock(dev);
 		nvdimm_bus_lock(dev);
 	} while (true);
 }
@@ -1107,7 +1113,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		goto out;
 	}
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
 	if (rc)
@@ -1129,7 +1135,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 
 out_unlock:
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 out:
 	kfree(in_env);
 	kfree(out_env);
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 5e1f060547bf..9204f1e9fd14 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -246,7 +246,7 @@ static int nd_uuid_parse(struct device *dev, u8 *uuid_out, const char *buf,
  *
  * Enforce that uuids can only be changed while the device is disabled
  * (driver detached)
- * LOCKING: expects device_lock() is held on entry
+ * LOCKING: expects nd_device_lock() is held on entry
  */
 int nd_uuid_store(struct device *dev, u8 **uuid_out, const char *buf,
 		size_t len)
@@ -347,15 +347,15 @@ static DEVICE_ATTR_RO(provider);
 
 static int flush_namespaces(struct device *dev, void *data)
 {
-	device_lock(dev);
-	device_unlock(dev);
+	nd_device_lock(dev);
+	nd_device_unlock(dev);
 	return 0;
 }
 
 static int flush_regions_dimms(struct device *dev, void *data)
 {
-	device_lock(dev);
-	device_unlock(dev);
+	nd_device_lock(dev);
+	nd_device_unlock(dev);
 	device_for_each_child(dev, NULL, flush_namespaces);
 	return 0;
 }
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index dfecd6e17043..29a065e769ea 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -484,12 +484,12 @@ static ssize_t security_store(struct device *dev,
 	 * done while probing is idle and the DIMM is not in active use
 	 * in any region.
 	 */
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	rc = __security_store(dev, buf, len);
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index a434a5964cb9..92cd809d7e43 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -410,7 +410,7 @@ static ssize_t alt_name_store(struct device *dev,
 	struct nd_region *nd_region = to_nd_region(dev->parent);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	rc = __alt_name_store(dev, buf, len);
@@ -418,7 +418,7 @@ static ssize_t alt_name_store(struct device *dev,
 		rc = nd_namespace_label_update(nd_region, dev);
 	dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc < 0 ? rc : len;
 }
@@ -1077,7 +1077,7 @@ static ssize_t size_store(struct device *dev,
 	if (rc)
 		return rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	rc = __size_store(dev, val);
@@ -1103,7 +1103,7 @@ static ssize_t size_store(struct device *dev,
 	dev_dbg(dev, "%llx %s (%d)\n", val, rc < 0 ? "fail" : "success", rc);
 
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc < 0 ? rc : len;
 }
@@ -1286,7 +1286,7 @@ static ssize_t uuid_store(struct device *dev,
 	} else
 		return -ENXIO;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	if (to_ndns(dev)->claim)
@@ -1302,7 +1302,7 @@ static ssize_t uuid_store(struct device *dev,
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc < 0 ? rc : len;
 }
@@ -1376,7 +1376,7 @@ static ssize_t sector_size_store(struct device *dev,
 	} else
 		return -ENXIO;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	if (to_ndns(dev)->claim)
 		rc = -EBUSY;
@@ -1387,7 +1387,7 @@ static ssize_t sector_size_store(struct device *dev,
 	dev_dbg(dev, "result: %zd %s: %s%s", rc, rc < 0 ? "tried" : "wrote",
 			buf, buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -1502,9 +1502,9 @@ static ssize_t holder_show(struct device *dev,
 	struct nd_namespace_common *ndns = to_ndns(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	rc = sprintf(buf, "%s\n", ndns->claim ? dev_name(ndns->claim) : "");
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -1541,7 +1541,7 @@ static ssize_t holder_class_store(struct device *dev,
 	struct nd_region *nd_region = to_nd_region(dev->parent);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	rc = __holder_class_store(dev, buf);
@@ -1549,7 +1549,7 @@ static ssize_t holder_class_store(struct device *dev,
 		rc = nd_namespace_label_update(nd_region, dev);
 	dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc < 0 ? rc : len;
 }
@@ -1560,7 +1560,7 @@ static ssize_t holder_class_show(struct device *dev,
 	struct nd_namespace_common *ndns = to_ndns(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (ndns->claim_class == NVDIMM_CCLASS_NONE)
 		rc = sprintf(buf, "\n");
 	else if ((ndns->claim_class == NVDIMM_CCLASS_BTT) ||
@@ -1572,7 +1572,7 @@ static ssize_t holder_class_show(struct device *dev,
 		rc = sprintf(buf, "dax\n");
 	else
 		rc = sprintf(buf, "<unknown>\n");
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -1586,7 +1586,7 @@ static ssize_t mode_show(struct device *dev,
 	char *mode;
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	claim = ndns->claim;
 	if (claim && is_nd_btt(claim))
 		mode = "safe";
@@ -1599,7 +1599,7 @@ static ssize_t mode_show(struct device *dev,
 	else
 		mode = "raw";
 	rc = sprintf(buf, "%s\n", mode);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -1703,8 +1703,8 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
 		 * Flush any in-progess probes / removals in the driver
 		 * for the raw personality of this namespace.
 		 */
-		device_lock(&ndns->dev);
-		device_unlock(&ndns->dev);
+		nd_device_lock(&ndns->dev);
+		nd_device_unlock(&ndns->dev);
 		if (ndns->dev.driver) {
 			dev_dbg(&ndns->dev, "is active, can't bind %s\n",
 					dev_name(dev));
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 6cd470547106..0ac52b6eb00e 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -9,6 +9,7 @@
 #include <linux/sizes.h>
 #include <linux/mutex.h>
 #include <linux/nd.h>
+#include "nd.h"
 
 extern struct list_head nvdimm_bus_list;
 extern struct mutex nvdimm_bus_list_mutex;
@@ -182,4 +183,71 @@ ssize_t nd_namespace_store(struct device *dev,
 		struct nd_namespace_common **_ndns, const char *buf,
 		size_t len);
 struct nd_pfn *to_nd_pfn_safe(struct device *dev);
+bool is_nvdimm_bus(struct device *dev);
+
+#ifdef CONFIG_PROVE_LOCKING
+extern struct class *nd_class;
+
+enum {
+	LOCK_BUS,
+	LOCK_NDCTL,
+	LOCK_REGION,
+	LOCK_DIMM = LOCK_REGION,
+	LOCK_NAMESPACE,
+	LOCK_CLAIM,
+};
+
+static inline void debug_nvdimm_lock(struct device *dev)
+{
+	if (is_nd_region(dev))
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_REGION);
+	else if (is_nvdimm(dev))
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_DIMM);
+	else if (is_nd_btt(dev) || is_nd_pfn(dev) || is_nd_dax(dev))
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_CLAIM);
+	else if (dev->parent && (is_nd_region(dev->parent)))
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_NAMESPACE);
+	else if (is_nvdimm_bus(dev))
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_BUS);
+	else if (dev->class && dev->class == nd_class)
+		mutex_lock_nested(&dev->lockdep_mutex, LOCK_NDCTL);
+	else
+		dev_WARN(dev, "unknown lock level\n");
+}
+
+static inline void debug_nvdimm_unlock(struct device *dev)
+{
+	mutex_unlock(&dev->lockdep_mutex);
+}
+
+static inline void nd_device_lock(struct device *dev)
+{
+	device_lock(dev);
+	debug_nvdimm_lock(dev);
+}
+
+static inline void nd_device_unlock(struct device *dev)
+{
+	debug_nvdimm_unlock(dev);
+	device_unlock(dev);
+}
+#else
+static inline void nd_device_lock(struct device *dev)
+{
+	device_lock(dev);
+}
+
+static inline void nd_device_unlock(struct device *dev)
+{
+	device_unlock(dev);
+}
+
+static inline void debug_nvdimm_lock(struct device *dev)
+{
+}
+
+static inline void debug_nvdimm_unlock(struct device *dev)
+{
+}
+#endif
 #endif /* __ND_CORE_H__ */
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 0f81fc56bbfd..9b09fe18e666 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -67,7 +67,7 @@ static ssize_t mode_store(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc = 0;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	if (dev->driver)
 		rc = -EBUSY;
@@ -89,7 +89,7 @@ static ssize_t mode_store(struct device *dev,
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -132,14 +132,14 @@ static ssize_t align_store(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	rc = nd_size_select_store(dev, buf, &nd_pfn->align,
 			nd_pfn_supported_alignments());
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -161,11 +161,11 @@ static ssize_t uuid_store(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	rc = nd_uuid_store(dev, &nd_pfn->uuid, buf, len);
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc ? rc : len;
 }
@@ -190,13 +190,13 @@ static ssize_t namespace_store(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	rc = nd_namespace_store(dev, &nd_pfn->ndns, buf, len);
 	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
 			buf[len - 1] == '\n' ? "" : "\n");
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -208,7 +208,7 @@ static ssize_t resource_show(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (dev->driver) {
 		struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
 		u64 offset = __le64_to_cpu(pfn_sb->dataoff);
@@ -222,7 +222,7 @@ static ssize_t resource_show(struct device *dev,
 		/* no address to convey if the pfn instance is disabled */
 		rc = -ENXIO;
 	}
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
@@ -234,7 +234,7 @@ static ssize_t size_show(struct device *dev,
 	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (dev->driver) {
 		struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
 		u64 offset = __le64_to_cpu(pfn_sb->dataoff);
@@ -250,7 +250,7 @@ static ssize_t size_show(struct device *dev,
 		/* no size to convey if the pfn instance is disabled */
 		rc = -ENXIO;
 	}
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 28cb44c61d4a..53797e7be18a 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -520,8 +520,8 @@ static int nd_pmem_remove(struct device *dev)
 		nvdimm_namespace_detach_btt(to_nd_btt(dev));
 	else {
 		/*
-		 * Note, this assumes device_lock() context to not race
-		 * nd_pmem_notify()
+		 * Note, this assumes nd_device_lock() context to not
+		 * race nd_pmem_notify()
 		 */
 		sysfs_put(pmem->bb_state);
 		pmem->bb_state = NULL;
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 488c47ac4c4a..37bf8719a2a4 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -102,7 +102,7 @@ static int nd_region_remove(struct device *dev)
 	nvdimm_bus_unlock(dev);
 
 	/*
-	 * Note, this assumes device_lock() context to not race
+	 * Note, this assumes nd_device_lock() context to not race
 	 * nd_region_notify()
 	 */
 	sysfs_put(nd_region->bb_state);
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index a15276cdec7d..91b5a7ade0d5 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -329,7 +329,7 @@ static ssize_t set_cookie_show(struct device *dev,
 	 * the v1.1 namespace label cookie definition. To read all this
 	 * data we need to wait for probing to settle.
 	 */
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	if (nd_region->ndr_mappings) {
@@ -346,7 +346,7 @@ static ssize_t set_cookie_show(struct device *dev,
 		}
 	}
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	if (rc)
 		return rc;
@@ -422,12 +422,12 @@ static ssize_t available_size_show(struct device *dev,
 	 * memory nvdimm_bus_lock() is dropped, but that's userspace's
 	 * problem to not race itself.
 	 */
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	available = nd_region_available_dpa(nd_region);
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return sprintf(buf, "%llu\n", available);
 }
@@ -439,12 +439,12 @@ static ssize_t max_available_extent_show(struct device *dev,
 	struct nd_region *nd_region = to_nd_region(dev);
 	unsigned long long available = 0;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	nvdimm_bus_lock(dev);
 	wait_nvdimm_bus_probe_idle(dev);
 	available = nd_region_allocatable_dpa(nd_region);
 	nvdimm_bus_unlock(dev);
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return sprintf(buf, "%llu\n", available);
 }
@@ -563,12 +563,12 @@ static ssize_t region_badblocks_show(struct device *dev,
 	struct nd_region *nd_region = to_nd_region(dev);
 	ssize_t rc;
 
-	device_lock(dev);
+	nd_device_lock(dev);
 	if (dev->driver)
 		rc = badblocks_show(&nd_region->bb, buf, 0);
 	else
 		rc = -ENXIO;
-	device_unlock(dev);
+	nd_device_unlock(dev);
 
 	return rc;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 0da5c67f6be1..9237b857b598 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -909,6 +909,8 @@ struct dev_links_info {
  * 		This identifies the device type and carries type-specific
  * 		information.
  * @mutex:	Mutex to synchronize calls to its driver.
+ * @lockdep_mutex: An optional debug lock that a subsystem can use as a
+ * 		peer lock to gain localized lockdep coverage of the device_lock.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
  * @platform_data: Platform data specific to the device.
@@ -991,6 +993,9 @@ struct device {
 					   core doesn't touch it */
 	void		*driver_data;	/* Driver data, set and get with
 					   dev_set_drvdata/dev_get_drvdata */
+#ifdef CONFIG_PROVE_LOCKING
+	struct mutex		lockdep_mutex;
+#endif
 	struct mutex		mutex;	/* mutex to synchronize calls to
 					 * its driver.
 					 */

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

Re: [PATCH v2 7/7] driver-core, libnvdimm: Let device subsystems add local lockdep coverage

[Greg Kroah-Hartman]

On Wed, Jul 17, 2019 at 06:08:26PM -0700, Dan Williams wrote:
> For good reason, the standard device_lock() is marked
> lockdep_set_novalidate_class() because there is simply no sane way to
> describe the myriad ways the device_lock() ordered with other locks.
> However, that leaves subsystems that know their own local device_lock()
> ordering rules to find lock ordering mistakes manually. Instead,
> introduce an optional / additional lockdep-enabled lock that a subsystem
> can acquire in all the same paths that the device_lock() is acquired.
> 
> A conversion of the NFIT driver and NVDIMM subsystem to a
> lockdep-validate device_lock() scheme is included. The
> debug_nvdimm_lock() implementation implements the correct lock-class and
> stacking order for the libnvdimm device topology hierarchy.
> 
> Yes, this is a hack, but hopefully it is a useful hack for other
> subsystems device_lock() debug sessions. Quoting Greg:
> 
>     "Yeah, it feels a bit hacky but it's really up to a subsystem to mess up
>      using it as much as anything else, so user beware :)
> 
>      I don't object to it if it makes things easier for you to debug."

Sure, apeal to my vanity and quote me in the changelog, it's as if you
are making it trivial for me to ack this...

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

:)


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

Re: [PATCH v2 7/7] driver-core, libnvdimm: Let device subsystems add local lockdep coverage

[Ira Weiny]

On Wed, Jul 17, 2019 at 06:08:26PM -0700, Dan Williams wrote:
> For good reason, the standard device_lock() is marked
> lockdep_set_novalidate_class() because there is simply no sane way to
> describe the myriad ways the device_lock() ordered with other locks.
> However, that leaves subsystems that know their own local device_lock()
> ordering rules to find lock ordering mistakes manually. Instead,
> introduce an optional / additional lockdep-enabled lock that a subsystem
> can acquire in all the same paths that the device_lock() is acquired.
> 
> A conversion of the NFIT driver and NVDIMM subsystem to a
> lockdep-validate device_lock() scheme is included. The
> debug_nvdimm_lock() implementation implements the correct lock-class and
> stacking order for the libnvdimm device topology hierarchy.
> 
> Yes, this is a hack, but hopefully it is a useful hack for other
> subsystems device_lock() debug sessions. Quoting Greg:
> 
>     "Yeah, it feels a bit hacky but it's really up to a subsystem to mess up
>      using it as much as anything else, so user beware :)
> 
>      I don't object to it if it makes things easier for you to debug."
> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/acpi/nfit/core.c        |   28 ++++++++--------
>  drivers/acpi/nfit/nfit.h        |   24 ++++++++++++++
>  drivers/base/core.c             |    3 ++
>  drivers/nvdimm/btt_devs.c       |   16 +++++----
>  drivers/nvdimm/bus.c            |   28 ++++++++++------
>  drivers/nvdimm/core.c           |   10 +++---
>  drivers/nvdimm/dimm_devs.c      |    4 +-
>  drivers/nvdimm/namespace_devs.c |   36 ++++++++++-----------
>  drivers/nvdimm/nd-core.h        |   68 +++++++++++++++++++++++++++++++++++++++
>  drivers/nvdimm/pfn_devs.c       |   24 +++++++-------
>  drivers/nvdimm/pmem.c           |    4 +-
>  drivers/nvdimm/region.c         |    2 +
>  drivers/nvdimm/region_devs.c    |   16 +++++----
>  include/linux/device.h          |    5 +++
>  14 files changed, 187 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 23022cf20d26..f22139458ce1 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1282,7 +1282,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
>  	if (rc)
>  		return rc;
>  
> -	device_lock(dev);
> +	nfit_device_lock(dev);
>  	nd_desc = dev_get_drvdata(dev);
>  	if (nd_desc) {
>  		struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
> @@ -1299,7 +1299,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
>  			break;
>  		}
>  	}
> -	device_unlock(dev);
> +	nfit_device_unlock(dev);
>  	if (rc)
>  		return rc;
>  	return size;
> @@ -1319,7 +1319,7 @@ static ssize_t scrub_show(struct device *dev,
>  	ssize_t rc = -ENXIO;
>  	bool busy;
>  
> -	device_lock(dev);
> +	nfit_device_lock(dev);
>  	nd_desc = dev_get_drvdata(dev);
>  	if (!nd_desc) {
>  		device_unlock(dev);
> @@ -1339,7 +1339,7 @@ static ssize_t scrub_show(struct device *dev,
>  	}
>  
>  	mutex_unlock(&acpi_desc->init_mutex);
> -	device_unlock(dev);
> +	nfit_device_unlock(dev);
>  	return rc;
>  }
>  
> @@ -1356,14 +1356,14 @@ static ssize_t scrub_store(struct device *dev,
>  	if (val != 1)
>  		return -EINVAL;
>  
> -	device_lock(dev);
> +	nfit_device_lock(dev);
>  	nd_desc = dev_get_drvdata(dev);
>  	if (nd_desc) {
>  		struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
>  
>  		rc = acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_LONG);
>  	}
> -	device_unlock(dev);
> +	nfit_device_unlock(dev);
>  	if (rc)
>  		return rc;
>  	return size;
> @@ -1749,9 +1749,9 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
>  	struct acpi_device *adev = data;
>  	struct device *dev = &adev->dev;
>  
> -	device_lock(dev->parent);
> +	nfit_device_lock(dev->parent);
>  	__acpi_nvdimm_notify(dev, event);
> -	device_unlock(dev->parent);
> +	nfit_device_unlock(dev->parent);
>  }
>  
>  static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
> @@ -3457,8 +3457,8 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
>  	struct device *dev = acpi_desc->dev;
>  
>  	/* Bounce the device lock to flush acpi_nfit_add / acpi_nfit_notify */
> -	device_lock(dev);
> -	device_unlock(dev);
> +	nfit_device_lock(dev);
> +	nfit_device_unlock(dev);
>  
>  	/* Bounce the init_mutex to complete initial registration */
>  	mutex_lock(&acpi_desc->init_mutex);
> @@ -3602,8 +3602,8 @@ void acpi_nfit_shutdown(void *data)
>  	 * acpi_nfit_ars_rescan() submissions have had a chance to
>  	 * either submit or see ->cancel set.
>  	 */
> -	device_lock(bus_dev);
> -	device_unlock(bus_dev);
> +	nfit_device_lock(bus_dev);
> +	nfit_device_unlock(bus_dev);
>  
>  	flush_workqueue(nfit_wq);
>  }
> @@ -3746,9 +3746,9 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>  
>  static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>  {
> -	device_lock(&adev->dev);
> +	nfit_device_lock(&adev->dev);
>  	__acpi_nfit_notify(&adev->dev, adev->handle, event);
> -	device_unlock(&adev->dev);
> +	nfit_device_unlock(&adev->dev);
>  }
>  
>  static const struct acpi_device_id acpi_nfit_ids[] = {
> diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
> index 6ee2b02af73e..24241941181c 100644
> --- a/drivers/acpi/nfit/nfit.h
> +++ b/drivers/acpi/nfit/nfit.h
> @@ -312,6 +312,30 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
>  	return container_of(nd_desc, struct acpi_nfit_desc, nd_desc);
>  }
>  
> +#ifdef CONFIG_PROVE_LOCKING
> +static inline void nfit_device_lock(struct device *dev)
> +{
> +	device_lock(dev);
> +	mutex_lock(&dev->lockdep_mutex);
> +}
> +
> +static inline void nfit_device_unlock(struct device *dev)
> +{
> +	mutex_unlock(&dev->lockdep_mutex);
> +	device_unlock(dev);
> +}
> +#else
> +static inline void nfit_device_lock(struct device *dev)
> +{
> +	device_lock(dev);
> +}
> +
> +static inline void nfit_device_unlock(struct device *dev)
> +{
> +	device_unlock(dev);
> +}
> +#endif
> +
>  const guid_t *to_nfit_uuid(enum nfit_uuids id);
>  int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz);
>  void acpi_nfit_shutdown(void *data);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index eaf3aa0cb803..4825949d6547 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1663,6 +1663,9 @@ void device_initialize(struct device *dev)
>  	kobject_init(&dev->kobj, &device_ktype);
>  	INIT_LIST_HEAD(&dev->dma_pools);
>  	mutex_init(&dev->mutex);
> +#ifdef CONFIG_PROVE_LOCKING
> +	mutex_init(&dev->lockdep_mutex);
> +#endif
>  	lockdep_set_novalidate_class(&dev->mutex);
>  	spin_lock_init(&dev->devres_lock);
>  	INIT_LIST_HEAD(&dev->devres_head);
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 62d00fffa4af..3508a79110c7 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -62,14 +62,14 @@ static ssize_t sector_size_store(struct device *dev,
>  	struct nd_btt *nd_btt = to_nd_btt(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	rc = nd_size_select_store(dev, buf, &nd_btt->lbasize,
>  			btt_lbasize_supported);
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -91,11 +91,11 @@ static ssize_t uuid_store(struct device *dev,
>  	struct nd_btt *nd_btt = to_nd_btt(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	rc = nd_uuid_store(dev, &nd_btt->uuid, buf, len);
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -120,13 +120,13 @@ static ssize_t namespace_store(struct device *dev,
>  	struct nd_btt *nd_btt = to_nd_btt(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	rc = nd_namespace_store(dev, &nd_btt->ndns, buf, len);
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -138,14 +138,14 @@ static ssize_t size_show(struct device *dev,
>  	struct nd_btt *nd_btt = to_nd_btt(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (dev->driver)
>  		rc = sprintf(buf, "%llu\n", nd_btt->size);
>  	else {
>  		/* no size to convey if the btt instance is disabled */
>  		rc = -ENXIO;
>  	}
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index df41f3571dc9..798c5c4aea9c 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -26,7 +26,7 @@
>  
>  int nvdimm_major;
>  static int nvdimm_bus_major;
> -static struct class *nd_class;
> +struct class *nd_class;
>  static DEFINE_IDA(nd_ida);
>  
>  static int to_nd_device_type(struct device *dev)
> @@ -91,7 +91,10 @@ static int nvdimm_bus_probe(struct device *dev)
>  			dev->driver->name, dev_name(dev));
>  
>  	nvdimm_bus_probe_start(nvdimm_bus);
> +	debug_nvdimm_lock(dev);
>  	rc = nd_drv->probe(dev);
> +	debug_nvdimm_unlock(dev);
> +
>  	if (rc == 0)
>  		nd_region_probe_success(nvdimm_bus, dev);
>  	else
> @@ -113,8 +116,11 @@ static int nvdimm_bus_remove(struct device *dev)
>  	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
>  	int rc = 0;
>  
> -	if (nd_drv->remove)
> +	if (nd_drv->remove) {
> +		debug_nvdimm_lock(dev);
>  		rc = nd_drv->remove(dev);
> +		debug_nvdimm_unlock(dev);
> +	}
>  	nd_region_disable(nvdimm_bus, dev);
>  
>  	dev_dbg(&nvdimm_bus->dev, "%s.remove(%s) = %d\n", dev->driver->name,
> @@ -140,7 +146,7 @@ static void nvdimm_bus_shutdown(struct device *dev)
>  
>  void nd_device_notify(struct device *dev, enum nvdimm_event event)
>  {
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (dev->driver) {
>  		struct nd_device_driver *nd_drv;
>  
> @@ -148,7 +154,7 @@ void nd_device_notify(struct device *dev, enum nvdimm_event event)
>  		if (nd_drv->notify)
>  			nd_drv->notify(dev, event);
>  	}
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  }
>  EXPORT_SYMBOL(nd_device_notify);
>  
> @@ -296,7 +302,7 @@ static void nvdimm_bus_release(struct device *dev)
>  	kfree(nvdimm_bus);
>  }
>  
> -static bool is_nvdimm_bus(struct device *dev)
> +bool is_nvdimm_bus(struct device *dev)
>  {
>  	return dev->release == nvdimm_bus_release;
>  }
> @@ -575,9 +581,9 @@ void nd_device_unregister(struct device *dev, enum nd_async_mode mode)
>  		 * or otherwise let the async path handle it if the
>  		 * unregistration was already queued.
>  		 */
> -		device_lock(dev);
> +		nd_device_lock(dev);
>  		killed = kill_device(dev);
> -		device_unlock(dev);
> +		nd_device_unlock(dev);
>  
>  		if (!killed)
>  			return;
> @@ -888,10 +894,10 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
>  		if (nvdimm_bus->probe_active == 0)
>  			break;
>  		nvdimm_bus_unlock(dev);
> -		device_unlock(dev);
> +		nd_device_unlock(dev);
>  		wait_event(nvdimm_bus->wait,
>  				nvdimm_bus->probe_active == 0);
> -		device_lock(dev);
> +		nd_device_lock(dev);
>  		nvdimm_bus_lock(dev);
>  	} while (true);
>  }
> @@ -1107,7 +1113,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>  		goto out;
>  	}
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
>  	if (rc)
> @@ -1129,7 +1135,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>  
>  out_unlock:
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  out:
>  	kfree(in_env);
>  	kfree(out_env);
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index 5e1f060547bf..9204f1e9fd14 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -246,7 +246,7 @@ static int nd_uuid_parse(struct device *dev, u8 *uuid_out, const char *buf,
>   *
>   * Enforce that uuids can only be changed while the device is disabled
>   * (driver detached)
> - * LOCKING: expects device_lock() is held on entry
> + * LOCKING: expects nd_device_lock() is held on entry
>   */
>  int nd_uuid_store(struct device *dev, u8 **uuid_out, const char *buf,
>  		size_t len)
> @@ -347,15 +347,15 @@ static DEVICE_ATTR_RO(provider);
>  
>  static int flush_namespaces(struct device *dev, void *data)
>  {
> -	device_lock(dev);
> -	device_unlock(dev);
> +	nd_device_lock(dev);
> +	nd_device_unlock(dev);
>  	return 0;
>  }
>  
>  static int flush_regions_dimms(struct device *dev, void *data)
>  {
> -	device_lock(dev);
> -	device_unlock(dev);
> +	nd_device_lock(dev);
> +	nd_device_unlock(dev);
>  	device_for_each_child(dev, NULL, flush_namespaces);
>  	return 0;
>  }
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index dfecd6e17043..29a065e769ea 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -484,12 +484,12 @@ static ssize_t security_store(struct device *dev,
>  	 * done while probing is idle and the DIMM is not in active use
>  	 * in any region.
>  	 */
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	rc = __security_store(dev, buf, len);
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index a434a5964cb9..92cd809d7e43 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -410,7 +410,7 @@ static ssize_t alt_name_store(struct device *dev,
>  	struct nd_region *nd_region = to_nd_region(dev->parent);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	rc = __alt_name_store(dev, buf, len);
> @@ -418,7 +418,7 @@ static ssize_t alt_name_store(struct device *dev,
>  		rc = nd_namespace_label_update(nd_region, dev);
>  	dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc < 0 ? rc : len;
>  }
> @@ -1077,7 +1077,7 @@ static ssize_t size_store(struct device *dev,
>  	if (rc)
>  		return rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	rc = __size_store(dev, val);
> @@ -1103,7 +1103,7 @@ static ssize_t size_store(struct device *dev,
>  	dev_dbg(dev, "%llx %s (%d)\n", val, rc < 0 ? "fail" : "success", rc);
>  
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc < 0 ? rc : len;
>  }
> @@ -1286,7 +1286,7 @@ static ssize_t uuid_store(struct device *dev,
>  	} else
>  		return -ENXIO;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	if (to_ndns(dev)->claim)
> @@ -1302,7 +1302,7 @@ static ssize_t uuid_store(struct device *dev,
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc < 0 ? rc : len;
>  }
> @@ -1376,7 +1376,7 @@ static ssize_t sector_size_store(struct device *dev,
>  	} else
>  		return -ENXIO;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	if (to_ndns(dev)->claim)
>  		rc = -EBUSY;
> @@ -1387,7 +1387,7 @@ static ssize_t sector_size_store(struct device *dev,
>  	dev_dbg(dev, "result: %zd %s: %s%s", rc, rc < 0 ? "tried" : "wrote",
>  			buf, buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -1502,9 +1502,9 @@ static ssize_t holder_show(struct device *dev,
>  	struct nd_namespace_common *ndns = to_ndns(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	rc = sprintf(buf, "%s\n", ndns->claim ? dev_name(ndns->claim) : "");
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -1541,7 +1541,7 @@ static ssize_t holder_class_store(struct device *dev,
>  	struct nd_region *nd_region = to_nd_region(dev->parent);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	rc = __holder_class_store(dev, buf);
> @@ -1549,7 +1549,7 @@ static ssize_t holder_class_store(struct device *dev,
>  		rc = nd_namespace_label_update(nd_region, dev);
>  	dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc < 0 ? rc : len;
>  }
> @@ -1560,7 +1560,7 @@ static ssize_t holder_class_show(struct device *dev,
>  	struct nd_namespace_common *ndns = to_ndns(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (ndns->claim_class == NVDIMM_CCLASS_NONE)
>  		rc = sprintf(buf, "\n");
>  	else if ((ndns->claim_class == NVDIMM_CCLASS_BTT) ||
> @@ -1572,7 +1572,7 @@ static ssize_t holder_class_show(struct device *dev,
>  		rc = sprintf(buf, "dax\n");
>  	else
>  		rc = sprintf(buf, "<unknown>\n");
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -1586,7 +1586,7 @@ static ssize_t mode_show(struct device *dev,
>  	char *mode;
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	claim = ndns->claim;
>  	if (claim && is_nd_btt(claim))
>  		mode = "safe";
> @@ -1599,7 +1599,7 @@ static ssize_t mode_show(struct device *dev,
>  	else
>  		mode = "raw";
>  	rc = sprintf(buf, "%s\n", mode);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -1703,8 +1703,8 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>  		 * Flush any in-progess probes / removals in the driver
>  		 * for the raw personality of this namespace.
>  		 */
> -		device_lock(&ndns->dev);
> -		device_unlock(&ndns->dev);
> +		nd_device_lock(&ndns->dev);
> +		nd_device_unlock(&ndns->dev);
>  		if (ndns->dev.driver) {
>  			dev_dbg(&ndns->dev, "is active, can't bind %s\n",
>  					dev_name(dev));
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index 6cd470547106..0ac52b6eb00e 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -9,6 +9,7 @@
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/nd.h>
> +#include "nd.h"
>  
>  extern struct list_head nvdimm_bus_list;
>  extern struct mutex nvdimm_bus_list_mutex;
> @@ -182,4 +183,71 @@ ssize_t nd_namespace_store(struct device *dev,
>  		struct nd_namespace_common **_ndns, const char *buf,
>  		size_t len);
>  struct nd_pfn *to_nd_pfn_safe(struct device *dev);
> +bool is_nvdimm_bus(struct device *dev);
> +
> +#ifdef CONFIG_PROVE_LOCKING
> +extern struct class *nd_class;
> +
> +enum {
> +	LOCK_BUS,
> +	LOCK_NDCTL,
> +	LOCK_REGION,
> +	LOCK_DIMM = LOCK_REGION,
> +	LOCK_NAMESPACE,
> +	LOCK_CLAIM,
> +};
> +
> +static inline void debug_nvdimm_lock(struct device *dev)
> +{
> +	if (is_nd_region(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_REGION);
> +	else if (is_nvdimm(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_DIMM);
> +	else if (is_nd_btt(dev) || is_nd_pfn(dev) || is_nd_dax(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_CLAIM);
> +	else if (dev->parent && (is_nd_region(dev->parent)))
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_NAMESPACE);
> +	else if (is_nvdimm_bus(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_BUS);
> +	else if (dev->class && dev->class == nd_class)
> +		mutex_lock_nested(&dev->lockdep_mutex, LOCK_NDCTL);
> +	else
> +		dev_WARN(dev, "unknown lock level\n");
> +}
> +
> +static inline void debug_nvdimm_unlock(struct device *dev)
> +{
> +	mutex_unlock(&dev->lockdep_mutex);
> +}
> +
> +static inline void nd_device_lock(struct device *dev)
> +{
> +	device_lock(dev);
> +	debug_nvdimm_lock(dev);
> +}
> +
> +static inline void nd_device_unlock(struct device *dev)
> +{
> +	debug_nvdimm_unlock(dev);
> +	device_unlock(dev);
> +}
> +#else
> +static inline void nd_device_lock(struct device *dev)
> +{
> +	device_lock(dev);
> +}
> +
> +static inline void nd_device_unlock(struct device *dev)
> +{
> +	device_unlock(dev);
> +}
> +
> +static inline void debug_nvdimm_lock(struct device *dev)
> +{
> +}
> +
> +static inline void debug_nvdimm_unlock(struct device *dev)
> +{
> +}
> +#endif
>  #endif /* __ND_CORE_H__ */
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 0f81fc56bbfd..9b09fe18e666 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -67,7 +67,7 @@ static ssize_t mode_store(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc = 0;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	if (dev->driver)
>  		rc = -EBUSY;
> @@ -89,7 +89,7 @@ static ssize_t mode_store(struct device *dev,
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -132,14 +132,14 @@ static ssize_t align_store(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	rc = nd_size_select_store(dev, buf, &nd_pfn->align,
>  			nd_pfn_supported_alignments());
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -161,11 +161,11 @@ static ssize_t uuid_store(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	rc = nd_uuid_store(dev, &nd_pfn->uuid, buf, len);
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc ? rc : len;
>  }
> @@ -190,13 +190,13 @@ static ssize_t namespace_store(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	rc = nd_namespace_store(dev, &nd_pfn->ndns, buf, len);
>  	dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
>  			buf[len - 1] == '\n' ? "" : "\n");
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -208,7 +208,7 @@ static ssize_t resource_show(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (dev->driver) {
>  		struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
>  		u64 offset = __le64_to_cpu(pfn_sb->dataoff);
> @@ -222,7 +222,7 @@ static ssize_t resource_show(struct device *dev,
>  		/* no address to convey if the pfn instance is disabled */
>  		rc = -ENXIO;
>  	}
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> @@ -234,7 +234,7 @@ static ssize_t size_show(struct device *dev,
>  	struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (dev->driver) {
>  		struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
>  		u64 offset = __le64_to_cpu(pfn_sb->dataoff);
> @@ -250,7 +250,7 @@ static ssize_t size_show(struct device *dev,
>  		/* no size to convey if the pfn instance is disabled */
>  		rc = -ENXIO;
>  	}
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 28cb44c61d4a..53797e7be18a 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -520,8 +520,8 @@ static int nd_pmem_remove(struct device *dev)
>  		nvdimm_namespace_detach_btt(to_nd_btt(dev));
>  	else {
>  		/*
> -		 * Note, this assumes device_lock() context to not race
> -		 * nd_pmem_notify()
> +		 * Note, this assumes nd_device_lock() context to not
> +		 * race nd_pmem_notify()
>  		 */
>  		sysfs_put(pmem->bb_state);
>  		pmem->bb_state = NULL;
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index 488c47ac4c4a..37bf8719a2a4 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -102,7 +102,7 @@ static int nd_region_remove(struct device *dev)
>  	nvdimm_bus_unlock(dev);
>  
>  	/*
> -	 * Note, this assumes device_lock() context to not race
> +	 * Note, this assumes nd_device_lock() context to not race
>  	 * nd_region_notify()
>  	 */
>  	sysfs_put(nd_region->bb_state);
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index a15276cdec7d..91b5a7ade0d5 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -329,7 +329,7 @@ static ssize_t set_cookie_show(struct device *dev,
>  	 * the v1.1 namespace label cookie definition. To read all this
>  	 * data we need to wait for probing to settle.
>  	 */
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	if (nd_region->ndr_mappings) {
> @@ -346,7 +346,7 @@ static ssize_t set_cookie_show(struct device *dev,
>  		}
>  	}
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	if (rc)
>  		return rc;
> @@ -422,12 +422,12 @@ static ssize_t available_size_show(struct device *dev,
>  	 * memory nvdimm_bus_lock() is dropped, but that's userspace's
>  	 * problem to not race itself.
>  	 */
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	available = nd_region_available_dpa(nd_region);
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return sprintf(buf, "%llu\n", available);
>  }
> @@ -439,12 +439,12 @@ static ssize_t max_available_extent_show(struct device *dev,
>  	struct nd_region *nd_region = to_nd_region(dev);
>  	unsigned long long available = 0;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	nvdimm_bus_lock(dev);
>  	wait_nvdimm_bus_probe_idle(dev);
>  	available = nd_region_allocatable_dpa(nd_region);
>  	nvdimm_bus_unlock(dev);
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return sprintf(buf, "%llu\n", available);
>  }
> @@ -563,12 +563,12 @@ static ssize_t region_badblocks_show(struct device *dev,
>  	struct nd_region *nd_region = to_nd_region(dev);
>  	ssize_t rc;
>  
> -	device_lock(dev);
> +	nd_device_lock(dev);
>  	if (dev->driver)
>  		rc = badblocks_show(&nd_region->bb, buf, 0);
>  	else
>  		rc = -ENXIO;
> -	device_unlock(dev);
> +	nd_device_unlock(dev);
>  
>  	return rc;
>  }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0da5c67f6be1..9237b857b598 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -909,6 +909,8 @@ struct dev_links_info {
>   * 		This identifies the device type and carries type-specific
>   * 		information.
>   * @mutex:	Mutex to synchronize calls to its driver.
> + * @lockdep_mutex: An optional debug lock that a subsystem can use as a
> + * 		peer lock to gain localized lockdep coverage of the device_lock.
>   * @bus:	Type of bus device is on.
>   * @driver:	Which driver has allocated this
>   * @platform_data: Platform data specific to the device.
> @@ -991,6 +993,9 @@ struct device {
>  					   core doesn't touch it */
>  	void		*driver_data;	/* Driver data, set and get with
>  					   dev_set_drvdata/dev_get_drvdata */
> +#ifdef CONFIG_PROVE_LOCKING
> +	struct mutex		lockdep_mutex;
> +#endif
>  	struct mutex		mutex;	/* mutex to synchronize calls to
>  					 * its driver.
>  					 */
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm