RE: [PATCH v3 3/4] irqchip/imx-irqsteer: Mask all interrupts in probe() and remove()

[Zhipeng Wang (OSS)]

NXP Confidential
> -----Original Message-----
> From: Frank Li (OSS) <frank.li@xxxxxxxxxxx>
> Sent: 2026年8月8日 3:14
> To: Zhipeng Wang (OSS) <zhipeng.wang_1@xxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxx>; Marc Zyngier <maz@xxxxxxxxxx>;
> Frank Li <frank.li@xxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>;
> Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam
> <festevam@xxxxxxxxx>; Jindong Yue <jindong.yue@xxxxxxx>; Xuegang Liu
> <xuegang.liu@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/4] irqchip/imx-irqsteer: Mask all interrupts in probe()
> and remove()
>
> On Fri, Aug 07, 2026 at 04:23:45PM +0900, Zhipeng.wang_1@xxxxxxxxxxx
> wrote:
> > From: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> >
> > probe() sets up the chained handlers without first masking the input
> > interrupts, and remove() leaves the CHANMASK registers untouched. For
> > a built-in driver this happened to be harmless because CHANMASK resets
> > to all-masked, but once the driver can be unloaded and reloaded a
> > child interrupt left unmasked at unload time survives in hardware. On
> > the next
> > probe() the parent interrupts are re-mapped and unmasked before the
> > new domain is ready, so a still-asserted line immediately storms the
> > parent with no handler to service it.
> >
> > Mask all interrupts in probe() before wiring up the chained handlers,
> > and again in remove() so the hardware is left quiesced for the next probe().
> > Note CHANMASK uses inverted polarity (a set bit enables the
> > interrupt), so masking means writing zero. This mirrors the sibling
> > NXP chained mux irq-imx-intmux.c, which has masked all sources in both
> > probe() and remove() since commit 2fbb13961e74 ("irqchip: Add NXP
> > INTMUX interrupt multiplexer support").
> >
> > Signed-off-by: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> > ---
> > Changes in v3:
> >  - New patch. Masks all CHANMASK interrupts in probe() and remove() to
> >    prevent the interrupt storm on module reload reported by Sashiko AI
> >    on v2.
> >
> >  drivers/irqchip/irq-imx-irqsteer.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 4a2fe8ba97f5..0c9c99f1141a 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -236,6 +236,10 @@ 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 setting up the chained handlers */
> > +   for (i = 0; i < data->reg_num; i++)
> > +           writel_relaxed(0, data->regs + CHANMASK(i, data->reg_num));
> > +
> >     data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev),
> data->reg_num * 32,
> >                                             &imx_irqsteer_domain_ops, data);
> >     if (!data->domain) {
> > @@ -279,6 +283,11 @@ static void imx_irqsteer_remove(struct
> platform_device *pdev)
> >     struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> >     int hwirq, i;
> >
> > +   /* mask all interrupts so a stale line cannot storm on the next probe */
> > +   for (i = 0; i < irqsteer_data->reg_num; i++)
> > +           writel_relaxed(0, irqsteer_data->regs +
> > +                          CHANMASK(i, irqsteer_data->reg_num));
> > +
>
> You access register here, do you need call runtime pm get to enable clock first?
>
> Frank
>

Good catch. Rather than adding a runtime PM get/put around the register
access, I dropped the masking in remove() entirely (v4 patch 4/5).

The remove()-time masking was unnecessary: the next probe() masks all
sources before it re-maps and unmasks the parent interrupts, which is
the only window in which a stale line could storm. Masking in remove()
would also mean touching CHANMASK while the device may already be
runtime-suspended with the clock gated - exactly the problem you point
out. So v4 masks only in probe().

BRs,
Zhipeng

> >     for (i = 0; i < irqsteer_data->irq_count; i++) {
> >             if (!irqsteer_data->irq[i])
> >                     break;
> > --
> > 2.34.1
> >
> >