[PATCH 1/2] clk: pxa mark dummy helper as 'inline'

[PATCH 1/2] clk: pxa mark dummy helper as 'inline'

[Arnd Bergmann]

The dummy_clk_set_parent function is marked as 'static' but is
no longer referenced from the pxa25x clk driver after the last use
of the RATE_RO_OPS() macro is gone from this file, causing a
harmless build warning:

In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]

This marks the functon as 'inline', which lets the compiler simply
drop it when it gets referenced.

Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/clk/pxa/clk-pxa.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/pxa/clk-pxa.h b/drivers/clk/pxa/clk-pxa.h
index f60a7bccae4e..58abfa816d53 100644
--- a/drivers/clk/pxa/clk-pxa.h
+++ b/drivers/clk/pxa/clk-pxa.h
@@ -143,7 +143,7 @@ struct pxa2xx_freq {
 	unsigned int clkcfg;
 };
 
-static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
+static inline int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
 {
 	return 0;
 }
-- 
2.9.0

[PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return

[Arnd Bergmann]

The new pxa2xx_determine_rate() function seems lacking in a few
regards:

- For an exact match or no match at all, the rate is uninitialized
  as reported by gcc -Wmaybe-unintialized:
   drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
   drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in this function

- If we get a non-exact match, the req->rate output is never set
  to the actual rate but remains at the requested rate.

- We should not attempt to print a rate if none could be found

This rewrites the logic accordingly.

Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/clk/pxa/clk-pxa.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/pxa/clk-pxa.c b/drivers/clk/pxa/clk-pxa.c
index 50fb9d0ea58d..c423b064c753 100644
--- a/drivers/clk/pxa/clk-pxa.c
+++ b/drivers/clk/pxa/clk-pxa.c
@@ -211,7 +211,7 @@ void pxa2xx_cpll_change(struct pxa2xx_freq *freq,
 int pxa2xx_determine_rate(struct clk_rate_request *req,
 			  struct pxa2xx_freq *freqs, int nb_freqs)
 {
-	int i, closest_below = -1, closest_above = -1, ret = 0;
+	int i, closest_below = -1, closest_above = -1;
 	unsigned long rate;
 
 	for (i = 0; i < nb_freqs; i++) {
@@ -230,18 +230,19 @@ int pxa2xx_determine_rate(struct clk_rate_request *req,
 
 	req->best_parent_hw = NULL;
 
-	if (i < nb_freqs)
-		ret = 0;
-	else if (closest_below >= 0)
+	if (i < nb_freqs) {
+		rate = req->rate;
+	} else if (closest_below >= 0) {
 		rate = freqs[closest_below].cpll;
-	else if (closest_above >= 0)
+	} else if (closest_above >= 0) {
 		rate = freqs[closest_above].cpll;
-	else
-		ret = -EINVAL;
+	} else {
+		pr_debug("%s(rate=%lu) no match\n", __func__, req->rate);
+		return -EINVAL;
+	}
 
-	pr_debug("%s(rate=%lu) rate=%lu: %d\n", __func__, req->rate, rate, ret);
-	if (!rate)
-		req->rate = rate;
+	pr_debug("%s(rate=%lu) rate=%lu\n", __func__, req->rate, rate);
+	req->rate = rate;
 
-	return ret;
+	return 0;
 }
-- 
2.9.0

Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> The new pxa2xx_determine_rate() function seems lacking in a few
> regards:
>
> - For an exact match or no match at all, the rate is uninitialized
>   as reported by gcc -Wmaybe-unintialized:
>    drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
>    drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in
>   this function
Euh I don't think that is true.

For an exact match, rate is assigned the exact value in the first line after the
for(xxx).

For no match at all, there are 2 cases :
 - either a closest match is found, and rate is actually assigned (see below)
 - or no match is found, and it's true rate remains uninitialized, but we have
   ret = -EINVAL

> - If we get a non-exact match, the req->rate output is never set
>   to the actual rate but remains at the requested rate.
Euh no, that doesn't seem correct to me.

If a non-exact match is found, either by closest_below or closest_above, rate is
set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
if/else, req->rate = rate is set as well, so I don't think this part of the
commit message is accurate.

> - We should not attempt to print a rate if none could be found
True.

> This rewrites the logic accordingly.
Unless I'm wrong in the analysis above, I'd rather have just "unsigned long rate
= 0" in the variable declaration, and keep the pr_debug() even if -EINVAL is
returned (it's better for bug tracking, with a rate == 0 in this case for example).

Cheers.

--
Robert

Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return

[Arnd Bergmann]

On Tuesday, November 8, 2016 7:01:57 PM CET Robert Jarzmik wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > The new pxa2xx_determine_rate() function seems lacking in a few
> > regards:
> >
> > - For an exact match or no match at all, the rate is uninitialized
> >   as reported by gcc -Wmaybe-unintialized:
> >    drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
> >    drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in
> >   this function
> Euh I don't think that is true.
> 
> For an exact match, rate is assigned the exact value in the first line after the
> for(xxx).

Right, my mistake.

> For no match at all, there are 2 cases :
>  - either a closest match is found, and rate is actually assigned (see below)
>  - or no match is found, and it's true rate remains uninitialized, but we have
>    ret = -EINVAL

Or a third case that gcc finds but that probably won't happen in practice:

- nb_freqs==0, rate is never initialized

This is what I'm addressing by returning early in the 'else' case.

> > - If we get a non-exact match, the req->rate output is never set
> >   to the actual rate but remains at the requested rate.
> Euh no, that doesn't seem correct to me.
> 
> If a non-exact match is found, either by closest_below or closest_above, rate is
> set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
> if/else, req->rate = rate is set as well, so I don't think this part of the
> commit message is accurate.

It is only set if rate is zero, and that normally is not the case here:

       if (!rate)
               req->rate = rate;


> > - We should not attempt to print a rate if none could be found
> True.
> 
> > This rewrites the logic accordingly.
> Unless I'm wrong in the analysis above, I'd rather have just "unsigned long rate
> = 0" in the variable declaration, and keep the pr_debug() even if -EINVAL is
> returned (it's better for bug tracking, with a rate == 0 in this case for example).

I think it's safer not to initialize the variable, to ensure we get a
warning if the function is changed incorrectly again later.

	Arnd

Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> On Tuesday, November 8, 2016 7:01:57 PM CET Robert Jarzmik wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>> If a non-exact match is found, either by closest_below or closest_above, rate is
>> set (rate = freqs[closest_xxx].cpll). And a couple of lines later after the
>> if/else, req->rate = rate is set as well, so I don't think this part of the
>> commit message is accurate.
>
> It is only set if rate is zero, and that normally is not the case here:
>
>        if (!rate)
>                req->rate = rate;
Ah ok, that's where the bug was lurking, if should have been "if (rate)".

But anyway, after comparing the end result of your code and mine, I find yours
more maintainable, especially the replacement of 'ret = 0'.

So let's proceed, thanks for finding this one out.
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

--
Robert

Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return

[Stephen Boyd]

On 11/08, Arnd Bergmann wrote:
> The new pxa2xx_determine_rate() function seems lacking in a few
> regards:
> 
> - For an exact match or no match at all, the rate is uninitialized
>   as reported by gcc -Wmaybe-unintialized:
>    drivers/clk/pxa/clk-pxa.c: In function 'pxa2xx_determine_rate':
>    drivers/clk/pxa/clk-pxa.c:243:5: error: 'rate' may be used uninitialized in this function
> 
> - If we get a non-exact match, the req->rate output is never set
>   to the actual rate but remains at the requested rate.
> 
> - We should not attempt to print a rate if none could be found
> 
> This rewrites the logic accordingly.
> 
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> The dummy_clk_set_parent function is marked as 'static' but is
> no longer referenced from the pxa25x clk driver after the last use
> of the RATE_RO_OPS() macro is gone from this file, causing a
> harmless build warning:
>
> In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
>
> This marks the functon as 'inline', which lets the compiler simply
> drop it when it gets referenced.
>
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'

[Stephen Boyd]

On 11/08, Arnd Bergmann wrote:
> The dummy_clk_set_parent function is marked as 'static' but is
> no longer referenced from the pxa25x clk driver after the last use
> of the RATE_RO_OPS() macro is gone from this file, causing a
> harmless build warning:
> 
> In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
> 
> This marks the functon as 'inline', which lets the compiler simply
> drop it when it gets referenced.
> 
> Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")

I hope I don't rewrite clk-next history... I need some sort of
magic git pre-commit hook that rewrites fixes tags if the hash
changes.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'

[Arnd Bergmann]

On Tuesday, November 8, 2016 2:42:59 PM CET Stephen Boyd wrote:
> On 11/08, Arnd Bergmann wrote:
> > The dummy_clk_set_parent function is marked as 'static' but is
> > no longer referenced from the pxa25x clk driver after the last use
> > of the RATE_RO_OPS() macro is gone from this file, causing a
> > harmless build warning:
> > 
> > In file included from drivers/clk/pxa/clk-pxa25x.c:24:0:
> > drivers/clk/pxa/clk-pxa.h:146:12: error: 'dummy_clk_set_parent' defined but not used [-Werror=unused-function]
> > 
> > This marks the functon as 'inline', which lets the compiler simply
> > drop it when it gets referenced.
> > 
> > Fixes: 9fe694295098 ("clk: pxa: transfer CPU clock setting from pxa2xx-cpufreq")
> 
> I hope I don't rewrite clk-next history... I need some sort of
> magic git pre-commit hook that rewrites fixes tags if the hash
> changes.
> 

I think if you end up rebasing clk-next, the correct approach
would be to fold simple bugfixes into the patches that introduce
the problems. Obviously you still need a way to find them though.

	Arnd