* [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock. [not found] <cover.1489395260.git.rvarsha016@gmail.com> @ 2017-03-13 9:23 ` Varsha Rao 2017-03-13 11:47 ` Lars-Peter Clausen 2017-03-13 9:23 ` [PATCH 2/2] staging: iio: adis16240: Modify existing comment Varsha Rao 1 sibling, 1 reply; 5+ messages in thread From: Varsha Rao @ 2017-03-13 9:23 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Barry Song, linux-iio Cc: 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. Also declare and initialize variable st which is pointer to struct adis, to access txrx_lock. Signed-off-by: Varsha Rao <rvarsha016@gmail.com> --- drivers/staging/iio/accel/adis16240.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c index 27d7f6a..c182b2d 100644 --- a/drivers/staging/iio/accel/adis16240.c +++ b/drivers/staging/iio/accel/adis16240.c @@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev, { ssize_t ret; struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct adis *st = iio_priv(indio_dev); /* Take the iio_dev status lock */ - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->txrx_lock); ret = adis16240_spi_read_signed(dev, attr, buf, 12); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->txrx_lock); return ret; } @@ -295,31 +296,31 @@ static int adis16240_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_CALIBBIAS: bits = 10; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->txrx_lock); addr = adis16240_addresses[chan->scan_index][0]; ret = adis_read_reg_16(st, addr, &val16); if (ret) { - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->txrx_lock); return ret; } val16 &= (1 << bits) - 1; val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); *val = val16; - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->txrx_lock); return IIO_VAL_INT; case IIO_CHAN_INFO_PEAK: bits = 10; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->txrx_lock); addr = adis16240_addresses[chan->scan_index][1]; ret = adis_read_reg_16(st, addr, &val16); if (ret) { - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->txrx_lock); return ret; } val16 &= (1 << bits) - 1; val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); *val = val16; - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->txrx_lock); return IIO_VAL_INT; } return -EINVAL; -- 2.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock. 2017-03-13 9:23 ` [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock Varsha Rao @ 2017-03-13 11:47 ` Lars-Peter Clausen 2017-03-13 14:39 ` Varsha Rao 0 siblings, 1 reply; 5+ messages in thread From: Lars-Peter Clausen @ 2017-03-13 11:47 UTC (permalink / raw) To: Varsha Rao, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Barry Song, linux-iio Cc: outreachy-kernel On 03/13/2017 10:23 AM, Varsha Rao 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. Also declare and > initialize variable st which is pointer to struct adis, to access > txrx_lock. > > Signed-off-by: Varsha Rao <rvarsha016@gmail.com> > --- > drivers/staging/iio/accel/adis16240.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c > index 27d7f6a..c182b2d 100644 > --- a/drivers/staging/iio/accel/adis16240.c > +++ b/drivers/staging/iio/accel/adis16240.c > @@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev, > { > ssize_t ret; > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct adis *st = iio_priv(indio_dev); > > /* Take the iio_dev status lock */ > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->txrx_lock); This unfortunately wont work. The adis_read_reg()/adis_write_reg() functions take the txrx_lock. So this change causes a deadlock, trying to take a lock that is already locked. But this lock can probably be removed. It should be safe to run the function multiple times in parallel. Same for the other changes in this patch. > ret = adis16240_spi_read_signed(dev, attr, buf, 12); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->txrx_lock); > > return ret; > } > @@ -295,31 +296,31 @@ static int adis16240_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > case IIO_CHAN_INFO_CALIBBIAS: > bits = 10; > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->txrx_lock); > addr = adis16240_addresses[chan->scan_index][0]; > ret = adis_read_reg_16(st, addr, &val16); > if (ret) { > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->txrx_lock); > return ret; > } > val16 &= (1 << bits) - 1; > val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); > *val = val16; > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->txrx_lock); > return IIO_VAL_INT; > case IIO_CHAN_INFO_PEAK: > bits = 10; > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->txrx_lock); > addr = adis16240_addresses[chan->scan_index][1]; > ret = adis_read_reg_16(st, addr, &val16); > if (ret) { > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->txrx_lock); > return ret; > } > val16 &= (1 << bits) - 1; > val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); > *val = val16; > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->txrx_lock); > return IIO_VAL_INT; > } > return -EINVAL; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock. 2017-03-13 11:47 ` Lars-Peter Clausen @ 2017-03-13 14:39 ` Varsha Rao 0 siblings, 0 replies; 5+ messages in thread From: Varsha Rao @ 2017-03-13 14:39 UTC (permalink / raw) To: outreachy-kernel Cc: rvarsha016, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, 21cnbao, linux-iio, lars [-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --] > > +++ b/drivers/staging/iio/accel/adis16240.c > > @@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct > device *dev, > > { > > ssize_t ret; > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > + struct adis *st = iio_priv(indio_dev); > > > > /* Take the iio_dev status lock */ > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&st->txrx_lock); > > This unfortunately wont work. The adis_read_reg()/adis_write_reg() > functions > take the txrx_lock. So this change causes a deadlock, trying to take a > lock > that is already locked. > > Yes, it would cause deadlock. Can the lock be placed within the adis_read_reg() function and removing locks from other place? > But this lock can probably be removed. It should be safe to run the > function > multiple times in parallel. > > Same for the other changes in this patch. > Then should I remove all the locks in this file? Thanks, Varsha [-- Attachment #1.2: Type: text/html, Size: 1484 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] staging: iio: adis16240: Modify existing comment. [not found] <cover.1489395260.git.rvarsha016@gmail.com> 2017-03-13 9:23 ` [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock Varsha Rao @ 2017-03-13 9:23 ` Varsha Rao 2017-03-13 11:39 ` Lars-Peter Clausen 1 sibling, 1 reply; 5+ messages in thread From: Varsha Rao @ 2017-03-13 9:23 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Barry Song, linux-iio Cc: outreachy-kernel Modify the existing comment to reflect the changes from mlock to txrx_lock. Signed-off-by: Varsha Rao <rvarsha016@gmail.com> --- drivers/staging/iio/accel/adis16240.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c index c182b2d..aa3030c 100644 --- a/drivers/staging/iio/accel/adis16240.c +++ b/drivers/staging/iio/accel/adis16240.c @@ -231,7 +231,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev, struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct adis *st = iio_priv(indio_dev); - /* Take the iio_dev status lock */ + /* Take adis txrx_lock to protect sensor state */ mutex_lock(&st->txrx_lock); ret = adis16240_spi_read_signed(dev, attr, buf, 12); mutex_unlock(&st->txrx_lock); -- 2.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] staging: iio: adis16240: Modify existing comment. 2017-03-13 9:23 ` [PATCH 2/2] staging: iio: adis16240: Modify existing comment Varsha Rao @ 2017-03-13 11:39 ` Lars-Peter Clausen 0 siblings, 0 replies; 5+ messages in thread From: Lars-Peter Clausen @ 2017-03-13 11:39 UTC (permalink / raw) To: Varsha Rao, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Barry Song, linux-iio Cc: outreachy-kernel On 03/13/2017 10:23 AM, Varsha Rao wrote: > Modify the existing comment to reflect the changes from mlock to > txrx_lock. > > Signed-off-by: Varsha Rao <rvarsha016@gmail.com> > --- > drivers/staging/iio/accel/adis16240.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c > index c182b2d..aa3030c 100644 > --- a/drivers/staging/iio/accel/adis16240.c > +++ b/drivers/staging/iio/accel/adis16240.c > @@ -231,7 +231,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev, > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > struct adis *st = iio_priv(indio_dev); > > - /* Take the iio_dev status lock */ > + /* Take adis txrx_lock to protect sensor state */ This should be part of the patch that changes the lock. > mutex_lock(&st->txrx_lock); > ret = adis16240_spi_read_signed(dev, attr, buf, 12); > mutex_unlock(&st->txrx_lock); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-13 14:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1489395260.git.rvarsha016@gmail.com>
2017-03-13 9:23 ` [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock Varsha Rao
2017-03-13 11:47 ` Lars-Peter Clausen
2017-03-13 14:39 ` Varsha Rao
2017-03-13 9:23 ` [PATCH 2/2] staging: iio: adis16240: Modify existing comment Varsha Rao
2017-03-13 11:39 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox