Re: [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The spinlock scope is too narrow, allowing the timer softirq and interrupt handler to interleave their event reporting, which can leave the key permanently stuck in the pressed state.
--

commit 115416424bcbaae3f95f1195f01f6e828f579e10
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - report press event in interrupt handler

This patch modifies the driver to report key press events directly in the
interrupt handler during system suspend to prevent event loss. It also adds a
spinlock to protect the keystate and suspended tracking flag.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c

[ ... ]

> @@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
>  	struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
>  							   check_timer);
>  	struct input_dev *input = pdata->input;
> +	bool state_changed = false;
>  	u32 state;
>  
>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>  
> -	/* only report new event if status changed */
> -	if (state ^ pdata->keystate) {
> -		pdata->keystate = state;
> +	scoped_guard(spinlock_irqsave, &pdata->lock) {
> +		/* only report new event if status changed */
> +		if (state ^ pdata->keystate) {
> +			pdata->keystate = state;
> +			state_changed = true;
> +		}
> +	}
> +
> +	if (state_changed) {
>  		input_event(input, EV_KEY, pdata->keycode, state);

[Severity: High]
Is the spinlock scope here too narrow? 

Because the lock is dropped before calling input_event(), it seems the
interrupt handler could preempt this thread and reorder the event delivery
to the input core.

>  		input_sync(input);
>  		pm_relax(pdata->input->dev.parent);

[ ... ]

> @@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  			input_sync(input);
>  			pm_relax(input->dev.parent);
>  		} else {
> +			/*
> +			 * Report key press events directly in interrupt handler to prevent event
> +			 * loss during system suspend.
> +			 */
> +			scoped_guard(spinlock_irqsave, &pdata->lock) {
> +				suspended = pdata->suspended;
> +				if (suspended) {
> +					pdata->keystate = 1;
> +					keystate = pdata->keystate;
> +				}
> +			}
> +			if (suspended) {
> +				input_report_key(input, pdata->keycode, keystate);

[Severity: High]
Could dropping the lock before calling input_report_key() lead to a race
with the timer softirq emitting its own input events?

For example, if the timer softirq reads state as 0 in
imx_imx_snvs_check_for_events(), updates pdata->keystate to 0, and drops
the lock, an interrupt could then preempt it:

CPU0 (timer softirq)
    scoped_guard(...) {
        pdata->keystate = 0;
    }

CPU0 (hardirq preempts before input_event is called)
    scoped_guard(...) {
        pdata->keystate = 1;
    }
    input_report_key(..., 1); // PRESS emitted

CPU0 (timer softirq resumes)
    input_event(..., 0);      // RELEASE emitted

In this scenario, the input core would receive PRESS followed by RELEASE,
leaving the final input state as RELEASE (0). However, pdata->keystate
remains PRESS (1).

Would this cause the next physical key press to be ignored since the timer
will see state=1, but the XOR against keystate (already 1) evaluates to 0,
effectively losing the event permanently?

> +				input_sync(input);
> +			}
>  			mod_timer(&pdata->check_timer,
>  			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=4