On Sun, 15 Sep 2019 18:31:33 +0100,
Palmer Dabbelt <pal...@sifive.com> wrote:
Hi Palmer,
>
> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), m...@kernel.org wrote:
> > On Thu, 12 Sep 2019 22:40:34 +0100,
> > Darius Rad <dar...@bluespec.com> wrote:
> >
> > Hi Darius,
> >
> >>
> >> As per the existing comment, irq_mask and irq_unmask do not need
> >> to do anything for the PLIC. However, the functions must exist
> >> (the pointers cannot be NULL) as they are not optional, based on
> >> the documentation (Documentation/core-api/genericirq.rst) as well
> >> as existing usage (e.g., include/linux/irqchip/chained_irq.h).
> >>
> >> Signed-off-by: Darius Rad <dar...@bluespec.com>
> >> ---
> >> drivers/irqchip/irq-sifive-plic.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-sifive-plic.c
> >> b/drivers/irqchip/irq-sifive-plic.c
> >> index cf755964f2f8..52d5169f924f 100644
> >> --- a/drivers/irqchip/irq-sifive-plic.c
> >> +++ b/drivers/irqchip/irq-sifive-plic.c
> >> @@ -111,6 +111,13 @@ static void plic_irq_disable(struct irq_data *d)
> >> plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> >> }
> >> +/*
> >> + * There is no need to mask/unmask PLIC interrupts. They are "masked"
> >> + * by reading claim and "unmasked" when writing it back.
> >> + */
> >> +static void plic_irq_mask(struct irq_data *d) { }
> >> +static void plic_irq_unmask(struct irq_data *d) { }
> >
> > This outlines a bigger issue. If your irqchip doesn't require
> > mask/unmask, you're probably not using the right interrupt
> > flow. Looking at the code, I see you're using handle_simple_irq, which
> > is almost universally wrong.
> >
> > As per the description above, these interrupts should be using the
> > fasteoi flow, which is designed for this exact behaviour (the
> > interrupt controller knows which interrupt is in flight and doesn't
> > require SW to do anything bar signalling the EOI).
> >
> > Another thing is that mask/unmask tends to be a requirement, while
> > enable/disable tends to be optional. There is no hard line here, but
> > the expectations are that:
> >
> > (a) A disabled line can drop interrupts
> > (b) A masked line cannot drop interrupts
> >
> > Depending what the PLIC architecture mandates, you'll need to
> > implement one and/or the other. Having just (a) is indicative of a HW
> > bug, and I'm not assuming that this is the case. (b) only is pretty
> > common, and (a)+(b) has a few adepts. My bet is that it requires (b)
> > only.
> >
> >> +
> >> #ifdef CONFIG_SMP
> >> static int plic_set_affinity(struct irq_data *d,
> >> const struct cpumask *mask_val, bool force)
> >> @@ -138,12 +145,10 @@ static int plic_set_affinity(struct irq_data *d,
> >> static struct irq_chip plic_chip = {
> >> .name = "SiFive PLIC",
> >> - /*
> >> - * There is no need to mask/unmask PLIC interrupts. They are "masked"
> >> - * by reading claim and "unmasked" when writing it back.
> >> - */
> >> .irq_enable = plic_irq_enable,
> >> .irq_disable = plic_irq_disable,
> >> + .irq_mask = plic_irq_mask,
> >> + .irq_unmask = plic_irq_unmask,
> >> #ifdef CONFIG_SMP
> >> .irq_set_affinity = plic_set_affinity,
> >> #endif
> >
> > Can you give the following patch a go? It brings the irq flow in line
> > with what the HW can do. It is of course fully untested (not even
> > compile tested...).
> >
> > Thanks,
> >
> > M.
> >
> > From c0ce33a992ec18f5d3bac7f70de62b1ba2b42090 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <m...@kernel.org>
> > Date: Sun, 15 Sep 2019 15:17:45 +0100
> > Subject: [PATCH] irqchip/sifive-plic: Switch to fasteoi flow
> >
> > The SiFive PLIC interrupt controller seems to have all the HW
> > features to support the fasteoi flow, but the driver seems to be
> > stuck in a distant past. Bring it into the 21st century.
>
> Thanks. We'd gotten these comments during the review process but
> nobody had gotten the time to actually fix the issues.
No worries. The IRQ subsystem is an acquired taste... ;-)
> >
> > Signed-off-by: Marc Zyngier <m...@kernel.org>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 29 +++++++++++++++--------------
> > 1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c
> > b/drivers/irqchip/irq-sifive-plic.c
> > index cf755964f2f8..8fea384d392b 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -97,7 +97,7 @@ static inline void plic_irq_toggle(const struct cpumask
> > *mask,
> > }
> > }
> > -static void plic_irq_enable(struct irq_data *d)
> > +static void plic_irq_mask(struct irq_data *d)
Of course, this is wrong. The perks of trying to do something at the
last minute while boarding an airplane. Don't do that.
This should of course read "plic_irq_unmask"...
> > {
> > unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > cpu_online_mask);
> > @@ -106,7 +106,7 @@ static void plic_irq_enable(struct irq_data *d)
> > plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
> > }
> > -static void plic_irq_disable(struct irq_data *d)
> > +static void plic_irq_unmask(struct irq_data *d)
... and this should be "plic_irq_mask".
[...]
> Reviewed-by: Palmer Dabbelt <pal...@sifive.com>
> Tested-by: Palmer Dabbelt <pal...@sifive.com> (QEMU Boot)
Huhuh... It may be that QEMU doesn't implement the full-fat PLIC, as
the above bug should have kept the IRQ lines masked.
> We should test them on the hardware, but I don't have any with me
> right now. David's probably in the best spot to do this, as he's got
> a setup that does all the weird interrupt sources (ie, PCIe).
>
> David: do you mind testing this? I've put the patch here:
>
> ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git
> -b plic-fasteoi
I've pushed out a branch with the fixed patch:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
irq/plic-fasteoi
Thanks,
M.
--
Jazz is not dead, it just smells funny.
