Greg, Rafael,
Here are some extensions to the 'lockdep_mutex' I came up with after
getting tired of alternating debug builds between CXL and NVDIMM
subsystem testing, and worrying about the missing lockdep coverage from
device-lock acquisition in the device-core.
The primary insight is that the existing users of the 'lockdep_mutex'
are just wrapping calls to device_lock() with a subsystem local helper
that can apply the proper lock_class for how those subsystems nest the
device_lock(). Instead of local wrapping just instruct the subsystem to
annotate the lock_class directly in the device and let the device_lock()
common code handle acquiring lockdep_mutex with the proper class.
The final patch in the series extends this further and adds an array of
lockdep_mutex instances, 1 per subsystem, so that multiple subsystems can
be validated in a single kernel image.
This has been useful for identifying scenarios when it is safe to
acquire the device_lock() in a sysfs attribute.
Thoughts?
I know its late in the cycle to be messing with device-core internals,
so feel free to put this off until 5.19. This series is based on the
cxl_device_lock() enabling that is currently in -next.
---
Dan Williams (11):
device-core: Enable 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: Introduce cxl_set_lock_class()
cxl/acpi: Add a 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: Introduce a per-subsystem lockdep_mutex
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 | 52 ++++++++--------
drivers/cxl/core/region.c | 1
drivers/cxl/cxl.h | 72 ++++++++--------------
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 | 130 ++++++++++++++++++++++++++++++++++++++-
lib/Kconfig.debug | 23 -------
24 files changed, 291 insertions(+), 263 deletions(-)
base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4
In preparation for switching to the core device_lock lockdep validation
scheme, convert nd_device_lock() calls back to device_lock().
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/nvdimm/btt_devs.c | 16 ++++++++--------
drivers/nvdimm/bus.c | 23 +++++++++-------------
drivers/nvdimm/core.c | 10 +++++-----
drivers/nvdimm/dimm_devs.c | 8 ++++----
drivers/nvdimm/namespace_devs.c | 36 ++++++++++++++++++-----------------
drivers/nvdimm/nd-core.h | 40 ---------------------------------------
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 | 3 ++-
11 files changed, 68 insertions(+), 112 deletions(-)
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 8b52e5144f08..d4164b2cf22e 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -51,14 +51,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;
}
@@ -80,11 +80,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;
}
@@ -109,13 +109,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;
}
@@ -127,14 +127,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 9dc7f3edd42b..f481831929eb 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -91,9 +91,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))
@@ -114,11 +112,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));
@@ -142,7 +137,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;
@@ -150,7 +145,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);
@@ -575,9 +570,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;
@@ -933,10 +928,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);
}
@@ -1170,7 +1165,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)
@@ -1192,7 +1187,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 dc7449a40003..00001024d0b2 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -362,9 +362,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;
}
@@ -407,12 +407,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 b57a2d36c517..16cedd854fa8 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -383,7 +383,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);
@@ -391,7 +391,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;
}
@@ -1056,7 +1056,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);
@@ -1082,7 +1082,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;
}
@@ -1268,7 +1268,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)
@@ -1284,7 +1284,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;
}
@@ -1358,7 +1358,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;
@@ -1369,7 +1369,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;
}
@@ -1484,9 +1484,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;
}
@@ -1523,7 +1523,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);
@@ -1531,7 +1531,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;
}
@@ -1542,7 +1542,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) ||
@@ -1554,7 +1554,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;
}
@@ -1568,7 +1568,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";
@@ -1581,7 +1581,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;
}
@@ -1715,8 +1715,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 772568ffb9d6..90c4029c87d2 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -214,45 +214,5 @@ static inline int nvdimm_lock_class(struct device *dev)
else
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)
-{
- 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 58eda16f5c53..ee357401fdd1 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -56,7 +56,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;
@@ -78,7 +78,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;
}
@@ -124,14 +124,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;
}
@@ -153,11 +153,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;
}
@@ -182,13 +182,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;
}
@@ -200,7 +200,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);
@@ -214,7 +214,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;
}
@@ -226,7 +226,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);
@@ -242,7 +242,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 e0c34120df37..466b3e9247b6 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -103,7 +103,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 9ccf3d608799..0f6c85b45df3 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -305,7 +305,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) {
@@ -322,7 +322,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;
@@ -398,12 +398,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);
}
@@ -415,12 +415,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);
}
@@ -594,12 +594,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 5942b22d7f1b..a4bd109f6178 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1529,7 +1529,8 @@ config PROVE_NVDIMM_LOCKING
bool "NVDIMM"
depends on LIBNVDIMM
help
- Enable lockdep to validate nd_device_lock() usage.
+ Enable lockdep to validate libnvdimm subsystem usage of the
+ device lock.
config PROVE_CXL_LOCKING
bool "CXL"
MAX_LOCKDEP_SUBCLASSES limits the depth of the CXL topology that can be
validated by lockdep. Given that the cxl_test topology is already at
this limit collapse some of the levels and clamp the max depth.
Cc: Alison Schofield <[email protected]>
Cc: Vishal Verma <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Ben Widawsky <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/cxl.h | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 97e6ca7e4940..1357a245037d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -501,20 +501,33 @@ enum cxl_lock_class {
CXL_ANON_LOCK,
CXL_NVDIMM_LOCK,
CXL_NVDIMM_BRIDGE_LOCK,
- CXL_PORT_LOCK,
+ CXL_PORT_LOCK = 2,
/*
* 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.
+ * depth would need to be defined first. Also, the max
+ * validation depth is limited by MAX_LOCKDEP_SUBCLASSES.
*/
};
+static inline int clamp_lock_class(struct device *dev, int lock_class)
+{
+ if (lock_class >= MAX_LOCKDEP_SUBCLASSES) {
+ dev_warn_once(dev,
+ "depth: %d, disabling lockdep for this device\n",
+ lock_class);
+ return 0;
+ }
+
+ return lock_class;
+}
+
static inline int cxl_lock_class(struct device *dev)
{
if (is_cxl_port(dev)) {
struct cxl_port *port = to_cxl_port(dev);
- return CXL_PORT_LOCK + port->depth;
+ return clamp_lock_class(dev, CXL_PORT_LOCK + port->depth);
} else if (is_cxl_decoder(dev)) {
struct cxl_port *port = to_cxl_port(dev->parent);
@@ -522,7 +535,7 @@ static inline int cxl_lock_class(struct device *dev)
* A decoder is the immediate child of a port, so set
* its lock class equal to other child device siblings.
*/
- return CXL_PORT_LOCK + port->depth + 1;
+ return clamp_lock_class(dev, CXL_PORT_LOCK + port->depth + 1);
} else if (is_cxl_nvdimm_bridge(dev))
return CXL_NVDIMM_BRIDGE_LOCK;
else if (is_cxl_nvdimm(dev))
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]>
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 2650a852eeaf..772568ffb9d6 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -197,22 +197,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)
Register libnvdimm subsystem devices with a non-zero lock_class to
enable the device-core to track lock dependencies.
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/nvdimm/bus.c | 3 +++
drivers/nvdimm/nd-core.h | 9 +++++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index f481831929eb..c2f70f9a8b70 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -363,6 +363,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
if (rc)
goto err;
+ nvdimm_set_lock_class(&nvdimm_bus->dev);
rc = device_add(&nvdimm_bus->dev);
if (rc) {
dev_dbg(&nvdimm_bus->dev, "registration failed: %d\n", rc);
@@ -488,6 +489,7 @@ static void nd_async_device_register(void *d, async_cookie_t cookie)
{
struct device *dev = d;
+ nvdimm_set_lock_class(dev);
if (device_add(dev) != 0) {
dev_err(dev, "%s: failed\n", __func__);
put_device(dev);
@@ -741,6 +743,7 @@ int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
if (rc)
goto err;
+ nvdimm_set_lock_class(dev);
rc = device_add(dev);
if (rc) {
dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %d\n",
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 90c4029c87d2..40065606a6e6 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -214,5 +214,14 @@ static inline int nvdimm_lock_class(struct device *dev)
else
return -1;
}
+
+static inline void nvdimm_set_lock_class(struct device *dev)
+{
+ device_set_lock_class(dev, nvdimm_lock_class(dev));
+}
+#else
+static inline void nvdimm_set_lock_class(struct device *dev)
+{
+}
#endif
#endif /* __ND_CORE_H__ */
Update CONFIG_PROVE_CXL_LOCKING to use the common device-core helpers
for device_lock validation.
When CONFIG_PROVE_LOCKING is enabled and device_set_lock_class() is
passed a non-zero lock class the core acquires the 'struct device'
@lockdep_mutex everywhere it acquires the device_lock. Where
lockdep_mutex does not skip lockdep validation like device_lock.
cxl_set_lock_class() wraps device_set_lock_class() as to not collide
with other subsystems that may also support this lockdep validation
scheme. See the 'choice' for the various CONFIG_PROVE_$SUBSYS_LOCKING
options in lib/Kconfig.debug.
Cc: Alison Schofield <[email protected]>
Cc: Vishal Verma <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Ben Widawsky <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/core/memdev.c | 1 +
drivers/cxl/core/pmem.c | 2 ++
drivers/cxl/core/port.c | 2 ++
drivers/cxl/core/region.c | 1 +
drivers/cxl/cxl.h | 9 +++++++++
5 files changed, 15 insertions(+)
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 1f76b28f9826..ad8c9f61c38a 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -343,6 +343,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
cxlmd->cxlds = cxlds;
cdev = &cxlmd->cdev;
+ cxl_set_lock_class(dev);
rc = cdev_device_add(cdev, dev);
if (rc)
goto err;
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index c3d7e6ce3fdf..e0bdda788b01 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -171,6 +171,7 @@ struct cxl_nvdimm_bridge *devm_cxl_add_nvdimm_bridge(struct device *host,
if (rc)
goto err;
+ cxl_set_lock_class(dev);
rc = device_add(dev);
if (rc)
goto err;
@@ -283,6 +284,7 @@ int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
if (rc)
goto err;
+ cxl_set_lock_class(dev);
rc = device_add(dev);
if (rc)
goto err;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 44006d8eb64d..5eee7de1c7f9 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -507,6 +507,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
if (rc)
goto err;
+ cxl_set_lock_class(dev);
rc = device_add(dev);
if (rc)
goto err;
@@ -1389,6 +1390,7 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
if (is_root_decoder(dev))
cxld->platform_res.name = dev_name(dev);
+ cxl_set_lock_class(dev);
return device_add(dev);
}
EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b1a4ba622739..f0a821de94cf 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -476,6 +476,7 @@ int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *cxlr)
if (rc)
goto err;
+ cxl_set_lock_class(dev);
rc = device_add(dev);
if (rc)
goto err;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1357a245037d..f94eff659cce 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -543,5 +543,14 @@ static inline int cxl_lock_class(struct device *dev)
else
return CXL_ANON_LOCK;
}
+
+static inline void cxl_set_lock_class(struct device *dev)
+{
+ device_set_lock_class(dev, cxl_lock_class(dev));
+}
+#else
+static inline void cxl_set_lock_class(struct device *dev)
+{
+}
#endif
#endif /* __CXL_H__ */
The @lockdep_mutex attribute of 'struct device' was introduced to allow
subsystems to wrap their usage of device_lock() with a local definition
that adds nested annotations and lockdep validation. However, that
approach leaves lockdep blind to the device_lock usage in the
device-core. Instead of requiring the subsystem to replace device_lock()
teach the core device_lock() to consider a subsystem specified lock
class.
While this enables increased coverage of the device_lock() it is still
limited to one subsystem at a time unless / until a unique
"lockdep_mutex" is added for each subsystem that wants a distinct lock
class number space.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/base/core.c | 1 +
include/linux/device.h | 73 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7bb957b11861..96430fa5152e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2866,6 +2866,7 @@ void device_initialize(struct device *dev)
mutex_init(&dev->mutex);
#ifdef CONFIG_PROVE_LOCKING
mutex_init(&dev->lockdep_mutex);
+ dev->lock_class = -1;
#endif
lockdep_set_novalidate_class(&dev->mutex);
spin_lock_init(&dev->devres_lock);
diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..e313ff21d670 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -402,6 +402,7 @@ struct dev_msi_info {
* @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.
+ * @lock_class: per-subsystem annotated device lock class
* @bus: Type of bus device is on.
* @driver: Which driver has allocated this
* @platform_data: Platform data specific to the device.
@@ -501,6 +502,7 @@ struct device {
dev_set_drvdata/dev_get_drvdata */
#ifdef CONFIG_PROVE_LOCKING
struct mutex lockdep_mutex;
+ int lock_class;
#endif
struct mutex mutex; /* mutex to synchronize calls to
* its driver.
@@ -762,6 +764,12 @@ static inline bool dev_pm_test_driver_flags(struct device *dev, u32 flags)
return !!(dev->power.driver_flags & flags);
}
+static inline void device_lock_assert(struct device *dev)
+{
+ lockdep_assert_held(&dev->mutex);
+}
+
+#ifndef CONFIG_PROVE_LOCKING
static inline void device_lock(struct device *dev)
{
mutex_lock(&dev->mutex);
@@ -782,10 +790,71 @@ static inline void device_unlock(struct device *dev)
mutex_unlock(&dev->mutex);
}
-static inline void device_lock_assert(struct device *dev)
+static inline void device_set_lock_class(struct device *dev, int lock_class)
{
- lockdep_assert_held(&dev->mutex);
}
+#else
+static inline void device_lock(struct device *dev)
+{
+ lockdep_assert_not_held(&dev->lockdep_mutex);
+
+ mutex_lock(&dev->mutex);
+ if (dev->lock_class >= 0)
+ mutex_lock_nested(&dev->lockdep_mutex, dev->lock_class);
+}
+
+static inline int device_lock_interruptible(struct device *dev)
+{
+ int rc = mutex_lock_interruptible(&dev->mutex);
+
+ if (rc || dev->lock_class < 0)
+ return rc;
+
+ return mutex_lock_interruptible_nested(&dev->lockdep_mutex,
+ dev->lock_class);
+}
+
+static inline int device_trylock(struct device *dev)
+{
+ if (mutex_trylock(&dev->mutex)) {
+ if (dev->lock_class >= 0)
+ mutex_lock_nested(&dev->lockdep_mutex, dev->lock_class);
+ return 1;
+ }
+
+ return 0;
+}
+
+static inline void device_unlock(struct device *dev)
+{
+ if (dev->lock_class >= 0)
+ mutex_unlock(&dev->lockdep_mutex);
+ mutex_unlock(&dev->mutex);
+}
+
+static inline void device_set_lock_class(struct device *dev, int lock_class)
+{
+ if (dev->lock_class < 0 && lock_class > 0) {
+ if (mutex_is_locked(&dev->mutex)) {
+ /*
+ * device_unlock() will unlock lockdep_mutex now that
+ * lock_class is set, so take the paired lock now
+ */
+ mutex_lock_nested(&dev->lockdep_mutex, lock_class);
+ }
+ } else if (dev->lock_class >= 0 && lock_class < 0) {
+ if (mutex_is_locked(&dev->mutex)) {
+ /*
+ * device_unlock() will no longer drop lockdep_mutex now
+ * that lock_class is disabled, so drop the paired lock
+ * now.
+ */
+ mutex_unlock(&dev->lockdep_mutex);
+ }
+ }
+ dev->lock_class = lock_class;
+}
+#endif
static inline struct device_node *dev_of_node(struct device *dev)
{
In preparation for the libnvdimm subsystem switching to device-core
common lockdep validation. Delete nfit_device_lock() which will need to
be replaced with an implementation that specifies a non-zero lock class.
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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 e5d7f2bda13f..315fccdbfd84 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1305,7 +1305,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);
@@ -1322,7 +1322,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;
@@ -1342,10 +1342,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);
@@ -1362,7 +1362,7 @@ static ssize_t scrub_show(struct device *dev,
}
mutex_unlock(&acpi_desc->init_mutex);
- nfit_device_unlock(dev);
+ device_unlock(dev);
return rc;
}
@@ -1379,14 +1379,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;
@@ -1774,9 +1774,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)
@@ -3532,8 +3532,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);
@@ -3686,8 +3686,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);
}
@@ -3830,9 +3830,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 c674f3df9be7..2c16a2dafb15 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -343,30 +343,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);
On Wed, Mar 9, 2022 at 10:27 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Mon, 28 Feb 2022 18:49:11 -0800
> Dan Williams <[email protected]> wrote:
>
> > MAX_LOCKDEP_SUBCLASSES limits the depth of the CXL topology that can be
> > validated by lockdep. Given that the cxl_test topology is already at
> > this limit collapse some of the levels and clamp the max depth.
> >
> > Cc: Alison Schofield <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Cc: Ben Widawsky <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > drivers/cxl/cxl.h | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 97e6ca7e4940..1357a245037d 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -501,20 +501,33 @@ enum cxl_lock_class {
> > CXL_ANON_LOCK,
> > CXL_NVDIMM_LOCK,
> > CXL_NVDIMM_BRIDGE_LOCK,
>
> I'd be tempted to give explicit value to the one above as well
> so it's immediate clear there is deliberate duplication here.
Sounds good.
I also notice that clamp_lock_class() should return -1 when it wants
to disable validation, not zero.
On Mon, 28 Feb 2022 18:49:17 -0800
Dan Williams <[email protected]> wrote:
> Update CONFIG_PROVE_CXL_LOCKING to use the common device-core helpers
> for device_lock validation.
>
> When CONFIG_PROVE_LOCKING is enabled and device_set_lock_class() is
> passed a non-zero lock class the core acquires the 'struct device'
> @lockdep_mutex everywhere it acquires the device_lock. Where
> lockdep_mutex does not skip lockdep validation like device_lock.
>
> cxl_set_lock_class() wraps device_set_lock_class() as to not collide
> with other subsystems that may also support this lockdep validation
> scheme. See the 'choice' for the various CONFIG_PROVE_$SUBSYS_LOCKING
> options in lib/Kconfig.debug.
>
> Cc: Alison Schofield <[email protected]>
> Cc: Vishal Verma <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Ben Widawsky <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
One query inline - otherwise looks good to me.
> ---
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b1a4ba622739..f0a821de94cf 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -476,6 +476,7 @@ int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *cxlr)
> if (rc)
> goto err;
>
> + cxl_set_lock_class(dev);
I didn't see a cxl_lock_class for regions. Or is this meant to use
the ANON_LOCK?
> rc = device_add(dev);
> if (rc)
> goto err;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1357a245037d..f94eff659cce 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -543,5 +543,14 @@ static inline int cxl_lock_class(struct device *dev)
> else
> return CXL_ANON_LOCK;
> }
> +
> +static inline void cxl_set_lock_class(struct device *dev)
> +{
> + device_set_lock_class(dev, cxl_lock_class(dev));
> +}
> +#else
> +static inline void cxl_set_lock_class(struct device *dev)
> +{
> +}
> #endif
> #endif /* __CXL_H__ */
>
On Wed, Mar 9, 2022 at 10:33 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Mon, 28 Feb 2022 18:49:17 -0800
> Dan Williams <[email protected]> wrote:
>
> > Update CONFIG_PROVE_CXL_LOCKING to use the common device-core helpers
> > for device_lock validation.
> >
> > When CONFIG_PROVE_LOCKING is enabled and device_set_lock_class() is
> > passed a non-zero lock class the core acquires the 'struct device'
> > @lockdep_mutex everywhere it acquires the device_lock. Where
> > lockdep_mutex does not skip lockdep validation like device_lock.
> >
> > cxl_set_lock_class() wraps device_set_lock_class() as to not collide
> > with other subsystems that may also support this lockdep validation
> > scheme. See the 'choice' for the various CONFIG_PROVE_$SUBSYS_LOCKING
> > options in lib/Kconfig.debug.
> >
> > Cc: Alison Schofield <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Cc: Ben Widawsky <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
>
> One query inline - otherwise looks good to me.
>
> > ---
>
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index b1a4ba622739..f0a821de94cf 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -476,6 +476,7 @@ int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *cxlr)
> > if (rc)
> > goto err;
> >
> > + cxl_set_lock_class(dev);
>
> I didn't see a cxl_lock_class for regions. Or is this meant to use
> the ANON_LOCK?
Oh, yes, first I need to rebase this set before the region series
which is going through a major revision. Second, I expect that the
region lock_class may end up needing to nest inside the decoder lock
class in order to facilitate decoders disconnecting themselves from
regions if a memdev goes through ->remove() while an associated region
is active.
This series was motivated by wanting to validate the locking of the
region creation sysfs-ABI versus device removal events.
On Mon, 28 Feb 2022 18:49:11 -0800
Dan Williams <[email protected]> wrote:
> MAX_LOCKDEP_SUBCLASSES limits the depth of the CXL topology that can be
> validated by lockdep. Given that the cxl_test topology is already at
> this limit collapse some of the levels and clamp the max depth.
>
> Cc: Alison Schofield <[email protected]>
> Cc: Vishal Verma <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Ben Widawsky <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> drivers/cxl/cxl.h | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 97e6ca7e4940..1357a245037d 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -501,20 +501,33 @@ enum cxl_lock_class {
> CXL_ANON_LOCK,
> CXL_NVDIMM_LOCK,
> CXL_NVDIMM_BRIDGE_LOCK,
I'd be tempted to give explicit value to the one above as well
so it's immediate clear there is deliberate duplication here.
> - CXL_PORT_LOCK,
> + CXL_PORT_LOCK = 2,
> /*
> * 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.
> + * depth would need to be defined first. Also, the max
> + * validation depth is limited by MAX_LOCKDEP_SUBCLASSES.
> */
> };
>
> +static inline int clamp_lock_class(struct device *dev, int lock_class)
> +{
> + if (lock_class >= MAX_LOCKDEP_SUBCLASSES) {
> + dev_warn_once(dev,
> + "depth: %d, disabling lockdep for this device\n",
> + lock_class);
> + return 0;
> + }
> +
> + return lock_class;
> +}
> +
> static inline int cxl_lock_class(struct device *dev)
> {
> if (is_cxl_port(dev)) {
> struct cxl_port *port = to_cxl_port(dev);
>
> - return CXL_PORT_LOCK + port->depth;
> + return clamp_lock_class(dev, CXL_PORT_LOCK + port->depth);
> } else if (is_cxl_decoder(dev)) {
> struct cxl_port *port = to_cxl_port(dev->parent);
>
> @@ -522,7 +535,7 @@ static inline int cxl_lock_class(struct device *dev)
> * A decoder is the immediate child of a port, so set
> * its lock class equal to other child device siblings.
> */
> - return CXL_PORT_LOCK + port->depth + 1;
> + return clamp_lock_class(dev, CXL_PORT_LOCK + port->depth + 1);
> } else if (is_cxl_nvdimm_bridge(dev))
> return CXL_NVDIMM_BRIDGE_LOCK;
> else if (is_cxl_nvdimm(dev))
>