diff mbox series

iio: adc: exynos: do not rely on 'users' counter in ISR

Message ID 20201005052420.GA3262631@dtor-ws (mailing list archive)
State Not Applicable
Headers show
Series iio: adc: exynos: do not rely on 'users' counter in ISR | expand

Commit Message

Dmitry Torokhov Oct. 5, 2020, 5:24 a.m. UTC
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(-)

Comments

Krzysztof Kozlowski Oct. 5, 2020, 8:32 a.m. UTC | #1
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
Michał Mirosław Oct. 5, 2020, 11:09 a.m. UTC | #2
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
Dmitry Torokhov Oct. 5, 2020, 5:36 p.m. UTC | #3
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.
Michał Mirosław Oct. 5, 2020, 7 p.m. UTC | #4
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
Dmitry Torokhov Oct. 6, 2020, 12:34 a.m. UTC | #5
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.
diff mbox series

Patch

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);
 }