Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple PLIC instances

[Marc Zyngier <maz@kernel.org>]

On 2020-05-16 07:39, Anup Patel wrote:
> To distinguish interrupts from multiple PLIC instances, we use a
> per-PLIC irq_chip instance with a different name.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c
> b/drivers/irqchip/irq-sifive-plic.c
> index 2d3db927a551..e42fc082ad18 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -60,6 +60,7 @@
>  #define	PLIC_ENABLE_THRESHOLD		0
> 
>  struct plic_priv {
> +	struct irq_chip chip;
>  	struct cpumask lmask;
>  	struct irq_domain *irqdomain;
>  	void __iomem *regs;
> @@ -76,6 +77,7 @@ struct plic_handler {
>  	void __iomem		*enable_base;
>  	struct plic_priv	*priv;
>  };
> +static unsigned int plic_count;
>  static bool plic_cpuhp_setup_done;
>  static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> 
> @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d)
>  	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
>  }
> 
> -static struct irq_chip plic_chip = {
> -	.name		= "SiFive PLIC",
> -	.irq_mask	= plic_irq_mask,
> -	.irq_unmask	= plic_irq_unmask,
> -	.irq_eoi	= plic_irq_eoi,
> -#ifdef CONFIG_SMP
> -	.irq_set_affinity = plic_set_affinity,
> -#endif
> -};
> -
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  			      irq_hw_number_t hwirq)
>  {
> -	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> +	struct plic_priv *priv = d->host_data;
> +
> +	irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data,
>  			    handle_fasteoi_irq, NULL, NULL);
>  	irq_set_noprobe(irq);
>  	return 0;
> @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node 
> *node,
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++);
> +	priv->chip.irq_mask = plic_irq_mask,
> +	priv->chip.irq_unmask = plic_irq_unmask,
> +	priv->chip.irq_eoi = plic_irq_eoi,
> +#ifdef CONFIG_SMP
> +	priv->chip.irq_set_affinity = plic_set_affinity,
> +#endif
> +
>  	priv->regs = of_iomap(node, 0);
>  	if (WARN_ON(!priv->regs)) {
>  		error = -EIO;
> @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node 
> *node,
>  	}
> 
>  	pr_info("interrupt-controller at 0x%llx "
> -		"(interrupts=%d, contexts=%d, handlers=%d)\n",
> +		"(interrupts=%d, contexts=%d, handlers=%d) (%s)\n",
>  		(unsigned long long)iores.start, nr_irqs,
> -		nr_contexts, nr_handlers);
> +		nr_contexts, nr_handlers, priv->chip.name);
>  	set_handle_irq(plic_handle_irq);
>  	return 0;

I really dislike this patch for multiple reasons:

- Allocating a new struc irq_chip just for a string seems over the top,
   specially as all the *useful* stuff stays the same.

- Even if I hate it, /proc is API. I'm sure something, somewhere is
   parsing this. Changing the string is likely to confuse it.

- If you do this for debug purposes, then CONFIG_GENERIC_IRQ_DEBUGFS
   is the right way to look up the information.

- If, for reasons that are beyond me, you actually *need* this, then
   implementing irq_print_chip in your irq_chip structure is the way
   to go.

But frankly, I'd rather you drop this altogether.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...