| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 01 Jan 2015 14:47:14 +0100
From: Hartmut Knaack <knaack.h@....de>
To: Daniel Baluta <daniel.baluta@...el.com>, jic23@...nel.org
CC: lars@...afoo.de, pmeerw@...erw.net, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, srinivas.pandruvada@...ux.intel.com
Subject: Re: [PATCH 03/10] iio: imu: kmx61: Enhance error handling
Daniel Baluta schrieb am 23.12.2014 um 14:22:
> This fixes parts of kmx61 error handling to make code easier to read and to be
> more consistent with IIO coding conventions:
> * prefer as single point for error handling instead of duplicating code
> for each function
> * directly return a value from a case branch instead of breaking
> * fix error message for writing REG_CTRL1
>
> Also, add separate error paths for kmx61_trigger_setup/iio_triggered_buffer_setup
> calls.
Some issues remain in this one, please see inline. Otherwise looking good.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@...el.com>
> ---
> drivers/iio/imu/kmx61.c | 100 +++++++++++++++++++++---------------------------
> 1 file changed, 43 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index e9cbd91..4fc4f63 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -656,11 +656,7 @@ static int kmx61_setup_new_data_interrupt(struct kmx61_data *data,
> return ret;
> }
>
> - ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> - if (ret)
> - return ret;
> -
> - return 0;
> + return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> }
>
> static int kmx61_chip_update_thresholds(struct kmx61_data *data)
> @@ -678,12 +674,10 @@ static int kmx61_chip_update_thresholds(struct kmx61_data *data)
> ret = i2c_smbus_write_byte_data(data->client,
> KMX61_REG_WUF_THRESH,
> data->wake_thresh);
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(&data->client->dev, "Error writing reg_wuf_thresh\n");
> - return ret;
> - }
>
> - return 0;
> + return ret;
> }
>
> static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
> @@ -737,11 +731,7 @@ static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
> return ret;
> }
> mode |= KMX61_ACT_STBY_BIT;
> - ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> - if (ret)
> - return ret;
> -
> - return 0;
> + return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> }
>
> /**
> @@ -924,10 +914,10 @@ static int kmx61_read_event(struct iio_dev *indio_dev,
> switch (info) {
> case IIO_EV_INFO_VALUE:
> *val = data->wake_thresh;
> - break;
> + return IIO_VAL_INT;
> case IIO_EV_INFO_PERIOD:
> *val = data->wake_duration;
> - break;
> + return IIO_VAL_INT;
> default:
> return -EINVAL;
> }
Down below this line is still a return IIO_VAL_INT, which is now obsolete.
> @@ -950,10 +940,10 @@ static int kmx61_write_event(struct iio_dev *indio_dev,
> switch (info) {
> case IIO_EV_INFO_VALUE:
> data->wake_thresh = val;
> - break;
> + return IIO_VAL_INT;
> case IIO_EV_INFO_PERIOD:
> data->wake_duration = val;
> - break;
> + return IIO_VAL_INT;
> default:
> return -EINVAL;
> }
Same obsolete return exists below this line.
> @@ -978,7 +968,7 @@ static int kmx61_write_event_config(struct iio_dev *indio_dev,
> int state)
> {
> struct kmx61_data *data = kmx61_get_data(indio_dev);
> - int ret;
> + int ret = 0;
>
> if (state && data->ev_enable_state)
> return 0;
> @@ -987,27 +977,25 @@ static int kmx61_write_event_config(struct iio_dev *indio_dev,
>
> if (!state && data->motion_trig_on) {
> data->ev_enable_state = 0;
> - mutex_unlock(&data->lock);
> - return 0;
> + goto err_unlock;
> }
>
> ret = kmx61_set_power_state(data, state, KMX61_ACC);
> - if (ret < 0) {
> - mutex_unlock(&data->lock);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_unlock;
>
> ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
> if (ret < 0) {
> kmx61_set_power_state(data, false, KMX61_ACC);
> - mutex_unlock(&data->lock);
> - return ret;
> + goto err_unlock;
> }
>
> data->ev_enable_state = state;
> +
> +err_unlock:
> mutex_unlock(&data->lock);
>
> - return 0;
> + return ret;
> }
>
> static int kmx61_acc_validate_trigger(struct iio_dev *indio_dev,
> @@ -1066,8 +1054,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>
> if (!state && data->ev_enable_state && data->motion_trig_on) {
> data->motion_trig_on = false;
> - mutex_unlock(&data->lock);
> - return 0;
> + goto err_unlock;
> }
>
>
> @@ -1077,10 +1064,8 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
> device = KMX61_MAG;
>
> ret = kmx61_set_power_state(data, state, device);
> - if (ret < 0) {
> - mutex_unlock(&data->lock);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_unlock;
>
> if (data->acc_dready_trig == trig || data->mag_dready_trig == trig)
> ret = kmx61_setup_new_data_interrupt(data, state, device);
> @@ -1088,8 +1073,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
> ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
> if (ret < 0) {
> kmx61_set_power_state(data, false, device);
> - mutex_unlock(&data->lock);
> - return ret;
> + goto err_unlock;
> }
>
> if (data->acc_dready_trig == trig)
> @@ -1098,10 +1082,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
> data->mag_dready_trig_on = state;
> else
> data->motion_trig_on = state;
> -
> +err_unlock:
> mutex_unlock(&data->lock);
>
> - return 0;
> + return ret;
> }
>
> static int kmx61_trig_try_reenable(struct iio_trigger *trig)
> @@ -1207,7 +1191,7 @@ ack_intr:
> ret |= KMX61_REG_CTRL1_BIT_RES;
> ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
> if (ret < 0)
> - dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> + dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>
> ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INL);
> if (ret < 0)
> @@ -1409,15 +1393,17 @@ static int kmx61_probe(struct i2c_client *client,
> data->acc_dready_trig =
> kmx61_trigger_setup(data, data->acc_indio_dev,
> "dready");
> - if (IS_ERR(data->acc_dready_trig))
> - return PTR_ERR(data->acc_dready_trig);
> + if (IS_ERR(data->acc_dready_trig)) {
> + ret = PTR_ERR(data->acc_dready_trig);
Double whitespace.
> + goto err_chip_uninit;
> + }
>
> data->mag_dready_trig =
> kmx61_trigger_setup(data, data->mag_indio_dev,
> "dready");
> if (IS_ERR(data->mag_dready_trig)) {
> ret = PTR_ERR(data->mag_dready_trig);
> - goto err_trigger_unregister;
> + goto err_trigger_unregister_acc_dready;
> }
>
> data->motion_trig =
> @@ -1425,7 +1411,7 @@ static int kmx61_probe(struct i2c_client *client,
> "any-motion");
> if (IS_ERR(data->motion_trig)) {
> ret = PTR_ERR(data->motion_trig);
> - goto err_trigger_unregister;
> + goto err_trigger_unregister_mag_dready;
> }
>
> ret = iio_triggered_buffer_setup(data->acc_indio_dev,
> @@ -1435,7 +1421,7 @@ static int kmx61_probe(struct i2c_client *client,
> if (ret < 0) {
> dev_err(&data->client->dev,
> "Failed to setup acc triggered buffer\n");
> - goto err_trigger_unregister;
> + goto err_trigger_unregister_motion;
> }
>
> ret = iio_triggered_buffer_setup(data->mag_indio_dev,
> @@ -1445,14 +1431,14 @@ static int kmx61_probe(struct i2c_client *client,
> if (ret < 0) {
> dev_err(&data->client->dev,
> "Failed to setup mag triggered buffer\n");
> - goto err_trigger_unregister;
> + goto err_buffer_cleanup_acc;
> }
> }
>
> ret = iio_device_register(data->acc_indio_dev);
> if (ret < 0) {
> dev_err(&client->dev, "Failed to register acc iio device\n");
> - goto err_buffer_cleanup;
> + goto err_buffer_cleanup_mag;
> }
>
> ret = iio_device_register(data->mag_indio_dev);
> @@ -1475,18 +1461,18 @@ err_iio_unregister_mag:
> iio_device_unregister(data->mag_indio_dev);
> err_iio_unregister_acc:
> iio_device_unregister(data->acc_indio_dev);
> -err_buffer_cleanup:
> - if (client->irq >= 0) {
> - iio_triggered_buffer_cleanup(data->acc_indio_dev);
> +err_buffer_cleanup_mag:
> + if (client->irq >= 0)
> iio_triggered_buffer_cleanup(data->mag_indio_dev);
> - }
> -err_trigger_unregister:
> - if (data->acc_dready_trig)
> - iio_trigger_unregister(data->acc_dready_trig);
> - if (data->mag_dready_trig)
> - iio_trigger_unregister(data->mag_dready_trig);
> - if (data->motion_trig)
> - iio_trigger_unregister(data->motion_trig);
> +err_buffer_cleanup_acc:
> + if (client->irq >= 0)
> + iio_triggered_buffer_cleanup(data->acc_indio_dev);
> +err_trigger_unregister_motion:
> + iio_trigger_unregister(data->motion_trig);
> +err_trigger_unregister_mag_dready:
> + iio_trigger_unregister(data->mag_dready_trig);
> +err_trigger_unregister_acc_dready:
> + iio_trigger_unregister(data->acc_dready_trig);
> err_chip_uninit:
> kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> return ret;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists