Message-ID: <be21db32-ff5c-b25a-c8d6-af5bbd0c5469@bluespec.com>
Date:   Mon, 16 Sep 2019 17:41:07 -0400
From:   Darius Rad <darius@...espec.com>
To:     Palmer Dabbelt <palmer@...ive.com>,
        David Abdurachmanov <david.abdurachmanov@...ive.com>
Cc:     maz@...nel.org, Paul Walmsley <paul.walmsley@...ive.com>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, jason@...edaemon.net
Subject: Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask

On 9/16/19 4:51 PM, Palmer Dabbelt 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 <palmer@...ive.com> wrote:
>>>
>>> Hi Palmer,
>>>
>>>>
>>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@...nel.org wrote:
>>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>>> Darius Rad <darius@...espec.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 <darius@...espec.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 <maz@...nel.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 <maz@...nel.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 <palmer@...ive.com>
>>>> Tested-by: Palmer Dabbelt <palmer@...ive.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 <darius@...espec.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?

Fine by me.

> 
> 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 :)

One of the systems I tested was based on rocket-chip, and the
associated PLIC, which I guess is the SiFive PLIC, right?  Can't hurt
to have more testing, though.

> 
>>
>>> Thanks,
>>>
>>>     M.
>>>

Powered by blists - more mailing lists