3.18: lockdep problems in cpufreq

Here's a nice Christmas present one of my iMX6 machines gave me tonight. I haven't begun to look into it.

======================================================
[ INFO: possible circular locking dependency detected ]
3.18.0+ #1453 Not tainted
-------------------------------------------------------
rc.local/770 is trying to acquire lock:
(cooling_cpufreq_lock){+.+.+.}, at: [<c04abfc4>] cpufreq_thermal_notifier+0x34/0xfc

but task is already holding lock:
((cpufreq_policy_notifier_list).rwsem){++++.+}, at: [<c0042f04>] __blocking_notifier_call_chain+0x34/0x68

which lock already depends on the new lock.

Quoted 2 lines · Russell King - ARM Linux

Here's a nice Christmas present one of my iMX6 machines gave me tonight. I haven't begun to look into it.

Quoted 7 lines

======================================================
[ INFO: possible circular locking dependency detected ]
3.18.0+ #1453 Not tainted
-------------------------------------------------------
rc.local/770 is trying to acquire lock:
(cooling_cpufreq_lock){+.+.+.}, at: [<c04abfc4>] cpufreq_thermal_notifier+0x34/0xfc

Quoted 4 lines

but task is already holding lock:
((cpufreq_policy_notifier_list).rwsem){++++.+}, at: [<c0042f04>] __blocking_notifier_call_chain+0x34/0x68

which lock already depends on the new lock.

Well, that's for Viresh.

-----Original Message-----
From: Viresh Kumar [mailto:[email protected]]
Sent: Monday, December 15, 2014 9:27 AM
To: Rafael J. Wysocki; [email protected]
Cc: Russell King - ARM Linux; [email protected]; [email protected]; Eduardo Valentin
Subject: Re: 3.18: lockdep problems in cpufreq

Quoted 1 line · Rafael J. Wysocki

On Sunday, December 14, 2014 09:36:55 PM Russell King - ARM Linux

Quoted 1 line · wrote:

Quoted 1 line

Here's a nice Christmas present one of my iMX6 machines gave me

tonight.

Quoted 1 line

Quoted 1 line

I haven't begun to look into it.

Is the culprit this one?

Fixes 2dcd851fe4b4 ("thermal: cpu_cooling: Update always cpufreq

policy with thermal constraints")

As this is what it changed:

(quoted diff) +17 -10

@@ -316,21 +312,28 @@ static int cpufreq_thermal_notifier(struct
 notifier_block *nb,  {
        struct cpufreq_policy *policy = data;
        unsigned long max_freq = 0;
+       struct cpufreq_cooling_device *cpufreq_dev;
 
-       if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID)
+       if (event != CPUFREQ_ADJUST)
                return 0;
 
-       if (cpumask_test_cpu(policy->cpu, &notify_device-
 >allowed_cpus))
-               max_freq = notify_device->cpufreq_val;
-       else
-               return 0;
+       mutex_lock(&cooling_cpufreq_lock);
+       list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) {
+               if (!cpumask_test_cpu(policy->cpu,
+                                       &cpufreq_dev->allowed_cpus))
+                       continue;
+
+               if (!cpufreq_dev->cpufreq_val)
+                       cpufreq_dev->cpufreq_val = get_cpu_frequency(
+                                       cpumask_any(&cpufreq_dev-
 >allowed_cpus),
+                                       cpufreq_dev->cpufreq_state);
 
-       /* Never exceed user_policy.max */
-       if (max_freq > policy->user_policy.max)
-               max_freq = policy->user_policy.max;
+               max_freq = cpufreq_dev->cpufreq_val;
 
-       if (policy->max != max_freq)
-               cpufreq_verify_within_limits(policy, 0, max_freq);
+               if (policy->max != max_freq)
+                       cpufreq_verify_within_limits(policy, 0,
 max_freq);
+       }
+       mutex_unlock(&cooling_cpufreq_lock);

        return 0;

}

Quoted 2 lines

Quoted 2 lines

======================================================
[ INFO: possible circular locking dependency detected ] 3.18.0+

#1453

Quoted 10 lines

Quoted 10 lines

Not tainted
-------------------------------------------------------
rc.local/770 is trying to acquire lock:
(cooling_cpufreq_lock){+.+.+.}, at: [<c04abfc4>]
cpufreq_thermal_notifier+0x34/0xfc

