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

[Marc Zyngier]

On 2020-05-16 14:01, Anup Patel wrote:
> > -----Original Message-----
> > From: Marc Zyngier <maz@xxxxxxxxxx>
> > Sent: 16 May 2020 17:59
> > To: Anup Patel <Anup.Patel@xxxxxxx>
> > Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>; Paul Walmsley
> > <paul.walmsley@xxxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>; 
> > Jason
> > Cooper <jason@xxxxxxxxxxxxxx>; Atish Patra <Atish.Patra@xxxxxxx>; 
> > Alistair
> > Francis <Alistair.Francis@xxxxxxx>; Anup Patel <anup@xxxxxxxxxxxxxx>; 
> > linux-
> > riscv@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for 
> > muiltiple PLIC
> > instances
> >
> > 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@xxxxxxx>
> > > ---
> > >  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.
>
> AFAIK, we don't have scripts in RISC-V world that depend on
> /proc/interrupts content. May be in future such scripts will show up.

How do you know that? Do you keep an exhaustive repository of all
the possible parsers of /proc/cpuinfo (rhetorical question)?

> For system with multiple PLICs, we are seeing same "SiFive PLIC"
> string for all PLIC interrupts in "cat /proc/interrupts". I am trying 
> to
> assign different string based on PLIC instance. This is similar to
> what GICv2 driver is doing (e.g. GIC-0, GIC-1, ... in 
> /proc/interrupts).

Which was a *very* bad idea the first place, and I wish I could get
rid of it. I cannot, for the reason outlined above (it's ABI).
Furthermore, in this case, the GICs are different (they are cascaded).
In your case, they have the same position in the interrupt hierarchy.

> Is there a better way to do this ?
>
> > - 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.
>
> I just want to differentiate which interrupt belongs to which PLIC
> Instance in /proc/interrupts. I can take a different approach if you
> suggest.

I *have* given you a way to implement that in a better way. But again,
I'd rather you *don't* do it for the reason I have outlined above.

Thanks,

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