diff mbox series

[03/15] iio: adc: axp288_adc: do not use internal iio_dev lock

Message ID 20220920112821.975359-4-nuno.sa@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Make 'mlock' really private | expand

Commit Message

Nuno Sa Sept. 20, 2022, 11:28 a.m. UTC
The iio_device lock is only meant for internal use. Hence define a
device local lock to protect against concurrent accesses.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/axp288_adc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Sept. 20, 2022, 1:13 p.m. UTC | #1
On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.

...

>         info = iio_priv(indio_dev);
> +       mutex_init(&info->lock);
>         info->irq = platform_get_irq(pdev, 0);
>         if (info->irq < 0)
>                 return info->irq;

Consider initializing it as late as possible, like after IRQ retrieval
in this context (maybe even deeper, but no context available). Ditto
for the rest of the series.
Nuno Sa Sept. 20, 2022, 1:18 p.m. UTC | #2
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, September 20, 2022 3:13 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Jonathan Cameron <jic23@kernel.org>; Neil Armstrong
> <narmstrong@baylibre.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Jyoti Bhayana <jbhayana@google.com>;
> Chen-Yu Tsai <wens@csie.org>; Orson Zhai <orsonzhai@gmail.com>
> Subject: Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev
> lock
> 
> [External]
> 
> On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > The iio_device lock is only meant for internal use. Hence define a
> > device local lock to protect against concurrent accesses.
> 
> ...
> 
> >         info = iio_priv(indio_dev);
> > +       mutex_init(&info->lock);
> >         info->irq = platform_get_irq(pdev, 0);
> >         if (info->irq < 0)
> >                 return info->irq;
> 
> Consider initializing it as late as possible, like after IRQ retrieval
> in this context (maybe even deeper, but no context available). Ditto
> for the rest of the series.

Any special reason for it (maybe related to lockdep :wondering: ) ? Just
curious as I never noticed such a pattern when initializing mutexes.

- Nuno Sá
Andy Shevchenko Sept. 20, 2022, 1:37 p.m. UTC | #3
On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > >         info = iio_priv(indio_dev);
> > > +       mutex_init(&info->lock);
> > >         info->irq = platform_get_irq(pdev, 0);
> > >         if (info->irq < 0)
> > >                 return info->irq;
> >
> > Consider initializing it as late as possible, like after IRQ retrieval
> > in this context (maybe even deeper, but no context available). Ditto
> > for the rest of the series.
>
> Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> curious as I never noticed such a pattern when initializing mutexes.

Yes. Micro-optimization based on the rule "don't create a resource in
case of known error".

OTOH, you have to be sure that the mutex (and generally speaking a
locking) should be initialized early enough.
Andy Shevchenko Sept. 20, 2022, 1:39 p.m. UTC | #4
On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > >         info = iio_priv(indio_dev);
> > > > +       mutex_init(&info->lock);
> > > >         info->irq = platform_get_irq(pdev, 0);
> > > >         if (info->irq < 0)
> > > >                 return info->irq;
> > >
> > > Consider initializing it as late as possible, like after IRQ retrieval
> > > in this context (maybe even deeper, but no context available). Ditto
> > > for the rest of the series.
> >
> > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > curious as I never noticed such a pattern when initializing mutexes.
>
> Yes. Micro-optimization based on the rule "don't create a resource in
> case of known error".
>
> OTOH, you have to be sure that the mutex (and generally speaking a
> locking) should be initialized early enough.

Note that "micro" in the above can be multiplied by 'k', where 'k' is
the amount of deferred probes (probably not the case here, but again,
"generally speaking").
Nuno Sa Sept. 20, 2022, 1:46 p.m. UTC | #5
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, September 20, 2022 3:39 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Jonathan Cameron <jic23@kernel.org>; Neil Armstrong
> <narmstrong@baylibre.com>; Baolin Wang
> <baolin.wang@linux.alibaba.com>; Jyoti Bhayana <jbhayana@google.com>;
> Chen-Yu Tsai <wens@csie.org>; Orson Zhai <orsonzhai@gmail.com>
> Subject: Re: [PATCH 03/15] iio: adc: axp288_adc: do not use internal iio_dev
> lock
> 
> [External]
> 
> On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com>
> wrote:
> 
> ...
> 
> > > > >         info = iio_priv(indio_dev);
> > > > > +       mutex_init(&info->lock);
> > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > >         if (info->irq < 0)
> > > > >                 return info->irq;
> > > >
> > > > Consider initializing it as late as possible, like after IRQ retrieval
> > > > in this context (maybe even deeper, but no context available). Ditto
> > > > for the rest of the series.
> > >
> > > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > > curious as I never noticed such a pattern when initializing mutexes.
> >
> > Yes. Micro-optimization based on the rule "don't create a resource in
> > case of known error".
> >
> > OTOH, you have to be sure that the mutex (and generally speaking a
> > locking) should be initialized early enough.
> 

