All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: iio: ade7753:  replace mlock with driver private lock
@ 2017-09-18  6:59 Himanshi Jain
  2017-09-18 10:06 ` Greg KH
  2017-09-24 14:17 ` Jonathan Cameron
  0 siblings, 2 replies; 9+ messages in thread
From: Himanshi Jain @ 2017-09-18  6:59 UTC (permalink / raw)
  To: outreachy-kernel, jic23, knaack.h, lars, pmeerw, linux-iio,
	daniel.baluta, amsfield22, Michael.Hennerich, gregkh

Replace driver usage of mlock with driver private lock to meet the new
model where usage of iio_dev->mlock is being redefined as protecting
operating mode changes(changes between BUFFER* and DIRECT modes).

Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
---
 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index ce26abdea..745c5a6 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -81,10 +81,12 @@
  * @tx:         transmit buffer
  * @rx:         receive buffer
  * @buf_lock:       mutex to protect tx and rx
+ * @lock:	protect sensor data
  **/
 struct ade7753_state {
 	struct spi_device   *us;
 	struct mutex        buf_lock;
+	struct mutex	    lock; /* protect sensor data */
 	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
 	u8          rx[ADE7753_MAX_RX];
 };
@@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
 
 	t = 27900 / val;
 	if (t > 0)
@@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
 	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
 
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 
 	return ret ? ret : len;
 }
@@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 	st->us = spi;
 	mutex_init(&st->buf_lock);
+	mutex_init(&st->lock);
 
 	indio_dev->name = spi->dev.driver->name;
 	indio_dev->dev.parent = &spi->dev;
-- 
1.9.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753:  replace mlock with driver private lock
  2017-09-18  6:59 [PATCH] staging: iio: ade7753: replace mlock with driver private lock Himanshi Jain
@ 2017-09-18 10:06 ` Greg KH
  2017-09-24 14:17 ` Jonathan Cameron
  1 sibling, 0 replies; 9+ messages in thread
From: Greg KH @ 2017-09-18 10:06 UTC (permalink / raw)
  To: Himanshi Jain
  Cc: outreachy-kernel, jic23, knaack.h, lars, pmeerw, linux-iio,
	daniel.baluta, amsfield22, Michael.Hennerich

On Mon, Sep 18, 2017 at 12:29:52PM +0530, Himanshi Jain wrote:
> Replace driver usage of mlock with driver private lock to meet the new
> model where usage of iio_dev->mlock is being redefined as protecting
> operating mode changes(changes between BUFFER* and DIRECT modes).

This is more complex than a "normal" outreachy coding style cleanup, so
I'll leave it to the IIO developers/maintainer to review/apply it.

thanks,

greg k-h


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753:  replace mlock with driver private lock
  2017-09-18  6:59 [PATCH] staging: iio: ade7753: replace mlock with driver private lock Himanshi Jain
  2017-09-18 10:06 ` Greg KH
@ 2017-09-24 14:17 ` Jonathan Cameron
  2017-09-27 14:57   ` Himanshi Jain
  1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2017-09-24 14:17 UTC (permalink / raw)
  To: Himanshi Jain
  Cc: outreachy-kernel, knaack.h, lars, pmeerw, linux-iio,
	daniel.baluta, amsfield22, Michael.Hennerich, gregkh

On Mon, 18 Sep 2017 12:29:52 +0530
Himanshi Jain <himshijain.hj@gmail.com> wrote:

> Replace driver usage of mlock with driver private lock to meet the new
> model where usage of iio_dev->mlock is being redefined as protecting
> operating mode changes(changes between BUFFER* and DIRECT modes).
> 
> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>

A well presented patch making a sensible change.  My only thought here
is that ultimately we could cover bother the buffer protection and
the state protection with a single lock.

Hmm. The new lock (as was the old mlock code) is providing protection
to ensure we end up with consistent state when changing the
sampling frequency between the sampling frequency and the
bus clock speed (which has different maximum values depending
on the current sampling frequency). In similar cases we have
expanded the meaning on the buffer lock to fulfil both roles
- that could be done here as well, although you have to be careful
about deadlocks.

On reflection I think this patch is worthwhile applying even
if this new lock gets dropped again in some later rework of this
driver.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/meter/ade7753.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index ce26abdea..745c5a6 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -81,10 +81,12 @@
>   * @tx:         transmit buffer
>   * @rx:         receive buffer
>   * @buf_lock:       mutex to protect tx and rx
> + * @lock:	protect sensor data
>   **/
>  struct ade7753_state {
>  	struct spi_device   *us;
>  	struct mutex        buf_lock;
> +	struct mutex	    lock; /* protect sensor data */
>  	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>  	u8          rx[ADE7753_MAX_RX];
>  };
> @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>  	if (!val)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
>  
>  	t = 27900 / val;
>  	if (t > 0)
> @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>  	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>  
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  
>  	return ret ? ret : len;
>  }
> @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  	st->us = spi;
>  	mutex_init(&st->buf_lock);
> +	mutex_init(&st->lock);
>  
>  	indio_dev->name = spi->dev.driver->name;
>  	indio_dev->dev.parent = &spi->dev;


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock
  2017-09-24 14:17 ` Jonathan Cameron
@ 2017-09-27 14:57   ` Himanshi Jain
  2017-09-30 20:17     ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Himanshi Jain @ 2017-09-27 14:57 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: outreachy-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald, linux-iio, Daniel Baluta, Alison Schofield,
	Hennerich, Michael, Greg Kroah-Hartman

On Sun, Sep 24, 2017 at 7:47 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 18 Sep 2017 12:29:52 +0530
> Himanshi Jain <himshijain.hj@gmail.com> wrote:
>
>> Replace driver usage of mlock with driver private lock to meet the new
>> model where usage of iio_dev->mlock is being redefined as protecting
>> operating mode changes(changes between BUFFER* and DIRECT modes).
>>
>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
>
> A well presented patch making a sensible change.  My only thought here
> is that ultimately we could cover bother the buffer protection and
> the state protection with a single lock.
>
> Hmm. The new lock (as was the old mlock code) is providing protection
> to ensure we end up with consistent state when changing the
> sampling frequency between the sampling frequency and the
> bus clock speed (which has different maximum values depending
> on the current sampling frequency). In similar cases we have
> expanded the meaning on the buffer lock to fulfil both roles
> - that could be done here as well, although you have to be careful
> about deadlocks.
>

I have understood how to use a single lock for both the purposes as you have
quoted in Katie's patch too i.e. by making an unlocked version of the function
ade7753_spi_write_reg_16(). And since the function is not publicly available
to be used, an unlocked version will not create any issue(which is why I am
assuming, we are not using nested locks - Please correct me if I am wrong.)

I will submit a new patch with a single lock.

> On reflection I think this patch is worthwhile applying even
> if this new lock gets dropped again in some later rework of this
> driver.
>
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
>

Thank you for reviewing and applying the patch and guiding for a better logical
solution.

> Thanks,
>
> Jonathan
>> ---
>>  drivers/staging/iio/meter/ade7753.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>> index ce26abdea..745c5a6 100644
>> --- a/drivers/staging/iio/meter/ade7753.c
>> +++ b/drivers/staging/iio/meter/ade7753.c
>> @@ -81,10 +81,12 @@
>>   * @tx:         transmit buffer
>>   * @rx:         receive buffer
>>   * @buf_lock:       mutex to protect tx and rx
>> + * @lock:    protect sensor data
>>   **/
>>  struct ade7753_state {
>>       struct spi_device   *us;
>>       struct mutex        buf_lock;
>> +     struct mutex        lock; /* protect sensor data */
>>       u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>>       u8          rx[ADE7753_MAX_RX];
>>  };
>> @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>       if (!val)
>>               return -EINVAL;
>>
>> -     mutex_lock(&indio_dev->mlock);
>> +     mutex_lock(&st->lock);
>>
>>       t = 27900 / val;
>>       if (t > 0)
>> @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>       ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>
>>  out:
>> -     mutex_unlock(&indio_dev->mlock);
>> +     mutex_unlock(&st->lock);
>>
>>       return ret ? ret : len;
>>  }
>> @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi)
>>       st = iio_priv(indio_dev);
>>       st->us = spi;
>>       mutex_init(&st->buf_lock);
>> +     mutex_init(&st->lock);
>>
>>       indio_dev->name = spi->dev.driver->name;
>>       indio_dev->dev.parent = &spi->dev;
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock
  2017-09-27 14:57   ` Himanshi Jain
