* [PATCH 1/2] clk: pxa mark dummy helper as 'inline'
@ 2016-11-08 14:49 Arnd Bergmann
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-08 14:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: Arnd Bergmann, Michael Turquette, Robert Jarzmik, linux-clk,
linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return
2016-11-08 14:49 [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Arnd Bergmann
@ 2016-11-08 14:49 ` Arnd Bergmann
2016-11-08 18:01 ` Robert Jarzmik
2016-11-09 20:04 ` Stephen Boyd
2016-11-08 17:50 ` [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Robert Jarzmik
2016-11-08 22:42 ` Stephen Boyd
2 siblings, 2 replies; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-08 14:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: Arnd Bergmann, Michael Turquette, Robert Jarzmik, linux-clk,
linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'
2016-11-08 14:49 [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Arnd Bergmann
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
@ 2016-11-08 17:50 ` Robert Jarzmik
2016-11-08 22:42 ` Stephen Boyd
2 siblings, 0 replies; 9+ messages in thread
From: Robert Jarzmik @ 2016-11-08 17:50 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Stephen Boyd, Michael Turquette, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
@ 2016-11-08 18:01 ` Robert Jarzmik
2016-11-08 22:22 ` Arnd Bergmann
2016-11-09 20:04 ` Stephen Boyd
1 sibling, 1 reply; 9+ messages in thread
From: Robert Jarzmik @ 2016-11-08 18:01 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Stephen Boyd, Michael Turquette, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return
2016-11-08 18:01 ` Robert Jarzmik
@ 2016-11-08 22:22 ` Arnd Bergmann
2016-11-09 7:31 ` Robert Jarzmik
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-08 22:22 UTC (permalink / raw)
To: Robert Jarzmik; +Cc: Stephen Boyd, Michael Turquette, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'
2016-11-08 14:49 [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Arnd Bergmann
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
2016-11-08 17:50 ` [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Robert Jarzmik
@ 2016-11-08 22:42 ` Stephen Boyd
2016-11-08 22:50 ` Arnd Bergmann
2 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2016-11-08 22:42 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Michael Turquette, Robert Jarzmik, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: pxa mark dummy helper as 'inline'
2016-11-08 22:42 ` Stephen Boyd
@ 2016-11-08 22:50 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-08 22:50 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, Robert Jarzmik, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return
2016-11-08 22:22 ` Arnd Bergmann
@ 2016-11-09 7:31 ` Robert Jarzmik
0 siblings, 0 replies; 9+ messages in thread
From: Robert Jarzmik @ 2016-11-09 7:31 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Stephen Boyd, Michael Turquette, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
2016-11-08 18:01 ` Robert Jarzmik
@ 2016-11-09 20:04 ` Stephen Boyd
1 sibling, 0 replies; 9+ messages in thread
From: Stephen Boyd @ 2016-11-09 20:04 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Michael Turquette, Robert Jarzmik, linux-clk, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-09 20:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-08 14:49 [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Arnd Bergmann
2016-11-08 14:49 ` [PATCH 2/2] clk: pxa: fix pxa2xx_determine_rate return Arnd Bergmann
2016-11-08 18:01 ` Robert Jarzmik
2016-11-08 22:22 ` Arnd Bergmann
2016-11-09 7:31 ` Robert Jarzmik
2016-11-09 20:04 ` Stephen Boyd
2016-11-08 17:50 ` [PATCH 1/2] clk: pxa mark dummy helper as 'inline' Robert Jarzmik
2016-11-08 22:42 ` Stephen Boyd
2016-11-08 22:50 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).