| Message ID | 20190919115716.25909-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: gyro: adis16260: remove indio_dev mlock | expand |
On Thu, 19 Sep 2019 14:57:16 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > The internal lock that is by the ADIS library should be sufficient to keep > state consistent. There is no need for an extra lock. I'm not sure that's true. In theory we could get two different attempts to set the sampling frequency running concurrently. That could lead to a race between the point where we set the spi frequency for future messages and the point where we set the devices sampling frequency. Bang it stops working. So, whilst it is arguably paranoid I think you do need a lock here, but it should be something that is driver local rather than mlock. thanks, Jonathan > > This patch removes it. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > drivers/iio/gyro/adis16260.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c > index 207a0ce13439..0fa93d02062a 100644 > --- a/drivers/iio/gyro/adis16260.c > +++ b/drivers/iio/gyro/adis16260.c > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, > addr = adis16260_addresses[chan->scan_index][1]; > return adis_write_reg_16(adis, addr, val); > case IIO_CHAN_INFO_SAMP_FREQ: > - mutex_lock(&indio_dev->mlock); > if (spi_get_device_id(adis->spi)->driver_data) > t = 256 / val; > else > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, > adis->spi->max_speed_hz = ADIS16260_SPI_FAST; > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t); > > - mutex_unlock(&indio_dev->mlock); > return ret; > } > return -EINVAL;
On Sat, 5 Oct 2019 15:38:45 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 19 Sep 2019 14:57:16 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > The internal lock that is by the ADIS library should be sufficient to keep > > state consistent. There is no need for an extra lock. > > I'm not sure that's true. In theory we could get two different attempts > to set the sampling frequency running concurrently. That could lead to > a race between the point where we set the spi frequency for future > messages and the point where we set the devices sampling frequency. > > Bang it stops working. So, whilst it is arguably paranoid I think > you do need a lock here, but it should be something that is driver > local rather than mlock. Thinking a bit more on this do we have a potential issue where we race with a read on the spi bus as we are changing this frequency? They might all be 'safe' but I haven't thought it through properly yet. > > thanks, > > Jonathan > > > > > > This patch removes it. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > drivers/iio/gyro/adis16260.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c > > index 207a0ce13439..0fa93d02062a 100644 > > --- a/drivers/iio/gyro/adis16260.c > > +++ b/drivers/iio/gyro/adis16260.c > > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, > > addr = adis16260_addresses[chan->scan_index][1]; > > return adis_write_reg_16(adis, addr, val); > > case IIO_CHAN_INFO_SAMP_FREQ: > > - mutex_lock(&indio_dev->mlock); > > if (spi_get_device_id(adis->spi)->driver_data) > > t = 256 / val; > > else > > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, > > adis->spi->max_speed_hz = ADIS16260_SPI_FAST; > > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t); > > > > - mutex_unlock(&indio_dev->mlock); > > return ret; > > } > > return -EINVAL; >
On Sat, 2019-10-05 at 15:49 +0100, Jonathan Cameron wrote: > [External] > > On Sat, 5 Oct 2019 15:38:45 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Thu, 19 Sep 2019 14:57:16 +0300 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > The internal lock that is by the ADIS library should be sufficient to > > > keep > > > state consistent. There is no need for an extra lock. > > > > I'm not sure that's true. In theory we could get two different > > attempts > > to set the sampling frequency running concurrently. That could lead to > > a race between the point where we set the spi frequency for future > > messages and the point where we set the devices sampling frequency. > > > > Bang it stops working. So, whilst it is arguably paranoid I think > > you do need a lock here, but it should be something that is driver > > local rather than mlock. > > Thinking a bit more on this do we have a potential issue where we > race with a read on the spi bus as we are changing this frequency? > They might all be 'safe' but I haven't thought it through properly yet. > To be honest, I don't know. I am seeing what you're saying about the `max_speed_hz` change and that could be an issue. That detail has slipped me. I guess, I could rework this patch to use the ADIS library's lock. thanks Alex > > thanks, > > > > Jonathan > > > > > > > This patch removes it. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > --- > > > drivers/iio/gyro/adis16260.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/iio/gyro/adis16260.c > > > b/drivers/iio/gyro/adis16260.c > > > index 207a0ce13439..0fa93d02062a 100644 > > > --- a/drivers/iio/gyro/adis16260.c > > > +++ b/drivers/iio/gyro/adis16260.c > > > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev > > > *indio_dev, > > > addr = adis16260_addresses[chan->scan_index][1]; > > > return adis_write_reg_16(adis, addr, val); > > > case IIO_CHAN_INFO_SAMP_FREQ: > > > - mutex_lock(&indio_dev->mlock); > > > if (spi_get_device_id(adis->spi)->driver_data) > > > t = 256 / val; > > > else > > > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev > > > *indio_dev, > > > adis->spi->max_speed_hz = ADIS16260_SPI_FAST; > > > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t); > > > > > > - mutex_unlock(&indio_dev->mlock); > > > return ret; > > > } > > > return -EINVAL;
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c index 207a0ce13439..0fa93d02062a 100644 --- a/drivers/iio/gyro/adis16260.c +++ b/drivers/iio/gyro/adis16260.c @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, addr = adis16260_addresses[chan->scan_index][1]; return adis_write_reg_16(adis, addr, val); case IIO_CHAN_INFO_SAMP_FREQ: - mutex_lock(&indio_dev->mlock); if (spi_get_device_id(adis->spi)->driver_data) t = 256 / val; else @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev *indio_dev, adis->spi->max_speed_hz = ADIS16260_SPI_FAST; ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t); - mutex_unlock(&indio_dev->mlock); return ret; } return -EINVAL;
The internal lock that is by the ADIS library should be sufficient to keep state consistent. There is no need for an extra lock. This patch removes it. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/gyro/adis16260.c | 2 -- 1 file changed, 2 deletions(-)