[PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Thomas Petazzoni]

The of_cpumask_init_opp_table() function will print an error message
(with pr_err) if it cannot find the OPP table for a certain CPU in the
Device Tree.

There are users of the cpufreq-dt driver (which is the one calling
of_cpumask_init_opp_table) that do not have the OPP points defined in
the Device Tree. Instead, such users dynamically create the OPP table
at boot time depending on the system configuration. Such a case is
planned in the cpufreq-dt driver, which on purpose ignores the return
value of of_cpumask_init_opp_table() with the following comment: "OPPs
might be populated at runtime, don't check for error here".

For such platforms, the of_cpumask_init_opp_table() prints a spurious
and confusing error message for each CPU:

[    1.749548] of_cpumask_init_opp_table: couldn't find opp table for cpu:0, -19
[    1.756784] of_cpumask_init_opp_table: couldn't find opp table for cpu:1, -19
[    1.764031] of_cpumask_init_opp_table: couldn't find opp table for cpu:2, -19
[    1.771268] of_cpumask_init_opp_table: couldn't find opp table for cpu:3, -19

This is confusing because everything is working fine, cpufreq works
and it knows the OPP table that was registered at boot time (on
Marvell Armada XP):

$ cat /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
666500 1884
1333000 23333

To avoid this confusion, this patch simply deletes the error message.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/base/power/opp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 7ae7cd9..11206d2 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1489,9 +1489,6 @@ int of_cpumask_init_opp_table(cpumask_var_t cpumask)
 
 		ret = of_init_opp_table(cpu_dev);
 		if (ret) {
-			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
-			       __func__, cpu, ret);
-
 			/* Free all other OPPs */
 			of_cpumask_free_opp_table(cpumask);
 			break;
-- 
2.6.1

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Rafael J. Wysocki]

On Wednesday, October 14, 2015 05:52:23 PM Thomas Petazzoni wrote:
> The of_cpumask_init_opp_table() function will print an error message
> (with pr_err) if it cannot find the OPP table for a certain CPU in the
> Device Tree.
> 
> There are users of the cpufreq-dt driver (which is the one calling
> of_cpumask_init_opp_table) that do not have the OPP points defined in
> the Device Tree. Instead, such users dynamically create the OPP table
> at boot time depending on the system configuration. Such a case is
> planned in the cpufreq-dt driver, which on purpose ignores the return
> value of of_cpumask_init_opp_table() with the following comment: "OPPs
> might be populated at runtime, don't check for error here".
> 
> For such platforms, the of_cpumask_init_opp_table() prints a spurious
> and confusing error message for each CPU:
> 
> [    1.749548] of_cpumask_init_opp_table: couldn't find opp table for cpu:0, -19
> [    1.756784] of_cpumask_init_opp_table: couldn't find opp table for cpu:1, -19
> [    1.764031] of_cpumask_init_opp_table: couldn't find opp table for cpu:2, -19
> [    1.771268] of_cpumask_init_opp_table: couldn't find opp table for cpu:3, -19
> 
> This is confusing because everything is working fine, cpufreq works
> and it knows the OPP table that was registered at boot time (on
> Marvell Armada XP):
> 
> $ cat /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
> 666500 1884
> 1333000 23333
> 
> To avoid this confusion, this patch simply deletes the error message.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

That's only present in linux-next now, right?

Thanks,
Rafael

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Thomas Petazzoni]

Rafael,

On Wed, 14 Oct 2015 23:28:51 +0200, Rafael J. Wysocki wrote:

> > To avoid this confusion, this patch simply deletes the error message.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> That's only present in linux-next now, right?

No, it is already in 4.3-rc since 4.3-rc1. The commit adding the
problematic function is 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2, and
this function gets used in cpufreq-dt starting at commit
2e02d8723edf6599988852a8ade8f83b2f766cb8.

$ git tag --contains 2e02d8723edf6599988852a8ade8f83b2f766cb8
v4.3-rc1
v4.3-rc2
v4.3-rc3
v4.3-rc4
v4.3-rc5

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Rafael J. Wysocki]

On Wednesday, October 14, 2015 11:10:39 PM Thomas Petazzoni wrote:
> Rafael,
> 
> On Wed, 14 Oct 2015 23:28:51 +0200, Rafael J. Wysocki wrote:
> 
> > > To avoid this confusion, this patch simply deletes the error message.
> > > 
> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > 
> > That's only present in linux-next now, right?
> 
> No, it is already in 4.3-rc since 4.3-rc1. The commit adding the
> problematic function is 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2, and
> this function gets used in cpufreq-dt starting at commit
> 2e02d8723edf6599988852a8ade8f83b2f766cb8.
> 
> $ git tag --contains 2e02d8723edf6599988852a8ade8f83b2f766cb8
> v4.3-rc1
> v4.3-rc2
> v4.3-rc3
> v4.3-rc4
> v4.3-rc5

OK, thanks!

It should carry a

Fixes: 2e02d8723edf

tag then, right?

Thanks,
Rafael

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Thomas Petazzoni]

Hello,

On Wed, 14 Oct 2015 23:44:03 +0200, Rafael J. Wysocki wrote:
> On Wednesday, October 14, 2015 11:10:39 PM Thomas Petazzoni wrote:
> > Rafael,
> > 
> > On Wed, 14 Oct 2015 23:28:51 +0200, Rafael J. Wysocki wrote:
> > 
> > > > To avoid this confusion, this patch simply deletes the error message.
> > > > 
> > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > 
> > > That's only present in linux-next now, right?
> > 
> > No, it is already in 4.3-rc since 4.3-rc1. The commit adding the
> > problematic function is 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2, and
> > this function gets used in cpufreq-dt starting at commit
> > 2e02d8723edf6599988852a8ade8f83b2f766cb8.
> > 
> > $ git tag --contains 2e02d8723edf6599988852a8ade8f83b2f766cb8
> > v4.3-rc1
> > v4.3-rc2
> > v4.3-rc3
> > v4.3-rc4
> > v4.3-rc5
> 
> OK, thanks!
> 
> It should carry a
> 
> Fixes: 2e02d8723edf
> 
> tag then, right?

