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

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

[Darius Rad]

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
-- 
2.20.1

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

[Darius Rad]

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
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

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

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

[Palmer Dabbelt]

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

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

[Charles Papon]

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

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

[Charles Papon]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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@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

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@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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Marc Zyngier]

On Thu, 12 Sep 2019 22:40:34 +0100,
Darius Rad <darius@bluespec.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@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) { }

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@kernel.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.

Signed-off-by: Marc Zyngier <maz@kernel.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)
 {
 	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)
 {
 	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
 }
@@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	if (!irqd_irq_disabled(d)) {
-		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
-		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
-	}
+	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
+	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
@@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
 }
 #endif
 
+static void plic_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+
+	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+}
+
 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,
+	.irq_eoi	= plic_irq_eoi,
 #ifdef CONFIG_SMP
 	.irq_set_affinity = plic_set_affinity,
 #endif
@@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
+	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
 	irq_set_chip_data(irq, NULL);
 	irq_set_noprobe(irq);
 	return 0;
@@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
 					hwirq);
 		else
 			generic_handle_irq(irq);
-		writel(hwirq, claim);
 	}
 	csr_set(sie, SIE_SEIE);
 }
-- 
2.20.1


-- 
Jazz is not dead, it just smells funny.

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

[Marc Zyngier]

On Thu, 12 Sep 2019 22:40:34 +0100,
Darius Rad <darius@bluespec.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@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) { }

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@kernel.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.

Signed-off-by: Marc Zyngier <maz@kernel.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)
 {
 	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)
 {
 	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
 }
@@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	if (!irqd_irq_disabled(d)) {
-		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
-		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
-	}
+	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
+	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
@@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
 }
 #endif
 
+static void plic_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+
+	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+}
+
 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,
+	.irq_eoi	= plic_irq_eoi,
 #ifdef CONFIG_SMP
 	.irq_set_affinity = plic_set_affinity,
 #endif
@@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
+	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
 	irq_set_chip_data(irq, NULL);
 	irq_set_noprobe(irq);
 	return 0;
@@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
 					hwirq);
 		else
 			generic_handle_irq(irq);
-		writel(hwirq, claim);
 	}
 	csr_set(sie, SIE_SEIE);
 }
-- 
2.20.1


-- 
Jazz is not dead, it just smells funny.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
> On Thu, 12 Sep 2019 22:40:34 +0100,
> Darius Rad <darius@bluespec.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@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) { }
>
> 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@kernel.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.

>
> Signed-off-by: Marc Zyngier <maz@kernel.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)
>  {
>  	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)
>  {
>  	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
>  }
> @@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
>  	if (cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
> -	if (!irqd_irq_disabled(d)) {
> -		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> -		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
> -	}
> +	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> +	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
>  
>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  
> @@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
>  }
>  #endif
>  
> +static void plic_irq_eoi(struct irq_data *d)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +
> +	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +}
> +
>  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,
> +	.irq_eoi	= plic_irq_eoi,
>  #ifdef CONFIG_SMP
>  	.irq_set_affinity = plic_set_affinity,
>  #endif
> @@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  			      irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
> +	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
>  	irq_set_chip_data(irq, NULL);
>  	irq_set_noprobe(irq);
>  	return 0;
> @@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
>  					hwirq);
>  		else
>  			generic_handle_irq(irq);
> -		writel(hwirq, claim);
>  	}
>  	csr_set(sie, SIE_SEIE);
>  }
> -- 
> 2.20.1
>
>
> -- 
> Jazz is not dead, it just smells funny.

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
Tested-by: Palmer Dabbelt <palmer@sifive.com> (QEMU Boot)

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

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

[Palmer Dabbelt]

On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
> On Thu, 12 Sep 2019 22:40:34 +0100,
> Darius Rad <darius@bluespec.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@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) { }
>
> 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@kernel.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.

>
> Signed-off-by: Marc Zyngier <maz@kernel.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)
>  {
>  	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)
>  {
>  	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
>  }
> @@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
>  	if (cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
> -	if (!irqd_irq_disabled(d)) {
> -		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> -		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
> -	}
> +	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
> +	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
>  
>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  
> @@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
>  }
>  #endif
>  
> +static void plic_irq_eoi(struct irq_data *d)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +
> +	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +}
> +
>  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,
> +	.irq_eoi	= plic_irq_eoi,
>  #ifdef CONFIG_SMP
>  	.irq_set_affinity = plic_set_affinity,
>  #endif
> @@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  			      irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
> +	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
>  	irq_set_chip_data(irq, NULL);
>  	irq_set_noprobe(irq);
>  	return 0;
> @@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
>  					hwirq);
>  		else
>  			generic_handle_irq(irq);
> -		writel(hwirq, claim);
>  	}
>  	csr_set(sie, SIE_SEIE);
>  }
> -- 
> 2.20.1
>
>
> -- 
> Jazz is not dead, it just smells funny.

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
Tested-by: Palmer Dabbelt <palmer@sifive.com> (QEMU Boot)

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Marc Zyngier]

On Sun, 15 Sep 2019 18:31:33 +0100,
Palmer Dabbelt <palmer@sifive.com> wrote:

Hi Palmer,

> 
> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
> > On Thu, 12 Sep 2019 22:40:34 +0100,
> > Darius Rad <darius@bluespec.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@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) { }
> > 
> > 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@kernel.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@kernel.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@sifive.com>
> Tested-by: Palmer Dabbelt <palmer@sifive.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

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

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

[Marc Zyngier]

On Sun, 15 Sep 2019 18:31:33 +0100,
Palmer Dabbelt <palmer@sifive.com> wrote:

Hi Palmer,

> 
> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
> > On Thu, 12 Sep 2019 22:40:34 +0100,
> > Darius Rad <darius@bluespec.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@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) { }
> > 
> > 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@kernel.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@kernel.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@sifive.com>
> Tested-by: Palmer Dabbelt <palmer@sifive.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

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Sun, 15 Sep 2019 11:20:40 PDT (-0700), maz@kernel.org wrote:
> On Sun, 15 Sep 2019 18:31:33 +0100,
> Palmer Dabbelt <palmer@sifive.com> wrote:
>
> Hi Palmer,
>
>>
>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>> > On Thu, 12 Sep 2019 22:40:34 +0100,
>> > Darius Rad <darius@bluespec.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@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) { }
>> >
>> > 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@kernel.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@kernel.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@sifive.com>
>> Tested-by: Palmer Dabbelt <palmer@sifive.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.

Yep, looks like the PLIC implementation in QEMU is nonsense.  That needs to be 
rewritten, and this needs to be tested on hardware.

>> 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
>
> Thanks,
>
> 	M.

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

[Palmer Dabbelt]

On Sun, 15 Sep 2019 11:20:40 PDT (-0700), maz@kernel.org wrote:
> On Sun, 15 Sep 2019 18:31:33 +0100,
> Palmer Dabbelt <palmer@sifive.com> wrote:
>
> Hi Palmer,
>
>>
>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>> > On Thu, 12 Sep 2019 22:40:34 +0100,
>> > Darius Rad <darius@bluespec.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@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) { }
>> >
>> > 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@kernel.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@kernel.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@sifive.com>
>> Tested-by: Palmer Dabbelt <palmer@sifive.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.

Yep, looks like the PLIC implementation in QEMU is nonsense.  That needs to be 
rewritten, and this needs to be tested on hardware.

>> 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
>
> Thanks,
>
> 	M.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Darius Rad]

On 9/15/19 2:20 PM, Marc Zyngier wrote:
> On Sun, 15 Sep 2019 18:31:33 +0100,
> Palmer Dabbelt <palmer@sifive.com> wrote:
> 
> Hi Palmer,
> 
>>
>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>> Darius Rad <darius@bluespec.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@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) { }
>>>
>>> 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@kernel.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@kernel.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@sifive.com>
>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.com>

> Thanks,
> 
> 	M.
> 

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

[Darius Rad]

On 9/15/19 2:20 PM, Marc Zyngier wrote:
> On Sun, 15 Sep 2019 18:31:33 +0100,
> Palmer Dabbelt <palmer@sifive.com> wrote:
> 
> Hi Palmer,
> 
>>
>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>> Darius Rad <darius@bluespec.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@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) { }
>>>
>>> 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@kernel.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@kernel.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@sifive.com>
>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.com>

