| Message ID | 20200928131333.36646-4-mircea.caprioru@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] iio: adc: spear_adc: Replace indio_dev->mlock with own device lock | expand |
On Mon, 28 Sep 2020 16:13:32 +0300 Mircea Caprioru <mircea.caprioru@analog.com> wrote: > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > As part of the general cleanup of indio_dev->mlock, this change replaces > it with a local lock on the device's state structure. > > This is part of a bigger cleanup. > Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/ > > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com> There are more problems in the locking in here than just this one. See below. The taking of mlock like this was what originally motivated the efforts to hide it away from drivers. In this particular case I don't think a local lock is the correct solution. Thanks, Jonathan > --- > drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c > index 1d794cf3e3f1..b7d583993f0b 100644 > --- a/drivers/iio/adc/vf610_adc.c > +++ b/drivers/iio/adc/vf610_adc.c > @@ -168,6 +168,15 @@ struct vf610_adc { > > struct completion completion; > u16 buffer[8]; Side note. That buffer isn't correctly aligned. I'll add this one to my next series fixing those. > + /* > + * Lock to protect the device state during a potential concurrent > + * read access from userspace. Reading a raw value requires a sequence > + * of register writes, then a wait for a completion callback, > + * and finally a register read, during which userspace could issue > + * another read request. This lock protects a read access from > + * ocurring before another one has finished. > + */ > + struct mutex lock; > }; > > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 }; > @@ -464,11 +473,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev, > { > struct vf610_adc *info = iio_priv(indio_dev); > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&info->lock); Hmm. So there is a bit of a question on what the locking here is doing. (see below for a different use of mlock). What it will do currently is to prevent the conversion mode changing whilst we are in buffered mode. It will also protect against concurrent calls of this function. I would replace this with iio_device_claim_direct_mode() rather than a local lock. > info->adc_feature.conv_mode = mode; > vf610_adc_calculate_rates(info); > vf610_adc_hw_init(info); > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > > return 0; > } > @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > switch (mask) { > case IIO_CHAN_INFO_RAW: > case IIO_CHAN_INFO_PROCESSED: > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&info->lock); > if (iio_buffer_enabled(indio_dev)) { > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); Should be use iio_device_claim_direct_mode() mlock is being taken here to stop us entering buffered mode. Whilst I'd rather a driver didn't rely on internal details of IIO, it is rather fiddly to get the locking right when there is a completion going on, so I think here you are safe to do so. > return -EBUSY; > } > > @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > ret = wait_for_completion_interruptible_timeout > (&info->completion, VF610_ADC_TIMEOUT); > if (ret == 0) { > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > return -ETIMEDOUT; > } > if (ret < 0) { > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > return ret; > } > > @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > break; > default: > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > return -EINVAL; > } > > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev) > } > > info = iio_priv(indio_dev); > + > + mutex_init(&info->lock); > + > info->dev = &pdev->dev; > > info->regs = devm_platform_ioremap_resource(pdev, 0);
On Tue, Sep 29, 2020 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 28 Sep 2020 16:13:32 +0300 > Mircea Caprioru <mircea.caprioru@analog.com> wrote: > > > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > > > As part of the general cleanup of indio_dev->mlock, this change replaces > > it with a local lock on the device's state structure. > > > > This is part of a bigger cleanup. > > Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/ > > > > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com> > > There are more problems in the locking in here than just this one. > See below. The taking of mlock like this was what originally motivated > the efforts to hide it away from drivers. > > In this particular case I don't think a local lock is the correct solution. > > Thanks, > > Jonathan > > > > --- > > drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c > > index 1d794cf3e3f1..b7d583993f0b 100644 > > --- a/drivers/iio/adc/vf610_adc.c > > +++ b/drivers/iio/adc/vf610_adc.c > > @@ -168,6 +168,15 @@ struct vf610_adc { > > > > struct completion completion; > > u16 buffer[8]; > > Side note. That buffer isn't correctly aligned. I'll add this one to > my next series fixing those. > > > + /* > > + * Lock to protect the device state during a potential concurrent > > + * read access from userspace. Reading a raw value requires a sequence > > + * of register writes, then a wait for a completion callback, > > + * and finally a register read, during which userspace could issue > > + * another read request. This lock protects a read access from > > + * ocurring before another one has finished. > > + */ > > + struct mutex lock; > > }; > > > > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 }; > > @@ -464,11 +473,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev, > > { > > struct vf610_adc *info = iio_priv(indio_dev); > > > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&info->lock); > Hmm. So there is a bit of a question on what the locking here is doing. > (see below for a different use of mlock). > > What it will do currently is to prevent the conversion mode changing whilst > we are in buffered mode. It will also protect against concurrent > calls of this function. > > I would replace this with iio_device_claim_direct_mode() rather than a > local lock. This raises a new question: if there's any drivers that we missed [for iio_device_claim_direct_mode()]. While I was aware of iio_device_claim_direct_mode(), I missed this fact when pushing the mlock cleanup. Oh well, I'll do a quick audit over the current drivers that were converted. Hopefully I don't find anything :P > > > info->adc_feature.conv_mode = mode; > > vf610_adc_calculate_rates(info); > > vf610_adc_hw_init(info); > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > > > return 0; > > } > > @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > switch (mask) { > > case IIO_CHAN_INFO_RAW: > > case IIO_CHAN_INFO_PROCESSED: > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&info->lock); > > if (iio_buffer_enabled(indio_dev)) { > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > Should be use iio_device_claim_direct_mode() > > mlock is being taken here to stop us entering buffered mode. > > Whilst I'd rather a driver didn't rely on internal details of > IIO, it is rather fiddly to get the locking right when there is a completion > going on, so I think here you are safe to do so. > > > return -EBUSY; > > } > > > > @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > ret = wait_for_completion_interruptible_timeout > > (&info->completion, VF610_ADC_TIMEOUT); > > if (ret == 0) { > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > return -ETIMEDOUT; > > } > > if (ret < 0) { > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > return ret; > > } > > > > @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > > > break; > > default: > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > return -EINVAL; > > } > > > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&info->lock); > > return IIO_VAL_INT; > > > > case IIO_CHAN_INFO_SCALE: > > @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev) > > } > > > > info = iio_priv(indio_dev); > > + > > + mutex_init(&info->lock); > > + > > info->dev = &pdev->dev; > > > > info->regs = devm_platform_ioremap_resource(pdev, 0); >
On Wed, 30 Sep 2020 08:57:55 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Tue, Sep 29, 2020 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Mon, 28 Sep 2020 16:13:32 +0300 > > Mircea Caprioru <mircea.caprioru@analog.com> wrote: > > > > > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > > > > > As part of the general cleanup of indio_dev->mlock, this change replaces > > > it with a local lock on the device's state structure. > > > > > > This is part of a bigger cleanup. > > > Link: https://lore.kernel.org/linux-iio/CA+U=Dsoo6YABe5ODLp+eFNPGFDjk5ZeQEceGkqjxXcVEhLWubw@mail.gmail.com/ > > > > > > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com> > > > > There are more problems in the locking in here than just this one. > > See below. The taking of mlock like this was what originally motivated > > the efforts to hide it away from drivers. > > > > In this particular case I don't think a local lock is the correct solution. > > > > Thanks, > > > > Jonathan > > > > > > > --- > > > drivers/iio/adc/vf610_adc.c | 28 ++++++++++++++++++++-------- > > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c > > > index 1d794cf3e3f1..b7d583993f0b 100644 > > > --- a/drivers/iio/adc/vf610_adc.c > > > +++ b/drivers/iio/adc/vf610_adc.c > > > @@ -168,6 +168,15 @@ struct vf610_adc { > > > > > > struct completion completion; > > > u16 buffer[8]; > > > > Side note. That buffer isn't correctly aligned. I'll add this one to > > my next series fixing those. > > > > > + /* > > > + * Lock to protect the device state during a potential concurrent > > > + * read access from userspace. Reading a raw value requires a sequence > > > + * of register writes, then a wait for a completion callback, > > > + * and finally a register read, during which userspace could issue > > > + * another read request. This lock protects a read access from > > > + * ocurring before another one has finished. > > > + */ > > > + struct mutex lock; > > > }; > > > > > > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 }; > > > @@ -464,11 +473,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev, > > > { > > > struct vf610_adc *info = iio_priv(indio_dev); > > > > > > - mutex_lock(&indio_dev->mlock); > > > + mutex_lock(&info->lock); > > Hmm. So there is a bit of a question on what the locking here is doing. > > (see below for a different use of mlock). > > > > What it will do currently is to prevent the conversion mode changing whilst > > we are in buffered mode. It will also protect against concurrent > > calls of this function. > > > > I would replace this with iio_device_claim_direct_mode() rather than a > > local lock. > > This raises a new question: if there's any drivers that we missed [for > iio_device_claim_direct_mode()]. > While I was aware of iio_device_claim_direct_mode(), I missed this > fact when pushing the mlock cleanup. > > Oh well, I'll do a quick audit over the current drivers that were converted. > Hopefully I don't find anything :P I was keeping an eye out for this, so hopefully I didn't miss any! Good to check though :) Jonathan > > > > > > info->adc_feature.conv_mode = mode; > > > vf610_adc_calculate_rates(info); > > > vf610_adc_hw_init(info); > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > > > > return 0; > > > } > > > @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > > switch (mask) { > > > case IIO_CHAN_INFO_RAW: > > > case IIO_CHAN_INFO_PROCESSED: > > > - mutex_lock(&indio_dev->mlock); > > > + mutex_lock(&info->lock); > > > if (iio_buffer_enabled(indio_dev)) { > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > > Should be use iio_device_claim_direct_mode() > > > > mlock is being taken here to stop us entering buffered mode. > > > > Whilst I'd rather a driver didn't rely on internal details of > > IIO, it is rather fiddly to get the locking right when there is a completion > > going on, so I think here you are safe to do so. > > > > > return -EBUSY; > > > } > > > > > > @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > > ret = wait_for_completion_interruptible_timeout > > > (&info->completion, VF610_ADC_TIMEOUT); > > > if (ret == 0) { > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > return -ETIMEDOUT; > > > } > > > if (ret < 0) { > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > return ret; > > > } > > > > > > @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, > > > > > > break; > > > default: > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > return -EINVAL; > > > } > > > > > > - mutex_unlock(&indio_dev->mlock); > > > + mutex_unlock(&info->lock); > > > return IIO_VAL_INT; > > > > > > case IIO_CHAN_INFO_SCALE: > > > @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev) > > > } > > > > > > info = iio_priv(indio_dev); > > > + > > > + mutex_init(&info->lock); > > > + > > > info->dev = &pdev->dev; > > > > > > info->regs = devm_platform_ioremap_resource(pdev, 0); > >
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c index 1d794cf3e3f1..b7d583993f0b 100644 --- a/drivers/iio/adc/vf610_adc.c +++ b/drivers/iio/adc/vf610_adc.c @@ -168,6 +168,15 @@ struct vf610_adc { struct completion completion; u16 buffer[8]; + /* + * Lock to protect the device state during a potential concurrent + * read access from userspace. Reading a raw value requires a sequence + * of register writes, then a wait for a completion callback, + * and finally a register read, during which userspace could issue + * another read request. This lock protects a read access from + * ocurring before another one has finished. + */ + struct mutex lock; }; static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 }; @@ -464,11 +473,11 @@ static int vf610_set_conversion_mode(struct iio_dev *indio_dev, { struct vf610_adc *info = iio_priv(indio_dev); - mutex_lock(&indio_dev->mlock); + mutex_lock(&info->lock); info->adc_feature.conv_mode = mode; vf610_adc_calculate_rates(info); vf610_adc_hw_init(info); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return 0; } @@ -632,9 +641,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: case IIO_CHAN_INFO_PROCESSED: - mutex_lock(&indio_dev->mlock); + mutex_lock(&info->lock); if (iio_buffer_enabled(indio_dev)) { - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return -EBUSY; } @@ -645,11 +654,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, ret = wait_for_completion_interruptible_timeout (&info->completion, VF610_ADC_TIMEOUT); if (ret == 0) { - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return -ETIMEDOUT; } if (ret < 0) { - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return ret; } @@ -668,11 +677,11 @@ static int vf610_read_raw(struct iio_dev *indio_dev, break; default: - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return -EINVAL; } - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: @@ -807,6 +816,9 @@ static int vf610_adc_probe(struct platform_device *pdev) } info = iio_priv(indio_dev); + + mutex_init(&info->lock); + info->dev = &pdev->dev; info->regs = devm_platform_ioremap_resource(pdev, 0);