Re: [PATCH v2 2/2] Input: st-keyscan: Improve probe error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver requests and leaves the interrupt enabled while the hardware clock is explicitly disabled, leading to a potential crash (synchronous external abort) upon a spurious or pending interrupt.
--

--- Patch [2]: [PATCH v2 2/2] Input: st-keyscan: Improve probe error handling ---
commit 036864a311a2de2b62ec154e1ff242ede73fb89b
Author: bui duc phuc <phucduc.bui@gmail.com>

Input: st-keyscan: Improve probe error handling

This commit improves error handling in the probe function by using
dev_err_probe() for clock fetching, removing redundant error logs for
devm_request_irq(), and returning the original error code from
platform_get_irq().

> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> index e53ef4c670e4b..8e9bac339dc47 100644
> --- a/drivers/input/keyboard/st-keyscan.c
> +++ b/drivers/input/keyboard/st-keyscan.c

[ ... ]

> @@ -188,14 +187,12 @@ static int keyscan_probe(struct platform_device *pdev)
>  
>  	keypad_data->irq = platform_get_irq(pdev, 0);
>  	if (keypad_data->irq < 0)
> -		return -EINVAL;
> +		return keypad_data->irq;
>  
>  	error = devm_request_irq(&pdev->dev, keypad_data->irq, keyscan_isr, 0,
>  				 pdev->name, keypad_data);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here while the
hardware clock is explicitly disabled risk a bus fault?

Earlier in keyscan_probe(), keyscan_stop() is called to disable the clock.
When devm_request_irq() is called here, it unmasks and enables the interrupt
while the clock remains gated.

If a spurious interrupt or a bootloader-pending interrupt occurs at this
point, keyscan_isr() will execute and attempt to read the hardware register:

drivers/input/keyboard/st-keyscan.c:keyscan_isr() {
    ...
    state = readl(keypad->base + KEYSCAN_MATRIX_STATE_OFF) & 0xffff;
    ...
}

Accessing the memory-mapped register while the IP block's clock is disabled
can trigger a synchronous external abort. The same condition appears to happen
during keyscan_close(), which disables the clock while the IRQ remains active.

Would it be safer to request the interrupt with IRQF_NO_AUTOEN or delay
requesting it until the device is actually opened and clocked?

> -	if (error) {
> -		dev_err(&pdev->dev, "failed to request IRQ\n");
> +	if (error)
>  		return error;
> -	}
>  
>  	error = input_register_device(input_dev);
>  	if (error) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720090851.230244-1-phucduc.bui@gmail.com?part=2