Re: 3.18: lockdep problems in cpufreq

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Tue, Dec 16, 2014 at 09:11:53AM +0530, Viresh Kumar wrote:
> On 16 December 2014 at 04:39, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Well, here's a patch which I'm running on top of 3.18 at the moment,
> > which is basically what I described in my email, and I'm running with it
> > and it is without any lockdep complaint.
> 
> We need two separate patches now, one for 3.18 and other one for 3.19-rc.
> 3.19 has see lots of changes in this particular file and so we need to
> change few things here.

What happened with this?  I'm still carrying the patch.

> > 8<===
> > From: Russell King <rmk+kernel@arm.linux.org.uk>
> > thermal: cpu_cooling: fix lockdep problems in cpu_cooling
> >
> > A recent change to the cpu_cooling code introduced a AB-BA deadlock
> > scenario between the cpufreq_policy_notifier_list rwsem and the
> > cooling_cpufreq_lock.  This is caused by cooling_cpufreq_lock being held
> > before the registration/removal of the notifier block (an operation
> > which takes the rwsem), and the notifier code itself which takes the
> > locks in the reverse order.
> >
> > Solve this by moving to finer grained locking - use one mutex to protect
> > the cpufreq_dev_list as a whole, and a separate lock to ensure correct
> > ordering of cpufreq notifier registration and removal.
> >
> > I considered taking the cooling_list_lock within cooling_cpufreq_lock to
> > protect the registration sequence as a whole, but that adds a dependency
> > between these two locks which is best avoided (lest someone tries to
> > take those two new locks in the reverse order.)  In any case, it's safer
> > to have an empty cpufreq_dev_list than to have unnecessary dependencies
> > between locks.
> >
> > Fixes: 2dcd851fe4b4 ("thermal: cpu_cooling: Update always cpufreq policy with thermal constraints")
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >
> >  drivers/thermal/cpu_cooling.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index ad09e51ffae4..9e42c6f30785 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -57,6 +57,7 @@ static DEFINE_MUTEX(cooling_cpufreq_lock);
> >
> >  static unsigned int cpufreq_dev_count;
> >
> > +static DEFINE_MUTEX(cooling_list_lock);
> >  static LIST_HEAD(cpufreq_dev_list);
> >
> >  /**
> > @@ -317,7 +318,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
> >         if (event != CPUFREQ_ADJUST)
> >                 return 0;
> >
> > -       mutex_lock(&cooling_cpufreq_lock);
> > +       mutex_lock(&cooling_list_lock);
> >         list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) {
> >                 if (!cpumask_test_cpu(policy->cpu,
> >                                         &cpufreq_dev->allowed_cpus))
> > @@ -333,7 +334,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
> >                 if (policy->max != max_freq)
> >                         cpufreq_verify_within_limits(policy, 0, max_freq);
> >         }
> > -       mutex_unlock(&cooling_cpufreq_lock);
> > +       mutex_unlock(&cooling_list_lock);
> >
> >         return 0;
> >  }
> > @@ -482,6 +483,11 @@ __cpufreq_cooling_register(struct device_node *np,
> >         }
> >         cpufreq_dev->cool_dev = cool_dev;
> >         cpufreq_dev->cpufreq_state = 0;
> > +
> > +       mutex_lock(&cooling_list_lock);
> > +       list_add(&cpufreq_dev->node, &cpufreq_dev_list);
> > +       mutex_unlock(&cooling_list_lock);
> > +
> >         mutex_lock(&cooling_cpufreq_lock);
> >
> >         /* Register the notifier for first cpufreq cooling device */
> > @@ -489,7 +495,6 @@ __cpufreq_cooling_register(struct device_node *np,
> >                 cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> >                                           CPUFREQ_POLICY_NOTIFIER);
> >         cpufreq_dev_count++;
> > -       list_add(&cpufreq_dev->node, &cpufreq_dev_list);
> >
> >         mutex_unlock(&cooling_cpufreq_lock);
> >
> > @@ -553,7 +558,6 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >
> >         cpufreq_dev = cdev->devdata;
> >         mutex_lock(&cooling_cpufreq_lock);
> > -       list_del(&cpufreq_dev->node);
> >         cpufreq_dev_count--;
> >
> >         /* Unregister the notifier for the last cpufreq cooling device */
> > @@ -562,6 +566,10 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >                                             CPUFREQ_POLICY_NOTIFIER);
> >         mutex_unlock(&cooling_cpufreq_lock);
> >
> > +       mutex_lock(&cooling_list_lock);
> > +       list_del(&cpufreq_dev->node);
> > +       mutex_unlock(&cooling_list_lock);
> > +
> >         thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
> >         release_idr(&cpufreq_idr, cpufreq_dev->id);
> >         kfree(cpufreq_dev);
> 
> For 3.18
> 
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.