but task is already holding lock:
((cpufreq_policy_notifier_list).rwsem){++++.+}, at: [<c0042f04>]
__blocking_notifier_call_chain+0x34/0x68

Quoted 3 lines

Quoted 1 line

which lock already depends on the new lock.

Well, that's for Viresh.

Maybe not as the rework I have done is queued for this merge window. I was afraid really after reading the "offenders" discussion on IRC :)

Cc'd Yadwinder as well who wrote this patch.

Unfortunately, I didn?t get any such warning though I tested patch enabling CONFIG_PROVE_LOCKING before posting. It seems Russell is trying to load imx_thermal as module and parallely Changing cpufreq governor from userspace, which was not my test case.

Anyways, after analyzing log and code,I think problem is not in cpufreq_thermal_notifier which was modified in patch as stated above. Actual problem is in __cpufreq_cooling_register which is unnecessarily calling cpufreq_register_notifier() inside section protected by cooling_cpufreq_lock.
Because cpufreq_policy_notifier_list).rwsem is already held
by store_scaling_governor when __cpufreq_cooling_register is trying to cpufreq_policy_notifier_list while holding cooling_cpufreq_lock.

So I think following can fix the problem:

drivers/thermal/cpu_cooling.c +0 -3

index ad09e51..622ea40 100644
@@ -484,15 +484,15 @@ __cpufreq_cooling_register(struct device_node *np,
        cpufreq_dev->cpufreq_state = 0;
        mutex_lock(&cooling_cpufreq_lock);
 
-       /* Register the notifier for first cpufreq cooling device */
-       if (cpufreq_dev_count == 0)
-               cpufreq_register_notifier(&thermal_cpufreq_notifier_block,

-----Original Message-----
From: Russell King - ARM Linux [mailto:[email protected]]
Sent: Monday, December 15, 2014 7:17 PM
To: Yadwinder Singh Brar
Cc: 'Viresh Kumar'; 'Rafael J. Wysocki'; linux-arm- [email protected]; [email protected]; 'Eduardo Valentin'
Subject: Re: 3.18: lockdep problems in cpufreq

Quoted 4 lines · Yadwinder Singh Brar

Unfortunately, I didn’t get any such warning though I tested patch enabling CONFIG_PROVE_LOCKING before posting. It seems Russell is trying to load imx_thermal as module and parallely Changing cpufreq governor from userspace, which was not my test case.

No. Yes, imx_thermal is a module, which is loaded by udev very early in the userspace boot. Later on, in the rc.local, I poke the governor from userspace.

This is evidenced by the bluetooth modules being loaded after imx_thermal:

Module                  Size  Used by
bnep                   10574  2
rfcomm                 33720  0
bluetooth             293598  10 bnep,rfcomm
nfsd                   90264  0
exportfs                3988  1 nfsd
hid_cypress             1763  0
snd_soc_fsl_spdif       9753  2
imx_pcm_dma             1137  1 snd_soc_fsl_spdif
imx_sdma               12885  2
imx2_wdt                3248  0
imx_thermal             5386  0
snd_soc_imx_spdif       1877  0

and the timestamp on the bluetooth message:

[   25.503739] Bluetooth: Core ver 2.19

vs the timestamp on the lockdep message:

[   29.499195] [ INFO: possible circular locking dependency detected ]

My rc.local does this:

# Configure cpufreq
echo 1 > /sys/devices/system/cpu/cpufreq/ondemand/io_is_busy echo performance >
/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

Quoted 3 lines

Anyways, after analyzing log and code,I think problem is not in cpufreq_thermal_notifier which was modified in patch as stated above. Actual problem is in __cpufreq_cooling_register which is

unnecessarily

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node

Quoted 13 lines

calling cpufreq_register_notifier() inside section protected by cooling_cpufreq_lock.
Because cpufreq_policy_notifier_list).rwsem is already held by store_scaling_governor when __cpufreq_cooling_register is trying to cpufreq_policy_notifier_list while holding cooling_cpufreq_lock.

So I think following can fix the problem:

(quoted diff) +0 -0

b/drivers/thermal/cpu_cooling.c index ad09e51..622ea40 100644

drivers/thermal/cpu_cooling.c +0 -0

@@ -484,15 +484,15 @@ __cpufreq_cooling_register(struct device_node

*np,

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node *np,

Quoted 6 lines

drivers/thermal/cpu_cooling.c +0 -3

        cpufreq_dev->cpufreq_state = 0;
        mutex_lock(&cooling_cpufreq_lock);
 
-       /* Register the notifier for first cpufreq cooling device */
-       if (cpufreq_dev_count == 0)
-

cpufreq_register_notifier(&thermal_cpufreq_notifier_block,

Quoted 6 lines

drivers/thermal/cpu_cooling.c +0 -1

-                                         CPUFREQ_POLICY_NOTIFIER);
        cpufreq_dev_count++;
        list_add(&cpufreq_dev->node, &cpufreq_dev_list);
 
        mutex_unlock(&cooling_cpufreq_lock);
 

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node *np,

Quoted 3 lines

drivers/thermal/cpu_cooling.c +3 -0

+       /* Register the notifier for first cpufreq cooling device */
+       if (cpufreq_dev_count == 0)
+

cpufreq_register_notifier(&thermal_cpufreq_notifier_block,

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node *np, · line 495

Quoted 1 line

drivers/thermal/cpu_cooling.c +1 -0

+                                         CPUFREQ_POLICY_NOTIFIER);

No, sorry, this patch is worse.

1. cpufreq_register_notifier() will never be called, even on the first caller to this code: at the point where it's tested for zero, it
will
be incremented to one by the previous code.

2. What happens if two threads come here?

The first thread succeeds in grabbing cooling_cpufreq_lock. The
second
thread stalls waiting for cooling_cpufreq_lock to be released.

The first thread increments cpufreq_dev_count, adds to the list, and
then
releases the lock. At this point, control may be passed to the
second
thread (since mutex_unlock() will wake it up.) The second thread
gets
into the critical region, and increments cpufreq_dev_count again.

By the time the first thread runs, cpufreq_dev_count may be two at
this
point.

Unfortunately, you do need some kind of synchronisation here. If it's not important when cpufreq_register_notifier() gets called, maybe this can work:

	bool register;

	mutex_lock(&cooling_cpufreq_lock);
	register = cpufreq_dev_count++ == 0;
	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
	mutex_unlock(&cooling_cpufreq_lock);

	if (register)

		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
					  CPUFREQ_POLICY_NOTIFIER);

However, I suspect that may well be buggy if we have a cleanup path which unregisters the notifier when cpufreq_dev_count is decremented to zero...
which we seem to have in cpufreq_cooling_unregister().

The answer may well be to have finer grained locking here:

- one lock to protect cpufreq_dev_list, which is only ever taken when adding or removing entries from it

- a second lock to protect cpufreq_dev_count and the calls to cpufreq_register_notifier() and cpufreq_unregister_notifier()

and you would /never/ take either of those two locks inside each other. In other words:

	mutex_lock(&cooling_list_lock);
	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
	mutex_unlock(&cooling_list_lock);

	mutex_lock(&cooling_cpufreq_lock);
	if (cpufreq_dev_count++ == 0)
		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
					  CPUFREQ_POLICY_NOTIFIER);
	mutex_unlock(&cooling_cpufreq_lock);

and similar in the cleanup path. The notifier itself would only ever take the cooling_list_lock.

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

Quoted 21 lines

The answer may well be to have finer grained locking here:

- one lock to protect cpufreq_dev_list, which is only ever taken when adding or removing entries from it

- a second lock to protect cpufreq_dev_count and the calls to cpufreq_register_notifier() and cpufreq_unregister_notifier()

and you would /never/ take either of those two locks inside each other. In other words:

	mutex_lock(&cooling_list_lock);
	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
	mutex_unlock(&cooling_list_lock);

	mutex_lock(&cooling_cpufreq_lock);
	if (cpufreq_dev_count++ == 0)
		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
					  CPUFREQ_POLICY_NOTIFIER);
	mutex_unlock(&cooling_cpufreq_lock);

Quoted 3 lines

and similar in the cleanup path. The notifier itself would only ever take the cooling_list_lock.

I agree with this approach, if its fine for others also, I can implement and post patch.

Quoted 21 lines · Yadwinder Singh Brar

Quoted 21 lines

The answer may well be to have finer grained locking here:

- one lock to protect cpufreq_dev_list, which is only ever taken when adding or removing entries from it

- a second lock to protect cpufreq_dev_count and the calls to cpufreq_register_notifier() and cpufreq_unregister_notifier()

