[PATCH 1/3] cpufreq: Fix crash in cpufreq-stats during suspend/resume

[PATCH 1/3] cpufreq: Fix crash in cpufreq-stats during suspend/resume

[Srivatsa S. Bhat]

Stephen Warren reported that the cpufreq-stats code hits a NULL pointer
dereference during the second attempt to suspend a system. He also
pin-pointed the problem to commit 5302c3f "cpufreq: Perform light-weight
init/teardown during suspend/resume".

That commit actually ensured that the cpufreq-stats table and the
cpufreq-stats sysfs entries are *not* torn down (ie., not freed) during
suspend/resume, which makes it all the more surprising. However, it turns
out that the root-cause is not that we access an already freed memory, but
that the reference to the allocated memory gets moved around and we lose
track of that during resume, leading to the reported crash in a subsequent
suspend attempt.

In the suspend path, during CPU offline, the value of policy->cpu is updated
by choosing one of the surviving CPUs in that policy, as long as there is
atleast one CPU in that policy. And cpufreq_stats_update_policy_cpu() is
invoked to update the reference to the stats structure by assigning it to
the new CPU. However, in the resume path, during CPU online, we end up
assigning a fresh CPU as the policy->cpu, without letting cpufreq-stats
know about this. Thus the reference to the stats structure remains
(incorrectly) associated with the old CPU. So, in a subsequent suspend attempt,
during CPU offline, we end up accessing an incorrect location to get the
stats structure, which eventually leads to the NULL pointer dereference.

Fix this by letting cpufreq-stats know about the update of the policy->cpu
during CPU online in the resume path. (Also, move the update_policy_cpu()
function higher up in the file, so that __cpufreq_add_dev() can invoke
it).

Reported-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
---

 drivers/cpufreq/cpufreq.c |   37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5a64f66..62bdb95 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -947,6 +947,18 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	kfree(policy);
 }
 
+static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
+{
+	policy->last_cpu = policy->cpu;
+	policy->cpu = cpu;
+
+#ifdef CONFIG_CPU_FREQ_TABLE
+	cpufreq_frequency_table_update_policy_cpu(policy);
+#endif
+	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+			CPUFREQ_UPDATE_POLICY_CPU, policy);
+}
+
 static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 			     bool frozen)
 {
@@ -1000,7 +1012,18 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 	if (!policy)
 		goto nomem_out;
 
-	policy->cpu = cpu;
+
+	/*
+	 * In the resume path, since we restore a saved policy, the assignment
+	 * to policy->cpu is like an update of the existing policy, rather than
+	 * the creation of a brand new one. So we need to perform this update
+	 * by invoking update_policy_cpu().
+	 */
+	if (frozen && cpu != policy->cpu)
+		update_policy_cpu(policy, cpu);
+	else
+		policy->cpu = cpu;
+
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
@@ -1092,18 +1115,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	return __cpufreq_add_dev(dev, sif, false);
 }
 
