stable 3-10-3: strange output of "lsmod | grep ^acpi_cpufreq"

DISCUSSION Toralf Förster 4 participants 22 messages Jul 27, 2013 – Jul 30, 2013 CPU frequency scaling framework

lore

Share & tools

lore

Toralf Förster

author

2013-07-27 17:40 UTC #0 lore raw

it gives at a ThinkPad T420:

tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647

wow !

Signature · 3 lines

MfG/Sincerely
Toralf FÃ¶rster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Rafael J. Wysocki

2013-07-27 23:39 UTC #1 lore raw

Quoted 4 lines · Toralf FÃ¶rster

it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647

That is -1, which indicates some module refcount woes.

I definitely can't see that with the mainline on my machines.

Thanks,
Rafael

Toralf Förster

author

2013-07-28 08:08 UTC #2 lore raw

Quoted 9 lines · Rafael J. Wysocki

Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
I definitely can't see that with the mainline on my machines.

It might be a regression in -stable only, b/c in 3,10.2 I did not observed it.

Srivatsa,

by any chance - could the revert of the cpufreq patches have something to do with that ?

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Srivatsa S. Bhat

2013-07-28 08:08 UTC #3 lore raw

Quoted 11 lines · Toralf Förster

Quoted 9 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
I definitely can't see that with the mainline on my machines.

Quoted 8 lines

It might be a regression in -stable only, b/c in 3,10.2 I did not observed it.
Srivatsa,
by any chance - could the revert of the cpufreq patches have something to do with that ?

Hmmm? Those reverts didn't touch anything related to module refcounts.. So I don't think they have anything to do with this.

Also, is the issue related to suspend/resume at all? (Sorry, I don't have your original email, so I'm not sure what the exact issue is).

Regards,
Srivatsa S. Bhat

Toralf Förster

author

2013-07-28 10:21 UTC #4 lore raw

In reply to Rafael J. Wysocki

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.

yes, ofc.

The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.

Quoted 1 line

I definitely can't see that with the mainline on my machines.

It is in mainline too.

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Rafael J. Wysocki

2013-07-28 22:11 UTC #5 lore raw

Quoted 9 lines · Toralf Förster

Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.

Quoted 7 lines

yes, ofc.
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.

Does the appended patch help?

Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume

Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, the latter has to bump up that refcount to start with to balance the drop, or the cpufreq driver refcount will become negative after a suspend/resume cycle.

Reported-by Toralf Förster <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

drivers/cpufreq/cpufreq.c +11 -3

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -1016,6 +1016,9 @@ static int __cpufreq_remove_dev(struct d
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
+	if (!try_module_get(cpufreq_driver->owner))
+		return -EINVAL;
+
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 
 	data = per_cpu(cpufreq_cpu_data, cpu);

@@ -1025,7 +1028,8 @@ static int __cpufreq_remove_dev(struct d
 
 	if (!data) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	if (cpufreq_driver->target)

@@ -1063,9 +1067,9 @@ static int __cpufreq_remove_dev(struct d
 
 			unlock_policy_rwsem_write(cpu);
 
-			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
+			sysfs_create_link(&cpu_dev->kobj, &data->kobj,
 					"cpufreq");
-			return -EINVAL;
+			goto err;
 		}
 
 		WARN_ON(lock_policy_rwsem_write(cpu));

@@ -1110,6 +1114,10 @@ static int __cpufreq_remove_dev(struct d
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	return 0;
+
+ err:
+	module_put(cpufreq_driver->owner);
+	return ret;
 }

Rafael J. Wysocki

2013-07-28 22:43 UTC #6 lore raw

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.

Quoted 7 lines

Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?

Actually, something as simple as this also should help:

---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume

Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.

To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().

Reported-by Toralf Förster <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq.c +1 -1

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -1181,7 +1181,7 @@ static int __cpufreq_remove_dev(struct d
 		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 
 	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
+	kobject_put(&data->kobj);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {

Rafael J. Wysocki

2013-07-28 23:20 UTC #7 lore raw

Quoted 27 lines · Rafael J. Wysocki

Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume

Quoted 6 lines

Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().

Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).

Srivatsa, does this make sense to you?

Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume

Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the acpi-cpufreq driver after a suspend/resume cycle.

This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.

Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().

On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call pufreq_cpu_put() in that case.

Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.

Reported-by Toralf Förster <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/cpufreq/cpufreq.c +6 -4

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -1181,7 +1181,6 @@ static int __cpufreq_remove_dev(struct d
 		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 
 	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {

@@ -1205,9 +1204,12 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Viresh Kumar

maintainer

2013-07-29 07:51 UTC #8 lore raw

Quoted 6 lines · Rafael J. Wysocki

Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?

It makes atleast to me :)

Quoted 9 lines

This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().

But I see one more issue with this code. For the last cpu we are just calling kobject_put() and not cpufreq_cpu_put() and hence call to module_put() is skipped. I am not sure, but that will probably cause a problem when we try to rmmod the module? But which module then? As we can't compile cpufreq.c as module.. So, is this part of code junk? And so can be removed?

Srivatsa S. Bhat

2013-07-29 09:44 UTC #9 lore raw

Quoted 20 lines · Viresh Kumar

Quoted 6 lines · Rafael J. Wysocki
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?
It makes atleast to me :)
Quoted 9 lines
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().

Quoted 7 lines

But I see one more issue with this code. For the last cpu we are just calling kobject_put() and not cpufreq_cpu_put() and hence call to module_put() is skipped. I am not sure, but that will probably cause a problem when we try to rmmod the module? But which module then? As we can't compile cpufreq.c as module.. So, is this part of code junk? And so can be removed?

I tried to address this concern in my other mail to Rafael. (Sorry I forgot to CC you on that!). Let me know what you think of that solution.

Regards,
Srivatsa S. Bhat

Srivatsa S. Bhat

2013-07-29 09:41 UTC #10 lore raw

In reply to Rafael J. Wysocki

Quoted 35 lines · Rafael J. Wysocki

Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().

Quoted 7 lines

Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?

Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.

I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.

But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.

1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");

1060
1061 return 0;

So at that point, the module refcount of the cpufreq backend driver will become zero, since we dropped it. Perhaps that was not the original intention.

Looking further (with respect to the problem we are discussing), the root-cause is that a fresh CPU never does a cpufreq_cpu_get() for itself, but does it only for the other CPUs in its policy->cpus mask.

From cpufreq_add_dev_symlink():

814 for_each_cpu(j, policy->cpus) {
815 struct cpufreq_policy *managed_policy;
816 struct device *cpu_dev;
817
818 if (j == cpu)
819 continue;
820

 821                 pr_debug("CPU %u already managed, adding link\n", j);
 822                 managed_policy = cpufreq_cpu_get(cpu);
 823                 cpu_dev = get_cpu_device(j);

So, that 'continue' statement skips bumping the refcount on behalf of the master CPU.

So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.

Something like this:

drivers/cpufreq/cpufreq.c +6 -1

index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 	}
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	/* Bump up the refcount for this CPU */
+	policy = cpufreq_cpu_get(cpu);
+
 	ret = cpufreq_add_dev_symlink(cpu, policy);
-	if (ret)
+	if (ret) {
+		cpufreq_cpu_put(policy);
 		goto err_out_kobj_put;
+	}
 
 	memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
 	/* assure that the starting sequence is run in __cpufreq_set_policy */

Thoughts?

Regards,
Srivatsa S. Bhat

Quoted 65 lines

---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the acpi-cpufreq driver after a suspend/resume cycle.
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().
On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call pufreq_cpu_put() in that case.
Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.
Reported-by Toralf Förster <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +6 -4
@@ -1181,7 +1181,6 @@ static int __cpufreq_remove_dev(struct d
 		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 
 	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
@@ -1205,9 +1204,12 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Viresh Kumar

maintainer

2013-07-29 11:22 UTC #11 lore raw

Quoted 3 lines · Srivatsa S. Bhat

I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.

I missed this simple stuff in my email.. :(

Quoted 8 lines

But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.
1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");
1060
1061 return 0;

That actually looks wrong. And shoud be fixed.

Quoted 14 lines

So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.
Something like this:
drivers/cpufreq/cpufreq.c +0 -0
index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
        }
        write_unlock_irqrestore(&cpufreq_driver_lock, flags);

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_interface(unsigned int cpu, · line 888

Quoted 10 lines

drivers/cpufreq/cpufreq.c +6 -1

 
+       /* Bump up the refcount for this CPU */
+       policy = cpufreq_cpu_get(cpu);
+
        ret = cpufreq_add_dev_symlink(cpu, policy);
-       if (ret)
+       if (ret) {
+               cpufreq_cpu_put(policy);
                goto err_out_kobj_put;
+       }

That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe.

Over that, I think below code in __cpufreq_governor() is also wrong.

	/* we keep one module reference alive for
			each CPU governed by this CPU */
	if ((event != CPUFREQ_GOV_START) || ret)
		module_put(policy->governor->owner);
	if ((event == CPUFREQ_GOV_STOP) && !ret)
		module_put(policy->governor->owner);

The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START..

But this routine is called for other stuff too:
- INIT/Exit
- LIMITS,

And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched.

Rafael J. Wysocki

2013-07-29 11:54 UTC #12 lore raw

Quoted 11 lines · Viresh Kumar

Quoted 3 lines · Srivatsa S. Bhat
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
I missed this simple stuff in my email.. :(
Quoted 3 lines
But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.

Quoted 7 lines

Quoted 5 lines
1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");
1060
1061 return 0;
That actually looks wrong. And shoud be fixed.

Quoted 41 lines

Quoted 14 lines
So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.
Something like this:
drivers/cpufreq/cpufreq.c +0 -0
index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
        }
        write_unlock_irqrestore(&cpufreq_driver_lock, flags);
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_interface(unsigned int cpu, · line 888
Quoted 10 lines
drivers/cpufreq/cpufreq.c +6 -1
 
+       /* Bump up the refcount for this CPU */
+       policy = cpufreq_cpu_get(cpu);
+
        ret = cpufreq_add_dev_symlink(cpu, policy);
-       if (ret)
+       if (ret) {
+               cpufreq_cpu_put(policy);
                goto err_out_kobj_put;
+       }
That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe.
Over that, I think below code in __cpufreq_governor() is also wrong.
	/* we keep one module reference alive for
			each CPU governed by this CPU */
	if ((event != CPUFREQ_GOV_START) || ret)
		module_put(policy->governor->owner);
	if ((event == CPUFREQ_GOV_STOP) && !ret)
		module_put(policy->governor->owner);
The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START..

Quoted 7 lines

But this routine is called for other stuff too:
- INIT/Exit
- LIMITS,
And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched.

That sounds good, but I don't want to make those changes for 3.11 and at the same time I *do* want the reference count issue to go away.

So the patch below is the one I'd like to apply for the time being and we can work on more improvements on top of that.

Any objections?

Toralf, please test this patch in the meantime.

Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume

Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the cpufreq driver after a suspend/resume cycle.

This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.

Reported-by Toralf FÃ¶rster <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/cpufreq/cpufreq.c +10 -9

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -1177,14 +1177,11 @@ static int __cpufreq_remove_dev(struct d
 				__func__, cpu_dev->id, cpu);
 	}
 
-	if ((cpus == 1) && (cpufreq_driver->target))
-		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
-
-	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
-
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
+		if (cpufreq_driver->target)
+			__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+
 		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;

@@ -1205,9 +1202,13 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Srivatsa S. Bhat

Reviewed-by

2013-07-29 11:48 UTC #13 lore raw

Quoted 109 lines · Rafael J. Wysocki

Quoted 11 lines · Viresh Kumar
Quoted 3 lines · Srivatsa S. Bhat
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
I missed this simple stuff in my email.. :(
Quoted 3 lines
But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.
Quoted 7 lines
Quoted 5 lines
1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");
1060
1061 return 0;
That actually looks wrong. And shoud be fixed.
OK
Quoted 41 lines
Quoted 14 lines
So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.
Something like this:
drivers/cpufreq/cpufreq.c +0 -0
index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
        }
        write_unlock_irqrestore(&cpufreq_driver_lock, flags);
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_interface(unsigned int cpu, · line 888
Quoted 10 lines
drivers/cpufreq/cpufreq.c +6 -1
 
+       /* Bump up the refcount for this CPU */
+       policy = cpufreq_cpu_get(cpu);
+
        ret = cpufreq_add_dev_symlink(cpu, policy);
-       if (ret)
+       if (ret) {
+               cpufreq_cpu_put(policy);
                goto err_out_kobj_put;
+       }
That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe.
Over that, I think below code in __cpufreq_governor() is also wrong.
	/* we keep one module reference alive for
			each CPU governed by this CPU */
	if ((event != CPUFREQ_GOV_START) || ret)
		module_put(policy->governor->owner);
	if ((event == CPUFREQ_GOV_STOP) && !ret)
		module_put(policy->governor->owner);
The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START..
Quoted 7 lines
But this routine is called for other stuff too:
- INIT/Exit
- LIMITS,
And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched.
That sounds good, but I don't want to make those changes for 3.11 and at the same time I *do* want the reference count issue to go away.
So the patch below is the one I'd like to apply for the time being and we can work on more improvements on top of that.
Any objections?
Toralf, please test this patch in the meantime.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the cpufreq driver after a suspend/resume cycle.
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().
On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call cpufreq_cpu_put() in that case.

Quoted 7 lines

Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.
Reported-by Toralf FÃ¶rster <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

This version looks good as well.

Reviewed-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Quoted 44 lines

---
 drivers/cpufreq/cpufreq.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================

drivers/cpufreq/cpufreq.c +10 -9

@@ -1177,14 +1177,11 @@ static int __cpufreq_remove_dev(struct d
 				__func__, cpu_dev->id, cpu);
 	}
 
-	if ((cpus == 1) && (cpufreq_driver->target))
-		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
-
-	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
-
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
+		if (cpufreq_driver->target)
+			__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+
 		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;

@@ -1205,9 +1202,13 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Toralf Förster

author

2013-07-29 17:23 UTC #14 lore raw

In reply to Rafael J. Wysocki

Quoted 74 lines · Rafael J. Wysocki

Quoted 11 lines · Viresh Kumar
Quoted 3 lines · Srivatsa S. Bhat
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
I missed this simple stuff in my email.. :(
Quoted 3 lines
But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.
Quoted 7 lines
Quoted 5 lines
1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");
1060
1061 return 0;
That actually looks wrong. And shoud be fixed.
OK
Quoted 41 lines
Quoted 14 lines
So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.
Something like this:
drivers/cpufreq/cpufreq.c +0 -0
index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
        }
        write_unlock_irqrestore(&cpufreq_driver_lock, flags);
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_interface(unsigned int cpu, · line 888
Quoted 10 lines
drivers/cpufreq/cpufreq.c +6 -1
 
+       /* Bump up the refcount for this CPU */
+       policy = cpufreq_cpu_get(cpu);
+
        ret = cpufreq_add_dev_symlink(cpu, policy);
-       if (ret)
+       if (ret) {
+               cpufreq_cpu_put(policy);
                goto err_out_kobj_put;
+       }
That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe.
Over that, I think below code in __cpufreq_governor() is also wrong.
	/* we keep one module reference alive for
			each CPU governed by this CPU */
	if ((event != CPUFREQ_GOV_START) || ret)
		module_put(policy->governor->owner);
	if ((event == CPUFREQ_GOV_STOP) && !ret)
		module_put(policy->governor->owner);
The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START..
Quoted 7 lines
But this routine is called for other stuff too:
- INIT/Exit
- LIMITS,
And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched.
That sounds good, but I don't want to make those changes for 3.11 and at the same time I *do* want the reference count issue to go away.

Quoted 6 lines

So the patch below is the one I'd like to apply for the time being and we can work on more improvements on top of that.
Any objections?
Toralf, please test this patch in the meantime.

works - tested on top of 3.10.4

Quoted 80 lines

Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the cpufreq driver after a suspend/resume cycle.
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().
On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call cpufreq_cpu_put() in that case.
Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.
Reported-by Toralf Förster <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +10 -9
@@ -1177,14 +1177,11 @@ static int __cpufreq_remove_dev(struct d
 				__func__, cpu_dev->id, cpu);
 	}
 
-	if ((cpus == 1) && (cpufreq_driver->target))
-		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
-
-	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
-
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
+		if (cpufreq_driver->target)
+			__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+
 		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;
@@ -1205,9 +1202,13 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Rafael J. Wysocki

2013-07-29 20:08 UTC #15 lore raw

Quoted 75 lines · Toralf Förster

Quoted 74 lines · Rafael J. Wysocki
Quoted 11 lines · Viresh Kumar
Quoted 3 lines · Srivatsa S. Bhat
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
I missed this simple stuff in my email.. :(
Quoted 3 lines
But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU.
Quoted 7 lines
Quoted 5 lines
1057         kobject_uevent(&policy->kobj, KOBJ_ADD);
1058         module_put(cpufreq_driver->owner);
1059         pr_debug("initialization complete\n");
1060
1061 return 0;
That actually looks wrong. And shoud be fixed.
OK
Quoted 41 lines
Quoted 14 lines
So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero.
Something like this:
drivers/cpufreq/cpufreq.c +0 -0
index a4ad733..ecfbc52 100644
@@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
        }
        write_unlock_irqrestore(&cpufreq_driver_lock, flags);
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev_interface(unsigned int cpu, · line 888
Quoted 10 lines
drivers/cpufreq/cpufreq.c +6 -1
 
+       /* Bump up the refcount for this CPU */
+       policy = cpufreq_cpu_get(cpu);
+
        ret = cpufreq_add_dev_symlink(cpu, policy);
-       if (ret)
+       if (ret) {
+               cpufreq_cpu_put(policy);
                goto err_out_kobj_put;
+       }
That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe.
Over that, I think below code in __cpufreq_governor() is also wrong.
	/* we keep one module reference alive for
			each CPU governed by this CPU */
	if ((event != CPUFREQ_GOV_START) || ret)
		module_put(policy->governor->owner);
	if ((event == CPUFREQ_GOV_STOP) && !ret)
		module_put(policy->governor->owner);
The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START..
Quoted 7 lines
But this routine is called for other stuff too:
- INIT/Exit
- LIMITS,
And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched.
That sounds good, but I don't want to make those changes for 3.11 and at the same time I *do* want the reference count issue to go away.

Quoted 8 lines

Quoted 6 lines
So the patch below is the one I'd like to apply for the time being and we can work on more improvements on top of that.
Any objections?
Toralf, please test this patch in the meantime.
works - tested on top of 3.10.4

Great, thanks for the confirmation!

Rafael

Quoted 80 lines

Quoted 77 lines
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the cpufreq driver after a suspend/resume cycle.
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().
On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call cpufreq_cpu_put() in that case.
Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.
Reported-by Toralf Förster <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +10 -9
@@ -1177,14 +1177,11 @@ static int __cpufreq_remove_dev(struct d
 				__func__, cpu_dev->id, cpu);
 	}
 
-	if ((cpus == 1) && (cpufreq_driver->target))
-		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
-
-	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-	cpufreq_cpu_put(data);
-
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
+		if (cpufreq_driver->target)
+			__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+
 		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;
@@ -1205,9 +1202,13 @@ static int __cpufreq_remove_dev(struct d
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
-	} else if (cpufreq_driver->target) {
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	} else {
+		pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+		cpufreq_cpu_put(data);
+		if (cpufreq_driver->target) {
+			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+		}
 	}
 
 	per_cpu(cpufreq_policy_cpu, cpu) = -1;

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Viresh Kumar

maintainer Acked-by

2013-07-30 05:23 UTC #16 lore raw

In reply to Rafael J. Wysocki

Quoted 29 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative for the cpufreq driver after a suspend/resume cycle.
This is not the only bad thing that happens there, however, because kobject_put() should only be called for the policy kobject at this point if the CPU is not the last one for that policy.
Namely, if the given CPU is the last one for that policy, the policy kobject's refcount should be 1 at this point, as set by cpufreq_add_dev_interface(), and only needs to be dropped once for the kobject to go away. This actually happens under the cpu == 1 check, so it need not be done before by cpufreq_cpu_put().
On the other hand, if the given CPU is not the last one for that policy, this means that cpufreq_add_policy_cpu() has been called at least once for that policy and cpufreq_cpu_get() has been called for it too. To balance that cpufreq_cpu_get(), we need to call cpufreq_cpu_put() in that case.
Thus, to fix the described problem and keep the reference counters balanced in both cases, move the cpufreq_cpu_get() call in __cpufreq_remove_dev() to the code path executed only for CPUs that share the policy with other CPUs.

Quoted 5 lines

Reported-by Toralf Förster <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Looks fine.

Acked-by Viresh Kumar <[email protected]>

Rafael J. Wysocki

2013-07-29 11:39 UTC #17 lore raw

In reply to Srivatsa S. Bhat

Quoted 44 lines · Srivatsa S. Bhat

Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().
Quoted 7 lines
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?

Quoted 6 lines

Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.

Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does.

That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* .

Thanks,
Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Srivatsa S. Bhat

Reviewed-by

2013-07-29 11:44 UTC #18 lore raw

Quoted 52 lines · Rafael J. Wysocki

Quoted 44 lines · Srivatsa S. Bhat
Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().
Quoted 7 lines
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?
Quoted 6 lines
Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.

Quoted 6 lines

Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does.
That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* .

Sure, in that case, I agree that your patch is the right fix at this point, since it resolves the immediate problem that we have with the refcounts.

Reviewed-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Rafael J. Wysocki

2013-07-29 12:04 UTC #19 lore raw

Quoted 56 lines · Srivatsa S. Bhat

Quoted 55 lines · Rafael J. Wysocki
Quoted 44 lines · Srivatsa S. Bhat
Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().
Quoted 7 lines
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?
Quoted 6 lines
Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does.

Quoted 8 lines

Quoted 3 lines
That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* .
Sure, in that case, I agree that your patch is the right fix at this point, since it resolves the immediate problem that we have with the refcounts.
Reviewed-by Srivatsa S. Bhat <[email protected]>

Great, thanks!

Does your patchset avoiding the creation/removal of sysfs directories over suspend/resume need to be modified to take this patch into account?

Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Srivatsa S. Bhat

2013-07-29 15:27 UTC #20 lore raw

Quoted 64 lines · Rafael J. Wysocki

Quoted 63 lines · Srivatsa S. Bhat
Quoted 52 lines · Rafael J. Wysocki
Quoted 44 lines · Srivatsa S. Bhat
Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().
Quoted 7 lines
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?
Quoted 6 lines
Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
Quoted 6 lines
Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does.
That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* .
Sure, in that case, I agree that your patch is the right fix at this point, since it resolves the immediate problem that we have with the refcounts.

Quoted 7 lines

Quoted 1 line
Reviewed-by Srivatsa S. Bhat <[email protected]>
Great, thanks!
Does your patchset avoiding the creation/removal of sysfs directories over suspend/resume need to be modified to take this patch into account?

There will be some trivial conflicts, but looking a bit closer at my patchset, I believe it has a subtle refcounting bug, relating to patches 6 and 7.

Patch 6: http://marc.info/?l=linux-kernel&m=137358141628042&w=2
Patch 7: http://marc.info/?l=linux-pm&m=137358124527993&w=2

Patch 6 modifies cpufreq_add_policy_cpu() to return 0 if frozen == true. But by that time, it would have already done a cpufreq_cpu_get().

In patch 7, under __cpufreq_remove_dev(), we avoid dropping refcounts for *any* CPU when frozen == true.

In the general case, this is fine (like myself and Viresh had discussed earlier in that thread), because __cpufreq_add_dev() doesn't increment the refcount in the frozen case.

But we overlooked a rather subtle scenario: say, CPU 'X' went through cpufreq_add_dev in the resume path, and restored its policy structure. Since this was fresh, no refcount was taken. And then CPU 'Y' came along, but it was related to CPU 'X', so it will take this path:

1018         /* Check if this cpu was hot-unplugged earlier and has siblings */
1019         read_lock_irqsave(&cpufreq_driver_lock, flags);
1020         for_each_online_cpu(sibling) {

1021 struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
1022 if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
1023 read_unlock_irqrestore(&cpufreq_driver_lock, flags);
1024 return cpufreq_add_policy_cpu(cpu, sibling, dev,
1025 frozen);
1026 }
1027 }

This time, cp won't be NULL for CPU 'X', because its policy was restored in the previous call. So we end up calling cpufreq_add_policy_cpu() for CPU 'Y'. And that function takes a new refcount, as mentioned above.

So we need to have this extra hunk in cpufreq_add_policy_cpu() to ensure we don't take any additional refcounts during suspend/resume:

drivers/cpufreq/cpufreq.c +4 -1

index c7f59e8..9681c01 100644
@@ -932,8 +932,11 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	}
 
 	/* Don't touch sysfs links during light-weight init */
-	if (frozen)
+	if (frozen) {
+		/* Drop the extra refcount that we took above */
+		cpufreq_cpu_put(policy);
 		return 0;
+	}
 
 	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 	if (ret)

I can respin my entire patchset with this fix included, on top of your patch. Please let me know if you'd prefer some other method for resolving this.

Regards,
Srivatsa S. Bhat

Rafael J. Wysocki

2013-07-29 20:28 UTC #21 lore raw

Quoted 133 lines · Srivatsa S. Bhat

Quoted 64 lines · Rafael J. Wysocki
Quoted 63 lines · Srivatsa S. Bhat
Quoted 52 lines · Rafael J. Wysocki
Quoted 44 lines · Srivatsa S. Bhat
Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · Toralf Förster
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Toralf Förster
it gives at a ThinkPad T420:
tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
acpi_cpufreq 12902 2147483647
That is -1, which indicates some module refcount woes.
yes, ofc.
Quoted 7 lines
Quoted 5 lines
The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
Quoted 1 line
I definitely can't see that with the mainline on my machines.
It is in mainline too.
Does the appended patch help?
Actually, something as simple as this also should help:
---
From: Rafael J. Wysocki <[email protected]>
Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
Quoted 6 lines
Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the driver module refcount, __cpufreq_remove_dev() causes that refcount to become negative after a suspend/resume cycle, for example.
To prevent this from happening make __cpufreq_remove_dev() put the policy kobject only instead of calling cpufreq_cpu_put().
Quoted 7 lines
Having a deeper look at it, though, I see that in fact the whole cpufreq_cpu_put() is needed if the CPU is not the last one for the given policy and is not needed at all otherwise (as described in the changelog of the patch below).
Srivatsa, does this make sense to you?
Quoted 6 lines
Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange.
I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed.
Quoted 6 lines
Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does.
That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* .
Sure, in that case, I agree that your patch is the right fix at this point, since it resolves the immediate problem that we have with the refcounts.
Quoted 7 lines
Quoted 1 line
Reviewed-by Srivatsa S. Bhat <[email protected]>
Great, thanks!
Does your patchset avoiding the creation/removal of sysfs directories over suspend/resume need to be modified to take this patch into account?
There will be some trivial conflicts, but looking a bit closer at my patchset, I believe it has a subtle refcounting bug, relating to patches 6 and 7.
Patch 6: http://marc.info/?l=linux-kernel&m=137358141628042&w=2
Patch 7: http://marc.info/?l=linux-pm&m=137358124527993&w=2
Patch 6 modifies cpufreq_add_policy_cpu() to return 0 if frozen == true. But by that time, it would have already done a cpufreq_cpu_get().
In patch 7, under __cpufreq_remove_dev(), we avoid dropping refcounts for *any* CPU when frozen == true.
In the general case, this is fine (like myself and Viresh had discussed earlier in that thread), because __cpufreq_add_dev() doesn't increment the refcount in the frozen case.
But we overlooked a rather subtle scenario: say, CPU 'X' went through cpufreq_add_dev in the resume path, and restored its policy structure. Since this was fresh, no refcount was taken. And then CPU 'Y' came along, but it was related to CPU 'X', so it will take this path:
1018         /* Check if this cpu was hot-unplugged earlier and has siblings */
1019         read_lock_irqsave(&cpufreq_driver_lock, flags);
1020         for_each_online_cpu(sibling) {
1021 struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
1022 if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
1023 read_unlock_irqrestore(&cpufreq_driver_lock, flags);
1024 return cpufreq_add_policy_cpu(cpu, sibling, dev,
1025 frozen);
1026 }
1027 }
This time, cp won't be NULL for CPU 'X', because its policy was restored in the previous call. So we end up calling cpufreq_add_policy_cpu() for CPU 'Y'. And that function takes a new refcount, as mentioned above.
So we need to have this extra hunk in cpufreq_add_policy_cpu() to ensure we don't take any additional refcounts during suspend/resume:
drivers/cpufreq/cpufreq.c +4 -1
index c7f59e8..9681c01 100644
@@ -932,8 +932,11 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	}
 
 	/* Don't touch sysfs links during light-weight init */
-	if (frozen)
+	if (frozen) {
+		/* Drop the extra refcount that we took above */
+		cpufreq_cpu_put(policy);
 		return 0;
+	}
 
 	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 	if (ret)

Quoted 3 lines

I can respin my entire patchset with this fix included, on top of your patch. Please let me know if you'd prefer some other method for resolving this.

Please respin the patchset with the fix folded in.

My patch is in the bleeding-edge branch of linux-pm.git, so please rebase the new version on that if that's not a problem.

Thanks,
Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.