[PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code

[PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code

[simran singhal]

The IIO subsystem is 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.

In this driver, mlock was being used to protect hardware state
changes. Replace it with buf_lock in the devices global data.

As buf_lock protects both the adis16060_spi_write() and
adis16060_spi_read() functions and both are always called in
pair. First write, then read. Thus, refactor the code to have
one single function adis16060_spi_write_than_read() which is
protected by the existing buf_lock.

Removed nested locks as the function adis16060_read_raw call
a lock on &st->buf_lock and then calls the function
adis16060_spi_write which again tries to get hold
of the same lock.

The locks in adis16060_read_raw are dropped and now have a
single safe function adis16060_spi_write_than_read
with the locks inside it being called.

Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---

 v7:
   -Change subject
   -Remove lock from read_raw instead of from 
    function adis16060_spi_write_than_read().
 
 drivers/staging/iio/gyro/adis16060_core.c | 38 +++++++++----------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
index c9d46e7..193587c 100644
--- a/drivers/staging/iio/gyro/adis16060_core.c
+++ b/drivers/staging/iio/gyro/adis16060_core.c
@@ -40,25 +40,18 @@ struct adis16060_state {
 
 static struct iio_dev *adis16060_iio_dev;
 
-static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
+static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
+					 u8 conf, u16 *val)
 {
 	int ret;
 	struct adis16060_state *st = iio_priv(indio_dev);
 
 	mutex_lock(&st->buf_lock);
-	st->buf[2] = val; /* The last 8 bits clocked in are latched */
+	st->buf[2] = conf; /* The last 8 bits clocked in are latched */
 	ret = spi_write(st->us_w, st->buf, 3);
-	mutex_unlock(&st->buf_lock);
-
-	return ret;
-}
-
-static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
-{
-	int ret;
-	struct adis16060_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->buf_lock);
+	if (ret < 0)
+		return ret;
 
 	ret = spi_read(st->us_r, st->buf, 3);
 
@@ -69,8 +62,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
 	 */
 	if (!ret)
 		*val = ((st->buf[0] & 0x3) << 12) |
-			(st->buf[1] << 4) |
-			((st->buf[2] >> 4) & 0xF);
+			 (st->buf[1] << 4) |
+			 ((st->buf[2] >> 4) & 0xF);
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
@@ -83,20 +76,15 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
 {
 	u16 tval = 0;
 	int ret;
+	struct adis16060_state *st = iio_priv(indio_dev);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		/* Take the iio_dev status lock */
-		mutex_lock(&indio_dev->mlock);
-		ret = adis16060_spi_write(indio_dev, chan->address);
+		ret = adis16060_spi_write_than_read(indio_dev,
+						    chan->address, &tval);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
-		ret = adis16060_spi_read(indio_dev, &tval);
-		if (ret < 0)
-			goto out_unlock;
-
-		mutex_unlock(&indio_dev->mlock);
 		*val = tval;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
@@ -110,10 +98,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
 	}
 
 	return -EINVAL;
-
-out_unlock:
-	mutex_unlock(&indio_dev->mlock);
-	return ret;
 }
 
 static const struct iio_info adis16060_info = {
-- 
2.7.4

Re: [PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code

[Jonathan Cameron]

On 20/03/17 12:53, simran singhal wrote:
> The IIO subsystem is 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.
> 
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with buf_lock in the devices global data.
> 
> As buf_lock protects both the adis16060_spi_write() and
> adis16060_spi_read() functions and both are always called in
> pair. First write, then read. Thus, refactor the code to have
> one single function adis16060_spi_write_than_read() which is
> protected by the existing buf_lock.
> 
> Removed nested locks as the function adis16060_read_raw call
> a lock on &st->buf_lock and then calls the function
> adis16060_spi_write which again tries to get hold
> of the same lock.
> 
> The locks in adis16060_read_raw are dropped and now have a
> single safe function adis16060_spi_write_than_read
> with the locks inside it being called.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
So close!  If part of the exercise here wasn't learning how
to get things near perfect, I'd probably have picked this up
and fixed it (whilst moaning extensively :)  Not this time
though: roll on v8.

Anyhow, see inline.  Always worth checking the patch before
emailing for anything that has snuck in and shouldn't be there.

I always find git gui particularly useful for seeing this stuff
as well.

The actual chance is correct and the patch well presented.

Jonathan
p.s. I'd probably have picked it up anyway, but there is
always a slight pause after one sends a pull request as
I will want to fast forward my tree if / when Greg pulls.
> ---
> 
>  v7:
>    -Change subject
>    -Remove lock from read_raw instead of from 
>     function adis16060_spi_write_than_read().
>  
>  drivers/staging/iio/gyro/adis16060_core.c | 38 +++++++++----------------------
>  1 file changed, 11 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..193587c 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -40,25 +40,18 @@ struct adis16060_state {
>  
>  static struct iio_dev *adis16060_iio_dev;
>  
> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> +					 u8 conf, u16 *val)
>  {
>  	int ret;
>  	struct adis16060_state *st = iio_priv(indio_dev);
>  
>  	mutex_lock(&st->buf_lock);
> -	st->buf[2] = val; /* The last 8 bits clocked in are latched */
> +	st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>  	ret = spi_write(st->us_w, st->buf, 3);
> -	mutex_unlock(&st->buf_lock);
> -
> -	return ret;
> -}
> -
> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> -{
> -	int ret;
> -	struct adis16060_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->buf_lock);
> +	if (ret < 0)
> +		return ret;
>  
>  	ret = spi_read(st->us_r, st->buf, 3);
>  
> @@ -69,8 +62,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>  	 */
>  	if (!ret)
>  		*val = ((st->buf[0] & 0x3) << 12) |
> -			(st->buf[1] << 4) |
> -			((st->buf[2] >> 4) & 0xF);
> +			 (st->buf[1] << 4) |
> +			 ((st->buf[2] >> 4) & 0xF);
Why the above change? Not related to everything else in the patch
(looks like an editor thinking it knows better on alignment to me!)
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
> @@ -83,20 +76,15 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>  {
>  	u16 tval = 0;
>  	int ret;
> +	struct adis16060_state *st = iio_priv(indio_dev);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		/* Take the iio_dev status lock */
> -		mutex_lock(&indio_dev->mlock);
> -		ret = adis16060_spi_write(indio_dev, chan->address);
> +		ret = adis16060_spi_write_than_read(indio_dev,
> +						    chan->address, &tval);
>  		if (ret < 0)
> -			goto out_unlock;
> +			return ret;
>  
> -		ret = adis16060_spi_read(indio_dev, &tval);
> -		if (ret < 0)
> -			goto out_unlock;
> -
> -		mutex_unlock(&indio_dev->mlock);
>  		*val = tval;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_OFFSET:
> @@ -110,10 +98,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>  	}
>  
>  	return -EINVAL;
> -
> -out_unlock:
> -	mutex_unlock(&indio_dev->mlock);
> -	return ret;
>  }
>  
>  static const struct iio_info adis16060_info = {
> 

Re: [PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Hi simran,

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.11-rc3 next-20170322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/simran-singhal/staging-adis16060_core-Replace-mlock-with-buf_lock-and-refactor-code/20170323-031726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-s0-03230201 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/staging/iio/gyro/adis16060_core.c: In function 'adis16060_read_raw':
>> drivers/staging/iio/gyro/adis16060_core.c:79: warning: unused variable 'st'

vim +/st +79 drivers/staging/iio/gyro/adis16060_core.c

    63		if (!ret)
    64			*val = ((st->buf[0] & 0x3) << 12) |
    65				 (st->buf[1] << 4) |
    66				 ((st->buf[2] >> 4) & 0xF);
    67		mutex_unlock(&st->buf_lock);
    68	
    69		return ret;
    70	}
    71	
    72	static int adis16060_read_raw(struct iio_dev *indio_dev,
    73				      struct iio_chan_spec const *chan,
    74				      int *val, int *val2,
    75				      long mask)
    76	{
    77		u16 tval = 0;
    78		int ret;
  > 79		struct adis16060_state *st = iio_priv(indio_dev);
    80	
    81		switch (mask) {
    82		case IIO_CHAN_INFO_RAW:
    83			ret = adis16060_spi_write_than_read(indio_dev,
    84							    chan->address, &tval);
    85			if (ret < 0)
    86				return ret;
    87	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29884 bytes --]

Re: [PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code

[Jonathan Cameron]

On 22/03/17 20:00, kbuild test robot wrote:
> Hi simran,
> 
> [auto build test WARNING on iio/togreg]
> [also build test WARNING on v4.11-rc3 next-20170322]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/simran-singhal/staging-adis16060_core-Replace-mlock-with-buf_lock-and-refactor-code/20170323-031726
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: x86_64-randconfig-s0-03230201 (attached as .config)
> compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/staging/iio/gyro/adis16060_core.c: In function 'adis16060_read_raw':
>>> drivers/staging/iio/gyro/adis16060_core.c:79: warning: unused variable 'st'
Already fixed in V8 which has been applied to the togreg branch of iio.git.

Thanks,
Jonathan
> 
> vim +/st +79 drivers/staging/iio/gyro/adis16060_core.c
> 
>     63		if (!ret)
>     64			*val = ((st->buf[0] & 0x3) << 12) |
>     65				 (st->buf[1] << 4) |
>     66				 ((st->buf[2] >> 4) & 0xF);
>     67		mutex_unlock(&st->buf_lock);
>     68	
>     69		return ret;
>     70	}
>     71	
>     72	static int adis16060_read_raw(struct iio_dev *indio_dev,
>     73				      struct iio_chan_spec const *chan,
>     74				      int *val, int *val2,
>     75				      long mask)
>     76	{
>     77		u16 tval = 0;
>     78		int ret;
>   > 79		struct adis16060_state *st = iio_priv(indio_dev);
>     80	
>     81		switch (mask) {
>     82		case IIO_CHAN_INFO_RAW:
>     83			ret = adis16060_spi_write_than_read(indio_dev,
>     84							    chan->address, &tval);
>     85			if (ret < 0)
>     86				return ret;
>     87	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>