* [PATCH v3 1/8] cxl: Replace lockdep_mutex with local lock classes
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-22 23:43 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation Dan Williams
` (6 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
Boqun Feng, Alison Schofield, Vishal Verma, Ira Weiny,
Ben Widawsky, Jonathan Cameron, nvdimm, linux-kernel
In response to an attempt to expand dev->lockdep_mutex for device_lock()
validation [1], Peter points out [2] that the lockdep API already has
the ability to assign a dedicated lock class per subsystem device-type.
Use lockdep_set_class() to override the default device_lock()
'__lockdep_no_validate__' class for each CXL subsystem device-type. This
enables lockdep to detect deadlocks and recursive locking within the
device-driver core and the subsystem. The
lockdep_set_class_and_subclass() API is used for port objects that
recursively lock the 'cxl_port_key' class by hierarchical topology
depth.
Link: https://lore.kernel.org/r/164982968798.684294.15817853329823976469.stgit@dwillia2-desk3.amr.corp.intel.com [1]
Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [2]
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/memdev.c | 3 +++
drivers/cxl/core/pmem.c | 6 ++++++
drivers/cxl/core/port.c | 13 +++++++++----
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 1f76b28f9826..f7cdcd33504a 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -228,6 +228,8 @@ static void detach_memdev(struct work_struct *work)
put_device(&cxlmd->dev);
}
+static struct lock_class_key cxl_memdev_key;
+
static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
const struct file_operations *fops)
{
@@ -247,6 +249,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
dev = &cxlmd->dev;
device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &cxl_memdev_key);
dev->parent = cxlds->dev;
dev->bus = &cxl_bus_type;
dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index 8de240c4d96b..e825e261278d 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -80,6 +80,8 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd)
}
EXPORT_SYMBOL_NS_GPL(cxl_find_nvdimm_bridge, CXL);
+static struct lock_class_key cxl_nvdimm_bridge_key;
+
static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
{
struct cxl_nvdimm_bridge *cxl_nvb;
@@ -99,6 +101,7 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
cxl_nvb->port = port;
cxl_nvb->state = CXL_NVB_NEW;
device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &cxl_nvdimm_bridge_key);
device_set_pm_not_required(dev);
dev->parent = &port->dev;
dev->bus = &cxl_bus_type;
@@ -214,6 +217,8 @@ struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev)
}
EXPORT_SYMBOL_NS_GPL(to_cxl_nvdimm, CXL);
+static struct lock_class_key cxl_nvdimm_key;
+
static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
{
struct cxl_nvdimm *cxl_nvd;
@@ -226,6 +231,7 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
dev = &cxl_nvd->dev;
cxl_nvd->cxlmd = cxlmd;
device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
device_set_pm_not_required(dev);
dev->parent = &cxlmd->dev;
dev->bus = &cxl_bus_type;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 2ab1ba4499b3..750aac95ed5f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -391,6 +391,8 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
return devm_add_action_or_reset(host, cxl_unlink_uport, port);
}
+static struct lock_class_key cxl_port_key;
+
static struct cxl_port *cxl_port_alloc(struct device *uport,
resource_size_t component_reg_phys,
struct cxl_port *parent_port)
@@ -415,9 +417,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
* description.
*/
dev = &port->dev;
- if (parent_port)
+ if (parent_port) {
dev->parent = &parent_port->dev;
- else
+ port->depth = parent_port->depth + 1;
+ } else
dev->parent = uport;
port->uport = uport;
@@ -427,6 +430,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
INIT_LIST_HEAD(&port->endpoints);
device_initialize(dev);
+ lockdep_set_class_and_subclass(&dev->mutex, &cxl_port_key, port->depth);
device_set_pm_not_required(dev);
dev->bus = &cxl_bus_type;
dev->type = &cxl_port_type;
@@ -457,8 +461,6 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
if (IS_ERR(port))
return port;
- if (parent_port)
- port->depth = parent_port->depth + 1;
dev = &port->dev;
if (is_cxl_memdev(uport))
rc = dev_set_name(dev, "endpoint%d", port->id);
@@ -1173,6 +1175,8 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
return rc;
}
+static struct lock_class_key cxl_decoder_key;
+
/**
* cxl_decoder_alloc - Allocate a new CXL decoder
* @port: owning port of this decoder
@@ -1214,6 +1218,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
seqlock_init(&cxld->target_lock);
dev = &cxld->dev;
device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &cxl_decoder_key);
device_set_pm_not_required(dev);
dev->parent = &port->dev;
dev->bus = &cxl_bus_type;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 1/8] cxl: Replace lockdep_mutex with local lock classes
2022-04-21 15:33 ` [PATCH v3 1/8] cxl: Replace lockdep_mutex with local lock classes Dan Williams
@ 2022-04-22 23:43 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-22 23:43 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
Boqun Feng, Alison Schofield, Vishal Verma, Ben Widawsky,
Jonathan Cameron, nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:13AM -0700, Dan Williams wrote:
> In response to an attempt to expand dev->lockdep_mutex for device_lock()
> validation [1], Peter points out [2] that the lockdep API already has
> the ability to assign a dedicated lock class per subsystem device-type.
>
> Use lockdep_set_class() to override the default device_lock()
> '__lockdep_no_validate__' class for each CXL subsystem device-type. This
> enables lockdep to detect deadlocks and recursive locking within the
> device-driver core and the subsystem. The
> lockdep_set_class_and_subclass() API is used for port objects that
> recursively lock the 'cxl_port_key' class by hierarchical topology
> depth.
>
> Link: https://lore.kernel.org/r/164982968798.684294.15817853329823976469.stgit@dwillia2-desk3.amr.corp.intel.com [1]
> Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [2]
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/cxl/core/memdev.c | 3 +++
> drivers/cxl/core/pmem.c | 6 ++++++
> drivers/cxl/core/port.c | 13 +++++++++----
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 1f76b28f9826..f7cdcd33504a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -228,6 +228,8 @@ static void detach_memdev(struct work_struct *work)
> put_device(&cxlmd->dev);
> }
>
> +static struct lock_class_key cxl_memdev_key;
> +
> static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> const struct file_operations *fops)
> {
> @@ -247,6 +249,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>
> dev = &cxlmd->dev;
> device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_memdev_key);
> dev->parent = cxlds->dev;
> dev->bus = &cxl_bus_type;
> dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index 8de240c4d96b..e825e261278d 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -80,6 +80,8 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd)
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_nvdimm_bridge, CXL);
>
> +static struct lock_class_key cxl_nvdimm_bridge_key;
> +
> static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
> {
> struct cxl_nvdimm_bridge *cxl_nvb;
> @@ -99,6 +101,7 @@ static struct cxl_nvdimm_bridge *cxl_nvdimm_bridge_alloc(struct cxl_port *port)
> cxl_nvb->port = port;
> cxl_nvb->state = CXL_NVB_NEW;
> device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_nvdimm_bridge_key);
> device_set_pm_not_required(dev);
> dev->parent = &port->dev;
> dev->bus = &cxl_bus_type;
> @@ -214,6 +217,8 @@ struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev)
> }
> EXPORT_SYMBOL_NS_GPL(to_cxl_nvdimm, CXL);
>
> +static struct lock_class_key cxl_nvdimm_key;
> +
> static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
> {
> struct cxl_nvdimm *cxl_nvd;
> @@ -226,6 +231,7 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
> dev = &cxl_nvd->dev;
> cxl_nvd->cxlmd = cxlmd;
> device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
> device_set_pm_not_required(dev);
> dev->parent = &cxlmd->dev;
> dev->bus = &cxl_bus_type;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 2ab1ba4499b3..750aac95ed5f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -391,6 +391,8 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> return devm_add_action_or_reset(host, cxl_unlink_uport, port);
> }
>
> +static struct lock_class_key cxl_port_key;
> +
> static struct cxl_port *cxl_port_alloc(struct device *uport,
> resource_size_t component_reg_phys,
> struct cxl_port *parent_port)
> @@ -415,9 +417,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> * description.
> */
> dev = &port->dev;
> - if (parent_port)
> + if (parent_port) {
> dev->parent = &parent_port->dev;
> - else
> + port->depth = parent_port->depth + 1;
> + } else
> dev->parent = uport;
>
> port->uport = uport;
> @@ -427,6 +430,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> INIT_LIST_HEAD(&port->endpoints);
>
> device_initialize(dev);
> + lockdep_set_class_and_subclass(&dev->mutex, &cxl_port_key, port->depth);
> device_set_pm_not_required(dev);
> dev->bus = &cxl_bus_type;
> dev->type = &cxl_port_type;
> @@ -457,8 +461,6 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> if (IS_ERR(port))
> return port;
>
> - if (parent_port)
> - port->depth = parent_port->depth + 1;
> dev = &port->dev;
> if (is_cxl_memdev(uport))
> rc = dev_set_name(dev, "endpoint%d", port->id);
> @@ -1173,6 +1175,8 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> return rc;
> }
>
> +static struct lock_class_key cxl_decoder_key;
> +
> /**
> * cxl_decoder_alloc - Allocate a new CXL decoder
> * @port: owning port of this decoder
> @@ -1214,6 +1218,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> seqlock_init(&cxld->target_lock);
> dev = &cxld->dev;
> device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_decoder_key);
> device_set_pm_not_required(dev);
> dev->parent = &port->dev;
> dev->bus = &cxl_bus_type;
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
2022-04-21 15:33 ` [PATCH v3 1/8] cxl: Replace lockdep_mutex with local lock classes Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-21 16:10 ` Greg Kroah-Hartman
` (2 more replies)
2022-04-21 15:33 ` [PATCH v3 3/8] cxl: Drop cxl_device_lock() Dan Williams
` (5 subsequent siblings)
7 siblings, 3 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky,
Jonathan Cameron, nvdimm, linux-kernel
The CXL "root" device, ACPI0017, is an attach point for coordinating
platform level CXL resources and is the parent device for a CXL port
topology tree. As such it has distinct locking rules relative to other
CXL subsystem objects, but because it is an ACPI device the lock class
is established well before it is given to the cxl_acpi driver.
However, the lockdep API does support changing the lock class "live" for
situations like this. Add a device_lock_set_class() helper that a driver
can use in ->probe() to set a custom lock class, and
device_lock_reset_class() to return to the default "no validate" class
before the custom lock class key goes out of scope after ->remove().
Note the helpers are all macros to support dead code elimination in the
CONFIG_PROVE_LOCKING=n case.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/acpi.c | 15 +++++++++++++++
include/linux/device.h | 25 +++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..e19cea27387e 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -275,6 +275,15 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+static struct lock_class_key cxl_root_key;
+
+static void cxl_acpi_lock_reset_class(void *_dev)
+{
+ struct device *dev = _dev;
+
+ device_lock_reset_class(dev);
+}
+
static int cxl_acpi_probe(struct platform_device *pdev)
{
int rc;
@@ -283,6 +292,12 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ device_lock_set_class(&pdev->dev, &cxl_root_key);
+ rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
+ &pdev->dev);
+ if (rc)
+ return rc;
+
root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
if (IS_ERR(root_port))
return PTR_ERR(root_port);
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..82c9d307e7bd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -850,6 +850,31 @@ static inline bool device_supports_offline(struct device *dev)
return dev->bus && dev->bus->offline && dev->bus->online;
}
+#define __device_lock_set_class(dev, name, key) \
+ lock_set_class(&(dev)->mutex.dep_map, name, key, 0, _THIS_IP_)
+
+/**
+ * device_lock_set_class - Specify a temporary lock class while a device
+ * is attached to a driver
+ * @dev: device to modify
+ * @key: lock class key data
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->probe().
+ */
+#define device_lock_set_class(dev, key) \
+ __device_lock_set_class(dev, #key, key)
+
+/**
+ * device_lock_reset_class - Return a device to the default lockdep novalidate state
+ * @dev: device to modify
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->remove().
+ */
+#define device_lock_reset_class(dev) \
+ device_lock_set_class(dev, &__lockdep_no_validate__)
+
void lock_device_hotplug(void);
void unlock_device_hotplug(void);
int lock_device_hotplug_sysfs(void);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-21 15:33 ` [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation Dan Williams
@ 2022-04-21 16:10 ` Greg Kroah-Hartman
2022-04-22 23:58 ` Ira Weiny
2022-04-23 21:05 ` [PATCH v4 " Dan Williams
2 siblings, 0 replies; 30+ messages in thread
From: Greg Kroah-Hartman @ 2022-04-21 16:10 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Peter Zijlstra, Rafael J. Wysocki, Ingo Molnar,
Will Deacon, Waiman Long, Boqun Feng, Alison Schofield,
Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron, nvdimm,
linux-kernel
On Thu, Apr 21, 2022 at 08:33:18AM -0700, Dan Williams wrote:
> The CXL "root" device, ACPI0017, is an attach point for coordinating
> platform level CXL resources and is the parent device for a CXL port
> topology tree. As such it has distinct locking rules relative to other
> CXL subsystem objects, but because it is an ACPI device the lock class
> is established well before it is given to the cxl_acpi driver.
>
> However, the lockdep API does support changing the lock class "live" for
> situations like this. Add a device_lock_set_class() helper that a driver
> can use in ->probe() to set a custom lock class, and
> device_lock_reset_class() to return to the default "no validate" class
> before the custom lock class key goes out of scope after ->remove().
>
> Note the helpers are all macros to support dead code elimination in the
> CONFIG_PROVE_LOCKING=n case.
>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/acpi.c | 15 +++++++++++++++
> include/linux/device.h | 25 +++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
Much simpler, great work.
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-21 15:33 ` [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation Dan Williams
2022-04-21 16:10 ` Greg Kroah-Hartman
@ 2022-04-22 23:58 ` Ira Weiny
2022-04-23 0:08 ` Dan Williams
2022-04-23 21:05 ` [PATCH v4 " Dan Williams
2 siblings, 1 reply; 30+ messages in thread
From: Ira Weiny @ 2022-04-22 23:58 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Peter Zijlstra, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:18AM -0700, Dan Williams wrote:
> The CXL "root" device, ACPI0017, is an attach point for coordinating
> platform level CXL resources and is the parent device for a CXL port
> topology tree. As such it has distinct locking rules relative to other
> CXL subsystem objects, but because it is an ACPI device the lock class
> is established well before it is given to the cxl_acpi driver.
This final sentence gave me pause because it implied that the device lock class
was set to something other than no validate. But I don't see that anywhere in
the acpi code. So given that it looks to me like ACPI is just using the
default no validate class...
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> However, the lockdep API does support changing the lock class "live" for
> situations like this. Add a device_lock_set_class() helper that a driver
> can use in ->probe() to set a custom lock class, and
> device_lock_reset_class() to return to the default "no validate" class
> before the custom lock class key goes out of scope after ->remove().
>
> Note the helpers are all macros to support dead code elimination in the
> CONFIG_PROVE_LOCKING=n case.
>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/acpi.c | 15 +++++++++++++++
> include/linux/device.h | 25 +++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d15a6aec0331..e19cea27387e 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -275,6 +275,15 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
> return 1;
> }
>
> +static struct lock_class_key cxl_root_key;
> +
> +static void cxl_acpi_lock_reset_class(void *_dev)
> +{
> + struct device *dev = _dev;
> +
> + device_lock_reset_class(dev);
> +}
> +
> static int cxl_acpi_probe(struct platform_device *pdev)
> {
> int rc;
> @@ -283,6 +292,12 @@ static int cxl_acpi_probe(struct platform_device *pdev)
> struct acpi_device *adev = ACPI_COMPANION(host);
> struct cxl_cfmws_context ctx;
>
> + device_lock_set_class(&pdev->dev, &cxl_root_key);
> + rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
> + &pdev->dev);
> + if (rc)
> + return rc;
> +
> root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> if (IS_ERR(root_port))
> return PTR_ERR(root_port);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 93459724dcde..82c9d307e7bd 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -850,6 +850,31 @@ static inline bool device_supports_offline(struct device *dev)
> return dev->bus && dev->bus->offline && dev->bus->online;
> }
>
> +#define __device_lock_set_class(dev, name, key) \
> + lock_set_class(&(dev)->mutex.dep_map, name, key, 0, _THIS_IP_)
> +
> +/**
> + * device_lock_set_class - Specify a temporary lock class while a device
> + * is attached to a driver
> + * @dev: device to modify
> + * @key: lock class key data
> + *
> + * This must be called with the device_lock() already held, for example
> + * from driver ->probe().
> + */
> +#define device_lock_set_class(dev, key) \
> + __device_lock_set_class(dev, #key, key)
> +
> +/**
> + * device_lock_reset_class - Return a device to the default lockdep novalidate state
> + * @dev: device to modify
> + *
> + * This must be called with the device_lock() already held, for example
> + * from driver ->remove().
> + */
> +#define device_lock_reset_class(dev) \
> + device_lock_set_class(dev, &__lockdep_no_validate__)
> +
> void lock_device_hotplug(void);
> void unlock_device_hotplug(void);
> int lock_device_hotplug_sysfs(void);
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-22 23:58 ` Ira Weiny
@ 2022-04-23 0:08 ` Dan Williams
2022-04-23 17:27 ` Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-23 0:08 UTC (permalink / raw)
To: Ira Weiny
Cc: linux-cxl, Peter Zijlstra, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
Linux NVDIMM, Linux Kernel Mailing List
On Fri, Apr 22, 2022 at 4:58 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Thu, Apr 21, 2022 at 08:33:18AM -0700, Dan Williams wrote:
> > The CXL "root" device, ACPI0017, is an attach point for coordinating
> > platform level CXL resources and is the parent device for a CXL port
> > topology tree. As such it has distinct locking rules relative to other
> > CXL subsystem objects, but because it is an ACPI device the lock class
> > is established well before it is given to the cxl_acpi driver.
>
> This final sentence gave me pause because it implied that the device lock class
> was set to something other than no validate. But I don't see that anywhere in
> the acpi code. So given that it looks to me like ACPI is just using the
> default no validate class...
Oh, good observation. *If* ACPI had set a custom lock class then
cxl_acpi would need to be careful to restore that ACPI-specific class
and not reset it to "no validate" on exit, or skip setting its own
custom class. However, I think for generic buses like ACPI that feed
devices into other subsystems it likely has little reason to set its
own class. For safety, since device_lock_set_class() is general
purpose, I'll have it emit a debug message and fail if the class is
not "no validate" on entry.
Thanks Ira!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-23 0:08 ` Dan Williams
@ 2022-04-23 17:27 ` Dan Williams
2022-04-25 10:33 ` Peter Zijlstra
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-23 17:27 UTC (permalink / raw)
To: Ira Weiny
Cc: linux-cxl, Peter Zijlstra, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
Linux NVDIMM, Linux Kernel Mailing List
On Fri, Apr 22, 2022 at 5:08 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Fri, Apr 22, 2022 at 4:58 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Thu, Apr 21, 2022 at 08:33:18AM -0700, Dan Williams wrote:
> > > The CXL "root" device, ACPI0017, is an attach point for coordinating
> > > platform level CXL resources and is the parent device for a CXL port
> > > topology tree. As such it has distinct locking rules relative to other
> > > CXL subsystem objects, but because it is an ACPI device the lock class
> > > is established well before it is given to the cxl_acpi driver.
> >
> > This final sentence gave me pause because it implied that the device lock class
> > was set to something other than no validate. But I don't see that anywhere in
> > the acpi code. So given that it looks to me like ACPI is just using the
> > default no validate class...
>
> Oh, good observation. *If* ACPI had set a custom lock class then
> cxl_acpi would need to be careful to restore that ACPI-specific class
> and not reset it to "no validate" on exit, or skip setting its own
> custom class. However, I think for generic buses like ACPI that feed
> devices into other subsystems it likely has little reason to set its
> own class. For safety, since device_lock_set_class() is general
> purpose, I'll have it emit a debug message and fail if the class is
> not "no validate" on entry.
>
So this turned out way uglier than I expected:
drivers/cxl/acpi.c | 4 +++-
include/linux/device.h | 33 +++++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 9 deletions(-)
...so I'm going to drop it and just add a comment about the
expectations. As Peter said there's already a multitude of ways to
cause false positive / negative results with lockdep so this is just
one more area where one needs to be careful and understand the lock
context they might be overriding.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-23 17:27 ` Dan Williams
@ 2022-04-25 10:33 ` Peter Zijlstra
2022-04-25 16:05 ` Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Peter Zijlstra @ 2022-04-25 10:33 UTC (permalink / raw)
To: Dan Williams
Cc: Ira Weiny, linux-cxl, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
Linux NVDIMM, Linux Kernel Mailing List
On Sat, Apr 23, 2022 at 10:27:52AM -0700, Dan Williams wrote:
> ...so I'm going to drop it and just add a comment about the
> expectations. As Peter said there's already a multitude of ways to
> cause false positive / negative results with lockdep so this is just
> one more area where one needs to be careful and understand the lock
> context they might be overriding.
One safe-guard might be to check the class you're overriding is indeed
__no_validate__, and WARN if not. Then the unconditional reset is
conistent.
Then, if/when, that WARN ever triggers you can revisit all this.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-25 10:33 ` Peter Zijlstra
@ 2022-04-25 16:05 ` Dan Williams
2022-04-25 18:57 ` Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-25 16:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ira Weiny, linux-cxl, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
Linux NVDIMM, Linux Kernel Mailing List
On Mon, Apr 25, 2022 at 3:33 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Sat, Apr 23, 2022 at 10:27:52AM -0700, Dan Williams wrote:
>
> > ...so I'm going to drop it and just add a comment about the
> > expectations. As Peter said there's already a multitude of ways to
> > cause false positive / negative results with lockdep so this is just
> > one more area where one needs to be careful and understand the lock
> > context they might be overriding.
>
> One safe-guard might be to check the class you're overriding is indeed
> __no_validate__, and WARN if not. Then the unconditional reset is
> conistent.
>
> Then, if/when, that WARN ever triggers you can revisit all this.
Ok, that does seem to need a dummy definition of lockdep_match_class()
in the CONFIG_LOCKDEP=n case, but that seems worth it to me for the
sanity check.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation
2022-04-25 16:05 ` Dan Williams
@ 2022-04-25 18:57 ` Dan Williams
0 siblings, 0 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-25 18:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ira Weiny, linux-cxl, Greg Kroah-Hartman, Rafael J. Wysocki,
Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Alison Schofield, Vishal Verma, Ben Widawsky, Jonathan Cameron,
Linux NVDIMM, Linux Kernel Mailing List
On Mon, Apr 25, 2022 at 9:05 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Apr 25, 2022 at 3:33 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Sat, Apr 23, 2022 at 10:27:52AM -0700, Dan Williams wrote:
> >
> > > ...so I'm going to drop it and just add a comment about the
> > > expectations. As Peter said there's already a multitude of ways to
> > > cause false positive / negative results with lockdep so this is just
> > > one more area where one needs to be careful and understand the lock
> > > context they might be overriding.
> >
> > One safe-guard might be to check the class you're overriding is indeed
> > __no_validate__, and WARN if not. Then the unconditional reset is
> > conistent.
> >
> > Then, if/when, that WARN ever triggers you can revisit all this.
>
> Ok, that does seem to need a dummy definition of lockdep_match_class()
> in the CONFIG_LOCKDEP=n case, but that seems worth it to me for the
> sanity check.
Thankfully the comment in lockdep.h to not define a
lockdep_match_class() for the CONFIG_LOCKDEP=n stopped me from going
that direction.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 2/8] cxl/acpi: Add root device lockdep validation
2022-04-21 15:33 ` [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation Dan Williams
2022-04-21 16:10 ` Greg Kroah-Hartman
2022-04-22 23:58 ` Ira Weiny
@ 2022-04-23 21:05 ` Dan Williams
2022-04-26 4:23 ` [PATCH v5 " Dan Williams
2 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-23 21:05 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Rafael J. Wysocki, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng, Alison Schofield, Vishal Verma,
Ben Widawsky, Jonathan Cameron, Greg Kroah-Hartman, Ira Weiny,
nvdimm
The CXL "root" device, ACPI0017, is an attach point for coordinating
platform level CXL resources and is the parent device for a CXL port
topology tree. As such it has distinct locking rules relative to other
CXL subsystem objects, but because it is an ACPI device the lock class
is established well before it is given to the cxl_acpi driver.
However, the lockdep API does support changing the lock class "live" for
situations like this. Add a device_lock_set_class() helper that a driver
can use in ->probe() to set a custom lock class, and
device_lock_reset_class() to return to the default "no validate" class
before the custom lock class key goes out of scope after ->remove().
Note the helpers are all macros to support dead code elimination in the
CONFIG_PROVE_LOCKING=n case.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- Add a comment about expected usage of device_lock_set_class() to only
override the "no validate" class (Ira)
- Clang-format the macros
- Fix device_lock_reset_class() to reset the lock class name to
"&dev->mutex".
drivers/cxl/acpi.c | 15 +++++++++++++++
include/linux/device.h | 25 +++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..e19cea27387e 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -275,6 +275,15 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+static struct lock_class_key cxl_root_key;
+
+static void cxl_acpi_lock_reset_class(void *_dev)
+{
+ struct device *dev = _dev;
+
+ device_lock_reset_class(dev);
+}
+
static int cxl_acpi_probe(struct platform_device *pdev)
{
int rc;
@@ -283,6 +292,12 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ device_lock_set_class(&pdev->dev, &cxl_root_key);
+ rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
+ &pdev->dev);
+ if (rc)
+ return rc;
+
root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
if (IS_ERR(root_port))
return PTR_ERR(root_port);
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..a4a7c1bf3b72 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -850,6 +850,31 @@ static inline bool device_supports_offline(struct device *dev)
return dev->bus && dev->bus->offline && dev->bus->online;
}
+#define __device_lock_set_class(dev, name, key) \
+ lock_set_class(&(dev)->mutex.dep_map, name, key, 0, _THIS_IP_)
+
+/**
+ * device_lock_set_class - Specify a temporary lock class while a device
+ * is attached to a driver
+ * @dev: device to modify
+ * @key: lock class key data
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->probe(). Take care to only override the default
+ * lockdep_no_validate class.
+ */
+#define device_lock_set_class(dev, key) __device_lock_set_class(dev, #key, key)
+
+/**
+ * device_lock_reset_class - Return a device to the default lockdep novalidate state
+ * @dev: device to modify
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->remove().
+ */
+#define device_lock_reset_class(dev) \
+ __device_lock_set_class(dev, "&dev->mutex", &__lockdep_no_validate__)
+
void lock_device_hotplug(void);
void unlock_device_hotplug(void);
int lock_device_hotplug_sysfs(void);
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v5 2/8] cxl/acpi: Add root device lockdep validation
2022-04-23 21:05 ` [PATCH v4 " Dan Williams
@ 2022-04-26 4:23 ` Dan Williams
2022-04-26 19:22 ` [PATCH v6 " Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-26 4:23 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Rafael J. Wysocki, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng, Alison Schofield, Vishal Verma,
Ben Widawsky, Jonathan Cameron, Greg Kroah-Hartman, Ira Weiny,
nvdimm
The CXL "root" device, ACPI0017, is an attach point for coordinating
platform level CXL resources and is the parent device for a CXL port
topology tree. As such it has distinct locking rules relative to other
CXL subsystem objects, but because it is an ACPI device the lock class
is established well before it is given to the cxl_acpi driver.
However, the lockdep API does support changing the lock class "live" for
situations like this. Add a device_lock_set_class() helper that a driver
can use in ->probe() to set a custom lock class, and
device_lock_reset_class() to return to the default "no validate" class
before the custom lock class key goes out of scope after ->remove().
Note the helpers are all macros to support dead code elimination in the
CONFIG_PROVE_LOCKING=n case, however device_set_lock_class() still needs
#ifdef CONFIG_PROVE_LOCKING since lockdep_match_class() explicitly does
not have a helper in the CONFIG_PROVE_LOCKING=n case (see comment in
lockdep.h). The lockdep API needs 2 small tweaks to prevent "unused"
warnings for the @key argument to lock_set_class(), and a new
lock_set_novalidate_class() is added to supplement
lockdep_set_novalidate_class() in the cases where the lock class is
converted while the lock is held.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v4:
- Rather than attempt to error out when device_set_lock_class() is
overriding an existing lock class, just WARN and revisit if it becomes
a problem. (Peter).
drivers/cxl/acpi.c | 13 +++++++++++++
include/linux/device.h | 43 +++++++++++++++++++++++++++++++++++++++++++
include/linux/lockdep.h | 6 +++++-
3 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..40286f5df812 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -275,6 +275,13 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+static struct lock_class_key cxl_root_key;
+
+static void cxl_acpi_lock_reset_class(void *dev)
+{
+ device_lock_reset_class(dev);
+}
+
static int cxl_acpi_probe(struct platform_device *pdev)
{
int rc;
@@ -283,6 +290,12 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ device_lock_set_class(&pdev->dev, &cxl_root_key);
+ rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
+ &pdev->dev);
+ if (rc)
+ return rc;
+
root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
if (IS_ERR(root_port))
return PTR_ERR(root_port);
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..355127d0de78 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -850,6 +850,49 @@ static inline bool device_supports_offline(struct device *dev)
return dev->bus && dev->bus->offline && dev->bus->online;
}
+#define __device_lock_set_class(dev, name, key) \
+do { \
+ struct device *__d __maybe_unused = dev; \
+ lock_set_class(&__d->mutex.dep_map, name, key, 0, _THIS_IP_); \
+} while (0)
+
+/**
+ * device_lock_set_class - Specify a temporary lock class while a device
+ * is attached to a driver
+ * @dev: device to modify
+ * @key: lock class key data
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->probe(). Take care to only override the default
+ * lockdep_no_validate class.
+ */
+#ifdef CONFIG_LOCKDEP
+#define device_lock_set_class(dev, key) \
+do { \
+ struct device *__d = dev; \
+ dev_WARN_ONCE(__d, !lockdep_match_class(&__d->mutex, \
+ &__lockdep_no_validate__), \
+ "overriding existing custom lock class\n"); \
+ __device_lock_set_class(__d, #key, key); \
+} while (0)
+#else
+#define device_lock_set_class(dev, key) __device_lock_set_class(dev, #key, key)
+#endif
+
+/**
+ * device_lock_reset_class - Return a device to the default lockdep novalidate state
+ * @dev: device to modify
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->remove().
+ */
+#define device_lock_reset_class(dev) \
+do { \
+ struct device *__d __maybe_unused = dev; \
+ lock_set_novalidate_class(&__d->mutex.dep_map, "&dev->mutex", \
+ _THIS_IP_); \
+} while (0)
+
void lock_device_hotplug(void);
void unlock_device_hotplug(void);
int lock_device_hotplug_sysfs(void);
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 467b94257105..43b0dc6a0b21 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -290,6 +290,9 @@ extern void lock_set_class(struct lockdep_map *lock, const char *name,
struct lock_class_key *key, unsigned int subclass,
unsigned long ip);
+#define lock_set_novalidate_class(l, n, i) \
+ lock_set_class(l, n, &__lockdep_no_validate__, 0, i)
+
static inline void lock_set_subclass(struct lockdep_map *lock,
unsigned int subclass, unsigned long ip)
{
@@ -357,7 +360,8 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
# define lock_acquire(l, s, t, r, c, n, i) do { } while (0)
# define lock_release(l, i) do { } while (0)
# define lock_downgrade(l, i) do { } while (0)
-# define lock_set_class(l, n, k, s, i) do { } while (0)
+# define lock_set_class(l, n, key, s, i) do { (void)(key); } while (0)
+# define lock_set_novalidate_class(l, n, i) do { } while (0)
# define lock_set_subclass(l, s, i) do { } while (0)
# define lockdep_init() do { } while (0)
# define lockdep_init_map_type(lock, name, key, sub, inner, outer, type) \
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v6 2/8] cxl/acpi: Add root device lockdep validation
2022-04-26 4:23 ` [PATCH v5 " Dan Williams
@ 2022-04-26 19:22 ` Dan Williams
0 siblings, 0 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-26 19:22 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Rafael J. Wysocki, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng, Alison Schofield, Vishal Verma,
Ben Widawsky, Jonathan Cameron, Greg Kroah-Hartman, Ira Weiny,
nvdimm
The CXL "root" device, ACPI0017, is an attach point for coordinating
platform level CXL resources and is the parent device for a CXL port
topology tree. As such it has distinct locking rules relative to other
CXL subsystem objects, but because it is an ACPI device the lock class
is established well before it is given to the cxl_acpi driver.
However, the lockdep API does support changing the lock class "live" for
situations like this. Add a device_lock_set_class() helper that a driver
can use in ->probe() to set a custom lock class, and
device_lock_reset_class() to return to the default "no validate" class
before the custom lock class key goes out of scope after ->remove().
Note the helpers are all macros to support dead code elimination in the
CONFIG_PROVE_LOCKING=n case, however device_set_lock_class() still needs
#ifdef CONFIG_PROVE_LOCKING since lockdep_match_class() explicitly does
not have a helper in the CONFIG_PROVE_LOCKING=n case (see comment in
lockdep.h). The lockdep API needs 2 small tweaks to prevent "unused"
warnings for the @key argument to lock_set_class(), and a new
lock_set_novalidate_class() is added to supplement
lockdep_set_novalidate_class() in the cases where the lock class is
converted while the lock is held.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v5:
- 0day reports [1] that clang does not like how the macros shadow the
definition of the @__d variable. Move __device_lock_set_class() to a
unique variable name. Note that the variable is needed as some macro
callers may pass a 'void *' to device_set_lock_class().
[1]: https://lore.kernel.org/all/202204261758.lzXWne7H-lkp@intel.com/
drivers/cxl/acpi.c | 13 +++++++++++++
include/linux/device.h | 43 +++++++++++++++++++++++++++++++++++++++++++
include/linux/lockdep.h | 6 +++++-
3 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..40286f5df812 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -275,6 +275,13 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+static struct lock_class_key cxl_root_key;
+
+static void cxl_acpi_lock_reset_class(void *dev)
+{
+ device_lock_reset_class(dev);
+}
+
static int cxl_acpi_probe(struct platform_device *pdev)
{
int rc;
@@ -283,6 +290,12 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ device_lock_set_class(&pdev->dev, &cxl_root_key);
+ rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
+ &pdev->dev);
+ if (rc)
+ return rc;
+
root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
if (IS_ERR(root_port))
return PTR_ERR(root_port);
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..833b0b3b0193 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -850,6 +850,49 @@ static inline bool device_supports_offline(struct device *dev)
return dev->bus && dev->bus->offline && dev->bus->online;
}
+#define __device_lock_set_class(dev, name, key) \
+do { \
+ struct device *__d2 __maybe_unused = dev; \
+ lock_set_class(&__d2->mutex.dep_map, name, key, 0, _THIS_IP_); \
+} while (0)
+
+/**
+ * device_lock_set_class - Specify a temporary lock class while a device
+ * is attached to a driver
+ * @dev: device to modify
+ * @key: lock class key data
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->probe(). Take care to only override the default
+ * lockdep_no_validate class.
+ */
+#ifdef CONFIG_LOCKDEP
+#define device_lock_set_class(dev, key) \
+do { \
+ struct device *__d = dev; \
+ dev_WARN_ONCE(__d, !lockdep_match_class(&__d->mutex, \
+ &__lockdep_no_validate__), \
+ "overriding existing custom lock class\n"); \
+ __device_lock_set_class(__d, #key, key); \
+} while (0)
+#else
+#define device_lock_set_class(dev, key) __device_lock_set_class(dev, #key, key)
+#endif
+
+/**
+ * device_lock_reset_class - Return a device to the default lockdep novalidate state
+ * @dev: device to modify
+ *
+ * This must be called with the device_lock() already held, for example
+ * from driver ->remove().
+ */
+#define device_lock_reset_class(dev) \
+do { \
+ struct device *__d __maybe_unused = dev; \
+ lock_set_novalidate_class(&__d->mutex.dep_map, "&dev->mutex", \
+ _THIS_IP_); \
+} while (0)
+
void lock_device_hotplug(void);
void unlock_device_hotplug(void);
int lock_device_hotplug_sysfs(void);
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 467b94257105..43b0dc6a0b21 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -290,6 +290,9 @@ extern void lock_set_class(struct lockdep_map *lock, const char *name,
struct lock_class_key *key, unsigned int subclass,
unsigned long ip);
+#define lock_set_novalidate_class(l, n, i) \
+ lock_set_class(l, n, &__lockdep_no_validate__, 0, i)
+
static inline void lock_set_subclass(struct lockdep_map *lock,
unsigned int subclass, unsigned long ip)
{
@@ -357,7 +360,8 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
# define lock_acquire(l, s, t, r, c, n, i) do { } while (0)
# define lock_release(l, i) do { } while (0)
# define lock_downgrade(l, i) do { } while (0)
-# define lock_set_class(l, n, k, s, i) do { } while (0)
+# define lock_set_class(l, n, key, s, i) do { (void)(key); } while (0)
+# define lock_set_novalidate_class(l, n, i) do { } while (0)
# define lock_set_subclass(l, s, i) do { } while (0)
# define lockdep_init() do { } while (0)
# define lockdep_init_map_type(lock, name, key, sub, inner, outer, type) \
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 3/8] cxl: Drop cxl_device_lock()
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
2022-04-21 15:33 ` [PATCH v3 1/8] cxl: Replace lockdep_mutex with local lock classes Dan Williams
2022-04-21 15:33 ` [PATCH v3 2/8] cxl/acpi: Add root device lockdep validation Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-23 0:07 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 4/8] nvdimm: Replace lockdep_mutex with local lock classes Dan Williams
` (4 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky,
Jonathan Cameron, peterz, nvdimm, linux-kernel
Now that all CXL subsystem locking is validated with custom lock
classes, there is no need for the custom usage of the lockdep_mutex.
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/pmem.c | 4 +-
drivers/cxl/core/port.c | 55 ++++++++++++++-------------------
drivers/cxl/cxl.h | 78 -----------------------------------------------
drivers/cxl/mem.c | 4 +-
drivers/cxl/pmem.c | 12 ++++---
lib/Kconfig.debug | 6 ----
6 files changed, 33 insertions(+), 126 deletions(-)
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index e825e261278d..bec7cfb54ebf 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -124,10 +124,10 @@ static void unregister_nvb(void *_cxl_nvb)
* work to flush. Once the state has been changed to 'dead' then no new
* work can be queued by user-triggered bind.
*/
- cxl_device_lock(&cxl_nvb->dev);
+ device_lock(&cxl_nvb->dev);
flush = cxl_nvb->state != CXL_NVB_NEW;
cxl_nvb->state = CXL_NVB_DEAD;
- cxl_device_unlock(&cxl_nvb->dev);
+ device_unlock(&cxl_nvb->dev);
/*
* Even though the device core will trigger device_release_driver()
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 750aac95ed5f..ea60abda6500 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -312,10 +312,10 @@ static void cxl_port_release(struct device *dev)
struct cxl_port *port = to_cxl_port(dev);
struct cxl_ep *ep, *_e;
- cxl_device_lock(dev);
+ device_lock(dev);
list_for_each_entry_safe(ep, _e, &port->endpoints, list)
cxl_ep_release(ep);
- cxl_device_unlock(dev);
+ device_unlock(dev);
ida_free(&cxl_port_ida, port->id);
kfree(port);
}
@@ -556,7 +556,7 @@ static int match_root_child(struct device *dev, const void *match)
return 0;
port = to_cxl_port(dev);
- cxl_device_lock(dev);
+ device_lock(dev);
list_for_each_entry(dport, &port->dports, list) {
iter = match;
while (iter) {
@@ -566,7 +566,7 @@ static int match_root_child(struct device *dev, const void *match)
}
}
out:
- cxl_device_unlock(dev);
+ device_unlock(dev);
return !!iter;
}
@@ -625,13 +625,13 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new)
static void cond_cxl_root_lock(struct cxl_port *port)
{
if (is_cxl_root(port))
- cxl_device_lock(&port->dev);
+ device_lock(&port->dev);
}
static void cond_cxl_root_unlock(struct cxl_port *port)
{
if (is_cxl_root(port))
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
}
static void cxl_dport_remove(void *data)
@@ -738,15 +738,15 @@ static int add_ep(struct cxl_port *port, struct cxl_ep *new)
{
struct cxl_ep *dup;
- cxl_device_lock(&port->dev);
+ device_lock(&port->dev);
if (port->dead) {
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
return -ENXIO;
}
dup = find_ep(port, new->ep);
if (!dup)
list_add_tail(&new->list, &port->endpoints);
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
return dup ? -EEXIST : 0;
}
@@ -856,12 +856,12 @@ static void delete_endpoint(void *data)
goto out;
parent = &parent_port->dev;
- cxl_device_lock(parent);
+ device_lock(parent);
if (parent->driver && endpoint->uport) {
devm_release_action(parent, cxl_unlink_uport, endpoint);
devm_release_action(parent, unregister_port, endpoint);
}
- cxl_device_unlock(parent);
+ device_unlock(parent);
put_device(parent);
out:
put_device(&endpoint->dev);
@@ -922,7 +922,7 @@ static void cxl_detach_ep(void *data)
}
parent_port = to_cxl_port(port->dev.parent);
- cxl_device_lock(&parent_port->dev);
+ device_lock(&parent_port->dev);
if (!parent_port->dev.driver) {
/*
* The bottom-up race to delete the port lost to a
@@ -930,12 +930,12 @@ static void cxl_detach_ep(void *data)
* parent_port ->remove() will have cleaned up all
* descendants.
*/
- cxl_device_unlock(&parent_port->dev);
+ device_unlock(&parent_port->dev);
put_device(&port->dev);
continue;
}
- cxl_device_lock(&port->dev);
+ device_lock(&port->dev);
ep = find_ep(port, &cxlmd->dev);
dev_dbg(&cxlmd->dev, "disconnect %s from %s\n",
ep ? dev_name(ep->ep) : "", dev_name(&port->dev));
@@ -950,7 +950,7 @@ static void cxl_detach_ep(void *data)
port->dead = true;
list_splice_init(&port->dports, &reap_dports);
}
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
if (!list_empty(&reap_dports)) {
dev_dbg(&cxlmd->dev, "delete %s\n",
@@ -958,7 +958,7 @@ static void cxl_detach_ep(void *data)
delete_switch_port(port, &reap_dports);
}
put_device(&port->dev);
- cxl_device_unlock(&parent_port->dev);
+ device_unlock(&parent_port->dev);
}
}
@@ -1006,7 +1006,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
return -EAGAIN;
}
- cxl_device_lock(&parent_port->dev);
+ device_lock(&parent_port->dev);
if (!parent_port->dev.driver) {
dev_warn(&cxlmd->dev,
"port %s:%s disabled, failed to enumerate CXL.mem\n",
@@ -1024,7 +1024,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
get_device(&port->dev);
}
out:
- cxl_device_unlock(&parent_port->dev);
+ device_unlock(&parent_port->dev);
if (IS_ERR(port))
rc = PTR_ERR(port);
@@ -1135,14 +1135,14 @@ struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
{
struct cxl_dport *dport;
- cxl_device_lock(&port->dev);
+ device_lock(&port->dev);
list_for_each_entry(dport, &port->dports, list)
if (dport->dport == dev) {
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
return dport;
}
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
return NULL;
}
EXPORT_SYMBOL_NS_GPL(cxl_find_dport_by_dev, CXL);
@@ -1384,9 +1384,9 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
port = to_cxl_port(cxld->dev.parent);
- cxl_device_lock(&port->dev);
+ device_lock(&port->dev);
rc = cxl_decoder_add_locked(cxld, target_map);
- cxl_device_unlock(&port->dev);
+ device_unlock(&port->dev);
return rc;
}
@@ -1457,14 +1457,7 @@ static int cxl_bus_probe(struct device *dev)
{
int rc;
- /*
- * Take the CXL nested lock since the driver core only holds
- * @dev->mutex and not @dev->lockdep_mutex.
- */
- cxl_nested_lock(dev);
rc = to_cxl_drv(dev->driver)->probe(dev);
- cxl_nested_unlock(dev);
-
dev_dbg(dev, "probe: %d\n", rc);
return rc;
}
@@ -1473,10 +1466,8 @@ static void cxl_bus_remove(struct device *dev)
{
struct cxl_driver *cxl_drv = to_cxl_drv(dev->driver);
- cxl_nested_lock(dev);
if (cxl_drv->remove)
cxl_drv->remove(dev);
- cxl_nested_unlock(dev);
}
static struct workqueue_struct *cxl_bus_wq;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 990b6670222e..140dc3278cde 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -405,82 +405,4 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd);
#define __mock static
#endif
-#ifdef CONFIG_PROVE_CXL_LOCKING
-enum cxl_lock_class {
- CXL_ANON_LOCK,
- CXL_NVDIMM_LOCK,
- CXL_NVDIMM_BRIDGE_LOCK,
- CXL_PORT_LOCK,
- /*
- * Be careful to add new lock classes here, CXL_PORT_LOCK is
- * extended by the port depth, so a maximum CXL port topology
- * depth would need to be defined first.
- */
-};
-
-static inline void cxl_nested_lock(struct device *dev)
-{
- if (is_cxl_port(dev)) {
- struct cxl_port *port = to_cxl_port(dev);
-
- mutex_lock_nested(&dev->lockdep_mutex,
- CXL_PORT_LOCK + port->depth);
- } else if (is_cxl_decoder(dev)) {
- struct cxl_port *port = to_cxl_port(dev->parent);
-
- /*
- * A decoder is the immediate child of a port, so set
- * its lock class equal to other child device siblings.
- */
- mutex_lock_nested(&dev->lockdep_mutex,
- CXL_PORT_LOCK + port->depth + 1);
- } else if (is_cxl_nvdimm_bridge(dev))
- mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_BRIDGE_LOCK);
- else if (is_cxl_nvdimm(dev))
- mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_LOCK);
- else
- mutex_lock_nested(&dev->lockdep_mutex, CXL_ANON_LOCK);
-}
-
-static inline void cxl_nested_unlock(struct device *dev)
-{
- mutex_unlock(&dev->lockdep_mutex);
-}
-
-static inline void cxl_device_lock(struct device *dev)
-{
- /*
- * For double lock errors the lockup will happen before lockdep
- * warns at cxl_nested_lock(), so assert explicitly.
- */
- lockdep_assert_not_held(&dev->lockdep_mutex);
-
- device_lock(dev);
- cxl_nested_lock(dev);
-}
-
-static inline void cxl_device_unlock(struct device *dev)
-{
- cxl_nested_unlock(dev);
- device_unlock(dev);
-}
-#else
-static inline void cxl_nested_lock(struct device *dev)
-{
-}
-
-static inline void cxl_nested_unlock(struct device *dev)
-{
-}
-
-static inline void cxl_device_lock(struct device *dev)
-{
- device_lock(dev);
-}
-
-static inline void cxl_device_unlock(struct device *dev)
-{
- device_unlock(dev);
-}
-#endif
#endif /* __CXL_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 49a4b1c47299..91fb8d5b21a7 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -195,7 +195,7 @@ static int cxl_mem_probe(struct device *dev)
return -ENXIO;
}
- cxl_device_lock(&parent_port->dev);
+ device_lock(&parent_port->dev);
if (!parent_port->dev.driver) {
dev_err(dev, "CXL port topology %s not enabled\n",
dev_name(&parent_port->dev));
@@ -205,7 +205,7 @@ static int cxl_mem_probe(struct device *dev)
rc = create_endpoint(cxlmd, parent_port);
out:
- cxl_device_unlock(&parent_port->dev);
+ device_unlock(&parent_port->dev);
put_device(&parent_port->dev);
return rc;
}
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 15ad666ab03e..b65a272a2d6d 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -43,7 +43,7 @@ static int cxl_nvdimm_probe(struct device *dev)
if (!cxl_nvb)
return -ENXIO;
- cxl_device_lock(&cxl_nvb->dev);
+ device_lock(&cxl_nvb->dev);
if (!cxl_nvb->nvdimm_bus) {
rc = -ENXIO;
goto out;
@@ -68,7 +68,7 @@ static int cxl_nvdimm_probe(struct device *dev)
dev_set_drvdata(dev, nvdimm);
rc = devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
out:
- cxl_device_unlock(&cxl_nvb->dev);
+ device_unlock(&cxl_nvb->dev);
put_device(&cxl_nvb->dev);
return rc;
@@ -233,7 +233,7 @@ static void cxl_nvb_update_state(struct work_struct *work)
struct nvdimm_bus *victim_bus = NULL;
bool release = false, rescan = false;
- cxl_device_lock(&cxl_nvb->dev);
+ device_lock(&cxl_nvb->dev);
switch (cxl_nvb->state) {
case CXL_NVB_ONLINE:
if (!online_nvdimm_bus(cxl_nvb)) {
@@ -251,7 +251,7 @@ static void cxl_nvb_update_state(struct work_struct *work)
default:
break;
}
- cxl_device_unlock(&cxl_nvb->dev);
+ device_unlock(&cxl_nvb->dev);
if (release)
device_release_driver(&cxl_nvb->dev);
@@ -327,9 +327,9 @@ static int cxl_nvdimm_bridge_reset(struct device *dev, void *data)
return 0;
cxl_nvb = to_cxl_nvdimm_bridge(dev);
- cxl_device_lock(dev);
+ device_lock(dev);
cxl_nvb->state = CXL_NVB_NEW;
- cxl_device_unlock(dev);
+ device_unlock(dev);
return 0;
}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 075cd25363ac..4a2c853c948b 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1559,12 +1559,6 @@ config PROVE_NVDIMM_LOCKING
help
Enable lockdep to validate nd_device_lock() usage.
-config PROVE_CXL_LOCKING
- bool "CXL"
- depends on CXL_BUS
- help
- Enable lockdep to validate cxl_device_lock() usage.
-
endchoice
endmenu # lock debugging
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 3/8] cxl: Drop cxl_device_lock()
2022-04-21 15:33 ` [PATCH v3 3/8] cxl: Drop cxl_device_lock() Dan Williams
@ 2022-04-23 0:07 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 0:07 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Alison Schofield, Vishal Verma, Ben Widawsky,
Jonathan Cameron, peterz, nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:23AM -0700, Dan Williams wrote:
> Now that all CXL subsystem locking is validated with custom lock
> classes, there is no need for the custom usage of the lockdep_mutex.
>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/cxl/core/pmem.c | 4 +-
> drivers/cxl/core/port.c | 55 ++++++++++++++-------------------
> drivers/cxl/cxl.h | 78 -----------------------------------------------
> drivers/cxl/mem.c | 4 +-
> drivers/cxl/pmem.c | 12 ++++---
> lib/Kconfig.debug | 6 ----
> 6 files changed, 33 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index e825e261278d..bec7cfb54ebf 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -124,10 +124,10 @@ static void unregister_nvb(void *_cxl_nvb)
> * work to flush. Once the state has been changed to 'dead' then no new
> * work can be queued by user-triggered bind.
> */
> - cxl_device_lock(&cxl_nvb->dev);
> + device_lock(&cxl_nvb->dev);
> flush = cxl_nvb->state != CXL_NVB_NEW;
> cxl_nvb->state = CXL_NVB_DEAD;
> - cxl_device_unlock(&cxl_nvb->dev);
> + device_unlock(&cxl_nvb->dev);
>
> /*
> * Even though the device core will trigger device_release_driver()
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 750aac95ed5f..ea60abda6500 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -312,10 +312,10 @@ static void cxl_port_release(struct device *dev)
> struct cxl_port *port = to_cxl_port(dev);
> struct cxl_ep *ep, *_e;
>
> - cxl_device_lock(dev);
> + device_lock(dev);
> list_for_each_entry_safe(ep, _e, &port->endpoints, list)
> cxl_ep_release(ep);
> - cxl_device_unlock(dev);
> + device_unlock(dev);
> ida_free(&cxl_port_ida, port->id);
> kfree(port);
> }
> @@ -556,7 +556,7 @@ static int match_root_child(struct device *dev, const void *match)
> return 0;
>
> port = to_cxl_port(dev);
> - cxl_device_lock(dev);
> + device_lock(dev);
> list_for_each_entry(dport, &port->dports, list) {
> iter = match;
> while (iter) {
> @@ -566,7 +566,7 @@ static int match_root_child(struct device *dev, const void *match)
> }
> }
> out:
> - cxl_device_unlock(dev);
> + device_unlock(dev);
>
> return !!iter;
> }
> @@ -625,13 +625,13 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> static void cond_cxl_root_lock(struct cxl_port *port)
> {
> if (is_cxl_root(port))
> - cxl_device_lock(&port->dev);
> + device_lock(&port->dev);
> }
>
> static void cond_cxl_root_unlock(struct cxl_port *port)
> {
> if (is_cxl_root(port))
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
> }
>
> static void cxl_dport_remove(void *data)
> @@ -738,15 +738,15 @@ static int add_ep(struct cxl_port *port, struct cxl_ep *new)
> {
> struct cxl_ep *dup;
>
> - cxl_device_lock(&port->dev);
> + device_lock(&port->dev);
> if (port->dead) {
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
> return -ENXIO;
> }
> dup = find_ep(port, new->ep);
> if (!dup)
> list_add_tail(&new->list, &port->endpoints);
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
>
> return dup ? -EEXIST : 0;
> }
> @@ -856,12 +856,12 @@ static void delete_endpoint(void *data)
> goto out;
> parent = &parent_port->dev;
>
> - cxl_device_lock(parent);
> + device_lock(parent);
> if (parent->driver && endpoint->uport) {
> devm_release_action(parent, cxl_unlink_uport, endpoint);
> devm_release_action(parent, unregister_port, endpoint);
> }
> - cxl_device_unlock(parent);
> + device_unlock(parent);
> put_device(parent);
> out:
> put_device(&endpoint->dev);
> @@ -922,7 +922,7 @@ static void cxl_detach_ep(void *data)
> }
>
> parent_port = to_cxl_port(port->dev.parent);
> - cxl_device_lock(&parent_port->dev);
> + device_lock(&parent_port->dev);
> if (!parent_port->dev.driver) {
> /*
> * The bottom-up race to delete the port lost to a
> @@ -930,12 +930,12 @@ static void cxl_detach_ep(void *data)
> * parent_port ->remove() will have cleaned up all
> * descendants.
> */
> - cxl_device_unlock(&parent_port->dev);
> + device_unlock(&parent_port->dev);
> put_device(&port->dev);
> continue;
> }
>
> - cxl_device_lock(&port->dev);
> + device_lock(&port->dev);
> ep = find_ep(port, &cxlmd->dev);
> dev_dbg(&cxlmd->dev, "disconnect %s from %s\n",
> ep ? dev_name(ep->ep) : "", dev_name(&port->dev));
> @@ -950,7 +950,7 @@ static void cxl_detach_ep(void *data)
> port->dead = true;
> list_splice_init(&port->dports, &reap_dports);
> }
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
>
> if (!list_empty(&reap_dports)) {
> dev_dbg(&cxlmd->dev, "delete %s\n",
> @@ -958,7 +958,7 @@ static void cxl_detach_ep(void *data)
> delete_switch_port(port, &reap_dports);
> }
> put_device(&port->dev);
> - cxl_device_unlock(&parent_port->dev);
> + device_unlock(&parent_port->dev);
> }
> }
>
> @@ -1006,7 +1006,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> return -EAGAIN;
> }
>
> - cxl_device_lock(&parent_port->dev);
> + device_lock(&parent_port->dev);
> if (!parent_port->dev.driver) {
> dev_warn(&cxlmd->dev,
> "port %s:%s disabled, failed to enumerate CXL.mem\n",
> @@ -1024,7 +1024,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> get_device(&port->dev);
> }
> out:
> - cxl_device_unlock(&parent_port->dev);
> + device_unlock(&parent_port->dev);
>
> if (IS_ERR(port))
> rc = PTR_ERR(port);
> @@ -1135,14 +1135,14 @@ struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
> {
> struct cxl_dport *dport;
>
> - cxl_device_lock(&port->dev);
> + device_lock(&port->dev);
> list_for_each_entry(dport, &port->dports, list)
> if (dport->dport == dev) {
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
> return dport;
> }
>
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
> return NULL;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_dport_by_dev, CXL);
> @@ -1384,9 +1384,9 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>
> port = to_cxl_port(cxld->dev.parent);
>
> - cxl_device_lock(&port->dev);
> + device_lock(&port->dev);
> rc = cxl_decoder_add_locked(cxld, target_map);
> - cxl_device_unlock(&port->dev);
> + device_unlock(&port->dev);
>
> return rc;
> }
> @@ -1457,14 +1457,7 @@ static int cxl_bus_probe(struct device *dev)
> {
> int rc;
>
> - /*
> - * Take the CXL nested lock since the driver core only holds
> - * @dev->mutex and not @dev->lockdep_mutex.
> - */
> - cxl_nested_lock(dev);
> rc = to_cxl_drv(dev->driver)->probe(dev);
> - cxl_nested_unlock(dev);
> -
> dev_dbg(dev, "probe: %d\n", rc);
> return rc;
> }
> @@ -1473,10 +1466,8 @@ static void cxl_bus_remove(struct device *dev)
> {
> struct cxl_driver *cxl_drv = to_cxl_drv(dev->driver);
>
> - cxl_nested_lock(dev);
> if (cxl_drv->remove)
> cxl_drv->remove(dev);
> - cxl_nested_unlock(dev);
> }
>
> static struct workqueue_struct *cxl_bus_wq;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 990b6670222e..140dc3278cde 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -405,82 +405,4 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd);
> #define __mock static
> #endif
>
> -#ifdef CONFIG_PROVE_CXL_LOCKING
> -enum cxl_lock_class {
> - CXL_ANON_LOCK,
> - CXL_NVDIMM_LOCK,
> - CXL_NVDIMM_BRIDGE_LOCK,
> - CXL_PORT_LOCK,
> - /*
> - * Be careful to add new lock classes here, CXL_PORT_LOCK is
> - * extended by the port depth, so a maximum CXL port topology
> - * depth would need to be defined first.
> - */
> -};
> -
> -static inline void cxl_nested_lock(struct device *dev)
> -{
> - if (is_cxl_port(dev)) {
> - struct cxl_port *port = to_cxl_port(dev);
> -
> - mutex_lock_nested(&dev->lockdep_mutex,
> - CXL_PORT_LOCK + port->depth);
> - } else if (is_cxl_decoder(dev)) {
> - struct cxl_port *port = to_cxl_port(dev->parent);
> -
> - /*
> - * A decoder is the immediate child of a port, so set
> - * its lock class equal to other child device siblings.
> - */
> - mutex_lock_nested(&dev->lockdep_mutex,
> - CXL_PORT_LOCK + port->depth + 1);
> - } else if (is_cxl_nvdimm_bridge(dev))
> - mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_BRIDGE_LOCK);
> - else if (is_cxl_nvdimm(dev))
> - mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_LOCK);
> - else
> - mutex_lock_nested(&dev->lockdep_mutex, CXL_ANON_LOCK);
> -}
> -
> -static inline void cxl_nested_unlock(struct device *dev)
> -{
> - mutex_unlock(&dev->lockdep_mutex);
> -}
> -
> -static inline void cxl_device_lock(struct device *dev)
> -{
> - /*
> - * For double lock errors the lockup will happen before lockdep
> - * warns at cxl_nested_lock(), so assert explicitly.
> - */
> - lockdep_assert_not_held(&dev->lockdep_mutex);
> -
> - device_lock(dev);
> - cxl_nested_lock(dev);
> -}
> -
> -static inline void cxl_device_unlock(struct device *dev)
> -{
> - cxl_nested_unlock(dev);
> - device_unlock(dev);
> -}
> -#else
> -static inline void cxl_nested_lock(struct device *dev)
> -{
> -}
> -
> -static inline void cxl_nested_unlock(struct device *dev)
> -{
> -}
> -
> -static inline void cxl_device_lock(struct device *dev)
> -{
> - device_lock(dev);
> -}
> -
> -static inline void cxl_device_unlock(struct device *dev)
> -{
> - device_unlock(dev);
> -}
> -#endif
> #endif /* __CXL_H__ */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 49a4b1c47299..91fb8d5b21a7 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -195,7 +195,7 @@ static int cxl_mem_probe(struct device *dev)
> return -ENXIO;
> }
>
> - cxl_device_lock(&parent_port->dev);
> + device_lock(&parent_port->dev);
> if (!parent_port->dev.driver) {
> dev_err(dev, "CXL port topology %s not enabled\n",
> dev_name(&parent_port->dev));
> @@ -205,7 +205,7 @@ static int cxl_mem_probe(struct device *dev)
>
> rc = create_endpoint(cxlmd, parent_port);
> out:
> - cxl_device_unlock(&parent_port->dev);
> + device_unlock(&parent_port->dev);
> put_device(&parent_port->dev);
> return rc;
> }
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 15ad666ab03e..b65a272a2d6d 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -43,7 +43,7 @@ static int cxl_nvdimm_probe(struct device *dev)
> if (!cxl_nvb)
> return -ENXIO;
>
> - cxl_device_lock(&cxl_nvb->dev);
> + device_lock(&cxl_nvb->dev);
> if (!cxl_nvb->nvdimm_bus) {
> rc = -ENXIO;
> goto out;
> @@ -68,7 +68,7 @@ static int cxl_nvdimm_probe(struct device *dev)
> dev_set_drvdata(dev, nvdimm);
> rc = devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
> out:
> - cxl_device_unlock(&cxl_nvb->dev);
> + device_unlock(&cxl_nvb->dev);
> put_device(&cxl_nvb->dev);
>
> return rc;
> @@ -233,7 +233,7 @@ static void cxl_nvb_update_state(struct work_struct *work)
> struct nvdimm_bus *victim_bus = NULL;
> bool release = false, rescan = false;
>
> - cxl_device_lock(&cxl_nvb->dev);
> + device_lock(&cxl_nvb->dev);
> switch (cxl_nvb->state) {
> case CXL_NVB_ONLINE:
> if (!online_nvdimm_bus(cxl_nvb)) {
> @@ -251,7 +251,7 @@ static void cxl_nvb_update_state(struct work_struct *work)
> default:
> break;
> }
> - cxl_device_unlock(&cxl_nvb->dev);
> + device_unlock(&cxl_nvb->dev);
>
> if (release)
> device_release_driver(&cxl_nvb->dev);
> @@ -327,9 +327,9 @@ static int cxl_nvdimm_bridge_reset(struct device *dev, void *data)
> return 0;
>
> cxl_nvb = to_cxl_nvdimm_bridge(dev);
> - cxl_device_lock(dev);
> + device_lock(dev);
> cxl_nvb->state = CXL_NVB_NEW;
> - cxl_device_unlock(dev);
> + device_unlock(dev);
>
> return 0;
> }
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 075cd25363ac..4a2c853c948b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1559,12 +1559,6 @@ config PROVE_NVDIMM_LOCKING
> help
> Enable lockdep to validate nd_device_lock() usage.
>
> -config PROVE_CXL_LOCKING
> - bool "CXL"
> - depends on CXL_BUS
> - help
> - Enable lockdep to validate cxl_device_lock() usage.
> -
> endchoice
>
> endmenu # lock debugging
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 4/8] nvdimm: Replace lockdep_mutex with local lock classes
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
` (2 preceding siblings ...)
2022-04-21 15:33 ` [PATCH v3 3/8] cxl: Drop cxl_device_lock() Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-23 0:19 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 5/8] ACPI: NFIT: Drop nfit_device_lock() Dan Williams
` (3 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Peter Zijlstra, Vishal Verma, Dave Jiang, Ira Weiny,
alison.schofield, nvdimm, linux-kernel
In response to an attempt to expand dev->lockdep_mutex for device_lock()
validation [1], Peter points out [2] that the lockdep API already has
the ability to assign a dedicated lock class per subsystem device-type.
Use lockdep_set_class() to override the default device_lock()
'__lockdep_no_validate__' class for each NVDIMM subsystem device-type. This
enables lockdep to detect deadlocks and recursive locking within the
device-driver core and the subsystem.
Link: https://lore.kernel.org/r/164982968798.684294.15817853329823976469.stgit@dwillia2-desk3.amr.corp.intel.com [1]
Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [2]
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/nvdimm/btt_devs.c | 7 +++++--
drivers/nvdimm/bus.c | 14 +++++++-------
drivers/nvdimm/dax_devs.c | 4 ++--
drivers/nvdimm/dimm_devs.c | 4 ++++
drivers/nvdimm/namespace_devs.c | 10 +++++++++-
drivers/nvdimm/nd-core.h | 2 +-
drivers/nvdimm/pfn_devs.c | 7 +++++--
drivers/nvdimm/region_devs.c | 4 ++++
8 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index e5a58520d398..120821796668 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -178,6 +178,8 @@ bool is_nd_btt(struct device *dev)
}
EXPORT_SYMBOL(is_nd_btt);
+static struct lock_class_key nvdimm_btt_key;
+
static struct device *__nd_btt_create(struct nd_region *nd_region,
unsigned long lbasize, uuid_t *uuid,
struct nd_namespace_common *ndns)
@@ -205,6 +207,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
dev->parent = &nd_region->dev;
dev->type = &nd_btt_device_type;
device_initialize(&nd_btt->dev);
+ lockdep_set_class(&nd_btt->dev.mutex, &nvdimm_btt_key);
if (ndns && !__nd_attach_ndns(&nd_btt->dev, ndns, &nd_btt->ndns)) {
dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
dev_name(ndns->claim));
@@ -225,7 +228,7 @@ struct device *nd_btt_create(struct nd_region *nd_region)
{
struct device *dev = __nd_btt_create(nd_region, 0, NULL, NULL);
- __nd_device_register(dev);
+ nd_device_register(dev);
return dev;
}
@@ -324,7 +327,7 @@ static int __nd_btt_probe(struct nd_btt *nd_btt,
if (!nd_btt->uuid)
return -ENOMEM;
- __nd_device_register(&nd_btt->dev);
+ nd_device_register(&nd_btt->dev);
return 0;
}
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7b0d1443217a..85ffa04e93c2 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -334,6 +334,8 @@ struct nvdimm_bus *nvdimm_to_bus(struct nvdimm *nvdimm)
}
EXPORT_SYMBOL_GPL(nvdimm_to_bus);
+static struct lock_class_key nvdimm_bus_key;
+
struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
struct nvdimm_bus_descriptor *nd_desc)
{
@@ -360,6 +362,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
nvdimm_bus->dev.bus = &nvdimm_bus_type;
nvdimm_bus->dev.of_node = nd_desc->of_node;
device_initialize(&nvdimm_bus->dev);
+ lockdep_set_class(&nvdimm_bus->dev.mutex, &nvdimm_bus_key);
device_set_pm_not_required(&nvdimm_bus->dev);
rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id);
if (rc)
@@ -511,7 +514,7 @@ static void nd_async_device_unregister(void *d, async_cookie_t cookie)
put_device(dev);
}
-void __nd_device_register(struct device *dev)
+void nd_device_register(struct device *dev)
{
if (!dev)
return;
@@ -537,12 +540,6 @@ void __nd_device_register(struct device *dev)
async_schedule_dev_domain(nd_async_device_register, dev,
&nd_async_domain);
}
-
-void nd_device_register(struct device *dev)
-{
- device_initialize(dev);
- __nd_device_register(dev);
-}
EXPORT_SYMBOL(nd_device_register);
void nd_device_unregister(struct device *dev, enum nd_async_mode mode)
@@ -724,6 +721,8 @@ static void ndctl_release(struct device *dev)
kfree(dev);
}
+static struct lock_class_key nvdimm_ndctl_key;
+
int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
{
dev_t devt = MKDEV(nvdimm_bus_major, nvdimm_bus->id);
@@ -734,6 +733,7 @@ int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
if (!dev)
return -ENOMEM;
device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &nvdimm_ndctl_key);
device_set_pm_not_required(dev);
dev->class = nd_class;
dev->parent = &nvdimm_bus->dev;
diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
index 99965077bac4..7f4a9d28b670 100644
--- a/drivers/nvdimm/dax_devs.c
+++ b/drivers/nvdimm/dax_devs.c
@@ -80,7 +80,7 @@ struct device *nd_dax_create(struct nd_region *nd_region)
nd_dax = nd_dax_alloc(nd_region);
if (nd_dax)
dev = nd_pfn_devinit(&nd_dax->nd_pfn, NULL);
- __nd_device_register(dev);
+ nd_device_register(dev);
return dev;
}
@@ -119,7 +119,7 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
nd_detach_ndns(dax_dev, &nd_pfn->ndns);
put_device(dax_dev);
} else
- __nd_device_register(dax_dev);
+ nd_device_register(dax_dev);
return rc;
}
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index ee507eed42b5..27b8f8cd1b48 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -570,6 +570,8 @@ bool is_nvdimm(struct device *dev)
return dev->type == &nvdimm_device_type;
}
+static struct lock_class_key nvdimm_key;
+
struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
void *provider_data, const struct attribute_group **groups,
unsigned long flags, unsigned long cmd_mask, int num_flush,
@@ -613,6 +615,8 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
/* get security state and extended (master) state */
nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
+ device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &nvdimm_key);
nd_device_register(dev);
return nvdimm;
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 62b83b2e26e3..5197a813849d 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1830,6 +1830,8 @@ static struct device *nd_namespace_pmem_create(struct nd_region *nd_region)
return dev;
}
+static struct lock_class_key nvdimm_namespace_key;
+
void nd_region_create_ns_seed(struct nd_region *nd_region)
{
WARN_ON(!is_nvdimm_bus_locked(&nd_region->dev));
@@ -1845,8 +1847,12 @@ void nd_region_create_ns_seed(struct nd_region *nd_region)
*/
if (!nd_region->ns_seed)
dev_err(&nd_region->dev, "failed to create namespace\n");
- else
+ else {
+ device_initialize(nd_region->ns_seed);
+ lockdep_set_class(&nd_region->ns_seed->mutex,
+ &nvdimm_namespace_key);
nd_device_register(nd_region->ns_seed);
+ }
}
void nd_region_create_dax_seed(struct nd_region *nd_region)
@@ -2200,6 +2206,8 @@ int nd_region_register_namespaces(struct nd_region *nd_region, int *err)
if (id < 0)
break;
dev_set_name(dev, "namespace%d.%d", nd_region->id, id);
+ device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &nvdimm_namespace_key);
nd_device_register(dev);
}
if (i)
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 448f9dcb4bb7..99b04106434b 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -106,7 +106,7 @@ void nd_region_create_dax_seed(struct nd_region *nd_region);
int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus);
void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus);
void nd_synchronize(void);
-void __nd_device_register(struct device *dev);
+void nd_device_register(struct device *dev);
struct nd_label_id;
char *nd_label_gen_id(struct nd_label_id *label_id, const uuid_t *uuid,
u32 flags);
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index c31e184bfa45..0cd396d0024c 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -291,6 +291,8 @@ bool is_nd_pfn(struct device *dev)
}
EXPORT_SYMBOL(is_nd_pfn);
+static struct lock_class_key nvdimm_pfn_key;
+
struct device *nd_pfn_devinit(struct nd_pfn *nd_pfn,
struct nd_namespace_common *ndns)
{
@@ -303,6 +305,7 @@ struct device *nd_pfn_devinit(struct nd_pfn *nd_pfn,
nd_pfn->align = nd_pfn_default_alignment();
dev = &nd_pfn->dev;
device_initialize(&nd_pfn->dev);
+ lockdep_set_class(&nd_pfn->dev.mutex, &nvdimm_pfn_key);
if (ndns && !__nd_attach_ndns(&nd_pfn->dev, ndns, &nd_pfn->ndns)) {
dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
dev_name(ndns->claim));
@@ -346,7 +349,7 @@ struct device *nd_pfn_create(struct nd_region *nd_region)
nd_pfn = nd_pfn_alloc(nd_region);
dev = nd_pfn_devinit(nd_pfn, NULL);
- __nd_device_register(dev);
+ nd_device_register(dev);
return dev;
}
@@ -643,7 +646,7 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
put_device(pfn_dev);
} else
- __nd_device_register(pfn_dev);
+ nd_device_register(pfn_dev);
return rc;
}
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 0cb274c2b508..8650e8d39c89 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -949,6 +949,8 @@ static unsigned long default_align(struct nd_region *nd_region)
return align;
}
+static struct lock_class_key nvdimm_region_key;
+
static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
struct nd_region_desc *ndr_desc,
const struct device_type *dev_type, const char *caller)
@@ -1035,6 +1037,8 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
else
nd_region->flush = NULL;
+ device_initialize(dev);
+ lockdep_set_class(&dev->mutex, &nvdimm_region_key);
nd_device_register(dev);
return nd_region;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 4/8] nvdimm: Replace lockdep_mutex with local lock classes
2022-04-21 15:33 ` [PATCH v3 4/8] nvdimm: Replace lockdep_mutex with local lock classes Dan Williams
@ 2022-04-23 0:19 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 0:19 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Peter Zijlstra, Vishal Verma, Dave Jiang,
alison.schofield, nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:29AM -0700, Dan Williams wrote:
> In response to an attempt to expand dev->lockdep_mutex for device_lock()
> validation [1], Peter points out [2] that the lockdep API already has
> the ability to assign a dedicated lock class per subsystem device-type.
>
> Use lockdep_set_class() to override the default device_lock()
> '__lockdep_no_validate__' class for each NVDIMM subsystem device-type. This
> enables lockdep to detect deadlocks and recursive locking within the
> device-driver core and the subsystem.
>
> Link: https://lore.kernel.org/r/164982968798.684294.15817853329823976469.stgit@dwillia2-desk3.amr.corp.intel.com [1]
> Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [2]
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/nvdimm/btt_devs.c | 7 +++++--
> drivers/nvdimm/bus.c | 14 +++++++-------
> drivers/nvdimm/dax_devs.c | 4 ++--
> drivers/nvdimm/dimm_devs.c | 4 ++++
> drivers/nvdimm/namespace_devs.c | 10 +++++++++-
> drivers/nvdimm/nd-core.h | 2 +-
> drivers/nvdimm/pfn_devs.c | 7 +++++--
> drivers/nvdimm/region_devs.c | 4 ++++
> 8 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index e5a58520d398..120821796668 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -178,6 +178,8 @@ bool is_nd_btt(struct device *dev)
> }
> EXPORT_SYMBOL(is_nd_btt);
>
> +static struct lock_class_key nvdimm_btt_key;
> +
> static struct device *__nd_btt_create(struct nd_region *nd_region,
> unsigned long lbasize, uuid_t *uuid,
> struct nd_namespace_common *ndns)
> @@ -205,6 +207,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
> dev->parent = &nd_region->dev;
> dev->type = &nd_btt_device_type;
> device_initialize(&nd_btt->dev);
> + lockdep_set_class(&nd_btt->dev.mutex, &nvdimm_btt_key);
> if (ndns && !__nd_attach_ndns(&nd_btt->dev, ndns, &nd_btt->ndns)) {
> dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
> dev_name(ndns->claim));
> @@ -225,7 +228,7 @@ struct device *nd_btt_create(struct nd_region *nd_region)
> {
> struct device *dev = __nd_btt_create(nd_region, 0, NULL, NULL);
>
> - __nd_device_register(dev);
> + nd_device_register(dev);
> return dev;
> }
>
> @@ -324,7 +327,7 @@ static int __nd_btt_probe(struct nd_btt *nd_btt,
> if (!nd_btt->uuid)
> return -ENOMEM;
>
> - __nd_device_register(&nd_btt->dev);
> + nd_device_register(&nd_btt->dev);
>
> return 0;
> }
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 7b0d1443217a..85ffa04e93c2 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -334,6 +334,8 @@ struct nvdimm_bus *nvdimm_to_bus(struct nvdimm *nvdimm)
> }
> EXPORT_SYMBOL_GPL(nvdimm_to_bus);
>
> +static struct lock_class_key nvdimm_bus_key;
> +
> struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
> struct nvdimm_bus_descriptor *nd_desc)
> {
> @@ -360,6 +362,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
> nvdimm_bus->dev.bus = &nvdimm_bus_type;
> nvdimm_bus->dev.of_node = nd_desc->of_node;
> device_initialize(&nvdimm_bus->dev);
> + lockdep_set_class(&nvdimm_bus->dev.mutex, &nvdimm_bus_key);
> device_set_pm_not_required(&nvdimm_bus->dev);
> rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id);
> if (rc)
> @@ -511,7 +514,7 @@ static void nd_async_device_unregister(void *d, async_cookie_t cookie)
> put_device(dev);
> }
>
> -void __nd_device_register(struct device *dev)
> +void nd_device_register(struct device *dev)
> {
> if (!dev)
> return;
> @@ -537,12 +540,6 @@ void __nd_device_register(struct device *dev)
> async_schedule_dev_domain(nd_async_device_register, dev,
> &nd_async_domain);
> }
> -
> -void nd_device_register(struct device *dev)
> -{
> - device_initialize(dev);
> - __nd_device_register(dev);
> -}
> EXPORT_SYMBOL(nd_device_register);
>
> void nd_device_unregister(struct device *dev, enum nd_async_mode mode)
> @@ -724,6 +721,8 @@ static void ndctl_release(struct device *dev)
> kfree(dev);
> }
>
> +static struct lock_class_key nvdimm_ndctl_key;
> +
> int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
> {
> dev_t devt = MKDEV(nvdimm_bus_major, nvdimm_bus->id);
> @@ -734,6 +733,7 @@ int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
> if (!dev)
> return -ENOMEM;
> device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &nvdimm_ndctl_key);
> device_set_pm_not_required(dev);
> dev->class = nd_class;
> dev->parent = &nvdimm_bus->dev;
> diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
> index 99965077bac4..7f4a9d28b670 100644
> --- a/drivers/nvdimm/dax_devs.c
> +++ b/drivers/nvdimm/dax_devs.c
> @@ -80,7 +80,7 @@ struct device *nd_dax_create(struct nd_region *nd_region)
> nd_dax = nd_dax_alloc(nd_region);
> if (nd_dax)
> dev = nd_pfn_devinit(&nd_dax->nd_pfn, NULL);
> - __nd_device_register(dev);
> + nd_device_register(dev);
> return dev;
> }
>
> @@ -119,7 +119,7 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
> nd_detach_ndns(dax_dev, &nd_pfn->ndns);
> put_device(dax_dev);
> } else
> - __nd_device_register(dax_dev);
> + nd_device_register(dax_dev);
>
> return rc;
> }
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index ee507eed42b5..27b8f8cd1b48 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -570,6 +570,8 @@ bool is_nvdimm(struct device *dev)
> return dev->type == &nvdimm_device_type;
> }
>
> +static struct lock_class_key nvdimm_key;
> +
> struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
> void *provider_data, const struct attribute_group **groups,
> unsigned long flags, unsigned long cmd_mask, int num_flush,
> @@ -613,6 +615,8 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
> /* get security state and extended (master) state */
> nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
> + device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &nvdimm_key);
> nd_device_register(dev);
>
> return nvdimm;
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 62b83b2e26e3..5197a813849d 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1830,6 +1830,8 @@ static struct device *nd_namespace_pmem_create(struct nd_region *nd_region)
> return dev;
> }
>
> +static struct lock_class_key nvdimm_namespace_key;
> +
> void nd_region_create_ns_seed(struct nd_region *nd_region)
> {
> WARN_ON(!is_nvdimm_bus_locked(&nd_region->dev));
> @@ -1845,8 +1847,12 @@ void nd_region_create_ns_seed(struct nd_region *nd_region)
> */
> if (!nd_region->ns_seed)
> dev_err(&nd_region->dev, "failed to create namespace\n");
> - else
> + else {
> + device_initialize(nd_region->ns_seed);
> + lockdep_set_class(&nd_region->ns_seed->mutex,
> + &nvdimm_namespace_key);
> nd_device_register(nd_region->ns_seed);
> + }
> }
>
> void nd_region_create_dax_seed(struct nd_region *nd_region)
> @@ -2200,6 +2206,8 @@ int nd_region_register_namespaces(struct nd_region *nd_region, int *err)
> if (id < 0)
> break;
> dev_set_name(dev, "namespace%d.%d", nd_region->id, id);
> + device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &nvdimm_namespace_key);
> nd_device_register(dev);
> }
> if (i)
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index 448f9dcb4bb7..99b04106434b 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -106,7 +106,7 @@ void nd_region_create_dax_seed(struct nd_region *nd_region);
> int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus);
> void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus);
> void nd_synchronize(void);
> -void __nd_device_register(struct device *dev);
> +void nd_device_register(struct device *dev);
> struct nd_label_id;
> char *nd_label_gen_id(struct nd_label_id *label_id, const uuid_t *uuid,
> u32 flags);
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index c31e184bfa45..0cd396d0024c 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -291,6 +291,8 @@ bool is_nd_pfn(struct device *dev)
> }
> EXPORT_SYMBOL(is_nd_pfn);
>
> +static struct lock_class_key nvdimm_pfn_key;
> +
> struct device *nd_pfn_devinit(struct nd_pfn *nd_pfn,
> struct nd_namespace_common *ndns)
> {
> @@ -303,6 +305,7 @@ struct device *nd_pfn_devinit(struct nd_pfn *nd_pfn,
> nd_pfn->align = nd_pfn_default_alignment();
> dev = &nd_pfn->dev;
> device_initialize(&nd_pfn->dev);
> + lockdep_set_class(&nd_pfn->dev.mutex, &nvdimm_pfn_key);
> if (ndns && !__nd_attach_ndns(&nd_pfn->dev, ndns, &nd_pfn->ndns)) {
> dev_dbg(&ndns->dev, "failed, already claimed by %s\n",
> dev_name(ndns->claim));
> @@ -346,7 +349,7 @@ struct device *nd_pfn_create(struct nd_region *nd_region)
> nd_pfn = nd_pfn_alloc(nd_region);
> dev = nd_pfn_devinit(nd_pfn, NULL);
>
> - __nd_device_register(dev);
> + nd_device_register(dev);
> return dev;
> }
>
> @@ -643,7 +646,7 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
> nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
> put_device(pfn_dev);
> } else
> - __nd_device_register(pfn_dev);
> + nd_device_register(pfn_dev);
>
> return rc;
> }
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 0cb274c2b508..8650e8d39c89 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -949,6 +949,8 @@ static unsigned long default_align(struct nd_region *nd_region)
> return align;
> }
>
> +static struct lock_class_key nvdimm_region_key;
> +
> static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
> struct nd_region_desc *ndr_desc,
> const struct device_type *dev_type, const char *caller)
> @@ -1035,6 +1037,8 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
> else
> nd_region->flush = NULL;
>
> + device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &nvdimm_region_key);
> nd_device_register(dev);
>
> return nd_region;
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 5/8] ACPI: NFIT: Drop nfit_device_lock()
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
` (3 preceding siblings ...)
2022-04-21 15:33 ` [PATCH v3 4/8] nvdimm: Replace lockdep_mutex with local lock classes Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-23 0:21 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 6/8] nvdimm: Drop nd_device_lock() Dan Williams
` (2 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Vishal Verma, Dave Jiang, Ira Weiny, peterz, alison.schofield,
nvdimm, linux-kernel
The nfit_device_lock() helper was added to provide lockdep coverage for
the NFIT driver's usage of device_lock() on the nvdimm_bus object. Now
that nvdimm_bus objects have their own lock class this wrapper can be
dropped.
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/acpi/nfit/core.c | 30 +++++++++++++++---------------
drivers/acpi/nfit/nfit.h | 24 ------------------------
2 files changed, 15 insertions(+), 39 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index fe61f617a943..ae5f4acf2675 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1230,7 +1230,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
if (rc)
return rc;
- nfit_device_lock(dev);
+ device_lock(dev);
nd_desc = dev_get_drvdata(dev);
if (nd_desc) {
struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
@@ -1247,7 +1247,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
break;
}
}
- nfit_device_unlock(dev);
+ device_unlock(dev);
if (rc)
return rc;
return size;
@@ -1267,10 +1267,10 @@ static ssize_t scrub_show(struct device *dev,
ssize_t rc = -ENXIO;
bool busy;
- nfit_device_lock(dev);
+ device_lock(dev);
nd_desc = dev_get_drvdata(dev);
if (!nd_desc) {
- nfit_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
acpi_desc = to_acpi_desc(nd_desc);
@@ -1287,7 +1287,7 @@ static ssize_t scrub_show(struct device *dev,
}
mutex_unlock(&acpi_desc->init_mutex);
- nfit_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -1304,14 +1304,14 @@ static ssize_t scrub_store(struct device *dev,
if (val != 1)
return -EINVAL;
- nfit_device_lock(dev);
+ 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);
}
- nfit_device_unlock(dev);
+ device_unlock(dev);
if (rc)
return rc;
return size;
@@ -1697,9 +1697,9 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
struct acpi_device *adev = data;
struct device *dev = &adev->dev;
- nfit_device_lock(dev->parent);
+ device_lock(dev->parent);
__acpi_nvdimm_notify(dev, event);
- nfit_device_unlock(dev->parent);
+ device_unlock(dev->parent);
}
static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
@@ -3152,8 +3152,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 */
- nfit_device_lock(dev);
- nfit_device_unlock(dev);
+ device_lock(dev);
+ device_unlock(dev);
/* Bounce the init_mutex to complete initial registration */
mutex_lock(&acpi_desc->init_mutex);
@@ -3305,8 +3305,8 @@ void acpi_nfit_shutdown(void *data)
* acpi_nfit_ars_rescan() submissions have had a chance to
* either submit or see ->cancel set.
*/
- nfit_device_lock(bus_dev);
- nfit_device_unlock(bus_dev);
+ device_lock(bus_dev);
+ device_unlock(bus_dev);
flush_workqueue(nfit_wq);
}
@@ -3449,9 +3449,9 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
{
- nfit_device_lock(&adev->dev);
+ device_lock(&adev->dev);
__acpi_nfit_notify(&adev->dev, adev->handle, event);
- nfit_device_unlock(&adev->dev);
+ 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 50882bdbeb96..6023ad61831a 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -337,30 +337,6 @@ 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);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 5/8] ACPI: NFIT: Drop nfit_device_lock()
2022-04-21 15:33 ` [PATCH v3 5/8] ACPI: NFIT: Drop nfit_device_lock() Dan Williams
@ 2022-04-23 0:21 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 0:21 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Vishal Verma, Dave Jiang, peterz, alison.schofield,
nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:34AM -0700, Dan Williams wrote:
> The nfit_device_lock() helper was added to provide lockdep coverage for
> the NFIT driver's usage of device_lock() on the nvdimm_bus object. Now
> that nvdimm_bus objects have their own lock class this wrapper can be
> dropped.
>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/acpi/nfit/core.c | 30 +++++++++++++++---------------
> drivers/acpi/nfit/nfit.h | 24 ------------------------
> 2 files changed, 15 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index fe61f617a943..ae5f4acf2675 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1230,7 +1230,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
> if (rc)
> return rc;
>
> - nfit_device_lock(dev);
> + device_lock(dev);
> nd_desc = dev_get_drvdata(dev);
> if (nd_desc) {
> struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
> @@ -1247,7 +1247,7 @@ static ssize_t hw_error_scrub_store(struct device *dev,
> break;
> }
> }
> - nfit_device_unlock(dev);
> + device_unlock(dev);
> if (rc)
> return rc;
> return size;
> @@ -1267,10 +1267,10 @@ static ssize_t scrub_show(struct device *dev,
> ssize_t rc = -ENXIO;
> bool busy;
>
> - nfit_device_lock(dev);
> + device_lock(dev);
> nd_desc = dev_get_drvdata(dev);
> if (!nd_desc) {
> - nfit_device_unlock(dev);
> + device_unlock(dev);
> return rc;
> }
> acpi_desc = to_acpi_desc(nd_desc);
> @@ -1287,7 +1287,7 @@ static ssize_t scrub_show(struct device *dev,
> }
>
> mutex_unlock(&acpi_desc->init_mutex);
> - nfit_device_unlock(dev);
> + device_unlock(dev);
> return rc;
> }
>
> @@ -1304,14 +1304,14 @@ static ssize_t scrub_store(struct device *dev,
> if (val != 1)
> return -EINVAL;
>
> - nfit_device_lock(dev);
> + 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);
> }
> - nfit_device_unlock(dev);
> + device_unlock(dev);
> if (rc)
> return rc;
> return size;
> @@ -1697,9 +1697,9 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
> struct acpi_device *adev = data;
> struct device *dev = &adev->dev;
>
> - nfit_device_lock(dev->parent);
> + device_lock(dev->parent);
> __acpi_nvdimm_notify(dev, event);
> - nfit_device_unlock(dev->parent);
> + device_unlock(dev->parent);
> }
>
> static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
> @@ -3152,8 +3152,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 */
> - nfit_device_lock(dev);
> - nfit_device_unlock(dev);
> + device_lock(dev);
> + device_unlock(dev);
>
> /* Bounce the init_mutex to complete initial registration */
> mutex_lock(&acpi_desc->init_mutex);
> @@ -3305,8 +3305,8 @@ void acpi_nfit_shutdown(void *data)
> * acpi_nfit_ars_rescan() submissions have had a chance to
> * either submit or see ->cancel set.
> */
> - nfit_device_lock(bus_dev);
> - nfit_device_unlock(bus_dev);
> + device_lock(bus_dev);
> + device_unlock(bus_dev);
>
> flush_workqueue(nfit_wq);
> }
> @@ -3449,9 +3449,9 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>
> static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> {
> - nfit_device_lock(&adev->dev);
> + device_lock(&adev->dev);
> __acpi_nfit_notify(&adev->dev, adev->handle, event);
> - nfit_device_unlock(&adev->dev);
> + 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 50882bdbeb96..6023ad61831a 100644
> --- a/drivers/acpi/nfit/nfit.h
> +++ b/drivers/acpi/nfit/nfit.h
> @@ -337,30 +337,6 @@ 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);
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 6/8] nvdimm: Drop nd_device_lock()
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
` (4 preceding siblings ...)
2022-04-21 15:33 ` [PATCH v3 5/8] ACPI: NFIT: Drop nfit_device_lock() Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-23 0:24 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 7/8] device-core: Kill the lockdep_mutex Dan Williams
2022-04-21 15:33 ` [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios Dan Williams
7 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Vishal Verma, Dave Jiang, Ira Weiny, peterz, alison.schofield,
nvdimm, linux-kernel
Now that all NVDIMM subsystem locking is validated with custom lock
classes, there is no need for the custom usage of the lockdep_mutex.
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/nvdimm/btt_devs.c | 16 +++++----
drivers/nvdimm/bus.c | 24 +++++---------
drivers/nvdimm/core.c | 10 +++---
drivers/nvdimm/dimm_devs.c | 8 ++---
drivers/nvdimm/namespace_devs.c | 36 +++++++++++----------
drivers/nvdimm/nd-core.h | 66 ---------------------------------------
drivers/nvdimm/pfn_devs.c | 24 +++++++-------
drivers/nvdimm/pmem.c | 2 +
drivers/nvdimm/region.c | 2 +
drivers/nvdimm/region_devs.c | 16 +++++----
lib/Kconfig.debug | 17 ----------
11 files changed, 66 insertions(+), 155 deletions(-)
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 120821796668..fabbb31f2c35 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -50,14 +50,14 @@ static ssize_t sector_size_store(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -79,11 +79,11 @@ static ssize_t uuid_store(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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");
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -108,13 +108,13 @@ static ssize_t namespace_store(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -126,14 +126,14 @@ static ssize_t size_show(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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;
}
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 85ffa04e93c2..a4fc17db707c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -88,10 +88,7 @@ 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 || rc == -EOPNOTSUPP) &&
dev->parent && is_nd_region(dev->parent))
nd_region_advance_seeds(to_nd_region(dev->parent), dev);
@@ -111,11 +108,8 @@ static void nvdimm_bus_remove(struct device *dev)
struct module *provider = to_bus_provider(dev);
struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
- if (nd_drv->remove) {
- debug_nvdimm_lock(dev);
+ if (nd_drv->remove)
nd_drv->remove(dev);
- debug_nvdimm_unlock(dev);
- }
dev_dbg(&nvdimm_bus->dev, "%s.remove(%s)\n", dev->driver->name,
dev_name(dev));
@@ -139,7 +133,7 @@ static void nvdimm_bus_shutdown(struct device *dev)
void nd_device_notify(struct device *dev, enum nvdimm_event event)
{
- nd_device_lock(dev);
+ device_lock(dev);
if (dev->driver) {
struct nd_device_driver *nd_drv;
@@ -147,7 +141,7 @@ void nd_device_notify(struct device *dev, enum nvdimm_event event)
if (nd_drv->notify)
nd_drv->notify(dev, event);
}
- nd_device_unlock(dev);
+ device_unlock(dev);
}
EXPORT_SYMBOL(nd_device_notify);
@@ -569,9 +563,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.
*/
- nd_device_lock(dev);
+ device_lock(dev);
killed = kill_device(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
if (!killed)
return;
@@ -930,10 +924,10 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
if (nvdimm_bus->probe_active == 0)
break;
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
wait_event(nvdimm_bus->wait,
nvdimm_bus->probe_active == 0);
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
} while (true);
}
@@ -1167,7 +1161,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
goto out;
}
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
if (rc)
@@ -1189,7 +1183,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
out_unlock:
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
out:
kfree(in_env);
kfree(out_env);
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 69a03358817f..144926b7451c 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -215,7 +215,7 @@ EXPORT_SYMBOL_GPL(to_nvdimm_bus_dev);
*
* Enforce that uuids can only be changed while the device is disabled
* (driver detached)
- * LOCKING: expects nd_device_lock() is held on entry
+ * LOCKING: expects device_lock() is held on entry
*/
int nd_uuid_store(struct device *dev, uuid_t **uuid_out, const char *buf,
size_t len)
@@ -316,15 +316,15 @@ static DEVICE_ATTR_RO(provider);
static int flush_namespaces(struct device *dev, void *data)
{
- nd_device_lock(dev);
- nd_device_unlock(dev);
+ device_lock(dev);
+ device_unlock(dev);
return 0;
}
static int flush_regions_dimms(struct device *dev, void *data)
{
- nd_device_lock(dev);
- nd_device_unlock(dev);
+ device_lock(dev);
+ 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 27b8f8cd1b48..c7c980577491 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -341,9 +341,9 @@ static ssize_t available_slots_show(struct device *dev,
{
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
rc = __available_slots_show(dev_get_drvdata(dev), buf);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -386,12 +386,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.
*/
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = nvdimm_security_store(dev, buf, len);
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 5197a813849d..bf4f5c09d9b1 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -264,7 +264,7 @@ static ssize_t alt_name_store(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev->parent);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __alt_name_store(dev, buf, len);
@@ -272,7 +272,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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -846,7 +846,7 @@ static ssize_t size_store(struct device *dev,
if (rc)
return rc;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __size_store(dev, val);
@@ -868,7 +868,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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -1043,7 +1043,7 @@ static ssize_t uuid_store(struct device *dev,
} else
return -ENXIO;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
if (to_ndns(dev)->claim)
@@ -1059,7 +1059,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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -1118,7 +1118,7 @@ static ssize_t sector_size_store(struct device *dev,
} else
return -ENXIO;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
if (to_ndns(dev)->claim)
rc = -EBUSY;
@@ -1129,7 +1129,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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -1239,9 +1239,9 @@ static ssize_t holder_show(struct device *dev,
struct nd_namespace_common *ndns = to_ndns(dev);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
rc = sprintf(buf, "%s\n", ndns->claim ? dev_name(ndns->claim) : "");
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -1278,7 +1278,7 @@ static ssize_t holder_class_store(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev->parent);
int rc;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __holder_class_store(dev, buf);
@@ -1286,7 +1286,7 @@ static ssize_t holder_class_store(struct device *dev,
rc = nd_namespace_label_update(nd_region, dev);
dev_dbg(dev, "%s(%d)\n", rc < 0 ? "fail " : "", rc);
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -1297,7 +1297,7 @@ static ssize_t holder_class_show(struct device *dev,
struct nd_namespace_common *ndns = to_ndns(dev);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
if (ndns->claim_class == NVDIMM_CCLASS_NONE)
rc = sprintf(buf, "\n");
else if ((ndns->claim_class == NVDIMM_CCLASS_BTT) ||
@@ -1309,7 +1309,7 @@ static ssize_t holder_class_show(struct device *dev,
rc = sprintf(buf, "dax\n");
else
rc = sprintf(buf, "<unknown>\n");
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -1323,7 +1323,7 @@ static ssize_t mode_show(struct device *dev,
char *mode;
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
claim = ndns->claim;
if (claim && is_nd_btt(claim))
mode = "safe";
@@ -1336,7 +1336,7 @@ static ssize_t mode_show(struct device *dev,
else
mode = "raw";
rc = sprintf(buf, "%s\n", mode);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -1456,8 +1456,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.
*/
- nd_device_lock(&ndns->dev);
- nd_device_unlock(&ndns->dev);
+ device_lock(&ndns->dev);
+ 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 99b04106434b..cc86ee09d7c0 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -161,70 +161,4 @@ static inline void devm_nsio_disable(struct device *dev,
{
}
#endif
-
-#ifdef CONFIG_PROVE_NVDIMM_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 0cd396d0024c..0e92ab4b3283 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -55,7 +55,7 @@ static ssize_t mode_store(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc = 0;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
if (dev->driver)
rc = -EBUSY;
@@ -77,7 +77,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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -123,14 +123,14 @@ static ssize_t align_store(struct device *dev,
unsigned long aligns[MAX_NVDIMM_ALIGN] = { [0] = 0, };
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
rc = nd_size_select_store(dev, buf, &nd_pfn->align,
nd_pfn_supported_alignments(aligns));
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -152,11 +152,11 @@ static ssize_t uuid_store(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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");
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc ? rc : len;
}
@@ -181,13 +181,13 @@ static ssize_t namespace_store(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- nd_device_lock(dev);
+ 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);
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -199,7 +199,7 @@ static ssize_t resource_show(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
if (dev->driver) {
struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
u64 offset = __le64_to_cpu(pfn_sb->dataoff);
@@ -213,7 +213,7 @@ static ssize_t resource_show(struct device *dev,
/* no address to convey if the pfn instance is disabled */
rc = -ENXIO;
}
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -225,7 +225,7 @@ static ssize_t size_show(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
if (dev->driver) {
struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
u64 offset = __le64_to_cpu(pfn_sb->dataoff);
@@ -241,7 +241,7 @@ static ssize_t size_show(struct device *dev,
/* no size to convey if the pfn instance is disabled */
rc = -ENXIO;
}
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 58d95242a836..3992521c151f 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -573,7 +573,7 @@ static void nd_pmem_remove(struct device *dev)
nvdimm_namespace_detach_btt(to_nd_btt(dev));
else {
/*
- * Note, this assumes nd_device_lock() context to not
+ * Note, this assumes device_lock() context to not
* race nd_pmem_notify()
*/
sysfs_put(pmem->bb_state);
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 188560b1c110..390123d293ea 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -95,7 +95,7 @@ static void nd_region_remove(struct device *dev)
nvdimm_bus_unlock(dev);
/*
- * Note, this assumes nd_device_lock() context to not race
+ * Note, this assumes 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 8650e8d39c89..d976260eca7a 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -279,7 +279,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.
*/
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
if (nd_region->ndr_mappings) {
@@ -296,7 +296,7 @@ static ssize_t set_cookie_show(struct device *dev,
}
}
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
if (rc)
return rc;
@@ -353,12 +353,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.
*/
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
available = nd_region_available_dpa(nd_region);
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
return sprintf(buf, "%llu\n", available);
}
@@ -370,12 +370,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;
- nd_device_lock(dev);
+ device_lock(dev);
nvdimm_bus_lock(dev);
wait_nvdimm_bus_probe_idle(dev);
available = nd_region_allocatable_dpa(nd_region);
nvdimm_bus_unlock(dev);
- nd_device_unlock(dev);
+ device_unlock(dev);
return sprintf(buf, "%llu\n", available);
}
@@ -549,12 +549,12 @@ static ssize_t region_badblocks_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
ssize_t rc;
- nd_device_lock(dev);
+ device_lock(dev);
if (dev->driver)
rc = badblocks_show(&nd_region->bb, buf, 0);
else
rc = -ENXIO;
- nd_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4a2c853c948b..cfe3b092c31d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1544,23 +1544,6 @@ config CSD_LOCK_WAIT_DEBUG
include the IPI handler function currently executing (if any)
and relevant stack traces.
-choice
- prompt "Lock debugging: prove subsystem device_lock() correctness"
- depends on PROVE_LOCKING
- help
- For subsystems that have instrumented their usage of the device_lock()
- with nested annotations, enable lock dependency checking. The locking
- hierarchy 'subclass' identifiers are not compatible across
- sub-systems, so only one can be enabled at a time.
-
-config PROVE_NVDIMM_LOCKING
- bool "NVDIMM"
- depends on LIBNVDIMM
- help
- Enable lockdep to validate nd_device_lock() usage.
-
-endchoice
-
endmenu # lock debugging
config TRACE_IRQFLAGS
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 6/8] nvdimm: Drop nd_device_lock()
2022-04-21 15:33 ` [PATCH v3 6/8] nvdimm: Drop nd_device_lock() Dan Williams
@ 2022-04-23 0:24 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 0:24 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Vishal Verma, Dave Jiang, peterz, alison.schofield,
nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:39AM -0700, Dan Williams wrote:
> Now that all NVDIMM subsystem locking is validated with custom lock
> classes, there is no need for the custom usage of the lockdep_mutex.
>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/nvdimm/btt_devs.c | 16 +++++----
> drivers/nvdimm/bus.c | 24 +++++---------
> drivers/nvdimm/core.c | 10 +++---
> drivers/nvdimm/dimm_devs.c | 8 ++---
> drivers/nvdimm/namespace_devs.c | 36 +++++++++++----------
> drivers/nvdimm/nd-core.h | 66 ---------------------------------------
> drivers/nvdimm/pfn_devs.c | 24 +++++++-------
> drivers/nvdimm/pmem.c | 2 +
> drivers/nvdimm/region.c | 2 +
> drivers/nvdimm/region_devs.c | 16 +++++----
> lib/Kconfig.debug | 17 ----------
> 11 files changed, 66 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 120821796668..fabbb31f2c35 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -50,14 +50,14 @@ static ssize_t sector_size_store(struct device *dev,
> struct nd_btt *nd_btt = to_nd_btt(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -79,11 +79,11 @@ static ssize_t uuid_store(struct device *dev,
> struct nd_btt *nd_btt = to_nd_btt(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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");
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -108,13 +108,13 @@ static ssize_t namespace_store(struct device *dev,
> struct nd_btt *nd_btt = to_nd_btt(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -126,14 +126,14 @@ static ssize_t size_show(struct device *dev,
> struct nd_btt *nd_btt = to_nd_btt(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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;
> }
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 85ffa04e93c2..a4fc17db707c 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -88,10 +88,7 @@ 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 || rc == -EOPNOTSUPP) &&
> dev->parent && is_nd_region(dev->parent))
> nd_region_advance_seeds(to_nd_region(dev->parent), dev);
> @@ -111,11 +108,8 @@ static void nvdimm_bus_remove(struct device *dev)
> struct module *provider = to_bus_provider(dev);
> struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
>
> - if (nd_drv->remove) {
> - debug_nvdimm_lock(dev);
> + if (nd_drv->remove)
> nd_drv->remove(dev);
> - debug_nvdimm_unlock(dev);
> - }
>
> dev_dbg(&nvdimm_bus->dev, "%s.remove(%s)\n", dev->driver->name,
> dev_name(dev));
> @@ -139,7 +133,7 @@ static void nvdimm_bus_shutdown(struct device *dev)
>
> void nd_device_notify(struct device *dev, enum nvdimm_event event)
> {
> - nd_device_lock(dev);
> + device_lock(dev);
> if (dev->driver) {
> struct nd_device_driver *nd_drv;
>
> @@ -147,7 +141,7 @@ void nd_device_notify(struct device *dev, enum nvdimm_event event)
> if (nd_drv->notify)
> nd_drv->notify(dev, event);
> }
> - nd_device_unlock(dev);
> + device_unlock(dev);
> }
> EXPORT_SYMBOL(nd_device_notify);
>
> @@ -569,9 +563,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.
> */
> - nd_device_lock(dev);
> + device_lock(dev);
> killed = kill_device(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> if (!killed)
> return;
> @@ -930,10 +924,10 @@ void wait_nvdimm_bus_probe_idle(struct device *dev)
> if (nvdimm_bus->probe_active == 0)
> break;
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
> wait_event(nvdimm_bus->wait,
> nvdimm_bus->probe_active == 0);
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> } while (true);
> }
> @@ -1167,7 +1161,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
> goto out;
> }
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
> if (rc)
> @@ -1189,7 +1183,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>
> out_unlock:
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
> out:
> kfree(in_env);
> kfree(out_env);
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index 69a03358817f..144926b7451c 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -215,7 +215,7 @@ EXPORT_SYMBOL_GPL(to_nvdimm_bus_dev);
> *
> * Enforce that uuids can only be changed while the device is disabled
> * (driver detached)
> - * LOCKING: expects nd_device_lock() is held on entry
> + * LOCKING: expects device_lock() is held on entry
> */
> int nd_uuid_store(struct device *dev, uuid_t **uuid_out, const char *buf,
> size_t len)
> @@ -316,15 +316,15 @@ static DEVICE_ATTR_RO(provider);
>
> static int flush_namespaces(struct device *dev, void *data)
> {
> - nd_device_lock(dev);
> - nd_device_unlock(dev);
> + device_lock(dev);
> + device_unlock(dev);
> return 0;
> }
>
> static int flush_regions_dimms(struct device *dev, void *data)
> {
> - nd_device_lock(dev);
> - nd_device_unlock(dev);
> + device_lock(dev);
> + 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 27b8f8cd1b48..c7c980577491 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -341,9 +341,9 @@ static ssize_t available_slots_show(struct device *dev,
> {
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> rc = __available_slots_show(dev_get_drvdata(dev), buf);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -386,12 +386,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.
> */
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> rc = nvdimm_security_store(dev, buf, len);
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5197a813849d..bf4f5c09d9b1 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -264,7 +264,7 @@ static ssize_t alt_name_store(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev->parent);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> rc = __alt_name_store(dev, buf, len);
> @@ -272,7 +272,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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc < 0 ? rc : len;
> }
> @@ -846,7 +846,7 @@ static ssize_t size_store(struct device *dev,
> if (rc)
> return rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> rc = __size_store(dev, val);
> @@ -868,7 +868,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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc < 0 ? rc : len;
> }
> @@ -1043,7 +1043,7 @@ static ssize_t uuid_store(struct device *dev,
> } else
> return -ENXIO;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> if (to_ndns(dev)->claim)
> @@ -1059,7 +1059,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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc < 0 ? rc : len;
> }
> @@ -1118,7 +1118,7 @@ static ssize_t sector_size_store(struct device *dev,
> } else
> return -ENXIO;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> if (to_ndns(dev)->claim)
> rc = -EBUSY;
> @@ -1129,7 +1129,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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -1239,9 +1239,9 @@ static ssize_t holder_show(struct device *dev,
> struct nd_namespace_common *ndns = to_ndns(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> rc = sprintf(buf, "%s\n", ndns->claim ? dev_name(ndns->claim) : "");
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -1278,7 +1278,7 @@ static ssize_t holder_class_store(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev->parent);
> int rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> rc = __holder_class_store(dev, buf);
> @@ -1286,7 +1286,7 @@ static ssize_t holder_class_store(struct device *dev,
> rc = nd_namespace_label_update(nd_region, dev);
> dev_dbg(dev, "%s(%d)\n", rc < 0 ? "fail " : "", rc);
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc < 0 ? rc : len;
> }
> @@ -1297,7 +1297,7 @@ static ssize_t holder_class_show(struct device *dev,
> struct nd_namespace_common *ndns = to_ndns(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> if (ndns->claim_class == NVDIMM_CCLASS_NONE)
> rc = sprintf(buf, "\n");
> else if ((ndns->claim_class == NVDIMM_CCLASS_BTT) ||
> @@ -1309,7 +1309,7 @@ static ssize_t holder_class_show(struct device *dev,
> rc = sprintf(buf, "dax\n");
> else
> rc = sprintf(buf, "<unknown>\n");
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -1323,7 +1323,7 @@ static ssize_t mode_show(struct device *dev,
> char *mode;
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> claim = ndns->claim;
> if (claim && is_nd_btt(claim))
> mode = "safe";
> @@ -1336,7 +1336,7 @@ static ssize_t mode_show(struct device *dev,
> else
> mode = "raw";
> rc = sprintf(buf, "%s\n", mode);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -1456,8 +1456,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.
> */
> - nd_device_lock(&ndns->dev);
> - nd_device_unlock(&ndns->dev);
> + device_lock(&ndns->dev);
> + 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 99b04106434b..cc86ee09d7c0 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -161,70 +161,4 @@ static inline void devm_nsio_disable(struct device *dev,
> {
> }
> #endif
> -
> -#ifdef CONFIG_PROVE_NVDIMM_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 0cd396d0024c..0e92ab4b3283 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -55,7 +55,7 @@ static ssize_t mode_store(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc = 0;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> if (dev->driver)
> rc = -EBUSY;
> @@ -77,7 +77,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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -123,14 +123,14 @@ static ssize_t align_store(struct device *dev,
> unsigned long aligns[MAX_NVDIMM_ALIGN] = { [0] = 0, };
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> rc = nd_size_select_store(dev, buf, &nd_pfn->align,
> nd_pfn_supported_alignments(aligns));
> dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
> buf[len - 1] == '\n' ? "" : "\n");
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -152,11 +152,11 @@ static ssize_t uuid_store(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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");
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -181,13 +181,13 @@ static ssize_t namespace_store(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + 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);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -199,7 +199,7 @@ static ssize_t resource_show(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> if (dev->driver) {
> struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> u64 offset = __le64_to_cpu(pfn_sb->dataoff);
> @@ -213,7 +213,7 @@ static ssize_t resource_show(struct device *dev,
> /* no address to convey if the pfn instance is disabled */
> rc = -ENXIO;
> }
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> @@ -225,7 +225,7 @@ static ssize_t size_show(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> if (dev->driver) {
> struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> u64 offset = __le64_to_cpu(pfn_sb->dataoff);
> @@ -241,7 +241,7 @@ static ssize_t size_show(struct device *dev,
> /* no size to convey if the pfn instance is disabled */
> rc = -ENXIO;
> }
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 58d95242a836..3992521c151f 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -573,7 +573,7 @@ static void nd_pmem_remove(struct device *dev)
> nvdimm_namespace_detach_btt(to_nd_btt(dev));
> else {
> /*
> - * Note, this assumes nd_device_lock() context to not
> + * Note, this assumes device_lock() context to not
> * race nd_pmem_notify()
> */
> sysfs_put(pmem->bb_state);
> diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
> index 188560b1c110..390123d293ea 100644
> --- a/drivers/nvdimm/region.c
> +++ b/drivers/nvdimm/region.c
> @@ -95,7 +95,7 @@ static void nd_region_remove(struct device *dev)
> nvdimm_bus_unlock(dev);
>
> /*
> - * Note, this assumes nd_device_lock() context to not race
> + * Note, this assumes 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 8650e8d39c89..d976260eca7a 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -279,7 +279,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.
> */
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> if (nd_region->ndr_mappings) {
> @@ -296,7 +296,7 @@ static ssize_t set_cookie_show(struct device *dev,
> }
> }
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> if (rc)
> return rc;
> @@ -353,12 +353,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.
> */
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> available = nd_region_available_dpa(nd_region);
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return sprintf(buf, "%llu\n", available);
> }
> @@ -370,12 +370,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;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> nvdimm_bus_lock(dev);
> wait_nvdimm_bus_probe_idle(dev);
> available = nd_region_allocatable_dpa(nd_region);
> nvdimm_bus_unlock(dev);
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return sprintf(buf, "%llu\n", available);
> }
> @@ -549,12 +549,12 @@ static ssize_t region_badblocks_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> ssize_t rc;
>
> - nd_device_lock(dev);
> + device_lock(dev);
> if (dev->driver)
> rc = badblocks_show(&nd_region->bb, buf, 0);
> else
> rc = -ENXIO;
> - nd_device_unlock(dev);
> + device_unlock(dev);
>
> return rc;
> }
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 4a2c853c948b..cfe3b092c31d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1544,23 +1544,6 @@ config CSD_LOCK_WAIT_DEBUG
> include the IPI handler function currently executing (if any)
> and relevant stack traces.
>
> -choice
> - prompt "Lock debugging: prove subsystem device_lock() correctness"
> - depends on PROVE_LOCKING
> - help
> - For subsystems that have instrumented their usage of the device_lock()
> - with nested annotations, enable lock dependency checking. The locking
> - hierarchy 'subclass' identifiers are not compatible across
> - sub-systems, so only one can be enabled at a time.
> -
> -config PROVE_NVDIMM_LOCKING
> - bool "NVDIMM"
> - depends on LIBNVDIMM
> - help
> - Enable lockdep to validate nd_device_lock() usage.
> -
> -endchoice
> -
> endmenu # lock debugging
>
> config TRACE_IRQFLAGS
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 7/8] device-core: Kill the lockdep_mutex
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
` (5 preceding siblings ...)
2022-04-21 15:33 ` [PATCH v3 6/8] nvdimm: Drop nd_device_lock() Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-21 16:09 ` Greg Kroah-Hartman
2022-04-23 0:25 ` Ira Weiny
2022-04-21 15:33 ` [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios Dan Williams
7 siblings, 2 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Peter Zijlstra,
vishal.l.verma, alison.schofield, nvdimm, linux-kernel
Per Peter [1], the lockdep API has native support for all the use cases
lockdep_mutex was attempting to enable. Now that all lockdep_mutex users
have been converted to those APIs, drop this lock.
Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [1]
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/base/core.c | 3 ---
include/linux/device.h | 5 -----
2 files changed, 8 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3d6430eb0c6a..2eede2ec3d64 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2864,9 +2864,6 @@ 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/include/linux/device.h b/include/linux/device.h
index 82c9d307e7bd..c00ab223da50 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -400,8 +400,6 @@ struct dev_msi_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.
@@ -499,9 +497,6 @@ 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.
*/
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 7/8] device-core: Kill the lockdep_mutex
2022-04-21 15:33 ` [PATCH v3 7/8] device-core: Kill the lockdep_mutex Dan Williams
@ 2022-04-21 16:09 ` Greg Kroah-Hartman
2022-04-23 0:25 ` Ira Weiny
1 sibling, 0 replies; 30+ messages in thread
From: Greg Kroah-Hartman @ 2022-04-21 16:09 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Rafael J. Wysocki, Peter Zijlstra, vishal.l.verma,
alison.schofield, nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:45AM -0700, Dan Williams wrote:
> Per Peter [1], the lockdep API has native support for all the use cases
> lockdep_mutex was attempting to enable. Now that all lockdep_mutex users
> have been converted to those APIs, drop this lock.
>
> Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [1]
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
YES!!!!!
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Nice work.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 7/8] device-core: Kill the lockdep_mutex
2022-04-21 15:33 ` [PATCH v3 7/8] device-core: Kill the lockdep_mutex Dan Williams
2022-04-21 16:09 ` Greg Kroah-Hartman
@ 2022-04-23 0:25 ` Ira Weiny
1 sibling, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 0:25 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Greg Kroah-Hartman, Rafael J. Wysocki, Peter Zijlstra,
vishal.l.verma, alison.schofield, nvdimm, linux-kernel
On Thu, Apr 21, 2022 at 08:33:45AM -0700, Dan Williams wrote:
> Per Peter [1], the lockdep API has native support for all the use cases
> lockdep_mutex was attempting to enable. Now that all lockdep_mutex users
> have been converted to those APIs, drop this lock.
>
> Link: https://lore.kernel.org/r/Ylf0dewci8myLvoW@hirez.programming.kicks-ass.net [1]
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/base/core.c | 3 ---
> include/linux/device.h | 5 -----
> 2 files changed, 8 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3d6430eb0c6a..2eede2ec3d64 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2864,9 +2864,6 @@ 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/include/linux/device.h b/include/linux/device.h
> index 82c9d307e7bd..c00ab223da50 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -400,8 +400,6 @@ struct dev_msi_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.
> @@ -499,9 +497,6 @@ 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.
> */
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios
2022-04-21 15:33 [PATCH v3 0/8] device-core: Enable device_lock() lockdep validation Dan Williams
` (6 preceding siblings ...)
2022-04-21 15:33 ` [PATCH v3 7/8] device-core: Kill the lockdep_mutex Dan Williams
@ 2022-04-21 15:33 ` Dan Williams
2022-04-23 4:28 ` Ira Weiny
2022-04-23 21:22 ` [PATCH v4 " Dan Williams
7 siblings, 2 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-21 15:33 UTC (permalink / raw)
To: linux-cxl; +Cc: peterz, vishal.l.verma, alison.schofield, nvdimm, linux-kernel
Lockdep reports the following deadlock scenarios for CXL root device
power-management, device_prepare(), operations, and device_shutdown()
operations for 'nd_region' devices:
---
Chain exists of:
&nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(system_transition_mutex);
lock(&nvdimm_bus->reconfig_mutex);
lock(system_transition_mutex);
lock(&nvdimm_region_key);
--
Chain exists of:
&cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&cxl_root_key);
lock(acpi_scan_lock);
lock(&cxl_root_key);
lock(&cxl_nvdimm_bridge_key);
---
These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
which walks the entire system device topology taking device_lock() along
the way. The nvdimm_bus_lock() is protecting against unregistration,
multiple simultaneous ops callers, and preventing activate_show() from
racing activate_store(). For the first 2, the lock is redundant.
Unregistration already flushes all ops users, and sysfs already prevents
multiple threads to be active in an ops handler at the same time. For
the last userspace should already be waiting for its last
activate_store() to complete, and does not need activate_show() to flush
the write side, so this lock usage can be deleted in these attributes.
Fixes: 48001ea50d17 ("PM, libnvdimm: Add runtime firmware activation support")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/nvdimm/core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 144926b7451c..7c7f4a43fd4f 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -395,10 +395,8 @@ static ssize_t activate_show(struct device *dev,
if (!nd_desc->fw_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(dev);
cap = nd_desc->fw_ops->capability(nd_desc);
state = nd_desc->fw_ops->activate_state(nd_desc);
- nvdimm_bus_unlock(dev);
if (cap < NVDIMM_FWA_CAP_QUIESCE)
return -EOPNOTSUPP;
@@ -443,7 +441,6 @@ static ssize_t activate_store(struct device *dev,
else
return -EINVAL;
- nvdimm_bus_lock(dev);
state = nd_desc->fw_ops->activate_state(nd_desc);
switch (state) {
@@ -461,7 +458,6 @@ static ssize_t activate_store(struct device *dev,
default:
rc = -ENXIO;
}
- nvdimm_bus_unlock(dev);
if (rc == 0)
rc = len;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios
2022-04-21 15:33 ` [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios Dan Williams
@ 2022-04-23 4:28 ` Ira Weiny
2022-04-23 17:29 ` Dan Williams
2022-04-23 21:22 ` [PATCH v4 " Dan Williams
1 sibling, 1 reply; 30+ messages in thread
From: Ira Weiny @ 2022-04-23 4:28 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, peterz, vishal.l.verma, alison.schofield, nvdimm,
linux-kernel
On Thu, Apr 21, 2022 at 08:33:51AM -0700, Dan Williams wrote:
> Lockdep reports the following deadlock scenarios for CXL root device
> power-management, device_prepare(), operations, and device_shutdown()
> operations for 'nd_region' devices:
>
> ---
> Chain exists of:
> &nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(system_transition_mutex);
> lock(&nvdimm_bus->reconfig_mutex);
> lock(system_transition_mutex);
> lock(&nvdimm_region_key);
>
> --
>
> Chain exists of:
> &cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&cxl_root_key);
> lock(acpi_scan_lock);
> lock(&cxl_root_key);
> lock(&cxl_nvdimm_bridge_key);
>
> ---
>
> These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
> which walks the entire system device topology taking device_lock() along
> the way. The nvdimm_bus_lock() is protecting against unregistration,
> multiple simultaneous ops callers, and preventing activate_show() from
> racing activate_store(). For the first 2, the lock is redundant.
> Unregistration already flushes all ops users, and sysfs already prevents
> multiple threads to be active in an ops handler at the same time. For
> the last userspace should already be waiting for its last
> activate_store() to complete, and does not need activate_show() to flush
> the write side, so this lock usage can be deleted in these attributes.
>
I'm sorry if this is obvious but why can't the locking be removed from
capability_show() and nvdimm_bus_firmware_visible() as well?
Effectively it sounds like we don't care if the cap read is racing any state
change? And we know the device can't go away while sysfs is calling those
functions.
Ira
>
> Fixes: 48001ea50d17 ("PM, libnvdimm: Add runtime firmware activation support")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/nvdimm/core.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index 144926b7451c..7c7f4a43fd4f 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -395,10 +395,8 @@ static ssize_t activate_show(struct device *dev,
> if (!nd_desc->fw_ops)
> return -EOPNOTSUPP;
>
> - nvdimm_bus_lock(dev);
> cap = nd_desc->fw_ops->capability(nd_desc);
> state = nd_desc->fw_ops->activate_state(nd_desc);
> - nvdimm_bus_unlock(dev);
>
> if (cap < NVDIMM_FWA_CAP_QUIESCE)
> return -EOPNOTSUPP;
> @@ -443,7 +441,6 @@ static ssize_t activate_store(struct device *dev,
> else
> return -EINVAL;
>
> - nvdimm_bus_lock(dev);
> state = nd_desc->fw_ops->activate_state(nd_desc);
>
> switch (state) {
> @@ -461,7 +458,6 @@ static ssize_t activate_store(struct device *dev,
> default:
> rc = -ENXIO;
> }
> - nvdimm_bus_unlock(dev);
>
> if (rc == 0)
> rc = len;
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios
2022-04-23 4:28 ` Ira Weiny
@ 2022-04-23 17:29 ` Dan Williams
0 siblings, 0 replies; 30+ messages in thread
From: Dan Williams @ 2022-04-23 17:29 UTC (permalink / raw)
To: Ira Weiny
Cc: linux-cxl, Peter Zijlstra, Vishal L Verma, Schofield, Alison,
Linux NVDIMM, Linux Kernel Mailing List
On Fri, Apr 22, 2022 at 9:28 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Thu, Apr 21, 2022 at 08:33:51AM -0700, Dan Williams wrote:
> > Lockdep reports the following deadlock scenarios for CXL root device
> > power-management, device_prepare(), operations, and device_shutdown()
> > operations for 'nd_region' devices:
> >
> > ---
> > Chain exists of:
> > &nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(system_transition_mutex);
> > lock(&nvdimm_bus->reconfig_mutex);
> > lock(system_transition_mutex);
> > lock(&nvdimm_region_key);
> >
> > --
> >
> > Chain exists of:
> > &cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&cxl_root_key);
> > lock(acpi_scan_lock);
> > lock(&cxl_root_key);
> > lock(&cxl_nvdimm_bridge_key);
> >
> > ---
> >
> > These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
> > which walks the entire system device topology taking device_lock() along
> > the way. The nvdimm_bus_lock() is protecting against unregistration,
> > multiple simultaneous ops callers, and preventing activate_show() from
> > racing activate_store(). For the first 2, the lock is redundant.
> > Unregistration already flushes all ops users, and sysfs already prevents
> > multiple threads to be active in an ops handler at the same time. For
> > the last userspace should already be waiting for its last
> > activate_store() to complete, and does not need activate_show() to flush
> > the write side, so this lock usage can be deleted in these attributes.
> >
>
> I'm sorry if this is obvious but why can't the locking be removed from
> capability_show() and nvdimm_bus_firmware_visible() as well?
It can, that's a good catch, thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 8/8] nvdimm: Fix firmware activation deadlock scenarios
2022-04-21 15:33 ` [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios Dan Williams
2022-04-23 4:28 ` Ira Weiny
@ 2022-04-23 21:22 ` Dan Williams
2022-04-24 23:30 ` Ira Weiny
1 sibling, 1 reply; 30+ messages in thread
From: Dan Williams @ 2022-04-23 21:22 UTC (permalink / raw)
To: linux-cxl; +Cc: nvdimm
Lockdep reports the following deadlock scenarios for CXL root device
power-management, device_prepare(), operations, and device_shutdown()
operations for 'nd_region' devices:
---
Chain exists of:
&nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(system_transition_mutex);
lock(&nvdimm_bus->reconfig_mutex);
lock(system_transition_mutex);
lock(&nvdimm_region_key);
--
Chain exists of:
&cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&cxl_root_key);
lock(acpi_scan_lock);
lock(&cxl_root_key);
lock(&cxl_nvdimm_bridge_key);
---
These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
which walks the entire system device topology taking device_lock() along
the way. The nvdimm_bus_lock() is protecting against unregistration,
multiple simultaneous ops callers, and preventing activate_show() from
racing activate_store(). For the first 2, the lock is redundant.
Unregistration already flushes all ops users, and sysfs already prevents
multiple threads to be active in an ops handler at the same time. For
the last userspace should already be waiting for its last
activate_store() to complete, and does not need activate_show() to flush
the write side, so this lock usage can be deleted in these attributes.
Fixes: 48001ea50d17 ("PM, libnvdimm: Add runtime firmware activation support")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- Remove nvdimm_bus_lock() from all ->capability() invocations (Ira)
drivers/nvdimm/core.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 144926b7451c..d91799b71d23 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -368,9 +368,7 @@ static ssize_t capability_show(struct device *dev,
if (!nd_desc->fw_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(dev);
cap = nd_desc->fw_ops->capability(nd_desc);
- nvdimm_bus_unlock(dev);
switch (cap) {
case NVDIMM_FWA_CAP_QUIESCE:
@@ -395,10 +393,8 @@ static ssize_t activate_show(struct device *dev,
if (!nd_desc->fw_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(dev);
cap = nd_desc->fw_ops->capability(nd_desc);
state = nd_desc->fw_ops->activate_state(nd_desc);
- nvdimm_bus_unlock(dev);
if (cap < NVDIMM_FWA_CAP_QUIESCE)
return -EOPNOTSUPP;
@@ -443,7 +439,6 @@ static ssize_t activate_store(struct device *dev,
else
return -EINVAL;
- nvdimm_bus_lock(dev);
state = nd_desc->fw_ops->activate_state(nd_desc);
switch (state) {
@@ -461,7 +456,6 @@ static ssize_t activate_store(struct device *dev,
default:
rc = -ENXIO;
}
- nvdimm_bus_unlock(dev);
if (rc == 0)
rc = len;
@@ -484,10 +478,7 @@ static umode_t nvdimm_bus_firmware_visible(struct kobject *kobj, struct attribut
if (!nd_desc->fw_ops)
return 0;
- nvdimm_bus_lock(dev);
cap = nd_desc->fw_ops->capability(nd_desc);
- nvdimm_bus_unlock(dev);
-
if (cap < NVDIMM_FWA_CAP_QUIESCE)
return 0;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v4 8/8] nvdimm: Fix firmware activation deadlock scenarios
2022-04-23 21:22 ` [PATCH v4 " Dan Williams
@ 2022-04-24 23:30 ` Ira Weiny
0 siblings, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2022-04-24 23:30 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl, nvdimm
On Sat, Apr 23, 2022 at 02:22:18PM -0700, Dan Williams wrote:
> Lockdep reports the following deadlock scenarios for CXL root device
> power-management, device_prepare(), operations, and device_shutdown()
> operations for 'nd_region' devices:
>
> ---
> Chain exists of:
> &nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(system_transition_mutex);
> lock(&nvdimm_bus->reconfig_mutex);
> lock(system_transition_mutex);
> lock(&nvdimm_region_key);
>
> --
>
> Chain exists of:
> &cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&cxl_root_key);
> lock(acpi_scan_lock);
> lock(&cxl_root_key);
> lock(&cxl_nvdimm_bridge_key);
>
> ---
>
> These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
> which walks the entire system device topology taking device_lock() along
> the way. The nvdimm_bus_lock() is protecting against unregistration,
> multiple simultaneous ops callers, and preventing activate_show() from
> racing activate_store(). For the first 2, the lock is redundant.
> Unregistration already flushes all ops users, and sysfs already prevents
> multiple threads to be active in an ops handler at the same time. For
> the last userspace should already be waiting for its last
> activate_store() to complete, and does not need activate_show() to flush
> the write side, so this lock usage can be deleted in these attributes.
>
> Fixes: 48001ea50d17 ("PM, libnvdimm: Add runtime firmware activation support")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Changes since v3:
> - Remove nvdimm_bus_lock() from all ->capability() invocations (Ira)
>
> drivers/nvdimm/core.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index 144926b7451c..d91799b71d23 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -368,9 +368,7 @@ static ssize_t capability_show(struct device *dev,
> if (!nd_desc->fw_ops)
> return -EOPNOTSUPP;
>
> - nvdimm_bus_lock(dev);
> cap = nd_desc->fw_ops->capability(nd_desc);
> - nvdimm_bus_unlock(dev);
>
> switch (cap) {
> case NVDIMM_FWA_CAP_QUIESCE:
> @@ -395,10 +393,8 @@ static ssize_t activate_show(struct device *dev,
> if (!nd_desc->fw_ops)
> return -EOPNOTSUPP;
>
> - nvdimm_bus_lock(dev);
> cap = nd_desc->fw_ops->capability(nd_desc);
> state = nd_desc->fw_ops->activate_state(nd_desc);
> - nvdimm_bus_unlock(dev);
>
> if (cap < NVDIMM_FWA_CAP_QUIESCE)
> return -EOPNOTSUPP;
> @@ -443,7 +439,6 @@ static ssize_t activate_store(struct device *dev,
> else
> return -EINVAL;
>
> - nvdimm_bus_lock(dev);
> state = nd_desc->fw_ops->activate_state(nd_desc);
>
> switch (state) {
> @@ -461,7 +456,6 @@ static ssize_t activate_store(struct device *dev,
> default:
> rc = -ENXIO;
> }
> - nvdimm_bus_unlock(dev);
>
> if (rc == 0)
> rc = len;
> @@ -484,10 +478,7 @@ static umode_t nvdimm_bus_firmware_visible(struct kobject *kobj, struct attribut
> if (!nd_desc->fw_ops)
> return 0;
>
> - nvdimm_bus_lock(dev);
> cap = nd_desc->fw_ops->capability(nd_desc);
> - nvdimm_bus_unlock(dev);
> -
> if (cap < NVDIMM_FWA_CAP_QUIESCE)
> return 0;
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread