* [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock @ 2017-03-17 15:11 sayli karnik 2017-03-17 17:11 ` [Outreachy kernel] " Alison Schofield 0 siblings, 1 reply; 4+ messages in thread From: sayli karnik @ 2017-03-17 15:11 UTC (permalink / raw) To: outreachy-kernel Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio iio_dev->mlock should be used by the IIO core only for protecting device operating mode changes. ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes. Replace mlock with a lock in the device's global data to protect hardware state changes. Signed-off-by: sayli karnik <karniksayli1995@gmail.com> --- drivers/staging/iio/meter/ade7758_core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c index 99c89e6..1723eb8 100644 --- a/drivers/staging/iio/meter/ade7758_core.c +++ b/drivers/staging/iio/meter/ade7758_core.c @@ -522,13 +522,14 @@ static int ade7758_read_raw(struct iio_dev *indio_dev, int *val2, long mask) { + struct ade7758_state *st = iio_priv(indio_dev); int ret; switch (mask) { case IIO_CHAN_INFO_SAMP_FREQ: - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->buf_lock); ret = ade7758_read_samp_freq(&indio_dev->dev, val); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->buf_lock); return ret; default: return -EINVAL; @@ -541,15 +542,16 @@ static int ade7758_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, int val2, long mask) { + struct ade7758_state *st = iio_priv(indio_dev); int ret; switch (mask) { case IIO_CHAN_INFO_SAMP_FREQ: if (val2) return -EINVAL; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->buf_lock); ret = ade7758_write_samp_freq(&indio_dev->dev, val); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->buf_lock); return ret; default: return -EINVAL; -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock 2017-03-17 15:11 [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock sayli karnik @ 2017-03-17 17:11 ` Alison Schofield 2017-03-18 17:38 ` Gargi Sharma 0 siblings, 1 reply; 4+ messages in thread From: Alison Schofield @ 2017-03-17 17:11 UTC (permalink / raw) To: sayli karnik Cc: outreachy-kernel, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio On Fri, Mar 17, 2017 at 08:41:11PM +0530, sayli karnik wrote: > iio_dev->mlock should be used by the IIO core only for protecting > device operating mode changes. ie. Changes between INDIO_DIRECT_MODE, > INDIO_BUFFER_* modes. > Replace mlock with a lock in the device's global data to protect > hardware state changes. Hi Sayli, There are nested locks here, hence deadlock. I'm wondering if we can grab the lock once at the top layers (read/write_raw functions) and hold it. Seems like that's a pattern in IIO drivers. Let's see what the reviewers suggest on this one. alisons > > Signed-off-by: sayli karnik <karniksayli1995@gmail.com> > --- > drivers/staging/iio/meter/ade7758_core.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c > index 99c89e6..1723eb8 100644 > --- a/drivers/staging/iio/meter/ade7758_core.c > +++ b/drivers/staging/iio/meter/ade7758_core.c > @@ -522,13 +522,14 @@ static int ade7758_read_raw(struct iio_dev *indio_dev, > int *val2, > long mask) > { > + struct ade7758_state *st = iio_priv(indio_dev); > int ret; > > switch (mask) { > case IIO_CHAN_INFO_SAMP_FREQ: > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->buf_lock); > ret = ade7758_read_samp_freq(&indio_dev->dev, val); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->buf_lock); > return ret; > default: > return -EINVAL; > @@ -541,15 +542,16 @@ static int ade7758_write_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int val, int val2, long mask) > { > + struct ade7758_state *st = iio_priv(indio_dev); > int ret; > > switch (mask) { > case IIO_CHAN_INFO_SAMP_FREQ: > if (val2) > return -EINVAL; > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->buf_lock); > ret = ade7758_write_samp_freq(&indio_dev->dev, val); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->buf_lock); > return ret; > default: > return -EINVAL; > -- > 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/20170317151110.GA26848%40sayli-HP-15-Notebook-PC. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock 2017-03-17 17:11 ` [Outreachy kernel] " Alison Schofield @ 2017-03-18 17:38 ` Gargi Sharma 2017-03-20 19:33 ` sayli karnik 0 siblings, 1 reply; 4+ messages in thread From: Gargi Sharma @ 2017-03-18 17:38 UTC (permalink / raw) To: Alison Schofield Cc: sayli karnik, outreachy-kernel, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio On Fri, Mar 17, 2017 at 10:41 PM, Alison Schofield <amsfield22@gmail.com> wrote: > > On Fri, Mar 17, 2017 at 08:41:11PM +0530, sayli karnik wrote: > > iio_dev->mlock should be used by the IIO core only for protecting > > device operating mode changes. ie. Changes between INDIO_DIRECT_MODE, > > INDIO_BUFFER_* modes. > > Replace mlock with a lock in the device's global data to protect > > hardware state changes. > > Hi Sayli, > > There are nested locks here, hence deadlock. I'm wondering if we can grab > the lock once at the top layers (read/write_raw functions) and hold it. > Seems like that's a pattern in IIO drivers. Let's see what the reviewers > suggest on this one. > Hey Alison, I was trying to work through a similar patch. I'm having trouble understanding how a deadlock will be created? Also, Lars suggested in this{https://groups.google.com/forum/#!msg/outreachy-kernel/GapBLSp5WCo/cju_A8B-AwAJ} thread that writing a variant of <driver_name>_spi_{read,write}_reg_16() that does not take a lock and instead uses read-modify-write cycle in a protected section is the way to go. Is there any example I can look to for inspiration :). Also, in similar vain there are other(8 bit) variants for read/write functions as well. Should they be modified as well? Thanks! Gargi > alisons > > > > > Signed-off-by: sayli karnik <karniksayli1995@gmail.com> > > --- > > drivers/staging/iio/meter/ade7758_core.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c > > index 99c89e6..1723eb8 100644 > > --- a/drivers/staging/iio/meter/ade7758_core.c > > +++ b/drivers/staging/iio/meter/ade7758_core.c > > @@ -522,13 +522,14 @@ static int ade7758_read_raw(struct iio_dev *indio_dev, > > int *val2, > > long mask) > > { > > + struct ade7758_state *st = iio_priv(indio_dev); > > int ret; > > > > switch (mask) { > > case IIO_CHAN_INFO_SAMP_FREQ: > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&st->buf_lock); > > ret = ade7758_read_samp_freq(&indio_dev->dev, val); > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&st->buf_lock); > > return ret; > > default: > > return -EINVAL; > > @@ -541,15 +542,16 @@ static int ade7758_write_raw(struct iio_dev *indio_dev, > > struct iio_chan_spec const *chan, > > int val, int val2, long mask) > > { > > + struct ade7758_state *st = iio_priv(indio_dev); > > int ret; > > > > switch (mask) { > > case IIO_CHAN_INFO_SAMP_FREQ: > > if (val2) > > return -EINVAL; > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&st->buf_lock); > > ret = ade7758_write_samp_freq(&indio_dev->dev, val); > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&st->buf_lock); > > return ret; > > default: > > return -EINVAL; > > -- > > 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/20170317151110.GA26848%40sayli-HP-15-Notebook-PC. > > For more options, visit https://groups.google.com/d/optout. > > -- > 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/20170317171136.GA19487%40d830.WORKGROUP. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock 2017-03-18 17:38 ` Gargi Sharma @ 2017-03-20 19:33 ` sayli karnik 0 siblings, 0 replies; 4+ messages in thread From: sayli karnik @ 2017-03-20 19:33 UTC (permalink / raw) To: Gargi Sharma Cc: Alison Schofield, outreachy-kernel, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio On Sat, Mar 18, 2017 at 11:08 PM, Gargi Sharma <gs051095@gmail.com> wrote: > On Fri, Mar 17, 2017 at 10:41 PM, Alison Schofield <amsfield22@gmail.com> wrote: >> >> On Fri, Mar 17, 2017 at 08:41:11PM +0530, sayli karnik wrote: >> > iio_dev->mlock should be used by the IIO core only for protecting >> > device operating mode changes. ie. Changes between INDIO_DIRECT_MODE, >> > INDIO_BUFFER_* modes. >> > Replace mlock with a lock in the device's global data to protect >> > hardware state changes. >> >> Hi Sayli, >> >> There are nested locks here, hence deadlock. I'm wondering if we can grab >> the lock once at the top layers (read/write_raw functions) and hold it. >> Seems like that's a pattern in IIO drivers. Let's see what the reviewers >> suggest on this one. >> > To avoid the mutex lock nesting, should I remove the locking in ade7758_{read, write}_raw() or ade7758_spi_{read, write}_reg_8() ? I think removing it from the outer function i.e ade7758_{read, write}_raw() is safer so that other places where ade7758_spi_{read, write}_reg_8() might be used are unaffected. thanks, sayli > Hey Alison, > > I was trying to work through a similar patch. I'm having trouble > understanding how a deadlock will be created? Also, Lars suggested in > this{https://groups.google.com/forum/#!msg/outreachy-kernel/GapBLSp5WCo/cju_A8B-AwAJ} > thread that writing a variant of > <driver_name>_spi_{read,write}_reg_16() that does not take a lock and > instead uses read-modify-write cycle in a protected section is the way > to go. Is there any example I can look to for inspiration :). Also, in > similar vain there are other(8 bit) variants for read/write functions > as well. Should they be modified as well? > > Thanks! > Gargi > >> alisons >> >> > >> > Signed-off-by: sayli karnik <karniksayli1995@gmail.com> >> > --- >> > drivers/staging/iio/meter/ade7758_core.c | 10 ++++++---- >> > 1 file changed, 6 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c >> > index 99c89e6..1723eb8 100644 >> > --- a/drivers/staging/iio/meter/ade7758_core.c >> > +++ b/drivers/staging/iio/meter/ade7758_core.c >> > @@ -522,13 +522,14 @@ static int ade7758_read_raw(struct iio_dev *indio_dev, >> > int *val2, >> > long mask) >> > { >> > + struct ade7758_state *st = iio_priv(indio_dev); >> > int ret; >> > >> > switch (mask) { >> > case IIO_CHAN_INFO_SAMP_FREQ: >> > - mutex_lock(&indio_dev->mlock); >> > + mutex_lock(&st->buf_lock); >> > ret = ade7758_read_samp_freq(&indio_dev->dev, val); >> > - mutex_unlock(&indio_dev->mlock); >> > + mutex_unlock(&st->buf_lock); >> > return ret; >> > default: >> > return -EINVAL; >> > @@ -541,15 +542,16 @@ static int ade7758_write_raw(struct iio_dev *indio_dev, >> > struct iio_chan_spec const *chan, >> > int val, int val2, long mask) >> > { >> > + struct ade7758_state *st = iio_priv(indio_dev); >> > int ret; >> > >> > switch (mask) { >> > case IIO_CHAN_INFO_SAMP_FREQ: >> > if (val2) >> > return -EINVAL; >> > - mutex_lock(&indio_dev->mlock); >> > + mutex_lock(&st->buf_lock); >> > ret = ade7758_write_samp_freq(&indio_dev->dev, val); >> > - mutex_unlock(&indio_dev->mlock); >> > + mutex_unlock(&st->buf_lock); >> > return ret; >> > default: >> > return -EINVAL; >> > -- >> > 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/20170317151110.GA26848%40sayli-HP-15-Notebook-PC. >> > For more options, visit https://groups.google.com/d/optout. >> >> -- >> 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/20170317171136.GA19487%40d830.WORKGROUP. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-20 19:33 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-17 15:11 [PATCH] staging: iio: meter: ade7758_core: Replace mlock with driver's private lock sayli karnik 2017-03-17 17:11 ` [Outreachy kernel] " Alison Schofield 2017-03-18 17:38 ` Gargi Sharma 2017-03-20 19:33 ` sayli karnik
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.