| 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
| ||
|
Date: Sat, 16 May 2020 14:16:22 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anup Patel <Anup.Patel@....com>
Cc: Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Atish Patra <Atish.Patra@....com>,
Alistair Francis <Alistair.Francis@....com>,
Anup Patel <anup@...infault.org>,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple
PLIC instances
On 2020-05-16 14:01, Anup Patel wrote:
>> -----Original Message-----
>> From: Marc Zyngier <maz@...nel.org>
>> Sent: 16 May 2020 17:59
>> To: Anup Patel <Anup.Patel@....com>
>> Cc: Palmer Dabbelt <palmer@...belt.com>; Paul Walmsley
>> <paul.walmsley@...ive.com>; Thomas Gleixner <tglx@...utronix.de>;
>> Jason
>> Cooper <jason@...edaemon.net>; Atish Patra <Atish.Patra@....com>;
>> Alistair
>> Francis <Alistair.Francis@....com>; Anup Patel <anup@...infault.org>;
>> linux-
>> riscv@...ts.infradead.org; linux-kernel@...r.kernel.org
>> 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@....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.
>
> 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...
Powered by blists - more mailing lists