[PATCH] cpufreq: move policy kobj to policy->cpu at resume

[PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

commit 92c14bd9477a20a83144f08c0ca25b0308bf0730 upstream.

This is only relevant to implementations with multiple clusters, where clusters
have separate clock lines but all CPUs within a cluster share it.

Consider a dual cluster platform with 2 cores per cluster. During suspend we
start hot unplugging CPUs in order 1 to 3. When CPU2 is removed, policy->kobj
would be moved to CPU3 and when CPU3 goes down we wouldn't free policy or its
kobj as we want to retain permissions/values/etc.

Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev().
We will recover the old policy and update policy->cpu from 3 to 2 from
update_policy_cpu().

But the kobj is still tied to CPU3 and isn't moved to CPU2. We wouldn't create a
link for CPU2, but would try that for CPU3 while bringing it online. Which will
report errors as CPU3 already has kobj assigned to it.

This bug got introduced with commit 42f921a, which overlooked this scenario.

To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a
cluster back. Also do a WARN_ON() if kobject_move failed, as we would reach here
only for the first CPU of a non-boot cluster. And we can't recover from this
situation, if kobject_move() fails.

Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume")
Cc: <stable@vger.kernel.org> # 3.13
Cc: <stable@vger.kernel.org> # 3.14
Reported-and-tested-by: Bu Yitian <ybu@qti.qualcomm.com>
Reported-by: Saravana Kannan <skannan@codeaurora.org>
Reviewed-by: Srivatsa S. Bhat <srivatsa@mit.edu>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 199b52b..153f4b9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1089,10 +1089,12 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 	 * the creation of a brand new one. So we need to perform this update
 	 * by invoking update_policy_cpu().
 	 */
-	if (frozen && cpu != policy->cpu)
+	if (frozen && cpu != policy->cpu) {
 		update_policy_cpu(policy, cpu);
-	else
+		WARN_ON(kobject_move(&policy->kobj, &dev->kobj));
+	} else {
 		policy->cpu = cpu;
+	}
 
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
-- 
2.0.0.rc2

[PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

This is only relevant to implementations with multiple clusters, where clusters
have separate clock lines but all CPUs within a cluster share it.

Consider a dual cluster platform with 2 cores per cluster. During suspend we
start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be
moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj.

Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev().
We will recover the old policy and update policy->cpu from 3 to 2 from
update_policy_cpu().

But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create
a link for CPU2, but would try that while bringing CPU3 online. Which will
report errors as CPU3 already has kobj assigned to it.

This bug got introduced with commit 42f921a, which overlooked this scenario.

To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a
cluster back.

Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume")
Cc: Stable <stable@vger.kernel.org> # 3.13+
Reported-by: Bu Yitian <ybu@qti.qualcomm.com>
Reported-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Rafael,

This is for 3.16 release, please take it once Yitian/Saravana test this out.

@Yitian/Saravana: Sorry of overlooking this when both of you reported this
first. I (and Srivatsa as well) was damn sure that this scenario is taken into
account in current code and a close look proved that wrong.

I couldn't test it out, can any of you please see if it fixes things for you?

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 62259d2..6f02485 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1153,10 +1153,12 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	 * the creation of a brand new one. So we need to perform this update
 	 * by invoking update_policy_cpu().
 	 */
-	if (recover_policy && cpu != policy->cpu)
+	if (recover_policy && cpu != policy->cpu) {
 		update_policy_cpu(policy, cpu);
-	else
+		WARN_ON(kobject_move(&policy->kobj, &dev->kobj));
+	} else {
 		policy->cpu = cpu;
+	}
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-- 
2.0.0.rc2

RE: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Bu, Yitian]

Hi Viresh:

I have verified this patch, all the issues I reported disappeared.
Thanks for the quick fix.

Best Regards

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Thursday, July 10, 2014 1:19 PM
> To: rjw@rjwysocki.net
> Cc: linaro-kernel@lists.linaro.org; linux-pm@vger.kernel.org;
> arvind.chauhan@arm.com; srivatsa@mit.edu; skannan@codeaurora.org; Bu,
> Yitian; Viresh Kumar; Stable
> Subject: [PATCH] cpufreq: move policy kobj to policy->cpu at resume
> 
> This is only relevant to implementations with multiple clusters, where clusters
> have separate clock lines but all CPUs within a cluster share it.
> 
> Consider a dual cluster platform with 2 cores per cluster. During suspend we
> start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be
> moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj.
> 
> Now on resume, we will get CPU2 before CPU3 and will call
> __cpufreq_add_dev().
> We will recover the old policy and update policy->cpu from 3 to 2 from
> update_policy_cpu().
> 
> But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create
> a link for CPU2, but would try that while bringing CPU3 online. Which will
> report errors as CPU3 already has kobj assigned to it.
> 
> This bug got introduced with commit 42f921a, which overlooked this scenario.
> 
> To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a
> cluster back.
> 
> Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back
> after resume")
> Cc: Stable <stable@vger.kernel.org> # 3.13+
> Reported-by: Bu Yitian <ybu@qti.qualcomm.com>
> Reported-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Hi Rafael,
> 
> This is for 3.16 release, please take it once Yitian/Saravana test this out.
> 
> @Yitian/Saravana: Sorry of overlooking this when both of you reported this first.
> I (and Srivatsa as well) was damn sure that this scenario is taken into account in
> current code and a close look proved that wrong.
> 
> I couldn't test it out, can any of you please see if it fixes things for you?
> 
>  drivers/cpufreq/cpufreq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index
> 62259d2..6f02485 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1153,10 +1153,12 @@ static int __cpufreq_add_dev(struct device *dev,
> struct subsys_interface *sif)
>  	 * the creation of a brand new one. So we need to perform this update
>  	 * by invoking update_policy_cpu().
>  	 */
> -	if (recover_policy && cpu != policy->cpu)
> +	if (recover_policy && cpu != policy->cpu) {
>  		update_policy_cpu(policy, cpu);
> -	else
> +		WARN_ON(kobject_move(&policy->kobj, &dev->kobj));
> +	} else {
>  		policy->cpu = cpu;
> +	}
> 
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
> 
> --
> 2.0.0.rc2

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

On 10 July 2014 12:38, Bu, Yitian <ybu@qti.qualcomm.com> wrote:
> Hi Viresh:
>
> I have verified this patch, all the issues I reported disappeared.
> Thanks for the quick fix.

Please don't top post. http://en.wikipedia.org/wiki/Posting_style

Rafael would be adding your:

Tested-by: Bu Yitian <ybu@qti.qualcomm.com>

Let us know if you don't want that.

RE: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Bu, Yitian]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Thursday, July 10, 2014 3:10 PM
> To: Bu, Yitian
> Cc: rjw@rjwysocki.net; linaro-kernel@lists.linaro.org; linux-
> pm@vger.kernel.org; arvind.chauhan@arm.com; srivatsa@mit.edu;
> skannan@codeaurora.org; Stable
> Subject: Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume
> 
> On 10 July 2014 12:38, Bu, Yitian <ybu@qti.qualcomm.com> wrote:
> > Hi Viresh:
> >
> > I have verified this patch, all the issues I reported disappeared.
> > Thanks for the quick fix.
> 
> Please don't top post. http://en.wikipedia.org/wiki/Posting_style
> 
> Rafael would be adding your:
> 
> Tested-by: Bu Yitian <ybu@qti.qualcomm.com>
> 
> Let us know if you don't want that.

