Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR

[Michał Mirosław]

On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@xxxxxxxxx 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@xxxxxxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  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