[PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[Varsha Rao]

Remove mutex_lock() and mutex_unlock() function calls, as the
adis16240_spi_read_signed() function can be run parallel and safely
multiple times. Also remove the mutex.h header file and comment, which
are no longer required.

As indio_dev is declared and initialized in adis16240_spi_read_signed(),
again declaration to same type and initialization to same value is not
required, remove it from adis16240_read_12bit_signed().

Simplify the return logic, by merging assignment and return into a single
line.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
Changes in v2:

- mutex_lock() and mutex_unlock() are removed, instead of replacing
  mlock with driver private lock.
- Commit message modified.
- mutex.h header file is removed.
- Unnecessary comment is removed.
- Return logic is compressed to single line.
- Removed indio_dev declaration and initialization from
  adis16240_read_12bit_signed().

 drivers/staging/iio/accel/adis16240.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index 27d7f6a..3a5f6ab 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -10,7 +10,6 @@
 #include <linux/irq.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
-#include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/spi/spi.h>
@@ -227,15 +226,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)
 {
-	ssize_t ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-
-	/* Take the iio_dev status lock */
-	mutex_lock(&indio_dev->mlock);
-	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret;
+	return adis16240_spi_read_signed(dev, attr, buf, 12);
 }
 
 static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
@@ -295,31 +286,25 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16240_addresses[chan->scan_index][0];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16240_addresses[chan->scan_index][1];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
-- 
2.9.3

Re: [PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[Jonathan Cameron]

On 14/03/17 16:23, Varsha Rao wrote:
> Remove mutex_lock() and mutex_unlock() function calls, as the
> adis16240_spi_read_signed() function can be run parallel and safely
> multiple times. Also remove the mutex.h header file and comment, which
> are no longer required.
> 
> As indio_dev is declared and initialized in adis16240_spi_read_signed(),
> again declaration to same type and initialization to same value is not
> required, remove it from adis16240_read_12bit_signed().
> 
> Simplify the return logic, by merging assignment and return into a single
> line.
> 
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
It's a bit obscure, but isn't there the potential to get the check_status
call spitting out text for the wrong read, or potentially miss a status
value entirely?

(kind of more for Lars really, but feel free to comment Varsha!)

The various faults look like this wouldn't be an issue, but I'm not
totally sure about the SPI failure and whether a following good read
at exactly the wrong time might result in the error not being reported...
> ---
> Changes in v2:
> 
> - mutex_lock() and mutex_unlock() are removed, instead of replacing
>   mlock with driver private lock.
> - Commit message modified.
> - mutex.h header file is removed.
> - Unnecessary comment is removed.
> - Return logic is compressed to single line.
> - Removed indio_dev declaration and initialization from
>   adis16240_read_12bit_signed().
> 
>  drivers/staging/iio/accel/adis16240.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
> index 27d7f6a..3a5f6ab 100644
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ b/drivers/staging/iio/accel/adis16240.c
> @@ -10,7 +10,6 @@
>  #include <linux/irq.h>
>  #include <linux/gpio.h>
>  #include <linux/delay.h>
> -#include <linux/mutex.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/spi/spi.h>
> @@ -227,15 +226,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
>  					   struct device_attribute *attr,
>  					   char *buf)
>  {
> -	ssize_t ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -
> -	/* Take the iio_dev status lock */
> -	mutex_lock(&indio_dev->mlock);
> -	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
> -	mutex_unlock(&indio_dev->mlock);
> -
> -	return ret;
> +	return adis16240_spi_read_signed(dev, attr, buf, 12);
>  }
>  
>  static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> @@ -295,31 +286,25 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		bits = 10;
> -		mutex_lock(&indio_dev->mlock);
>  		addr = adis16240_addresses[chan->scan_index][0];
>  		ret = adis_read_reg_16(st, addr, &val16);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
>  			return ret;
>  		}
>  		val16 &= (1 << bits) - 1;
>  		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
>  		*val = val16;
> -		mutex_unlock(&indio_dev->mlock);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_PEAK:
>  		bits = 10;
> -		mutex_lock(&indio_dev->mlock);
>  		addr = adis16240_addresses[chan->scan_index][1];
>  		ret = adis_read_reg_16(st, addr, &val16);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
>  			return ret;
>  		}
>  		val16 &= (1 << bits) - 1;
>  		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
>  		*val = val16;
> -		mutex_unlock(&indio_dev->mlock);
>  		return IIO_VAL_INT;
>  	}
>  	return -EINVAL;
> 

