[PATCH] misc: ioc4: fix variable may be used uninitialized warning

[PATCH] misc: ioc4: fix variable may be used uninitialized warning

[Richard Leitner]

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.

Signed-off-by: Richard Leitner <dev@g0hl1n.net>
---
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(-)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 3336ddc..7ec2733 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -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 */
-- 
2.1.3

Re: [PATCH] misc: ioc4: fix variable may be used uninitialized warning

[Arnd Bergmann]

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;
>            ^
> 
> 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 <dev@g0hl1n.net>

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

> ---
> 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(-)
> 
> diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> index 3336ddc..7ec2733 100644
> --- a/drivers/misc/ioc4.c
> +++ b/drivers/misc/ioc4.c
> @@ -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 */
> 

Re: [PATCH] misc: ioc4: fix variable may be used uninitialized warning

[Richard Leitner]

On Mon, 08 Dec 2014 13:42:47 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> 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

Re: [PATCH] misc: ioc4: fix variable may be used uninitialized warning

[Prabhakar Lad]

On Mon, Dec 8, 2014 at 1:18 PM, Richard Leitner <dev@g0hl1n.net> wrote:
> On Mon, 08 Dec 2014 13:42:47 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> 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.
>
Definitely you can go ahead, I am busy with other stuff.

Thanks,
--Prabhakar Lad

[PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate

[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 <dev@g0hl1n.net>
---
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(-)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 3336ddc..8758d03 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -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:
-- 
2.1.3

Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate

[Arnd Bergmann]

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) /
>                 ^
> drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here
>   uint64_t start, end, period;
>            ^
> 
> Signed-off-by: Richard Leitner <dev@g0hl1n.net>

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

> ---
> 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

[Richard Leitner]

On Mon, 08 Dec 2014 16:46:31 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 08 December 2014 16:28:10 Richard Leitner wrote:
> > 
> > Signed-off-by: Richard Leitner <dev@g0hl1n.net>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> 
> 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

[Paul Bolle]

On Mon, 2014-12-08 at 16:28 +0100, 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) /
>                 ^
> drivers/misc/ioc4.c:148:11: note: ‘start’ was declared here
>   uint64_t start, end, period;
>            ^
> 
> Signed-off-by: Richard Leitner <dev@g0hl1n.net>
> ---
> 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(-)
> 
> diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> index 3336ddc..8758d03 100644
> --- a/drivers/misc/ioc4.c
> +++ b/drivers/misc/ioc4.c
> @@ -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:

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
---
>From 6fd7078b622323a2d9b72c9fffad347a1321204d Mon Sep 17 00:00:00 2001
From: Paul Bolle <pebolle@tiscali.nl>
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.

Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/misc/ioc4.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 3336ddca45ac..e8209fa78713 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -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:
-- 
1.9.3

Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate

[Richard Leitner]

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.

On Mon, 08 Dec 2014 16:54:11 +0100
Paul Bolle <pebolle@tiscali.nl> wrote:

> On Mon, 2014-12-08 at 16:28 +0100, 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.
> > 
> 
> 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,

...

> 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?

> +/* 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);

Shouldn't this be IOC4_CALIBRATE_CYCLES instead of IOC4_CALIBRATE_COUNT?
IOC4_CALIBRATE_END was (IOC4_CALIBRATE_CYCLES + IOC4_CALIBRATE_DISCARD).

> +	end = ktime_get_ns();
>  
>  	/* Calculation rearranged to preserve intermediate precision.
>  	 * Logically:

regards,
richard