[PATCH] staging: iio: ade7753: replace mlock with driver private lock

[PATCH] staging: iio: ade7753: replace mlock with driver private lock

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

Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Alison Schofield]

On Sun, Mar 12, 2017 at 07:02:50PM +0530, 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>

Hi Simran, This looks good to me.  Let's see what the
reviewers say.  I think the white space stuff is ok,
since it was right where you were editing. 
alisons

> ---
>  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
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[SIMRAN SINGHAL]

On Mon, Mar 13, 2017 at 12:03 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> On Sun, Mar 12, 2017 at 07:02:50PM +0530, 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>
>
> Hi Simran, This looks good to me.  Let's see what the
> reviewers say.  I think the white space stuff is ok,
> since it was right where you were editing.
> alisons
>
Alison, so sending this patch here on outreachy mailing list is fine.
Still confuse :P

>> ---
>>  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
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
>> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Alison Schofield]

On Mon, Mar 13, 2017 at 09:28:34AM +0530, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 12:03 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> > On Sun, Mar 12, 2017 at 07:02:50PM +0530, 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>
> >
> > Hi Simran, This looks good to me.  Let's see what the
> > reviewers say.  I think the white space stuff is ok,
> > since it was right where you were editing.
> > alisons
> >
> Alison, so sending this patch here on outreachy mailing list is fine.
> Still confuse :P

You are OK.  You sent it to everyone suggested in the Task Description.

This patch was sent before I posted the Task Description.  I'm assuming
that since then you've found the posted Task:
https://kernelnewbies.org/IIO_tasks 
Find Coding Task 2 --> "PATCHES need to be sent to all of:"

> 
> >> ---
> >>  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
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> >> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Lars-Peter Clausen]

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;
>  }
> 

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[SIMRAN SINGHAL]

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;
>>  }
>>
>

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Lars-Peter Clausen]

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;
>>>  }
>>>
>>

Re: [Outreachy kernel] Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Gargi Sharma]

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.

There are other read/write functions for example,
ade7753_spi_{read/write}_reg_8 that use the mutex as well. Should a
variant of these functions be introduced as well? Also, how does one
go about implementing RMW inside 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.

Am I right in understanding that we want to introduce mutex lock for
writes in other drivers as well?

Thanks,
Gargi
>
> >   **/
> >  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;
> >  }
> >
>
> --

Re: [Outreachy kernel] Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Jonathan Cameron]

On 17/03/17 09:32, Gargi Sharma 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.
> 
> There are other read/write functions for example,
> ade7753_spi_{read/write}_reg_8 that use the mutex as well. Should a
> variant of these functions be introduced as well? Also, how does one
> go about implementing RMW inside a protected section.
Hmm. Simran has also been progressing with patches for this.

You raise a good question. There are other read/modify/write sequences in
the driver.  They don't have the same issue with potentially deadlocking
against the buf lock as they are all using the spi subsystems provisions
for small write/read cycles where buffer protection is handled internally.

So let us address the cases in turn:

static int ade7753_reset(struct device *dev)
{
	u16 val;
	int ret;

	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
	if (ret)
		return ret;

	val |= BIT(6); /* Software Chip Reset */

	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
}
This is only called in the device initialization.  At that point
we should be fine in assuming no parallel calls.  Crucial point
is it is before the call to iio_device_register which exposes
the userspace interfaces.

static int ade7753_set_irq(struct device *dev, bool enable)
{
	int ret;
	u8 irqen;

	ret = ade7753_spi_read_reg_8(dev, ADE7753_IRQEN, &irqen);
	if (ret)
		goto error_ret;

	if (enable)
		irqen |= BIT(3); /* Enables an interrupt when a data is
				  * present in the waveform register
				  */
	else
		irqen &= ~BIT(3);

	ret = ade7753_spi_write_reg_8(dev, ADE7753_IRQEN, irqen);

error_ret:
	return ret;
}

This one is actually safe because it is the only function that
modifies that particular register.

/* Power down the device */
static int ade7753_stop_device(struct device *dev)
{
	u16 val;
	int ret;

	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
	if (ret)
		return ret;

	val |= BIT(4);  /* AD converters can be turned off */

	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
}

Only called in remove (after userspace interfaces have been
removed by the iio_device_unregister call so also should not
be running concurrently with much else.

So I think all the other cases are safe.  Perhaps it would have
been better to have had a lock around them, purely to make
the code more resilient against future changes though.  
Probably a job to do as part of a larger scale pile of work
on that driver rather than as a one off patch.

Jonathan







> 
> 
>>
>> 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.
> 
> Am I right in understanding that we want to introduce mutex lock for
> writes in other drivers as well?
> 
> Thanks,
> Gargi
>>
>>>   **/
>>>  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;
>>>  }
>>>
>>
>> --
> --
> 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: [Outreachy kernel] Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Gargi Sharma]