-static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
-{
-	policy->last_cpu = policy->cpu;
-	policy->cpu = cpu;
-
-#ifdef CONFIG_CPU_FREQ_TABLE
-	cpufreq_frequency_table_update_policy_cpu(policy);
-#endif
-	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
-			CPUFREQ_UPDATE_POLICY_CPU, policy);
-}
-
 static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
 					   unsigned int old_cpu, bool frozen)
 {

[PATCH 2/3] cpufreq: Restructure if/else block to avoid unintended behavior

[Srivatsa S. Bhat]

In __cpufreq_remove_dev_prepare(), the code which decides whether to remove
the sysfs link or nominate a new policy cpu, is governed by an if/else block
with a rather complex set of conditionals. Worse, they harbor a subtlety
which leads to certain unintended behavior.

The code looks like this:

        if (cpu != policy->cpu && !frozen) {
                sysfs_remove_link(&dev->kobj, "cpufreq");
        } else if (cpus > 1) {
		new_cpu = cpufreq_nominate_new_policy_cpu(...);
		...
		update_policy_cpu(..., new_cpu);
	}

The original intention was:
If the CPU going offline is not policy->cpu, just remove the link.
On the other hand, if the CPU going offline is the policy->cpu itself,
handover the policy->cpu job to some other surviving CPU in that policy.

But because the 'if' condition also includes the 'frozen' check, now there
are *two* possibilities by which we can enter the 'else' block:

1. cpu == policy->cpu (intended)
2. cpu != policy->cpu && frozen (unintended)

Due to the second (unintended) scenario, we end up spuriously nominating
a CPU as the policy->cpu, even when the existing policy->cpu is alive and
well. This can cause problems further down the line, especially when we end
up nominating the same policy->cpu as the new one (ie., old == new),
because it totally confuses update_policy_cpu().

To avoid this mess, restructure the if/else block to only do what was
originally intended, and thus prevent any unwelcome surprises.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
---

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 62bdb95..247842b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1193,8 +1193,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 		cpumask_clear_cpu(cpu, policy->cpus);
 	unlock_policy_rwsem_write(cpu);
 
-	if (cpu != policy->cpu && !frozen) {
-		sysfs_remove_link(&dev->kobj, "cpufreq");
+	if (cpu != policy->cpu) {
+		if (!frozen)
+			sysfs_remove_link(&dev->kobj, "cpufreq");
 	} else if (cpus > 1) {
 
 		new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);

[PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Srivatsa S. Bhat]

If update_policy_cpu() is invoked with the existing policy->cpu itself
as the new-cpu parameter, then a lot of things can go terribly wrong.

In its present form, update_policy_cpu() always assumes that the new-cpu
is different from policy->cpu and invokes other functions to perform their
respective updates. And those functions implement the actual update like
this:

per_cpu(..., new_cpu) = per_cpu(..., last_cpu);
per_cpu(..., last_cpu) = NULL;

Thus, when new_cpu == last_cpu, the final NULL assignment makes the per-cpu
references vanish into thin air! (memory leak). From there, it leads to more
problems: cpufreq_stats_create_table() now doesn't find the per-cpu reference
and hence tries to create a new sysfs-group; but sysfs already had created
the group earlier, so it complains that it cannot create a duplicate filename.
In short, the repercussions of a rather innocuous invocation of
update_policy_cpu() can turn out to be pretty nasty.

Ideally update_policy_cpu() should handle this situation (new == last)
gracefully, and not lead to such severe problems. So fix it by adding an
appropriate check.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
---

 drivers/cpufreq/cpufreq.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 247842b..d32040c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -949,6 +949,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 
 static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	if (cpu == policy->cpu)
+		return;
+
 	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
 

Re: [PATCH 1/3] cpufreq: Fix crash in cpufreq-stats during suspend/resume

[Rafael J. Wysocki]

On Thursday, September 12, 2013 01:42:59 AM Srivatsa S. Bhat wrote:
> Stephen Warren reported that the cpufreq-stats code hits a NULL pointer
> dereference during the second attempt to suspend a system. He also
> pin-pointed the problem to commit 5302c3f "cpufreq: Perform light-weight
> init/teardown during suspend/resume".
> 
> That commit actually ensured that the cpufreq-stats table and the
> cpufreq-stats sysfs entries are *not* torn down (ie., not freed) during
> suspend/resume, which makes it all the more surprising. However, it turns
> out that the root-cause is not that we access an already freed memory, but
> that the reference to the allocated memory gets moved around and we lose
> track of that during resume, leading to the reported crash in a subsequent
> suspend attempt.
> 
> In the suspend path, during CPU offline, the value of policy->cpu is updated
> by choosing one of the surviving CPUs in that policy, as long as there is
> atleast one CPU in that policy. And cpufreq_stats_update_policy_cpu() is
> invoked to update the reference to the stats structure by assigning it to
> the new CPU. However, in the resume path, during CPU online, we end up
> assigning a fresh CPU as the policy->cpu, without letting cpufreq-stats
> know about this. Thus the reference to the stats structure remains
> (incorrectly) associated with the old CPU. So, in a subsequent suspend attempt,
> during CPU offline, we end up accessing an incorrect location to get the
> stats structure, which eventually leads to the NULL pointer dereference.
> 
> Fix this by letting cpufreq-stats know about the update of the policy->cpu
> during CPU online in the resume path. (Also, move the update_policy_cpu()
> function higher up in the file, so that __cpufreq_add_dev() can invoke
> it).
> 
> Reported-by: Stephen Warren <swarren@nvidia.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>

Applied, thanks Srivatsa!

> ---
> 
>  drivers/cpufreq/cpufreq.c |   37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5a64f66..62bdb95 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -947,6 +947,18 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>  	kfree(policy);
>  }
>  
> +static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
> +{
> +	policy->last_cpu = policy->cpu;
> +	policy->cpu = cpu;
> +
> +#ifdef CONFIG_CPU_FREQ_TABLE
> +	cpufreq_frequency_table_update_policy_cpu(policy);
> +#endif
> +	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> +			CPUFREQ_UPDATE_POLICY_CPU, policy);
> +}
> +
>  static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>  			     bool frozen)
>  {
> @@ -1000,7 +1012,18 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>  	if (!policy)
>  		goto nomem_out;
>  
> -	policy->cpu = cpu;
> +
> +	/*
> +	 * In the resume path, since we restore a saved policy, the assignment
> +	 * to policy->cpu is like an update of the existing policy, rather than
> +	 * the creation of a brand new one. So we need to perform this update
> +	 * by invoking update_policy_cpu().
> +	 */
> +	if (frozen && cpu != policy->cpu)
> +		update_policy_cpu(policy, cpu);
> +	else
> +		policy->cpu = cpu;
> +
>  	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
>  
> @@ -1092,18 +1115,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>  	return __cpufreq_add_dev(dev, sif, false);
>  }
>  
> -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
> -{
> -	policy->last_cpu = policy->cpu;
> -	policy->cpu = cpu;
> -
> -#ifdef CONFIG_CPU_FREQ_TABLE
> -	cpufreq_frequency_table_update_policy_cpu(policy);
> -#endif
> -	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> -			CPUFREQ_UPDATE_POLICY_CPU, policy);
> -}
> -
>  static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>  					   unsigned int old_cpu, bool frozen)
>  {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Viresh Kumar]

