Skip to contentSkip to conversation

iio/adc/ad7291: Improve unlocking of a mutex in ad7291_read_raw()

PATCH In review SF Markus Elfring 2 participants 2 messages Mar 13, 2018 – Mar 18, 2018

Subsystems (3)
Kernel janitors IIO Analog Devices IIO drivers
lore
Share & tools
lore

SF Markus Elfring

author
#0 lore raw

PATCH 1/1 iio/adc/ad7291: Improve unlocking of a mutex in ad7291_read_raw()

From: Markus Elfring <[email protected]>
Date: Tue, 13 Mar 2018 20:08:40 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored only once in this function implementation.

* Replace three calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/iio/adc/ad7291.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
drivers/iio/adc/ad7291.c +11 -11
index a862b5d8fb4b..b2c3d4c79909 100644
@@ -335,27 +335,27 @@ static int ad7291_read_raw(struct iio_dev *indio_dev,
 			mutex_lock(&chip->state_lock);
 			/* If in autocycle mode drop through */
 			if (chip->command & AD7291_AUTOCYCLE) {
-				mutex_unlock(&chip->state_lock);
-				return -EBUSY;
+				ret = -EBUSY;
+				goto unlock;
 			}
 			/* Enable this channel alone */
 			regval = chip->command & (~AD7291_VOLTAGE_MASK);
 			regval |= BIT(15 - chan->channel);
 			ret = ad7291_i2c_write(chip, AD7291_COMMAND, regval);
-			if (ret < 0) {
-				mutex_unlock(&chip->state_lock);
-				return ret;
-			}
+			if (ret < 0)
+				goto unlock;
+
 			/* Read voltage */
 			ret = i2c_smbus_read_word_swapped(chip->client,
 							  AD7291_VOLTAGE);
-			if (ret < 0) {
-				mutex_unlock(&chip->state_lock);
-				return ret;
-			}
+			if (ret < 0)
+				goto unlock;
+
 			*val = ret & AD7291_VALUE_MASK;
+			ret = IIO_VAL_INT;
+unlock:
 			mutex_unlock(&chip->state_lock);
-			return IIO_VAL_INT;
+			return ret;
 		case IIO_TEMP:
 			/* Assumes tsense bit of command register always set */
 			ret = i2c_smbus_read_word_swapped(chip->client,
Signature · 1 line
2.16.2
maintainer
#1 lore raw
Quoted 45 lines · On Tue, 13 Mar 2018 20:16:27 +0100 SF Markus Elfring…

From: Markus Elfring <[email protected]>
Date: Tue, 13 Mar 2018 20:08:40 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored only once in this function implementation.

* Replace three calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/iio/adc/ad7291.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
drivers/iio/adc/ad7291.c +5 -9
index a862b5d8fb4b..b2c3d4c79909 100644
@@ -335,27 +335,27 @@ static int ad7291_read_raw(struct iio_dev *indio_dev,
 			mutex_lock(&chip->state_lock);
 			/* If in autocycle mode drop through */
 			if (chip->command & AD7291_AUTOCYCLE) {
-				mutex_unlock(&chip->state_lock);
-				return -EBUSY;
+				ret = -EBUSY;
+				goto unlock;
 			}
 			/* Enable this channel alone */
 			regval = chip->command & (~AD7291_VOLTAGE_MASK);
 			regval |= BIT(15 - chan->channel);
 			ret = ad7291_i2c_write(chip, AD7291_COMMAND, regval);
-			if (ret < 0) {
-				mutex_unlock(&chip->state_lock);
-				return ret;
-			}
+			if (ret < 0)
+				goto unlock;
+
 			/* Read voltage */
 			ret = i2c_smbus_read_word_swapped(chip->client,
 							  AD7291_VOLTAGE);
-			if (ret < 0) {
-				mutex_unlock(&chip->state_lock);
-				return ret;

drivers/iio/adc/ad7291.c static int ad7291_read_raw(struct iio_dev *indio_dev, · line 358

Quoted 10 lines
drivers/iio/adc/ad7291.c +6 -2
-			}
+			if (ret < 0)
+				goto unlock;
+
 			*val = ret & AD7291_VALUE_MASK;
+			ret = IIO_VAL_INT;
+unlock:
 			mutex_unlock(&chip->state_lock);
-			return IIO_VAL_INT;
+			return ret;

This localized error handling makes for complex code flow. It does however suggest that perhaps this block of code would be better in it's own function with nice clean internal error handling.

drivers/iio/adc/ad7291.c static int ad7291_read_raw(struct iio_dev *indio_dev, · line 361

Quoted 3 lines
drivers/iio/adc/ad7291.c +0 -0
 		case IIO_TEMP:
 			/* Assumes tsense bit of command register always set */
 			ret = i2c_smbus_read_word_swapped(chip->client,

Markus, please throttle these patches to one at a time. If you send lots of similar patches before anyone has reviewed any of them it wastes both your and reviewers time.

Thanks,

Jonathan