* [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
@ 2025-09-22 21:13 Dave Jiang
2025-09-22 22:50 ` dan.j.williams
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dave Jiang @ 2025-09-22 21:13 UTC (permalink / raw)
To: nvdimm
Cc: ira.weiny, vishal.l.verma, dan.j.williams, jonathan.cameron,
s.neeraj
Converting nvdimm_bus_lock/unlock to guard() to clean up usage
of gotos for error handling and avoid future mistakes of missed
unlock on error paths.
Link: https://lore.kernel.org/linux-cxl/20250917163623.00004a3c@huawei.com/
Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
Ira, up to you if you want to take this cleanup.
---
drivers/nvdimm/badrange.c | 3 +-
drivers/nvdimm/btt_devs.c | 20 +++-----
drivers/nvdimm/bus.c | 17 +++----
drivers/nvdimm/claim.c | 5 +-
drivers/nvdimm/core.c | 17 ++++---
drivers/nvdimm/dax_devs.c | 8 ++--
drivers/nvdimm/dimm.c | 17 ++++---
drivers/nvdimm/dimm_devs.c | 43 ++++++++---------
drivers/nvdimm/namespace_devs.c | 81 +++++++++++++++------------------
drivers/nvdimm/nd.h | 2 +
drivers/nvdimm/pfn_devs.c | 29 +++++-------
drivers/nvdimm/region.c | 14 +++---
drivers/nvdimm/region_devs.c | 72 ++++++++++++++---------------
drivers/nvdimm/security.c | 6 +--
14 files changed, 149 insertions(+), 185 deletions(-)
diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index ee478ccde7c6..36c626db459a 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -278,8 +278,7 @@ void nvdimm_badblocks_populate(struct nd_region *nd_region,
}
nvdimm_bus = walk_to_nvdimm_bus(&nd_region->dev);
- nvdimm_bus_lock(&nvdimm_bus->dev);
+ guard(nvdimm_bus)(&nvdimm_bus->dev);
badblocks_populate(&nvdimm_bus->badrange, bb, range);
- nvdimm_bus_unlock(&nvdimm_bus->dev);
}
EXPORT_SYMBOL_GPL(nvdimm_badblocks_populate);
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 497fd434a6a1..b35bcbe5db7f 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -50,14 +50,12 @@ static ssize_t sector_size_store(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
rc = nd_size_select_store(dev, buf, &nd_btt->lbasize,
btt_lbasize_supported);
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc ? rc : len;
}
@@ -95,10 +93,9 @@ static ssize_t namespace_show(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
rc = sprintf(buf, "%s\n", nd_btt->ndns
? dev_name(&nd_btt->ndns->dev) : "");
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -108,13 +105,11 @@ static ssize_t namespace_store(struct device *dev,
struct nd_btt *nd_btt = to_nd_btt(dev);
ssize_t rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
rc = nd_namespace_store(dev, &nd_btt->ndns, buf, len);
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc;
}
@@ -351,9 +346,8 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
return -ENODEV;
}
- nvdimm_bus_lock(&ndns->dev);
- btt_dev = __nd_btt_create(nd_region, 0, NULL, ndns);
- nvdimm_bus_unlock(&ndns->dev);
+ scoped_guard(nvdimm_bus, &ndns->dev)
+ btt_dev = __nd_btt_create(nd_region, 0, NULL, ndns);
if (!btt_dev)
return -ENOMEM;
btt_sb = devm_kzalloc(dev, sizeof(*btt_sb), GFP_KERNEL);
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 0ccf4a9e523a..d2c2d71e7fe0 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -64,17 +64,15 @@ static struct module *to_bus_provider(struct device *dev)
static void nvdimm_bus_probe_start(struct nvdimm_bus *nvdimm_bus)
{
- nvdimm_bus_lock(&nvdimm_bus->dev);
+ guard(nvdimm_bus)(&nvdimm_bus->dev);
nvdimm_bus->probe_active++;
- nvdimm_bus_unlock(&nvdimm_bus->dev);
}
static void nvdimm_bus_probe_end(struct nvdimm_bus *nvdimm_bus)
{
- nvdimm_bus_lock(&nvdimm_bus->dev);
+ guard(nvdimm_bus)(&nvdimm_bus->dev);
if (--nvdimm_bus->probe_active == 0)
wake_up(&nvdimm_bus->wait);
- nvdimm_bus_unlock(&nvdimm_bus->dev);
}
static int nvdimm_bus_probe(struct device *dev)
@@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
goto out;
}
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
if (rc)
- goto out_unlock;
+ goto out;
rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
if (rc < 0)
- goto out_unlock;
+ goto out;
if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
struct nd_cmd_clear_error *clear_err = buf;
@@ -1197,9 +1195,6 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
if (copy_to_user(p, buf, buf_len))
rc = -EFAULT;
-out_unlock:
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
out:
kfree(in_env);
kfree(out_env);
diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index 51614651d2e7..e53a2cc04695 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -35,9 +35,8 @@ void nd_detach_ndns(struct device *dev,
if (!ndns)
return;
get_device(&ndns->dev);
- nvdimm_bus_lock(&ndns->dev);
- __nd_detach_ndns(dev, _ndns);
- nvdimm_bus_unlock(&ndns->dev);
+ scoped_guard(nvdimm_bus, &ndns->dev)
+ __nd_detach_ndns(dev, _ndns);
put_device(&ndns->dev);
}
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index eaa796629c27..5ba204113fe1 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -141,9 +141,8 @@ static void nvdimm_map_put(void *data)
struct nvdimm_map *nvdimm_map = data;
struct nvdimm_bus *nvdimm_bus = nvdimm_map->nvdimm_bus;
- nvdimm_bus_lock(&nvdimm_bus->dev);
+ guard(nvdimm_bus)(&nvdimm_bus->dev);
kref_put(&nvdimm_map->kref, nvdimm_map_release);
- nvdimm_bus_unlock(&nvdimm_bus->dev);
}
/**
@@ -158,13 +157,13 @@ void *devm_nvdimm_memremap(struct device *dev, resource_size_t offset,
{
struct nvdimm_map *nvdimm_map;
- nvdimm_bus_lock(dev);
- nvdimm_map = find_nvdimm_map(dev, offset);
- if (!nvdimm_map)
- nvdimm_map = alloc_nvdimm_map(dev, offset, size, flags);
- else
- kref_get(&nvdimm_map->kref);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev) {
+ nvdimm_map = find_nvdimm_map(dev, offset);
+ if (!nvdimm_map)
+ nvdimm_map = alloc_nvdimm_map(dev, offset, size, flags);
+ else
+ kref_get(&nvdimm_map->kref);
+ }
if (!nvdimm_map)
return NULL;
diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
index 37b743acbb7b..a5d44b5c9452 100644
--- a/drivers/nvdimm/dax_devs.c
+++ b/drivers/nvdimm/dax_devs.c
@@ -104,10 +104,10 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
return -ENODEV;
}
- nvdimm_bus_lock(&ndns->dev);
- nd_dax = nd_dax_alloc(nd_region);
- dax_dev = nd_dax_devinit(nd_dax, ndns);
- nvdimm_bus_unlock(&ndns->dev);
+ scoped_guard(nvdimm_bus, &ndns->dev) {
+ nd_dax = nd_dax_alloc(nd_region);
+ dax_dev = nd_dax_devinit(nd_dax, ndns);
+ }
if (!dax_dev)
return -ENOMEM;
pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
index 91d9163ee303..2018458a3dba 100644
--- a/drivers/nvdimm/dimm.c
+++ b/drivers/nvdimm/dimm.c
@@ -95,13 +95,13 @@ static int nvdimm_probe(struct device *dev)
dev_dbg(dev, "config data size: %d\n", ndd->nsarea.config_size);
- nvdimm_bus_lock(dev);
- if (ndd->ns_current >= 0) {
- rc = nd_label_reserve_dpa(ndd);
- if (rc == 0)
- nvdimm_set_labeling(dev);
+ scoped_guard(nvdimm_bus, dev) {
+ if (ndd->ns_current >= 0) {
+ rc = nd_label_reserve_dpa(ndd);
+ if (rc == 0)
+ nvdimm_set_labeling(dev);
+ }
}
- nvdimm_bus_unlock(dev);
if (rc)
goto err;
@@ -117,9 +117,8 @@ static void nvdimm_remove(struct device *dev)
{
struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
- nvdimm_bus_lock(dev);
- dev_set_drvdata(dev, NULL);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev)
+ dev_set_drvdata(dev, NULL);
put_ndd(ndd);
}
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 21498d461fde..4b293c4ad15c 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -226,10 +226,10 @@ void nvdimm_drvdata_release(struct kref *kref)
struct resource *res, *_r;
dev_dbg(dev, "trace\n");
- nvdimm_bus_lock(dev);
- for_each_dpa_resource_safe(ndd, res, _r)
- nvdimm_free_dpa(ndd, res);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev) {
+ for_each_dpa_resource_safe(ndd, res, _r)
+ nvdimm_free_dpa(ndd, res);
+ }
kvfree(ndd->data);
kfree(ndd);
@@ -326,7 +326,7 @@ static ssize_t __available_slots_show(struct nvdimm_drvdata *ndd, char *buf)
return -ENXIO;
dev = ndd->dev;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
nfree = nd_label_nfree(ndd);
if (nfree - 1 > nfree) {
dev_WARN_ONCE(dev, 1, "we ate our last label?\n");
@@ -334,7 +334,6 @@ static ssize_t __available_slots_show(struct nvdimm_drvdata *ndd, char *buf)
} else
nfree--;
rc = sprintf(buf, "%d\n", nfree);
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -395,12 +394,10 @@ static ssize_t security_store(struct device *dev,
* done while probing is idle and the DIMM is not in active use
* in any region.
*/
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = nvdimm_security_store(dev, buf, len);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc;
}
@@ -454,9 +451,8 @@ static ssize_t result_show(struct device *dev, struct device_attribute *attr, ch
if (!nvdimm->fw_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(dev);
- result = nvdimm->fw_ops->activate_result(nvdimm);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev)
+ result = nvdimm->fw_ops->activate_result(nvdimm);
switch (result) {
case NVDIMM_FWA_RESULT_NONE:
@@ -483,9 +479,8 @@ static ssize_t activate_show(struct device *dev, struct device_attribute *attr,
if (!nvdimm->fw_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(dev);
- state = nvdimm->fw_ops->activate_state(nvdimm);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev)
+ state = nvdimm->fw_ops->activate_state(nvdimm);
switch (state) {
case NVDIMM_FWA_IDLE:
@@ -516,9 +511,8 @@ static ssize_t activate_store(struct device *dev, struct device_attribute *attr,
else
return -EINVAL;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
rc = nvdimm->fw_ops->arm(nvdimm, arg);
- nvdimm_bus_unlock(dev);
if (rc < 0)
return rc;
@@ -545,9 +539,8 @@ static umode_t nvdimm_firmware_visible(struct kobject *kobj, struct attribute *a
if (!nvdimm->fw_ops)
return 0;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
cap = nd_desc->fw_ops->capability(nd_desc);
- nvdimm_bus_unlock(dev);
if (cap < NVDIMM_FWA_CAP_QUIESCE)
return 0;
@@ -641,11 +634,11 @@ void nvdimm_delete(struct nvdimm *nvdimm)
bool dev_put = false;
/* We are shutting down. Make state frozen artificially. */
- nvdimm_bus_lock(dev);
- set_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags);
- if (test_and_clear_bit(NDD_WORK_PENDING, &nvdimm->flags))
- dev_put = true;
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev) {
+ set_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags);
+ if (test_and_clear_bit(NDD_WORK_PENDING, &nvdimm->flags))
+ dev_put = true;
+ }
cancel_delayed_work_sync(&nvdimm->dwork);
if (dev_put)
put_device(dev);
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 55cfbf1e0a95..38933abfb2a6 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -264,15 +264,13 @@ static ssize_t alt_name_store(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev->parent);
ssize_t rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __alt_name_store(dev, buf, len);
if (rc >= 0)
rc = nd_namespace_label_update(nd_region, dev);
dev_dbg(dev, "%s(%zd)\n", rc < 0 ? "fail " : "", rc);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -849,8 +847,8 @@ static ssize_t size_store(struct device *dev,
if (rc)
return rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __size_store(dev, val);
if (rc >= 0)
@@ -866,9 +864,6 @@ static ssize_t size_store(struct device *dev,
dev_dbg(dev, "%llx %s (%d)\n", val, rc < 0 ? "fail" : "success", rc);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
-
return rc < 0 ? rc : len;
}
@@ -893,9 +888,8 @@ resource_size_t nvdimm_namespace_capacity(struct nd_namespace_common *ndns)
{
resource_size_t size;
- nvdimm_bus_lock(&ndns->dev);
+ guard(nvdimm_bus)(&ndns->dev);
size = __nvdimm_namespace_capacity(ndns);
- nvdimm_bus_unlock(&ndns->dev);
return size;
}
@@ -1044,8 +1038,8 @@ static ssize_t uuid_store(struct device *dev,
} else
return -ENXIO;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
if (to_ndns(dev)->claim)
rc = -EBUSY;
@@ -1059,8 +1053,6 @@ static ssize_t uuid_store(struct device *dev,
kfree(uuid);
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -1119,8 +1111,8 @@ static ssize_t sector_size_store(struct device *dev,
} else
return -ENXIO;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
if (to_ndns(dev)->claim)
rc = -EBUSY;
if (rc >= 0)
@@ -1129,8 +1121,6 @@ static ssize_t sector_size_store(struct device *dev,
rc = nd_namespace_label_update(nd_region, dev);
dev_dbg(dev, "result: %zd %s: %s%s", rc, rc < 0 ? "tried" : "wrote",
buf, buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc ? rc : len;
}
@@ -1145,7 +1135,7 @@ static ssize_t dpa_extents_show(struct device *dev,
int count = 0, i;
u32 flags = 0;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (is_namespace_pmem(dev)) {
struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
@@ -1167,8 +1157,6 @@ static ssize_t dpa_extents_show(struct device *dev,
count++;
}
out:
- nvdimm_bus_unlock(dev);
-
return sprintf(buf, "%d\n", count);
}
static DEVICE_ATTR_RO(dpa_extents);
@@ -1279,15 +1267,13 @@ static ssize_t holder_class_store(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev->parent);
int rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
rc = __holder_class_store(dev, buf);
if (rc >= 0)
rc = nd_namespace_label_update(nd_region, dev);
dev_dbg(dev, "%s(%d)\n", rc < 0 ? "fail " : "", rc);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc < 0 ? rc : len;
}
@@ -2152,31 +2138,38 @@ static int init_active_labels(struct nd_region *nd_region)
nd_region);
}
+static int __create_namespaces(struct nd_region *nd_region, int *type,
+ struct device ***devs)
+{
+ int rc;
+
+ guard(nvdimm_bus)(&nd_region->dev);
+ rc = init_active_labels(nd_region);
+ if (rc)
+ return rc;
+
+ *type = nd_region_to_nstype(nd_region);
+ switch (*type) {
+ case ND_DEVICE_NAMESPACE_IO:
+ *devs = create_namespace_io(nd_region);
+ break;
+ case ND_DEVICE_NAMESPACE_PMEM:
+ *devs = create_namespaces(nd_region);
+ break;
+ }
+
+ return 0;
+}
+
int nd_region_register_namespaces(struct nd_region *nd_region, int *err)
{
struct device **devs = NULL;
int i, rc = 0, type;
*err = 0;
- nvdimm_bus_lock(&nd_region->dev);
- rc = init_active_labels(nd_region);
- if (rc) {
- nvdimm_bus_unlock(&nd_region->dev);
+ rc = __create_namespaces(nd_region, &type, &devs);
+ if (rc)
return rc;
- }
-
- type = nd_region_to_nstype(nd_region);
- switch (type) {
- case ND_DEVICE_NAMESPACE_IO:
- devs = create_namespace_io(nd_region);
- break;
- case ND_DEVICE_NAMESPACE_PMEM:
- devs = create_namespaces(nd_region);
- break;
- default:
- break;
- }
- nvdimm_bus_unlock(&nd_region->dev);
if (!devs)
return -ENODEV;
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index cc5c8f3f81e8..a8013033509c 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -632,6 +632,8 @@ u64 nd_region_interleave_set_cookie(struct nd_region *nd_region,
u64 nd_region_interleave_set_altcookie(struct nd_region *nd_region);
void nvdimm_bus_lock(struct device *dev);
void nvdimm_bus_unlock(struct device *dev);
+DEFINE_GUARD(nvdimm_bus, struct device *, nvdimm_bus_lock(_T), nvdimm_bus_unlock(_T));
+
bool is_nvdimm_bus_locked(struct device *dev);
void nvdimm_check_and_set_ro(struct gendisk *disk);
void nvdimm_drvdata_release(struct kref *kref);
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 8f3e816e805d..f2a44d1f62be 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -57,8 +57,8 @@ static ssize_t mode_store(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc = 0;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
if (dev->driver)
rc = -EBUSY;
else {
@@ -78,8 +78,6 @@ static ssize_t mode_store(struct device *dev,
}
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc ? rc : len;
}
@@ -125,14 +123,12 @@ static ssize_t align_store(struct device *dev,
unsigned long aligns[MAX_NVDIMM_ALIGN] = { [0] = 0, };
ssize_t rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(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);
- device_unlock(dev);
return rc ? rc : len;
}
@@ -170,10 +166,9 @@ static ssize_t namespace_show(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
rc = sprintf(buf, "%s\n", nd_pfn->ndns
? dev_name(&nd_pfn->ndns->dev) : "");
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -183,13 +178,11 @@ static ssize_t namespace_store(struct device *dev,
struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
ssize_t rc;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
rc = nd_namespace_store(dev, &nd_pfn->ndns, buf, len);
dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
buf[len - 1] == '\n' ? "" : "\n");
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return rc;
}
@@ -639,10 +632,10 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
return -ENODEV;
}
- nvdimm_bus_lock(&ndns->dev);
- nd_pfn = nd_pfn_alloc(nd_region);
- pfn_dev = nd_pfn_devinit(nd_pfn, ndns);
- nvdimm_bus_unlock(&ndns->dev);
+ scoped_guard(nvdimm_bus, &ndns->dev) {
+ nd_pfn = nd_pfn_alloc(nd_region);
+ pfn_dev = nd_pfn_devinit(nd_pfn, ndns);
+ }
if (!pfn_dev)
return -ENOMEM;
pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
diff --git a/drivers/nvdimm/region.c b/drivers/nvdimm/region.c
index 88dc062af5f8..2abbdcef8d33 100644
--- a/drivers/nvdimm/region.c
+++ b/drivers/nvdimm/region.c
@@ -87,13 +87,13 @@ static void nd_region_remove(struct device *dev)
device_for_each_child(dev, NULL, child_unregister);
/* flush attribute readers and disable */
- nvdimm_bus_lock(dev);
- nd_region->ns_seed = NULL;
- nd_region->btt_seed = NULL;
- nd_region->pfn_seed = NULL;
- nd_region->dax_seed = NULL;
- dev_set_drvdata(dev, NULL);
- nvdimm_bus_unlock(dev);
+ scoped_guard(nvdimm_bus, dev) {
+ nd_region->ns_seed = NULL;
+ nd_region->btt_seed = NULL;
+ nd_region->pfn_seed = NULL;
+ nd_region->dax_seed = NULL;
+ dev_set_drvdata(dev, NULL);
+ }
/*
* Note, this assumes device_lock() context to not race
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index de1ee5ebc851..b2b4519e9f4c 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -102,31 +102,44 @@ static int nd_region_invalidate_memregion(struct nd_region *nd_region)
return 0;
}
-int nd_region_activate(struct nd_region *nd_region)
+static int get_flush_data(struct nd_region *nd_region, size_t *size, int *num_flush)
{
- int i, j, rc, num_flush = 0;
- struct nd_region_data *ndrd;
- struct device *dev = &nd_region->dev;
size_t flush_data_size = sizeof(void *);
+ int _num_flush = 0;
+ int i;
- nvdimm_bus_lock(&nd_region->dev);
+ guard(nvdimm_bus)(&nd_region->dev);
for (i = 0; i < nd_region->ndr_mappings; i++) {
struct nd_mapping *nd_mapping = &nd_region->mapping[i];
struct nvdimm *nvdimm = nd_mapping->nvdimm;
- if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
- nvdimm_bus_unlock(&nd_region->dev);
+ if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags))
return -EBUSY;
- }
/* at least one null hint slot per-dimm for the "no-hint" case */
flush_data_size += sizeof(void *);
- num_flush = min_not_zero(num_flush, nvdimm->num_flush);
+ _num_flush = min_not_zero(_num_flush, nvdimm->num_flush);
if (!nvdimm->num_flush)
continue;
flush_data_size += nvdimm->num_flush * sizeof(void *);
}
- nvdimm_bus_unlock(&nd_region->dev);
+
+ *size = flush_data_size;
+ *num_flush = _num_flush;
+
+ return 0;
+}
+
+int nd_region_activate(struct nd_region *nd_region)
+{
+ int i, j, rc, num_flush;
+ struct nd_region_data *ndrd;
+ struct device *dev = &nd_region->dev;
+ size_t flush_data_size;
+
+ rc = get_flush_data(nd_region, &flush_data_size, &num_flush);
+ if (rc)
+ return rc;
rc = nd_region_invalidate_memregion(nd_region);
if (rc)
@@ -327,8 +340,8 @@ static ssize_t set_cookie_show(struct device *dev,
* the v1.1 namespace label cookie definition. To read all this
* data we need to wait for probing to settle.
*/
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
if (nd_region->ndr_mappings) {
struct nd_mapping *nd_mapping = &nd_region->mapping[0];
@@ -343,8 +356,6 @@ static ssize_t set_cookie_show(struct device *dev,
nsindex));
}
}
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
if (rc)
return rc;
@@ -401,12 +412,10 @@ static ssize_t available_size_show(struct device *dev,
* memory nvdimm_bus_lock() is dropped, but that's userspace's
* problem to not race itself.
*/
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
available = nd_region_available_dpa(nd_region);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return sprintf(buf, "%llu\n", available);
}
@@ -418,12 +427,10 @@ static ssize_t max_available_extent_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
unsigned long long available = 0;
- device_lock(dev);
- nvdimm_bus_lock(dev);
+ guard(device)(dev);
+ guard(nvdimm_bus)(dev);
wait_nvdimm_bus_probe_idle(dev);
available = nd_region_allocatable_dpa(nd_region);
- nvdimm_bus_unlock(dev);
- device_unlock(dev);
return sprintf(buf, "%llu\n", available);
}
@@ -435,12 +442,11 @@ static ssize_t init_namespaces_show(struct device *dev,
struct nd_region_data *ndrd = dev_get_drvdata(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (ndrd)
rc = sprintf(buf, "%d/%d\n", ndrd->ns_active, ndrd->ns_count);
else
rc = -ENXIO;
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -452,12 +458,11 @@ static ssize_t namespace_seed_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (nd_region->ns_seed)
rc = sprintf(buf, "%s\n", dev_name(nd_region->ns_seed));
else
rc = sprintf(buf, "\n");
- nvdimm_bus_unlock(dev);
return rc;
}
static DEVICE_ATTR_RO(namespace_seed);
@@ -468,12 +473,11 @@ static ssize_t btt_seed_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (nd_region->btt_seed)
rc = sprintf(buf, "%s\n", dev_name(nd_region->btt_seed));
else
rc = sprintf(buf, "\n");
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -485,12 +489,11 @@ static ssize_t pfn_seed_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (nd_region->pfn_seed)
rc = sprintf(buf, "%s\n", dev_name(nd_region->pfn_seed));
else
rc = sprintf(buf, "\n");
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -502,12 +505,11 @@ static ssize_t dax_seed_show(struct device *dev,
struct nd_region *nd_region = to_nd_region(dev);
ssize_t rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (nd_region->dax_seed)
rc = sprintf(buf, "%s\n", dev_name(nd_region->dax_seed));
else
rc = sprintf(buf, "\n");
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -581,9 +583,8 @@ static ssize_t align_store(struct device *dev,
* times ensure it does not change for the duration of the
* allocation.
*/
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
nd_region->align = val;
- nvdimm_bus_unlock(dev);
return len;
}
@@ -890,7 +891,7 @@ void nd_mapping_free_labels(struct nd_mapping *nd_mapping)
*/
void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
{
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
if (nd_region->ns_seed == dev) {
nd_region_create_ns_seed(nd_region);
} else if (is_nd_btt(dev)) {
@@ -915,7 +916,6 @@ void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev)
if (nd_region->ns_seed == &nd_dax->nd_pfn.ndns->dev)
nd_region_create_ns_seed(nd_region);
}
- nvdimm_bus_unlock(dev);
}
/**
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index a03e3c45f297..e70dc3b08458 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -221,9 +221,8 @@ int nvdimm_security_unlock(struct device *dev)
struct nvdimm *nvdimm = to_nvdimm(dev);
int rc;
- nvdimm_bus_lock(dev);
+ guard(nvdimm_bus)(dev);
rc = __nvdimm_security_unlock(nvdimm);
- nvdimm_bus_unlock(dev);
return rc;
}
@@ -490,9 +489,8 @@ void nvdimm_security_overwrite_query(struct work_struct *work)
struct nvdimm *nvdimm =
container_of(work, typeof(*nvdimm), dwork.work);
- nvdimm_bus_lock(&nvdimm->dev);
+ guard(nvdimm_bus)(&nvdimm->dev);
__nvdimm_security_overwrite_query(nvdimm);
- nvdimm_bus_unlock(&nvdimm->dev);
}
#define OPS \
base-commit: 07e27ad16399afcd693be20211b0dfae63e0615f
--
2.51.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-22 21:13 [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock Dave Jiang
@ 2025-09-22 22:50 ` dan.j.williams
2025-09-22 23:00 ` Dave Jiang
2025-09-23 1:58 ` kernel test robot
2025-09-23 10:18 ` Jonathan Cameron
2 siblings, 1 reply; 7+ messages in thread
From: dan.j.williams @ 2025-09-22 22:50 UTC (permalink / raw)
To: Dave Jiang, nvdimm
Cc: ira.weiny, vishal.l.verma, dan.j.williams, jonathan.cameron,
s.neeraj
Dave Jiang wrote:
> Converting nvdimm_bus_lock/unlock to guard() to clean up usage
> of gotos for error handling and avoid future mistakes of missed
> unlock on error paths.
>
> Link: https://lore.kernel.org/linux-cxl/20250917163623.00004a3c@huawei.com/
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> Ira, up to you if you want to take this cleanup.
> ---
[..]
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 0ccf4a9e523a..d2c2d71e7fe0 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
> goto out;
> }
>
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
> if (rc)
> - goto out_unlock;
> + goto out;
>
> rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
> if (rc < 0)
> - goto out_unlock;
> + goto out;
>
> if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
> struct nd_cmd_clear_error *clear_err = buf;
[..]
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 91d9163ee303..2018458a3dba 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -95,13 +95,13 @@ static int nvdimm_probe(struct device *dev)
>
> dev_dbg(dev, "config data size: %d\n", ndd->nsarea.config_size);
>
> - nvdimm_bus_lock(dev);
> - if (ndd->ns_current >= 0) {
> - rc = nd_label_reserve_dpa(ndd);
> - if (rc == 0)
> - nvdimm_set_labeling(dev);
> + scoped_guard(nvdimm_bus, dev) {
> + if (ndd->ns_current >= 0) {
> + rc = nd_label_reserve_dpa(ndd);
> + if (rc == 0)
> + nvdimm_set_labeling(dev);
> + }
> }
> - nvdimm_bus_unlock(dev);
>
> if (rc)
> goto err;
[.. trim the hunks that successfully eliminated goto ..]
My litmus test for scoped-based-cleanup conversions is whether *all* of
the gotos are removed in a converted function. So either fully convert
all error unwind in a function to scope-based, or none of it. Do not
leave people to reason about potentially jumping through scopes with
goto.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-22 22:50 ` dan.j.williams
@ 2025-09-22 23:00 ` Dave Jiang
2025-09-22 23:23 ` dan.j.williams
0 siblings, 1 reply; 7+ messages in thread
From: Dave Jiang @ 2025-09-22 23:00 UTC (permalink / raw)
To: dan.j.williams, nvdimm
Cc: ira.weiny, vishal.l.verma, jonathan.cameron, s.neeraj
On 9/22/25 3:50 PM, dan.j.williams@intel.com wrote:
> Dave Jiang wrote:
>> Converting nvdimm_bus_lock/unlock to guard() to clean up usage
>> of gotos for error handling and avoid future mistakes of missed
>> unlock on error paths.
>>
>> Link: https://lore.kernel.org/linux-cxl/20250917163623.00004a3c@huawei.com/
>> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> Ira, up to you if you want to take this cleanup.
>> ---
> [..]
>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> index 0ccf4a9e523a..d2c2d71e7fe0 100644
>> --- a/drivers/nvdimm/bus.c
>> +++ b/drivers/nvdimm/bus.c
>> @@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>> goto out;
>> }
>>
>> - device_lock(dev);
>> - nvdimm_bus_lock(dev);
>> + guard(device)(dev);
>> + guard(nvdimm_bus)(dev);
>> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
>> if (rc)
>> - goto out_unlock;
>> + goto out;
>>
>> rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
>> if (rc < 0)
>> - goto out_unlock;
>> + goto out;
>>
>> if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
>> struct nd_cmd_clear_error *clear_err = buf;
> [..]
>> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
>> index 91d9163ee303..2018458a3dba 100644
>> --- a/drivers/nvdimm/dimm.c
>> +++ b/drivers/nvdimm/dimm.c
>> @@ -95,13 +95,13 @@ static int nvdimm_probe(struct device *dev)
>>
>> dev_dbg(dev, "config data size: %d\n", ndd->nsarea.config_size);
>>
>> - nvdimm_bus_lock(dev);
>> - if (ndd->ns_current >= 0) {
>> - rc = nd_label_reserve_dpa(ndd);
>> - if (rc == 0)
>> - nvdimm_set_labeling(dev);
>> + scoped_guard(nvdimm_bus, dev) {
>> + if (ndd->ns_current >= 0) {
>> + rc = nd_label_reserve_dpa(ndd);
>> + if (rc == 0)
>> + nvdimm_set_labeling(dev);
>> + }
>> }
>> - nvdimm_bus_unlock(dev);
>>
>> if (rc)
>> goto err;
> [.. trim the hunks that successfully eliminated goto ..]
>
> My litmus test for scoped-based-cleanup conversions is whether *all* of
> the gotos are removed in a converted function. So either fully convert
> all error unwind in a function to scope-based, or none of it. Do not
> leave people to reason about potentially jumping through scopes with
> goto.
This needs a bit more changes. I was undecided if I should include those changes in this patch or if they push out of scope. But it sounds like I should include them.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-22 23:00 ` Dave Jiang
@ 2025-09-22 23:23 ` dan.j.williams
0 siblings, 0 replies; 7+ messages in thread
From: dan.j.williams @ 2025-09-22 23:23 UTC (permalink / raw)
To: Dave Jiang, dan.j.williams, nvdimm
Cc: ira.weiny, vishal.l.verma, jonathan.cameron, s.neeraj
Dave Jiang wrote:
[..]
> This needs a bit more changes. I was undecided if I should include
> those changes in this patch or if they push out of scope. But it
> sounds like I should include them.
You can do them in their own patches, but I would keep those patches
together in a series.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-22 21:13 [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock Dave Jiang
2025-09-22 22:50 ` dan.j.williams
@ 2025-09-23 1:58 ` kernel test robot
2025-09-23 10:18 ` Jonathan Cameron
2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2025-09-23 1:58 UTC (permalink / raw)
To: Dave Jiang, nvdimm
Cc: llvm, oe-kbuild-all, ira.weiny, vishal.l.verma, dan.j.williams,
jonathan.cameron, s.neeraj
Hi Dave,
kernel test robot noticed the following build errors:
[auto build test ERROR on 07e27ad16399afcd693be20211b0dfae63e0615f]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Jiang/nvdimm-Introduce-guard-for-nvdimm_bus_lock/20250923-051440
base: 07e27ad16399afcd693be20211b0dfae63e0615f
patch link: https://lore.kernel.org/r/20250922211330.1433044-1-dave.jiang%40intel.com
patch subject: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
config: x86_64-buildonly-randconfig-006-20250923 (https://download.01.org/0day-ci/archive/20250923/202509230941.jMdKMPL8-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250923/202509230941.jMdKMPL8-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509230941.jMdKMPL8-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/nvdimm/bus.c:1175:3: error: cannot jump from this goto statement to its label
1175 | goto out;
| ^
drivers/nvdimm/bus.c:1179:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1179 | guard(nvdimm_bus)(dev);
| ^
include/linux/cleanup.h:401:15: note: expanded from macro 'guard'
401 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:93:1: note: expanded from here
93 | __UNIQUE_ID_guard458
| ^
drivers/nvdimm/bus.c:1178:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1178 | guard(device)(dev);
| ^
include/linux/cleanup.h:401:15: note: expanded from macro 'guard'
401 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:84:1: note: expanded from here
84 | __UNIQUE_ID_guard457
| ^
drivers/nvdimm/bus.c:1170:3: error: cannot jump from this goto statement to its label
1170 | goto out;
| ^
drivers/nvdimm/bus.c:1179:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1179 | guard(nvdimm_bus)(dev);
| ^
include/linux/cleanup.h:401:15: note: expanded from macro 'guard'
401 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:93:1: note: expanded from here
93 | __UNIQUE_ID_guard458
| ^
drivers/nvdimm/bus.c:1178:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1178 | guard(device)(dev);
| ^
include/linux/cleanup.h:401:15: note: expanded from macro 'guard'
401 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:84:1: note: expanded from here
84 | __UNIQUE_ID_guard457
| ^
drivers/nvdimm/bus.c:1164:3: error: cannot jump from this goto statement to its label
1164 | goto out;
| ^
drivers/nvdimm/bus.c:1179:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1179 | guard(nvdimm_bus)(dev);
| ^
include/linux/cleanup.h:401:15: note: expanded from macro 'guard'
401 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:93:1: note: expanded from here
93 | __UNIQUE_ID_guard458
| ^
drivers/nvdimm/bus.c:1178:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1178 | guard(device)(dev);
vim +1175 drivers/nvdimm/bus.c
eaf961536e1622 Dan Williams 2015-05-01 1023
62232e45f4a265 Dan Williams 2015-06-08 1024 static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
62232e45f4a265 Dan Williams 2015-06-08 1025 int read_only, unsigned int ioctl_cmd, unsigned long arg)
62232e45f4a265 Dan Williams 2015-06-08 1026 {
62232e45f4a265 Dan Williams 2015-06-08 1027 struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
62232e45f4a265 Dan Williams 2015-06-08 1028 const struct nd_cmd_desc *desc = NULL;
62232e45f4a265 Dan Williams 2015-06-08 1029 unsigned int cmd = _IOC_NR(ioctl_cmd);
62232e45f4a265 Dan Williams 2015-06-08 1030 struct device *dev = &nvdimm_bus->dev;
58738c495e15ba Dan Williams 2017-08-31 1031 void __user *p = (void __user *) arg;
6de5d06e657acd Dan Williams 2019-07-17 1032 char *out_env = NULL, *in_env = NULL;
62232e45f4a265 Dan Williams 2015-06-08 1033 const char *cmd_name, *dimm_name;
58738c495e15ba Dan Williams 2017-08-31 1034 u32 in_len = 0, out_len = 0;
58738c495e15ba Dan Williams 2017-08-31 1035 unsigned int func = cmd;
e3654eca70d637 Dan Williams 2016-04-28 1036 unsigned long cmd_mask;
58738c495e15ba Dan Williams 2017-08-31 1037 struct nd_cmd_pkg pkg;
006358b35c73ab Dave Jiang 2017-04-07 1038 int rc, i, cmd_rc;
6de5d06e657acd Dan Williams 2019-07-17 1039 void *buf = NULL;
58738c495e15ba Dan Williams 2017-08-31 1040 u64 buf_len = 0;
62232e45f4a265 Dan Williams 2015-06-08 1041
62232e45f4a265 Dan Williams 2015-06-08 1042 if (nvdimm) {
62232e45f4a265 Dan Williams 2015-06-08 1043 desc = nd_cmd_dimm_desc(cmd);
62232e45f4a265 Dan Williams 2015-06-08 1044 cmd_name = nvdimm_cmd_name(cmd);
e3654eca70d637 Dan Williams 2016-04-28 1045 cmd_mask = nvdimm->cmd_mask;
62232e45f4a265 Dan Williams 2015-06-08 1046 dimm_name = dev_name(&nvdimm->dev);
62232e45f4a265 Dan Williams 2015-06-08 1047 } else {
62232e45f4a265 Dan Williams 2015-06-08 1048 desc = nd_cmd_bus_desc(cmd);
62232e45f4a265 Dan Williams 2015-06-08 1049 cmd_name = nvdimm_bus_cmd_name(cmd);
e3654eca70d637 Dan Williams 2016-04-28 1050 cmd_mask = nd_desc->cmd_mask;
62232e45f4a265 Dan Williams 2015-06-08 1051 dimm_name = "bus";
62232e45f4a265 Dan Williams 2015-06-08 1052 }
62232e45f4a265 Dan Williams 2015-06-08 1053
92fe2aa859f52c Dan Williams 2020-07-20 1054 /* Validate command family support against bus declared support */
31eca76ba2fc98 Dan Williams 2016-04-28 1055 if (cmd == ND_CMD_CALL) {
92fe2aa859f52c Dan Williams 2020-07-20 1056 unsigned long *mask;
92fe2aa859f52c Dan Williams 2020-07-20 1057
31eca76ba2fc98 Dan Williams 2016-04-28 1058 if (copy_from_user(&pkg, p, sizeof(pkg)))
31eca76ba2fc98 Dan Williams 2016-04-28 1059 return -EFAULT;
92fe2aa859f52c Dan Williams 2020-07-20 1060
92fe2aa859f52c Dan Williams 2020-07-20 1061 if (nvdimm) {
92fe2aa859f52c Dan Williams 2020-07-20 1062 if (pkg.nd_family > NVDIMM_FAMILY_MAX)
92fe2aa859f52c Dan Williams 2020-07-20 1063 return -EINVAL;
92fe2aa859f52c Dan Williams 2020-07-20 1064 mask = &nd_desc->dimm_family_mask;
92fe2aa859f52c Dan Williams 2020-07-20 1065 } else {
92fe2aa859f52c Dan Williams 2020-07-20 1066 if (pkg.nd_family > NVDIMM_BUS_FAMILY_MAX)
92fe2aa859f52c Dan Williams 2020-07-20 1067 return -EINVAL;
92fe2aa859f52c Dan Williams 2020-07-20 1068 mask = &nd_desc->bus_family_mask;
92fe2aa859f52c Dan Williams 2020-07-20 1069 }
92fe2aa859f52c Dan Williams 2020-07-20 1070
92fe2aa859f52c Dan Williams 2020-07-20 1071 if (!test_bit(pkg.nd_family, mask))
92fe2aa859f52c Dan Williams 2020-07-20 1072 return -EINVAL;
31eca76ba2fc98 Dan Williams 2016-04-28 1073 }
31eca76ba2fc98 Dan Williams 2016-04-28 1074
f84afbdd3a9e5e Dan Carpenter 2020-02-25 1075 if (!desc ||
f84afbdd3a9e5e Dan Carpenter 2020-02-25 1076 (desc->out_num + desc->in_num == 0) ||
f84afbdd3a9e5e Dan Carpenter 2020-02-25 1077 cmd > ND_CMD_CALL ||
e3654eca70d637 Dan Williams 2016-04-28 1078 !test_bit(cmd, &cmd_mask))
62232e45f4a265 Dan Williams 2015-06-08 1079 return -ENOTTY;
62232e45f4a265 Dan Williams 2015-06-08 1080
62232e45f4a265 Dan Williams 2015-06-08 1081 /* fail write commands (when read-only) */
62232e45f4a265 Dan Williams 2015-06-08 1082 if (read_only)
07accfa9d1a8ba Jerry Hoemann 2016-01-06 1083 switch (cmd) {
07accfa9d1a8ba Jerry Hoemann 2016-01-06 1084 case ND_CMD_VENDOR:
07accfa9d1a8ba Jerry Hoemann 2016-01-06 1085 case ND_CMD_SET_CONFIG_DATA:
07accfa9d1a8ba Jerry Hoemann 2016-01-06 1086 case ND_CMD_ARS_START:
d4f323672aa637 Dan Williams 2016-03-03 1087 case ND_CMD_CLEAR_ERROR:
31eca76ba2fc98 Dan Williams 2016-04-28 1088 case ND_CMD_CALL:
ca6bf264f6d856 Dan Williams 2019-07-17 1089 dev_dbg(dev, "'%s' command while read-only.\n",
62232e45f4a265 Dan Williams 2015-06-08 1090 nvdimm ? nvdimm_cmd_name(cmd)
62232e45f4a265 Dan Williams 2015-06-08 1091 : nvdimm_bus_cmd_name(cmd));
62232e45f4a265 Dan Williams 2015-06-08 1092 return -EPERM;
62232e45f4a265 Dan Williams 2015-06-08 1093 default:
62232e45f4a265 Dan Williams 2015-06-08 1094 break;
62232e45f4a265 Dan Williams 2015-06-08 1095 }
62232e45f4a265 Dan Williams 2015-06-08 1096
62232e45f4a265 Dan Williams 2015-06-08 1097 /* process an input envelope */
6de5d06e657acd Dan Williams 2019-07-17 1098 in_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
6de5d06e657acd Dan Williams 2019-07-17 1099 if (!in_env)
6de5d06e657acd Dan Williams 2019-07-17 1100 return -ENOMEM;
62232e45f4a265 Dan Williams 2015-06-08 1101 for (i = 0; i < desc->in_num; i++) {
62232e45f4a265 Dan Williams 2015-06-08 1102 u32 in_size, copy;
62232e45f4a265 Dan Williams 2015-06-08 1103
62232e45f4a265 Dan Williams 2015-06-08 1104 in_size = nd_cmd_in_size(nvdimm, cmd, desc, i, in_env);
62232e45f4a265 Dan Williams 2015-06-08 1105 if (in_size == UINT_MAX) {
62232e45f4a265 Dan Williams 2015-06-08 1106 dev_err(dev, "%s:%s unknown input size cmd: %s field: %d\n",
62232e45f4a265 Dan Williams 2015-06-08 1107 __func__, dimm_name, cmd_name, i);
6de5d06e657acd Dan Williams 2019-07-17 1108 rc = -ENXIO;
6de5d06e657acd Dan Williams 2019-07-17 1109 goto out;
45def22c1fab85 Dan Williams 2015-04-26 1110 }
6de5d06e657acd Dan Williams 2019-07-17 1111 if (in_len < ND_CMD_MAX_ENVELOPE)
6de5d06e657acd Dan Williams 2019-07-17 1112 copy = min_t(u32, ND_CMD_MAX_ENVELOPE - in_len, in_size);
62232e45f4a265 Dan Williams 2015-06-08 1113 else
62232e45f4a265 Dan Williams 2015-06-08 1114 copy = 0;
6de5d06e657acd Dan Williams 2019-07-17 1115 if (copy && copy_from_user(&in_env[in_len], p + in_len, copy)) {
6de5d06e657acd Dan Williams 2019-07-17 1116 rc = -EFAULT;
6de5d06e657acd Dan Williams 2019-07-17 1117 goto out;
6de5d06e657acd Dan Williams 2019-07-17 1118 }
62232e45f4a265 Dan Williams 2015-06-08 1119 in_len += in_size;
62232e45f4a265 Dan Williams 2015-06-08 1120 }
62232e45f4a265 Dan Williams 2015-06-08 1121
31eca76ba2fc98 Dan Williams 2016-04-28 1122 if (cmd == ND_CMD_CALL) {
53b85a449b15e0 Jerry Hoemann 2017-06-30 1123 func = pkg.nd_command;
426824d63b77bd Dan Williams 2018-03-05 1124 dev_dbg(dev, "%s, idx: %llu, in: %u, out: %u, len %llu\n",
426824d63b77bd Dan Williams 2018-03-05 1125 dimm_name, pkg.nd_command,
31eca76ba2fc98 Dan Williams 2016-04-28 1126 in_len, out_len, buf_len);
31eca76ba2fc98 Dan Williams 2016-04-28 1127 }
31eca76ba2fc98 Dan Williams 2016-04-28 1128
62232e45f4a265 Dan Williams 2015-06-08 1129 /* process an output envelope */
6de5d06e657acd Dan Williams 2019-07-17 1130 out_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
6de5d06e657acd Dan Williams 2019-07-17 1131 if (!out_env) {
6de5d06e657acd Dan Williams 2019-07-17 1132 rc = -ENOMEM;
6de5d06e657acd Dan Williams 2019-07-17 1133 goto out;
6de5d06e657acd Dan Williams 2019-07-17 1134 }
6de5d06e657acd Dan Williams 2019-07-17 1135
62232e45f4a265 Dan Williams 2015-06-08 1136 for (i = 0; i < desc->out_num; i++) {
62232e45f4a265 Dan Williams 2015-06-08 1137 u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
efda1b5d87cbc3 Dan Williams 2016-12-06 1138 (u32 *) in_env, (u32 *) out_env, 0);
62232e45f4a265 Dan Williams 2015-06-08 1139 u32 copy;
62232e45f4a265 Dan Williams 2015-06-08 1140
62232e45f4a265 Dan Williams 2015-06-08 1141 if (out_size == UINT_MAX) {
426824d63b77bd Dan Williams 2018-03-05 1142 dev_dbg(dev, "%s unknown output size cmd: %s field: %d\n",
426824d63b77bd Dan Williams 2018-03-05 1143 dimm_name, cmd_name, i);
6de5d06e657acd Dan Williams 2019-07-17 1144 rc = -EFAULT;
6de5d06e657acd Dan Williams 2019-07-17 1145 goto out;
62232e45f4a265 Dan Williams 2015-06-08 1146 }
6de5d06e657acd Dan Williams 2019-07-17 1147 if (out_len < ND_CMD_MAX_ENVELOPE)
6de5d06e657acd Dan Williams 2019-07-17 1148 copy = min_t(u32, ND_CMD_MAX_ENVELOPE - out_len, out_size);
62232e45f4a265 Dan Williams 2015-06-08 1149 else
62232e45f4a265 Dan Williams 2015-06-08 1150 copy = 0;
62232e45f4a265 Dan Williams 2015-06-08 1151 if (copy && copy_from_user(&out_env[out_len],
6de5d06e657acd Dan Williams 2019-07-17 1152 p + in_len + out_len, copy)) {
6de5d06e657acd Dan Williams 2019-07-17 1153 rc = -EFAULT;
6de5d06e657acd Dan Williams 2019-07-17 1154 goto out;
6de5d06e657acd Dan Williams 2019-07-17 1155 }
62232e45f4a265 Dan Williams 2015-06-08 1156 out_len += out_size;
62232e45f4a265 Dan Williams 2015-06-08 1157 }
62232e45f4a265 Dan Williams 2015-06-08 1158
58738c495e15ba Dan Williams 2017-08-31 1159 buf_len = (u64) out_len + (u64) in_len;
62232e45f4a265 Dan Williams 2015-06-08 1160 if (buf_len > ND_IOCTL_MAX_BUFLEN) {
426824d63b77bd Dan Williams 2018-03-05 1161 dev_dbg(dev, "%s cmd: %s buf_len: %llu > %d\n", dimm_name,
426824d63b77bd Dan Williams 2018-03-05 1162 cmd_name, buf_len, ND_IOCTL_MAX_BUFLEN);
6de5d06e657acd Dan Williams 2019-07-17 1163 rc = -EINVAL;
6de5d06e657acd Dan Williams 2019-07-17 1164 goto out;
62232e45f4a265 Dan Williams 2015-06-08 1165 }
62232e45f4a265 Dan Williams 2015-06-08 1166
62232e45f4a265 Dan Williams 2015-06-08 1167 buf = vmalloc(buf_len);
6de5d06e657acd Dan Williams 2019-07-17 1168 if (!buf) {
6de5d06e657acd Dan Williams 2019-07-17 1169 rc = -ENOMEM;
6de5d06e657acd Dan Williams 2019-07-17 1170 goto out;
6de5d06e657acd Dan Williams 2019-07-17 1171 }
62232e45f4a265 Dan Williams 2015-06-08 1172
62232e45f4a265 Dan Williams 2015-06-08 1173 if (copy_from_user(buf, p, buf_len)) {
62232e45f4a265 Dan Williams 2015-06-08 1174 rc = -EFAULT;
62232e45f4a265 Dan Williams 2015-06-08 @1175 goto out;
62232e45f4a265 Dan Williams 2015-06-08 1176 }
62232e45f4a265 Dan Williams 2015-06-08 1177
eb0e82bc5f69da Dave Jiang 2025-09-22 1178 guard(device)(dev);
eb0e82bc5f69da Dave Jiang 2025-09-22 1179 guard(nvdimm_bus)(dev);
53b85a449b15e0 Jerry Hoemann 2017-06-30 1180 rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
eaf961536e1622 Dan Williams 2015-05-01 1181 if (rc)
eb0e82bc5f69da Dave Jiang 2025-09-22 1182 goto out;
eaf961536e1622 Dan Williams 2015-05-01 1183
006358b35c73ab Dave Jiang 2017-04-07 1184 rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
62232e45f4a265 Dan Williams 2015-06-08 1185 if (rc < 0)
eb0e82bc5f69da Dave Jiang 2025-09-22 1186 goto out;
006358b35c73ab Dave Jiang 2017-04-07 1187
006358b35c73ab Dave Jiang 2017-04-07 1188 if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
006358b35c73ab Dave Jiang 2017-04-07 1189 struct nd_cmd_clear_error *clear_err = buf;
006358b35c73ab Dave Jiang 2017-04-07 1190
23f4984483623c Dan Williams 2017-04-29 1191 nvdimm_account_cleared_poison(nvdimm_bus, clear_err->address,
006358b35c73ab Dave Jiang 2017-04-07 1192 clear_err->cleared);
006358b35c73ab Dave Jiang 2017-04-07 1193 }
0beb2012a17226 Dan Williams 2017-04-07 1194
62232e45f4a265 Dan Williams 2015-06-08 1195 if (copy_to_user(p, buf, buf_len))
62232e45f4a265 Dan Williams 2015-06-08 1196 rc = -EFAULT;
0beb2012a17226 Dan Williams 2017-04-07 1197
62232e45f4a265 Dan Williams 2015-06-08 1198 out:
6de5d06e657acd Dan Williams 2019-07-17 1199 kfree(in_env);
6de5d06e657acd Dan Williams 2019-07-17 1200 kfree(out_env);
62232e45f4a265 Dan Williams 2015-06-08 1201 vfree(buf);
62232e45f4a265 Dan Williams 2015-06-08 1202 return rc;
62232e45f4a265 Dan Williams 2015-06-08 1203 }
62232e45f4a265 Dan Williams 2015-06-08 1204
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-22 21:13 [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock Dave Jiang
2025-09-22 22:50 ` dan.j.williams
2025-09-23 1:58 ` kernel test robot
@ 2025-09-23 10:18 ` Jonathan Cameron
2025-09-23 16:36 ` Dave Jiang
2 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2025-09-23 10:18 UTC (permalink / raw)
To: Dave Jiang; +Cc: nvdimm, ira.weiny, vishal.l.verma, dan.j.williams, s.neeraj
On Mon, 22 Sep 2025 14:13:30 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> Converting nvdimm_bus_lock/unlock to guard() to clean up usage
> of gotos for error handling and avoid future mistakes of missed
> unlock on error paths.
>
> Link: https://lore.kernel.org/linux-cxl/20250917163623.00004a3c@huawei.com/
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hi Dave,
Thanks for looking at this.
Fully agree with Dan about the getting rid of all gotos by end of series.
A few other things inline. Mostly places where the use of guard()
opens up low hanging fruit that improves readability (+ shortens code).
This code has a lot of dev_dbg() and some of them are so generic I'm not
sure they are actually useful (cover a whole set of error paths). Perhaps
it is worth splitting some of those up, or reducing the paths that trigger
them as part of this refactor.
Jonathan
> EXPORT_SYMBOL_GPL(nvdimm_badblocks_populate);
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 497fd434a6a1..b35bcbe5db7f 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
...
> @@ -95,10 +93,9 @@ static ssize_t namespace_show(struct device *dev,
> struct nd_btt *nd_btt = to_nd_btt(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> rc = sprintf(buf, "%s\n", nd_btt->ndns
> ? dev_name(&nd_btt->ndns->dev) : "");
return sprintf();
and drop the local variable.
> - nvdimm_bus_unlock(dev);
> return rc;
> }
>
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 0ccf4a9e523a..d2c2d71e7fe0 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> static int nvdimm_bus_probe(struct device *dev)
> @@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
> goto out;
> }
>
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
> if (rc)
> - goto out_unlock;
> + goto out;
>
> rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
> if (rc < 0)
> - goto out_unlock;
> + goto out;
>
> if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
> struct nd_cmd_clear_error *clear_err = buf;
> @@ -1197,9 +1195,6 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
> if (copy_to_user(p, buf, buf_len))
> rc = -EFAULT;
>
> -out_unlock:
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
> out:
Hmm. I'm not a fan of gotos that rely on initializing a bunch of pointers to NULL
so fewer labels are used. Will be nice to replace that as well via __free
Going to need a DEFINE_FREE for the vfree that follow these but that looks standard to me.
> kfree(in_env);
> kfree(out_env);
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index 51614651d2e7..e53a2cc04695 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -35,9 +35,8 @@ void nd_detach_ndns(struct device *dev,
> if (!ndns)
> return;
> get_device(&ndns->dev);
> - nvdimm_bus_lock(&ndns->dev);
> - __nd_detach_ndns(dev, _ndns);
> - nvdimm_bus_unlock(&ndns->dev);
> + scoped_guard(nvdimm_bus, &ndns->dev)
> + __nd_detach_ndns(dev, _ndns);
> put_device(&ndns->dev);
maybe a guard for this as well? Then you could just use
guards for both rather than needing the scoped guard.
> }
> diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
> index 37b743acbb7b..a5d44b5c9452 100644
> --- a/drivers/nvdimm/dax_devs.c
> +++ b/drivers/nvdimm/dax_devs.c
> @@ -104,10 +104,10 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
> return -ENODEV;
> }
>
> - nvdimm_bus_lock(&ndns->dev);
> - nd_dax = nd_dax_alloc(nd_region);
> - dax_dev = nd_dax_devinit(nd_dax, ndns);
> - nvdimm_bus_unlock(&ndns->dev);
> + scoped_guard(nvdimm_bus, &ndns->dev) {
> + nd_dax = nd_dax_alloc(nd_region);
> + dax_dev = nd_dax_devinit(nd_dax, ndns);
> + }
> if (!dax_dev)
> return -ENOMEM;
Maybe move this check under the lock? For me that would be a little more obvious
as right next to where it is set.
> pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 91d9163ee303..2018458a3dba 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -95,13 +95,13 @@ static int nvdimm_probe(struct device *dev)
>
> dev_dbg(dev, "config data size: %d\n", ndd->nsarea.config_size);
>
> - nvdimm_bus_lock(dev);
> - if (ndd->ns_current >= 0) {
> - rc = nd_label_reserve_dpa(ndd);
> - if (rc == 0)
> - nvdimm_set_labeling(dev);
> + scoped_guard(nvdimm_bus, dev) {
> + if (ndd->ns_current >= 0) {
> + rc = nd_label_reserve_dpa(ndd);
> + if (rc == 0)
> + nvdimm_set_labeling(dev);
> + }
> }
> - nvdimm_bus_unlock(dev);
This one looks awkward wrt to the goto and put_ndd().
Might not be worth the bother. I tend to take the view that it is
fine to use guard() for a lock where it helps but not force it to be
used universally if there are places where it doesn't.
>
> if (rc)
> goto err;
> @@ -117,9 +117,8 @@ static void nvdimm_remove(struct device *dev)
> {
> struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
>
> - nvdimm_bus_lock(dev);
> - dev_set_drvdata(dev, NULL);
> - nvdimm_bus_unlock(dev);
> + scoped_guard(nvdimm_bus, dev)
> + dev_set_drvdata(dev, NULL);
> put_ndd(ndd);
> }
>
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 21498d461fde..4b293c4ad15c 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -326,7 +326,7 @@ static ssize_t __available_slots_show(struct nvdimm_drvdata *ndd, char *buf)
> return -ENXIO;
>
> dev = ndd->dev;
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> nfree = nd_label_nfree(ndd);
> if (nfree - 1 > nfree) {
> dev_WARN_ONCE(dev, 1, "we ate our last label?\n");
> @@ -334,7 +334,6 @@ static ssize_t __available_slots_show(struct nvdimm_drvdata *ndd, char *buf)
> } else
> nfree--;
> rc = sprintf(buf, "%d\n", nfree);
> - nvdimm_bus_unlock(dev);
> return rc;
return sprintf() nad drop then then unused rc.
> }
>
> @@ -395,12 +394,10 @@ static ssize_t security_store(struct device *dev,
> * done while probing is idle and the DIMM is not in active use
> * in any region.
> */
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> wait_nvdimm_bus_probe_idle(dev);
> rc = nvdimm_security_store(dev, buf, len);
return nvdimm_security_store()
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
>
> return rc;
> }
> @@ -454,9 +451,8 @@ static ssize_t result_show(struct device *dev, struct device_attribute *attr, ch
> if (!nvdimm->fw_ops)
> return -EOPNOTSUPP;
>
> - nvdimm_bus_lock(dev);
> - result = nvdimm->fw_ops->activate_result(nvdimm);
> - nvdimm_bus_unlock(dev);
> + scoped_guard(nvdimm_bus, dev)
Maybe just guard? Seems unlikely to matter if we do the prints under the lock.
> + result = nvdimm->fw_ops->activate_result(nvdimm);
>
> switch (result) {
> case NVDIMM_FWA_RESULT_NONE:
> @@ -483,9 +479,8 @@ static ssize_t activate_show(struct device *dev, struct device_attribute *attr,
> if (!nvdimm->fw_ops)
> return -EOPNOTSUPP;
>
> - nvdimm_bus_lock(dev);
> - state = nvdimm->fw_ops->activate_state(nvdimm);
> - nvdimm_bus_unlock(dev);
> + scoped_guard(nvdimm_bus, dev)
Similar to above. I'd just hold it to function exit to simplify the code a little.
> + state = nvdimm->fw_ops->activate_state(nvdimm);
>
> switch (state) {
> case NVDIMM_FWA_IDLE:
..
> @@ -641,11 +634,11 @@ void nvdimm_delete(struct nvdimm *nvdimm)
> bool dev_put = false;
>
> /* We are shutting down. Make state frozen artificially. */
> - nvdimm_bus_lock(dev);
> - set_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags);
> - if (test_and_clear_bit(NDD_WORK_PENDING, &nvdimm->flags))
> - dev_put = true;
> - nvdimm_bus_unlock(dev);
> + scoped_guard(nvdimm_bus, dev) {
> + set_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags);
> + if (test_and_clear_bit(NDD_WORK_PENDING, &nvdimm->flags))
> + dev_put = true;
Not sure why this isn't
dev_put = test_and_clear_bit();
Maybe some earlier refactoring left this somewhat odd bit of code or
> + }
> cancel_delayed_work_sync(&nvdimm->dwork);
> if (dev_put)
> put_device(dev);
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 55cfbf1e0a95..38933abfb2a6 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
...
> @@ -893,9 +888,8 @@ resource_size_t nvdimm_namespace_capacity(struct nd_namespace_common *ndns)
> {
> resource_size_t size;
>
> - nvdimm_bus_lock(&ndns->dev);
> + guard(nvdimm_bus)(&ndns->dev);
> size = __nvdimm_namespace_capacity(ndns);
> - nvdimm_bus_unlock(&ndns->dev);
>
> return size;
return __nvdimm_namespace_capacity(ndns);
> }
> @@ -1119,8 +1111,8 @@ static ssize_t sector_size_store(struct device *dev,
> } else
> return -ENXIO;
>
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> if (to_ndns(dev)->claim)
> rc = -EBUSY;
> if (rc >= 0)
There is a bit mess of if (rc >= 0)
stuff in here that could just become early exits and generally result I think
in more readable code flow (enabled by the guard() usage) The side effect
being the dev_dbg() might need to be replicated but it could ten say what actually
happened rather than current case of one of 3 things failed.
if (to_ndns(dev)->claim) {
dev_dbg(dev...)
return -EBUSY;
}
rc = nd_size_select_store();
if (rc < 0) {
dev_dbg()
return rc;
}
rc = nd_namespace_label_update(nd_region, dev);
if (rc < 0) {
dev_dbg(); // if all these are actually useful given we know what we wrote and what failed.
return rc;
}
> @@ -1145,7 +1135,7 @@ static ssize_t dpa_extents_show(struct device *dev,
> int count = 0, i;
> u32 flags = 0;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (is_namespace_pmem(dev)) {
> struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
>
> @@ -1167,8 +1157,6 @@ static ssize_t dpa_extents_show(struct device *dev,
> count++;
> }
> out:
Can drop this and return early particularly as it's just
return sprintf(buf, "0\n");
in that one path.
> - nvdimm_bus_unlock(dev);
> -
> return sprintf(buf, "%d\n", count);
> }
> static DEVICE_ATTR_RO(dpa_extents);
> @@ -2152,31 +2138,38 @@ static int init_active_labels(struct nd_region *nd_region)
> nd_region);
> }
>
> +static int __create_namespaces(struct nd_region *nd_region, int *type,
naming is a little odd given it calls create_namespaces internally.
Maybe
create_relevant_namespace() or something along those lines.
Or rename the create_namespaces() to be PMEM specific and reuse
that name for the wrapper.
> + struct device ***devs)
> +{
> + int rc;
> +
> + guard(nvdimm_bus)(&nd_region->dev);
> + rc = init_active_labels(nd_region);
> + if (rc)
> + return rc;
> +
> + *type = nd_region_to_nstype(nd_region);
> + switch (*type) {
> + case ND_DEVICE_NAMESPACE_IO:
> + *devs = create_namespace_io(nd_region);
> + break;
> + case ND_DEVICE_NAMESPACE_PMEM:
> + *devs = create_namespaces(nd_region);
> + break;
> + }
> +
> + return 0;
> +}
> +
> int nd_region_register_namespaces(struct nd_region *nd_region, int *err)
> {
> struct device **devs = NULL;
> int i, rc = 0, type;
>
> *err = 0;
> - nvdimm_bus_lock(&nd_region->dev);
> - rc = init_active_labels(nd_region);
> - if (rc) {
> - nvdimm_bus_unlock(&nd_region->dev);
> + rc = __create_namespaces(nd_region, &type, &devs);
> + if (rc)
> return rc;
> - }
> -
> - type = nd_region_to_nstype(nd_region);
> - switch (type) {
> - case ND_DEVICE_NAMESPACE_IO:
> - devs = create_namespace_io(nd_region);
> - break;
> - case ND_DEVICE_NAMESPACE_PMEM:
> - devs = create_namespaces(nd_region);
> - break;
> - default:
> - break;
> - }
> - nvdimm_bus_unlock(&nd_region->dev);
>
> if (!devs)
> return -ENODEV;
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index cc5c8f3f81e8..a8013033509c 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -632,6 +632,8 @@ u64 nd_region_interleave_set_cookie(struct nd_region *nd_region,
> u64 nd_region_interleave_set_altcookie(struct nd_region *nd_region);
> void nvdimm_bus_lock(struct device *dev);
> void nvdimm_bus_unlock(struct device *dev);
> +DEFINE_GUARD(nvdimm_bus, struct device *, nvdimm_bus_lock(_T), nvdimm_bus_unlock(_T));
You want that if (_T) or the IS_ERR_OR_NULL variant if appropriate.
Technically not needed, but as Peter Z pointed out in a similar case, sometimes the compiler
might spot that _T is always NULL in a path and optimize out the call. So he was
very keen to keep that guard in the definition unless the the cleanup was also visible
(As an inline in the header for instance).
> +
> bool is_nvdimm_bus_locked(struct device *dev);
> void nvdimm_check_and_set_ro(struct gendisk *disk);
> void nvdimm_drvdata_release(struct kref *kref);
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 8f3e816e805d..f2a44d1f62be 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -57,8 +57,8 @@ static ssize_t mode_store(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc = 0;
>
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> if (dev->driver)
> rc = -EBUSY;
Maybe just do an early return here. Is it really that useful to
see anything other than -EBUSY? If so maybe have a new dev_dbg()
for this path. Nice to reduce the indent on the rest.
> else {
> @@ -78,8 +78,6 @@ static ssize_t mode_store(struct device *dev,
> }
> dev_dbg(dev, "result: %zd wrote: %s%s", rc, buf,
> buf[len - 1] == '\n' ? "" : "\n");
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
>
> return rc ? rc : len;
> }
> @@ -170,10 +166,9 @@ static ssize_t namespace_show(struct device *dev,
> struct nd_pfn *nd_pfn = to_nd_pfn_safe(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> rc = sprintf(buf, "%s\n", nd_pfn->ndns
> ? dev_name(&nd_pfn->ndns->dev) : "");
> - nvdimm_bus_unlock(dev);
> return rc;
return sprintf()
> }
...
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index de1ee5ebc851..b2b4519e9f4c 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> +int nd_region_activate(struct nd_region *nd_region)
> +{
> + int i, j, rc, num_flush;
> + struct nd_region_data *ndrd;
> + struct device *dev = &nd_region->dev;
> + size_t flush_data_size;
> +
> + rc = get_flush_data(nd_region, &flush_data_size, &num_flush);
> + if (rc)
> + return rc;
>
> rc = nd_region_invalidate_memregion(nd_region);
> if (rc)
> @@ -327,8 +340,8 @@ static ssize_t set_cookie_show(struct device *dev,
> * the v1.1 namespace label cookie definition. To read all this
> * data we need to wait for probing to settle.
> */
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> wait_nvdimm_bus_probe_idle(dev);
> if (nd_region->ndr_mappings) {
> struct nd_mapping *nd_mapping = &nd_region->mapping[0];
> @@ -343,8 +356,6 @@ static ssize_t set_cookie_show(struct device *dev,
> nsindex));
> }
> }
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
>
> if (rc)
> return rc;
> @@ -401,12 +412,10 @@ static ssize_t available_size_show(struct device *dev,
> * memory nvdimm_bus_lock() is dropped, but that's userspace's
> * problem to not race itself.
> */
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> wait_nvdimm_bus_probe_idle(dev);
> available = nd_region_available_dpa(nd_region);
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
>
> return sprintf(buf, "%llu\n", available);
Could role this together without I think any real loss of readability.
return sprintf(buf, "%llu\n", nd_region_available_dpa(nd_region));
> }
> @@ -418,12 +427,10 @@ static ssize_t max_available_extent_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> unsigned long long available = 0;
>
> - device_lock(dev);
> - nvdimm_bus_lock(dev);
> + guard(device)(dev);
> + guard(nvdimm_bus)(dev);
> wait_nvdimm_bus_probe_idle(dev);
> available = nd_region_allocatable_dpa(nd_region);
> - nvdimm_bus_unlock(dev);
> - device_unlock(dev);
>
> return sprintf(buf, "%llu\n", available);
Similarly.
> }
> @@ -435,12 +442,11 @@ static ssize_t init_namespaces_show(struct device *dev,
> struct nd_region_data *ndrd = dev_get_drvdata(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (ndrd)
> rc = sprintf(buf, "%d/%d\n", ndrd->ns_active, ndrd->ns_count);
> else
> rc = -ENXIO;
> - nvdimm_bus_unlock(dev);
>
> return rc;
I'd do
if (!ndrd)
return -ENXIO;
return sprintf();
> }
> @@ -452,12 +458,11 @@ static ssize_t namespace_seed_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (nd_region->ns_seed)
> rc = sprintf(buf, "%s\n", dev_name(nd_region->ns_seed));
> else
> rc = sprintf(buf, "\n");
> - nvdimm_bus_unlock(dev);
> return rc;
likewise, I'd just return directly in each of the legs
> }
> static DEVICE_ATTR_RO(namespace_seed);
> @@ -468,12 +473,11 @@ static ssize_t btt_seed_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (nd_region->btt_seed)
> rc = sprintf(buf, "%s\n", dev_name(nd_region->btt_seed));
> else
> rc = sprintf(buf, "\n");
> - nvdimm_bus_unlock(dev);
>
> return rc;
Here as well.
> }
> @@ -485,12 +489,11 @@ static ssize_t pfn_seed_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (nd_region->pfn_seed)
> rc = sprintf(buf, "%s\n", dev_name(nd_region->pfn_seed));
> else
> rc = sprintf(buf, "\n");
> - nvdimm_bus_unlock(dev);
>
> return rc;
A common theme is emerging ;)
> }
> @@ -502,12 +505,11 @@ static ssize_t dax_seed_show(struct device *dev,
> struct nd_region *nd_region = to_nd_region(dev);
> ssize_t rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> if (nd_region->dax_seed)
> rc = sprintf(buf, "%s\n", dev_name(nd_region->dax_seed));
> else
> rc = sprintf(buf, "\n");
> - nvdimm_bus_unlock(dev);
And again.
>
> return rc;
> }
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index a03e3c45f297..e70dc3b08458 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -221,9 +221,8 @@ int nvdimm_security_unlock(struct device *dev)
> struct nvdimm *nvdimm = to_nvdimm(dev);
> int rc;
>
> - nvdimm_bus_lock(dev);
> + guard(nvdimm_bus)(dev);
> rc = __nvdimm_security_unlock(nvdimm);
> - nvdimm_bus_unlock(dev);
> return rc;
return __nvdimm_se...
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock
2025-09-23 10:18 ` Jonathan Cameron
@ 2025-09-23 16:36 ` Dave Jiang
0 siblings, 0 replies; 7+ messages in thread
From: Dave Jiang @ 2025-09-23 16:36 UTC (permalink / raw)
To: Jonathan Cameron
Cc: nvdimm, ira.weiny, vishal.l.verma, dan.j.williams, s.neeraj
On 9/23/25 3:18 AM, Jonathan Cameron wrote:
> On Mon, 22 Sep 2025 14:13:30 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Converting nvdimm_bus_lock/unlock to guard() to clean up usage
>> of gotos for error handling and avoid future mistakes of missed
>> unlock on error paths.
>>
>> Link: https://lore.kernel.org/linux-cxl/20250917163623.00004a3c@huawei.com/
>> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave,
>
> Thanks for looking at this.
>
> Fully agree with Dan about the getting rid of all gotos by end of series.
I'll do that in a different patch for that particular chunk.
>
> A few other things inline. Mostly places where the use of guard()
> opens up low hanging fruit that improves readability (+ shortens code).
>
> This code has a lot of dev_dbg() and some of them are so generic I'm not
> sure they are actually useful (cover a whole set of error paths). Perhaps
> it is worth splitting some of those up, or reducing the paths that trigger
> them as part of this refactor.
> > Jonathan
>
>
<snip>
>
>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> index 0ccf4a9e523a..d2c2d71e7fe0 100644
>> --- a/drivers/nvdimm/bus.c
>> +++ b/drivers/nvdimm/bus.c
>> static int nvdimm_bus_probe(struct device *dev)
>> @@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>> goto out;
>> }
>>
>> - device_lock(dev);
>> - nvdimm_bus_lock(dev);
>> + guard(device)(dev);
>> + guard(nvdimm_bus)(dev);
>> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
>> if (rc)
>> - goto out_unlock;
>> + goto out;
>>
>> rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
>> if (rc < 0)
>> - goto out_unlock;
>> + goto out;
>>
>> if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
>> struct nd_cmd_clear_error *clear_err = buf;
>> @@ -1197,9 +1195,6 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>> if (copy_to_user(p, buf, buf_len))
>> rc = -EFAULT;
>>
>> -out_unlock:
>> - nvdimm_bus_unlock(dev);
>> - device_unlock(dev);
>> out:
> Hmm. I'm not a fan of gotos that rely on initializing a bunch of pointers to NULL
> so fewer labels are used. Will be nice to replace that as well via __free
>
> Going to need a DEFINE_FREE for the vfree that follow these but that looks standard to me.
I ended up moving it to kvzalloc() instead. Seems reasonable and give us zeroing of the buffer as well.
DJ
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-23 16:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-22 21:13 [PATCH] nvdimm: Introduce guard() for nvdimm_bus_lock Dave Jiang
2025-09-22 22:50 ` dan.j.williams
2025-09-22 23:00 ` Dave Jiang
2025-09-22 23:23 ` dan.j.williams
2025-09-23 1:58 ` kernel test robot
2025-09-23 10:18 ` Jonathan Cameron
2025-09-23 16:36 ` Dave Jiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.