Re: [PATCH] Input: st-keyscan - disable interrupt while clock is gated

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Removing the hardware stop sequence in probe() fails to clear the hardware state left by the bootloader. If the bootloader left the keypad active and asserting an interrupt, this state persists, leading to immediate spurious wakeups that permanently prevent system sleep when the device is configured as a wakeup source.

Pre-existing issues:
- [High] If keyscan_start() fails during system resume (e.g. clock enable fails), the device remains marked as enabled. A subsequent close of the device will invoke keyscan_stop(). This triggers a bus fault (synchronous external abort) because writel() is called while the clock is gated, and causes unbalanced disable_irq() and clk_disable() calls.
--

commit cfa9ab2fa10d41e20ca2124965465f938faf3789
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: st-keyscan - disable interrupt while clock is gated

This commit fixes a bus fault during system suspend by requesting the
interrupt with IRQF_NO_AUTOEN. It also drops the clock enable and stop
sequence in probe, leaving the hardware untouched until opened.

> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> --- a/drivers/input/keyboard/st-keyscan.c
> +++ b/drivers/input/keyboard/st-keyscan.c
[ ... ]
> @@ -82,6 +84,8 @@ static void keyscan_stop(struct st_keyscan *keypad)
>  {
>  	writel(0, keypad->base + KEYSCAN_CONFIG_OFF);
>  
> +	disable_irq(keypad->irq);
> +
>  	clk_disable(keypad->clk);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditional
teardown risk a bus fault during close if resume fails?

If keyscan_start() fails during keyscan_resume() (for instance, if
clk_enable() fails), keyscan_start() returns an error and skips enable_irq().
The PM core aborts or logs the resume error, but the input device remains
marked as enabled and open.

When userspace subsequently closes the device, keyscan_stop() will execute
writel() with the clock still gated, which triggers a synchronous external
abort. Additionally, it will call disable_irq() and clk_disable(), causing
an unbalanced state since they were never enabled in resume.

The patch adds the unbalanced IRQ disable to the existing bus fault.

[ ... ]
> @@ -177,20 +181,12 @@ static int keyscan_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(keypad_data->clk),
>  				     "cannot get clock\n");
>  
> -	error = clk_enable(keypad_data->clk);
> -	if (error) {
> -		dev_err(&pdev->dev, "failed to enable clock\n");
> -		return error;
> -	}
> -
> -	keyscan_stop(keypad_data);
> -
>  	keypad_data->irq = platform_get_irq(pdev, 0);
>  	if (keypad_data->irq < 0)
>  		return keypad_data->irq;

[Severity: Medium]
Could removing the hardware stop sequence in keyscan_probe() cause spurious
wakeups if the bootloader leaves the hardware active?

Previously, this code ensured the hardware was quiescent and its interrupt
line deasserted before registering the device. If the bootloader hands off
the hardware in an active, interrupt-asserting state, that state now
persists.

When the system suspends and enable_irq_wake() is called, could a pending
interrupt from the bootloader's hardware state immediately wake the system,
permanently preventing sleep when the device is configured as a wakeup
source?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/apK-PZ5c97mCxBSC@google.com?part=1