* [PATCH v2 1/7] regulator: push allocation in regulator_init_coupling() outside of lock
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 2/7] regulator: push allocation in regulator_ena_gpio_request() out " Michał Mirosław
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Dmitry Osipenko, Vladimir Zapolskiy
Cc: linux-kernel
Allocating memory with regulator_list_mutex held makes lockdep unhappy
when memory pressure makes the system do fs_reclaim on eg. eMMC using
a regulator. Push the lock inside regulator_init_coupling() after the
allocation.
======================================================
WARNING: possible circular locking dependency detected
5.7.13+ #533 Not tainted
------------------------------------------------------
kswapd0/383 is trying to acquire lock:
cca78ca4 (&sbi->write_io[i][j].io_rwsem){++++}-{3:3}, at: __submit_merged_write_cond+0x104/0x154
but task is already holding lock:
c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (fs_reclaim){+.+.}-{0:0}:
fs_reclaim_acquire.part.11+0x40/0x50
fs_reclaim_acquire+0x24/0x28
__kmalloc+0x54/0x218
regulator_register+0x860/0x1584
dummy_regulator_probe+0x60/0xa8
[...]
other info that might help us debug this:
Chain exists of:
&sbi->write_io[i][j].io_rwsem --> regulator_list_mutex --> fs_reclaim
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
lock(regulator_list_mutex);
lock(fs_reclaim);
lock(&sbi->write_io[i][j].io_rwsem);
*** DEADLOCK ***
1 lock held by kswapd0/383:
#0: c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
[...]
Cc: stable@vger.kernel.org
Fixes: d8ca7d184b33 ("regulator: core: Introduce API for regulators coupling customization")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/regulator/core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0a32c3da0e26..510d234f6c46 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5010,7 +5010,10 @@ static int regulator_init_coupling(struct regulator_dev *rdev)
if (!of_check_coupling_data(rdev))
return -EPERM;
+ mutex_lock(®ulator_list_mutex);
rdev->coupling_desc.coupler = regulator_find_coupler(rdev);
+ mutex_unlock(®ulator_list_mutex);
+
if (IS_ERR(rdev->coupling_desc.coupler)) {
err = PTR_ERR(rdev->coupling_desc.coupler);
rdev_err(rdev, "failed to get coupler: %d\n", err);
@@ -5218,9 +5221,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
if (ret < 0)
goto wash;
- mutex_lock(®ulator_list_mutex);
ret = regulator_init_coupling(rdev);
- mutex_unlock(®ulator_list_mutex);
if (ret < 0)
goto wash;
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/7] regulator: push allocation in regulator_ena_gpio_request() out of lock
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 1/7] regulator: push allocation in regulator_init_coupling() outside of lock Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-18 0:25 ` Dmitry Osipenko
2020-08-12 1:31 ` [PATCH v2 3/7] regulator: push allocations in create_regulator() outside " Michał Mirosław
` (4 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Dmitry Osipenko, Liam Girdwood, Mark Brown, Vladimir Zapolskiy
Cc: linux-kernel
Move another allocation out of regulator_list_mutex-protected region, as
reclaim might want to take the same lock.
WARNING: possible circular locking dependency detected
5.7.13+ #534 Not tainted
------------------------------------------------------
kswapd0/383 is trying to acquire lock:
c0e5d920 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x54/0x2c0
but task is already holding lock:
c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (fs_reclaim){+.+.}-{0:0}:
fs_reclaim_acquire.part.11+0x40/0x50
fs_reclaim_acquire+0x24/0x28
kmem_cache_alloc_trace+0x40/0x1e8
regulator_register+0x384/0x1630
devm_regulator_register+0x50/0x84
reg_fixed_voltage_probe+0x248/0x35c
[...]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
lock(regulator_list_mutex);
lock(fs_reclaim);
lock(regulator_list_mutex);
*** DEADLOCK ***
[...]
2 locks held by kswapd0/383:
#0: c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
#1: cb70e5e0 (hctx->srcu){....}-{0:0}, at: hctx_lock+0x60/0xb8
[...]
Fixes: 541d052d7215 ("regulator: core: Only support passing enable GPIO descriptors")
[this commit only changes context]
Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
[this is when the regulator_list_mutex was introduced in reclaim locking path]
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/regulator/core.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 510d234f6c46..3dd4d4914075 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2203,10 +2203,13 @@ EXPORT_SYMBOL_GPL(regulator_bulk_unregister_supply_alias);
static int regulator_ena_gpio_request(struct regulator_dev *rdev,
const struct regulator_config *config)
{
- struct regulator_enable_gpio *pin;
+ struct regulator_enable_gpio *pin, *new_pin;
struct gpio_desc *gpiod;
gpiod = config->ena_gpiod;
+ new_pin = kzalloc(sizeof(*new_pin), GFP_KERNEL);
+
+ mutex_lock(®ulator_list_mutex);
list_for_each_entry(pin, ®ulator_ena_gpio_list, list) {
if (pin->gpiod == gpiod) {
@@ -2215,9 +2218,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
}
}
- pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
- if (pin == NULL)
+ if (new_pin == NULL) {
+ mutex_unlock(®ulator_list_mutex);
return -ENOMEM;
+ }
+
+ pin = new_pin;
+ new_pin = NULL;
pin->gpiod = gpiod;
list_add(&pin->list, ®ulator_ena_gpio_list);
@@ -2225,6 +2232,10 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
update_ena_gpio_to_rdev:
pin->request_count++;
rdev->ena_pin = pin;
+
+ mutex_unlock(®ulator_list_mutex);
+ kfree(new_pin);
+
return 0;
}
@@ -5179,9 +5190,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
}
if (config->ena_gpiod) {
- mutex_lock(®ulator_list_mutex);
ret = regulator_ena_gpio_request(rdev, config);
- mutex_unlock(®ulator_list_mutex);
if (ret != 0) {
rdev_err(rdev, "Failed to request enable GPIO: %d\n",
ret);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/7] regulator: push allocations in create_regulator() outside of lock
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 1/7] regulator: push allocation in regulator_init_coupling() outside of lock Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 2/7] regulator: push allocation in regulator_ena_gpio_request() out " Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path Michał Mirosław
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Dmitry Osipenko, Liam Girdwood, Mark Brown, Vladimir Zapolskiy
Cc: linux-kernel
Move all allocations outside of the regulator_lock()ed section.
======================================================
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
[...]
Cc: stable@vger.kernel.org
Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/regulator/core.c | 53 +++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3dd4d4914075..c95397798275 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1553,44 +1553,53 @@ 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;
+
+ regulator_lock(rdev);
list_add(®ulator->list, &rdev->consumer_list);
+ regulator_unlock(rdev);
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,
+ regulator->debugfs = debugfs_create_dir(supply_name,
rdev->debugfs);
if (!regulator->debugfs) {
rdev_dbg(rdev, "Failed to create debugfs directory\n");
@@ -1615,13 +1624,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
_regulator_is_enabled(rdev))
regulator->always_on = true;
- regulator_unlock(rdev);
return regulator;
-overflow_err:
- list_del(®ulator->list);
- kfree(regulator);
- regulator_unlock(rdev);
- return NULL;
}
static int _regulator_get_enable_time(struct regulator_dev *rdev)
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
` (2 preceding siblings ...)
2020-08-12 1:31 ` [PATCH v2 3/7] regulator: push allocations in create_regulator() outside " Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-12 6:29 ` Vladimir Zapolskiy
2020-08-12 1:31 ` [PATCH v2 4/7] regulator: push allocation in set_consumer_device_supply() out of lock Michał Mirosław
` (2 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Vladimir Zapolskiy, Dmitry Osipenko, Liam Girdwood, Mark Brown
Cc: linux-kernel
By calling device_initialize() earlier and noting that kfree(NULL) is
ok, we can save a bit of code in error handling and plug of_node leak.
Fixed commit already did part of the work.
Cc: stable@vger.kernel.org
Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Acked-by: Vladimir Zapolskiy <vz@mleia.com>
---
drivers/regulator/core.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 71749f48caee..448a267641df 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5137,6 +5137,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
ret = -ENOMEM;
goto rinse;
}
+ device_initialize(&rdev->dev);
/*
* Duplicate the config so the driver could override it after
@@ -5144,9 +5145,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
*/
config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
if (config == NULL) {
- kfree(rdev);
ret = -ENOMEM;
- goto rinse;
+ goto clean;
}
init_data = regulator_of_get_init_data(dev, regulator_desc, config,
@@ -5158,10 +5158,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
* from a gpio extender or something else.
*/
if (PTR_ERR(init_data) == -EPROBE_DEFER) {
- kfree(config);
- kfree(rdev);
ret = -EPROBE_DEFER;
- goto rinse;
+ goto clean;
}
/*
@@ -5214,7 +5212,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
}
/* register with sysfs */
- device_initialize(&rdev->dev);
rdev->dev.class = ®ulator_class;
rdev->dev.parent = dev;
dev_set_name(&rdev->dev, "regulator.%lu",
@@ -5292,13 +5289,11 @@ regulator_register(const struct regulator_desc *regulator_desc,
mutex_lock(®ulator_list_mutex);
regulator_ena_gpio_free(rdev);
mutex_unlock(®ulator_list_mutex);
- put_device(&rdev->dev);
- rdev = NULL;
clean:
if (dangling_of_gpiod)
gpiod_put(config->ena_gpiod);
- kfree(rdev);
kfree(config);
+ put_device(&rdev->dev);
rinse:
if (dangling_cfg_gpiod)
gpiod_put(cfg->ena_gpiod);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path
2020-08-12 1:31 ` [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path Michał Mirosław
@ 2020-08-12 6:29 ` Vladimir Zapolskiy
2020-08-12 14:09 ` Michał Mirosław
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Zapolskiy @ 2020-08-12 6:29 UTC (permalink / raw)
To: Michał Mirosław, Dmitry Osipenko, Liam Girdwood, Mark Brown
Cc: linux-kernel
Hi Michał,
On 8/12/20 4:31 AM, Michał Mirosław wrote:
> By calling device_initialize() earlier and noting that kfree(NULL) is
> ok, we can save a bit of code in error handling and plug of_node leak.
> Fixed commit already did part of the work.
>
> Cc: stable@vger.kernel.org
> Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Acked-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
> drivers/regulator/core.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 71749f48caee..448a267641df 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5137,6 +5137,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
> ret = -ENOMEM;
> goto rinse;
> }
> + device_initialize(&rdev->dev);
>
> /*
> * Duplicate the config so the driver could override it after
> @@ -5144,9 +5145,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
> */
> config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
> if (config == NULL) {
> - kfree(rdev);
> ret = -ENOMEM;
> - goto rinse;
> + goto clean;
Here config == NULL
> }
>
> init_data = regulator_of_get_init_data(dev, regulator_desc, config,
> @@ -5158,10 +5158,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
> * from a gpio extender or something else.
> */
> if (PTR_ERR(init_data) == -EPROBE_DEFER) {
> - kfree(config);
> - kfree(rdev);
> ret = -EPROBE_DEFER;
> - goto rinse;
> + goto clean;
> }
>
> /*
> @@ -5214,7 +5212,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
> }
>
> /* register with sysfs */
> - device_initialize(&rdev->dev);
> rdev->dev.class = ®ulator_class;
> rdev->dev.parent = dev;
> dev_set_name(&rdev->dev, "regulator.%lu",
> @@ -5292,13 +5289,11 @@ regulator_register(const struct regulator_desc *regulator_desc,
> mutex_lock(®ulator_list_mutex);
> regulator_ena_gpio_free(rdev);
> mutex_unlock(®ulator_list_mutex);
> - put_device(&rdev->dev);
> - rdev = NULL;
> clean:
> if (dangling_of_gpiod)
> gpiod_put(config->ena_gpiod);
And above 'config' NULL pointer could be dereferenced now, right?
> - kfree(rdev);
> kfree(config);
> + put_device(&rdev->dev);
> rinse:
> if (dangling_cfg_gpiod)
> gpiod_put(cfg->ena_gpiod);
>
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path
2020-08-12 6:29 ` Vladimir Zapolskiy
@ 2020-08-12 14:09 ` Michał Mirosław
2020-08-13 8:29 ` Vladimir Zapolskiy
0 siblings, 1 reply; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 14:09 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Dmitry Osipenko, Liam Girdwood, Mark Brown, linux-kernel
On Wed, Aug 12, 2020 at 09:29:12AM +0300, Vladimir Zapolskiy wrote:
> On 8/12/20 4:31 AM, Michał Mirosław wrote:
[...]
> > config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
> > if (config == NULL) {
> > - kfree(rdev);
> > ret = -ENOMEM;
> > - goto rinse;
> > + goto clean;
[...]
> > clean:
> > if (dangling_of_gpiod)
> > gpiod_put(config->ena_gpiod);
>
> And above 'config' NULL pointer could be dereferenced now, right?
If config is NULL, dangling_of_gpiod cannot be true.
Best Regards,
Michał Mirosław
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path
2020-08-12 14:09 ` Michał Mirosław
@ 2020-08-13 8:29 ` Vladimir Zapolskiy
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Zapolskiy @ 2020-08-13 8:29 UTC (permalink / raw)
To: Michał Mirosław, Liam Girdwood, Mark Brown
Cc: Dmitry Osipenko, linux-kernel
On 8/12/20 5:09 PM, Michał Mirosław wrote:
> On Wed, Aug 12, 2020 at 09:29:12AM +0300, Vladimir Zapolskiy wrote:
>> On 8/12/20 4:31 AM, Michał Mirosław wrote:
> [...]
>>> config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
>>> if (config == NULL) {
>>> - kfree(rdev);
>>> ret = -ENOMEM;
>>> - goto rinse;
>>> + goto clean;
> [...]
>>> clean:
>>> if (dangling_of_gpiod)
>>> gpiod_put(config->ena_gpiod);
>>
>> And above 'config' NULL pointer could be dereferenced now, right?
>
> If config is NULL, dangling_of_gpiod cannot be true.
>
It's true, thanks. Probably to avoid the known if(false) it might be better
to add a new goto label.
Also it seems to me that it's safe to enter regulator_dev_release() before
doing an assignment to rdev->dev.of_node and incrementing an of_node counter.
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/7] regulator: push allocation in set_consumer_device_supply() out of lock
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
` (3 preceding siblings ...)
2020-08-12 1:31 ` [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 6/7] regulator: cleanup regulator_ena_gpio_free() Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 7/7] regulator: remove superfluous lock in regulator_resolve_coupling() Michał Mirosław
6 siblings, 0 replies; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Dmitry Osipenko, Liam Girdwood, Mark Brown, Vladimir Zapolskiy
Cc: linux-kernel
Pull regulator_list_mutex into set_consumer_device_supply() and keep
allocations outside of it. Fourth of the fs_reclaim deadlock case.
Cc: stable@vger.kernel.org
Fixes: 45389c47526d ("regulator: core: Add early supply resolution for regulators")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: fix new node leak reported kernel test robot <lkp@intel.com>
---
drivers/regulator/core.c | 46 +++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c95397798275..71749f48caee 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1434,7 +1434,7 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
const char *consumer_dev_name,
const char *supply)
{
- struct regulator_map *node;
+ struct regulator_map *node, *new_node;
int has_dev;
if (supply == NULL)
@@ -1445,6 +1445,22 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
else
has_dev = 0;
+ new_node = kzalloc(sizeof(struct regulator_map), GFP_KERNEL);
+ if (new_node == NULL)
+ return -ENOMEM;
+
+ new_node->regulator = rdev;
+ new_node->supply = supply;
+
+ if (has_dev) {
+ new_node->dev_name = kstrdup(consumer_dev_name, GFP_KERNEL);
+ if (new_node->dev_name == NULL) {
+ kfree(new_node);
+ return -ENOMEM;
+ }
+ }
+
+ mutex_lock(®ulator_list_mutex);
list_for_each_entry(node, ®ulator_map_list, list) {
if (node->dev_name && consumer_dev_name) {
if (strcmp(node->dev_name, consumer_dev_name) != 0)
@@ -1462,26 +1478,19 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
node->regulator->desc->name,
supply,
dev_name(&rdev->dev), rdev_get_name(rdev));
- return -EBUSY;
+ goto fail;
}
- node = kzalloc(sizeof(struct regulator_map), GFP_KERNEL);
- if (node == NULL)
- return -ENOMEM;
+ list_add(&new_node->list, ®ulator_map_list);
+ mutex_unlock(®ulator_list_mutex);
- node->regulator = rdev;
- node->supply = supply;
-
- if (has_dev) {
- node->dev_name = kstrdup(consumer_dev_name, GFP_KERNEL);
- if (node->dev_name == NULL) {
- kfree(node);
- return -ENOMEM;
- }
- }
-
- list_add(&node->list, ®ulator_map_list);
return 0;
+
+fail:
+ mutex_unlock(®ulator_list_mutex);
+ kfree(new_node->dev_name);
+ kfree(new_node);
+ return -EBUSY;
}
static void unset_regulator_supplies(struct regulator_dev *rdev)
@@ -5239,19 +5248,16 @@ regulator_register(const struct regulator_desc *regulator_desc,
/* add consumers devices */
if (init_data) {
- mutex_lock(®ulator_list_mutex);
for (i = 0; i < init_data->num_consumer_supplies; i++) {
ret = set_consumer_device_supply(rdev,
init_data->consumer_supplies[i].dev_name,
init_data->consumer_supplies[i].supply);
if (ret < 0) {
- mutex_unlock(®ulator_list_mutex);
dev_err(dev, "Failed to set supply %s\n",
init_data->consumer_supplies[i].supply);
goto unset_supplies;
}
}
- mutex_unlock(®ulator_list_mutex);
}
if (!rdev->desc->ops->get_voltage &&
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 6/7] regulator: cleanup regulator_ena_gpio_free()
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
` (4 preceding siblings ...)
2020-08-12 1:31 ` [PATCH v2 4/7] regulator: push allocation in set_consumer_device_supply() out of lock Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
2020-08-12 1:31 ` [PATCH v2 7/7] regulator: remove superfluous lock in regulator_resolve_coupling() Michał Mirosław
6 siblings, 0 replies; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Dmitry Osipenko, Liam Girdwood, Mark Brown, Vladimir Zapolskiy
Cc: linux-kernel
Since only regulator_ena_gpio_request() allocates rdev->ena_pin, and it
guarantees that same gpiod gets same pin structure, it is enough to
compare just the pointers. Also we know there can be only one matching
entry on the list. Rework the code take advantage of the facts.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/regulator/core.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 448a267641df..bfd4114d6654 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2260,19 +2260,19 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
/* Free the GPIO only in case of no use */
list_for_each_entry_safe(pin, n, ®ulator_ena_gpio_list, list) {
- if (pin->gpiod == rdev->ena_pin->gpiod) {
- if (pin->request_count <= 1) {
- pin->request_count = 0;
- gpiod_put(pin->gpiod);
- list_del(&pin->list);
- kfree(pin);
- rdev->ena_pin = NULL;
- return;
- } else {
- pin->request_count--;
- }
- }
+ if (pin != rdev->ena_pin)
+ continue;
+
+ if (--pin->request_count)
+ break;
+
+ gpiod_put(pin->gpiod);
+ list_del(&pin->list);
+ kfree(pin);
+ break;
}
+
+ rdev->ena_pin = NULL;
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 7/7] regulator: remove superfluous lock in regulator_resolve_coupling()
2020-08-12 1:31 [PATCH v2 0/7] regulator: fix deadlock vs memory reclaim Michał Mirosław
` (5 preceding siblings ...)
2020-08-12 1:31 ` [PATCH v2 6/7] regulator: cleanup regulator_ena_gpio_free() Michał Mirosław
@ 2020-08-12 1:31 ` Michał Mirosław
6 siblings, 0 replies; 12+ messages in thread
From: Michał Mirosław @ 2020-08-12 1:31 UTC (permalink / raw)
To: Dmitry Osipenko, Liam Girdwood, Mark Brown, Vladimir Zapolskiy
Cc: linux-kernel
The code modifies rdev, but locks c_rdev instead. Remove the lock
as this is held together by regulator_list_mutex taken in the caller.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Fixes: f9503385b187 ("regulator: core: Mutually resolve regulators coupling")
---
v2: reword commitmsg
---
drivers/regulator/core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bfd4114d6654..9ca89fee0d7e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4942,13 +4942,9 @@ static void regulator_resolve_coupling(struct regulator_dev *rdev)
return;
}
- regulator_lock(c_rdev);
-
c_desc->coupled_rdevs[i] = c_rdev;
c_desc->n_resolved++;
- regulator_unlock(c_rdev);
-
regulator_resolve_coupling(c_rdev);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread