From: Dmitry Osipenko <digetx@gmail.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>,
"Mark Brown" <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>, linux-kernel@vger.kernel.org
Subject: Re: regulator: deadlock vs memory reclaim
Date: Mon, 10 Aug 2020 22:41:54 +0300 [thread overview]
Message-ID: <08f030a2-3a6f-3ab4-1855-3016884db79d@gmail.com> (raw)
In-Reply-To: <20200810192547.GB26750@qmqm.qmqm.pl>
10.08.2020 22:25, Michał Mirosław пишет:
>>>>> regulator_lock_dependent() starts by taking regulator_list_mutex, The
>>>>> same mutex covers eg. regulator initialization, including memory allocations
>>>>> that happen there. This will deadlock when you have filesystem on eg. eMMC
>>>>> (which uses a regulator to control module voltages) and you register
>>>>> a new regulator (hotplug a device?) when under memory pressure.
>>>> OK, that's very much a corner case, it only applies in the case of
>>>> coupled regulators. The most obvious thing here would be to move the
>>>> allocations on registration out of the locked region, we really only
>>>> need this in the regulator_find_coupler() call I think. If the
>>>> regulator isn't coupled we don't need to take the lock at all.
>>> Currently, regulator_lock_dependent() is called by eg. regulator_enable() and
>>> regulator_get_voltage(), so actually any regulator can deadlock this way.
>> The initialization cases that are the trigger are only done for coupled
>> regulators though AFAICT, otherwise we're not doing allocations with the
>> lock held and should be able to progress.
>
> I caught a few lockdep complaints that suggest otherwise, but I'm still
> looking into that.
The problem looks obvious to me. The regulator_init_coupling() is
protected with the list_mutex, the regulator_lock_dependent() also
protected with the list_mutex. Hence if offending reclaim happens from
init_coupling(), then there is a lockup.
1. mutex_lock(®ulator_list_mutex);
2. regulator_init_coupling()
3. kzalloc()
4. reclaim ...
5. regulator_get_voltage()
6. regulator_lock_dependent()
7. mutex_lock(®ulator_list_mutex);
It should be enough just to keep the regulator_find_coupler() under
lock, or even completely remove the locking around init_coupling(). I
think it should be better to keep the find_coupler() protected.
Michał, does this fix yours problem?
--- >8 ---
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 75ff7c563c5d..513f95c6f837 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5040,7 +5040,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);
@@ -5248,9 +5251,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;
--- >8 ---
next prev parent reply other threads:[~2020-08-10 19:42 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-09 22:25 regulator: deadlock vs memory reclaim Michał Mirosław
2020-08-10 0:09 ` Dmitry Osipenko
2020-08-10 15:39 ` Mark Brown
2020-08-10 16:09 ` Michał Mirosław
2020-08-10 17:31 ` Mark Brown
2020-08-10 19:25 ` Michał Mirosław
2020-08-10 19:41 ` Dmitry Osipenko [this message]
2020-08-10 19:51 ` Mark Brown
[not found] <cover.1597089543.git.mirq-linux@rere.qmqm.pl>
2020-08-10 20:15 ` Dmitry Osipenko
2020-08-10 20:18 ` Michał Mirosław
2020-08-10 20:21 ` Dmitry Osipenko
2020-08-10 20:56 ` Dmitry Osipenko
2020-08-10 21:23 ` Dmitry Osipenko
2020-08-11 0:07 ` Michał Mirosław
2020-08-11 15:44 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=08f030a2-3a6f-3ab4-1855-3016884db79d@gmail.com \
--to=digetx@gmail.com \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mirq-linux@rere.qmqm.pl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).