On Sun, Mar 19, 2017 at 4:01 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 17/03/17 09:32, Gargi Sharma 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.
>>
>> There are other read/write functions for example,
>> ade7753_spi_{read/write}_reg_8 that use the mutex as well. Should a
>> variant of these functions be introduced as well? Also, how does one
>> go about implementing RMW inside a protected section.
> Hmm. Simran has also been progressing with patches for this.
>
I was trying to work through a patch for ade7754. So ran into the same
problem :)

> You raise a good question. There are other read/modify/write sequences in
> the driver.  They don't have the same issue with potentially deadlocking
> against the buf lock as they are all using the spi subsystems provisions
> for small write/read cycles where buffer protection is handled internally.
>
> So let us address the cases in turn:
>
> static int ade7753_reset(struct device *dev)
> {
>         u16 val;
>         int ret;
>
>         ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
>         if (ret)
>                 return ret;
>
>         val |= BIT(6); /* Software Chip Reset */
>
>         return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
> }
> This is only called in the device initialization.  At that point
> we should be fine in assuming no parallel calls.  Crucial point
> is it is before the call to iio_device_register which exposes
> the userspace interfaces.
>
> static int ade7753_set_irq(struct device *dev, bool enable)
> {
>         int ret;
>         u8 irqen;
>
>         ret = ade7753_spi_read_reg_8(dev, ADE7753_IRQEN, &irqen);
>         if (ret)
>                 goto error_ret;
>
>         if (enable)
>                 irqen |= BIT(3); /* Enables an interrupt when a data is
>                                   * present in the waveform register
>                                   */
>         else
>                 irqen &= ~BIT(3);
>
>         ret = ade7753_spi_write_reg_8(dev, ADE7753_IRQEN, irqen);
>
> error_ret:
>         return ret;
> }
>
> This one is actually safe because it is the only function that
> modifies that particular register.
>
> /* Power down the device */
> static int ade7753_stop_device(struct device *dev)
> {
>         u16 val;
>         int ret;
>
>         ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
>         if (ret)
>                 return ret;
>
>         val |= BIT(4);  /* AD converters can be turned off */
>
>         return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
> }
>
> Only called in remove (after userspace interfaces have been
> removed by the iio_device_unregister call so also should not
> be running concurrently with much else.
>

The only nested lock here is ade7754_spi_write_reg_16, so as long as
that is refactored, it'll be fine.

> So I think all the other cases are safe.  Perhaps it would have
> been better to have had a lock around them, purely to make
> the code more resilient against future changes though.
> Probably a job to do as part of a larger scale pile of work
> on that driver rather than as a one off patch.

Another question that I have is why are we writing inside a read
function(ade7754_spi_read_reg_24)?

static int ade7754_spi_read_reg_24(struct device *dev,
                                   u8 reg_address, u32 *val)
{
        struct iio_dev *indio_dev = dev_to_iio_dev(dev);
        struct ade7754_state *st = iio_priv(indio_dev);
        int ret;
        struct spi_transfer xfers[] = {
                {
                        .tx_buf = st->tx,
                        .rx_buf = st->rx,
                        .bits_per_word = 8,
                        .len = 4,
                },
        };

        mutex_lock(&st->buf_lock);
        st->tx[0] = ADE7754_READ_REG(reg_address);
        st->tx[1] = 0;
        st->tx[2] = 0;
        st->tx[3] = 0;

        ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
        if (ret) {
                dev_err(&st->us->dev, "problem when reading 24 bit
register 0x%02X",
                        reg_address);
                goto error_ret;
        }
        *val = (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];

error_ret:
        mutex_unlock(&st->buf_lock);
        return ret;
}

Thanks!
Gargi

>
> Jonathan
>
>
>
>
>
>
>
>>
>>
>>>
>>> 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.
>>
>> Am I right in understanding that we want to introduce mutex lock for
>> writes in other drivers as well?
>>
>> Thanks,
>> Gargi
>>>
>>>>   **/
>>>>  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;
>>>>  }
>>>>
>>>
>>> --
>> --
>> 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: [Outreachy kernel] Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Jonathan Cameron]

