linux-kernel - Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

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] [day] [month] [year] [list]

Message-ID: <878qjwejpr.ffs@tglx>
Date: Wed, 06 Aug 2025 14:08:16 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Nick Hu <nick.hu@...ive.com>, anup@...infault.org, Alexandre Ghiti
 <alex@...ti.fr>, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Cc: Nick Hu <nick.hu@...ive.com>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:

"Restore the IMSIC registers" does not tell me anything useful. When are
they restored?

> When the system woken up from the low power state, the IMSIC might be in

is woken up from a low power state

> the reset state.

The real important information is:

   When the system enters a low power state the IMSIC might be reset,
   but on exit nothing restores the registers, which prevents interrupt
   delivery.

Or something like that.

> Therefore adding the CPU PM callbacks to restore the IMSIC register
> when the cpu resume from the low power state.

This is not a valid sentence.

  Solve this by registering a CPU power management notifier, which
  restores the IMSIC on exit.

Or such.

See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

for further explanation.

> diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> index d9ae87808651..62bcbcae8bd4 100644
> --- a/drivers/irqchip/irq-riscv-imsic-early.c
> +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> @@ -7,6 +7,7 @@
>  #define pr_fmt(fmt) "riscv-imsic: " fmt
>  #include <linux/acpi.h>
>  #include <linux/cpu.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/interrupt.h>

This does neither apply against Linus tree nor against tip 

> -static int imsic_starting_cpu(unsigned int cpu)
> +static void imsic_restore(void)

This function is used for both setup _and_ restore, so naming it
restore() is misleading at best.

>  {
> -	/* Mark per-CPU IMSIC state as online */
> -	imsic_state_online();
> -
> -	/* Enable per-CPU parent interrupt */
> -	enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
> -
>  	/* Setup IPIs */
>  	imsic_ipi_starting_cpu();
>  
> @@ -128,6 +123,19 @@ static int imsic_starting_cpu(unsigned int cpu)
>  
>  	/* Enable local interrupt delivery */
>  	imsic_local_delivery(true);
> +}
> +
> +static int imsic_starting_cpu(unsigned int cpu)
> +{
> +	/* Mark per-CPU IMSIC state as online */
> +	imsic_state_online();
> +
> +	/* Enable per-CPU parent interrupt */
> +	enable_percpu_irq(imsic_parent_irq,
> +			  irq_get_trigger_type(imsic_parent_irq));

No line break required. You have 100 characters.

> +
> +	/* Restore the imsic reg */

One IMSIC register? Can you please write proper sentences and write
words out? This is not twitter. Also use IMSIC uppercase as the rest of
the code does in the comments.

Thanks,

        tglx