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

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

Pre-existing issues:
- [High] Hardware access with disabled clock in ISR causes bus faults (spurious or pending interrupts)
--

--- Patch [2]: [PATCH v2 2/2] Input: st-keyscan: Improve probe error handling ---
commit 006b58b79139d033aaee4496bf01651147f36cad
Author: bui duc phuc <[email protected]>

Input: st-keyscan: Improve probe error handling

This commit updates probe error handling by using dev_err_probe() for
devm_clk_get(), dropping a redundant error message after 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
> --- 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);
> -	if (error) {
> -		dev_err(&pdev->dev, "failed to request IRQ\n");
> +	if (error)
>  		return error;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing issue
with how the interrupt is registered here.

Since keyscan_stop() is called just before this in keyscan_probe(), the IP
block's clock is already gated. By requesting the interrupt without
IRQF_NO_AUTOEN, the interrupt is immediately unmasked at the interrupt
controller.

If a pending or spurious interrupt fires before the device is opened, could
the ISR attempt to access hardware registers while the clock is disabled?

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

Does this sequence risk a synchronous external abort (bus fault) causing a
kernel panic on this platform? 

Would it be safer to request the IRQ with IRQF_NO_AUTOEN so it remains masked
until keyscan_open() explicitly enables the clock? I noticed this same window
might exist in keyscan_close(), which disables the clock via keyscan_stop()
but leaves the IRQ unmasked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2