* [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock @ 2017-03-23 18:35 simran singhal 2017-03-23 19:21 ` [Outreachy kernel] " Alison Schofield 0 siblings, 1 reply; 10+ messages in thread From: simran singhal @ 2017-03-23 18:35 UTC (permalink / raw) To: lars Cc: Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, outreachy-kernel The IIO subsystem is redefining iio_dev->mlock to be used by the IIO core only for protecting device operating mode changes. ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. In this driver, mlock was being used to protect hardware state changes. Replace it with a lock in the devices global data. Signed-off-by: simran singhal <singhalsimran0@gmail.com> --- v4: -Add mutex_init 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 b71fbd3..30aebaf 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; } @@ -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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-23 18:35 [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock simran singhal @ 2017-03-23 19:21 ` Alison Schofield 2017-03-28 17:25 ` SIMRAN SINGHAL 0 siblings, 1 reply; 10+ messages in thread From: Alison Schofield @ 2017-03-23 19:21 UTC (permalink / raw) To: simran singhal Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, outreachy-kernel On Fri, Mar 24, 2017 at 12:05:20AM +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. 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 > > Signed-off-by: simran singhal <singhalsimran0@gmail.com> > --- > > v4: > -Add mutex_init > > 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 b71fbd3..30aebaf 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; > } > @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-23 19:21 ` [Outreachy kernel] " Alison Schofield @ 2017-03-28 17:25 ` SIMRAN SINGHAL 2017-03-28 18:37 ` Alison Schofield 0 siblings, 1 reply; 10+ messages in thread From: SIMRAN SINGHAL @ 2017-03-28 17:25 UTC (permalink / raw) To: Alison Schofield Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, linux-kernel, outreachy-kernel On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield <amsfield22@gmail.com> wrote: > On Fri, Mar 24, 2017 at 12:05:20AM +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. > > Hi Simran, > > Please post all revision histories below the --- not just the most > recent. > Sorry, will not repeat this. > 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. > 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. > alisons >> >> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >> --- >> >> v4: >> -Add mutex_init >> >> 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 b71fbd3..30aebaf 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; >> } >> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-28 17:25 ` SIMRAN SINGHAL @ 2017-03-28 18:37 ` Alison Schofield 2017-03-30 18:32 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: Alison Schofield @ 2017-03-28 18:37 UTC (permalink / raw) To: SIMRAN SINGHAL Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, linux-kernel, outreachy-kernel On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: > On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield <amsfield22@gmail.com> wrote: > > On Fri, Mar 24, 2017 at 12:05:20AM +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. > > > > Hi Simran, > > > > Please post all revision histories below the --- not just the most > > recent. > > > Sorry, will not repeat this. > > > 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. I see it protecting this path from being re-entered. My uncertainty is about other paths to read/write. > > > 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 > > > alisons > >> > >> Signed-off-by: simran singhal <singhalsimran0@gmail.com> > >> --- > >> > >> v4: > >> -Add mutex_init > >> > >> 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 b71fbd3..30aebaf 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; > >> } > >> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. > >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-28 18:37 ` Alison Schofield @ 2017-03-30 18:32 ` Jonathan Cameron 2017-03-30 18:44 ` SIMRAN SINGHAL 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Cameron @ 2017-03-30 18:32 UTC (permalink / raw) To: Alison Schofield, SIMRAN SINGHAL Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, linux-kernel, outreachy-kernel On 28/03/17 19:37, Alison Schofield wrote: > On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield <amsfield22@gmail.com> wrote: >>> On Fri, Mar 24, 2017 at 12:05:20AM +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. >>> >>> Hi Simran, >>> >>> Please post all revision histories below the --- not just the most >>> recent. >>> >> Sorry, will not repeat this. >> >>> 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. > > I see it protecting this path from being re-entered. My uncertainty > is about other paths to read/write. > >> >>> 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 :) > 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 > Thanks, > alisons > >> >>> alisons >>>> >>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >>>> --- >>>> >>>> v4: >>>> -Add mutex_init >>>> >>>> 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 b71fbd3..30aebaf 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; >>>> } >>>> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >>>> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-30 18:32 ` Jonathan Cameron @ 2017-03-30 18:44 ` SIMRAN SINGHAL 2017-03-30 19:48 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: SIMRAN SINGHAL @ 2017-03-30 18:44 UTC (permalink / raw) To: Jonathan Cameron Cc: Alison Schofield, Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, Linux Kernel Mailing List, outreachy-kernel On Fri, Mar 31, 2017 at 12:02 AM, Jonathan Cameron <jic23@kernel.org> wrote: > On 28/03/17 19:37, Alison Schofield wrote: >> On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >>> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield <amsfield22@gmail.com> wrote: >>>> On Fri, Mar 24, 2017 at 12:05:20AM +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. >>>> >>>> Hi Simran, >>>> >>>> Please post all revision histories below the --- not just the most >>>> recent. >>>> >>> Sorry, will not repeat this. >>> >>>> 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. >> >> I see it protecting this path from being re-entered. My uncertainty >> is about other paths to read/write. >> >>> >>>> 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 :) >> > 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. > > So in conclusion both patches are good. > > Jonathan > >> Thanks, >> alisons >> >>> >>>> alisons >>>>> >>>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >>>>> --- >>>>> >>>>> v4: >>>>> -Add mutex_init >>>>> >>>>> 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 b71fbd3..30aebaf 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; >>>>> } >>>>> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >>>>> For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-30 18:44 ` SIMRAN SINGHAL 2017-03-30 19:48 ` Jonathan Cameron @ 2017-03-30 19:48 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2017-03-30 19:48 UTC (permalink / raw) To: SIMRAN SINGHAL, Jonathan Cameron Cc: Alison Schofield, Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, Linux Kernel Mailing List, outreachy-kernel On 30 March 2017 19:44:26 BST, SIMRAN SINGHAL <singhalsimran0@gmail.com> wrote: >On Fri, Mar 31, 2017 at 12:02 AM, Jonathan Cameron <jic23@kernel.org> >wrote: >> On 28/03/17 19:37, Alison Schofield wrote: >>> On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >>>> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield ><amsfield22@gmail.com> wrote: >>>>> On Fri, Mar 24, 2017 at 12:05:20AM +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. >>>>> >>>>> Hi Simran, >>>>> >>>>> Please post all revision histories below the --- not just the most >>>>> recent. >>>>> >>>> Sorry, will not repeat this. >>>> >>>>> 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. >>> >>> I see it protecting this path from being re-entered. My uncertainty >>> is about other paths to read/write. >>> >>>> >>>>> 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 :) >>> >> 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. Sorry, I meant of the driver rather than this improvement. Jonathan > >> >> So in conclusion both patches are good. >> >> Jonathan >> >>> Thanks, >>> alisons >>> >>>> >>>>> alisons >>>>>> >>>>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >>>>>> --- >>>>>> >>>>>> v4: >>>>>> -Add mutex_init >>>>>> >>>>>> 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 b71fbd3..30aebaf 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; >>>>>> } >>>>>> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >>>>>> For more options, visit https://groups.google.com/d/optout. >> -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock @ 2017-03-30 19:48 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2017-03-30 19:48 UTC (permalink / raw) To: SIMRAN SINGHAL, Jonathan Cameron Cc: Alison Schofield, Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, Linux Kernel Mailing List, outreachy-kernel On 30 March 2017 19:44:26 BST, SIMRAN SINGHAL <singhalsimran0@gmail.com> wrote: >On Fri, Mar 31, 2017 at 12:02 AM, Jonathan Cameron <jic23@kernel.org> >wrote: >> On 28/03/17 19:37, Alison Schofield wrote: >>> On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >>>> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield ><amsfield22@gmail.com> wrote: >>>>> On Fri, Mar 24, 2017 at 12:05:20AM +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. >>>>> >>>>> Hi Simran, >>>>> >>>>> Please post all revision histories below the --- not just the most >>>>> recent. >>>>> >>>> Sorry, will not repeat this. >>>> >>>>> 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. >>> >>> I see it protecting this path from being re-entered. My uncertainty >>> is about other paths to read/write. >>> >>>> >>>>> 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 :) >>> >> 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. Sorry, I meant of the driver rather than this improvement. Jonathan > >> >> So in conclusion both patches are good. >> >> Jonathan >> >>> Thanks, >>> alisons >>> >>>> >>>>> alisons >>>>>> >>>>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >>>>>> --- >>>>>> >>>>>> v4: >>>>>> -Add mutex_init >>>>>> >>>>>> 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 b71fbd3..30aebaf 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; >>>>>> } >>>>>> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >>>>>> For more options, visit https://groups.google.com/d/optout. >> -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock @ 2017-03-30 19:48 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2017-03-30 19:48 UTC (permalink / raw) To: SIMRAN SINGHAL, Jonathan Cameron Cc: Alison Schofield, Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, Linux Kernel Mailing List, outreachy-kernel On 30 March 2017 19:44:26 BST, SIMRAN SINGHAL <singhalsimran0@gmail=2Ecom>= wrote: >On Fri, Mar 31, 2017 at 12:02 AM, Jonathan Cameron <jic23@kernel=2Eorg> >wrote: >> On 28/03/17 19:37, Alison Schofield wrote: >>> On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >>>> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield ><amsfield22@gmail=2Ecom> wrote: >>>>> On Fri, Mar 24, 2017 at 12:05:20AM +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=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 >>>>> >>>>> Hi Simran, >>>>> >>>>> Please post all revision histories below the --- not just the most >>>>> recent=2E >>>>> >>>> Sorry, will not repeat this=2E >>>> >>>>> 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=2E >>> >>> I see it protecting this path from being re-entered=2E My uncertainty >>> is about other paths to read/write=2E >>> >>>> >>>>> 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 >>>> >>>> Should I do like her as her's also seem correct or go ahead with >this=2E >>> >>> 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 :) >>> >> 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 > >> >> So in conclusion both patches are good=2E >> >> Jonathan >> >>> Thanks, >>> alisons >>> >>>> >>>>> alisons >>>>>> >>>>>> Signed-off-by: simran singhal <singhalsimran0@gmail=2Ecom> >>>>>> --- >>>>>> >>>>>> v4: >>>>>> -Add mutex_init >>>>>> >>>>>> drivers/staging/iio/meter/ade7753=2Ec | 7 +++++-- >>>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/staging/iio/meter/ade7753=2Ec >b/drivers/staging/iio/meter/ade7753=2Ec >>>>>> index b71fbd3=2E=2E30aebaf 100644 >>>>>> --- a/drivers/staging/iio/meter/ade7753=2Ec >>>>>> +++ b/drivers/staging/iio/meter/ade7753=2Ec >>>>>> @@ -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 =3D 27900 / val; >>>>>> if (t > 0) >>>>>> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct >device *dev, >>>>>> 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) >>>>>> st =3D iio_priv(indio_dev); >>>>>> st->us =3D spi; >>>>>> mutex_init(&st->buf_lock); >>>>>> + mutex_init(&st->lock); >>>>>> >>>>>> 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 >Google Groups "outreachy-kernel" group=2E >>>>>> To unsubscribe from this group and stop receiving emails from it, >send an email to outreachy-kernel+unsubscribe@googlegroups=2Ecom=2E >>>>>> To post to this group, send email to >outreachy-kernel@googlegroups=2Ecom=2E >>>>>> To view this discussion on the web visit >https://groups=2Egoogle=2Ecom/d/msgid/outreachy-kernel/20170323183520=2EG= A9871%40singhal-Inspiron-5558=2E >>>>>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock 2017-03-30 19:48 ` Jonathan Cameron (?) (?) @ 2017-03-30 20:13 ` SIMRAN SINGHAL -1 siblings, 0 replies; 10+ messages in thread From: SIMRAN SINGHAL @ 2017-03-30 20:13 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, Alison Schofield, Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack, Peter Meerwald-Stadler, Greg KH, linux-iio, devel, Linux Kernel Mailing List On Fri, Mar 31, 2017 at 1:18 AM, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > On 30 March 2017 19:44:26 BST, SIMRAN SINGHAL <singhalsimran0@gmail.com> wrote: >>On Fri, Mar 31, 2017 at 12:02 AM, Jonathan Cameron <jic23@kernel.org> >>wrote: >>> On 28/03/17 19:37, Alison Schofield wrote: >>>> On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote: >>>>> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield >><amsfield22@gmail.com> wrote: >>>>>> On Fri, Mar 24, 2017 at 12:05:20AM +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. >>>>>> >>>>>> Hi Simran, >>>>>> >>>>>> Please post all revision histories below the --- not just the most >>>>>> recent. >>>>>> >>>>> Sorry, will not repeat this. >>>>> >>>>>> 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. >>>> >>>> I see it protecting this path from being re-entered. My uncertainty >>>> is about other paths to read/write. >>>> >>>>> >>>>>> 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 :) >>>> >>> 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. > > 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. > Jonathan >> >>> >>> So in conclusion both patches are good. >>> >>> Jonathan >>> >>>> Thanks, >>>> alisons >>>> >>>>> >>>>>> alisons >>>>>>> >>>>>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com> >>>>>>> --- >>>>>>> >>>>>>> v4: >>>>>>> -Add mutex_init >>>>>>> >>>>>>> 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 b71fbd3..30aebaf 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; >>>>>>> } >>>>>>> @@ -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 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/20170323183520.GA9871%40singhal-Inspiron-5558. >>>>>>> For more options, visit https://groups.google.com/d/optout. >>> > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-30 20:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-23 18:35 [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock simran singhal 2017-03-23 19:21 ` [Outreachy kernel] " Alison Schofield 2017-03-28 17:25 ` SIMRAN SINGHAL 2017-03-28 18:37 ` Alison Schofield 2017-03-30 18:32 ` Jonathan Cameron 2017-03-30 18:44 ` SIMRAN SINGHAL 2017-03-30 19:48 ` Jonathan Cameron 2017-03-30 19:48 ` Jonathan Cameron 2017-03-30 19:48 ` Jonathan Cameron 2017-03-30 20:13 ` SIMRAN SINGHAL
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.