@ 2017-09-30 20:17     ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-09-30 20:17 UTC (permalink / raw)
  To: Himanshi Jain
  Cc: outreachy-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald, linux-iio, Daniel Baluta, Alison Schofield,
	Hennerich, Michael, Greg Kroah-Hartman

On Wed, 27 Sep 2017 20:27:09 +0530
Himanshi Jain <himshijain.hj@gmail.com> wrote:

> On Sun, Sep 24, 2017 at 7:47 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Mon, 18 Sep 2017 12:29:52 +0530
> > Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >  
> >> Replace driver usage of mlock with driver private lock to meet the new
> >> model where usage of iio_dev->mlock is being redefined as protecting
> >> operating mode changes(changes between BUFFER* and DIRECT modes).
> >>
> >> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>  
> >
> > A well presented patch making a sensible change.  My only thought here
> > is that ultimately we could cover bother the buffer protection and
> > the state protection with a single lock.
> >
> > Hmm. The new lock (as was the old mlock code) is providing protection
> > to ensure we end up with consistent state when changing the
> > sampling frequency between the sampling frequency and the
> > bus clock speed (which has different maximum values depending
> > on the current sampling frequency). In similar cases we have
> > expanded the meaning on the buffer lock to fulfil both roles
> > - that could be done here as well, although you have to be careful
> > about deadlocks.
> >  
> 
> I have understood how to use a single lock for both the purposes as you have
> quoted in Katie's patch too i.e. by making an unlocked version of the function
> ade7753_spi_write_reg_16(). And since the function is not publicly available
> to be used, an unlocked version will not create any issue(which is why I am
> assuming, we are not using nested locks - Please correct me if I am wrong.)
> 
> I will submit a new patch with a single lock.

That's great, but please do it on top of your previous patch as that is now
in a non rebasing tree.

Jonathan

> 
> > On reflection I think this patch is worthwhile applying even
> > if this new lock gets dropped again in some later rework of this
> > driver.
> >
> > Applied to the togreg branch of iio.git and pushed out as testing for
> > the autobuilders to play with it.
> >  
> 
> Thank you for reviewing and applying the patch and guiding for a better logical
> solution.
> 
> > Thanks,
> >
> > Jonathan  
> >> ---
> >>  drivers/staging/iio/meter/ade7753.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> >> index ce26abdea..745c5a6 100644
> >> --- a/drivers/staging/iio/meter/ade7753.c
> >> +++ b/drivers/staging/iio/meter/ade7753.c
> >> @@ -81,10 +81,12 @@
> >>   * @tx:         transmit buffer
> >>   * @rx:         receive buffer
> >>   * @buf_lock:       mutex to protect tx and rx
> >> + * @lock:    protect sensor data
> >>   **/
> >>  struct ade7753_state {
> >>       struct spi_device   *us;
> >>       struct mutex        buf_lock;
> >> +     struct mutex        lock; /* protect sensor data */
> >>       u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
> >>       u8          rx[ADE7753_MAX_RX];
> >>  };
> >> @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> >>       if (!val)
> >>               return -EINVAL;
> >>
> >> -     mutex_lock(&indio_dev->mlock);
> >> +     mutex_lock(&st->lock);
> >>
> >>       t = 27900 / val;
> >>       if (t > 0)
> >> @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> >>       ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
> >>
> >>  out:
> >> -     mutex_unlock(&indio_dev->mlock);
> >> +     mutex_unlock(&st->lock);
> >>
> >>       return ret ? ret : len;
> >>  }
> >> @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi)
> >>       st = iio_priv(indio_dev);
> >>       st->us = spi;
> >>       mutex_init(&st->buf_lock);
> >> +     mutex_init(&st->lock);
> >>
> >>       indio_dev->name = spi->dev.driver->name;
> >>       indio_dev->dev.parent = &spi->dev;  
> >  
> --
> 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock
  2017-03-13 12:33   ` SIMRAN SINGHAL
@ 2017-03-13 13:34     ` Lars-Peter Clausen
  0 siblings, 0 replies; 9+ messages in thread
From: Lars-Peter Clausen @ 2017-03-13 13:34 UTC (permalink / raw)
  To: SIMRAN SINGHAL
  Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
	Pete Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel,
	linux-kernel, outreachy-kernel

