Re: [PATCH v6 5/9] irqchip/imx-irqsteer: Mask all interrupts in probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] irqchip/imx-irqsteer: missing wmb() allows interrupt storm race
--

commit e530007174d6dce33c6ec542e006f51e4a17dc73
Author: Zhipeng Wang <zhipeng.wang_1@nxp.com>

irqchip/imx-irqsteer: Mask all interrupts in probe()

This commit masks all input interrupts in probe() before setting up
chained handlers. This prevents an interrupt storm if a child interrupt
was left unmasked during driver unload, as the hardware would otherwise
assert it immediately upon re-mapping in the new domain.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 5acc04504e520..fa233de9bc2b2 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c

[ ... ]

> @@ -239,6 +239,14 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	if (irqsteer_has_chanctrl(data->devtype_data))
>  		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>  
> +	/*
> +	 * Mask all interrupts before wiring up the chained handlers. CHANMASK
> +	 * has inverted polarity (a set bit enables the interrupt), so writing
> +	 * zero masks the source.
> +	 */
> +	for (i = 0; i < data->reg_num; i++)
> +		writel_relaxed(0, data->regs + CHANMASK(i, data->reg_num));
> +

[Severity: Medium]
Is an explicit memory barrier needed here to ensure the masks are applied
before the parent interrupt is unmasked?

Since writel_relaxed() does not provide a barrier, these writes to the IRQ
Steer hardware could be reordered. The parent GIC interrupt may be unmasked
before the IRQ Steer actually processes the interrupt masks, leaving a window
for the interrupt storm this patch is designed to prevent.

The parent GIC driver uses writel_relaxed() to unmask (for example,
gic_unmask_irq() in GICv2 or gic_poke_irq() in GICv3). Furthermore,
intervening locks like desc->lock in the IRQ core or irq_domain_mutex
provide only ACQUIRE and RELEASE semantics. On ARM64, an intervening RELEASE
followed by an ACQUIRE to different lock addresses can be reordered by the
CPU.

Could this allow the subsequent GIC relaxed write to cross the prior IRQ
Steer relaxed write, meaning a full wmb() is strictly required to order
writes to different I/O peripherals?

>  	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
>  	if (ret)
>  		goto out;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008090231.796511-1-Zhipeng.wang_1@oss.nxp.com?part=5