Typically not really needed during probe...

> Note that "micro" in the above can be multiplied by 'k', where 'k' is
> the amount of deferred probes (probably not the case here, but again,
> "generally speaking").
> 

Well, I don't think 'mutex_init()' does that much that really matters in
these patches but ok, that rule is indeed a good practice that sometimes
makes a difference. And since I will definitely need a v2, I can make that
change. Where applicable, the best place is probably before registering the
IIO device...

- Nuno Sá
Andy Shevchenko Sept. 20, 2022, 3:12 p.m. UTC | #6
On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Tuesday, September 20, 2022 3:39 PM
> > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá <nuno.sa@analog.com>
> > wrote:
> >
> > ...
> >
> > > > > >         info = iio_priv(indio_dev);
> > > > > > +       mutex_init(&info->lock);
> > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > >         if (info->irq < 0)
> > > > > >                 return info->irq;
> > > > >
> > > > > Consider initializing it as late as possible, like after IRQ retrieval
> > > > > in this context (maybe even deeper, but no context available). Ditto
> > > > > for the rest of the series.
> > > >
> > > > Any special reason for it (maybe related to lockdep :wondering: ) ? Just
> > > > curious as I never noticed such a pattern when initializing mutexes.
> > >
> > > Yes. Micro-optimization based on the rule "don't create a resource in
> > > case of known error".
> > >
> > > OTOH, you have to be sure that the mutex (and generally speaking a
> > > locking) should be initialized early enough.
>
> Typically not really needed during probe...

Actually as long as you expose the ABI to the user anything can
happen, means that your code should be ready to receive the requests
in any possible callbacks / file ops. Same applies to the IRQ handler.
So it's very important to initialize locking just in time. Hence I can
say that "typically it needs to be carefully placed".

> > Note that "micro" in the above can be multiplied by 'k', where 'k' is
> > the amount of deferred probes (probably not the case here, but again,
> > "generally speaking").
>
> Well, I don't think 'mutex_init()' does that much that really matters in
> these patches but ok, that rule is indeed a good practice that sometimes
> makes a difference. And since I will definitely need a v2, I can make that
> change. Where applicable, the best place is probably before registering the
> IIO device...

Some drivers are pedantic and want to have mutex_destroy() to be
called, it also reduces the surface of returning with an undestroyed
object (let's not discuss the usefulness of such destruction, but in
principle).
Nuno Sá Sept. 21, 2022, 9:07 a.m. UTC | #7
On Tue, 2022-09-20 at 18:12 +0300, Andy Shevchenko wrote:
> On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Tuesday, September 20, 2022 3:39 PM
> > > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com>
> > > > wrote:
> > > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá
> > > > > > <nuno.sa@analog.com>
> > > wrote:
> > > 
> > > ...
> > > 
> > > > > > >         info = iio_priv(indio_dev);
> > > > > > > +       mutex_init(&info->lock);
> > > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > > >         if (info->irq < 0)
> > > > > > >                 return info->irq;
> > > > > > 
> > > > > > Consider initializing it as late as possible, like after
> > > > > > IRQ retrieval
> > > > > > in this context (maybe even deeper, but no context
> > > > > > available). Ditto
> > > > > > for the rest of the series.
> > > > > 
> > > > > Any special reason for it (maybe related to lockdep
> > > > > :wondering: ) ? Just
> > > > > curious as I never noticed such a pattern when initializing
> > > > > mutexes.
> > > > 
> > > > Yes. Micro-optimization based on the rule "don't create a
> > > > resource in
> > > > case of known error".
> > > > 
> > > > OTOH, you have to be sure that the mutex (and generally
> > > > speaking a
> > > > locking) should be initialized early enough.
> > 
> > Typically not really needed during probe...
> 
> Actually as long as you expose the ABI to the user anything can
> happen, means that your code should be ready to receive the requests
> in any possible callbacks / file ops. Same applies to the IRQ
> handler.
> So it's very important to initialize locking just in time. Hence I
> can
> say that "typically it needs to be carefully placed".
> 

Yes, I'm aware of that... For some reason I just thought about code
paths directly on probe. Anyways, hopefully these drivers mostly do the
right thing and register the IIO device as late as possible (ideally
the last thing to be done). The same goes for IRQs and for IIO, when
used as part of triggered buffering, the lock is often only used in the
trigger handler which means it's only reachable after the ABI is
exposed... 

