[PATCH] staging: iio: ad5933: Protect DIRECT mode using claim/release helpers

[PATCH] staging: iio: ad5933: Protect DIRECT mode using claim/release helpers

[Narcisa Ana Maria Vasile]

This device operates in DIRECT_MODE and BUFFER_HARDWARE mode.
Replace usages of iio_dev->mlock with iio_device_{claim|release}_direct_mode()
helper functions to guarantee DIRECT mode and consequently protect
BUFFER mode too.

Add and use a device private lock to protect against conflicting access of the
state data.
This helps with IIO subsystem redefining iio_dev->mlock to be used by
the IIO core only for protecting device operating mode changes.
ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.

Remove lock from ad5933_work() because buffer mode should be enabled
when we reach this, and claiming DIRECT mode in all the other places
should protect it.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/iio/impedance-analyzer/ad5933.c | 50 ++++++++++++++-----------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 297665d..3d539ee 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -98,6 +98,7 @@ struct ad5933_state {
 	struct i2c_client		*client;
 	struct regulator		*reg;
 	struct delayed_work		work;
+	struct mutex			lock; /* Protect sensor state */
 	unsigned long			mclk_hz;
 	unsigned char			ctrl_hb;
 	unsigned char			ctrl_lb;
@@ -306,9 +307,11 @@ static ssize_t ad5933_show_frequency(struct device *dev,
 		u8 d8[4];
 	} dat;
 
-	mutex_lock(&indio_dev->mlock);
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 	ret = ad5933_i2c_read(st->client, this_attr->address, 3, &dat.d8[1]);
-	mutex_unlock(&indio_dev->mlock);
+	iio_device_release_direct_mode(indio_dev);
 	if (ret < 0)
 		return ret;
 
@@ -338,9 +341,11 @@ static ssize_t ad5933_store_frequency(struct device *dev,
 	if (val > AD5933_MAX_OUTPUT_FREQ_Hz)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 	ret = ad5933_set_freq(st, this_attr->address, val);
-	mutex_unlock(&indio_dev->mlock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return ret ? ret : len;
 }
@@ -364,7 +369,7 @@ static ssize_t ad5933_show(struct device *dev,
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	int ret = 0, len = 0;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD5933_OUT_RANGE:
 		len = sprintf(buf, "%u\n",
@@ -393,7 +398,7 @@ static ssize_t ad5933_show(struct device *dev,
 		ret = -EINVAL;
 	}
 
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 	return ret ? ret : len;
 }
 
@@ -415,7 +420,10 @@ static ssize_t ad5933_store(struct device *dev,
 			return ret;
 	}
 
-	mutex_lock(&indio_dev->mlock);
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+	mutex_lock(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD5933_OUT_RANGE:
 		ret = -EINVAL;
@@ -465,7 +473,8 @@ static ssize_t ad5933_store(struct device *dev,
 		ret = -EINVAL;
 	}
 
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
+	iio_device_release_direct_mode(indio_dev);
 	return ret ? ret : len;
 }
 
@@ -532,11 +541,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev)) {
-			ret = -EBUSY;
-			goto out;
-		}
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
 		ret = ad5933_cmd(st, AD5933_CTRL_MEASURE_TEMP);
 		if (ret < 0)
 			goto out;
@@ -549,7 +556,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
 				      2, (u8 *)&dat);
 		if (ret < 0)
 			goto out;
-		mutex_unlock(&indio_dev->mlock);
+		iio_device_release_direct_mode(indio_dev);
 		*val = sign_extend32(be16_to_cpu(dat), 13);
 
 		return IIO_VAL_INT;
@@ -561,7 +568,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
 
 	return -EINVAL;
 out:
-	mutex_unlock(&indio_dev->mlock);
+	iio_device_release_direct_mode(indio_dev);
 	return ret;
 }
 
