Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Linux>
  3. Kernel
[PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code
singhalsimran0 at gmail
Mar 20, 2017, 8:20 AM
Post #1 of 4 (12 views)
Permalink
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 [ In reply to ]
jic23 at kernel
Mar 21, 2017, 2:20 PM
Post #2 of 4 (11 views)
Permalink
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 [ In reply to ]
lkp at intel
Mar 22, 2017, 2:20 PM
Post #3 of 4 (11 views)
Permalink
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

Attached Files:

Re: [PATCH v7] staging: adis16060_core: Replace mlock with buf_lock and refactor code [ In reply to ]
jic23 at kernel
Mar 22, 2017, 2:20 PM
Post #4 of 4 (11 views)
Permalink
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
>

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal