[PATCH 0/2] iio: humidity: hdc100x: update driver locking

[PATCH 0/2] iio: humidity: hdc100x: update driver locking

[Alison Schofield]

This patchset intends to tidy up the driver locking. 

The global data lock needs to to protect writes to the configuration
register and single channel reads which can be of temp or humidity.

First patch moves the config register locking to the config update
function.  This continues to protect updates to heater and integration
times. It puts the lock in one place, right where it needs to occur.

Second patch removes the lock on configuration reads of data stored
in global data.  While the write lock prevents two simultaneous writes
(ie. Heater and int_time) to the register, which could return false
success status, the read doesn't have the same issue.  You get the 
status at the moment, at it's guaranteed for that moment only.
This also aligns with drivers non-locking of other reads of global
data configuration status - ie integration time.
  
I believe these changes stand alone as good cleanups to the locking.
Alas, my ulterior motive is to prep it so that I can cleanly apply
the locks for triggered buffer mode.

 
Alison Schofield (2):
  iio: humidity: hdc100x: move lock on config updates to single function
  iio: humidity: hdc100x: remove lock on heater configuration read

 drivers/iio/humidity/hdc100x.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.1.4

[PATCH 1/2] iio: humidity: hdc100x: move lock on config updates to single function

[Alison Schofield]

Move the config register locking to the config update function.  This
continues to protect updates to heater and integration times. It puts
the lock in one place, right where it needs to occur.

Add the checkpatch required comment on this lock declaration.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
Cc: Daniel Baluta <daniel.baluta@gmail.com>
---
 drivers/iio/humidity/hdc100x.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index a03832a..ad5a12a 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -31,7 +31,7 @@
 
 struct hdc100x_data {
 	struct i2c_client *client;
-	struct mutex lock;
+	struct mutex lock;  /* protect config updates & raw measurements */
 	u16 config;
 
 	/* integration time of the sensor */
@@ -108,10 +108,12 @@ static int hdc100x_update_config(struct hdc100x_data *data, int mask, int val)
 	int tmp = (~mask & data->config) | val;
 	int ret;
 
+	mutex_lock(&data->lock);
 	ret = i2c_smbus_write_word_swapped(data->client,
 						HDC100X_REG_CONFIG, tmp);
 	if (!ret)
 		data->config = tmp;
+	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -234,26 +236,20 @@ static int hdc100x_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct hdc100x_data *data = iio_priv(indio_dev);
-	int ret = -EINVAL;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
 		if (val != 0)
 			return -EINVAL;
 
-		mutex_lock(&data->lock);
-		ret = hdc100x_set_it_time(data, chan->address, val2);
-		mutex_unlock(&data->lock);
-		return ret;
+		return hdc100x_set_it_time(data, chan->address, val2);
+
 	case IIO_CHAN_INFO_RAW:
 		if (chan->type != IIO_CURRENT || val2 != 0)
 			return -EINVAL;
 
-		mutex_lock(&data->lock);
-		ret = hdc100x_update_config(data, HDC100X_REG_CONFIG_HEATER_EN,
+		return hdc100x_update_config(data, HDC100X_REG_CONFIG_HEATER_EN,
 					val ? HDC100X_REG_CONFIG_HEATER_EN : 0);
-		mutex_unlock(&data->lock);
-		return ret;
 	default:
 		return -EINVAL;
 	}
-- 
2.1.4

[PATCH 2/2] iio: humidity: hdc100x: remove lock on heater configuration read

[Alison Schofield]

The lock around the read (from global data) of the heater configuration
status is not needed.  Move the lock in IIO_CHAN_INFO_RAW case to only
protect the get measurement function.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
Cc: Daniel Baluta <daniel.baluta@gmail.com>
---
 drivers/iio/humidity/hdc100x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index ad5a12a..8195b0c 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -193,18 +193,18 @@ static int hdc100x_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW: {
 		int ret;
 
-		mutex_lock(&data->lock);
 		if (chan->type == IIO_CURRENT) {
 			*val = hdc100x_get_heater_status(data);
 			ret = IIO_VAL_INT;
 		} else {
+			mutex_lock(&data->lock);
 			ret = hdc100x_get_measurement(data, chan);
+			mutex_unlock(&data->lock);
 			if (ret >= 0) {
 				*val = ret;
 				ret = IIO_VAL_INT;
 			}
 		}
-		mutex_unlock(&data->lock);
 		return ret;
 	}
 	case IIO_CHAN_INFO_INT_TIME:
-- 
2.1.4

Re: [PATCH 1/2] iio: humidity: hdc100x: move lock on config updates to single function

[Alison Schofield]

On Sun, Jul 10, 2016 at 02:30:01PM -0700, Alison Schofield wrote:
> Move the config register locking to the config update function.  This
> continues to protect updates to heater and integration times. It puts
> the lock in one place, right where it needs to occur.

Since creating this patch, I've learned that it is a design choice in
kernel drivers to keep the locking at a higher level - ie point of entry.
This patch goes against that design, so, I get that it's not do-able.

alisons

> 
> Add the checkpatch required comment on this lock declaration.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> ---
>  drivers/iio/humidity/hdc100x.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> index a03832a..ad5a12a 100644
> --- a/drivers/iio/humidity/hdc100x.c
> +++ b/drivers/iio/humidity/hdc100x.c
> @@ -31,7 +31,7 @@
>  
>  struct hdc100x_data {
>  	struct i2c_client *client;
> -	struct mutex lock;
> +	struct mutex lock;  /* protect config updates & raw measurements */
>  	u16 config;
>  
>  	/* integration time of the sensor */
> @@ -108,10 +108,12 @@ static int hdc100x_update_config(struct hdc100x_data *data, int mask, int val)
>  	int tmp = (~mask & data->config) | val;
>  	int ret;
>  
> +	mutex_lock(&data->lock);
>  	ret = i2c_smbus_write_word_swapped(data->client,
>  						HDC100X_REG_CONFIG, tmp);
>  	if (!ret)
>  		data->config = tmp;
> +	mutex_unlock(&data->lock);
>  
>  	return ret;
>  }
> @@ -234,26 +236,20 @@ static int hdc100x_write_raw(struct iio_dev *indio_dev,
>  			     int val, int val2, long mask)
>  {
>  	struct hdc100x_data *data = iio_priv(indio_dev);
> -	int ret = -EINVAL;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
>  		if (val != 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&data->lock);
> -		ret = hdc100x_set_it_time(data, chan->address, val2);
> -		mutex_unlock(&data->lock);
> -		return ret;
> +		return hdc100x_set_it_time(data, chan->address, val2);
> +
>  	case IIO_CHAN_INFO_RAW:
>  		if (chan->type != IIO_CURRENT || val2 != 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&data->lock);
> -		ret = hdc100x_update_config(data, HDC100X_REG_CONFIG_HEATER_EN,
> +		return hdc100x_update_config(data, HDC100X_REG_CONFIG_HEATER_EN,
>  					val ? HDC100X_REG_CONFIG_HEATER_EN : 0);
> -		mutex_unlock(&data->lock);
> -		return ret;
>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.1.4
>