It is ok, thanks.

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Srivatsa S. Bhat]

On 07/10/2014 10:49 AM, Viresh Kumar wrote:
> This is only relevant to implementations with multiple clusters, where clusters
> have separate clock lines but all CPUs within a cluster share it.
> 
> Consider a dual cluster platform with 2 cores per cluster. During suspend we
> start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be
> moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj.
> 
> Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev().
> We will recover the old policy and update policy->cpu from 3 to 2 from
> update_policy_cpu().
> 
> But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create
> a link for CPU2, but would try that while bringing CPU3 online. Which will
> report errors as CPU3 already has kobj assigned to it.
> 
> This bug got introduced with commit 42f921a, which overlooked this scenario.
> 
> To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a
> cluster back.
> 
> Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume")
> Cc: Stable <stable@vger.kernel.org> # 3.13+
> Reported-by: Bu Yitian <ybu@qti.qualcomm.com>
> Reported-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Looks good to me. But I think it would be better to move the invocation of
kobject_move() to update_policy_cpu() itself, so that update_policy_cpu()
will do all the work involved in updating the policy->cpu, as its name suggests.

With that small nit,

Reviewed-by: Srivatsa S. Bhat <srivatsa@mit.edu>

Regards,
Srivatsa S. Bhat

> Hi Rafael,
> 
> This is for 3.16 release, please take it once Yitian/Saravana test this out.
> 
> @Yitian/Saravana: Sorry of overlooking this when both of you reported this
> first. I (and Srivatsa as well) was damn sure that this scenario is taken into
> account in current code and a close look proved that wrong.
> 
> I couldn't test it out, can any of you please see if it fixes things for you?
> 
>  drivers/cpufreq/cpufreq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 62259d2..6f02485 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1153,10 +1153,12 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>  	 * the creation of a brand new one. So we need to perform this update
>  	 * by invoking update_policy_cpu().
>  	 */
> -	if (recover_policy && cpu != policy->cpu)
> +	if (recover_policy && cpu != policy->cpu) {
>  		update_policy_cpu(policy, cpu);
> -	else
> +		WARN_ON(kobject_move(&policy->kobj, &dev->kobj));
> +	} else {
>  		policy->cpu = cpu;
> +	}
>  
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
>  
> 

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