On 03/13/2017 01:33 PM, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 5:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 03/12/2017 02:32 PM, 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 a lock in the devices global data.
>>>
>>> Fix some coding style issues related to white space also.
>>>
>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>>> ---
>>>  drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>>> index dfd8b71..ca99d82 100644
>>> --- a/drivers/staging/iio/meter/ade7753.c
>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>> @@ -81,12 +81,14 @@
>>>   * @tx:         transmit buffer
>>>   * @rx:         receive buffer
>>>   * @buf_lock:       mutex to protect tx and rx
>>> + * @lock:    protect sensor state
>>
>> It might make sense to reuse the existing lock which currently protects the
>> read/write functions. You can do this by introducing a variant of
>> ade7753_spi_{read,write}_reg_16() that does not take a lock and use these to
>> implement the read-modify-write cycle in a protected section.
>>
>> Looking through the driver there seem to be other places as well that do
>> read-modify-write that should be protected by a lock, but currently are not.
>> This might be a good task.
>>
> 
> Are you trying to say that their is no need of introducing "lock",
> I can using "buf_lock" only.

Yes, there should be no need for two locks. But you need to slightly
refactor the code to avoid taking the same lock nested.

> 
> Thanks!
> 
>>>   **/
>>>  struct ade7753_state {
>>> -         struct spi_device   *us;
>>> -                 struct mutex        buf_lock;
>>> -                         u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>>> -                                 u8          rx[ADE7753_MAX_RX];
>>> +     struct spi_device   *us;
>>> +     struct mutex        buf_lock;
>>> +     struct mutex        lock;       /* protect sensor state */
>>> +     u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>>> +     u8          rx[ADE7753_MAX_RX];
>>>  };
>>>
>>>  static int ade7753_spi_write_reg_8(struct device *dev,
>>> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>>       if (!val)
>>>               return -EINVAL;
>>>
>>> -     mutex_lock(&indio_dev->mlock);
>>> +     mutex_lock(&st->lock);
>>>
>>>       t = 27900 / val;
>>>       if (t > 0)
>>> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>>       ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>>
>>>  out:
>>> -     mutex_unlock(&indio_dev->mlock);
>>> +     mutex_unlock(&st->lock);
>>>
>>>       return ret ? ret : len;
>>>  }
>>>
>>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock
  2017-03-13 12:00 ` Lars-Peter Clausen
@ 2017-03-13 12:33   ` SIMRAN SINGHAL
  2017-03-13 13:34     ` Lars-Peter Clausen
  0 siblings, 1 reply; 9+ messages in thread
From: SIMRAN SINGHAL @ 2017-03-13 12:33 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
	Pete Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel,
	linux-kernel, outreachy-kernel

On Mon, Mar 13, 2017 at 5:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 03/12/2017 02:32 PM, 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 a lock in the devices global data.
>>
>> Fix some coding style issues related to white space also.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> ---
>>  drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>> index dfd8b71..ca99d82 100644
>> --- a/drivers/staging/iio/meter/ade7753.c
>> +++ b/drivers/staging/iio/meter/ade7753.c
>> @@ -81,12 +81,14 @@
>>   * @tx:         transmit buffer
>>   * @rx:         receive buffer
>>   * @buf_lock:       mutex to protect tx and rx
>> + * @lock:    protect sensor state
>
> It might make sense to reuse the existing lock which currently protects the
> read/write functions. You can do this by introducing a variant of
> ade7753_spi_{read,write}_reg_16() that does not take a lock and use these to
> implement the read-modify-write cycle in a protected section.
>
> Looking through the driver there seem to be other places as well that do
> read-modify-write that should be protected by a lock, but currently are not.
> This might be a good task.
>

Are you trying to say that their is no need of introducing "lock",
I can using "buf_lock" only.

Thanks!

>>   **/
>>  struct ade7753_state {
>> -         struct spi_device   *us;
>> -                 struct mutex        buf_lock;
>> -                         u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>> -                                 u8          rx[ADE7753_MAX_RX];
>> +     struct spi_device   *us;
>> +     struct mutex        buf_lock;
>> +     struct mutex        lock;       /* protect sensor state */
>> +     u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>> +     u8          rx[ADE7753_MAX_RX];
>>  };
>>
>>  static int ade7753_spi_write_reg_8(struct device *dev,
>> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>       if (!val)
>>               return -EINVAL;
>>
>> -     mutex_lock(&indio_dev->mlock);
>> +     mutex_lock(&st->lock);
>>
>>       t = 27900 / val;
>>       if (t > 0)
>> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>       ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>
>>  out:
>> -     mutex_unlock(&indio_dev->mlock);
>> +     mutex_unlock(&st->lock);
>>
>>       return ret ? ret : len;
>>  }
>>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock
  2017-03-12 13:32 simran singhal
@ 2017-03-13 12:00 ` Lars-Peter Clausen
  2017-03-13 12:33   ` SIMRAN SINGHAL
  0 siblings, 1 reply; 9+ messages in thread
From: Lars-Peter Clausen @ 2017-03-13 12:00 UTC (permalink / raw)
  To: simran singhal
  Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
	Pete Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel,
	linux-kernel, outreachy-kernel

On 03/12/2017 02:32 PM, 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 a lock in the devices global data.
> 
> Fix some coding style issues related to white space also.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..ca99d82 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -81,12 +81,14 @@
>   * @tx:         transmit buffer
>   * @rx:         receive buffer
>   * @buf_lock:       mutex to protect tx and rx
> + * @lock:	protect sensor state

It might make sense to reuse the existing lock which currently protects the
read/write functions. You can do this by introducing a variant of
ade7753_spi_{read,write}_reg_16() that does not take a lock and use these to
implement the read-modify-write cycle in a protected section.

Looking through the driver there seem to be other places as well that do
read-modify-write that should be protected by a lock, but currently are not.
This might be a good task.

>   **/
>  struct ade7753_state {
> -	    struct spi_device   *us;
> -		    struct mutex        buf_lock;
> -			    u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
> -				    u8          rx[ADE7753_MAX_RX];
> +	struct spi_device   *us;
> +	struct mutex        buf_lock;
> +	struct mutex        lock;	/* protect sensor state */
> +	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
> +	u8          rx[ADE7753_MAX_RX];
>  };
>  
>  static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>  	if (!val)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->lock);
>  
>  	t = 27900 / val;
>  	if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>  	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>  
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->lock);
>  
>  	return ret ? ret : len;
>  }
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] staging: iio: ade7753: replace mlock with driver private lock
@ 2017-03-12 13:32 simran singhal
  2017-03-13 12:00 ` Lars-Peter Clausen
  0 siblings, 1 reply; 9+ messages in thread
