| Message ID | 20161108144950.3472058-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Stephen Boyd |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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; }
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(-)