Messages in this thread | | | Subject | Re: [PATCH] regulator: core: Avoid potential deadlock on regulator_unregister | From | Dmitry Osipenko <> | Date | Thu, 4 Apr 2019 18:55:11 +0300 |
| |
04.04.2019 18:32, Charles Keepax пишет: > Lockdep reports the following issue on my setup: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock((work_completion)(&(&rdev->disable_work)->work)); > lock(regulator_list_mutex); > lock((work_completion)(&(&rdev->disable_work)->work)); > lock(regulator_list_mutex); > > The problem is that regulator_unregister takes the > regulator_list_mutex and then calls flush_work on disable_work. But > regulator_disable_work calls regulator_lock_dependent which will > also take the regulator_list_mutex. Resulting in a deadlock if the > flush_work call actually needs to flush the work. > > Fix this issue by moving the flush_work outside of the > regulator_list_mutex. The list mutex is not used to guard the point at > which the delayed work is queued, so its use adds no additional safety. > > Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking") > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > > This patch follows on from my email the other day [1]. > > After looking at things in more detail I am fairly confident this is > a good fix. I do still have a slight nagging doubt that something > should be protecting this flush_work from additional works being > queued, and I can't see what that is. But as that is definitely not > the regulator_list_mutex the patch is not making this any more > dangerous. In practice I suspect this is fine as nothing should > really be using a regulator that is about to be unregistered, > or really this delayed work is probably the least of the systems > problems.
If anything tries to use regulator after invoking the regulator_unregister(), then indeed it's already in trouble. The patch looks good to me, thanks.
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
| |