* [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
@ 2020-10-05 5:24 dmitry.torokhov
2020-10-05 8:32 ` Krzysztof Kozlowski
2020-10-05 11:09 ` Michał Mirosław
0 siblings, 2 replies; 6+ messages in thread
From: dmitry.torokhov @ 2020-10-05 5:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Kukjin Kim, Krzysztof Kozlowski, Michał Mirosław,
Andrzej Pietrasiewicz, 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>
---
drivers/iio/adc/exynos_adc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 22131a677445..7eb2a5df6e98 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -135,6 +135,8 @@ struct exynos_adc {
u32 value;
unsigned int version;
+ bool ts_enabled;
+
bool read_ts;
u32 ts_x;
u32 ts_y;
@@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
bool pressed;
int ret;
- while (info->input->users) {
+ while (info->ts_enabled) {
ret = exynos_read_s3c64xx_ts(dev, &x, &y);
if (ret == -ETIMEDOUT)
break;
@@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev)
{
struct exynos_adc *info = input_get_drvdata(dev);
+ info->ts_enabled = true;
+ mb();
enable_irq(info->tsirq);
return 0;
@@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev)
{
struct exynos_adc *info = input_get_drvdata(dev);
+ info->ts_enabled = false;
+ mb();
disable_irq(info->tsirq);
}
--
2.28.0.806.g8561365e88-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-05 5:24 [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
@ 2020-10-05 8:32 ` Krzysztof Kozlowski
2020-10-05 11:09 ` Michał Mirosław
1 sibling, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-05 8:32 UTC (permalink / raw)
To: dmitry.torokhov
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Michał Mirosław,
Andrzej Pietrasiewicz, linux-iio, linux-samsung-soc,
linux-kernel
On Sun, Oct 04, 2020 at 10:24:20PM -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.
>
> Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/iio/adc/exynos_adc.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-05 5:24 [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
2020-10-05 8:32 ` Krzysztof Kozlowski
@ 2020-10-05 11:09 ` Michał Mirosław
2020-10-05 17:36 ` dmitry.torokhov
1 sibling, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2020-10-05 11:09 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 Sun, Oct 04, 2020 at 10:24:20PM -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.
>
> Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/iio/adc/exynos_adc.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 22131a677445..7eb2a5df6e98 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -135,6 +135,8 @@ struct exynos_adc {
> u32 value;
> unsigned int version;
>
> + bool ts_enabled;
> +
> bool read_ts;
> u32 ts_x;
> u32 ts_y;
> @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> bool pressed;
> int ret;
>
> - while (info->input->users) {
> + while (info->ts_enabled) {
> ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> if (ret == -ETIMEDOUT)
> break;
> @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> {
> struct exynos_adc *info = input_get_drvdata(dev);
>
> + info->ts_enabled = true;
> + mb();
> enable_irq(info->tsirq);
>
> return 0;
> @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> {
> struct exynos_adc *info = input_get_drvdata(dev);
>
> + info->ts_enabled = false;
> + mb();
> disable_irq(info->tsirq);
This should be WRITE_ONCE paired with READ_ONCE in the ISR.
But is the check really needed? I see that this is to break waiting for
a touch release event, so I would assume this shouldn't wait forever
(unless the hardware is buggy) and breaking the loop will desync touch
state (I would guess this would be noticable by next user).
Best Regards,
Michał Mirosław
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-05 11:09 ` Michał Mirosław
@ 2020-10-05 17:36 ` dmitry.torokhov
2020-10-05 19:00 ` Michał Mirosław
0 siblings, 1 reply; 6+ messages in thread
From: dmitry.torokhov @ 2020-10-05 17:36 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
Hi Michał,
On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote:
> On Sun, Oct 04, 2020 at 10:24:20PM -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.
> >
> > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/iio/adc/exynos_adc.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> > index 22131a677445..7eb2a5df6e98 100644
> > --- a/drivers/iio/adc/exynos_adc.c
> > +++ b/drivers/iio/adc/exynos_adc.c
> > @@ -135,6 +135,8 @@ struct exynos_adc {
> > u32 value;
> > unsigned int version;
> >
> > + bool ts_enabled;
> > +
> > bool read_ts;
> > u32 ts_x;
> > u32 ts_y;
> > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> > bool pressed;
> > int ret;
> >
> > - while (info->input->users) {
> > + while (info->ts_enabled) {
> > ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> > if (ret == -ETIMEDOUT)
> > break;
> > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> > {
> > struct exynos_adc *info = input_get_drvdata(dev);
> >
> > + info->ts_enabled = true;
> > + mb();
> > enable_irq(info->tsirq);
> >
> > return 0;
> > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> > {
> > struct exynos_adc *info = input_get_drvdata(dev);
> >
> > + info->ts_enabled = false;
> > + mb();
> > disable_irq(info->tsirq);
>
> This should be WRITE_ONCE paired with READ_ONCE in the ISR.
Why? I can see that maybe full memory barrier is too heavy when we set
the flag to true, but the only requirement is for the flag to be set
before we disable IRQ, so any additional guarantees provided by
WRITE_ONCE are not needed. On the read side we want the flag to be
noticed eventually, and there is no additional dependencies on the data,
so it is unclear what READ_ONCE() will give us here.
>
> But is the check really needed? I see that this is to break waiting for
> a touch release event, so I would assume this shouldn't wait forever
> (unless the hardware is buggy)
It is not hardware, it is user. Do you want to delay indefinitely
close() just because user has a finger on the touchscreen?
> and breaking the loop will desync touch
> state (I would guess this would be noticable by next user).
Upon next open driver will service the interrupt and provide new set of
touch coordinates. Userspace is supposed to query current state of
device when opening it before starting processing events. Or you are
concerned about some other state?
In any case, this is current driver behavior and if it needs to be
changed it is a topic for a separate patch.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-05 17:36 ` dmitry.torokhov
@ 2020-10-05 19:00 ` Michał Mirosław
2020-10-06 0:34 ` dmitry.torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2020-10-05 19:00 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 10:36:36AM -0700, dmitry.torokhov@gmail.com wrote:
> Hi Michał,
>
> On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote:
> > On Sun, Oct 04, 2020 at 10:24:20PM -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.
> > >
> > > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > > drivers/iio/adc/exynos_adc.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> > > index 22131a677445..7eb2a5df6e98 100644
> > > --- a/drivers/iio/adc/exynos_adc.c
> > > +++ b/drivers/iio/adc/exynos_adc.c
> > > @@ -135,6 +135,8 @@ struct exynos_adc {
> > > u32 value;
> > > unsigned int version;
> > >
> > > + bool ts_enabled;
> > > +
> > > bool read_ts;
> > > u32 ts_x;
> > > u32 ts_y;
> > > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> > > bool pressed;
> > > int ret;
> > >
> > > - while (info->input->users) {
> > > + while (info->ts_enabled) {
> > > ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> > > if (ret == -ETIMEDOUT)
> > > break;
> > > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> > > {
> > > struct exynos_adc *info = input_get_drvdata(dev);
> > >
> > > + info->ts_enabled = true;
> > > + mb();
> > > enable_irq(info->tsirq);
> > >
> > > return 0;
> > > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> > > {
> > > struct exynos_adc *info = input_get_drvdata(dev);
> > >
> > > + info->ts_enabled = false;
> > > + mb();
> > > disable_irq(info->tsirq);
> >
> > This should be WRITE_ONCE paired with READ_ONCE in the ISR.
>
> Why? I can see that maybe full memory barrier is too heavy when we set
> the flag to true, but the only requirement is for the flag to be set
> before we disable IRQ, so any additional guarantees provided by
> WRITE_ONCE are not needed. On the read side we want the flag to be
> noticed eventually, and there is no additional dependencies on the data,
> so it is unclear what READ_ONCE() will give us here.
Without READ_ONCE you have no guarantee that the compiler won't optimize
'while (flag) ...' to 'if (flag) for(;;) ...'.
Maybe the platform has stronger coherency guarantees than Linux memory model
assumes, but if not, a CPU running the ISR (without paired memory barrier)
might not ever see the store from another CPU (both in current and proposed
code).
> > But is the check really needed? I see that this is to break waiting for
> > a touch release event, so I would assume this shouldn't wait forever
> > (unless the hardware is buggy)
>
> It is not hardware, it is user. Do you want to delay indefinitely
> close() just because user has a finger on the touchscreen?
Ack. This covers the why of loop breaking.
> > and breaking the loop will desync touch
> > state (I would guess this would be noticable by next user).
> Upon next open driver will service the interrupt and provide new set of
> touch coordinates. Userspace is supposed to query current state of
> device when opening it before starting processing events. Or you are
> concerned about some other state?
From the code I would expect that there is a slight window, wher when the
user releases the touch between close() and open(), the client that open()s
will see a 'pressed' state until the ISR runs again (probably immediately
because of pending interrupt). OTOH, maybe the app should be prepared
for that anyway?
> In any case, this is current driver behavior and if it needs to be
> changed it is a topic for a separate patch.
Agreed.
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR
2020-10-05 19:00 ` Michał Mirosław
@ 2020-10-06 0:34 ` dmitry.torokhov
0 siblings, 0 replies; 6+ messages in thread
From: dmitry.torokhov @ 2020-10-06 0:34 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 Mon, Oct 05, 2020 at 09:00:10PM +0200, Michał Mirosław wrote:
> On Mon, Oct 05, 2020 at 10:36:36AM -0700, dmitry.torokhov@gmail.com wrote:
> > Hi Michał,
> >
> > On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote:
> > > and breaking the loop will desync touch
> > > state (I would guess this would be noticable by next user).
> > Upon next open driver will service the interrupt and provide new set of
> > touch coordinates. Userspace is supposed to query current state of
> > device when opening it before starting processing events. Or you are
> > concerned about some other state?
>
> From the code I would expect that there is a slight window, wher when the
> user releases the touch between close() and open(), the client that open()s
> will see a 'pressed' state until the ISR runs again (probably immediately
> because of pending interrupt). OTOH, maybe the app should be prepared
> for that anyway?
I think users should be prepared for it. There could be many users, so
anyone opening interface device (evdev or similar) can not expect that
the hardware device is in a quiesce state; it could just happen that
finger was on the surface and will be released as someone is opening the
device anyway.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-06 0:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 5:24 [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR dmitry.torokhov
2020-10-05 8:32 ` Krzysztof Kozlowski
2020-10-05 11:09 ` Michał Mirosław
2020-10-05 17:36 ` dmitry.torokhov
2020-10-05 19:00 ` Michał Mirosław
2020-10-06 0:34 ` dmitry.torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.