* [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases
@ 2020-10-26 13:21 Oleksij Rempel
2020-10-27 3:48 ` Dmitry Torokhov
0 siblings, 1 reply; 3+ messages in thread
From: Oleksij Rempel @ 2020-10-26 13:21 UTC (permalink / raw)
To: Dmitry Torokhov, Alexandru Ardelean
Cc: David Jander, Oleksij Rempel, kernel, linux-kernel, linux-input
From: David Jander <david@protonic.nl>
If touchscreen is released while busy reading HWMON device, the release
can be missed. The IRQ thread is not started because no touch is active
and BTN_TOUCH release event is never sent.
Fixes: f5a28a7d4858f94a ("Input: ads7846 - avoid pen up/down when reading hwmon")
Co-Developed-by: David Jander <david@protonic.nl>
Signed-off-by: David Jander <david@protonic.nl>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/input/touchscreen/ads7846.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index ea31956f3a90..0236a119c52d 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -211,10 +211,26 @@ static void ads7846_stop(struct ads7846 *ts)
}
}
+static int get_pendown_state(struct ads7846 *ts);
+
/* Must be called with ts->lock held */
static void ads7846_restart(struct ads7846 *ts)
{
+ unsigned int pdstate;
+
if (!ts->disabled && !ts->suspended) {
+ /* Check if pen was released since last stop */
+ if (ts->pendown && !get_pendown_state(ts)) {
+ struct input_dev *input = ts->input;
+
+ input_report_key(input, BTN_TOUCH, 0);
+ input_report_abs(input, ABS_PRESSURE, 0);
+ input_sync(input);
+
+ ts->pendown = false;
+ dev_vdbg(&ts->spi->dev, "UP\n");
+ }
+
/* Tell IRQ thread that it may poll the device. */
ts->stopped = false;
mb();
--
2.28.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases
2020-10-26 13:21 [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases Oleksij Rempel
@ 2020-10-27 3:48 ` Dmitry Torokhov
2020-10-27 8:45 ` Oleksij Rempel
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2020-10-27 3:48 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Alexandru Ardelean, David Jander, kernel, linux-kernel, linux-input
Hi Oleksij,
On Mon, Oct 26, 2020 at 02:21:17PM +0100, Oleksij Rempel wrote:
> From: David Jander <david@protonic.nl>
>
> If touchscreen is released while busy reading HWMON device, the release
> can be missed. The IRQ thread is not started because no touch is active
> and BTN_TOUCH release event is never sent.
>
> Fixes: f5a28a7d4858f94a ("Input: ads7846 - avoid pen up/down when reading hwmon")
> Co-Developed-by: David Jander <david@protonic.nl>
> Signed-off-by: David Jander <david@protonic.nl>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> drivers/input/touchscreen/ads7846.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index ea31956f3a90..0236a119c52d 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -211,10 +211,26 @@ static void ads7846_stop(struct ads7846 *ts)
> }
> }
>
> +static int get_pendown_state(struct ads7846 *ts);
Not a fan forward declarations, just move the definition if needed.
> +
> /* Must be called with ts->lock held */
> static void ads7846_restart(struct ads7846 *ts)
> {
> + unsigned int pdstate;
I do not see it being used. Do you have more patches for the driver?
> +
> if (!ts->disabled && !ts->suspended) {
> + /* Check if pen was released since last stop */
> + if (ts->pendown && !get_pendown_state(ts)) {
> + struct input_dev *input = ts->input;
> +
> + input_report_key(input, BTN_TOUCH, 0);
> + input_report_abs(input, ABS_PRESSURE, 0);
> + input_sync(input);
> +
> + ts->pendown = false;
> + dev_vdbg(&ts->spi->dev, "UP\n");
I wonder if we should not have ads7846_report_pen_up(struct ads7846 *ts)
> + }
> +
> /* Tell IRQ thread that it may poll the device. */
> ts->stopped = false;
> mb();
> --
> 2.28.0
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases
2020-10-27 3:48 ` Dmitry Torokhov
@ 2020-10-27 8:45 ` Oleksij Rempel
0 siblings, 0 replies; 3+ messages in thread
From: Oleksij Rempel @ 2020-10-27 8:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alexandru Ardelean, David Jander, kernel, linux-kernel, linux-input
On Mon, Oct 26, 2020 at 08:48:51PM -0700, Dmitry Torokhov wrote:
> Hi Oleksij,
>
> On Mon, Oct 26, 2020 at 02:21:17PM +0100, Oleksij Rempel wrote:
> > From: David Jander <david@protonic.nl>
> >
> > If touchscreen is released while busy reading HWMON device, the release
> > can be missed. The IRQ thread is not started because no touch is active
> > and BTN_TOUCH release event is never sent.
> >
> > Fixes: f5a28a7d4858f94a ("Input: ads7846 - avoid pen up/down when reading hwmon")
> > Co-Developed-by: David Jander <david@protonic.nl>
> > Signed-off-by: David Jander <david@protonic.nl>
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > drivers/input/touchscreen/ads7846.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> > index ea31956f3a90..0236a119c52d 100644
> > --- a/drivers/input/touchscreen/ads7846.c
> > +++ b/drivers/input/touchscreen/ads7846.c
> > @@ -211,10 +211,26 @@ static void ads7846_stop(struct ads7846 *ts)
> > }
> > }
> >
> > +static int get_pendown_state(struct ads7846 *ts);
>
> Not a fan forward declarations, just move the definition if needed.
ok
> > +
> > /* Must be called with ts->lock held */
> > static void ads7846_restart(struct ads7846 *ts)
> > {
> > + unsigned int pdstate;
>
> I do not see it being used. Do you have more patches for the driver?
Ooops. Artifact of previous version of this patch.
But, yes, there is one huge patch with major rework of this driver. I'll
need to split it before sending. Or, are you ready to accept a one big
patch? :)
> > +
> > if (!ts->disabled && !ts->suspended) {
> > + /* Check if pen was released since last stop */
> > + if (ts->pendown && !get_pendown_state(ts)) {
> > + struct input_dev *input = ts->input;
> > +
> > + input_report_key(input, BTN_TOUCH, 0);
> > + input_report_abs(input, ABS_PRESSURE, 0);
> > + input_sync(input);
> > +
> > + ts->pendown = false;
> > + dev_vdbg(&ts->spi->dev, "UP\n");
>
> I wonder if we should not have ads7846_report_pen_up(struct ads7846 *ts)
Sure, which is already done in rework patch. Should I move this change
here?
> > + }
> > +
> > /* Tell IRQ thread that it may poll the device. */
> > ts->stopped = false;
> > mb();
> > --
> > 2.28.0
> >
>
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-27 8:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 13:21 [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases Oleksij Rempel
2020-10-27 3:48 ` Dmitry Torokhov
2020-10-27 8:45 ` Oleksij Rempel
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).