misc: ioc4: fix variable may be used uninitialized warning
PATCH 1/1 misc: ioc4: fix variable may be used uninitialized warning
Fix the following build warning:drivers/misc/ioc4.c: In function ‘ioc4_probe’:drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /
^
drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here
uint64_t start, end, period;
^As far as I can tell 'start' cannot really be used uninitialized here, but for the sanity of gcc output explicitly initialize it. Same goes for the 'end' variable.
---
Used gcc version was 4.9.1 (Debian 4.9.1-19)P.S.: I know I'm too late for 3.18 but hopefully it still helps.
--- drivers/misc/ioc4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/misc/ioc4.c +1 -1
@@ -145,7 +145,7 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)union ioc4_int_out int_out;union ioc4_gpcr gpcr;unsigned int state, last_state =1 ; -uint64_t start, end, period; +uint64_t start =0 , end =0 , period;unsigned int count =0 ;/* Enable output */
Signature · 1 line
2.1.3
Arnd Bergmann
maintainerQuoted 10 lines · On Monday 08 December 2014 13:27:20 Richard Leitner wrote:
Fix the following build warning:
drivers/misc/ioc4.c: In function ‘ioc4_probe’:drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /^ drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here uint64_t start, end, period; ^
Quoted 5 lines
As far as I can tell 'start' cannot really be used uninitialized here, but for the sanity of gcc output explicitly initialize it. Same goes for the 'end' variable.
Signed-off-by Richard Leitner <[email protected]>
Prabhakar Lad also sent a patch for this already, which was lacking a good patch description. Your patch does this slightly better but still fails to explain how you concluded it was safe and you don't really explain why you initialize the 'end' variable that we don't even get a warning about.
Arnd
Quoted 21 lines
---
Used gcc version was 4.9.1 (Debian 4.9.1-19)P.S.: I know I'm too late for 3.18 but hopefully it still helps.--- drivers/misc/ioc4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/misc/ioc4.c +1
-1@@ -145,7 +145,7 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)union ioc4_int_out int_out;union ioc4_gpcr gpcr;unsigned int state, last_state =1 ; -uint64_t start, end, period; +uint64_t start =0 , end =0 , period;unsigned int count =0 ;/* Enable output */
Quoted 11 lines · On Mon, 08 Dec 2014 13:42:47 +0100 Arnd Bergmann <[email protected]> wrote:
Quoted 4 lines · On Monday 08 December 2014 13:27:20 Richard Leitner wrote:
As far as I can tell 'start' cannot really be used uninitialized here, but for the sanity of gcc output explicitly initialize it. Same goes for the 'end' variable.
Prabhakar Lad also sent a patch for this already, which was lacking a good patch description. Your patch does this slightly better but still fails to explain how you concluded it was safe and you don't really explain why you initialize the 'end' variable that we don't even get a warning about.
Oops, I'm sorry, I haven't seen the patch and the answers to it.
According to the comments by Andrew a simplification of this code section would be nice. I think it should be possible to do this in a way that the initialize-to-zero won't be needed anymore.
Prabhakar Lad, are you working on this already?
If not I'll take a look at it.
regards,
richard
Quoted 15 lines · Richard Leitner
Quoted 11 lines · On Mon, 08 Dec 2014 13:42:47 +0100 Arnd Bergmann <[email protected]> wrote:
Quoted 4 lines · On Monday 08 December 2014 13:27:20 Richard Leitner wrote:
As far as I can tell 'start' cannot really be used uninitialized here, but for the sanity of gcc output explicitly initialize it. Same goes for the 'end' variable.
Prabhakar Lad also sent a patch for this already, which was lacking a good patch description. Your patch does this slightly better but still fails to explain how you concluded it was safe and you don't really explain why you initialize the 'end' variable that we don't even get a warning about.
Quoted 9 lines
Oops, I'm sorry, I haven't seen the patch and the answers to it.
According to the comments by Andrew a simplification of this code section would be nice. I think it should be possible to do this in a way that the initialize-to-zero won't be needed anymore.
Prabhakar Lad, are you working on this already?
If not I'll take a look at it.
Definitely you can go ahead, I am busy with other stuff.
Thanks,
--Prabhakar Lad
PATCH 1/1 misc: ioc4: simplify wave period measurement in clock_calibrate
The loop for measuring the square wave periods over some cycles is refactored to be more easily readable. This includes avoiding a "by-hand-implemented" for loop with a "real" one and adding some comments.
Furthermore the following compiler warning is avoided by this patch:drivers/misc/ioc4.c: In function ‘ioc4_probe’:drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /
^
drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here
uint64_t start, end, period;
^---
A simplification of this loop was suggested by Andrew Morton [1]. This is my first proposal of such a simplification.
Furthermore I'm not sure if the commit message is sufficient. Please give me also some feedback on it.
If this simplification is not needed only initializing start to ktime_get_ns() would fix the compiler warning too.
[1] https://lkml.org/lkml/2014/12/5/76
--- drivers/misc/ioc4.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
drivers/misc/ioc4.c +16 -15
@@ -144,9 +144,9 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd) {union ioc4_int_out int_out;union ioc4_gpcr gpcr; -unsigned int state, last_state =1 ; +unsigned int state, last_state;uint64_t start, end, period; -unsigned int count =0 ; +unsigned int count;/* Enable output */ gpcr.raw =0 ;
@@ -167,19 +167,20 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)mmiowb ();/* Check square wave period averaged over some number of cycles */ -do { - int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); - state = int_out.fields.int_out; -if (!last_state && state) { - count++; -if (count ==IOC4_CALIBRATE_END ) { - end =ktime_get_ns (); -break ; - }else if (count ==IOC4_CALIBRATE_DISCARD ) - start =ktime_get_ns (); - } - last_state = state; - }while (1 ); + start =ktime_get_ns (); + state =1 ;/* make sure the first read isn't a rising edge */ +for (count =0 ; count <=IOC4_CALIBRATE_END ; count++) { +do {/* wait for a rising edge */ + last_state = state; + int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); + state = int_out.fields.int_out; + }while (last_state || !state); + +/* discard the first few cycles */ +if (count ==IOC4_CALIBRATE_DISCARD ) + start =ktime_get_ns (); + } + end =ktime_get_ns ();/* Calculation rearranged to preserve intermediate precision. * Logically:
Signature · 1 line
2.1.3
Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate
Quoted 10 lines · On Monday 08 December 2014 16:28:10 Richard Leitner wrote:
The loop for measuring the square wave periods over some cycles is refactored to be more easily readable. This includes avoiding a "by-hand-implemented" for loop with a "real" one and adding some comments.
Furthermore the following compiler warning is avoided by this patch:
drivers/misc/ioc4.c: In function ‘ioc4_probe’:drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /
Quoted 6 lines
^ drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here uint64_t start, end, period; ^Signed-off-by Richard Leitner <[email protected]>
Quoted 9 lines
---
A simplification of this loop was suggested by Andrew Morton [1]. This is my first proposal of such a simplification.Furthermore I'm not sure if the commit message is sufficient. Please give me also some feedback on it.
If this simplification is not needed only initializing start to
ktime_get_ns()would fix the compiler warning too.
With the changed loop, do you still get a warning if you remove the extra 'start = ktime_get_ns()' at the start of the loop?
Arnd
Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate
Quoted 10 lines · On Mon, 08 Dec 2014 16:46:31 +0100 Arnd Bergmann <[email protected]> wrote:
Quoted 2 lines · On Monday 08 December 2014 16:28:10 Richard Leitner wrote:
Signed-off-by Richard Leitner <[email protected]>Acked-by Arnd Bergmann <[email protected]>With the changed loop, do you still get a warning if you remove the extra 'start =
ktime_get_ns()' at the start of the loop?
Yes. Although IOC4_CALIBRATE_DISCARD is smaller than IOC4_CALIBRATE_END and there's no more break in the loop, it seems gcc doesn't get it...
regards,
richard
Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate
In reply to Richard LeitnerQuoted 71 lines · Richard Leitner
The loop for measuring the square wave periods over some cycles is refactored to be more easily readable. This includes avoiding a "by-hand-implemented" for loop with a "real" one and adding some comments.
Furthermore the following compiler warning is avoided by this patch:
drivers/misc/ioc4.c: In function ‘ioc4_probe’:drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /^ drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here uint64_t start, end, period; ^Signed-off-by Richard Leitner <[email protected]>---
A simplification of this loop was suggested by Andrew Morton [1]. This is my first proposal of such a simplification.Furthermore I'm not sure if the commit message is sufficient. Please give me also some feedback on it.
If this simplification is not needed only initializing start to
ktime_get_ns()would fix the compiler warning too.[1] https://lkml.org/lkml/2014/12/5/76
--- drivers/misc/ioc4.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)drivers/misc/ioc4.c +9
-15@@ -144,9 +144,9 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd) {union ioc4_int_out int_out;union ioc4_gpcr gpcr; -unsigned int state, last_state =1 ; +unsigned int state, last_state;uint64_t start, end, period; -unsigned int count =0 ; +unsigned int count;/* Enable output */ gpcr.raw =0 ;@@ -167,19 +167,20 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)mmiowb ();/* Check square wave period averaged over some number of cycles */ -do { - int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); - state = int_out.fields.int_out; -if (!last_state && state) { - count++; -if (count ==IOC4_CALIBRATE_END ) { - end =ktime_get_ns (); -break ; - }else if (count ==IOC4_CALIBRATE_DISCARD ) - start =ktime_get_ns (); - } - last_state = state; - }while (1 ); + start =ktime_get_ns (); + state =1 ;/* make sure the first read isn't a rising edge */ +for (count =0 ; count <=IOC4_CALIBRATE_END ; count++) { + do { /* wait for a rising edge */ + last_state = state; + int_out.raw = readl(&idd->idd_misc_regs->int_out.raw); + state = int_out.fields.int_out;
drivers/misc/ioc4.c ioc4_clock_calibrate(struct ioc4_driver_data *idd) · line 186
Quoted 10 lines
drivers/misc/ioc4.c +7
-0+ }while (last_state || !state); + +/* discard the first few cycles */ +if (count ==IOC4_CALIBRATE_DISCARD ) + start =ktime_get_ns (); + } + end =ktime_get_ns ();/* Calculation rearranged to preserve intermediate precision. * Logically:
I've had the patch pasted at the end of this message in my local stack of patches for some time now. It uses another approach: by using a helper function things become much simpler. I _think_ it doesn't introduce any functional changes but still need to a quiet day to make sure that is correct.
Hope this helps,
Paul Bolle
---
Quoted 1 line
From
6fd7078b622323a2d9b72c9fffad347a1321204dMon Sep 17 00:00:00 2001
From: Paul Bolle <[email protected]>
Date: Tue, 19 Aug 2014 09:11:40 +0200
Subject: [PATCH] ioc4: silence GCC warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Building ioc4.o triggers a GCC warning since v3.17-rc1:
drivers/misc/ioc4.c: In function ‘ioc4_probe’:
drivers/misc/ioc4.c:194:16: warning: ‘start’ may be used uninitialized in this function [-Wmaybe-uninitialized]
period = (end - start) /
^
drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here
uint64_t start, end, period;
^
This is a false positive. Apparently the recent change to use ktime_get_ns() makes it harder for GCC to analyze the codeflow.
Adding a small (inline) function that only returns after a certain number of wave cycles have been seen allows to reorder the code a bit. And after reordering it is clearer for both the compiler and the human reader what's going on here. So let's do that.
--- drivers/misc/ioc4.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)
drivers/misc/ioc4.c +21 -18
@@ -122,11 +122,25 @@ ioc4_unregister_submodule(struct ioc4_submodule *is)#define IOC4_CALIBRATE_DEFAULT \ (1000*IOC4_EXTINT_COUNT_DIVISOR/IOC4_CALIBRATE_DEFAULT_MHZ) -#define IOC4_CALIBRATE_END \ - (IOC4_CALIBRATE_CYCLES + IOC4_CALIBRATE_DISCARD) -#define IOC4_INT_OUT_MODE_TOGGLE 0x7/* Toggle INT_OUT every COUNT+1 ticks */ +/* return after count wave cycles have been seen */ +static inline void +ioc4_clock_wave_cycles (struct ioc4_driver_data *idd,unsigned int count) +{ +union ioc4_int_out int_out; +unsigned int state, last_state =1 ; +unsigned int i =0 ; + +do { + int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); + state = int_out.fields.int_out; +if (!last_state && state) + i++; + last_state = state; + }while (i < count); +} +/* Determines external interrupt output clock period of the PCI bus an * IOC4 is attached to. This value can be used to determine the PCI * bus speed.
@@ -144,9 +158,7 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd) {union ioc4_int_out int_out;union ioc4_gpcr gpcr; -unsigned int state, last_state =1 ;uint64_t start, end, period; -unsigned int count =0 ;/* Enable output */ gpcr.raw =0 ;
@@ -167,19 +179,10 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)mmiowb ();/* Check square wave period averaged over some number of cycles */ -do { - int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); - state = int_out.fields.int_out; -if (!last_state && state) { - count++; -if (count ==IOC4_CALIBRATE_END ) { - end =ktime_get_ns (); -break ; - }else if (count ==IOC4_CALIBRATE_DISCARD ) - start =ktime_get_ns (); - } - last_state = state; - }while (1 ); +ioc4_clock_wave_cycles (idd,IOC4_CALIBRATE_DISCARD ); + start =ktime_get_ns (); +ioc4_clock_wave_cycles (idd,IOC4_CALIBRATE_COUNT ); + end =ktime_get_ns ();/* Calculation rearranged to preserve intermediate precision. * Logically:
Signature · 1 line
1.9.3
Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate
Hi Paul,
regarding the general approach ("helper-function" versus loop) a maintainer should decide what suits better here.
IMHO on the one hand the readability of the ioc4_clock_calibrate is better with the "helper-function", on the other hand when using the loop you don't have to look up the function an see the implementation at the first glance.
Some comments on the code are embedded below.
Quoted 14 lines · On Mon, 08 Dec 2014 16:54:11 +0100 Paul Bolle <[email protected]> wrote:
Quoted 5 lines · Richard Leitner
The loop for measuring the square wave periods over some cycles is refactored to be more easily readable. This includes avoiding a "by-hand-implemented" for loop with a "real" one and adding some comments.
I've had the patch pasted at the end of this message in my local stack of patches for some time now. It uses another approach: by using a helper function things become much simpler. I _think_ it doesn't introduce any functional changes but still need to a quiet day to make sure that is correct.
Hope this helps,
...
Quoted 4 lines
Adding a small (inline) function that only returns after a certain number of wave cycles have been seen allows to reorder the code a bit. And after reordering it is clearer for both the compiler and the human reader what's going on here. So let's do that.
Maybe this function name should include "wait" or something similar to make clear it does nothing during the wave cycles?
Quoted 39 lines
drivers/misc/ioc4.c +17
-8+/* return after count wave cycles have been seen */ +static inline void +ioc4_clock_wave_cycles (struct ioc4_driver_data *idd,unsigned int count) +{ +union ioc4_int_out int_out; +unsigned int state, last_state =1 ; +unsigned int i =0 ; + +do { + int_out.raw =readl (&idd->idd_misc_regs->int_out.raw); + state = int_out.fields.int_out; +if (!last_state && state) + i++; + last_state = state; + }while (i < count); +} +/* Determines external interrupt output clock period of the PCI bus an * IOC4 is attached to. This value can be used to determine the PCI * bus speed. @@ -144,9 +158,7 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd) {union ioc4_int_out int_out;union ioc4_gpcr gpcr; -unsigned int state, last_state =1 ;uint64_t start, end, period; -unsigned int count =0 ;/* Enable output */ gpcr.raw =0 ;@@ -167,19 +179,10 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd) mmiowb();/* Check square wave period averaged over some number of cycles */ - do { - int_out.raw = readl(&idd->idd_misc_regs->int_out.raw); - state = int_out.fields.int_out; - if (!last_state && state) { - count++; - if (count == IOC4_CALIBRATE_END) {
drivers/misc/ioc4.c ioc4_clock_calibrate(struct ioc4_driver_data *idd) mmiowb(); · line 183
Quoted 10 lines
drivers/misc/ioc4.c +3
-7- end =ktime_get_ns (); -break ; - }else if (count ==IOC4_CALIBRATE_DISCARD ) - start =ktime_get_ns (); - } - last_state = state; - }while (1 ); +ioc4_clock_wave_cycles (idd,IOC4_CALIBRATE_DISCARD ); + start =ktime_get_ns (); +ioc4_clock_wave_cycles (idd,IOC4_CALIBRATE_COUNT );
Shouldn't this be IOC4_CALIBRATE_CYCLES instead of IOC4_CALIBRATE_COUNT? IOC4_CALIBRATE_END was (IOC4_CALIBRATE_CYCLES + IOC4_CALIBRATE_DISCARD).
drivers/misc/ioc4.c ioc4_clock_calibrate(struct ioc4_driver_data *idd) · line 188
Quoted 4 lines
drivers/misc/ioc4.c +1
-0+ end =ktime_get_ns ();/* Calculation rearranged to preserve intermediate precision. * Logically:
regards,
richard