> Thanks,
> 
> 	M.
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

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@sifive.com> wrote:
>>
>> Hi Palmer,
>>
>>>
>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>
>>>> 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@kernel.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@kernel.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@sifive.com>
>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.
>>

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

[Palmer Dabbelt]

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@sifive.com> wrote:
>>
>> Hi Palmer,
>>
>>>
>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>
>>>> 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@kernel.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@kernel.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@sifive.com>
>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Marc Zyngier]

On Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
Palmer Dabbelt <palmer@sifive.com> 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@sifive.com> wrote:
> >>
> >> Hi Palmer,
> >>  
> >>>
> >>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:  
> >>>> On Thu, 12 Sep 2019 22:40:34 +0100,
> >>>> Darius Rad <darius@bluespec.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@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) { }  
> >>>>
> >>>> 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@kernel.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@kernel.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@sifive.com>
> >>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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?

Sure, no problem.

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

Well, we still have time before -rc1. Once David gets a chance to test
it, I'll apply it. Additional question: do you want this backported to
-stable? If so, how far?

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

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

[Marc Zyngier]

On Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
Palmer Dabbelt <palmer@sifive.com> 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@sifive.com> wrote:
> >>
> >> Hi Palmer,
> >>  
> >>>
> >>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:  
> >>>> On Thu, 12 Sep 2019 22:40:34 +0100,
> >>>> Darius Rad <darius@bluespec.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@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) { }  
> >>>>
> >>>> 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@kernel.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@kernel.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@sifive.com>
> >>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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?

Sure, no problem.

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

Well, we still have time before -rc1. Once David gets a chance to test
it, I'll apply it. Additional question: do you want this backported to
-stable? If so, how far?

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Mon, 16 Sep 2019 14:33:23 PDT (-0700), maz@kernel.org wrote:
> On Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
> Palmer Dabbelt <palmer@sifive.com> 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@sifive.com> wrote:
>> >>
>> >> Hi Palmer,
>> >>
>> >>>
>> >>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>> >>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>> >>>> Darius Rad <darius@bluespec.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@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) { }
>> >>>>
>> >>>> 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@kernel.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@kernel.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@sifive.com>
>> >>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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?
>
> Sure, no problem.
>
>> 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 :)
>
> Well, we still have time before -rc1. Once David gets a chance to test
> it, I'll apply it. Additional question: do you want this backported to
> -stable? If so, how far?

Generally I've just been CCing stable on bug fixes and letting the automation 
handle it.  4.19 was the first version with a PLIC driver, and that was pretty 
broken so this should be deep down in the noise for anyone who's trying to use 
that -- and I doubt anyone is trying to do so.

>
> Thanks,
>
> 	M.

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

[Palmer Dabbelt]

On Mon, 16 Sep 2019 14:33:23 PDT (-0700), maz@kernel.org wrote:
> On Mon, 16 Sep 2019 13:51:58 -0700 (PDT)
> Palmer Dabbelt <palmer@sifive.com> 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@sifive.com> wrote:
>> >>
>> >> Hi Palmer,
>> >>
>> >>>
>> >>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>> >>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>> >>>> Darius Rad <darius@bluespec.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@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) { }
>> >>>>
>> >>>> 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@kernel.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@kernel.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@sifive.com>
>> >>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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?
>
> Sure, no problem.
>
>> 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 :)
>
> Well, we still have time before -rc1. Once David gets a chance to test
> it, I'll apply it. Additional question: do you want this backported to
> -stable? If so, how far?

Generally I've just been CCing stable on bug fixes and letting the automation 
handle it.  4.19 was the first version with a PLIC driver, and that was pretty 
broken so this should be deep down in the noise for anyone who's trying to use 
that -- and I doubt anyone is trying to do so.

>
> Thanks,
>
> 	M.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Paul Walmsley]

Just tested this on the SiFive HiFive Unleashed.  Seems to work OK; 
however I did not stress-test it.

Tested-by: Paul Walmsley <paul.walmsley@sifive.com> # HiFive Unleashed


- Paul


