Re: [PATCH v5 2/9] drivers: irqchip: Add STM32 external interrupts support

From: Thomas Gleixner <tglx@linutronix.de>
To: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Jason Cooper <jason@lakedaemon.net>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-gpio@vger.kernel.org, arnd@arndb.de,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Daniel Thompson <daniel.thompson@linaro.org>,
	bruherrera@gmail.com, lee.jones@linaro.org
Subject: Re: [PATCH v5 2/9] drivers: irqchip: Add STM32 external interrupts support
Date: Wed, 14 Sep 2016 11:19:02 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1609141103420.6233@nanos> (raw)
In-Reply-To: <6941f61d-0b33-4108-0135-b11887cd0488@st.com>

On Tue, 13 Sep 2016, Alexandre Torgue wrote:
> On 09/13/2016 05:21 PM, Thomas Gleixner wrote:
> > On Fri, 9 Sep 2016, Alexandre TORGUE wrote:
> > > +static void stm32_exti_free(struct irq_domain *d, unsigned int virq,
> > > +			    unsigned int nr_irqs)
> > > +{
> > > +	struct irq_data *data = irq_get_irq_data(virq);
> > > +
> > > +	irq_gc_mask_clr_bit(data->parent_data);
> > 
> > I have a hard time to understand this. The irq domain is not hierarchical.
> 
> Actually, I wanted to test ".free" callback function of gpio_irq_domain in
> STM32 pinctrl driver. To do that I modified gpio driver: just after getting
> virq through gpio_to_irq, I called "irq_dispose_mapping(virq)".
> I know it is dirty but I thought it was the only way to test.
> 
> Doing that, I see that ".free" callback of gpio domain is called but as it is
> hirerchical ".free" callback for parent domain (exti one) is also called. I
> observed that virq was well unmapped, but not masked at exti level. It is for
> this reason than I added "irq_gc_mask_clr_bit(data->parent_data);" which mask
> interrupt at exti level.

Aargh. I really misread the patch, but this is entirely non obvious and you
should do:

       struct irq_data *data = irq_domain_get_irq_data(d, virq);

       irq_gc_mask_clr_bit(d);

Then it is entirely clear that you mask the interrupt of _this_ (the exti)
domain.

Now what really bugs me is that you do that at all. An interrupt which is
freed must be masked already. Why is it unmasked in the first place?

Thanks,

	tglx