Re: [PATCH v8 4/7] RISC-V: Treat IPIs as normal Linux IRQs

From: Anup Patel <anup@brainfault.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Conor.Dooley@microchip.com, apatel@ventanamicro.com,
	palmer@dabbelt.com, paul.walmsley@sifive.com, tglx@linutronix.de,
	daniel.lezcano@linaro.org, atishp@atishpatra.org,
	Alistair.Francis@wdc.com, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 4/7] RISC-V: Treat IPIs as normal Linux IRQs
Date: Mon, 29 Aug 2022 10:13:21 +0530	[thread overview]
Message-ID: <CAAhSdy13G2fjtVMQCMHf0vJff9i1q1Gg_WKQ942Qr9bmQ0NeBg@mail.gmail.com> (raw)
In-Reply-To: <69e58f4dc2b74415a32a97998e862479@kernel.org>

On Sat, Aug 27, 2022 at 12:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2022-08-26 19:48, Conor.Dooley@microchip.com wrote:
> > On 20/08/2022 07:54, Anup Patel wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the content is safe
> >>
> >> Currently, the RISC-V kernel provides arch specific hooks (i.e.
> >> struct riscv_ipi_ops) to register IPI handling methods. The stats
> >> gathering of IPIs is also arch specific in the RISC-V kernel.
> >>
> >> Other architectures (such as ARM, ARM64, and MIPS) have moved away
> >> from custom arch specific IPI handling methods. Currently, these
> >> architectures have Linux irqchip drivers providing a range of Linux
> >> IRQ numbers to be used as IPIs and IPI triggering is done using
> >> generic IPI APIs. This approach allows architectures to treat IPIs
> >> as normal Linux IRQs and IPI stats gathering is done by the generic
> >> Linux IRQ subsystem.
> >>
> >> We extend the RISC-V IPI handling as-per above approach so that arch
> >> specific IPI handling methods (struct riscv_ipi_ops) can be removed
> >> and the IPI handling is done through the Linux IRQ subsystem.
> >>
> >> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >
> >> +void riscv_ipi_set_virq_range(int virq, int nr)
> >> +{
> >> +       int i, err;
> >>
> >> -               if (ops & (1 << IPI_IRQ_WORK)) {
> >> -                       stats[IPI_IRQ_WORK]++;
> >> -                       irq_work_run();
> >> -               }
> >> +       if (WARN_ON(ipi_virq_base))
> >> +               return;
> >>
> >> -#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> >> -               if (ops & (1 << IPI_TIMER)) {
> >> -                       stats[IPI_TIMER]++;
> >> -                       tick_receive_broadcast();
> >> -               }
> >> -#endif
> >> -               BUG_ON((ops >> IPI_MAX) != 0);
> >> +       WARN_ON(nr < IPI_MAX);
> >> +       nr_ipi = min(nr, IPI_MAX);
> >> +       ipi_virq_base = virq;
> >> +
> >> +       /* Request IPIs */
> >> +       for (i = 0; i < nr_ipi; i++) {
> >> +               err = request_percpu_irq(ipi_virq_base + i,
> >> handle_IPI,
> >> +                                        "IPI", &ipi_virq_base);
> >
> > FWIW, ?sparse? does not like this:
> > arch/riscv/kernel/smp.c:163:50: warning: incorrect type in argument 4
> > (different address spaces)
> > arch/riscv/kernel/smp.c:163:50:    expected void [noderef] __percpu
> > *percpu_dev_id
> > arch/riscv/kernel/smp.c:163:50:    got int *
>
> Huh, well spotted. This will totally give the wrong sort of
> result, as this is used as a percpu variable from the irq
> core code.
>
> The arm64 code passes instead a pointer to the CPU number, which
> is not very useful, but at least not completely wrong.
>
> I'm sure the RISC-V code has some sort of semi-useful data to
> stuff in there instead of this.

Unlike arm64, we don't have any percpu data in arch/riscv/kernel/smp.c
which can be passed here.

For now, I will just add dummy percpu data to make sparse happy.

I hope this is okay ?

Regards,
Anup

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