[PATCH] Input: tca8418_keypad: hide gcc-4.9 -Wmaybe-uninitialized warning

[PATCH] Input: tca8418_keypad: hide gcc-4.9 -Wmaybe-uninitialized warning

[Arnd Bergmann]

Older versions of gcc warn about the tca8418_irq_handler function
as they can't keep track of the variable assignment inside of the
loop when using the -Wmaybe-unintialized flag:

drivers/input/keyboard/tca8418_keypad.c: In function ‘tca8418_irq_handler’:
drivers/input/keyboard/tca8418_keypad.c:172:9: error: ‘reg’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/input/keyboard/tca8418_keypad.c:165:5: note: ‘reg’ was declared here

This is fixed in gcc-6, but it's possible to rearrange the code
in a way that avoids the warning on older compilers as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/input/keyboard/tca8418_keypad.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 9002298698fc..3048ef3e3e16 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -164,11 +164,18 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
 	int error, col, row;
 	u8 reg, state, code;
 
-	/* Initial read of the key event FIFO */
-	error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
+	do {
+		error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
+		if (error < 0) {
+			dev_err(&keypad_data->client->dev,
+				"unable to read REG_KEY_EVENT_A\n");
+			break;
+		}
+
+		/* Assume that key code 0 signifies empty FIFO */
+		if (reg <= 0)
+			break;
 
-	/* Assume that key code 0 signifies empty FIFO */
-	while (error >= 0 && reg > 0) {
 		state = reg & KEY_EVENT_VALUE;
 		code  = reg & KEY_EVENT_CODE;
 
@@ -184,11 +191,7 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
 
 		/* Read for next loop */
 		error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
-	}
-
-	if (error < 0)
-		dev_err(&keypad_data->client->dev,
-			"unable to read REG_KEY_EVENT_A\n");
+	} while (1);
 
 	input_sync(input);
 }
-- 
2.9.0

Re: [PATCH] Input: tca8418_keypad: hide gcc-4.9 -Wmaybe-uninitialized warning

[Dmitry Torokhov]

Hi Arnd,
On Mon, Oct 24, 2016 at 05:32:08PM +0200, Arnd Bergmann wrote:
> Older versions of gcc warn about the tca8418_irq_handler function
> as they can't keep track of the variable assignment inside of the
> loop when using the -Wmaybe-unintialized flag:
> 
> drivers/input/keyboard/tca8418_keypad.c: In function ‘tca8418_irq_handler’:
> drivers/input/keyboard/tca8418_keypad.c:172:9: error: ‘reg’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/input/keyboard/tca8418_keypad.c:165:5: note: ‘reg’ was declared here
> 
> This is fixed in gcc-6, but it's possible to rearrange the code
> in a way that avoids the warning on older compilers as well.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/input/keyboard/tca8418_keypad.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index 9002298698fc..3048ef3e3e16 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c
> @@ -164,11 +164,18 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
>  	int error, col, row;
>  	u8 reg, state, code;
>  
> -	/* Initial read of the key event FIFO */
> -	error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> +	do {
> +		error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> +		if (error < 0) {
> +			dev_err(&keypad_data->client->dev,
> +				"unable to read REG_KEY_EVENT_A\n");
> +			break;
> +		}
> +
> +		/* Assume that key code 0 signifies empty FIFO */
> +		if (reg <= 0)
> +			break;

I am unconvinced that this rearrangement fixes the issue for all older
GCCs. Can we simply do:

	u8 uninitialized_var(reg);

and be done with it?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: tca8418_keypad: hide gcc-4.9 -Wmaybe-uninitialized warning

[Arnd Bergmann]

On Monday, October 24, 2016 4:45:13 PM CEST Dmitry Torokhov wrote:
> > diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> > index 9002298698fc..3048ef3e3e16 100644
> > --- a/drivers/input/keyboard/tca8418_keypad.c
> > +++ b/drivers/input/keyboard/tca8418_keypad.c
> > @@ -164,11 +164,18 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
> >       int error, col, row;
> >       u8 reg, state, code;
> >  
> > -     /* Initial read of the key event FIFO */
> > -     error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> > +     do {
> > +             error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> > +             if (error < 0) {
> > +                     dev_err(&keypad_data->client->dev,
> > +                             "unable to read REG_KEY_EVENT_A\n");
> > +                     break;
> > +             }
> > +
> > +             /* Assume that key code 0 signifies empty FIFO */
> > +             if (reg <= 0)
> > +                     break;
> 
> I am unconvinced that this rearrangement fixes the issue for all older
> GCCs. Can we simply do:
> 
>         u8 uninitialized_var(reg);
> 
> and be done with it?

Yes, that would work. However:

- avoiding the fake intialization tends to produce better object
  code, as gcc actually knows what's going on
- Linus doesn't like uninitialized_var() and would rather see it
  removed from the kernel
- llvm produces warnings for uninitialized_var()

I have checked gcc-4.6/4.7/4.8/4.9/5.x/6.x, and only gcc-4.9
produces the warning. 4.9 changed the detection for uninitialized
variables significantly, which generally caused fewer false
positives but unfortunately introduced a couple of new ones
like this.

	Arnd

Re: [PATCH] Input: tca8418_keypad: hide gcc-4.9 -Wmaybe-uninitialized warning

[Dmitry Torokhov]

On Tue, Oct 25, 2016 at 11:59:22AM +0200, Arnd Bergmann wrote:
> On Monday, October 24, 2016 4:45:13 PM CEST Dmitry Torokhov wrote:
> > > diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> > > index 9002298698fc..3048ef3e3e16 100644
> > > --- a/drivers/input/keyboard/tca8418_keypad.c
> > > +++ b/drivers/input/keyboard/tca8418_keypad.c
> > > @@ -164,11 +164,18 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
> > >       int error, col, row;
> > >       u8 reg, state, code;
> > >  
> > > -     /* Initial read of the key event FIFO */
> > > -     error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> > > +     do {
> > > +             error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, &reg);
> > > +             if (error < 0) {
> > > +                     dev_err(&keypad_data->client->dev,
> > > +                             "unable to read REG_KEY_EVENT_A\n");
> > > +                     break;
> > > +             }
> > > +
> > > +             /* Assume that key code 0 signifies empty FIFO */
> > > +             if (reg <= 0)
> > > +                     break;
> > 
> > I am unconvinced that this rearrangement fixes the issue for all older
> > GCCs. Can we simply do:
> > 
> >         u8 uninitialized_var(reg);
> > 
> > and be done with it?
> 
> Yes, that would work. However:
> 
> - avoiding the fake intialization tends to produce better object
>   code, as gcc actually knows what's going on
> - Linus doesn't like uninitialized_var() and would rather see it
>   removed from the kernel
> - llvm produces warnings for uninitialized_var()
> 
> I have checked gcc-4.6/4.7/4.8/4.9/5.x/6.x, and only gcc-4.9
> produces the warning. 4.9 changed the detection for uninitialized
> variables significantly, which generally caused fewer false
> positives but unfortunately introduced a couple of new ones
> like this.

OK, I'll apply it then.

Thanks.

-- 
Dmitry