Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 22:46:33 +0100
Message-ID: <87k0b78sw6.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] irqchip/exiu: Fix acknowledgment of edge triggered interrupts
In-Reply-To: <20220429183605.gld37gz6taa5k7fk@maple.lan>
References: <20220429165314.2343705-1-daniel.thompson@linaro.org>
        <87pmkz94bc.wl-maz@kernel.org>
        <20220429183605.gld37gz6taa5k7fk@maple.lan>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 29 Apr 2022 19:36:05 +0100,
Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> On Fri, Apr 29, 2022 at 06:39:51PM +0100, Marc Zyngier wrote:
> > On Fri, 29 Apr 2022 17:53:14 +0100,
> > Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > 
> > > +
> > > +	if (!(edge_triggered & BIT(d->hwirq)))
> > > +		writel(BIT(d->hwirq), data->base + EIREQCLR);
> > 
> > Is this write even needed for a level interrupt? Or does the register
> > always behave as a latch irrespective of the trigger?
> 
> It is unconditionally latched; must be cleared or the interrupt will be
> jammed on. Of course, for level interrupts that are still asserted then
> the write will not clear the interrupt.

OK. The HW folks missed a trick here, but hey.

> 
> 
> > >  	irq_chip_eoi_parent(d);
> > >  }
> > > 
> > > @@ -91,10 +100,13 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
> > >  	writel_relaxed(val, data->base + EILVL);
> > > 
> > >  	val = readl_relaxed(data->base + EIEDG);
> > > -	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
> > > +	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) {
> > >  		val &= ~BIT(d->hwirq);
> > > -	else
> > > +		irq_set_handler_locked(d, handle_fasteoi_irq);
> > > +	} else {
> > >  		val |= BIT(d->hwirq);
> > > +		irq_set_handler_locked(d, handle_fasteoi_ack_irq);
> > > +	}
> > >  	writel_relaxed(val, data->base + EIEDG);
> > >
> > >  	writel_relaxed(BIT(d->hwirq), data->base + EIREQCLR);
> > > @@ -104,6 +116,7 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
> > > 
> > >  static struct irq_chip exiu_irq_chip = {
> > >  	.name			= "EXIU",
> > > +	.irq_ack		= exiu_irq_ack,
> > >  	.irq_eoi		= exiu_irq_eoi,
> > >  	.irq_enable		= exiu_irq_enable,
> > >  	.irq_mask		= exiu_irq_mask,
> > > @@ -148,6 +161,8 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
> > >  	struct irq_fwspec parent_fwspec;
> > >  	struct exiu_irq_data *info = dom->host_data;
> > >  	irq_hw_number_t hwirq;
> > > +	int i, ret;
> > > +	u32 edge_triggered;
> > > 
> > >  	parent_fwspec = *fwspec;
> > >  	if (is_of_node(dom->parent->fwnode)) {
> > > @@ -165,7 +180,17 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
> > >  	irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &exiu_irq_chip, info);
> > > 
> > >  	parent_fwspec.fwnode = dom->parent->fwnode;
> > > -	return irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
> > > +	ret = irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	edge_triggered = readl_relaxed(info->base + EIEDG);
> > > +	for (i = 0; i < nr_irqs; i++)
> > > +		irq_set_handler(virq + i, edge_triggered & BIT(i) ?
> > > +						  handle_fasteoi_ack_irq :
> > > +							handle_fasteoi_irq);
> > > +
> > > +	return 0;
> > 
> > Why do you need this at allocation time? I would have expected the
> > trigger configuration to be enough.
> 
> I saw the following in the description of the interrupt trigger modes
> : When requesting an interrupt without specifying a IRQF_TRIGGER, the
> : setting should be assumed to be "as already configured", which may
> : be as per machine or firmware initialisation.
> 
> From that I was concerned that the callback to set the trigger mode
> would not be called in all cases. Thus when I saw that calling
> __irq_set_trigger() was on a conditional code path in __setup_irq()
> then I wrote the above logic.
> 
> I assume I overlooked something? Is a call to exiu_irq_set_type()
> guaranteed to happen in all cases?

My expectations are that the interrupt is configured from
irq_create_fwspec_mapping(), which will set the trigger it obtained
from the firmware, long before the interrupt is setup.

The conditional code you saw in __setup_irq() is to handle the case
where request_irq() is passing a trigger configuration that isn't the
default one.

Either way, you should be able to safely remove this from the
allocation side.

	M.

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