Re: [PATCH v3 1/4] irqchip/imx-irqsteer: Fix error handling path in probe()

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

Pre-existing issues:
- [High] The chained IRQ handler accesses hardware registers without checking if the device is active, risking a system crash on spurious interrupts.
- [High] The driver fails to explicitly mask hardware interrupts during initialization, risking an unhandled interrupt storm during probe.
--

--- Patch [1]: [PATCH v3 1/4] irqchip/imx-irqsteer: Fix error handling path in probe() ---
commit bda073233892b00ff41d8b06dd3aac163410e9a2
Author: Zhipeng Wang <[email protected]>

irqchip/imx-irqsteer: Fix error handling path in probe()

This commit splits the error handling path in imx_irqsteer_probe() so that
failures occurring after the IRQ domain creation properly remove the domain
before disabling the clock. This prevents an IRQ domain leak during probe
failures.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be35..a2f0629b22a37 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -241,13 +241,13 @@ static int imx_irqsteer_probe(struct platform_device *pdev)

[ ... ]

>  	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
>  		ret = -EINVAL;
> -		goto out;
> +		goto err_domain;
>  	}
>  
>  	for (i = 0; i < data->irq_count; i++) {

[Severity: High]
This is a pre-existing issue, but does this initialization fail to explicitly
mask hardware interrupts before registering the chained handler?

When irq_set_chained_handler_and_data() is called in this loop to register
imx_irqsteer_irq_handler(), the parent IRQ is unmasked at the GIC. If the
bootloader leaves any input interrupt lines unmasked and asserted in the
CHANMASK and CHANSTATUS registers, the GIC will fire immediately.

Since no child handler is registered yet, the interrupt is never cleared,
which causes chained_irq_exit() to re-unmask the parent IRQ. Does this risk
an infinite unhandled interrupt storm during boot?

Also, regarding imx_irqsteer_irq_handler() which is registered in this
loop:

imx_irqsteer_irq_handler() {
    ...
    irqmap = readl_relaxed(data->regs + CHANSTATUS(idx, data->reg_num));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the chained handler
access hardware registers without checking if the device is active?

If no child interrupts are requested, the IRQ core drops its PM reference
and the device enters runtime suspend, disabling the IPG clock. If a
spurious interrupt arrives on the parent IRQ line from the GIC, the handler
is invoked and unconditionally reads the CHANSTATUS register with the clock
off.

Could this trigger a synchronous external abort (kernel panic)? Should the
handler verify the device's PM runtime state using pm_runtime_get_if_active()
before accessing registers?

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