| Message ID | 20221004134909.1692021-11-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Make 'mlock' really private | expand |
> -----Original Message----- > From: Nuno Sá <nuno.sa@analog.com> > Sent: 2022年10月4日 21:49 > To: linux-amlogic@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; > linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-rockchip@lists.infradead.org > Cc: Heiko Stuebner <heiko@sntech.de>; Martin Blumenstingl > <martin.blumenstingl@googlemail.com>; Neil Armstrong > <narmstrong@baylibre.com>; Shawn Guo <shawnguo@kernel.org>; Lars-Peter > Clausen <lars@metafoo.de>; Jyoti Bhayana <jbhayana@google.com>; Hans de > Goede <hdegoede@redhat.com>; Andriy Tryshnivskyy > <andriy.tryshnivskyy@opensynergy.com>; Pengutronix Kernel Team > <kernel@pengutronix.de>; Miquel Raynal <miquel.raynal@bootlin.com>; Cixi > Geng <cixi.geng1@unisoc.com>; Baolin Wang > <baolin.wang@linux.alibaba.com>; Ciprian Regus <ciprian.regus@analog.com>; > Fabio Estevam <festevam@gmail.com>; Nuno Sá <nuno.sa@analog.com>; > Sascha Hauer <s.hauer@pengutronix.de>; Alexandru Ardelean > <aardelean@deviqon.com>; Florian Boor <florian.boor@kernelconcepts.de>; > Michael Hennerich <Michael.Hennerich@analog.com>; Orson Zhai > <orsonzhai@gmail.com>; Chen-Yu Tsai <wens@csie.org>; Chunyan Zhang > <zhang.lyra@gmail.com>; Vladimir Zapolskiy <vz@mleia.com>; Andy > Shevchenko <andy.shevchenko@gmail.com>; Jerome Brunet > <jbrunet@baylibre.com>; Bough Chen <haibo.chen@nxp.com>; Kevin Hilman > <khilman@baylibre.com>; Jonathan Cameron <jic23@kernel.org> > Subject: [PATCH v2 10/16] iio: adc: vf610_adc: vf610_adc: do not use internal > iio_dev lock > > 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). > > While at it, properly include "mutex.h" for mutex related APIs. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> Reviewed-by: Haibo Chen <haibo.chen@nxp.com> Thanks! > --- > drivers/iio/adc/vf610_adc.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c index > a6f9182d7766..ae31aafd2653 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; > } > @@ -629,12 +633,11 @@ static int vf610_read_sample(struct iio_dev > *indio_dev, > unsigned int hc_cfg; > int ret; > > - mutex_lock(&indio_dev->mlock); > - if (iio_buffer_enabled(indio_dev)) { > - ret = -EBUSY; > - goto out_unlock; > - } > + 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; > @@ -669,7 +672,8 @@ static int vf610_read_sample(struct iio_dev *indio_dev, > } > > out_unlock: > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&info->lock); > + iio_device_release_direct_mode(indio_dev); > > return ret; > } > @@ -892,6 +896,8 @@ static int vf610_adc_probe(struct platform_device > *pdev) > goto error_iio_device_register; > } > > + mutex_init(&info->lock); > + > ret = iio_device_register(indio_dev); > if (ret) { > dev_err(&pdev->dev, "Couldn't register the device.\n"); > -- > 2.37.3
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c index a6f9182d7766..ae31aafd2653 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; } @@ -629,12 +633,11 @@ static int vf610_read_sample(struct iio_dev *indio_dev, unsigned int hc_cfg; int ret; - mutex_lock(&indio_dev->mlock); - if (iio_buffer_enabled(indio_dev)) { - ret = -EBUSY; - goto out_unlock; - } + 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; @@ -669,7 +672,8 @@ static int vf610_read_sample(struct iio_dev *indio_dev, } out_unlock: - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&info->lock); + iio_device_release_direct_mode(indio_dev); return ret; } @@ -892,6 +896,8 @@ static int vf610_adc_probe(struct platform_device *pdev) goto error_iio_device_register; } + mutex_init(&info->lock); + ret = iio_device_register(indio_dev); if (ret) { dev_err(&pdev->dev, "Couldn't register the device.\n");
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). While at it, properly include "mutex.h" for mutex related APIs. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/adc/vf610_adc.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)