Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <529ec882-734f-17ae-e4cb-3aeb563ad1d5@bluespec.com> <mhng-c06cc89b-42d9-4f95-b090-2db96628d5fb@palmer-si-x1e>
In-Reply-To: <mhng-c06cc89b-42d9-4f95-b090-2db96628d5fb@palmer-si-x1e>
From:   Charles Papon <charles.papon.90@gmail.com>
Date:   Sat, 14 Sep 2019 21:42:32 +0200
Message-ID: <CAMabmMJ=QcH-529O6ORWbFwOrAnMKeWTvQ=WGYgnOoihqj9uFA@mail.gmail.com>
Subject: Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask
To:     Palmer Dabbelt <palmer@sifive.com>
Cc:     Darius Rad <darius@bluespec.com>, jason@lakedaemon.net,
        maz@kernel.org, linux-kernel <linux-kernel@vger.kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        linux-riscv <linux-riscv@lists.infradead.org>, tglx@linutronix.de
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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