- Nuno Sá
> >
Jonathan Cameron Sept. 24, 2022, 3:23 p.m. UTC | #8
On Wed, 21 Sep 2022 11:07:50 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-09-20 at 18:12 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 20, 2022 at 4:46 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:  
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Tuesday, September 20, 2022 3:39 PM
> > > > On Tue, Sep 20, 2022 at 4:37 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:  
> > > > > On Tue, Sep 20, 2022 at 4:18 PM Sa, Nuno <Nuno.Sa@analog.com>
> > > > > wrote:  
> > > > > > > On Tue, Sep 20, 2022 at 2:28 PM Nuno Sá
> > > > > > > <nuno.sa@analog.com>  
> > > > wrote:
> > > > 
> > > > ...
> > > >   
> > > > > > > >         info = iio_priv(indio_dev);
> > > > > > > > +       mutex_init(&info->lock);
> > > > > > > >         info->irq = platform_get_irq(pdev, 0);
> > > > > > > >         if (info->irq < 0)
> > > > > > > >                 return info->irq;  
> > > > > > > 
> > > > > > > Consider initializing it as late as possible, like after
> > > > > > > IRQ retrieval
> > > > > > > in this context (maybe even deeper, but no context
> > > > > > > available). Ditto
> > > > > > > for the rest of the series.  
> > > > > > 
> > > > > > Any special reason for it (maybe related to lockdep
> > > > > > :wondering: ) ? Just
> > > > > > curious as I never noticed such a pattern when initializing
> > > > > > mutexes.  
> > > > > 
> > > > > Yes. Micro-optimization based on the rule "don't create a
> > > > > resource in
> > > > > case of known error".
> > > > > 
> > > > > OTOH, you have to be sure that the mutex (and generally
> > > > > speaking a
> > > > > locking) should be initialized early enough.  
> > > 
> > > Typically not really needed during probe...  
> > 
> > Actually as long as you expose the ABI to the user anything can
> > happen, means that your code should be ready to receive the requests
> > in any possible callbacks / file ops. Same applies to the IRQ
> > handler.
> > So it's very important to initialize locking just in time. Hence I
> > can
> > say that "typically it needs to be carefully placed".
> >   
> 
> Yes, I'm aware of that... For some reason I just thought about code
> paths directly on probe. Anyways, hopefully these drivers mostly do the
> right thing and register the IIO device as late as possible (ideally
> the last thing to be done). The same goes for IRQs and for IIO, when
> used as part of triggered buffering, the lock is often only used in the
> trigger handler which means it's only reachable after the ABI is
> exposed... 

Can't say I feel that strongly about a mutex_init() placement, but
no harm in moving them later - indeed before the iio_device_register()
should be correct - though care needed as might be some unnecessary locks
taken in probe because of code sharing (and them previously being harmless)

Jonathan
> 
> - Nuno Sá
> > >
Jonathan Cameron Sept. 24, 2022, 3:24 p.m. UTC | #9
On Tue, 20 Sep 2022 13:28:09 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> The iio_device lock is only meant for internal use. Hence define a
> device local lock to protect against concurrent accesses.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Looks good. Not sure how we missed cleaning this one up earlier
given how simple it is!  Anyhow, given Andy's feedback I'll wait for v2
to pick this up.

> ---
>  drivers/iio/adc/axp288_adc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 580361bd9849..3bbb96156739 100644
> --- a/drivers/iio/adc/axp288_adc.c
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/dmi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/kernel.h>
>  #include <linux/device.h>
>  #include <linux/regmap.h>
> @@ -50,6 +51,8 @@ enum axp288_adc_id {
>  struct axp288_adc_info {
>  	int irq;
>  	struct regmap *regmap;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
>  	bool ts_enabled;
>  };
>  
> @@ -161,7 +164,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  	struct axp288_adc_info *info = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&info->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
> @@ -178,7 +181,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  	default:
>  		ret = -EINVAL;
>  	}
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&info->lock);
>  
>  	return ret;
>  }
> @@ -264,6 +267,7 @@ static int axp288_adc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	info = iio_priv(indio_dev);
> +	mutex_init(&info->lock);
>  	info->irq = platform_get_irq(pdev, 0);
>  	if (info->irq < 0)
>  		return info->irq;
diff mbox series

Patch

diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
index 580361bd9849..3bbb96156739 100644
--- a/drivers/iio/adc/axp288_adc.c
+++ b/drivers/iio/adc/axp288_adc.c
@@ -9,6 +9,7 @@ 
 
 #include <linux/dmi.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
 #include <linux/regmap.h>
@@ -50,6 +51,8 @@  enum axp288_adc_id {
 struct axp288_adc_info {
 	int irq;
 	struct regmap *regmap;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
 	bool ts_enabled;
 };
 
@@ -161,7 +164,7 @@  static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	struct axp288_adc_info *info = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&info->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
@@ -178,7 +181,7 @@  static int axp288_adc_read_raw(struct iio_dev *indio_dev,
 	default:
 		ret = -EINVAL;
 	}
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&info->lock);
 
 	return ret;
 }
@@ -264,6 +267,7 @@  static int axp288_adc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info = iio_priv(indio_dev);
+	mutex_init(&info->lock);
 	info->irq = platform_get_irq(pdev, 0);
 	if (info->irq < 0)
 		return info->irq;