Changes since v1 [1]:
- Improve the clarity of the cover letter and changelogs of the
major patches (Patch2 and Patch12) (Pierre, Kevin, and Dave)
- Fix device_lock_interruptible() false negative deadlock detection
(Kevin)
- Fix off-by-one error in the device_set_lock_class() enable case (Kevin)
- Spelling fixes in Patch2 changelog (Pierre)
- Compilation fixes when both CONFIG_CXL_BUS=n and
CONFIG_LIBNVDIMM=n. (0day robot)
[1]: https://lore.kernel.org/all/164610292916.2682974.12924748003366352335.stgit@dwillia2-desk3.amr.corp.intel.com/
---
The device_lock() is why the lockdep_set_novalidate_class() API exists.
The lock is taken in too many disparate contexts, and lockdep by design
assumes that all device_lock() acquisitions are identical. The lack of
lockdep coverage leads to deadlock scenarios landing upstream. To
mitigate that problem the lockdep_mutex was added [2].
The lockdep_mutex lets a subsystem mirror device_lock() acquisitions
without lockdep_set_novalidate_class() to gain some limited lockdep
coverage. The mirroring approach is limited to taking the device_lock()
after-the-fact in a subsystem's 'struct bus_type' operations and fails
to cover device_lock() acquisition in the driver-core. It also can only
track the needs of one subsystem at a time so, for example the kernel
needs to be recompiled between CONFIG_PROVE_NVDIMM_LOCKING and
CONFIG_PROVE_CXL_LOCKING depending on which subsystem is being
regression tested. Obviously that also means that intra-subsystem
locking dependencies can not be validated.
Two enhancements are proposed to improve the current state of
device_lock() lockdep validation:
1/ Communicate a lock class to the device-core and let it acquire
dev->lockdep_mutex per the subsystem's nested locking expectations.
2/ Go further and provide a lockdep_mutex per-subsystem so each
has the full span of MAX_LOCKDEP_SUBCLASSES available for its use.
This enabling has already prevented at least one device_lock() deadlock
from making its way upstream.
[2]: commit 87a30e1f05d7 ("driver-core, libnvdimm: Let device subsystems add local lockdep coverage")
---
Dan Williams (12):
device-core: Move device_lock() lockdep init to a helper
device-core: Add dev->lock_class to enable device_lock() lockdep validation
cxl/core: Refactor a cxl_lock_class() out of cxl_nested_lock()
cxl/core: Remove cxl_device_lock()
cxl/core: Clamp max lock_class
cxl/core: Use dev->lock_class for device_lock() lockdep validation
cxl/acpi: Add a device_lock() lock class for the root platform device
libnvdimm: Refactor an nvdimm_lock_class() helper
ACPI: NFIT: Drop nfit_device_lock()
libnvdimm: Drop nd_device_lock()
libnvdimm: Enable lockdep validation
device-core: Enable multi-subsystem device_lock() lockdep validation
drivers/acpi/nfit/core.c | 30 ++++---
drivers/acpi/nfit/nfit.h | 24 ------
drivers/base/core.c | 5 -
drivers/cxl/acpi.c | 1
drivers/cxl/core/memdev.c | 1
drivers/cxl/core/pmem.c | 6 +
drivers/cxl/core/port.c | 56 ++++++-------
drivers/cxl/cxl.h | 76 +++++++-----------
drivers/cxl/mem.c | 4 -
drivers/cxl/pmem.c | 12 +--
drivers/cxl/port.c | 2
drivers/nvdimm/btt_devs.c | 16 ++--
drivers/nvdimm/bus.c | 26 +++---
drivers/nvdimm/core.c | 10 +-
drivers/nvdimm/dimm_devs.c | 8 +-
drivers/nvdimm/namespace_devs.c | 36 ++++-----
drivers/nvdimm/nd-core.h | 51 +++---------
drivers/nvdimm/pfn_devs.c | 24 +++---
drivers/nvdimm/pmem.c | 2
drivers/nvdimm/region.c | 2
drivers/nvdimm/region_devs.c | 16 ++--
include/linux/device.h | 162 ++++++++++++++++++++++++++++++++++++++-
lib/Kconfig.debug | 23 ------
23 files changed, 325 insertions(+), 268 deletions(-)
--
base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e
Now that the device-core can start validating lockdep usage after the
device has been added, use that capability to validate usage of
device_lock() against the ACPI0017 device relative to other subsystem
locks.
The 'enum cxl_lock_class' definition moves outside of the ifdef guard to
support device_set_lock_class() called from cxl_acpi_probe().
Cc: Alison Schofield <[email protected]>
Cc: Vishal Verma <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Ben Widawsky <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/acpi.c | 1 +
drivers/cxl/cxl.h | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..ef5c3252bdb2 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -283,6 +283,7 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ device_set_lock_class(&pdev->dev, CXL_ROOT_LOCK);
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/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index fddbcb380e84..05dc4c081ad2 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -405,9 +405,9 @@ 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_ROOT_LOCK,
CXL_NVDIMM_LOCK,
CXL_NVDIMM_BRIDGE_LOCK,
/*
@@ -423,6 +423,7 @@ enum cxl_lock_class {
*/
};
+#ifdef CONFIG_PROVE_CXL_LOCKING
static inline int clamp_lock_class(struct device *dev, int lock_class)
{
if (lock_class >= MAX_LOCKDEP_SUBCLASSES) {
In preparation for moving to the device-core device_lock lockdep
validation, refactor an nvdimm_lock_class() helper to be used with
device_set_lock_class().
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Ira Weiny <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/nvdimm/nd-core.h | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 448f9dcb4bb7..deb3d047571e 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -174,22 +174,27 @@ enum {
LOCK_CLAIM,
};
-static inline void debug_nvdimm_lock(struct device *dev)
+static inline int nvdimm_lock_class(struct device *dev)
{
if (is_nd_region(dev))
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_REGION);
+ return LOCK_REGION;
else if (is_nvdimm(dev))
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_DIMM);
+ return LOCK_DIMM;
else if (is_nd_btt(dev) || is_nd_pfn(dev) || is_nd_dax(dev))
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_CLAIM);
+ return LOCK_CLAIM;
else if (dev->parent && (is_nd_region(dev->parent)))
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_NAMESPACE);
+ return LOCK_NAMESPACE;
else if (is_nvdimm_bus(dev))
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_BUS);
+ return LOCK_BUS;
else if (dev->class && dev->class == nd_class)
- mutex_lock_nested(&dev->lockdep_mutex, LOCK_NDCTL);
+ return LOCK_NDCTL;
else
- dev_WARN(dev, "unknown lock level\n");
+ return -1;
+}
+
+static inline void debug_nvdimm_lock(struct device *dev)
+{
+ mutex_lock_nested(&dev->lockdep_mutex, nvdimm_lock_class(dev));
}
static inline void debug_nvdimm_unlock(struct device *dev)
On 4/13/22 02:01, Dan Williams wrote:
> Changes since v1 [1]:
> - Improve the clarity of the cover letter and changelogs of the
> major patches (Patch2 and Patch12) (Pierre, Kevin, and Dave)
> - Fix device_lock_interruptible() false negative deadlock detection
> (Kevin)
> - Fix off-by-one error in the device_set_lock_class() enable case (Kevin)
> - Spelling fixes in Patch2 changelog (Pierre)
> - Compilation fixes when both CONFIG_CXL_BUS=n and
> CONFIG_LIBNVDIMM=n. (0day robot)
>
> [1]: https://lore.kernel.org/all/164610292916.2682974.12924748003366352335.stgit@dwillia2-desk3.amr.corp.intel.com/
>
> ---
>
> The device_lock() is why the lockdep_set_novalidate_class() API exists.
> The lock is taken in too many disparate contexts, and lockdep by design
> assumes that all device_lock() acquisitions are identical. The lack of
> lockdep coverage leads to deadlock scenarios landing upstream. To
> mitigate that problem the lockdep_mutex was added [2].
>
> The lockdep_mutex lets a subsystem mirror device_lock() acquisitions
> without lockdep_set_novalidate_class() to gain some limited lockdep
> coverage. The mirroring approach is limited to taking the device_lock()
> after-the-fact in a subsystem's 'struct bus_type' operations and fails
> to cover device_lock() acquisition in the driver-core. It also can only
> track the needs of one subsystem at a time so, for example the kernel
> needs to be recompiled between CONFIG_PROVE_NVDIMM_LOCKING and
> CONFIG_PROVE_CXL_LOCKING depending on which subsystem is being
> regression tested. Obviously that also means that intra-subsystem
> locking dependencies can not be validated.
Instead of using a fake lockdep_mutex, maybe you can just use a unique
lockdep key for each subsystem and call lockdep_set_class() in the
device_initialize() if such key is present or
lockdep_set_novalidate_class() otherwise. The unique key can be passed
either as a parameter to device_initialize() or as part of the device
structure. It is certainly less cumbersome that having a fake
lockdep_mutex array in the device structure.
Cheers,
Longman