[PATCH v3 0/2] Replace mlock with private lock and delete whitespaces

[PATCH v3 0/2] Replace mlock with private lock and delete whitespaces

[simran singhal]

The patch series replaces mlock with a private lock for driver ad9834 and
Fix coding style issues related to white spaces.

v3:
  -Using new private "lock" instead of using "buf_lock"
   as it can cause deadlock.
  -Sending it as a series of two patches.

v2:
  -Using the existing buf_lock instead of lock.
   

simran singhal (2):
  staging: iio: ade7753: Remove trailing whitespaces
  staging: iio: ade7753: Replace mlock with driver private lock

 drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] staging: iio: ade7753: Remove trailing whitespaces

[simran singhal]

This patch removes trailing whitespaces in order to follow the Linux
coding style.

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

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index dfd8b71..b71fbd3 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -83,10 +83,10 @@
  * @buf_lock:       mutex to protect tx and rx
  **/
 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;
+	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
+	u8          rx[ADE7753_MAX_RX];
 };
 
 static int ade7753_spi_write_reg_8(struct device *dev,
-- 
2.7.4

[PATCH v3 2/2] 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.

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

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index b71fbd3..9674e05 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -80,11 +80,13 @@
  * @us:         actual spi_device
  * @tx:         transmit buffer
  * @rx:         receive buffer
+ * @lock:       protect sensor state
  * @buf_lock:       mutex to protect tx and rx
  **/
 struct ade7753_state {
 	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];
 };
@@ -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 v3 0/2] Replace mlock with private lock and delete whitespaces

[Alison Schofield]

On Tue, Mar 21, 2017 at 11:33:54PM +0530, simran singhal wrote:
> The patch series replaces mlock with a private lock for driver ad9834 and
> Fix coding style issues related to white spaces.

Hi Simran,  

I'm getting lost.  Patchset Subject Line needs subsystem and driver.
The comment above says ad9834 but the patches below say ade7753.

Can we drive adis16060 through ACK and then come back to this one?
(ie. applyling lessons learned)

thanks,
alisons
> 
> v3:
>   -Using new private "lock" instead of using "buf_lock"
>    as it can cause deadlock.
>   -Sending it as a series of two patches.
> 
> v2:
>   -Using the existing buf_lock instead of lock.
>    
> 
> simran singhal (2):
>   staging: iio: ade7753: Remove trailing whitespaces
>   staging: iio: ade7753: Replace mlock with driver private lock
> 
>  drivers/staging/iio/meter/ade7753.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> -- 
> 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/1490119436-20042-1-git-send-email-singhalsimran0%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v3 1/2] staging: iio: ade7753: Remove trailing whitespaces

[Jonathan Cameron]

On 21/03/17 18:03, simran singhal wrote:
> This patch removes trailing whitespaces in order to follow the Linux
> coding style.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
Applied and pushed out as testing...

Jonathan
> ---
>  drivers/staging/iio/meter/ade7753.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..b71fbd3 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -83,10 +83,10 @@
>   * @buf_lock:       mutex to protect tx and rx
>   **/
>  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;
> +	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
> +	u8          rx[ADE7753_MAX_RX];
>  };
>  
>  static int ade7753_spi_write_reg_8(struct device *dev,
> 

Re: [PATCH v3 2/2] staging: iio: ade7753: Replace mlock with driver private lock

[Jonathan Cameron]

On 21/03/17 18:03, 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.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
Mutex is not initialized...
> ---
>  drivers/staging/iio/meter/ade7753.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index b71fbd3..9674e05 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -80,11 +80,13 @@
>   * @us:         actual spi_device
>   * @tx:         transmit buffer
>   * @rx:         receive buffer
> + * @lock:       protect sensor state
>   * @buf_lock:       mutex to protect tx and rx
>   **/
>  struct ade7753_state {
>  	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];
>  };
> @@ -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 v3 2/2] staging: iio: ade7753: Replace mlock with driver private lock

[SIMRAN SINGHAL]

On Thu, Mar 23, 2017 at 1:55 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 21/03/17 18:03, 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.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> Mutex is not initialized...

Mutex is already initialized in ade7753_probe().

>> ---
>>  drivers/staging/iio/meter/ade7753.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>> index b71fbd3..9674e05 100644
>> --- a/drivers/staging/iio/meter/ade7753.c
>> +++ b/drivers/staging/iio/meter/ade7753.c
>> @@ -80,11 +80,13 @@
>>   * @us:         actual spi_device
>>   * @tx:         transmit buffer
>>   * @rx:         receive buffer
>> + * @lock:       protect sensor state
>>   * @buf_lock:       mutex to protect tx and rx
>>   **/
>>  struct ade7753_state {
>>       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];
>>  };
>> @@ -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 v3 2/2] staging: iio: ade7753: Replace mlock with driver private lock

[Jonathan Cameron]

On 23 March 2017 18:12:33 GMT+00:00, SIMRAN SINGHAL <singhalsimran0@gmail.com> wrote:
>On Thu, Mar 23, 2017 at 1:55 AM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> On 21/03/17 18:03, 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.
>>>
>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> Mutex is not initialized...
>
>Mutex is already initialized in ade7753_probe().
Given you introduce a new mutex it seems unlikely that one is. You have to initialise each mutex.
>
>>> ---
>>>  drivers/staging/iio/meter/ade7753.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7753.c
>b/drivers/staging/iio/meter/ade7753.c
>>> index b71fbd3..9674e05 100644
>>> --- a/drivers/staging/iio/meter/ade7753.c
>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>> @@ -80,11 +80,13 @@
>>>   * @us:         actual spi_device
>>>   * @tx:         transmit buffer
>>>   * @rx:         receive buffer
>>> + * @lock:       protect sensor state
>>>   * @buf_lock:       mutex to protect tx and rx
>>>   **/
>>>  struct ade7753_state {
>>>       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];
>>>  };
>>> @@ -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;
>>>  }
>>>
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3 2/2] staging: iio: ade7753: Replace mlock with driver private lock

[Jonathan Cameron]

On 23 March 2017 18:12:33 GMT+00:00, SIMRAN SINGHAL <singhalsimran0@gmail.com> wrote:
>On Thu, Mar 23, 2017 at 1:55 AM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> On 21/03/17 18:03, 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.
>>>
>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> Mutex is not initialized...
>
>Mutex is already initialized in ade7753_probe().
Given you introduce a new mutex it seems unlikely that one is. You have to initialise each mutex.
>
>>> ---
>>>  drivers/staging/iio/meter/ade7753.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7753.c
>b/drivers/staging/iio/meter/ade7753.c
>>> index b71fbd3..9674e05 100644
>>> --- a/drivers/staging/iio/meter/ade7753.c
>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>> @@ -80,11 +80,13 @@
>>>   * @us:         actual spi_device
>>>   * @tx:         transmit buffer
>>>   * @rx:         receive buffer
>>> + * @lock:       protect sensor state
>>>   * @buf_lock:       mutex to protect tx and rx
>>>   **/
>>>  struct ade7753_state {
>>>       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];
>>>  };
>>> @@ -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;
>>>  }
>>>
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.