| Message ID | 20190926111812.15957-2-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: adis: cleanup lock usage | expand |
On Thu, 26 Sep 2019 14:18:03 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > The lock can be extended a bit to protect other elements that are not > particular to just TX/RX. Another idea would have been to just add a new > `state_lock`, but that would mean 2 locks which would be redundant, and > probably cause more potential for dead-locks. > > What will be done in the next patches, will be to add some unlocked > versions for read/write_reg functions. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Would be good to document the scope of the lock as a comment when it is defined. What exactly is 'state' in this case? Thanks, Jonathan > --- > drivers/iio/imu/adis.c | 10 +++++----- > drivers/iio/imu/adis_buffer.c | 4 ++-- > include/linux/iio/imu/adis.h | 2 +- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > index 1631c255deab..3c2d896e3a96 100644 > --- a/drivers/iio/imu/adis.c > +++ b/drivers/iio/imu/adis.c > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, > }, > }; > > - mutex_lock(&adis->txrx_lock); > + mutex_lock(&adis->state_lock); > > spi_message_init(&msg); > > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, > } > > out_unlock: > - mutex_unlock(&adis->txrx_lock); > + mutex_unlock(&adis->state_lock); > > return ret; > } > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, > }, > }; > > - mutex_lock(&adis->txrx_lock); > + mutex_lock(&adis->state_lock); > spi_message_init(&msg); > > if (adis->current_page != page) { > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, > } > > out_unlock: > - mutex_unlock(&adis->txrx_lock); > + mutex_unlock(&adis->state_lock); > > return ret; > } > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion); > int adis_init(struct adis *adis, struct iio_dev *indio_dev, > struct spi_device *spi, const struct adis_data *data) > { > - mutex_init(&adis->txrx_lock); > + mutex_init(&adis->state_lock); > adis->spi = spi; > adis->data = data; > iio_device_set_drvdata(indio_dev, adis); > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c > index 9ac8356d9a95..bf581a2c321d 100644 > --- a/drivers/iio/imu/adis_buffer.c > +++ b/drivers/iio/imu/adis_buffer.c > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > return -ENOMEM; > > if (adis->data->has_paging) { > - mutex_lock(&adis->txrx_lock); > + mutex_lock(&adis->state_lock); > if (adis->current_page != 0) { > adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID); > adis->tx[1] = 0; > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > > if (adis->data->has_paging) { > adis->current_page = 0; > - mutex_unlock(&adis->txrx_lock); > + mutex_unlock(&adis->state_lock); > } > > iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer, > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > index 4c53815bb729..3ed5eceaac2d 100644 > --- a/include/linux/iio/imu/adis.h > +++ b/include/linux/iio/imu/adis.h > @@ -61,7 +61,7 @@ struct adis { > const struct adis_data *data; > struct adis_burst *burst; > > - struct mutex txrx_lock; > + struct mutex state_lock; > struct spi_message msg; > struct spi_transfer *xfer; > unsigned int current_page;
On Sun, 6 Oct 2019 09:53:33 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 26 Sep 2019 14:18:03 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > The lock can be extended a bit to protect other elements that are not > > particular to just TX/RX. Another idea would have been to just add a new > > `state_lock`, but that would mean 2 locks which would be redundant, and > > probably cause more potential for dead-locks. > > > > What will be done in the next patches, will be to add some unlocked > > versions for read/write_reg functions. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Would be good to document the scope of the lock as a comment when it > is defined. What exactly is 'state' in this case? As this can be done as a follow up and the rest of the series is fine as is... Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > > Thanks, > > Jonathan > > > --- > > drivers/iio/imu/adis.c | 10 +++++----- > > drivers/iio/imu/adis_buffer.c | 4 ++-- > > include/linux/iio/imu/adis.h | 2 +- > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > > index 1631c255deab..3c2d896e3a96 100644 > > --- a/drivers/iio/imu/adis.c > > +++ b/drivers/iio/imu/adis.c > > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, > > }, > > }; > > > > - mutex_lock(&adis->txrx_lock); > > + mutex_lock(&adis->state_lock); > > > > spi_message_init(&msg); > > > > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, > > } > > > > out_unlock: > > - mutex_unlock(&adis->txrx_lock); > > + mutex_unlock(&adis->state_lock); > > > > return ret; > > } > > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, > > }, > > }; > > > > - mutex_lock(&adis->txrx_lock); > > + mutex_lock(&adis->state_lock); > > spi_message_init(&msg); > > > > if (adis->current_page != page) { > > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, > > } > > > > out_unlock: > > - mutex_unlock(&adis->txrx_lock); > > + mutex_unlock(&adis->state_lock); > > > > return ret; > > } > > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion); > > int adis_init(struct adis *adis, struct iio_dev *indio_dev, > > struct spi_device *spi, const struct adis_data *data) > > { > > - mutex_init(&adis->txrx_lock); > > + mutex_init(&adis->state_lock); > > adis->spi = spi; > > adis->data = data; > > iio_device_set_drvdata(indio_dev, adis); > > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c > > index 9ac8356d9a95..bf581a2c321d 100644 > > --- a/drivers/iio/imu/adis_buffer.c > > +++ b/drivers/iio/imu/adis_buffer.c > > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > > return -ENOMEM; > > > > if (adis->data->has_paging) { > > - mutex_lock(&adis->txrx_lock); > > + mutex_lock(&adis->state_lock); > > if (adis->current_page != 0) { > > adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID); > > adis->tx[1] = 0; > > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > > > > if (adis->data->has_paging) { > > adis->current_page = 0; > > - mutex_unlock(&adis->txrx_lock); > > + mutex_unlock(&adis->state_lock); > > } > > > > iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer, > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > > index 4c53815bb729..3ed5eceaac2d 100644 > > --- a/include/linux/iio/imu/adis.h > > +++ b/include/linux/iio/imu/adis.h > > @@ -61,7 +61,7 @@ struct adis { > > const struct adis_data *data; > > struct adis_burst *burst; > > > > - struct mutex txrx_lock; > > + struct mutex state_lock; > > struct spi_message msg; > > struct spi_transfer *xfer; > > unsigned int current_page; >
On Sun, 2019-10-06 at 10:06 +0100, Jonathan Cameron wrote: > [External] > > On Sun, 6 Oct 2019 09:53:33 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Thu, 26 Sep 2019 14:18:03 +0300 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > The lock can be extended a bit to protect other elements that are not > > > particular to just TX/RX. Another idea would have been to just add a > > > new > > > `state_lock`, but that would mean 2 locks which would be redundant, > > > and > > > probably cause more potential for dead-locks. > > > > > > What will be done in the next patches, will be to add some unlocked > > > versions for read/write_reg functions. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > Would be good to document the scope of the lock as a comment when it > > is defined. What exactly is 'state' in this case? > As this can be done as a follow up and the rest of the series is fine > as is... > Will do. > Applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to play with it. > > Thanks, > > Jonathan > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/imu/adis.c | 10 +++++----- > > > drivers/iio/imu/adis_buffer.c | 4 ++-- > > > include/linux/iio/imu/adis.h | 2 +- > > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > > > index 1631c255deab..3c2d896e3a96 100644 > > > --- a/drivers/iio/imu/adis.c > > > +++ b/drivers/iio/imu/adis.c > > > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int > > > reg, > > > }, > > > }; > > > > > > - mutex_lock(&adis->txrx_lock); > > > + mutex_lock(&adis->state_lock); > > > > > > spi_message_init(&msg); > > > > > > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned > > > int reg, > > > } > > > > > > out_unlock: > > > - mutex_unlock(&adis->txrx_lock); > > > + mutex_unlock(&adis->state_lock); > > > > > > return ret; > > > } > > > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int > > > reg, > > > }, > > > }; > > > > > > - mutex_lock(&adis->txrx_lock); > > > + mutex_lock(&adis->state_lock); > > > spi_message_init(&msg); > > > > > > if (adis->current_page != page) { > > > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int > > > reg, > > > } > > > > > > out_unlock: > > > - mutex_unlock(&adis->txrx_lock); > > > + mutex_unlock(&adis->state_lock); > > > > > > return ret; > > > } > > > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion); > > > int adis_init(struct adis *adis, struct iio_dev *indio_dev, > > > struct spi_device *spi, const struct adis_data *data) > > > { > > > - mutex_init(&adis->txrx_lock); > > > + mutex_init(&adis->state_lock); > > > adis->spi = spi; > > > adis->data = data; > > > iio_device_set_drvdata(indio_dev, adis); > > > diff --git a/drivers/iio/imu/adis_buffer.c > > > b/drivers/iio/imu/adis_buffer.c > > > index 9ac8356d9a95..bf581a2c321d 100644 > > > --- a/drivers/iio/imu/adis_buffer.c > > > +++ b/drivers/iio/imu/adis_buffer.c > > > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, > > > void *p) > > > return -ENOMEM; > > > > > > if (adis->data->has_paging) { > > > - mutex_lock(&adis->txrx_lock); > > > + mutex_lock(&adis->state_lock); > > > if (adis->current_page != 0) { > > > adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID); > > > adis->tx[1] = 0; > > > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, > > > void *p) > > > > > > if (adis->data->has_paging) { > > > adis->current_page = 0; > > > - mutex_unlock(&adis->txrx_lock); > > > + mutex_unlock(&adis->state_lock); > > > } > > > > > > iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer, > > > diff --git a/include/linux/iio/imu/adis.h > > > b/include/linux/iio/imu/adis.h > > > index 4c53815bb729..3ed5eceaac2d 100644 > > > --- a/include/linux/iio/imu/adis.h > > > +++ b/include/linux/iio/imu/adis.h > > > @@ -61,7 +61,7 @@ struct adis { > > > const struct adis_data *data; > > > struct adis_burst *burst; > > > > > > - struct mutex txrx_lock; > > > + struct mutex state_lock; > > > struct spi_message msg; > > > struct spi_transfer *xfer; > > > unsigned int current_page;
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c index 1631c255deab..3c2d896e3a96 100644 --- a/drivers/iio/imu/adis.c +++ b/drivers/iio/imu/adis.c @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, }, }; - mutex_lock(&adis->txrx_lock); + mutex_lock(&adis->state_lock); spi_message_init(&msg); @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned int reg, } out_unlock: - mutex_unlock(&adis->txrx_lock); + mutex_unlock(&adis->state_lock); return ret; } @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, }, }; - mutex_lock(&adis->txrx_lock); + mutex_lock(&adis->state_lock); spi_message_init(&msg); if (adis->current_page != page) { @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int reg, } out_unlock: - mutex_unlock(&adis->txrx_lock); + mutex_unlock(&adis->state_lock); return ret; } @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion); int adis_init(struct adis *adis, struct iio_dev *indio_dev, struct spi_device *spi, const struct adis_data *data) { - mutex_init(&adis->txrx_lock); + mutex_init(&adis->state_lock); adis->spi = spi; adis->data = data; iio_device_set_drvdata(indio_dev, adis); diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c index 9ac8356d9a95..bf581a2c321d 100644 --- a/drivers/iio/imu/adis_buffer.c +++ b/drivers/iio/imu/adis_buffer.c @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) return -ENOMEM; if (adis->data->has_paging) { - mutex_lock(&adis->txrx_lock); + mutex_lock(&adis->state_lock); if (adis->current_page != 0) { adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID); adis->tx[1] = 0; @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) if (adis->data->has_paging) { adis->current_page = 0; - mutex_unlock(&adis->txrx_lock); + mutex_unlock(&adis->state_lock); } iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer, diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h index 4c53815bb729..3ed5eceaac2d 100644 --- a/include/linux/iio/imu/adis.h +++ b/include/linux/iio/imu/adis.h @@ -61,7 +61,7 @@ struct adis { const struct adis_data *data; struct adis_burst *burst; - struct mutex txrx_lock; + struct mutex state_lock; struct spi_message msg; struct spi_transfer *xfer; unsigned int current_page;
The lock can be extended a bit to protect other elements that are not particular to just TX/RX. Another idea would have been to just add a new `state_lock`, but that would mean 2 locks which would be redundant, and probably cause more potential for dead-locks. What will be done in the next patches, will be to add some unlocked versions for read/write_reg functions. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/imu/adis.c | 10 +++++----- drivers/iio/imu/adis_buffer.c | 4 ++-- include/linux/iio/imu/adis.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)