* [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
@ 2020-10-06 4:12 dmitry.torokhov
2020-10-06 7:08 ` Krzysztof Kozlowski
2020-10-06 19:39 ` Michał Mirosław
0 siblings, 2 replies; 4+ messages in thread
From: dmitry.torokhov @ 2020-10-06 4:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Kukjin Kim, Krzysztof Kozlowski, Andrzej Pietrasiewicz,
Michał Mirosław, linux-iio, linux-samsung-soc,
linux-kernel
The order in which 'users' counter is decremented vs calling drivers'
close() method is implementation specific, and we should not rely on
it. Let's introduce driver private flag and use it to signal ISR
to exit when device is being closed.
This has a side-effect of fixing issue of accessing inut->users
outside of input->mutex protection.
Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
v2: switched from ordinary read/write to READ_ONCE/WRITE_ONCE per Michał
Mirosław
drivers/iio/adc/exynos_adc.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 22131a677445..6c705fe599a3 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -7,6 +7,7 @@
* Copyright (C) 2013 Naveen Krishna Chatradhi <ch.naveen@samsung.com>
*/
+#include <linux/compiler.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/interrupt.h>
@@ -135,6 +136,8 @@ struct exynos_adc {
u32 value;
unsigned int version;
+ bool ts_enabled;
+
bool read_ts;
u32 ts_x;
u32 ts_y;
@@ -633,7 +636,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
bool pressed;
int ret;
- while (info->input->users) {
+ while (READ_ONCE(info->ts_enabled)) {
ret = exynos_read_s3c64xx_ts(dev, &x, &y);
if (ret == -ETIMEDOUT)
break;
@@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev)
{
struct exynos_adc *info = input_get_drvdata(dev);
+ WRITE_ONCE(info->ts_enabled, true);
enable_irq(info->tsirq);
return 0;
@@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev)
{
struct exynos_adc *info = input_get_drvdata(dev);
+ WRITE_ONCE(info->ts_enabled, true);
disable_irq(info->tsirq);
}
--
2.28.0.806.g8561365e88-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-06 4:12 [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
@ 2020-10-06 7:08 ` Krzysztof Kozlowski
2020-10-06 19:39 ` Michał Mirosław
1 sibling, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-06 7:08 UTC (permalink / raw)
To: dmitry.torokhov
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Andrzej Pietrasiewicz,
Michał Mirosław, linux-iio, linux-samsung-soc,
linux-kernel
On Tue, 6 Oct 2020 at 06:12, <dmitry.torokhov@gmail.com> wrote:
>
> The order in which 'users' counter is decremented vs calling drivers'
> close() method is implementation specific, and we should not rely on
> it. Let's introduce driver private flag and use it to signal ISR
> to exit when device is being closed.
>
> This has a side-effect of fixing issue of accessing inut->users
> outside of input->mutex protection.
>
> Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> v2: switched from ordinary read/write to READ_ONCE/WRITE_ONCE per Michał
> Mirosław
>
> drivers/iio/adc/exynos_adc.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-06 4:12 [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
2020-10-06 7:08 ` Krzysztof Kozlowski
@ 2020-10-06 19:39 ` Michał Mirosław
2020-10-06 21:50 ` dmitry.torokhov
1 sibling, 1 reply; 4+ messages in thread
From: Michał Mirosław @ 2020-10-06 19:39 UTC (permalink / raw)
To: dmitry.torokhov
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Andrzej Pietrasiewicz, linux-iio, linux-samsung-soc,
linux-kernel
On Mon, Oct 05, 2020 at 09:12:14PM -0700, dmitry.torokhov@gmail.com wrote:
> The order in which 'users' counter is decremented vs calling drivers'
> close() method is implementation specific, and we should not rely on
> it. Let's introduce driver private flag and use it to signal ISR
> to exit when device is being closed.
>
> This has a side-effect of fixing issue of accessing inut->users
> outside of input->mutex protection.
[...]
Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
(after with a fix mentioned below)
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
[...]
> @@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> {
> struct exynos_adc *info = input_get_drvdata(dev);
>
> + WRITE_ONCE(info->ts_enabled, true);
> enable_irq(info->tsirq);
>
> return 0;
> @@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> {
> struct exynos_adc *info = input_get_drvdata(dev);
>
> + WRITE_ONCE(info->ts_enabled, true);
> disable_irq(info->tsirq);
Shouldn't 'true' be 'false' here?
Best Regards,
Michał Mirosław
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-06 19:39 ` Michał Mirosław
@ 2020-10-06 21:50 ` dmitry.torokhov
0 siblings, 0 replies; 4+ messages in thread
From: dmitry.torokhov @ 2020-10-06 21:50 UTC (permalink / raw)
To: Michał Mirosław
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Andrzej Pietrasiewicz, linux-iio, linux-samsung-soc,
linux-kernel
On Tue, Oct 06, 2020 at 09:39:07PM +0200, Michał Mirosław wrote:
> On Mon, Oct 05, 2020 at 09:12:14PM -0700, dmitry.torokhov@gmail.com wrote:
> > The order in which 'users' counter is decremented vs calling drivers'
> > close() method is implementation specific, and we should not rely on
> > it. Let's introduce driver private flag and use it to signal ISR
> > to exit when device is being closed.
> >
> > This has a side-effect of fixing issue of accessing inut->users
> > outside of input->mutex protection.
> [...]
>
> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> (after with a fix mentioned below)
>
> > --- a/drivers/iio/adc/exynos_adc.c
> > +++ b/drivers/iio/adc/exynos_adc.c
> [...]
> > @@ -712,6 +715,7 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> > {
> > struct exynos_adc *info = input_get_drvdata(dev);
> >
> > + WRITE_ONCE(info->ts_enabled, true);
> > enable_irq(info->tsirq);
> >
> > return 0;
> > @@ -721,6 +725,7 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> > {
> > struct exynos_adc *info = input_get_drvdata(dev);
> >
> > + WRITE_ONCE(info->ts_enabled, true);
> > disable_irq(info->tsirq);
>
> Shouldn't 'true' be 'false' here?
I swear if we disable cut-n-paste functionality there will be markable
reduction in bug rates...
Thanks for noticing this!
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-06 21:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 4:12 [PATCH v2] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
2020-10-06 7:08 ` Krzysztof Kozlowski
2020-10-06 19:39 ` Michał Mirosław
2020-10-06 21:50 ` dmitry.torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).