Skip to contentSkip to conversation

staging: iio: ade7753: Replace mlock with driver private lock

PATCH In review simran singhal 4 participants 8 messages Mar 23, 2017 – Mar 30, 2017

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io
lore
Share & tools
lore

simran singhal

author
#0 lore raw

PATCH v4 1/1 staging: iio: ade7753: Replace mlock with driver private lock

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 <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -2
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
Signature · 1 line
2.7.4
#1 lore raw
Quoted 6 lines · 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.

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Does this lock enforce the needed "atomicity" in the write_frequency function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the device are changed in an atomic fashion"

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

alisons

Quoted 63 lines
Signed-off-by simran singhal <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -2
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
Signature · 8 lines
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 [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
For more options, visit https://groups.google.com/d/optout.
author
#2 lore raw
Quoted 5 lines · Alison Schofield
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 2 lines

In this driver, mlock was being used to protect hardware state changes. Replace it with a lock in the devices global data.

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Sorry, will not repeat this.

Quoted 5 lines

Does this lock enforce the needed "atomicity" in the write_frequency function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the device are changed in an atomic fashion"

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are about to write.

Quoted 3 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution.
https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Should I do like her as her's also seem correct or go ahead with this.

Quoted 64 lines

alisons

Quoted 63 lines
Signed-off-by simran singhal <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -3
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
--

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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
For more options, visit https://groups.google.com/d/optout.

#3 lore raw
Quoted 22 lines · SIMRAN SINGHAL
Quoted 5 lines · Alison Schofield
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 2 lines

In this driver, mlock was being used to protect hardware state changes. Replace it with a lock in the devices global data.

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Sorry, will not repeat this.

Quoted 5 lines

Does this lock enforce the needed "atomicity" in the write_frequency function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the device are changed in an atomic fashion"

Quoted 3 lines

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are about to write.

I see it protecting this path from being re-entered. My uncertainty is about other paths to read/write.

Quoted 9 lines
Quoted 4 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution.
https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Should I do like her as her's also seem correct or go ahead with this.

My suggestion would be to wait for feedback on Gargi's patch. (See the Outreachy log about creating similar solutions.)

We will not be able to close on this set of patches during the Outreachy application window. You can continue to push for closure beyond the March 30th date as your time allows :)

Thanks,
alisons

Quoted 65 lines
Quoted 65 lines

alisons

Quoted 63 lines
Signed-off-by simran singhal <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -3
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
--

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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
For more options, visit https://groups.google.com/d/optout.

maintainer
#4 lore raw
Quoted 38 lines · Alison Schofield
Quoted 22 lines · SIMRAN SINGHAL
Quoted 5 lines · Alison Schofield
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 2 lines

In this driver, mlock was being used to protect hardware state changes. Replace it with a lock in the devices global data.

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Sorry, will not repeat this.

Quoted 5 lines

Does this lock enforce the needed "atomicity" in the write_frequency function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the device are changed in an atomic fashion"

Quoted 3 lines

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are about to write.

I see it protecting this path from being re-entered. My uncertainty is about other paths to read/write.

Quoted 8 lines
Quoted 4 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution.
https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Quoted 9 lines
Quoted 1 line

Should I do like her as her's also seem correct or go ahead with this.

My suggestion would be to wait for feedback on Gargi's patch. (See the Outreachy log about creating similar solutions.)

We will not be able to close on this set of patches during the Outreachy application window. You can continue to push for closure beyond the March 30th date as your time allows :)

It is a close choice between the two approaches. In some ways yours is easier to follow, but Gargi's is more elegant.

Lets go with that one for consistency across similar drivers, but if you had been the original author and done it this way I certainly wouldn't bother asking you to change it!

So in conclusion both patches are good.

Jonathan

Quoted 68 lines

Thanks,
alisons

Quoted 65 lines
Quoted 65 lines

alisons

Quoted 63 lines
Signed-off-by simran singhal <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -3
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
--

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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
For more options, visit https://groups.google.com/d/optout.

author
#5 lore raw
Quoted 48 lines · Jonathan Cameron
Quoted 38 lines · Alison Schofield
Quoted 22 lines · SIMRAN SINGHAL
Quoted 5 lines · Alison Schofield
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 2 lines

In this driver, mlock was being used to protect hardware state changes. Replace it with a lock in the devices global data.

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Sorry, will not repeat this.

Quoted 5 lines

Does this lock enforce the needed "atomicity" in the write_frequency function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the device are changed in an atomic fashion"

Quoted 3 lines

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are about to write.

I see it protecting this path from being re-entered. My uncertainty is about other paths to read/write.

Quoted 8 lines
Quoted 4 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution.
https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Quoted 9 lines
Quoted 1 line

Should I do like her as her's also seem correct or go ahead with this.

