[RFC/PATCH v3] staging: iio: update locking method during frequency writes

[RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Gargi Sharma]

The driver needs to insure atomicity during frequency
changes of bus and device. The iiodev->mlock as used
was not doing that. Replace it with the drivers existing
buffer lock and introduce an auxiliary spi_write() that does
not hold the lock.

Signed-off-by: Gargi Sharma <gs051095@gmail.com>
---
Changes in v3:
        - Removed unnecessary ret variable inside
          __ade7754_spi_write_reg_8 function.
Changes in v2:
        - Added auxiliary function.
        - Updated the buf_lock comment appropriately.
---
 drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
index c8d2d4c..91f8740 100644
--- a/drivers/staging/iio/meter/ade7754.c
+++ b/drivers/staging/iio/meter/ade7754.c
@@ -97,7 +97,7 @@
 /**
  * struct ade7754_state - device instance specific data
  * @us:			actual spi_device
- * @buf_lock:		mutex to protect tx and rx
+ * @buf_lock:		mutex to protect tx, rx and write frequency
  * @tx:			transmit buffer
  * @rx:			receive buffer
  **/
@@ -108,17 +108,25 @@ struct ade7754_state {
 	u8			rx[ADE7754_MAX_RX];
 };
 
-static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
+/* Unlocked version of ade7754_spi_write_reg_8 function */
+static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
 {
-	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ade7754_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->buf_lock);
 	st->tx[0] = ADE7754_WRITE_REG(reg_address);
 	st->tx[1] = val;
+	return spi_write(st->us, st->tx, 2);
+}
 
-	ret = spi_write(st->us, st->tx, 2);
+static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
+{
+	int ret;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ade7754_state *st = iio_priv(indio_dev);
+
+	mutex_lock(&st->buf_lock);
+	ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
@@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->buf_lock);
 
 	t = 26000 / val;
 	if (t > 0)
@@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
 	reg &= ~(3 << 3);
 	reg |= t << 3;
 
-	ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
+	ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
 
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->buf_lock);
 
 	return ret ? ret : len;
 }
-- 
2.7.4

Re: [RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Jonathan Cameron]

On 30/03/17 10:33, Gargi Sharma wrote:
> The driver needs to insure atomicity during frequency
> changes of bus and device. The iiodev->mlock as used
> was not doing that. Replace it with the drivers existing
> buffer lock and introduce an auxiliary spi_write() that does
> not hold the lock.
> 
> Signed-off-by: Gargi Sharma <gs051095@gmail.com>
Only one comment on this.  Why an RFC?  I nearly
ignored this in my sweep of easy patches to pick up
this evening purely because I thought there might still
be something that needed real thought in it.

It's a clean nice patch that I don't think any substantial
questions have been raised against - so just submit it as
patch. As an RFC it is underselling itself!

Anyhow, never mind, I did look and have applied it ;)

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

Thanks.

Jonathan
> ---
> Changes in v3:
>         - Removed unnecessary ret variable inside
>           __ade7754_spi_write_reg_8 function.
> Changes in v2:
>         - Added auxiliary function.
>         - Updated the buf_lock comment appropriately.
> ---
>  drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
> index c8d2d4c..91f8740 100644
> --- a/drivers/staging/iio/meter/ade7754.c
> +++ b/drivers/staging/iio/meter/ade7754.c
> @@ -97,7 +97,7 @@
>  /**
>   * struct ade7754_state - device instance specific data
>   * @us:			actual spi_device
> - * @buf_lock:		mutex to protect tx and rx
> + * @buf_lock:		mutex to protect tx, rx and write frequency
>   * @tx:			transmit buffer
>   * @rx:			receive buffer
>   **/
> @@ -108,17 +108,25 @@ struct ade7754_state {
>  	u8			rx[ADE7754_MAX_RX];
>  };
>  
> -static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +/* Unlocked version of ade7754_spi_write_reg_8 function */
> +static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>  {
> -	int ret;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ade7754_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->buf_lock);
>  	st->tx[0] = ADE7754_WRITE_REG(reg_address);
>  	st->tx[1] = val;
> +	return spi_write(st->us, st->tx, 2);
> +}
>  
> -	ret = spi_write(st->us, st->tx, 2);
> +static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ade7754_state *st = iio_priv(indio_dev);
> +
> +	mutex_lock(&st->buf_lock);
> +	ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
> @@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>  	if (!val)
>  		return -EINVAL;
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->buf_lock);
>  
>  	t = 26000 / val;
>  	if (t > 0)
> @@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>  	reg &= ~(3 << 3);
>  	reg |= t << 3;
>  
> -	ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
> +	ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>  
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->buf_lock);
>  
>  	return ret ? ret : len;
>  }
> 

