On Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
Palmer Dabbelt <pal...@sifive.com> wrote:
> On Mon, 16 Sep 2019 12:04:56 PDT (-0700), Darius Rad wrote:
> > On 9/15/19 2:20 PM, Marc Zyngier wrote:
> >> 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
> >>
> >
> > That patch works for me on real-ish hardware. I tried on two FPGA
> > systems that have different PLIC implementations. Both include
> > a PCIe root port (and associated interrupt source). So for
> > whatever it's worth:
> >
> > Tested-by: Darius Rad <dar...@bluespec.com>
>
> Awesome, thanks. Would it be OK to put a "(on two hardware PLIC
> implementations)" after that, just so we're clear as to who tested
> what?
Sure, no problem.
> Assuming one of yours wasn't a SiFive PLIC then it'd be great if
> David could still give this a whack, but I don't think it strictly
> needs to block merging the patch. I've included the right David this
> time, with any luck that will be more fruitful :)
Well, we still have time before -rc1. Once David gets a chance to test
it, I'll apply it. Additional question: do you want this backported to
-stable? If so, how far?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