On 19/03/17 13:16, Gargi Sharma wrote:
> On Sun, Mar 19, 2017 at 4:01 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 17/03/17 09:32, Gargi Sharma 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.
>>>
>>> There are other read/write functions for example,
>>> ade7753_spi_{read/write}_reg_8 that use the mutex as well. Should a
>>> variant of these functions be introduced as well? Also, how does one
>>> go about implementing RMW inside a protected section.
>> Hmm. Simran has also been progressing with patches for this.
>>
> I was trying to work through a patch for ade7754. So ran into the same
> problem :)
> 
>> You raise a good question. There are other read/modify/write sequences in
>> the driver.  They don't have the same issue with potentially deadlocking
>> against the buf lock as they are all using the spi subsystems provisions
>> for small write/read cycles where buffer protection is handled internally.
>>
>> So let us address the cases in turn:
>>
>> static int ade7753_reset(struct device *dev)
>> {
>>         u16 val;
>>         int ret;
>>
>>         ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
>>         if (ret)
>>                 return ret;
>>
>>         val |= BIT(6); /* Software Chip Reset */
>>
>>         return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
>> }
>> This is only called in the device initialization.  At that point
>> we should be fine in assuming no parallel calls.  Crucial point
>> is it is before the call to iio_device_register which exposes
>> the userspace interfaces.
>>
>> static int ade7753_set_irq(struct device *dev, bool enable)
>> {
>>         int ret;
>>         u8 irqen;
>>
>>         ret = ade7753_spi_read_reg_8(dev, ADE7753_IRQEN, &irqen);
>>         if (ret)
>>                 goto error_ret;
>>
>>         if (enable)
>>                 irqen |= BIT(3); /* Enables an interrupt when a data is
>>                                   * present in the waveform register
>>                                   */
>>         else
>>                 irqen &= ~BIT(3);
>>
>>         ret = ade7753_spi_write_reg_8(dev, ADE7753_IRQEN, irqen);
>>
>> error_ret:
>>         return ret;
>> }
>>
>> This one is actually safe because it is the only function that
>> modifies that particular register.
>>
>> /* Power down the device */
>> static int ade7753_stop_device(struct device *dev)
>> {
>>         u16 val;
>>         int ret;
>>
>>         ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &val);
>>         if (ret)
>>                 return ret;
>>
>>         val |= BIT(4);  /* AD converters can be turned off */
>>
>>         return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
>> }
>>
>> Only called in remove (after userspace interfaces have been
>> removed by the iio_device_unregister call so also should not
>> be running concurrently with much else.
>>
> 
> The only nested lock here is ade7754_spi_write_reg_16, so as long as
> that is refactored, it'll be fine.
> 
>> So I think all the other cases are safe.  Perhaps it would have
>> been better to have had a lock around them, purely to make
>> the code more resilient against future changes though.
>> Probably a job to do as part of a larger scale pile of work
>> on that driver rather than as a one off patch.
> 
> Another question that I have is why are we writing inside a read
> function(ade7754_spi_read_reg_24)?
>
It's a register read (sort of) hence the reg in the name.
It's telling it which register to read by first writing that.
 
> static int ade7754_spi_read_reg_24(struct device *dev,
>                                    u8 reg_address, u32 *val)
> {
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>         struct ade7754_state *st = iio_priv(indio_dev);
>         int ret;
>         struct spi_transfer xfers[] = {
>                 {
>                         .tx_buf = st->tx,
>                         .rx_buf = st->rx,
>                         .bits_per_word = 8,
>                         .len = 4,
>                 },
>         };
> 
>         mutex_lock(&st->buf_lock);
>         st->tx[0] = ADE7754_READ_REG(reg_address);
>         st->tx[1] = 0;
>         st->tx[2] = 0;
>         st->tx[3] = 0;
> 
>         ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
>         if (ret) {
>                 dev_err(&st->us->dev, "problem when reading 24 bit
> register 0x%02X",
>                         reg_address);
>                 goto error_ret;
>         }
>         *val = (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
> 
> error_ret:
>         mutex_unlock(&st->buf_lock);
>         return ret;
> }
> 
> Thanks!
> Gargi
> 
>>
>> Jonathan
>>
>>
>>
>>
>>
>>
>>
>>>
>>>
>>>>
>>>> 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.
>>>
>>> Am I right in understanding that we want to introduce mutex lock for
>>> writes in other drivers as well?
>>>
>>> Thanks,
>>> Gargi
>>>>
>>>>>   **/
>>>>>  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;
>>>>>  }
>>>>>
>>>>
>>>> --
>>> --
>>> 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: [Outreachy kernel] Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Gargi Sharma]

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

The only instance I was able to find was drivers/iio/adc/vf610_adc.c
where read modify write cycles are present on the register
VF610_REG_ADC_CFG. I believe writel() & readl() is used for this
purpose.

In this case, we want to write to data to a device on the SPI bus. Can
we use writel() for this purpose?

> 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;
>>  }
>>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/6e55c61d-7587-4191-1fc5-de43e26986d7%40metafoo.de.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Jonathan Cameron]

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

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Himanshi Jain]

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

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Jonathan Cameron]

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;

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Greg KH]

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

[PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Himanshi Jain]

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