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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Deferred probe errors from `platform_get_irq()` are incorrectly masked and converted to `-EINVAL`.
- [High] The interrupt handler is registered and unmasked while the hardware clock is disabled, which could lead to a crash on an unhandled or pending interrupt.
--

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

Input: st-keyscan: Improve probe error handling

Use dev_err_probe() for devm_clk_get() so deferred probe errors are
handled correctly.
Drop the redundant error message after devm_request_irq(), since the
helper already reports request failures internally.

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

[ ... ]

> @@ -192,10 +191,8 @@ static int keyscan_probe(struct platform_device *pdev)
>  
>  	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 since you are improving error handling in 
keyscan_probe(), could we also fix the error return for platform_get_irq() 
just prior to this?

Currently the code does this:

	keypad_data->irq = platform_get_irq(pdev, 0);
	if (keypad_data->irq < 0)
		return -EINVAL;

Does overwriting the real error code from platform_get_irq() with -EINVAL
break deferred probe if the interrupt controller is not yet ready?

[Severity: High]
This isn't a bug introduced by this patch, but I also noticed a potential 
issue here related to the IRQ registration.

Just before calling devm_request_irq(), keyscan_probe() calls 
keyscan_stop() which disables the clock.

If the IRQ line is asserted (e.g. from a pending bootloader state or
spurious event), keyscan_isr() will fire immediately once devm_request_irq()
is called.

The ISR then accesses peripheral registers:

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

Can accessing peripheral registers while the corresponding clock is disabled 
cause a synchronous bus fault or kernel panic on these systems? Should the 
devm_request_irq() call be moved after the clock is re-enabled?

> -	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/20260720085041.224896-1-phucduc.bui@gmail.com?part=2