On 12 September 2013 01:43, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> If update_policy_cpu() is invoked with the existing policy->cpu itself
> as the new-cpu parameter, then a lot of things can go terribly wrong.
>
> In its present form, update_policy_cpu() always assumes that the new-cpu
> is different from policy->cpu and invokes other functions to perform their
> respective updates. And those functions implement the actual update like
> this:
>
> per_cpu(..., new_cpu) = per_cpu(..., last_cpu);
> per_cpu(..., last_cpu) = NULL;
>
> Thus, when new_cpu == last_cpu, the final NULL assignment makes the per-cpu
> references vanish into thin air! (memory leak). From there, it leads to more
> problems: cpufreq_stats_create_table() now doesn't find the per-cpu reference
> and hence tries to create a new sysfs-group; but sysfs already had created
> the group earlier, so it complains that it cannot create a duplicate filename.
> In short, the repercussions of a rather innocuous invocation of
> update_policy_cpu() can turn out to be pretty nasty.
>
> Ideally update_policy_cpu() should handle this situation (new == last)
> gracefully, and not lead to such severe problems. So fix it by adding an
> appropriate check.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
>
>  drivers/cpufreq/cpufreq.c |    3 +++
>  1 file changed, 3 insertions(+)

We don't need this patch for the reasons that I outlined in other thread.
We should never call this routine when cpu == policy->cpu

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Srivatsa S. Bhat]

