Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211011132431.2792797-1-guoren@kernel.org>
In-Reply-To: <20211011132431.2792797-1-guoren@kernel.org>
From:   Anup Patel <anup@brainfault.org>
Date:   Mon, 11 Oct 2021 20:31:51 +0530
Message-ID: <CAAhSdy1xjRE0TJ_Nd9fdz4RAK2J5FskQYbWDVXO7jhfcZsFm_g@mail.gmail.com>
Subject: Re: [PATCH 1/2] irqchip/sifive-plic: Fix duplicate mask/unmask for claim/complete
To:     Guo Ren <guoren@kernel.org>
Cc:     Atish Patra <atish.patra@wdc.com>, Marc Zyngier <maz@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Guo Ren <guoren@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>