Re: [PATCH 4/4] Input: hynitron-cst816x: time out gesture key release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] input: spurious key release due to timer callback race
--

commit 84ec9fce5971ed83d872b86322e5cd6170da5358
Author: Daniel Golle <daniel@makrotopia.org>

Input: hynitron-cst816x: time out gesture key release

This commit adds a 40ms timer to release gesture keys if a new touch
event with a gesture code is not reported in time. It helps prevent
keys from remaining stuck in the pressed state when the panel stops
sending events.

> diff --git a/drivers/input/touchscreen/hynitron-cst816x.c b/drivers/input/touchscreen/hynitron-cst816x.c
> index e2093518e8af1..30f8ea314ed2c 100644
> --- a/drivers/input/touchscreen/hynitron-cst816x.c
> +++ b/drivers/input/touchscreen/hynitron-cst816x.c
[ ... ]
> @@ -177,33 +184,65 @@ static void cst816x_release_keys(struct cst816x_priv *priv)
[ ... ]
> +static void cst816x_release_timeout(struct timer_list *t)
> +{
> +	struct cst816x_priv *priv = timer_container_of(priv, t, release_timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (cst816x_release_keys(priv))
> +		input_sync(priv->input);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +}

[Severity: Medium]
Could this unconditionally release keys even if the timer was just
concurrently re-armed by the IRQ handler?

If the timer softirq and the threaded IRQ handler (cst816x_irq_cb) run
concurrently on different CPUs, the timer callback might spin waiting for
the lock while the IRQ handler is reporting a new key press and re-arming
the timer.

Once the IRQ handler drops the lock, the timer callback acquires it and
unconditionally executes cst816x_release_keys(), undoing the IRQ handler's
action and causing a spurious release of gesture keys.

Would it be safer to check if the timer was re-enqueued (e.g., via
timer_pending()) while waiting for the lock before releasing the keys?

>  static irqreturn_t cst816x_irq_cb(int irq, void *cookie)
>  {
>  	struct cst816x_priv *priv = cookie;
>  	struct cst816x_touch tch;
> +	unsigned long flags;
>  
>  	if (!cst816x_process_touch(priv, &tch))
>  		return IRQ_HANDLED;
>  
> +	spin_lock_irqsave(&priv->lock, flags);
> +
>  	touchscreen_report_pos(priv->input, &priv->prop,
>  			       tch.abs_x, tch.abs_y, false);
>  
> -	if (tch.gest)
> +	if (tch.gest) {
>  		input_report_key(priv->input,
>  				 priv->keycode[cst816x_gest_idx(tch.gest)], 1);
> -	else
> +		mod_timer(&priv->release_timer,
> +			  jiffies + msecs_to_jiffies(CST816X_GEST_HOLD_MS));

[Severity: Medium]
Does re-arming the timer here race with the timer callback executing on
another CPU?

Because the timer callback does not check if the timer was re-armed, the
newly reported key press here could be immediately reversed by the racing
timer callback once the IRQ handler releases the lock.

> +	} else {
> +		timer_delete(&priv->release_timer);
>  		cst816x_release_keys(priv);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/d18629ede3f2fd5a08d0cbdaf82f4254e89500ed.1790851590.git.daniel@makrotopia.org?part=4