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 <[email protected]>
---
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
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 <[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
> ---
> 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 */
>
On Mon, 08 Dec 2014 13:42:47 +0100
Arnd Bergmann <[email protected]> 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
On Mon, Dec 8, 2014 at 1:18 PM, Richard Leitner <[email protected]> wrote:
> On Mon, 08 Dec 2014 13:42:47 +0100
> Arnd Bergmann <[email protected]> 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
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(-)
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
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 <[email protected]>
Acked-by: Arnd Bergmann <[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.
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
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 <[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(-)
>
> 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 <[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.
Not-yet-signed-off-by: Paul Bolle <[email protected]>
---
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
On Mon, 08 Dec 2014 16:46:31 +0100
Arnd Bergmann <[email protected]> wrote:
> 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
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 <[email protected]> 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