Re: [RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Alison Schofield]

On Thu, Mar 30, 2017 at 07:22:49PM +0100, Jonathan Cameron wrote:
> On 30/03/17 10:33, Gargi Sharma wrote:
> > The driver needs to insure atomicity during frequency
> > changes of bus and device. The iiodev->mlock as used
> > was not doing that. Replace it with the drivers existing
> > buffer lock and introduce an auxiliary spi_write() that does
> > not hold the lock.
> > 
> > Signed-off-by: Gargi Sharma <gs051095@gmail.com>
> Only one comment on this.  Why an RFC?  I nearly
> ignored this in my sweep of easy patches to pick up
> this evening purely because I thought there might still
> be something that needed real thought in it.

Oh, that was my bad influence!
We were reviewing proposals for these in Outreachy and I'd
suggested the RFC label for the proposals.  Thanks for
addressing it.
alisons

> 
> It's a clean nice patch that I don't think any substantial
> questions have been raised against - so just submit it as
> patch. As an RFC it is underselling itself!
> 
> Anyhow, never mind, I did look and have applied it ;)
> 
> Applied to the togreg branch of iio.git and pushed out
> as testing for the autobuilders to play with it.
> 
> Thanks.
> 
> Jonathan
> > ---
> > Changes in v3:
> >         - Removed unnecessary ret variable inside
> >           __ade7754_spi_write_reg_8 function.
> > Changes in v2:
> >         - Added auxiliary function.
> >         - Updated the buf_lock comment appropriately.
> > ---
> >  drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
> > index c8d2d4c..91f8740 100644
> > --- a/drivers/staging/iio/meter/ade7754.c
> > +++ b/drivers/staging/iio/meter/ade7754.c
> > @@ -97,7 +97,7 @@
> >  /**
> >   * struct ade7754_state - device instance specific data
> >   * @us:			actual spi_device
> > - * @buf_lock:		mutex to protect tx and rx
> > + * @buf_lock:		mutex to protect tx, rx and write frequency
> >   * @tx:			transmit buffer
> >   * @rx:			receive buffer
> >   **/
> > @@ -108,17 +108,25 @@ struct ade7754_state {
> >  	u8			rx[ADE7754_MAX_RX];
> >  };
> >  
> > -static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> > +/* Unlocked version of ade7754_spi_write_reg_8 function */
> > +static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> >  {
> > -	int ret;
> >  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >  	struct ade7754_state *st = iio_priv(indio_dev);
> >  
> > -	mutex_lock(&st->buf_lock);
> >  	st->tx[0] = ADE7754_WRITE_REG(reg_address);
> >  	st->tx[1] = val;
> > +	return spi_write(st->us, st->tx, 2);
> > +}
> >  
> > -	ret = spi_write(st->us, st->tx, 2);
> > +static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> > +{
> > +	int ret;
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct ade7754_state *st = iio_priv(indio_dev);
> > +
> > +	mutex_lock(&st->buf_lock);
> > +	ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
> >  	mutex_unlock(&st->buf_lock);
> >  
> >  	return ret;
> > @@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
> >  	if (!val)
> >  		return -EINVAL;
> >  
> > -	mutex_lock(&indio_dev->mlock);
> > +	mutex_lock(&st->buf_lock);
> >  
> >  	t = 26000 / val;
> >  	if (t > 0)
> > @@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
> >  	reg &= ~(3 << 3);
> >  	reg |= t << 3;
> >  
> > -	ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
> > +	ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
> >  
> >  out:
> > -	mutex_unlock(&indio_dev->mlock);
> > +	mutex_unlock(&st->buf_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: [RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Jonathan Cameron]

On 30/03/17 19:25, Alison Schofield wrote:
> On Thu, Mar 30, 2017 at 07:22:49PM +0100, Jonathan Cameron wrote:
>> On 30/03/17 10:33, Gargi Sharma wrote:
>>> The driver needs to insure atomicity during frequency
>>> changes of bus and device. The iiodev->mlock as used
>>> was not doing that. Replace it with the drivers existing
>>> buffer lock and introduce an auxiliary spi_write() that does
>>> not hold the lock.
>>>
>>> Signed-off-by: Gargi Sharma <gs051095@gmail.com>
>> Only one comment on this.  Why an RFC?  I nearly
>> ignored this in my sweep of easy patches to pick up
>> this evening purely because I thought there might still
>> be something that needed real thought in it.
> 
> Oh, that was my bad influence!
> We were reviewing proposals for these in Outreachy and I'd
> suggested the RFC label for the proposals.  Thanks for
> addressing it.
> alisons
Was fair enough for round one - though saying why it might
need comments in the patch description is always good.

By V3 with all positive comments, probably fine to loose the
RFC :)
> 
>>
>> It's a clean nice patch that I don't think any substantial
>> questions have been raised against - so just submit it as
>> patch. As an RFC it is underselling itself!
>>
>> Anyhow, never mind, I did look and have applied it ;)
>>
>> Applied to the togreg branch of iio.git and pushed out
>> as testing for the autobuilders to play with it.
>>
>> Thanks.
>>
>> Jonathan
>>> ---
>>> Changes in v3:
>>>         - Removed unnecessary ret variable inside
>>>           __ade7754_spi_write_reg_8 function.
>>> Changes in v2:
>>>         - Added auxiliary function.
>>>         - Updated the buf_lock comment appropriately.
>>> ---
>>>  drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
>>> index c8d2d4c..91f8740 100644
>>> --- a/drivers/staging/iio/meter/ade7754.c
>>> +++ b/drivers/staging/iio/meter/ade7754.c
>>> @@ -97,7 +97,7 @@
>>>  /**
>>>   * struct ade7754_state - device instance specific data
>>>   * @us:			actual spi_device
>>> - * @buf_lock:		mutex to protect tx and rx
>>> + * @buf_lock:		mutex to protect tx, rx and write frequency
>>>   * @tx:			transmit buffer
>>>   * @rx:			receive buffer
>>>   **/
>>> @@ -108,17 +108,25 @@ struct ade7754_state {
>>>  	u8			rx[ADE7754_MAX_RX];
>>>  };
>>>  
>>> -static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>> +/* Unlocked version of ade7754_spi_write_reg_8 function */
>>> +static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>  {
>>> -	int ret;
>>>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>  	struct ade7754_state *st = iio_priv(indio_dev);
>>>  
>>> -	mutex_lock(&st->buf_lock);
>>>  	st->tx[0] = ADE7754_WRITE_REG(reg_address);
>>>  	st->tx[1] = val;
>>> +	return spi_write(st->us, st->tx, 2);
>>> +}
>>>  
>>> -	ret = spi_write(st->us, st->tx, 2);
>>> +static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>> +{
>>> +	int ret;
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct ade7754_state *st = iio_priv(indio_dev);
>>> +
>>> +	mutex_lock(&st->buf_lock);
>>> +	ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
>>>  	mutex_unlock(&st->buf_lock);
>>>  
>>>  	return ret;
>>> @@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>  	if (!val)
>>>  		return -EINVAL;
>>>  
>>> -	mutex_lock(&indio_dev->mlock);
>>> +	mutex_lock(&st->buf_lock);
>>>  
>>>  	t = 26000 / val;
>>>  	if (t > 0)
>>> @@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>  	reg &= ~(3 << 3);
>>>  	reg |= t << 3;
>>>  
>>> -	ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>> +	ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>>  
>>>  out:
>>> -	mutex_unlock(&indio_dev->mlock);
>>> +	mutex_unlock(&st->buf_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
> --
> 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: [RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Gargi Sharma]

On Thu, Mar 30, 2017 at 11:58 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 30/03/17 19:25, Alison Schofield wrote:
>> On Thu, Mar 30, 2017 at 07:22:49PM +0100, Jonathan Cameron wrote:
>>> On 30/03/17 10:33, Gargi Sharma wrote:
>>>> The driver needs to insure atomicity during frequency
>>>> changes of bus and device. The iiodev->mlock as used
>>>> was not doing that. Replace it with the drivers existing
>>>> buffer lock and introduce an auxiliary spi_write() that does
>>>> not hold the lock.
>>>>
>>>> Signed-off-by: Gargi Sharma <gs051095@gmail.com>
>>> Only one comment on this.  Why an RFC?  I nearly
>>> ignored this in my sweep of easy patches to pick up
>>> this evening purely because I thought there might still
>>> be something that needed real thought in it.
>>
>> Oh, that was my bad influence!
>> We were reviewing proposals for these in Outreachy and I'd
>> suggested the RFC label for the proposals.  Thanks for
>> addressing it.
>> alisons
> Was fair enough for round one - though saying why it might
> need comments in the patch description is always good.
>
> By V3 with all positive comments, probably fine to loose the
> RFC :)

I was skeptical of the RFC tag hence put the PATCH in subject prefix
as well. :) Anyways, lesson learnt for future!

