From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from saturn.retrosnub.co.uk ([178.18.118.26]:42058 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932547AbcJYQ5S (ORCPT ); Tue, 25 Oct 2016 12:57:18 -0400 Subject: Re: [PATCH] [v2] staging: iio: ad5933: avoid uninitialized variable in error case To: Arnd Bergmann , Greg Kroah-Hartman References: <20161024152226.2536749-1-arnd@arndb.de> Cc: Lars-Peter Clausen , Michael Hennerich , Hartmut Knaack , Peter Meerwald-Stadler , Alison Schofield , linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org From: Jonathan Cameron Message-ID: <39545e74-af9b-cd31-c525-fba9f4c369b4@kernel.org> Date: Tue, 25 Oct 2016 17:57:15 +0100 MIME-Version: 1.0 In-Reply-To: <20161024152226.2536749-1-arnd@arndb.de> Content-Type: text/plain; charset=windows-1252 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 24/10/16 16:22, Arnd Bergmann wrote: > The ad5933_i2c_read function returns an error code to indicate > whether it could read data or not. However ad5933_work() ignores > this return code and just accesses the data unconditionally, > which gets detected by gcc as a possible bug: > > drivers/staging/iio/impedance-analyzer/ad5933.c: In function 'ad5933_work': > drivers/staging/iio/impedance-analyzer/ad5933.c:649:16: warning: 'status' may be used uninitialized in this function [-Wmaybe-uninitialized] > > This adds minimal error handling so we only evaluate the > data if it was correctly read. > > Link: https://patchwork.kernel.org/patch/8110281/ > Signed-off-by: Arnd Bergmann Looks good to me. Lars? > --- > v2: handle failure for both affected reads instead of just the one > we get a warning for > v2: do not retry on failure to avoid looping in case of permanent > errors > --- > drivers/staging/iio/impedance-analyzer/ad5933.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c > index 5eecf1cb1028..3892a7470410 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -655,6 +655,7 @@ static void ad5933_work(struct work_struct *work) > __be16 buf[2]; > int val[2]; > unsigned char status; > + int ret; > > mutex_lock(&indio_dev->mlock); > if (st->state == AD5933_CTRL_INIT_START_FREQ) { > @@ -662,19 +663,22 @@ static void ad5933_work(struct work_struct *work) > ad5933_cmd(st, AD5933_CTRL_START_SWEEP); > st->state = AD5933_CTRL_START_SWEEP; > schedule_delayed_work(&st->work, st->poll_time_jiffies); > - mutex_unlock(&indio_dev->mlock); > - return; > + goto out; > } > > - ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, &status); > + ret = ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, &status); > + if (ret) > + goto out; > > if (status & AD5933_STAT_DATA_VALID) { > int scan_count = bitmap_weight(indio_dev->active_scan_mask, > indio_dev->masklength); > - ad5933_i2c_read(st->client, > + ret = ad5933_i2c_read(st->client, > test_bit(1, indio_dev->active_scan_mask) ? > AD5933_REG_REAL_DATA : AD5933_REG_IMAG_DATA, > scan_count * 2, (u8 *)buf); > + if (ret) > + goto out; > > if (scan_count == 2) { > val[0] = be16_to_cpu(buf[0]); > @@ -686,8 +690,7 @@ static void ad5933_work(struct work_struct *work) > } else { > /* no data available - try again later */ > schedule_delayed_work(&st->work, st->poll_time_jiffies); > - mutex_unlock(&indio_dev->mlock); > - return; > + goto out; > } > > if (status & AD5933_STAT_SWEEP_DONE) { > @@ -700,7 +703,7 @@ static void ad5933_work(struct work_struct *work) > ad5933_cmd(st, AD5933_CTRL_INC_FREQ); > schedule_delayed_work(&st->work, st->poll_time_jiffies); > } > - > +out: > mutex_unlock(&indio_dev->mlock); > } > >