Re: [PATCH v1] irqchip/irq-sifive-plic: Add syscore callbacks for hibernation

[Marc Zyngier]

On Fri, 13 Jan 2023 09:42:16 +0000,
Mason Huo <mason.huo@xxxxxxxxxxxxxxxx> wrote:
> 
> The priority and enable registers of plic will be reset
> during hibernation power cycle in poweroff mode,
> add the syscore callbacks to save/restore those registers.
> 
> Signed-off-by: Mason Huo <mason.huo@xxxxxxxxxxxxxxxx>
> Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
> Reviewed-by: Sia Jee Heng <jeeheng.sia@xxxxxxxxxxxxxxxx>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 93 ++++++++++++++++++++++++++++++-
>  1 file changed, 91 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index ff47bd0dec45..80306de45d2b 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
>  #include <asm/smp.h>
>  
>  /*
> @@ -67,6 +68,8 @@ struct plic_priv {
>  	struct irq_domain *irqdomain;
>  	void __iomem *regs;
>  	unsigned long plic_quirks;
> +	unsigned int nr_irqs;
> +	u32 *priority_reg;
>  };
>  
>  struct plic_handler {
> @@ -79,10 +82,13 @@ struct plic_handler {
>  	raw_spinlock_t		enable_lock;
>  	void __iomem		*enable_base;
>  	struct plic_priv	*priv;
> +	/* To record interrupts that are enabled before suspend. */
> +	u32 enable_reg[MAX_DEVICES / 32];

What does MAX_DEVICES represent here? How is it related to the number
of interrupts you're trying to save? It seems to be related to the
number of CPUs, so it hardly makes any sense so far.

>  };
>  static int plic_parent_irq __ro_after_init;
>  static bool plic_cpuhp_setup_done __ro_after_init;
>  static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> +static struct plic_priv *priv_data;
>  
>  static int plic_irq_set_type(struct irq_data *d, unsigned int type);
>  
> @@ -229,6 +235,78 @@ static int plic_irq_set_type(struct irq_data *d, unsigned int type)
>  	return IRQ_SET_MASK_OK;
>  }
>  
> +static void plic_irq_resume(void)
> +{
> +	unsigned int i, cpu;
> +	u32 __iomem *reg;
> +
> +	for (i = 0; i < priv_data->nr_irqs; i++)
> +		writel(priv_data->priority_reg[i],
> +				priv_data->regs + PRIORITY_BASE + i * PRIORITY_PER_ID);

From what I can tell, this driver uses exactly 2 priorities: 0 and 1.
And yet you use a full 32bit to encode those. Does it seem like a good
idea?

> +
> +	for_each_cpu(cpu, cpu_present_mask) {
> +		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> +
> +		if (!handler->present)
> +			continue;
> +
> +		for (i = 0; i < DIV_ROUND_UP(priv_data->nr_irqs, 32); i++) {
> +			reg = handler->enable_base + i * sizeof(u32);
> +			raw_spin_lock(&handler->enable_lock);
> +			writel(handler->enable_reg[i], reg);
> +			raw_spin_unlock(&handler->enable_lock);

Why do you need to take/release the lock around *each* register
access? Isn't that lock constant for a given CPU?

> +		}
> +	}
> +}
> +
> +static int plic_irq_suspend(void)
> +{
> +	unsigned int i, cpu;
> +	u32 __iomem *reg;
> +
> +	for (i = 0; i < priv_data->nr_irqs; i++)
> +		priv_data->priority_reg[i] =
> +			readl(priv_data->regs + PRIORITY_BASE + i * PRIORITY_PER_ID);
> +
> +	for_each_cpu(cpu, cpu_present_mask) {
> +		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> +
> +		if (!handler->present)
> +			continue;
> +
> +		for (i = 0; i < DIV_ROUND_UP(priv_data->nr_irqs, 32); i++) {
> +			reg = handler->enable_base + i * sizeof(u32);
> +			raw_spin_lock(&handler->enable_lock);
> +			handler->enable_reg[i] = readl(reg);
> +			raw_spin_unlock(&handler->enable_lock);

Same remarks.

	M.

-- 
Without deviation from the norm, progress is not possible.