[PATCH 1/3] thermal: devfreq: Simplify expression

[PATCH 1/3] thermal: devfreq: Simplify expression

[Viresh Kumar]

There is no need to check for IS_ERR() as we are looking for a very
particular error value here. Drop the first check.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/devfreq_cooling.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index ba7a5cd994dc..cf71550b9d00 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -222,7 +222,7 @@ get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
 		return 0;
 
 	opp = dev_pm_opp_find_freq_exact(dev, freq, true);
-	if (IS_ERR(opp) && (PTR_ERR(opp) == -ERANGE))
+	if (PTR_ERR(opp) == -ERANGE)
 		opp = dev_pm_opp_find_freq_exact(dev, freq, false);
 
 	voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
-- 
2.7.1.410.g6faf27b

[PATCH 2/3] thermal: cpu_cooling: Check OPP for errors

[Viresh Kumar]

It is possible for dev_pm_opp_find_freq_exact() to return errors. It was
all fine earlier as dev_pm_opp_get_voltage() had a check within it to
check for invalid OPPs, but dev_pm_opp_put() doesn't have any similar
checks and the callers need to make sure OPP is valid before calling
them.

Also update the later dev_warn_ratelimited() to not print the error
message as the OPP is guaranteed to be valid now.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 85fdbf762fa0..9534540434e2 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -431,13 +431,20 @@ static int get_static_power(struct cpufreq_cooling_device *cpufreq_device,
 
 	opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
 					 true);
+	if (IS_ERR(opp)) {
+		dev_warn_ratelimited(cpufreq_device->cpu_dev,
+				     "Failed to find OPP for frequency %lu: %ld\n",
+				     freq_hz, PTR_ERR(opp));
+		return -EINVAL;
+	}
+
 	voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
 	if (voltage == 0) {
 		dev_warn_ratelimited(cpufreq_device->cpu_dev,
-				     "Failed to get voltage for frequency %lu: %ld\n",
-				     freq_hz, IS_ERR(opp) ? PTR_ERR(opp) : 0);
+				     "Failed to get voltage for frequency %lu\n",
+				     freq_hz);
 		return -EINVAL;
 	}
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH 2/3] thermal: cpu_cooling: Check OPP for errors

[Rafael J. Wysocki]

On Monday, February 06, 2017 03:56:28 PM Viresh Kumar wrote:
> It is possible for dev_pm_opp_find_freq_exact() to return errors. It was
> all fine earlier as dev_pm_opp_get_voltage() had a check within it to
> check for invalid OPPs, but dev_pm_opp_put() doesn't have any similar
> checks and the callers need to make sure OPP is valid before calling
> them.
> 
> Also update the later dev_warn_ratelimited() to not print the error
> message as the OPP is guaranteed to be valid now.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 85fdbf762fa0..9534540434e2 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -431,13 +431,20 @@ static int get_static_power(struct cpufreq_cooling_device *cpufreq_device,
>  
>  	opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
>  					 true);
> +	if (IS_ERR(opp)) {
> +		dev_warn_ratelimited(cpufreq_device->cpu_dev,
> +				     "Failed to find OPP for frequency %lu: %ld\n",
> +				     freq_hz, PTR_ERR(opp));

I'm quite unconvinced about the WARN level of these messages.

They seem to be mostly useful for the people who provide device trees for
platforms (ie. system integrators).  If users see them, there is not much
they can do to fix the problem by themselves and the hardware is OK,
actually.

> +		return -EINVAL;
> +	}
> +
>  	voltage = dev_pm_opp_get_voltage(opp);
>  	dev_pm_opp_put(opp);
>  
>  	if (voltage == 0) {
>  		dev_warn_ratelimited(cpufreq_device->cpu_dev,
> -				     "Failed to get voltage for frequency %lu: %ld\n",
> -				     freq_hz, IS_ERR(opp) ? PTR_ERR(opp) : 0);
> +				     "Failed to get voltage for frequency %lu\n",
> +				     freq_hz);
>  		return -EINVAL;
>  	}
>  

Thanks,
Rafael

Re: [PATCH 2/3] thermal: cpu_cooling: Check OPP for errors

[Viresh Kumar]

On 06-02-17, 13:02, Rafael J. Wysocki wrote:
> On Monday, February 06, 2017 03:56:28 PM Viresh Kumar wrote:
> > It is possible for dev_pm_opp_find_freq_exact() to return errors. It was
> > all fine earlier as dev_pm_opp_get_voltage() had a check within it to
> > check for invalid OPPs, but dev_pm_opp_put() doesn't have any similar
> > checks and the callers need to make sure OPP is valid before calling
> > them.
> > 
> > Also update the later dev_warn_ratelimited() to not print the error
> > message as the OPP is guaranteed to be valid now.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/thermal/cpu_cooling.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index 85fdbf762fa0..9534540434e2 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -431,13 +431,20 @@ static int get_static_power(struct cpufreq_cooling_device *cpufreq_device,
> >  
> >  	opp = dev_pm_opp_find_freq_exact(cpufreq_device->cpu_dev, freq_hz,
> >  					 true);
> > +	if (IS_ERR(opp)) {
> > +		dev_warn_ratelimited(cpufreq_device->cpu_dev,
> > +				     "Failed to find OPP for frequency %lu: %ld\n",
> > +				     freq_hz, PTR_ERR(opp));
> 
> I'm quite unconvinced about the WARN level of these messages.
> 
> They seem to be mostly useful for the people who provide device trees for
> platforms (ie. system integrators).  If users see them, there is not much
> they can do to fix the problem by themselves and the hardware is OK,
> actually.

Sure. I just wanted to keep it consistent within the function.

-- 
viresh

[PATCH 3/3] thermal: devfreq: Check OPP for errors

[Viresh Kumar]

It is possible for dev_pm_opp_find_freq_exact() to return errors. It was
all fine earlier as dev_pm_opp_get_voltage() had a check within it to
check for invalid OPPs, but dev_pm_opp_put() doesn't have any similar
checks and the callers need to make sure OPP is valid before calling
them.

Also update the later dev_warn_ratelimited() to not print the error
message as the OPP is guaranteed to be valid now.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/devfreq_cooling.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index cf71550b9d00..ee013fc02980 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -225,13 +225,20 @@ get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
 	if (PTR_ERR(opp) == -ERANGE)
 		opp = dev_pm_opp_find_freq_exact(dev, freq, false);
 
+	if (IS_ERR(opp)) {
+		dev_warn_ratelimited(dev,
+				     "Failed to find OPP for frequency %lu: %ld\n",
+				     freq, PTR_ERR(opp));
+		return 0;
+	}
+
 	voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
 	dev_pm_opp_put(opp);
 
 	if (voltage == 0) {
 		dev_warn_ratelimited(dev,
-				     "Failed to get voltage for frequency %lu: %ld\n",
-				     freq, IS_ERR(opp) ? PTR_ERR(opp) : 0);
+				     "Failed to get voltage for frequency %lu\n",
+				     freq);
 		return 0;
 	}
 
-- 
2.7.1.410.g6faf27b