and you would /never/ take either of those two locks inside each other. In other words:

	mutex_lock(&cooling_list_lock);
	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
	mutex_unlock(&cooling_list_lock);

	mutex_lock(&cooling_cpufreq_lock);
	if (cpufreq_dev_count++ == 0)
		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
					  CPUFREQ_POLICY_NOTIFIER);
	mutex_unlock(&cooling_cpufreq_lock);

Quoted 6 lines

Quoted 3 lines

and similar in the cleanup path. The notifier itself would only ever take the cooling_list_lock.

I agree with this approach, if its fine for others also, I can implement and post patch.

Yes, please do.

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Yadwinder Singh Brar

I agree with this approach, if its fine for others also, I can implement and post patch.

Yes, please do.

Indeed. We have a regression here to fix.

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.

8<===
From: Russell King <[email protected]>
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 <[email protected]>

---

 drivers/thermal/cpu_cooling.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/thermal/cpu_cooling.c +8 -4

index ad09e51ffae4..9e42c6f30785 100644
@@ -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 */

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 88 lines

8<===
From: Russell King <[email protected]>
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 <[email protected]>

---

 drivers/thermal/cpu_cooling.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/thermal/cpu_cooling.c +8 -4

index ad09e51ffae4..9e42c6f30785 100644
@@ -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 */

drivers/thermal/cpu_cooling.c void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) · line 575

Quoted 10 lines

drivers/thermal/cpu_cooling.c +4 -0

@@ -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 <[email protected]>

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 3 lines

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.

Quoted 103 lines

Quoted 89 lines

8<===
From: Russell King <[email protected]>
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 <[email protected]>

---

 drivers/thermal/cpu_cooling.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/thermal/cpu_cooling.c +8 -4

index ad09e51ffae4..9e42c6f30785 100644
@@ -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 */

drivers/thermal/cpu_cooling.c void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) · line 575

Quoted 10 lines

drivers/thermal/cpu_cooling.c +4 -0

@@ -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 <[email protected]>

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 5 lines

Quoted 3 lines

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.

This should go in through the thermal tree. Eduardo?

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · Russell King - ARM Linux

Quoted 2 lines · Viresh Kumar

On 16 December 2014 at 04:39, Russell King - ARM Linux <[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 9 lines · Russell King - ARM Linux

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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.

Was the patch for latest kernel ever circulated ? Probably that's why Eduardo never got back to it.

Maybe not - I often lose track of which patches are in what state - tracking the state of 250+ patches over many months and in some cases _years_ is not easy.

That's still no reason for Eduardo to _completely_ ignore this thread, especially when Rafael explicitly tried to get comment from Eduardo.

drivers/thermal/cpu_cooling.c +1 -1

 		list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) {
 			if (!cpumask_test_cpu(policy->cpu,
 					      &cpufreq_dev->allowed_cpus))

@@ -233,7 +236,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 				cpufreq_verify_within_limits(policy, 0,
 							     max_freq);
 		}
-		mutex_unlock(&cooling_cpufreq_lock);
+		mutex_unlock(&cooling_list_lock);
 		break;
 	default:
 		return NOTIFY_DONE;

drivers/thermal/cpu_cooling.c +3 -0

+	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->time_in_idle_timestamp);

Signature · 1 line

2.1.0

drivers/thermal/cpu_cooling.c static int cpufreq_thermal_notifier(struct notifier_block *nb, · line 227

Quoted 8 lines · Russell King

drivers/thermal/cpu_cooling.c +1 -1

 /**

@@ -221,7 +224,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 	switch (event) {
 
 	case CPUFREQ_ADJUST:
-		mutex_lock(&cooling_cpufreq_lock);
+		mutex_lock(&cooling_list_lock);

There is one more place where the list's locking needs update:
cpufreq_cooling_get_level().

Quoted 15 lines

drivers/thermal/cpu_cooling.c +1 -1

 		list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) {
 			if (!cpumask_test_cpu(policy->cpu,
 					      &cpufreq_dev->allowed_cpus))

@@ -233,7 +236,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 				cpufreq_verify_within_limits(policy, 0,
 							     max_freq);
 		}
-		mutex_unlock(&cooling_cpufreq_lock);
+		mutex_unlock(&cooling_list_lock);
 		break;
 	default:
 		return NOTIFY_DONE;

@@ -865,12 +868,15 @@ __cpufreq_cooling_register(struct device_node *np,
 	cpufreq_dev->cool_dev = cool_dev;
 

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node *np, · line 871

Quoted 2 lines

drivers/thermal/cpu_cooling.c +1 -0

 	mutex_lock(&cooling_cpufreq_lock);
+	mutex_lock(&cooling_list_lock);

Why is the list lock taken from within the existing lock here? and ...

drivers/thermal/cpu_cooling.c __cpufreq_cooling_register(struct device_node *np, · line 879

Quoted 10 lines

drivers/thermal/cpu_cooling.c +4 -2

+	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
+	mutex_unlock(&cooling_list_lock);
 
 	/* Register the notifier for first cpufreq cooling device */
-	if (list_empty(&cpufreq_dev_list))
+	if (cpufreq_dev_count == 0)
 		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
 					  CPUFREQ_POLICY_NOTIFIER);
