| Message ID | 20220920112821.975359-4-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Make 'mlock' really private | expand |
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.
> -----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á
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.
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").
> -----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á
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).
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á > >
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á > > >
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 --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;
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(-)