My suggestion would be to wait for feedback on Gargi's patch. (See the Outreachy log about creating similar solutions.)

We will not be able to close on this set of patches during the Outreachy application window. You can continue to push for closure beyond the March 30th date as your time allows :)

Quoted 6 lines

It is a close choice between the two approaches. In some ways yours is easier to follow, but Gargi's is more elegant.

Lets go with that one for consistency across similar drivers, but if you had been the original author and done it this way I certainly wouldn't bother asking you to change it!

Yes, jonathan I am the original author.

Quoted 74 lines

So in conclusion both patches are good.

Jonathan

Quoted 68 lines

Thanks,
alisons

Quoted 65 lines
Quoted 65 lines

alisons

Quoted 63 lines
Signed-off-by simran singhal <[email protected]>

---

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/staging/iio/meter/ade7753.c +5 -3
index b71fbd3..30aebaf 100644
@@ -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;
 }

@@ -581,6 +583,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;
--

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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
For more options, visit https://groups.google.com/d/optout.

#6 lore raw
Quoted 56 lines · On 30 March 2017 19:44:26 BST, SIMRAN SINGHAL <singhalsimran0@gmail=2Ecom>=…
Quoted 3 lines · Jonathan Cameron
Quoted 2 lines · Alison Schofield
Quoted 1 line · SIMRAN SINGHAL

On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield

Quoted 8 lines · <amsfield22@gmail=2Ecom> wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · simran singhal
Quoted 6 lines

The IIO subsystem is redefining iio_dev->mlock to be used by the IIO core only for protecting device operating mode changes=2E ie=2E Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes=2E

In this driver, mlock was being used to protect hardware state changes=2E Replace it with a lock in the devices global data=2E

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

Hi Simran,

Please post all revision histories below the --- not just the most recent=2E

Sorry, will not repeat this=2E

Quoted 1 line

Does this lock enforce the needed "atomicity" in the

write_frequency

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the

device

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

are changed in an atomic fashion"

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are

about

Quoted 6 lines
Quoted 6 lines
Quoted 1 line

to write=2E

I see it protecting this path from being re-entered=2E My uncertainty is about other paths to read/write=2E

Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution=2E

https://groups=2Egoogle=2Ecom/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Should I do like her as her's also seem correct or go ahead with

this=2E

Quoted 8 lines
Quoted 8 lines

My suggestion would be to wait for feedback on Gargi's patch=2E (See the Outreachy log about creating similar solutions=2E)

We will not be able to close on this set of patches during the Outreachy application window=2E You can continue to push for closure beyond the March 30th date as your time allows :)

Quoted 8 lines
Quoted 6 lines

It is a close choice between the two approaches=2E In some ways yours is easier to follow, but Gargi's is more elegant=2E

Lets go with that one for consistency across similar drivers, but if you had been the original author and done it this way I certainly wouldn't bother asking you to change it!

Yes, jonathan I am the original author=2E

Sorry, I meant of the driver rather than this improvement=2E

Jonathan

Quoted 74 lines
Quoted 15 lines

So in conclusion both patches are good=2E

Jonathan

Quoted 9 lines

Thanks,
alisons

Quoted 6 lines
Quoted 6 lines

alisons

Quoted 4 lines
Signed-off-by simran singhal <singhalsimran0@gmail=2Ecom>

---

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Review

Quoted 7 lines

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753=2Ec | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/staging/iio/meter/ade7753=2Ec

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

index b71fbd3=2E=2E30aebaf 100644

drivers/staging/iio/meter/ade7753=2Ec +1 -0
@@ -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
  **/
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

