Subject: Re: [PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow
 handlers.
To: David Daney <david.daney@cavium.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Alexandre Courbot <gnurou@gmail.com>, Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Thomas Gleixner <tglx@linutronix.de>, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org
References: <1488332932-2691-1-git-send-email-david.daney@cavium.com>
 <1488332932-2691-3-git-send-email-david.daney@cavium.com>
Cc: linux-kernel@vger.kernel.org
From: Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <254c0505-6890-2e9f-cf04-3c8efbcfeb2d@arm.com>
Date: Tue, 14 Mar 2017 16:54:08 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Icedove/45.6.0
MIME-Version: 1.0
In-Reply-To: <1488332932-2691-3-git-send-email-david.daney@cavium.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3458
Lines: 105

On 01/03/17 01:48, David Daney wrote:
> Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
> requires slightly different flow handlers, add them to chip.c which
> contains most of the other flow handlers.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  include/linux/irq.h |   2 ++
>  kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index f887351..3db0eb8 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
>  extern int irq_chip_pm_get(struct irq_data *data);
>  extern int irq_chip_pm_put(struct irq_data *data);
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
> +extern void handle_fasteoi_level_irq(struct irq_desc *desc);
>  extern void irq_chip_enable_parent(struct irq_data *data);
>  extern void irq_chip_disable_parent(struct irq_data *data);
>  extern void irq_chip_ack_parent(struct irq_data *data);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 73ea90b..213105d 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -981,6 +981,108 @@ void irq_cpu_offline(void)
>  
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>  /**
> + *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
> + *	stacked on transparent controllers
> + *
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Like handle_fasteoi_irq(), but for use with hierarchy where
> + *	the irq_chip also needs to have its ->irq_ack() function
> + *	called.
> + */
> +void handle_fasteoi_edge_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	raw_spin_lock(&desc->lock);
> +
> +	if (!irq_may_run(desc))
> +		goto out;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +
> +	/*
> +	 * If its disabled or no action available
> +	 * then mask it and get out of here:
> +	 */
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		mask_irq(desc);
> +		goto out;
> +	}
> +
> +	kstat_incr_irqs_this_cpu(desc);
> +	if (desc->istate & IRQS_ONESHOT)
> +		mask_irq(desc);
> +
> +	/* Start handling the irq */
> +	desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> +	preflow_handler(desc);
> +	handle_irq_event(desc);
> +
> +	cond_unmask_eoi_irq(desc, chip);
> +
> +	raw_spin_unlock(&desc->lock);
> +	return;
> +out:
> +	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
> +		chip->irq_eoi(&desc->irq_data);
> +	raw_spin_unlock(&desc->lock);
> +}
> +EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);

So this is handle_fasteoi_irq with an irq_ack() added in the middle. In
the spirit of making this a bit more maintainable, how about making the
handle_fasteoi_irq code reusable (if necessary by forcing the compiler
to inline stuff)?

But the one thing that makes me uncomfortable here is that we're seem to
have this irq_ack() propagated all along the irqdata chain, which is not
what's happening. Only the EOI gets propagated.

Why can't you just put the irq_ack call in your top level irq_eoi
callback? That'd make it similar to what is happening on the mbigen side
(not exactly surprising, since they are doing very similar things).

Same remark about handle_fasteoi_level_irq.

Thoughts?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...