-	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
+	cpufreq_dev_count++;

Maybe:

        if (!cpufreq_dev_count++)
                cpufreq_register_notifier();

Possibly, had the code previously been like that. See:

commit 2479bb6443d6a793f896219a34bfab0cc410f0b4

Author: Viresh Kumar <[email protected]>
Date:   Thu Dec 4 09:42:04 2014 +0530

which post-dates this patch, and removes this "style" in favour of using the broken list mechanism.

Frankly... you don't have a leg to stand on

because _you_ knew about this problem, and you saw my patch. So removing something that my patch to fix a serious bug relied upon...

You know what? At this point, I'm just not going to bother. You fix this lockdep problem, I'm obviously useless at kernel stuff.

For some reason, despite Rafael saying that this should go in, despite it having been reviewed, Eduardo appears to be ignoring basic fixes to code under his control. Having waited more than a reasonable amount of time, it is now time to take some action against this blockage. Hence, I'm putting this into linux-next in the hope that it'll conflict and elicit a response for this important bug fix.

Relevant message IDs:
20141214213655.GA11285 at n2100.arm.linux.org.uk 7573578.UE8ufgjWuX at vostro.rjw.lan 20141215230922.GL11285 at n2100.arm.linux.org.uk CAKohponhkk9BK6-7ScHeZa4R43iooWQFV8YoPLv6LjO40GNq=A at mail.gmail.com 20150518185645.GA28053 at n2100.arm.linux.org.uk 2574268.XBqpdL2VLI at vostro.rjw.lan

---
 drivers/thermal/cpu_cooling.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/thermal/cpu_cooling.c +16 -6

index 6509c61b9648..d1fd02eec58d 100644
@@ -107,6 +107,9 @@ struct cpufreq_cooling_device {
 static DEFINE_IDR(cpufreq_idr);
 static DEFINE_MUTEX(cooling_cpufreq_lock);
 
+static unsigned int cpufreq_dev_count;
+
+static DEFINE_MUTEX(cooling_list_lock);
 static LIST_HEAD(cpufreq_dev_list);
 
 /**

@@ -221,7 +224,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 	switch (event) {
 
 	case CPUFREQ_ADJUST:
-		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))

@@ -233,7 +236,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 				cpufreq_verify_within_limits(policy, 0,
 							     max_freq);
 		}
-		mutex_unlock(&cooling_cpufreq_lock);
+		mutex_unlock(&cooling_list_lock);
 		break;
 	default:
 		return NOTIFY_DONE;

@@ -865,12 +868,15 @@ __cpufreq_cooling_register(struct device_node *np,
 	cpufreq_dev->cool_dev = cool_dev;
 
 	mutex_lock(&cooling_cpufreq_lock);
+	mutex_lock(&cooling_list_lock);
+	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
+	mutex_unlock(&cooling_list_lock);
 
 	/* Register the notifier for first cpufreq cooling device */
-	if (list_empty(&cpufreq_dev_list))
+	if (cpufreq_dev_count == 0)
 		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
 					  CPUFREQ_POLICY_NOTIFIER);
-	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
+	cpufreq_dev_count++;
 
 	mutex_unlock(&cooling_cpufreq_lock);
 

@@ -1014,14 +1020,18 @@ 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 */
-	if (list_empty(&cpufreq_dev_list))
+	if (cpufreq_dev_count == 0)
 		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
 					    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->time_in_idle_timestamp);

Signature · 1 line