From: simran singhal @ 2017-03-12 13:32 UTC (permalink / raw)
  To: lars
  Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
	Pete Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel,
	linux-kernel, outreachy-kernel

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 a lock in the devices global data.

Fix some coding style issues related to white space also.

Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---
 drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index dfd8b71..ca99d82 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -81,12 +81,14 @@
  * @tx:         transmit buffer
  * @rx:         receive buffer
  * @buf_lock:       mutex to protect tx and rx
+ * @lock:	protect sensor state
  **/
 struct ade7753_state {
-	    struct spi_device   *us;
-		    struct mutex        buf_lock;
-			    u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
-				    u8          rx[ADE7753_MAX_RX];
+	struct spi_device   *us;
+	struct mutex        buf_lock;
+	struct mutex        lock;	/* protect sensor state */
+	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
+	u8          rx[ADE7753_MAX_RX];
 };
 
 static int ade7753_spi_write_reg_8(struct device *dev,
@@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->lock);
 
 	t = 27900 / val;
 	if (t > 0)
@@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
 	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
 
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->lock);
 
 	return ret ? ret : len;
 }
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-09-30 20:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-18  6:59 [PATCH] staging: iio: ade7753: replace mlock with driver private lock Himanshi Jain
2017-09-18 10:06 ` Greg KH
2017-09-24 14:17 ` Jonathan Cameron
2017-09-27 14:57   ` Himanshi Jain
2017-09-30 20:17     ` Jonathan Cameron
  -- strict thread matches above, loose matches on Subject: below --
2017-03-12 13:32 simran singhal
2017-03-13 12:00 ` Lars-Peter Clausen
2017-03-13 12:33   ` SIMRAN SINGHAL
2017-03-13 13:34     ` Lars-Peter Clausen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.