Re: [PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[Varsha Rao]

Hello,

> It's a bit obscure, but isn't there the potential to get the check_status
> call spitting out text for the wrong read, or potentially miss a status
> value entirely?

adis_read_reg()  function calls spi_sync(), which is inside mutex block,
then how will it misread wrong value?
Will not spi_sync() help, which also has mutex block in its definition?
Otherwise are changes to be made to if condition
(val & ADIS16240_ERROR_ACTIVE)?, which depends on the value of val.

I understand that we cannot use txrx_lock private lock in
adis16240_read_12bit_signed.
If the txrx_lock value is set, then it would cause deadlock as in
adis_read_reg again,
it will try to lock txrx_lock variable.

> (kind of more for Lars really, but feel free to comment Varsha!)
>
> The various faults look like this wouldn't be an issue, but I'm not
> totally sure about the SPI failure and whether a following good read
> at exactly the wrong time might result in the error not being reported...

  Was this case eliminated when mutex lock was used before
  adis16240_spi_read_signed() function call?

  Thanks,
  Varsha

Re: [PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[Lars-Peter Clausen]

On 03/15/2017 11:11 PM, Jonathan Cameron wrote:
> On 14/03/17 16:23, Varsha Rao wrote:
>> Remove mutex_lock() and mutex_unlock() function calls, as the
>> adis16240_spi_read_signed() function can be run parallel and safely
>> multiple times. Also remove the mutex.h header file and comment, which
>> are no longer required.
>>
>> As indio_dev is declared and initialized in adis16240_spi_read_signed(),
>> again declaration to same type and initialization to same value is not
>> required, remove it from adis16240_read_12bit_signed().
>>
>> Simplify the return logic, by merging assignment and return into a single
>> line.
>>
>> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> It's a bit obscure, but isn't there the potential to get the check_status
> call spitting out text for the wrong read, or potentially miss a status
> value entirely?
> 
> (kind of more for Lars really, but feel free to comment Varsha!)
> 
> The various faults look like this wouldn't be an issue, but I'm not
> totally sure about the SPI failure and whether a following good read
> at exactly the wrong time might result in the error not being reported...

I don't think it matters. The error reporting is asynchronous anyway and the
bits are read-to-clear. So even if two access see the error flag set in the
data only one of them will be able to read the bits in the diag_stat
register (unless the error is persistent, in which case the error flag in
the data would also be persistent).

Re: [PATCH v2] staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

[Jonathan Cameron]

On 17/03/17 09:03, Lars-Peter Clausen wrote:
> On 03/15/2017 11:11 PM, Jonathan Cameron wrote:
>> On 14/03/17 16:23, Varsha Rao wrote:
>>> Remove mutex_lock() and mutex_unlock() function calls, as the
>>> adis16240_spi_read_signed() function can be run parallel and safely
>>> multiple times. Also remove the mutex.h header file and comment, which
>>> are no longer required.
>>>
>>> As indio_dev is declared and initialized in adis16240_spi_read_signed(),
>>> again declaration to same type and initialization to same value is not
>>> required, remove it from adis16240_read_12bit_signed().
>>>
>>> Simplify the return logic, by merging assignment and return into a single
>>> line.
>>>
>>> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
>> It's a bit obscure, but isn't there the potential to get the check_status
>> call spitting out text for the wrong read, or potentially miss a status
>> value entirely?
>>
>> (kind of more for Lars really, but feel free to comment Varsha!)
>>
>> The various faults look like this wouldn't be an issue, but I'm not
>> totally sure about the SPI failure and whether a following good read
>> at exactly the wrong time might result in the error not being reported...
> 
> I don't think it matters. The error reporting is asynchronous anyway and the
> bits are read-to-clear. So even if two access see the error flag set in the
> data only one of them will be able to read the bits in the diag_stat
> register (unless the error is persistent, in which case the error flag in
> the data would also be persistent).
Fair enough.

Applied to the togreg branch of iio.git - will push out as testing later
for the autobuilders to play with it.


Thanks,

Jonathan
> 
> --
> 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
>