@@ -657,18 +664,17 @@ static void ad5933_work(struct work_struct *work)
 	unsigned char status;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
 	if (st->state == AD5933_CTRL_INIT_START_FREQ) {
 		/* start sweep */
 		ad5933_cmd(st, AD5933_CTRL_START_SWEEP);
 		st->state = AD5933_CTRL_START_SWEEP;
 		schedule_delayed_work(&st->work, st->poll_time_jiffies);
-		goto out;
+		return;
 	}
 
 	ret = ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, &status);
 	if (ret)
-		goto out;
+		return;
 
 	if (status & AD5933_STAT_DATA_VALID) {
 		int scan_count = bitmap_weight(indio_dev->active_scan_mask,
@@ -678,7 +684,7 @@ static void ad5933_work(struct work_struct *work)
 				AD5933_REG_REAL_DATA : AD5933_REG_IMAG_DATA,
 				scan_count * 2, (u8 *)buf);
 		if (ret)
-			goto out;
+			return;
 
 		if (scan_count == 2) {
 			val[0] = be16_to_cpu(buf[0]);
@@ -690,7 +696,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);
-		goto out;
+		return;
 	}
 
 	if (status & AD5933_STAT_SWEEP_DONE) {
@@ -703,8 +709,6 @@ 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);
 }
 
 static int ad5933_probe(struct i2c_client *client,
@@ -723,6 +727,8 @@ static int ad5933_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, indio_dev);
 	st->client = client;
 
+	mutex_init(&st->lock);
+
 	if (!pdata)
 		pdata = &ad5933_default_pdata;
 
-- 
1.9.1

Re: [PATCH] staging: iio: ad5933: Protect DIRECT mode using claim/release helpers

[Jonathan Cameron]

On 23/03/17 17:36, Narcisa Ana Maria Vasile wrote:
> This device operates in DIRECT_MODE and BUFFER_HARDWARE mode.
> Replace usages of iio_dev->mlock with iio_device_{claim|release}_direct_mode()
> helper functions to guarantee DIRECT mode and consequently protect
> BUFFER mode too.
> 
> Add and use a device private lock to protect against conflicting access of the
> state data.
> This helps with IIO subsystem redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> Remove lock from ad5933_work() because buffer mode should be enabled
> when we reach this, and claiming DIRECT mode in all the other places
> should protect it.
Well presented. I was wondering about the logic behind that...
> 
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
This is certainly a fiddly one so I'd like some more eyes on it.