On 09/12/2013 11:39 AM, Viresh Kumar wrote:
> On 12 September 2013 01:43, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> If update_policy_cpu() is invoked with the existing policy->cpu itself
>> as the new-cpu parameter, then a lot of things can go terribly wrong.
>>
>> In its present form, update_policy_cpu() always assumes that the new-cpu
>> is different from policy->cpu and invokes other functions to perform their
>> respective updates. And those functions implement the actual update like
>> this:
>>
>> per_cpu(..., new_cpu) = per_cpu(..., last_cpu);
>> per_cpu(..., last_cpu) = NULL;
>>
>> Thus, when new_cpu == last_cpu, the final NULL assignment makes the per-cpu
>> references vanish into thin air! (memory leak). From there, it leads to more
>> problems: cpufreq_stats_create_table() now doesn't find the per-cpu reference
>> and hence tries to create a new sysfs-group; but sysfs already had created
>> the group earlier, so it complains that it cannot create a duplicate filename.
>> In short, the repercussions of a rather innocuous invocation of
>> update_policy_cpu() can turn out to be pretty nasty.
>>
>> Ideally update_policy_cpu() should handle this situation (new == last)
>> gracefully, and not lead to such severe problems. So fix it by adding an
>> appropriate check.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Tested-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>
>>  drivers/cpufreq/cpufreq.c |    3 +++
>>  1 file changed, 3 insertions(+)
> 
> We don't need this patch for the reasons that I outlined in other thread.

Yeah, we don't need it, but its a good-to-have.

> We should never call this routine when cpu == policy->cpu
> 

Yeah, that's the rule. But no harm in having safe-guards to prevent catastrophes
when we have bugs (code which breaks the rule). Its the same as what we
regularly do in code that access pointers:

if (!ptr)
	return;
or

if (ptr)
	ptr->field = value;

Not having these checks crashes the kernel in case of a bug, which is far more
disastrous than surviving the erroneous input and returning an error code
gracefully. Same analogy applies to this patch as well.

That said, indeed currently there is no code in cpufreq that invokes the
function with last == new. So its not like we are masking an existing bug with
this patch. If you like, perhaps we can change this patch to print a warning
when it gets input values with last == new? That prevents disasters and also
warns when some code is buggy. Sounds like a win-win.

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Srivatsa S. Bhat]

On 09/12/2013 12:01 PM, Viresh Kumar wrote:
> On 12 September 2013 11:51, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> That said, indeed currently there is no code in cpufreq that invokes the
>> function with last == new. So its not like we are masking an existing bug with
>> this patch. If you like, perhaps we can change this patch to print a warning
>> when it gets input values with last == new? That prevents disasters and also
>> warns when some code is buggy. Sounds like a win-win.
> 
> Exactly what I thought while I was midway reading your mail :)
> Probably a WARN().. So that we don't miss any other bugs :)
> 

Looking at the rate at which we are bumping into each others thoughts, I think
maybe we should switch from email to IRC ;-) ;-)

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Viresh Kumar]

On 12 September 2013 11:51, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> That said, indeed currently there is no code in cpufreq that invokes the
> function with last == new. So its not like we are masking an existing bug with
> this patch. If you like, perhaps we can change this patch to print a warning
> when it gets input values with last == new? That prevents disasters and also
> warns when some code is buggy. Sounds like a win-win.

Exactly what I thought while I was midway reading your mail :)
Probably a WARN().. So that we don't miss any other bugs :)

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Viresh Kumar]

On 12 September 2013 12:00, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Looking at the rate at which we are bumping into each others thoughts, I think
> maybe we should switch from email to IRC ;-) ;-)

Unbelievable, Even I thought so this morning :)

One more thing that I wanted to say for some other threads..
Your changelogs are simply superb.. The amount of information that you put in
them is fantastic.. I never do it that way and so get caught by Rafael a number
of times :)

