linux-kernel - Re: [PATCH] irqchip/mvebu-gicp: clear pending irqs on init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ldo7jiy9.ffs@tglx>
Date: Tue, 29 Jul 2025 14:10:54 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: enachman@...vell.com, andrew@...n.ch, gregory.clement@...tlin.com,
 sebastian.hesselbarth@...il.com, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Cc: enachman@...vell.com
Subject: Re: [PATCH] irqchip/mvebu-gicp: clear pending irqs on init

On Tue, Jul 29 2025 at 11:48, enachman@...vell.com wrote:
> From: Elad Nachman <enachman@...vell.com>
>
> For kexec case, left interrupt might generate spurious
> interrupts in various A7/A8/CN913x interrupt system
> from the I/O SB to the NB.

That's a lot of incomprehensible acronyms. Is any of that relevant for
understanding the problem?

What's a 'left' interrupt?

Just say something like this:

  When a kexec'ed kernel boots up, there might be stale unhandled
  interrupts pending in the interrupt controller. These are delivered as
  spurious interrupts once the boot CPU enables interrupts.

That's clear and sufficient, no?

> Clear all pending interrupts
> when the driver is initialized to prevent these spurious
> interrupts.

> Signed-off-by: Elad Nachman <enachman@...vell.com>
> ---
>  drivers/irqchip/irq-mvebu-gicp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/irqchip/irq-mvebu-gicp.c b/drivers/irqchip/irq-mvebu-gicp.c
> index d3232d6d8dce..0fa21a45d4e1 100644
> --- a/drivers/irqchip/irq-mvebu-gicp.c
> +++ b/drivers/irqchip/irq-mvebu-gicp.c
> @@ -31,6 +31,7 @@ struct mvebu_gicp_spi_range {
>  
>  struct mvebu_gicp {
>  	struct mvebu_gicp_spi_range *spi_ranges;
> +	void __iomem *base;

Why?

>  	unsigned int spi_ranges_cnt;
>  	unsigned int spi_cnt;
>  	unsigned long *spi_bitmap;
> @@ -236,6 +237,14 @@ static int mvebu_gicp_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	gicp->base = devm_platform_ioremap_resource(pdev, 0);

There is no reason to create a permanent mapping, which is not used
anywhere else. Just map, write, unmap.

> +	if (IS_ERR(gicp->base))
> +		dev_err(&pdev->dev, "gicp - Cannot ioremap !\n");

So this emits a unspecific error message and then happily
continues. That does not make sense.

As the ioremap failure is not fatal, tell the user what this is about,
i.e. "ioremap() failed. Unable to clear pending interrupts."
or something to that effect.

> +	else {

Lacks parenthesis on the if clause for symmetry.

> +		for (i = 0; i < 64; i++)
> +			writel(i, gicp->base + GICP_CLRSPI_NSR_OFFSET);
> +	}
> +
>  	return msi_create_parent_irq_domain(&info, &gicp_msi_parent_ops) ? 0 : -ENOMEM;
>  }

Thanks,

        tglx