Thanks,
Gargi
>>
>>>
>>> It's a clean nice patch that I don't think any substantial
>>> questions have been raised against - so just submit it as
>>> patch. As an RFC it is underselling itself!
>>>
>>> Anyhow, never mind, I did look and have applied it ;)
>>>
>>> Applied to the togreg branch of iio.git and pushed out
>>> as testing for the autobuilders to play with it.
>>>
>>> Thanks.
>>>
>>> Jonathan
>>>> ---
>>>> Changes in v3:
>>>>         - Removed unnecessary ret variable inside
>>>>           __ade7754_spi_write_reg_8 function.
>>>> Changes in v2:
>>>>         - Added auxiliary function.
>>>>         - Updated the buf_lock comment appropriately.
>>>> ---
>>>>  drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
>>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
>>>> index c8d2d4c..91f8740 100644
>>>> --- a/drivers/staging/iio/meter/ade7754.c
>>>> +++ b/drivers/staging/iio/meter/ade7754.c
>>>> @@ -97,7 +97,7 @@
>>>>  /**
>>>>   * struct ade7754_state - device instance specific data
>>>>   * @us:                    actual spi_device
>>>> - * @buf_lock:              mutex to protect tx and rx
>>>> + * @buf_lock:              mutex to protect tx, rx and write frequency
>>>>   * @tx:                    transmit buffer
>>>>   * @rx:                    receive buffer
>>>>   **/
>>>> @@ -108,17 +108,25 @@ struct ade7754_state {
>>>>     u8                      rx[ADE7754_MAX_RX];
>>>>  };
>>>>
>>>> -static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>> +/* Unlocked version of ade7754_spi_write_reg_8 function */
>>>> +static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>>  {
>>>> -   int ret;
>>>>     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>     struct ade7754_state *st = iio_priv(indio_dev);
>>>>
>>>> -   mutex_lock(&st->buf_lock);
>>>>     st->tx[0] = ADE7754_WRITE_REG(reg_address);
>>>>     st->tx[1] = val;
>>>> +   return spi_write(st->us, st->tx, 2);
>>>> +}
>>>>
>>>> -   ret = spi_write(st->us, st->tx, 2);
>>>> +static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>> +{
>>>> +   int ret;
>>>> +   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>> +   struct ade7754_state *st = iio_priv(indio_dev);
>>>> +
>>>> +   mutex_lock(&st->buf_lock);
>>>> +   ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
>>>>     mutex_unlock(&st->buf_lock);
>>>>
>>>>     return ret;
>>>> @@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>>     if (!val)
>>>>             return -EINVAL;
>>>>
>>>> -   mutex_lock(&indio_dev->mlock);
>>>> +   mutex_lock(&st->buf_lock);
>>>>
>>>>     t = 26000 / val;
>>>>     if (t > 0)
>>>> @@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>>     reg &= ~(3 << 3);
>>>>     reg |= t << 3;
>>>>
>>>> -   ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>>> +   ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>>>
>>>>  out:
>>>> -   mutex_unlock(&indio_dev->mlock);
>>>> +   mutex_unlock(&st->buf_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
>> --
>> 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
>>
>