Need to learn this from you for sure :)

Btw, I am on Freenode for many Linaro channels.... Where can I find you on
IRC? (Haven't sent this in a private mail as others might also find it useful)

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Srivatsa S. Bhat]

On 09/12/2013 12:14 PM, Viresh Kumar wrote:
> On 12 September 2013 12:00, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Looking at the rate at which we are bumping into each others thoughts, I think
>> maybe we should switch from email to IRC ;-) ;-)
> 
> Unbelievable, Even I thought so this morning :)
> 
> One more thing that I wanted to say for some other threads..
> Your changelogs are simply superb.. The amount of information that you put in
> them is fantastic.. 

Thank you! :-) I'm glad to hear that!

Believe it or not, I spend almost an equal (if not more) amount of time ensuring
that I get the changelog absolutely right, compared to the time I spend actually
writing the code. The reason is that, I have been pleasantly surprised by the
power of the changelog in numerous occasions: the very act of composing a proper 
changelog forces me to think *much* more clearly than when writing code. And it
often gives me the opportunity to rethink the *entire* approach/solution and not
just the implementation, since I need to explain the full context in it, not
just what the code does. And *that* exercise can reveal more complex/subtle bugs
than mere code review can ever do. That's why I put so much emphasis on writing
a perfect changelog :-) [Believe it or not, I have had times when I figured out
that my entire solution was utterly nonsensical when I began writing the changelog,
*after* reviewing and testing the code! ... and of course I had to rework the
entire patch! ;-( ]

And to prevent myself from going overboard with writing the changelog (like making
it way too verbose or convoluted with too much detail), I have a simple mechanism/
handy rule in place:

The changelog should be such that, whoever reads the changelog should feel that
the time he spent reading it was totally worth it. IOW, it should not simply
regurgitate what is already obvious from the code. Instead it should provide
insights into the subtle aspects or tradeoffs relevant to the patch; in short, it
should explain the "_why_ behind the _what_" as clearly and in as few words as
possible :-)

Well, atleast I _try_ to stick to that rule :-)

> I never do it that way and so get caught by Rafael a number
> of times :)
> 

Hehe ;-)

> Need to learn this from you for sure :)
> 
> Btw, I am on Freenode for many Linaro channels.... Where can I find you on
> IRC? (Haven't sent this in a private mail as others might also find it useful)
> 

You can find me on ##kernel on freenode. Or, Rafael has a #pm channel for power
management discussions on tinc.sekrit.org.

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Viresh Kumar]

On 12 September 2013 16:10, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Can you please prepare a patch against Documentation/SubmittingPatches with the
> above paragraph in it?  Seriously.

+1

> There are people who don't really see a reason for writing good patch
> changelogs.

+1 for being one of those people :)

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Rafael J. Wysocki]

On Thursday, September 12, 2013 12:42:29 PM Srivatsa S. Bhat wrote:
> On 09/12/2013 12:14 PM, Viresh Kumar wrote:
> > On 12 September 2013 12:00, Srivatsa S. Bhat
> > <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> >> Looking at the rate at which we are bumping into each others thoughts, I think
> >> maybe we should switch from email to IRC ;-) ;-)
> > 
> > Unbelievable, Even I thought so this morning :)
> > 
> > One more thing that I wanted to say for some other threads..
> > Your changelogs are simply superb.. The amount of information that you put in
> > them is fantastic.. 
> 
> Thank you! :-) I'm glad to hear that!
> 
> Believe it or not, I spend almost an equal (if not more) amount of time ensuring
> that I get the changelog absolutely right, compared to the time I spend actually
> writing the code. The reason is that, I have been pleasantly surprised by the
> power of the changelog in numerous occasions: the very act of composing a proper 
> changelog forces me to think *much* more clearly than when writing code. And it
> often gives me the opportunity to rethink the *entire* approach/solution and not
> just the implementation, since I need to explain the full context in it, not
> just what the code does. And *that* exercise can reveal more complex/subtle bugs
> than mere code review can ever do. That's why I put so much emphasis on writing
> a perfect changelog :-) [Believe it or not, I have had times when I figured out
> that my entire solution was utterly nonsensical when I began writing the changelog,
> *after* reviewing and testing the code! ... and of course I had to rework the
> entire patch! ;-( ]
> 
> And to prevent myself from going overboard with writing the changelog (like making
> it way too verbose or convoluted with too much detail), I have a simple mechanism/
> handy rule in place:
> 
> The changelog should be such that, whoever reads the changelog should feel that
> the time he spent reading it was totally worth it. IOW, it should not simply
> regurgitate what is already obvious from the code. Instead it should provide
> insights into the subtle aspects or tradeoffs relevant to the patch; in short, it
> should explain the "_why_ behind the _what_" as clearly and in as few words as
> possible :-)
> 
> Well, atleast I _try_ to stick to that rule :-)

