On Tue, 20 Sep 2022 13:28:16 +0200
Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> In order to drop the internal lock usage we needed two different things:
>
> 1) The first place where 'mlock' was being used was a typical case where
> iio_device_claim_direct_mode() fits perfectly.
> 2) In the second case, it was being used to prevent concurrent accesses
> to the device and shared data but nothing was being enforced with
> regards to buffering (i.e, there was nothing preventing from changing
> the conversion mode while buffering). Hence, in this case, a new lock
> was introduced in the state structure.
>
> Note that the goal is not to introduce any functional change and that is
> the reason why a new lock was introduced to guarantee 2).
Hmm. If we can get enough review on this, I'd rather claim_direct_mode()
for part 2 even though it is a functional change. It's a bit tricky
as that function has no error paths currently so no real reason the
caller should be expecting errors (generic code will be fine, but generic
code doesn't know about this ABI element anyway).
A request for a refactor precursor patch in line. It will make
the end result cleaner I think.
>
> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
> drivers/iio/adc/vf610_adc.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index c6b16cf6e367..b9addd313ef1 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -7,6 +7,7 @@
>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/property.h>
> #include <linux/platform_device.h>
> #include <linux/interrupt.h>
> @@ -156,6 +157,9 @@ struct vf610_adc {
> void __iomem *regs;
> struct clk *clk;
>
> + /* lock to protect against multiple access to the device */
> + struct mutex lock;
> +
> u32 vref_uv;
> u32 value;
> struct regulator *vref;
> @@ -467,11 +471,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;
> }
> @@ -635,12 +639,11 @@ 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);
> - if (iio_buffer_enabled(indio_dev)) {
> - mutex_unlock(&indio_dev->mlock);
> - return -EBUSY;
> - }
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
>
> + mutex_lock(&info->lock);
> reinit_completion(&info->completion);
> hc_cfg = VF610_ADC_ADCHC(chan->channel);
> hc_cfg |= VF610_ADC_AIEN;
> @@ -648,11 +651,13 @@ 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);
The error paths in here are complex. I'd pull the whole of the
case block out to a new function in a precursor patch and
then you can add the new locking with a goto unlock; pattern.
> + iio_device_release_direct_mode(indio_dev);
> return -ETIMEDOUT;
> }
> if (ret < 0) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> + iio_device_release_direct_mode(indio_dev);
> return ret;
> }
>
> @@ -671,11 +676,13 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>
> break;
> default:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> + iio_device_release_direct_mode(indio_dev);
> return -EINVAL;
> }
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&info->lock);
> + iio_device_release_direct_mode(indio_dev);
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> @@ -812,6 +819,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>
> info = iio_priv(indio_dev);
> info->dev = &pdev->dev;
> + mutex_init(&info->lock);
>
> info->regs = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(info->regs))
