Re: [PATCH v1] Input: touchscreen: ads7846.c: Fix race that causes missing releases

[Oleksij Rempel]

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@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxx>
> > Signed-off-by: David Jander <david@xxxxxxxxxxx>
> > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > ---
> >  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 |