# !cat
cat /proc/interrupts 
           CPU0       CPU1       CPU2       CPU3       
  1:          0          0          0          0  SiFive PLIC   5  10011000.serial
  3:          0          0          0          0  SiFive PLIC  51  10040000.spi
  4:       6266          0          0          0  SiFive PLIC   4  10010000.serial
  5:        102          0          0          0  SiFive PLIC   6  10050000.spi
  6:         37          0          0          0  SiFive PLIC  53  eth0
IPI0:      1134      21128       9024     220261  Rescheduling interrupts
IPI1:        10        143         18          7  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts
#

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

[Paul Walmsley]

Just tested this on the SiFive HiFive Unleashed.  Seems to work OK; 
however I did not stress-test it.

Tested-by: Paul Walmsley <paul.walmsley@sifive.com> # HiFive Unleashed


- Paul


# !cat
cat /proc/interrupts 
           CPU0       CPU1       CPU2       CPU3       
  1:          0          0          0          0  SiFive PLIC   5  10011000.serial
  3:          0          0          0          0  SiFive PLIC  51  10040000.spi
  4:       6266          0          0          0  SiFive PLIC   4  10010000.serial
  5:        102          0          0          0  SiFive PLIC   6  10050000.spi
  6:         37          0          0          0  SiFive PLIC  53  eth0
IPI0:      1134      21128       9024     220261  Rescheduling interrupts
IPI1:        10        143         18          7  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts
#

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[David Abdurachmanov]

On Tue, Sep 17, 2019 at 3:26 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
>
> Just tested this on the SiFive HiFive Unleashed.  Seems to work OK;
> however I did not stress-test it.
>
> Tested-by: Paul Walmsley <paul.walmsley@sifive.com> # HiFive Unleashed
>
>
> - Paul
>
>
> # !cat
> cat /proc/interrupts
>            CPU0       CPU1       CPU2       CPU3
>   1:          0          0          0          0  SiFive PLIC   5  10011000.serial
>   3:          0          0          0          0  SiFive PLIC  51  10040000.spi
>   4:       6266          0          0          0  SiFive PLIC   4  10010000.serial
>   5:        102          0          0          0  SiFive PLIC   6  10050000.spi
>   6:         37          0          0          0  SiFive PLIC  53  eth0
> IPI0:      1134      21128       9024     220261  Rescheduling interrupts
> IPI1:        10        143         18          7  Function call interrupts
> IPI2:         0          0          0          0  CPU stop interrupts
> #

I have applied the patch on top of 5.2.9 kernel and tried to stress it
with stress-ng interrupt stressors for 2:30+ hours.

# cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3
  1:          0          0          0          0  SiFive PLIC   5
10011000.serial
  3:          0          0          0          0  SiFive PLIC  51  10040000.spi
  4:      34240          0          0          0  SiFive PLIC   4
10010000.serial
  5:        102          0          0          0  SiFive PLIC   6  10050000.spi
  6:          0          0          0          0  SiFive PLIC  53  eth0
  7:          0          0          0          0  SiFive PLIC  32
microsemi-pcie
IPI0:  32013933   28068736   29345256   23346339  Rescheduling interrupts
IPI1:     78514      78586      63144     100317  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts

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

[David Abdurachmanov]

On Tue, Sep 17, 2019 at 3:26 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
>
> Just tested this on the SiFive HiFive Unleashed.  Seems to work OK;
> however I did not stress-test it.
>
> Tested-by: Paul Walmsley <paul.walmsley@sifive.com> # HiFive Unleashed
>
>
> - Paul
>
>
> # !cat
> cat /proc/interrupts
>            CPU0       CPU1       CPU2       CPU3
>   1:          0          0          0          0  SiFive PLIC   5  10011000.serial
>   3:          0          0          0          0  SiFive PLIC  51  10040000.spi
>   4:       6266          0          0          0  SiFive PLIC   4  10010000.serial
>   5:        102          0          0          0  SiFive PLIC   6  10050000.spi
>   6:         37          0          0          0  SiFive PLIC  53  eth0
> IPI0:      1134      21128       9024     220261  Rescheduling interrupts
> IPI1:        10        143         18          7  Function call interrupts
> IPI2:         0          0          0          0  CPU stop interrupts
> #

I have applied the patch on top of 5.2.9 kernel and tried to stress it
with stress-ng interrupt stressors for 2:30+ hours.

# cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3
  1:          0          0          0          0  SiFive PLIC   5
10011000.serial
  3:          0          0          0          0  SiFive PLIC  51  10040000.spi
  4:      34240          0          0          0  SiFive PLIC   4
10010000.serial
  5:        102          0          0          0  SiFive PLIC   6  10050000.spi
  6:          0          0          0          0  SiFive PLIC  53  eth0
  7:          0          0          0          0  SiFive PLIC  32
microsemi-pcie
IPI0:  32013933   28068736   29345256   23346339  Rescheduling interrupts
IPI1:     78514      78586      63144     100317  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Darius Rad]

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@sifive.com> wrote:
>>>
>>> Hi Palmer,
>>>
>>>>
>>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>>
>>>>> 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@kernel.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@kernel.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@sifive.com>
>>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.
>>>

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

[Darius Rad]

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@sifive.com> wrote:
>>>
>>> Hi Palmer,
>>>
>>>>
>>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>>
>>>>> 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@kernel.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@kernel.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@sifive.com>
>>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.
>>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

On Mon, 16 Sep 2019 14:41:07 PDT (-0700), Darius Rad wrote:
> 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@sifive.com> wrote:
>>>>
>>>> Hi Palmer,
>>>>
>>>>>
>>>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>>>
>>>>>> 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@kernel.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@kernel.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@sifive.com>
>>>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.

Ya, that's ours.

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

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

[Palmer Dabbelt]

On Mon, 16 Sep 2019 14:41:07 PDT (-0700), Darius Rad wrote:
> 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@sifive.com> wrote:
>>>>
>>>> Hi Palmer,
>>>>
>>>>>
>>>>> On Sun, 15 Sep 2019 07:24:20 PDT (-0700), maz@kernel.org wrote:
>>>>>> On Thu, 12 Sep 2019 22:40:34 +0100,
>>>>>> Darius Rad <darius@bluespec.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@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) { }
>>>>>>
>>>>>> 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@kernel.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@kernel.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@sifive.com>
>>>>> Tested-by: Palmer Dabbelt <palmer@sifive.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@bluespec.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.

Ya, that's ours.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Christoph Hellwig]

With the fixes it seems to still work fine on the Kendryte KD210.
Although currently only the serial interrupt is tested and this might
not be a very exhaustive test case..

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

[Christoph Hellwig]

With the fixes it seems to still work fine on the Kendryte KD210.
Although currently only the serial interrupt is tested and this might
not be a very exhaustive test case..

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[tip: irq/urgent] irqchip/sifive-plic: Switch to fasteoi flow

[tip-bot2 for Marc Zyngier]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     bb0fed1c60cccbe4063b455a7228818395dac86e
Gitweb:        https://git.kernel.org/tip/bb0fed1c60cccbe4063b455a7228818395dac86e
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Sun, 15 Sep 2019 15:17:45 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Wed, 18 Sep 2019 12:29:52 +01:00

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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Palmer Dabbelt <palmer@sifive.com> (QEMU Boot)
Tested-by: Darius Rad <darius@bluespec.com> (on 2 HW PLIC implementations)
Tested-by: Paul Walmsley <paul.walmsley@sifive.com> (HiFive Unleashed)
Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/8636gxskmj.wl-maz@kernel.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 cf75596..3e51dee 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_unmask(struct irq_data *d)
 {
 	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_mask(struct irq_data *d)
 {
 	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
 }
@@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	if (!irqd_irq_disabled(d)) {
-		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
-		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
-	}
+	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
+	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
@@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d,
 }
 #endif
 
+static void plic_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+
+	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+}
+
 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,
+	.irq_eoi	= plic_irq_eoi,
 #ifdef CONFIG_SMP
 	.irq_set_affinity = plic_set_affinity,
 #endif
@@ -152,7 +154,7 @@ static struct irq_chip plic_chip = {
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
+	irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
 	irq_set_chip_data(irq, NULL);
 	irq_set_noprobe(irq);
 	return 0;
@@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs)
 					hwirq);
 		else
 			generic_handle_irq(irq);
-		writel(hwirq, claim);
 	}
 	csr_set(sie, SIE_SEIE);
 }