| Message ID | 20161024153222.2738294-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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, ®); > + do { > + error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, ®); > + 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.
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, ®); > > + do { > > + error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, ®); > > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, ®); > > > + do { > > > + error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, ®); > > > + 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.
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, ®); + do { + error = tca8418_read_byte(keypad_data, REG_KEY_EVENT_A, ®); + 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, ®); - } - - if (error < 0) - dev_err(&keypad_data->client->dev, - "unable to read REG_KEY_EVENT_A\n"); + } while (1); input_sync(input); }
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(-)