drivers/staging/iio/meter/ade7753=2Ec static ssize_t ade7753_write_frequency(struct

Quoted 8 lines
drivers/staging/iio/meter/ade7753=2Ec +1 -0
 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,

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/staging/iio/meter/ade7753=2Ec static ssize_t ade7753_write_frequency(struct

Quoted 9 lines
drivers/staging/iio/meter/ade7753=2Ec +1 -1
      if (!val)
              return -EINVAL;
 
-     mutex_lock(&indio_dev->mlock);
+     mutex_lock(&st->lock);
 
      t =3D 27900 / val;
      if (t > 0)

@@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct

device *dev,

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/staging/iio/meter/ade7753=2Ec static int ade7753_probe(struct spi_device

Quoted 9 lines
drivers/staging/iio/meter/ade7753=2Ec +1 -1
      ret =3D ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
 
 out:
-     mutex_unlock(&indio_dev->mlock);
+     mutex_unlock(&st->lock);
 
      return ret ? ret : len;
 }

@@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device

*spi)

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/meter/ade7753=2Ec +1 -0
      st =3D iio_priv(indio_dev);
      st->us =3D spi;
      mutex_init(&st->buf_lock);
+     mutex_init(&st->lock);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/staging/iio/meter/ade7753=2Ec +0 -1
      indio_dev->name =3D spi->dev=2Edriver->name;
      indio_dev->dev=2Eparent =3D &spi->dev;
--

2=2E7=2E4

--
You received this message because you are subscribed to the

Quoted 7 lines

Google Groups "outreachy-kernel" group=2E

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To unsubscribe from this group and stop receiving emails from it,

send an email to outreachy-kernel+unsubscribe@googlegroups=2Ecom=2E

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To post to this group, send email to

outreachy-kernel@googlegroups=2Ecom=2E

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To view this discussion on the web visit

https://groups=2Egoogle=2Ecom/d/msgid/outreachy-kernel/20170323183520=2EG=

A9871%40singhal-Inspiron-5558=2E

Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

For more options, visit https://groups=2Egoogle=2Ecom/d/optout=2E

--=20
Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E

author
#7 lore raw
Quoted 62 lines · Jonathan Cameron
Quoted 59 lines · SIMRAN SINGHAL
Quoted 3 lines · Jonathan Cameron
Quoted 2 lines · Alison Schofield
Quoted 1 line · SIMRAN SINGHAL

On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield

Quoted 8 lines · <[email protected]> wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · simran singhal
Quoted 6 lines

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.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

Hi Simran,

Please post all revision histories below the --- not just the most recent.

Sorry, will not repeat this.

Quoted 1 line

Does this lock enforce the needed "atomicity" in the

write_frequency

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

function? I read Jonathans comment on a previous revision about "ensuring the spi bus frequency and sampling frequency of the

device

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

are changed in an atomic fashion"

By introducing another lock I am protecting read_modify_write and in this way also protecting the designated register that we are

about

Quoted 6 lines
Quoted 6 lines
Quoted 1 line

to write.

I see it protecting this path from being re-entered. My uncertainty is about other paths to read/write.

Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Is it possible for another spi bus transaction (read or write) to occur between the read and write in write_frequency?

Gargi has also come up with a solution.

https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Should I do like her as her's also seem correct or go ahead with

this.

Quoted 11 lines
Quoted 8 lines

My suggestion would be to wait for feedback on Gargi's patch. (See the Outreachy log about creating similar solutions.)

We will not be able to close on this set of patches during the Outreachy application window. You can continue to push for closure beyond the March 30th date as your time allows :)

It is a close choice between the two approaches. In some ways yours is easier to follow, but Gargi's is more elegant.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Lets go with that one for consistency across similar drivers, but if you had been the original author and done it this way I certainly wouldn't bother asking you to change it!

Yes, jonathan I am the original author.

Sorry, I meant of the driver rather than this improvement.

By reading your pervious comment, I got what you mean!! For consistency, I will do it in the same way Gargi did.

Quoted 87 lines

Jonathan

Quoted 74 lines
Quoted 15 lines

So in conclusion both patches are good.

Jonathan

Quoted 9 lines

Thanks,
alisons

Quoted 6 lines
Quoted 6 lines

alisons

Quoted 4 lines
Signed-off-by simran singhal <[email protected]>

---

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Review

Quoted 7 lines

v4:
-Add mutex_init

 drivers/staging/iio/meter/ade7753.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/staging/iio/meter/ade7753.c

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

index b71fbd3..30aebaf 100644

drivers/staging/iio/meter/ade7753.c +1 -0
@@ -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
  **/
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

drivers/staging/iio/meter/ade7753.c static ssize_t ade7753_write_frequency(struct

Quoted 8 lines
drivers/staging/iio/meter/ade7753.c +1 -0
 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,

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/staging/iio/meter/ade7753.c static ssize_t ade7753_write_frequency(struct

Quoted 9 lines
drivers/staging/iio/meter/ade7753.c +1 -1
      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,

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/staging/iio/meter/ade7753.c static int ade7753_probe(struct spi_device

Quoted 9 lines
drivers/staging/iio/meter/ade7753.c +1 -1
      ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
 
 out:
-     mutex_unlock(&indio_dev->mlock);
+     mutex_unlock(&st->lock);
 
      return ret ? ret : len;
 }

@@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device

*spi)

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/meter/ade7753.c +1 -0
      st = iio_priv(indio_dev);
      st->us = spi;
      mutex_init(&st->buf_lock);
+     mutex_init(&st->lock);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/staging/iio/meter/ade7753.c +0 -1
      indio_dev->name = spi->dev.driver->name;
      indio_dev->dev.parent = &spi->dev;
--

2.7.4

--
You received this message because you are subscribed to the

Quoted 9 lines

Google Groups "outreachy-kernel" group.

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To unsubscribe from this group and stop receiving emails from it,

send an email to [email protected].

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To post to this group, send email to

[email protected].

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

To view this discussion on the web visit

https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.

Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

For more options, visit https://groups.google.com/d/optout.

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