Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask

[Palmer Dabbelt]

On Sat, 14 Sep 2019 12:42:32 PDT (-0700), charles.papon.90@xxxxxxxxx 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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv