[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mhng-df6c7aad-d4fd-4c44-96c8-bf63465e0c97@palmer-si-x1c4>
Date: Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
From: Palmer Dabbelt <palmer@...ive.com>
To: Darius Rad <darius@...espec.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 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?
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 :)
>
>> Thanks,
>>
>> M.
>>
Powered by blists - more mailing lists