Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Hartmut Knaack <knaack.h@gmx.de>]

Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:
>> -----Original Message-----
>> From: Crt Mori [mailto:cmo@melexis.com]
>> Sent: Thursday, July 30, 2015 10:04 AM
>> To: Hartmut Knaack
>> Cc: linux-iio@vger.kernel.org; Jonathan Cameron; Lars-Peter Clausen; Peter
>> Meerwald; Breana, Tiberiu A
>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling
>>
>> Hi, some comments/questions below:
>>
>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Improve error handling in the following ways:
>>>   - set return value on error condition to an appropriate error code
>>>   - return error code immediately in case of an error (slightly changes
>>>     code structure)
>>>   - pass up real error code
>>>   - add missing error handling
>>>   - return 0 when error have been caught already
>>>   - put device back in active mode after error occurs
>>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> Comments inline.
> 
>>> ---
>>>  drivers/iio/accel/stk8312.c | 60
>>> ++++++++++++++++++++++++++++-----------------
>>>  1 file changed, 38 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>> index 6592be8e6377..4e1dda7e896d 100644
>>> --- a/drivers/iio/accel/stk8312.c
>>> +++ b/drivers/iio/accel/stk8312.c
>>> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>>>                 count--;
>>>         } while (!(ret & 0x80) && count > 0);
>>>
>>> -       if (count == 0)
>>> +       if (count == 0) {
>>> +               ret = -ETIMEDOUT;
>>>                 goto exit_err;
>>> +       }
>> You dont need braces since it is a one word statement or add a dev_err()
>> report.
> 

It's two lines of code to be executed if this condition is true, so
braces are needed. The dev_err() however is located under exit_err,
which is the reason for the goto, instead of directly returning.

> +1
> 
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>         if (ret == 0)
>>> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>>>                 goto exit_err;
>> Why dont we return here as well?
> 
> I'm not sure I follow.
> 
>>>         msleep(150);
>>>
>>> -       return ret;
>>> +       return 0;
>>>
>>>  exit_err:
>>>         dev_err(&client->dev, "failed to initialize sensor\n"); @@
>>> -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data
>> *data, u8 int_mask)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
>> int_mask);
>>> -       if (ret < 0)
>>> +       if (ret < 0) {
>>>                 dev_err(&client->dev, "failed to set interrupts\n");
>>> +               stk8312_set_mode(data, mode);
>>> +               return ret;
>>> +       }
>>>
>>>         return stk8312_set_mode(data, mode);  } @@ -230,7 +235,7 @@
>>> static int stk8312_data_rdy_trigger_set_state(struct iio_trigger
>>> *trig,
>>>
>>>         data->dready_trigger_on = state;
>>>
>>> -       return ret;
>>> +       return 0;
>>>  }
>>>
>>>  static const struct iio_trigger_ops stk8312_trigger_ops = { @@
>>> -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct
>> stk8312_data *data, int rate)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
>>> -       if (ret < 0) {
>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto err_activate;
>>>
>>>         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>> masked_reg);
>>>         if (ret < 0)
>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>> -       else
>>> -               data->sample_rate_idx = rate;
>>> +               goto err_activate;
>>> +
>>> +       data->sample_rate_idx = rate;
>>>
>>>         return stk8312_set_mode(data, mode);
>>> +
>>> +err_activate:
>>> +       dev_err(&client->dev, "failed to set sampling rate\n");
>>> +       stk8312_set_mode(data, mode);
>>> +
>>> +       return ret;
>>>  }
>>>
>>>  static int stk8312_set_range(struct stk8312_data *data, u8 range) @@
>>> -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data
>> *data, u8 range)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
>>> -       if (ret < 0) {
>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto err_activate;
>>>
>>>         masked_reg = ret & (~STK8312_RNG_MASK);
>>>         masked_reg |= range << STK8312_RNG_SHIFT;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>> masked_reg);
>>>         if (ret < 0)
>> So in case of error we print error, but continue without returning error value,
>> but if it is a success then we go to err_activate and return a positive value?
>> Either label is
>> confusing or the whole process.
> 
> I suggest you take a closer look.
> 
>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>> -       else
>>> -               data->range = range;
>>> +               goto err_activate;
>>> +
>>> +       data->range = range;
>>>
>>>         return stk8312_set_mode(data, mode);
>>> +
>>> +err_activate:
>>> +       dev_err(&client->dev, "failed to change sensor range\n");
>>> +       stk8312_set_mode(data, mode);
>>> +
>>> +       return ret;
>>>  }
>>>
>>>  static int stk8312_read_accel(struct stk8312_data *data, u8 address)
>>> @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev
>> *indio_dev,
>>>                 ret = stk8312_set_mode(data, data->mode |
>> STK8312_MODE_ACTIVE);
>>>                 if (ret < 0) {
>>>                         mutex_unlock(&data->lock);
>>> -                       return -EINVAL;
>>> +                       return ret;
>>>                 }
>>>                 ret = stk8312_read_accel(data, chan->address);
>>>                 if (ret < 0) {
>>>                         stk8312_set_mode(data,
>>>                                          data->mode & (~STK8312_MODE_ACTIVE));
>>>                         mutex_unlock(&data->lock);
>>> -                       return -EINVAL;
>>> +                       return ret;
>>>                 }
>>>                 *val = sign_extend32(ret, 7);
>>> -               stk8312_set_mode(data, data->mode &
>> (~STK8312_MODE_ACTIVE));
>>> +               ret = stk8312_set_mode(data,
>>> +                                      data->mode &
>>> + (~STK8312_MODE_ACTIVE));
>>>                 mutex_unlock(&data->lock);
>>> +               if (ret < 0)
>>> +                       return ret;
> 
> Hartmut, I stand by my comment from the first patch version.
> We shouldn't return a read error if we're not able to put the
> sensor back in standby mode after reading the values.
> 

I don't know how often you would usually encounter problems
when setting the device in standby mode. Since it is just
a single I2C byte write, that should never fail, unless there
is some serious host adapter-, bus- or client-failure, which
qualifies to be reported. 
This is handled similarly in the following drivers, as well 
(probably some more):
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/bmc150-accel.c?h=testing#n643
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/kxcjk-1013.c?h=testing#n713
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/gyro/bmg160.c?h=testing#n512
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/light/jsa1212.c?h=testing#n218

>>>                 return IIO_VAL_INT;
>>>         case IIO_CHAN_INFO_SCALE:
>>>                 *val = stk8312_scale_table[data->range - 1][0]; @@
>>> -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>>>                 goto err_buffer_cleanup;
>>>         }
>>>
>>> -       return ret;
>>> +       return 0;
>>>
>>>  err_buffer_cleanup:
>>>         iio_triggered_buffer_cleanup(indio_dev);
>>> --
>>> 2.4.6
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
>