staging: iio: adis16240: Replace mlock with driver private lock.
Subsystems (4)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/2 staging: iio: adis16240: 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. Also declare and initialize variable st which is pointer to struct adis, to access txrx_lock.
--- drivers/staging/iio/accel/adis16240.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/staging/iio/accel/adis16240.c +9 -8
@@ -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 ;
Signature · 1 line
2.9.3
Quoted 19 lines · Varsha Rao
The IIO subsystem is redefining
iio_dev->mlockto be used by the IIO core only for protecting device operating mode changes. ie. Changes betweenINDIO_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 <[email protected]>--- drivers/staging/iio/accel/adis16240.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)drivers/staging/iio/accel/adis16240.c +0
-0
drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 235
Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +2
-1@@ -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.
Quoted 45 lines
drivers/staging/iio/accel/adis16240.c +7
-7ret =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 ;
Quoted 14 lines
Review static ssize_t adis16240_read_12bit_signed(struct
Quoted 4 lines
+++
b/drivers/staging/iio/accel/adis16240.c(quoted diff) +0
-0@@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(structdevice *dev,
drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 235
Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +2
-1{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);
Quoted 7 lines
This unfortunately wont work. The adis_read_reg()/adis_write_reg() functions
take thetxrx_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?
Quoted 6 lines
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
PATCH 2/2 staging: iio: adis16240: Modify existing comment.
In reply to a message not in the archiveModify the existing comment to reflect the changes from mlock to txrx_lock.
--- drivers/staging/iio/accel/adis16240.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/accel/adis16240.c +1 -1
@@ -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);
Signature · 1 line
2.9.3
Re: [PATCH 2/2] staging: iio: adis16240: Modify existing comment.
Quoted 13 lines · Varsha Rao
Modify the existing comment to reflect the changes from mlock to
txrx_lock.Signed-off-by Varsha Rao <[email protected]>--- drivers/staging/iio/accel/adis16240.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/iio/accel/adis16240.c +0
-0
drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 234
Quoted 5 lines
drivers/staging/iio/accel/adis16240.c +1
-1@@ -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.
Quoted 4 lines
drivers/staging/iio/accel/adis16240.c +0
-0mutex_lock (&st->txrx_lock); ret =adis16240_spi_read_signed (dev, attr, buf,12 );mutex_unlock (&st->txrx_lock);