lkml.org 
[lkml]   [2019]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask
From
On Sat, 14 Sep 2019 12:42:32 PDT (-0700), charles.papon.90@gmail.com wrote:
> I had issues with that plic driver. The current implementation wasn't
> usable with driver using level sensitive interrupt together with the
> IRQF_ONESHOT flag.
>
> Those null were producing crashes in the chained_irq_enter function.
> Filling them with dummy function fixed the issue.

I'm not arguing it fixes a crash, the code Darius pointed to obviously doesn't
check for NULL before calling these functions and will therefor crash. There
is a bunch of other code that does check, though, so I guess my question is
really: is the bug in the PLIC driver, or in this header?

If we're not allowed to have these as NULL and there's nothing to do, then this
is a reasonable patch. I'm just not capable of answering that question, as I'm
not an irqchip maintainer :)

> On Sat, Sep 14, 2019 at 9:00 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Thu, 12 Sep 2019 14:40:34 PDT (-0700), Darius Rad wrote:
>> > 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@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) { }
>> > +
>> > #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
>>
>> I can't find any other drivers in irqchip with empty irq_mask/irq_unmask. I'm
>> not well versed in irqchip stuff, so I'll leave it up to the irqchip
>> maintainers to comment on if this is the right way to do this. Either way, I'm
>> assuming it'll go in through some the irqchip tree so
>>
>> Acked-by: Palmer Dabbelt <palmer@sifive.com>
>>
>> just to make sure I don't get in the way if it is the right way to do it :).
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

\
 
 \ /
  Last update: 2019-09-14 21:51    [W:0.134 / U:3.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site