2020-08-10 20:13:43

by Michał Mirosław

Subject: [RFC PATCH 3/3] regulator: push supply_name allocation outside of lock

Move all allocations outside of the regulator_lock().

======================================================
WARNING: possible circular locking dependency detected
5.7.13+ #535 Not tainted
------------------------------------------------------
f2fs_discard-179:7/702 is trying to acquire lock:
c0e5d920 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x54/0x2c0

but task is already holding lock:
cb95b080 (&dcc->cmd_lock){+.+.}-{3:3}, at: __issue_discard_cmd+0xec/0x5f8

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

[...]

-> #3 (fs_reclaim){+.+.}-{0:0}:
fs_reclaim_acquire.part.11+0x40/0x50
fs_reclaim_acquire+0x24/0x28
__kmalloc_track_caller+0x54/0x218
kstrdup+0x40/0x5c
create_regulator+0xf4/0x368
regulator_resolve_supply+0x1a0/0x200
regulator_register+0x9c8/0x163c

[...]

other info that might help us debug this:

Chain exists of:
regulator_list_mutex --> &sit_i->sentry_lock --> &dcc->cmd_lock

[...]

Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
Signed-off-by: Michał Mirosław <[email protected]>
---
drivers/regulator/core.c | 46 +++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 05c9657c99d9..5f518e034c90 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1557,41 +1557,48 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
const char *supply_name)
{
struct regulator *regulator;
- char buf[REG_STR_SIZE];
- int err, size;
+ int err;
+
+ if (dev) {
+ char buf[REG_STR_SIZE];
+ int size;
+
+ size = snprintf(buf, REG_STR_SIZE, "%s-%s",
+ dev->kobj.name, supply_name);
+ if (size >= REG_STR_SIZE)
+ return NULL;
+
+ supply_name = kstrdup(buf, GFP_KERNEL);
+ if (supply_name == NULL)
+ return NULL;
+ } else {
+ supply_name = kstrdup_const(supply_name, GFP_KERNEL);
+ if (supply_name == NULL)
+ return NULL;
+ }

regulator = kzalloc(sizeof(*regulator), GFP_KERNEL);
- if (regulator == NULL)
+ if (regulator == NULL) {
+ kfree(supply_name);
return NULL;
+ }

regulator_lock(rdev);
regulator->rdev = rdev;
+ regulator->supply_name = supply_name;
list_add(&regulator->list, &rdev->consumer_list);

if (dev) {
regulator->dev = dev;

/* Add a link to the device sysfs entry */
- size = snprintf(buf, REG_STR_SIZE, "%s-%s",
- dev->kobj.name, supply_name);
- if (size >= REG_STR_SIZE)
- goto overflow_err;
-
- regulator->supply_name = kstrdup(buf, GFP_KERNEL);
- if (regulator->supply_name == NULL)
- goto overflow_err;
-
err = sysfs_create_link_nowarn(&rdev->dev.kobj, &dev->kobj,
- buf);
+ supply_name);
if (err) {
rdev_dbg(rdev, "could not add device link %s err %d\n",
dev->kobj.name, err);
/* non-fatal */
}
- } else {
- regulator->supply_name = kstrdup_const(supply_name, GFP_KERNEL);
- if (regulator->supply_name == NULL)
- goto overflow_err;
}

regulator->debugfs = debugfs_create_dir(regulator->supply_name,
@@ -1621,11 +1628,6 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,

regulator_unlock(rdev);
return regulator;
-overflow_err:
- list_del(&regulator->list);
- kfree(regulator);
- regulator_unlock(rdev);
- return NULL;
}

static int _regulator_get_enable_time(struct regulator_dev *rdev)
--
2.20.1