Lars - could you take a look? (he gets all the analog devices stuff ;)
> ---
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 50 ++++++++++++++-----------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 297665d..3d539ee 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -98,6 +98,7 @@ struct ad5933_state {
>  	struct i2c_client		*client;
>  	struct regulator		*reg;
>  	struct delayed_work		work;
> +	struct mutex			lock; /* Protect sensor state */
>  	unsigned long			mclk_hz;
>  	unsigned char			ctrl_hb;
>  	unsigned char			ctrl_lb;
> @@ -306,9 +307,11 @@ static ssize_t ad5933_show_frequency(struct device *dev,
>  		u8 d8[4];
>  	} dat;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
>  	ret = ad5933_i2c_read(st->client, this_attr->address, 3, &dat.d8[1]);
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -338,9 +341,11 @@ static ssize_t ad5933_store_frequency(struct device *dev,
>  	if (val > AD5933_MAX_OUTPUT_FREQ_Hz)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
>  	ret = ad5933_set_freq(st, this_attr->address, val);
This is the one case that standas out where we aren't taking the new lock..
It is setting a few internal state variables (st->freq_start and st->freq_inc)
but this is safe because those aren't actually read anywhere..
Presumably they were going to be cached, but the original author decided
against at some point later...
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  
>  	return ret ? ret : len;
>  }
> @@ -364,7 +369,7 @@ static ssize_t ad5933_show(struct device *dev,
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  	int ret = 0, len = 0;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
>  	case AD5933_OUT_RANGE:
>  		len = sprintf(buf, "%u\n",
> @@ -393,7 +398,7 @@ static ssize_t ad5933_show(struct device *dev,
>  		ret = -EINVAL;
>  	}
>  
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  	return ret ? ret : len;
>  }
>  
> @@ -415,7 +420,10 @@ static ssize_t ad5933_store(struct device *dev,
>  			return ret;
>  	}
>  
> -	mutex_lock(&indio_dev->mlock);
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
>  	case AD5933_OUT_RANGE:
>  		ret = -EINVAL;
> @@ -465,7 +473,8 @@ static ssize_t ad5933_store(struct device *dev,
>  		ret = -EINVAL;
>  	}
>  
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
> +	iio_device_release_direct_mode(indio_dev);
>  	return ret ? ret : len;
>  }
>  
> @@ -532,11 +541,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&indio_dev->mlock);
> -		if (iio_buffer_enabled(indio_dev)) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  		ret = ad5933_cmd(st, AD5933_CTRL_MEASURE_TEMP);
>  		if (ret < 0)
>  			goto out;
> @@ -549,7 +556,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  				      2, (u8 *)&dat);
>  		if (ret < 0)
>  			goto out;
> -		mutex_unlock(&indio_dev->mlock);
> +		iio_device_release_direct_mode(indio_dev);
>  		*val = sign_extend32(be16_to_cpu(dat), 13);
>  
>  		return IIO_VAL_INT;
> @@ -561,7 +568,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  
>  	return -EINVAL;
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  	return ret;
>  }
>  
> @@ -657,18 +664,17 @@ static void ad5933_work(struct work_struct *work)
>  	unsigned char status;
>  	int ret;
>  
> -	mutex_lock(&indio_dev->mlock);
>  	if (st->state == AD5933_CTRL_INIT_START_FREQ) {
>  		/* start sweep */
>  		ad5933_cmd(st, AD5933_CTRL_START_SWEEP);
>  		st->state = AD5933_CTRL_START_SWEEP;
>  		schedule_delayed_work(&st->work, st->poll_time_jiffies);
> -		goto out;
> +		return;
>  	}
>  
>  	ret = ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, &status);
>  	if (ret)
> -		goto out;
> +		return;
>  
>  	if (status & AD5933_STAT_DATA_VALID) {
>  		int scan_count = bitmap_weight(indio_dev->active_scan_mask,
> @@ -678,7 +684,7 @@ static void ad5933_work(struct work_struct *work)
>  				AD5933_REG_REAL_DATA : AD5933_REG_IMAG_DATA,
>  				scan_count * 2, (u8 *)buf);
>  		if (ret)
> -			goto out;
> +			return;
>  
>  		if (scan_count == 2) {
>  			val[0] = be16_to_cpu(buf[0]);
> @@ -690,7 +696,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);
> -		goto out;
> +		return;
>  	}
>  
>  	if (status & AD5933_STAT_SWEEP_DONE) {
> @@ -703,8 +709,6 @@ 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);
>  }
>  
>  static int ad5933_probe(struct i2c_client *client,
> @@ -723,6 +727,8 @@ static int ad5933_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, indio_dev);
>  	st->client = client;
>  
> +	mutex_init(&st->lock);
> +
>  	if (!pdata)
>  		pdata = &ad5933_default_pdata;
>  
> 

Re: [PATCH] staging: iio: ad5933: Protect DIRECT mode using claim/release helpers

[Alison Schofield]

On Sat, Mar 25, 2017 at 05:56:54PM +0000, Jonathan Cameron wrote:
> On 23/03/17 17:36, Narcisa Ana Maria Vasile wrote:
> > This device operates in DIRECT_MODE and BUFFER_HARDWARE mode.
> > Replace usages of iio_dev->mlock with iio_device_{claim|release}_direct_mode()
> > helper functions to guarantee DIRECT mode and consequently protect
> > BUFFER mode too.
> > 
> > Add and use a device private lock to protect against conflicting access of the
> > state data.
> > This helps with IIO subsystem redefining iio_dev->mlock to be used by
> > the IIO core only for protecting device operating mode changes.
> > ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> > 
> > Remove lock from ad5933_work() because buffer mode should be enabled
> > when we reach this, and claiming DIRECT mode in all the other places
> > should protect it.
> Well presented. I was wondering about the logic behind that...
> > 
> > Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> This is certainly a fiddly one so I'd like some more eyes on it.
> 
> Lars - could you take a look? (he gets all the analog devices stuff ;)