2.1.0

Sorry if I let this one to fall into the cracks for this long.

I am looking at this now and queuing as soon as possible.

Quoted 3 lines · Eduardo Valentin

Sorry if I let this one to fall into the cracks for this long.

I am looking at this now and queuing as soon as possible.

You applied the wrong one, please drop it. And apply the one I have sent as a separate thread, with my sob on it.

Signature · 2 lines

-- 
viresh

Quoted 7 lines · Viresh Kumar

Quoted 3 lines · Eduardo Valentin

Sorry if I let this one to fall into the cracks for this long.

I am looking at this now and queuing as soon as possible.

You applied the wrong one, please drop it. And apply the one I have sent as a separate thread, with my sob on it.

OK. I am updating it to your V2.

Quoted 3 lines · Russell King - ARM Linux

Maybe not - I often lose track of which patches are in what state - tracking the state of 250+ patches over many months and in some cases _years_ is not easy.

I agree..

Quoted 2 lines

That's still no reason for Eduardo to _completely_ ignore this thread, especially when Rafael explicitly tried to get comment from Eduardo.

Oh, no. I am not trying to be anybody's advocate here. I was just highlighting the point here, that I remembered.

Even I was quite busy then and couldn't get back to it.

Lets fix this now. I believe you have a patch for this based on latest kernel? Mind pasting that here for quick review?

The problem will be back-porting it to stable kernels, because I think it's had to be updated at least a couple of times. I don't have the old versions anymore, so I'm just going to say "not my problem" - sorry.

Quoted 3 lines · Russell King - ARM Linux

The problem will be back-porting it to stable kernels, because I think it's had to be updated at least a couple of times. I don't have the old versions anymore, so I'm just going to say "not my problem" - sorry.

Your old 3.18 version :)

And now generate versions for all the stable kernels inbetween. Now your problem. I'm sick of this.

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

This should go in through the thermal tree. Eduardo?

Having waited a long time for any kind of response from Eduardo, I've given up. My conclusion is that Eduardo isn't interested in this.

I've re-checked, and the AB-BA deadlock is still there in the latest code. So, I've taken it upon myself to throw this into my for-next branch to force the issue - not something I _want_ to do, but I'm doing this out of frustration. It's clear to me that "playing nice" by email does _not_ work with some people.

I'm rather hoping that Stephen reports a merge conflict with linux-next this evening to highlight this situation. I've added additional commentry to the commit message on the patch giving the reason why I've done this, and the relevant message IDs showing the past history.

I've not decided whether I'm going to ask Linus to take this patch directly or not, that rather depends whether there's any co-operation from Eduardo on this. I'd rather Eduardo took the patch.

The patch I have has had to be updated again for changes to the driver, but I really don't see the point of re-posting it just for it to be ignored yet again.

I'm really disappointed by this dysfunctional state of affairs, and that what should be an urgent fix for an observable problem is still not merged some nine months after it was first identified.

Quoted 39 lines · Russell King - ARM Linux

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

This should go in through the thermal tree. Eduardo?

Having waited a long time for any kind of response from Eduardo, I've given up. My conclusion is that Eduardo isn't interested in this.

I've re-checked, and the AB-BA deadlock is still there in the latest code. So, I've taken it upon myself to throw this into my for-next branch to force the issue - not something I _want_ to do, but I'm doing this out of frustration. It's clear to me that "playing nice" by email does _not_ work with some people.

I'm rather hoping that Stephen reports a merge conflict with linux-next this evening to highlight this situation. I've added additional commentry to the commit message on the patch giving the reason why I've done this, and the relevant message IDs showing the past history.

I've not decided whether I'm going to ask Linus to take this patch directly or not, that rather depends whether there's any co-operation from Eduardo on this. I'd rather Eduardo took the patch.

The patch I have has had to be updated again for changes to the driver, but I really don't see the point of re-posting it just for it to be ignored yet again.

Quoted 3 lines

I'm really disappointed by this dysfunctional state of affairs, and that what should be an urgent fix for an observable problem is still not merged some nine months after it was first identified.

I guess it might help if you sent the updated patch in a new thread.

That I doubt. Eduardo has not bothered to reply at _any_ time. I have to question whether there is anyone even reading that email address, or whether it's a redirect to /dev/null. All the evidence I have right now is that this Eduardo is a ficticous character.

