Date:   Sun, 15 Sep 2019 10:31:33 -0700 (PDT)
From:   Palmer Dabbelt <palmer@...ive.com>
To:     maz@...nel.org, Paul Walmsley <paul.walmsley@...ive.com>,
        David Johnson <davidj@...ive.com>
CC:     Darius Rad <darius@...espec.com>, linux-riscv@...ts.infradead.org,
        Paul Walmsley <paul.walmsley@...ive.com>,
        linux-kernel@...r.kernel.org, tglx@...utronix.de,
        jason@...edaemon.net
Subject:     Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask

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

Powered by blists - more mailing lists