Yes, I wasn't sure if it really is a fix, and if so, which commit it is
fixing. It is really 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2 that
introduces the confusing message, but the message will not appear until
the newly added function gets used in
2e02d8723edf6599988852a8ade8f83b2f766cb8.

But if 2e02d8723edf6599988852a8ade8f83b2f766cb8 is considered to be the
commit actually introducing the regression, then I'll resubmit an
updated version with the Fixes: tag.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Rafael J. Wysocki]

On Wednesday, October 14, 2015 11:29:27 PM Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 14 Oct 2015 23:44:03 +0200, Rafael J. Wysocki wrote:
> > On Wednesday, October 14, 2015 11:10:39 PM Thomas Petazzoni wrote:
> > > Rafael,
> > > 
> > > On Wed, 14 Oct 2015 23:28:51 +0200, Rafael J. Wysocki wrote:
> > > 
> > > > > To avoid this confusion, this patch simply deletes the error message.
> > > > > 
> > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > > 
> > > > That's only present in linux-next now, right?
> > > 
> > > No, it is already in 4.3-rc since 4.3-rc1. The commit adding the
> > > problematic function is 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2, and
> > > this function gets used in cpufreq-dt starting at commit
> > > 2e02d8723edf6599988852a8ade8f83b2f766cb8.
> > > 
> > > $ git tag --contains 2e02d8723edf6599988852a8ade8f83b2f766cb8
> > > v4.3-rc1
> > > v4.3-rc2
> > > v4.3-rc3
> > > v4.3-rc4
> > > v4.3-rc5
> > 
> > OK, thanks!
> > 
> > It should carry a
> > 
> > Fixes: 2e02d8723edf
> > 
> > tag then, right?
> 
> Yes, I wasn't sure if it really is a fix, and if so, which commit it is
> fixing. It is really 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2 that
> introduces the confusing message, but the message will not appear until
> the newly added function gets used in
> 2e02d8723edf6599988852a8ade8f83b2f766cb8.
> 
> But if 2e02d8723edf6599988852a8ade8f83b2f766cb8 is considered to be the
> commit actually introducing the regression, then I'll resubmit an
> updated version with the Fixes: tag.

I've added the tag and applied the patch already.

Thanks,
Rafael

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Thomas Petazzoni]

Rafael,

On Wed, 14 Oct 2015 23:59:45 +0200, Rafael J. Wysocki wrote:

> > Yes, I wasn't sure if it really is a fix, and if so, which commit it is
> > fixing. It is really 8d4d4e98acd68c31435ebb7beea591dbf60b9eb2 that
> > introduces the confusing message, but the message will not appear until
> > the newly added function gets used in
> > 2e02d8723edf6599988852a8ade8f83b2f766cb8.
> > 
> > But if 2e02d8723edf6599988852a8ade8f83b2f766cb8 is considered to be the
> > commit actually introducing the regression, then I'll resubmit an
> > updated version with the Fixes: tag.
> 
> I've added the tag and applied the patch already.

Awesome, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Viresh Kumar]

On 14-10-15, 17:52, Thomas Petazzoni wrote:
>  		ret = of_init_opp_table(cpu_dev);
>  		if (ret) {
> -			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
> -			       __func__, cpu, ret);
> -

It should have be modified to pr_debug() at least.

-- 
viresh

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Thomas Petazzoni]

Viresh, Rafael,

On Thu, 15 Oct 2015 13:05:44 +0530, Viresh Kumar wrote:
> On 14-10-15, 17:52, Thomas Petazzoni wrote:
> >  		ret = of_init_opp_table(cpu_dev);
> >  		if (ret) {
> > -			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
> > -			       __func__, cpu, ret);
> > -
> 
> It should have be modified to pr_debug() at least.

Would be fine as well, but then maybe with a rewording:

	pr_debug("%s: no static opp table found for cpu %d, assuming one will be added at runtime (%d)\n",
		 __func__, cpu, ret);

Do you want me to resend a new version? Or a follow-up patch based on
the latest tree from Rafael now that he has applied the previous version?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Viresh Kumar]

On 15-10-15, 09:42, Thomas Petazzoni wrote:
> Do you want me to resend a new version? Or a follow-up patch based on
> the latest tree from Rafael now that he has applied the previous version?

A follow up patch, without any Fixes tag would be fine.

-- 
viresh

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Rafael J. Wysocki]

On Thursday, October 15, 2015 08:07:38 PM Viresh Kumar wrote:
> On 15-10-15, 09:42, Thomas Petazzoni wrote:
> > Do you want me to resend a new version? Or a follow-up patch based on
> > the latest tree from Rafael now that he has applied the previous version?
> 
> A follow up patch, without any Fixes tag would be fine.

Well, to be honest, I would prefer a replacement.

And since it is going to miss -rc6 anyway, I'll probably simply let it wait
for the v4.4 merge window.

Thanks,
Rafael

Re: [PATCH] PM / OPP: Remove confusing error message in of_cpumask_init_opp_table()

[Viresh Kumar]

On 15-10-15, 23:26, Rafael J. Wysocki wrote:
> Well, to be honest, I would prefer a replacement.

Ahh, I thought you will suggest a separate patch so that you don't
need to rebase your tree. But I don't have any issues with a new
version of the patch.

-- 
viresh