[PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

PLIC only has enable-registers not mask/unmask registers. Mixing
mask/unmask with irq_eoi is wrong, because readl(claim) could mask
irq by hardware. We only need mask/unmask to fixup the hardware
which couldn't claim + mask correctly.

If hardware supports claim + mask, it would cause unnecessary
mask/umak operations.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Atish Patra <atish.patra@wdc.com>
---
 drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index cf74cfa82045..0fa46912f452 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -64,6 +64,7 @@ struct plic_priv {
 	struct cpumask lmask;
 	struct irq_domain *irqdomain;
 	void __iomem *regs;
+	bool claim_mask_support;
 };
 
 struct plic_handler {
@@ -111,7 +112,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
 	}
 }
 
-static void plic_irq_unmask(struct irq_data *d)
+static void plic_irq_enable(struct irq_data *d)
 {
 	struct cpumask amask;
 	unsigned int cpu;
@@ -125,7 +126,7 @@ static void plic_irq_unmask(struct irq_data *d)
 	plic_irq_toggle(cpumask_of(cpu), d, 1);
 }
 
-static void plic_irq_mask(struct irq_data *d)
+static void plic_irq_disable(struct irq_data *d)
 {
 	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
 
@@ -168,8 +169,8 @@ static void plic_irq_eoi(struct irq_data *d)
 
 static struct irq_chip plic_chip = {
 	.name		= "SiFive PLIC",
-	.irq_mask	= plic_irq_mask,
-	.irq_unmask	= plic_irq_unmask,
+	.irq_enable	= plic_irq_enable,
+	.irq_disable	= plic_irq_disable,
 	.irq_eoi	= plic_irq_eoi,
 #ifdef CONFIG_SMP
 	.irq_set_affinity = plic_set_affinity,
@@ -181,6 +182,11 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 {
 	struct plic_priv *priv = d->host_data;
 
+	if (!priv->claim_mask_support) {
+		plic_chip.irq_mask	= plic_irq_disable;
+		plic_chip.irq_unmask	= plic_irq_enable;
+	}
+
 	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
 			    handle_fasteoi_irq, NULL, NULL);
 	irq_set_noprobe(irq);
@@ -298,6 +304,8 @@ static int __init plic_init(struct device_node *node,
 	if (WARN_ON(!nr_contexts))
 		goto out_iounmap;
 
+	priv->claim_mask_support = of_property_read_bool(node, "claim-mask-support");
+
 	error = -ENOMEM;
 	priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
 			&plic_irqdomain_ops, priv);
-- 
2.25.1

[PATCH 2/2] dt-bindings: update riscv plic claim-mask-support property

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

Add claim-mask-support to control riscv,plic don't call
unnecessary mask/unmask operations.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Atish Patra <atish.patra@wdc.com>
---
 .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml        | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 08d5a57ce00f..f32c1792604c 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -71,6 +71,8 @@ properties:
     description:
       Specifies how many external interrupts are supported by this controller.
 
+  claim-mask-support: true
+
 required:
   - compatible
   - '#address-cells'
-- 
2.25.1

Re: [PATCH 2/2] dt-bindings: update riscv plic claim-mask-support property

[Anup Patel]

On Mon, Oct 11, 2021 at 6:54 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> Add claim-mask-support to control riscv,plic don't call
> unnecessary mask/unmask operations.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Palmer Dabbelt <palmerdabbelt@google.com>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Atish Patra <atish.patra@wdc.com>
> ---
>  .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml        | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> index 08d5a57ce00f..f32c1792604c 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> @@ -71,6 +71,8 @@ properties:
>      description:
>        Specifies how many external interrupts are supported by this controller.
>
> +  claim-mask-support: true
> +

This DT property does not describe HW in any way, rather it enable/disable
a SW behaviour in the driver. IMO, this DT property should not be added.

Regards,
Anup

>  required:
>    - compatible
>    - '#address-cells'
> --
> 2.25.1
>

Re: [PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[Anup Patel]

On Mon, Oct 11, 2021 at 6:54 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> PLIC only has enable-registers not mask/unmask registers. Mixing
> mask/unmask with irq_eoi is wrong, because readl(claim) could mask

This is an incorrect assumption about readl(claim). When SW does
read(claim) the HW updates internal state that IRQ has been claimed.
The HW can still get same (already claimed) IRQ again before
writel(claim) which will be delivered after writel(claim).

> irq by hardware. We only need mask/unmask to fixup the hardware
> which couldn't claim + mask correctly.

The handle_fasteoi_irq() only calls unmask_irq() mostly when the
underlying IRQ is threaded. Is there any other case ?

Another fact is that all irqchip drivers using handle_fasteoi_irq()
implement irq_mask/unmask().

Regards,
Anup

>
> If hardware supports claim + mask, it would cause unnecessary
> mask/umak operations.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Atish Patra <atish.patra@wdc.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index cf74cfa82045..0fa46912f452 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -64,6 +64,7 @@ struct plic_priv {
>         struct cpumask lmask;
>         struct irq_domain *irqdomain;
>         void __iomem *regs;
> +       bool claim_mask_support;
>  };
>
>  struct plic_handler {
> @@ -111,7 +112,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
>         }
>  }
>
> -static void plic_irq_unmask(struct irq_data *d)
> +static void plic_irq_enable(struct irq_data *d)
>  {
>         struct cpumask amask;
>         unsigned int cpu;
> @@ -125,7 +126,7 @@ static void plic_irq_unmask(struct irq_data *d)
>         plic_irq_toggle(cpumask_of(cpu), d, 1);
>  }
>
> -static void plic_irq_mask(struct irq_data *d)
> +static void plic_irq_disable(struct irq_data *d)
>  {
>         struct plic_priv *priv = irq_data_get_irq_chip_data(d);
>
> @@ -168,8 +169,8 @@ static void plic_irq_eoi(struct irq_data *d)
>
>  static struct irq_chip plic_chip = {
>         .name           = "SiFive PLIC",
> -       .irq_mask       = plic_irq_mask,
> -       .irq_unmask     = plic_irq_unmask,
> +       .irq_enable     = plic_irq_enable,
> +       .irq_disable    = plic_irq_disable,
>         .irq_eoi        = plic_irq_eoi,
>  #ifdef CONFIG_SMP
>         .irq_set_affinity = plic_set_affinity,
> @@ -181,6 +182,11 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>  {
>         struct plic_priv *priv = d->host_data;
>
> +       if (!priv->claim_mask_support) {
> +               plic_chip.irq_mask      = plic_irq_disable;
> +               plic_chip.irq_unmask    = plic_irq_enable;
> +       }
> +
>         irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
>                             handle_fasteoi_irq, NULL, NULL);
>         irq_set_noprobe(irq);
> @@ -298,6 +304,8 @@ static int __init plic_init(struct device_node *node,
>         if (WARN_ON(!nr_contexts))
>                 goto out_iounmap;
>
> +       priv->claim_mask_support = of_property_read_bool(node, "claim-mask-support");
> +
>         error = -ENOMEM;
>         priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
>                         &plic_irqdomain_ops, priv);
> --
> 2.25.1
>

Re: [PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[Guo Ren]

On Mon, Oct 11, 2021 at 11:02 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Mon, Oct 11, 2021 at 6:54 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > PLIC only has enable-registers not mask/unmask registers. Mixing
> > mask/unmask with irq_eoi is wrong, because readl(claim) could mask
>
> This is an incorrect assumption about readl(claim). When SW does
> read(claim) the HW updates internal state that IRQ has been claimed.
> The HW can still get same (already claimed) IRQ again before
> writel(claim) which will be delivered after writel(claim).
Our hw would mask IRQ with readl(claim), so it's unnecessary for our
board. I agree some hardware won't mask IRQ after readl(claim), so I
put DT-bool to control it.

>
> > irq by hardware. We only need mask/unmask to fixup the hardware
> > which couldn't claim + mask correctly.
>
> The handle_fasteoi_irq() only calls unmask_irq() mostly when the
> underlying IRQ is threaded. Is there any other case ?
When in handle_fasteoi_irq ONESHOT path, it will call:
        if (desc->istate & IRQS_ONESHOT)
                mask_irq(desc);

mask_irq->plic_irq_mask->"write 0 to PRIORITY & ENABLE_BASE-bit"

In this IRQ context, it wouldn't call unmask_irq with
cond_unmask_eoi_irq when in IRQCHIP_EOI_THREADED. Then the path would
writel(hwirq, claim) with irq disabled.
static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
{
...
        } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
                chip->irq_eoi(&desc->irq_data);
        }

When IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid
and cause a blocking irq bug.

>
> Another fact is that all irqchip drivers using handle_fasteoi_irq()
> implement irq_mask/unmask().
C9xx needn't call mask&unmask after readl(claim), because:
1. If hw supports readl(claim) with acquiring irq then the mask/unmask
is unnecessary and causes performance problems.
2. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
operation would cause a blocking irq bug.

>
> Regards,
> Anup
>
> >
> > If hardware supports claim + mask, it would cause unnecessary
> > mask/umak operations.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Atish Patra <atish.patra@wdc.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index cf74cfa82045..0fa46912f452 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -64,6 +64,7 @@ struct plic_priv {
> >         struct cpumask lmask;
> >         struct irq_domain *irqdomain;
> >         void __iomem *regs;
> > +       bool claim_mask_support;
> >  };
> >
> >  struct plic_handler {
> > @@ -111,7 +112,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> >         }
> >  }
> >
> > -static void plic_irq_unmask(struct irq_data *d)
> > +static void plic_irq_enable(struct irq_data *d)
> >  {
> >         struct cpumask amask;
> >         unsigned int cpu;
> > @@ -125,7 +126,7 @@ static void plic_irq_unmask(struct irq_data *d)
> >         plic_irq_toggle(cpumask_of(cpu), d, 1);
> >  }
> >
> > -static void plic_irq_mask(struct irq_data *d)
> > +static void plic_irq_disable(struct irq_data *d)
> >  {
> >         struct plic_priv *priv = irq_data_get_irq_chip_data(d);
> >
> > @@ -168,8 +169,8 @@ static void plic_irq_eoi(struct irq_data *d)
> >
> >  static struct irq_chip plic_chip = {
> >         .name           = "SiFive PLIC",
> > -       .irq_mask       = plic_irq_mask,
> > -       .irq_unmask     = plic_irq_unmask,
> > +       .irq_enable     = plic_irq_enable,
> > +       .irq_disable    = plic_irq_disable,
> >         .irq_eoi        = plic_irq_eoi,
> >  #ifdef CONFIG_SMP
> >         .irq_set_affinity = plic_set_affinity,
> > @@ -181,6 +182,11 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> >  {
> >         struct plic_priv *priv = d->host_data;
> >
> > +       if (!priv->claim_mask_support) {
> > +               plic_chip.irq_mask      = plic_irq_disable;
> > +               plic_chip.irq_unmask    = plic_irq_enable;
> > +       }
> > +
> >         irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> >                             handle_fasteoi_irq, NULL, NULL);
> >         irq_set_noprobe(irq);
> > @@ -298,6 +304,8 @@ static int __init plic_init(struct device_node *node,
> >         if (WARN_ON(!nr_contexts))
> >                 goto out_iounmap;
> >
> > +       priv->claim_mask_support = of_property_read_bool(node, "claim-mask-support");
> > +
> >         error = -ENOMEM;
> >         priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
> >                         &plic_irqdomain_ops, priv);
> > --
> > 2.25.1
> >



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[Anup Patel]

On Tue, Oct 12, 2021 at 6:52 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Mon, Oct 11, 2021 at 11:02 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Mon, Oct 11, 2021 at 6:54 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > PLIC only has enable-registers not mask/unmask registers. Mixing
> > > mask/unmask with irq_eoi is wrong, because readl(claim) could mask
> >
> > This is an incorrect assumption about readl(claim). When SW does
> > read(claim) the HW updates internal state that IRQ has been claimed.
> > The HW can still get same (already claimed) IRQ again before
> > writel(claim) which will be delivered after writel(claim).
> Our hw would mask IRQ with readl(claim), so it's unnecessary for our
> board. I agree some hardware won't mask IRQ after readl(claim), so I
> put DT-bool to control it.

Clearly, the PLIC on C9xx does not comply with RISC-V PLIC because
it is violating the "Interrupt Claim Process" and "Interrupt Completion
Process" defined by the RISC-V PLIC specification.
(Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)

I would suggest you to have a separate compatible string
"thead,c9xx-plic" for C9xx PLIC implementation.

>
> >
> > > irq by hardware. We only need mask/unmask to fixup the hardware
> > > which couldn't claim + mask correctly.
> >
> > The handle_fasteoi_irq() only calls unmask_irq() mostly when the
> > underlying IRQ is threaded. Is there any other case ?
> When in handle_fasteoi_irq ONESHOT path, it will call:
>         if (desc->istate & IRQS_ONESHOT)
>                 mask_irq(desc);
>
> mask_irq->plic_irq_mask->"write 0 to PRIORITY & ENABLE_BASE-bit"
>
> In this IRQ context, it wouldn't call unmask_irq with
> cond_unmask_eoi_irq when in IRQCHIP_EOI_THREADED. Then the path would
> writel(hwirq, claim) with irq disabled.
> static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
> {
> ...
>         } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
>                 chip->irq_eoi(&desc->irq_data);
>         }
>
> When IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid
> and cause a blocking irq bug.

Like mentioned above, this is not inline with RISC-V PLIC. You
should call it C9xx PLIC.

>
> >
> > Another fact is that all irqchip drivers using handle_fasteoi_irq()
> > implement irq_mask/unmask().
> C9xx needn't call mask&unmask after readl(claim), because:
> 1. If hw supports readl(claim) with acquiring irq then the mask/unmask
> is unnecessary and causes performance problems.
> 2. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> operation would cause a blocking irq bug.

I would suggest the following:

1) Add a separate compatible string "thead,c9xx-plic" for your PLIC
implementation.

2) In plic_init() when DT node is compatible to "thead,c9xx-plic",
you should set irq_mask/unmask of "plic_chip" to NULL and point
irq_enable/disable of "plic_chip" to plic_irq_mask/unmask.

3) Add a detailed comment block in plic_init() about the differences
in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

Regards,
Anup

>
> >
> > Regards,
> > Anup
> >
> > >
> > > If hardware supports claim + mask, it would cause unnecessary
> > > mask/umak operations.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > Cc: Anup Patel <anup@brainfault.org>
> > > Cc: Atish Patra <atish.patra@wdc.com>
> > > ---
> > >  drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index cf74cfa82045..0fa46912f452 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -64,6 +64,7 @@ struct plic_priv {
> > >         struct cpumask lmask;
> > >         struct irq_domain *irqdomain;
> > >         void __iomem *regs;
> > > +       bool claim_mask_support;
> > >  };
> > >
> > >  struct plic_handler {
> > > @@ -111,7 +112,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> > >         }
> > >  }
> > >
> > > -static void plic_irq_unmask(struct irq_data *d)
> > > +static void plic_irq_enable(struct irq_data *d)
> > >  {
> > >         struct cpumask amask;
> > >         unsigned int cpu;
> > > @@ -125,7 +126,7 @@ static void plic_irq_unmask(struct irq_data *d)
> > >         plic_irq_toggle(cpumask_of(cpu), d, 1);
> > >  }
> > >
> > > -static void plic_irq_mask(struct irq_data *d)
> > > +static void plic_irq_disable(struct irq_data *d)
> > >  {
> > >         struct plic_priv *priv = irq_data_get_irq_chip_data(d);
> > >
> > > @@ -168,8 +169,8 @@ static void plic_irq_eoi(struct irq_data *d)
> > >
> > >  static struct irq_chip plic_chip = {
> > >         .name           = "SiFive PLIC",
> > > -       .irq_mask       = plic_irq_mask,
> > > -       .irq_unmask     = plic_irq_unmask,
> > > +       .irq_enable     = plic_irq_enable,
> > > +       .irq_disable    = plic_irq_disable,
> > >         .irq_eoi        = plic_irq_eoi,
> > >  #ifdef CONFIG_SMP
> > >         .irq_set_affinity = plic_set_affinity,
> > > @@ -181,6 +182,11 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > >  {
> > >         struct plic_priv *priv = d->host_data;
> > >
> > > +       if (!priv->claim_mask_support) {
> > > +               plic_chip.irq_mask      = plic_irq_disable;
> > > +               plic_chip.irq_unmask    = plic_irq_enable;
> > > +       }
> > > +
> > >         irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > >                             handle_fasteoi_irq, NULL, NULL);
> > >         irq_set_noprobe(irq);
> > > @@ -298,6 +304,8 @@ static int __init plic_init(struct device_node *node,
> > >         if (WARN_ON(!nr_contexts))
> > >                 goto out_iounmap;
> > >
> > > +       priv->claim_mask_support = of_property_read_bool(node, "claim-mask-support");
> > > +
> > >         error = -ENOMEM;
> > >         priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
> > >                         &plic_irqdomain_ops, priv);
> > > --
> > > 2.25.1
> > >
>
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/

Re: [PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete

[Guo Ren]

On Tue, Oct 12, 2021 at 11:37 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Oct 12, 2021 at 6:52 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Mon, Oct 11, 2021 at 11:02 PM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 6:54 PM <guoren@kernel.org> wrote:
> > > >
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > PLIC only has enable-registers not mask/unmask registers. Mixing
> > > > mask/unmask with irq_eoi is wrong, because readl(claim) could mask
> > >
> > > This is an incorrect assumption about readl(claim). When SW does
> > > read(claim) the HW updates internal state that IRQ has been claimed.
> > > The HW can still get same (already claimed) IRQ again before
> > > writel(claim) which will be delivered after writel(claim).
> > Our hw would mask IRQ with readl(claim), so it's unnecessary for our
> > board. I agree some hardware won't mask IRQ after readl(claim), so I
> > put DT-bool to control it.
>
> Clearly, the PLIC on C9xx does not comply with RISC-V PLIC because
> it is violating the "Interrupt Claim Process" and "Interrupt Completion
> Process" defined by the RISC-V PLIC specification.
> (Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)
10. Interrupt Completion
The PLIC signals it has completed executing an interrupt handler by
writing the interrupt ID it received from the claim to the
claim/complete register. The PLIC does not check whether the
completion ID is the same as the last claim ID for that target. If the
completion ID does not match an interrupt source that is *currently
enabled* for the target, the completion is silently *ignored*.

How do you think about *currently enabled* & *silently ignored*?


>
> I would suggest you to have a separate compatible string
> "thead,c9xx-plic" for C9xx PLIC implementation.
>
> >
> > >
> > > > irq by hardware. We only need mask/unmask to fixup the hardware
> > > > which couldn't claim + mask correctly.
> > >
> > > The handle_fasteoi_irq() only calls unmask_irq() mostly when the
> > > underlying IRQ is threaded. Is there any other case ?
> > When in handle_fasteoi_irq ONESHOT path, it will call:
> >         if (desc->istate & IRQS_ONESHOT)
> >                 mask_irq(desc);
> >
> > mask_irq->plic_irq_mask->"write 0 to PRIORITY & ENABLE_BASE-bit"
> >
> > In this IRQ context, it wouldn't call unmask_irq with
> > cond_unmask_eoi_irq when in IRQCHIP_EOI_THREADED. Then the path would
> > writel(hwirq, claim) with irq disabled.
> > static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
> > {
> > ...
> >         } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
> >                 chip->irq_eoi(&desc->irq_data);
> >         }
> >
> > When IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid
> > and cause a blocking irq bug.
>
> Like mentioned above, this is not inline with RISC-V PLIC. You
> should call it C9xx PLIC.
>
> >
> > >
> > > Another fact is that all irqchip drivers using handle_fasteoi_irq()
> > > implement irq_mask/unmask().
> > C9xx needn't call mask&unmask after readl(claim), because:
> > 1. If hw supports readl(claim) with acquiring irq then the mask/unmask
> > is unnecessary and causes performance problems.
> > 2. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug.
>
> I would suggest the following:
>
> 1) Add a separate compatible string "thead,c9xx-plic" for your PLIC
> implementation.
>
> 2) In plic_init() when DT node is compatible to "thead,c9xx-plic",
> you should set irq_mask/unmask of "plic_chip" to NULL and point
> irq_enable/disable of "plic_chip" to plic_irq_mask/unmask.
>
> 3) Add a detailed comment block in plic_init() about the differences
> in Claim/Completion process of RISC-V PLIC and C9xx PLIC.
>
> Regards,
> Anup
>
> >
> > >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > If hardware supports claim + mask, it would cause unnecessary
> > > > mask/umak operations.
> > > >
> > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > Cc: Anup Patel <anup@brainfault.org>
> > > > Cc: Atish Patra <atish.patra@wdc.com>
> > > > ---
> > > >  drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > > index cf74cfa82045..0fa46912f452 100644
> > > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > > @@ -64,6 +64,7 @@ struct plic_priv {
> > > >         struct cpumask lmask;
> > > >         struct irq_domain *irqdomain;
> > > >         void __iomem *regs;
> > > > +       bool claim_mask_support;
> > > >  };
> > > >
> > > >  struct plic_handler {
> > > > @@ -111,7 +112,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> > > >         }
> > > >  }
> > > >
> > > > -static void plic_irq_unmask(struct irq_data *d)
> > > > +static void plic_irq_enable(struct irq_data *d)
> > > >  {
> > > >         struct cpumask amask;
> > > >         unsigned int cpu;
> > > > @@ -125,7 +126,7 @@ static void plic_irq_unmask(struct irq_data *d)
> > > >         plic_irq_toggle(cpumask_of(cpu), d, 1);
> > > >  }
> > > >
> > > > -static void plic_irq_mask(struct irq_data *d)
> > > > +static void plic_irq_disable(struct irq_data *d)
> > > >  {
> > > >         struct plic_priv *priv = irq_data_get_irq_chip_data(d);
> > > >
> > > > @@ -168,8 +169,8 @@ static void plic_irq_eoi(struct irq_data *d)
> > > >
> > > >  static struct irq_chip plic_chip = {
> > > >         .name           = "SiFive PLIC",
> > > > -       .irq_mask       = plic_irq_mask,
> > > > -       .irq_unmask     = plic_irq_unmask,
> > > > +       .irq_enable     = plic_irq_enable,
> > > > +       .irq_disable    = plic_irq_disable,
> > > >         .irq_eoi        = plic_irq_eoi,
> > > >  #ifdef CONFIG_SMP
> > > >         .irq_set_affinity = plic_set_affinity,
> > > > @@ -181,6 +182,11 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > > >  {
> > > >         struct plic_priv *priv = d->host_data;
> > > >
> > > > +       if (!priv->claim_mask_support) {
> > > > +               plic_chip.irq_mask      = plic_irq_disable;
> > > > +               plic_chip.irq_unmask    = plic_irq_enable;
> > > > +       }
> > > > +
> > > >         irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > > >                             handle_fasteoi_irq, NULL, NULL);
> > > >         irq_set_noprobe(irq);
> > > > @@ -298,6 +304,8 @@ static int __init plic_init(struct device_node *node,
> > > >         if (WARN_ON(!nr_contexts))
> > > >                 goto out_iounmap;
> > > >
> > > > +       priv->claim_mask_support = of_property_read_bool(node, "claim-mask-support");
> > > > +
> > > >         error = -ENOMEM;
> > > >         priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
> > > >                         &plic_irqdomain_ops, priv);
> > > > --
> > > > 2.25.1
> > > >
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/