Lars - Your thoughts?  

Narcisa traced through the callbacks carefully since was the
only/first driver in IIO setting INDIO_BUFFER_HARDWARE.

thanks,
alisons

> > ---
> >  drivers/staging/iio/impedance-analyzer/ad5933.c | 50 ++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index 297665d..3d539ee 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -98,6 +98,7 @@ struct ad5933_state {
> >  	struct i2c_client		*client;
> >  	struct regulator		*reg;
> >  	struct delayed_work		work;
> > +	struct mutex			lock; /* Protect sensor state */
> >  	unsigned long			mclk_hz;
> >  	unsigned char			ctrl_hb;
> >  	unsigned char			ctrl_lb;
> > @@ -306,9 +307,11 @@ static ssize_t ad5933_show_frequency(struct device *dev,
> >  		u8 d8[4];
> >  	} dat;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	ret = iio_device_claim_direct_mode(indio_dev);
> > +	if (ret)
> > +		return ret;
> >  	ret = ad5933_i2c_read(st->client, this_attr->address, 3, &dat.d8[1]);
> > -	mutex_unlock(&indio_dev->mlock);
> > +	iio_device_release_direct_mode(indio_dev);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -338,9 +341,11 @@ static ssize_t ad5933_store_frequency(struct device *dev,
> >  	if (val > AD5933_MAX_OUTPUT_FREQ_Hz)
> >  		return -EINVAL;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	ret = iio_device_claim_direct_mode(indio_dev);
> > +	if (ret)
> > +		return ret;
> >  	ret = ad5933_set_freq(st, this_attr->address, val);
> This is the one case that standas out where we aren't taking the new lock..
> It is setting a few internal state variables (st->freq_start and st->freq_inc)
> but this is safe because those aren't actually read anywhere..
> Presumably they were going to be cached, but the original author decided
> against at some point later...
> > -	mutex_unlock(&indio_dev->mlock);
> > +	iio_device_release_direct_mode(indio_dev);
> >  
> >  	return ret ? ret : len;
> >  }
> > @@ -364,7 +369,7 @@ static ssize_t ad5933_show(struct device *dev,
> >  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >  	int ret = 0, len = 0;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	mutex_lock(&st->lock);
> >  	switch ((u32)this_attr->address) {
> >  	case AD5933_OUT_RANGE:
> >  		len = sprintf(buf, "%u\n",
> > @@ -393,7 +398,7 @@ static ssize_t ad5933_show(struct device *dev,
> >  		ret = -EINVAL;
> >  	}
> >  
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->lock);
> >  	return ret ? ret : len;
> >  }
> >  
> > @@ -415,7 +420,10 @@ static ssize_t ad5933_store(struct device *dev,
> >  			return ret;
> >  	}
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	ret = iio_device_claim_direct_mode(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +	mutex_lock(&st->lock);
> >  	switch ((u32)this_attr->address) {
> >  	case AD5933_OUT_RANGE:
> >  		ret = -EINVAL;
> > @@ -465,7 +473,8 @@ static ssize_t ad5933_store(struct device *dev,
> >  		ret = -EINVAL;
> >  	}
> >  
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->lock);
> > +	iio_device_release_direct_mode(indio_dev);
> >  	return ret ? ret : len;
> >  }
> >  
> > @@ -532,11 +541,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (m) {
> >  	case IIO_CHAN_INFO_RAW:
> > -		mutex_lock(&indio_dev->mlock);
> > -		if (iio_buffer_enabled(indio_dev)) {
> > -			ret = -EBUSY;
> > -			goto out;
> > -		}
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> >  		ret = ad5933_cmd(st, AD5933_CTRL_MEASURE_TEMP);
> >  		if (ret < 0)
> >  			goto out;
> > @@ -549,7 +556,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
> >  				      2, (u8 *)&dat);
> >  		if (ret < 0)
> >  			goto out;
> > -		mutex_unlock(&indio_dev->mlock);
> > +		iio_device_release_direct_mode(indio_dev);
> >  		*val = sign_extend32(be16_to_cpu(dat), 13);
> >  
> >  		return IIO_VAL_INT;
> > @@ -561,7 +568,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
> >  
> >  	return -EINVAL;
> >  out:
> > -	mutex_unlock(&indio_dev->mlock);
> > +	iio_device_release_direct_mode(indio_dev);
> >  	return ret;
> >  }
> >  
> > @@ -657,18 +664,17 @@ static void ad5933_work(struct work_struct *work)
> >  	unsigned char status;
> >  	int ret;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> >  	if (st->state == AD5933_CTRL_INIT_START_FREQ) {
> >  		/* start sweep */
> >  		ad5933_cmd(st, AD5933_CTRL_START_SWEEP);
> >  		st->state = AD5933_CTRL_START_SWEEP;
> >  		schedule_delayed_work(&st->work, st->poll_time_jiffies);
> > -		goto out;
> > +		return;
> >  	}
> >  
> >  	ret = ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, &status);
> >  	if (ret)
> > -		goto out;
> > +		return;
> >  
> >  	if (status & AD5933_STAT_DATA_VALID) {
> >  		int scan_count = bitmap_weight(indio_dev->active_scan_mask,
> > @@ -678,7 +684,7 @@ static void ad5933_work(struct work_struct *work)
> >  				AD5933_REG_REAL_DATA : AD5933_REG_IMAG_DATA,
> >  				scan_count * 2, (u8 *)buf);
> >  		if (ret)
> > -			goto out;
> > +			return;
> >  
> >  		if (scan_count == 2) {
> >  			val[0] = be16_to_cpu(buf[0]);
> > @@ -690,7 +696,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);
> > -		goto out;
> > +		return;
> >  	}
> >  
> >  	if (status & AD5933_STAT_SWEEP_DONE) {
> > @@ -703,8 +709,6 @@ 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);
> >  }
> >  
> >  static int ad5933_probe(struct i2c_client *client,
> > @@ -723,6 +727,8 @@ static int ad5933_probe(struct i2c_client *client,
> >  	i2c_set_clientdata(client, indio_dev);
> >  	st->client = client;
> >  
> > +	mutex_init(&st->lock);
> > +
> >  	if (!pdata)
> >  		pdata = &ad5933_default_pdata;
> >  
> > 
> 
> --
> 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

Re: [PATCH] staging: iio: ad5933: Protect DIRECT mode using claim/release helpers

[Lars-Peter Clausen]

On 03/23/2017 06:36 PM, Narcisa Ana Maria Vasile wrote:
[...]
> @@ -415,7 +420,10 @@ static ssize_t ad5933_store(struct device *dev,
>  			return ret;
>  	}
>  
> -	mutex_lock(&indio_dev->mlock);
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;

This is a change in semantics. Previously it was possible to change the
attributes while in buffered mode, but now it no longer is. This is probably
OK, but should certainly be noted in the commit message.

> +	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
>  	case AD5933_OUT_RANGE:
>  		ret = -EINVAL;
> @@ -465,7 +473,8 @@ static ssize_t ad5933_store(struct device *dev,
>  		ret = -EINVAL;
>  	}
>  
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
> +	iio_device_release_direct_mode(indio_dev);
>  	return ret ? ret : len;
>  }
>  
> @@ -532,11 +541,9 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&indio_dev->mlock);
> -		if (iio_buffer_enabled(indio_dev)) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  		ret = ad5933_cmd(st, AD5933_CTRL_MEASURE_TEMP);
>  		if (ret < 0)
>  			goto out;
> @@ -549,7 +556,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  				      2, (u8 *)&dat);
>  		if (ret < 0)
>  			goto out;
> -		mutex_unlock(&indio_dev->mlock);
> +		iio_device_release_direct_mode(indio_dev);
>  		*val = sign_extend32(be16_to_cpu(dat), 13);
>  
>  		return IIO_VAL_INT;
> @@ -561,7 +568,7 @@ static int ad5933_read_raw(struct iio_dev *indio_dev,
>  
>  	return -EINVAL;
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  	return ret;
>  }
>