Can you please prepare a patch against Documentation/SubmittingPatches with the
above paragraph in it?  Seriously.

There are people who don't really see a reason for writing good patch
changelogs.

Thanks,
Rafael

Re: [PATCH 3/3] cpufreq: Prevent problems in update_policy_cpu() if last_cpu == new_cpu

[Srivatsa S. Bhat]

On 09/12/2013 04:10 PM, Rafael J. Wysocki wrote:
> On Thursday, September 12, 2013 12:42:29 PM Srivatsa S. Bhat wrote:
>> On 09/12/2013 12:14 PM, Viresh Kumar wrote:
>>> On 12 September 2013 12:00, Srivatsa S. Bhat
>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>> Looking at the rate at which we are bumping into each others thoughts, I think
>>>> maybe we should switch from email to IRC ;-) ;-)
>>>
>>> Unbelievable, Even I thought so this morning :)
>>>
>>> One more thing that I wanted to say for some other threads..
>>> Your changelogs are simply superb.. The amount of information that you put in
>>> them is fantastic.. 
>>
>> Thank you! :-) I'm glad to hear that!
>>
>> Believe it or not, I spend almost an equal (if not more) amount of time ensuring
>> that I get the changelog absolutely right, compared to the time I spend actually
>> writing the code. The reason is that, I have been pleasantly surprised by the
>> power of the changelog in numerous occasions: the very act of composing a proper 
>> changelog forces me to think *much* more clearly than when writing code. And it
>> often gives me the opportunity to rethink the *entire* approach/solution and not
>> just the implementation, since I need to explain the full context in it, not
>> just what the code does. And *that* exercise can reveal more complex/subtle bugs
>> than mere code review can ever do. That's why I put so much emphasis on writing
>> a perfect changelog :-) [Believe it or not, I have had times when I figured out
>> that my entire solution was utterly nonsensical when I began writing the changelog,
>> *after* reviewing and testing the code! ... and of course I had to rework the
>> entire patch! ;-( ]
>>
>> And to prevent myself from going overboard with writing the changelog (like making
>> it way too verbose or convoluted with too much detail), I have a simple mechanism/
>> handy rule in place:
>>
>> The changelog should be such that, whoever reads the changelog should feel that
>> the time he spent reading it was totally worth it. IOW, it should not simply
>> regurgitate what is already obvious from the code. Instead it should provide
>> insights into the subtle aspects or tradeoffs relevant to the patch; in short, it
>> should explain the "_why_ behind the _what_" as clearly and in as few words as
>> possible :-)
>>
>> Well, atleast I _try_ to stick to that rule :-)
> 
> Can you please prepare a patch against Documentation/SubmittingPatches with the
> above paragraph in it?  Seriously.
> 

Sure, I'd be delighted to :-)

> There are people who don't really see a reason for writing good patch
> changelogs.
> 

Regards,
Srivatsa S. Bhat