On 10 July 2014 16:45, Srivatsa S. Bhat <srivatsa@mit.edu> wrote:
> Looks good to me. But I think it would be better to move the invocation of
> kobject_move() to update_policy_cpu() itself, so that update_policy_cpu()
> will do all the work involved in updating the policy->cpu, as its name suggests.

Its called from remove path as well ..

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Srivatsa S. Bhat]

On 07/10/2014 04:50 PM, Viresh Kumar wrote:
> On 10 July 2014 16:45, Srivatsa S. Bhat <srivatsa@mit.edu> wrote:
>> Looks good to me. But I think it would be better to move the invocation of
>> kobject_move() to update_policy_cpu() itself, so that update_policy_cpu()
>> will do all the work involved in updating the policy->cpu, as its name suggests.
> 
> Its called from remove path as well ..
> 

I know.. That's why it makes even more sense to consolidate all the
work into one function. We can restructure cpufreq_nominate_new_policy_cpu()
such that the kobject_move() can be moved to update_policy_cpu().

Regards,
Srivatsa S. Bhat

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

On 10 July 2014 16:52, Srivatsa S. Bhat <srivatsa@mit.edu> wrote:
> I know.. That's why it makes even more sense to consolidate all the
> work into one function. We can restructure cpufreq_nominate_new_policy_cpu()
> such that the kobject_move() can be moved to update_policy_cpu().

Done.

Re: [PATCH] cpufreq: move policy kobj to policy->cpu at resume

[Viresh Kumar]

On 10 July 2014 16:45, Srivatsa S. Bhat <srivatsa@mit.edu> wrote:
> Looks good to me. But I think it would be better to move the invocation of
> kobject_move() to update_policy_cpu() itself, so that update_policy_cpu()
> will do all the work involved in updating the policy->cpu, as its name suggests.
>
> With that small nit,
>
> Reviewed-by: Srivatsa S. Bhat <srivatsa@mit.edu>

Hi Rafael,

I had a chat with Srivatsa about this patch and the V2 version which
people aren't able to test yet.

I proposed that we take this patch as is and hold V2 for some time.
- V2 wouldn't apply cleanly to stable kernels for sure
- V2 isn't yet tested and V1 is.
- Saravana already proposed a patch which would remove most of what
  V2 is adding: http://www.spinics.net/lists/arm-kernel/msg346604.html

Our chats:

<vireshk> srivatsa, http://www.spinics.net/lists/arm-kernel/msg346604.html
<srivatsa> vireshk, thanks for the pointer.. will try to take a look
by tonight..

<vireshk> srivatsa, Because saravana is actually looking to change much of
                the stuff, what about dropping the V2 fix that I sent
yesterday and
                take V1 only for now?
<vireshk> srivatsa, As V1 would apply cleanly over stable kernels as well
<srivatsa> vireshk, sure, no problem... perhaps you can make the code
                 reorganization of the kobject_move as a separate
patch and submit
                 it for merge window instead of -rc
<vireshk> srivatsa, I was even thinking of that as well
<vireshk> Have two patches, only first one for rc and second one for
next release
<vireshk> srivatsa, But the second patch would be overwritten by Saravanna,
                so might not be of any use
<srivatsa> vireshk, ah, i see.. in that case, we can hold off on the
second patch for
                 now.. just v1 should do..


What do you say?