I would have at least expected some complaints when I said "I've put it in linux-next" but... absolutely nothing.

So... my only conclusion is that you're all pulling my leg that there _is_ this "Eduardo" maintainer who's supposed to be taking patches for this stuff.

As I've said, I'm not bothering with this anymore, it's just far too much effort to play these stupid games. The deadlock can stay for all I care.

Quoted 40 lines · Rafael J. Wysocki

Quoted 39 lines · Russell King - ARM Linux

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Viresh Kumar

Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

This should go in through the thermal tree. Eduardo?

Having waited a long time for any kind of response from Eduardo, I've given up. My conclusion is that Eduardo isn't interested in this.

I've re-checked, and the AB-BA deadlock is still there in the latest code. So, I've taken it upon myself to throw this into my for-next branch to force the issue - not something I _want_ to do, but I'm doing this out of frustration. It's clear to me that "playing nice" by email does _not_ work with some people.

I'm rather hoping that Stephen reports a merge conflict with linux-next this evening to highlight this situation. I've added additional commentry to the commit message on the patch giving the reason why I've done this, and the relevant message IDs showing the past history.

I've not decided whether I'm going to ask Linus to take this patch directly or not, that rather depends whether there's any co-operation from Eduardo on this. I'd rather Eduardo took the patch.

The patch I have has had to be updated again for changes to the driver, but I really don't see the point of re-posting it just for it to be ignored yet again.

Quoted 5 lines

Quoted 3 lines

I'm really disappointed by this dysfunctional state of affairs, and that what should be an urgent fix for an observable problem is still not merged some nine months after it was first identified.

I guess it might help if you sent the updated patch in a new thread.

That I doubt. Eduardo has not bothered to reply at _any_ time. I have to question whether there is anyone even reading that email address, or whether it's a redirect to /dev/null. All the evidence I have right now is that this Eduardo is a ficticous character.

I would have at least expected some complaints when I said "I've put it in linux-next" but... absolutely nothing.

So... my only conclusion is that you're all pulling my leg that there _is_ this "Eduardo" maintainer who's supposed to be taking patches for this stuff.

As I've said, I'm not bothering with this anymore, it's just far too much effort to play these stupid games.

The deadlock can stay for all I care.

Well, that's the situation as per MAINTAINERS today. You seem to be concerned that it may not reflect the reality, but in that case I can only recommend sending a patch against MAINTAINERS to remove Eduardo from there.

Quoted 2 lines

As I've said, I'm not bothering with this anymore, it's just far too much effort to play these stupid games.

I'm not sure what you mean by "these stupid games" here.

Quoted 1 line

The deadlock can stay for all I care.

Fair enough.

Thanks,
Rafael

Quoted 11 lines

Unfortunately, you do need some kind of synchronisation here. If it's not important when cpufreq_register_notifier() gets called, maybe this can work:

      bool register;

      mutex_lock(&cooling_cpufreq_lock);
      register = cpufreq_dev_count++ == 0;
      list_add(&cpufreq_dev->node, &cpufreq_dev_list);
      mutex_unlock(&cooling_cpufreq_lock);

Quoted 1 line

if (register)

register may be 0 in scenario you stated above in second point. So this approach will not work.

Unfortunately, I didn?t get any such warning though I tested patch enabling CONFIG_PROVE_LOCKING before posting. It seems

Anyways, after analyzing log and code,I think problem is not in cpufreq_thermal_notifier which was modified in patch as stated above. Actual problem is in __cpufreq_cooling_register which is unnecessarily calling cpufreq_register_notifier() inside section protected by cooling_cpufreq_lock.

Because cpufreq_policy_notifier_list).rwsem is already held
by store_scaling_governor when __cpufreq_cooling_register is trying to cpufreq_policy_notifier_list while holding cooling_cpufreq_lock.

So I think following can fix the problem:

drivers/thermal/cpu_cooling.c +0 -1

index ad09e51..622ea40 100644
@@ -484,15 +484,15 @@ __cpufreq_cooling_register(struct device_node *np,
        cpufreq_dev->cpufreq_state = 0;
        mutex_lock(&cooling_cpufreq_lock);
 
-       /* Register the notifier for first cpufreq cooling device */

drivers/thermal/cpu_cooling.c +2 -0

+               cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
+                                         CPUFREQ_POLICY_NOTIFIER);
        return cool_dev;
 }