Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request_resources to parent

From: Marc Zyngier <maz@kernel.org>
To: Antonio Borneo <antonio.borneo@foss.st.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	<linux-kernel@vger.kernel.org>,
	<linux-stm32@st-md-mailman.stormreply.com>,
	<linux-arm-kernel@lists.infradead.org>,
	Pascal Paillet <p.paillet@foss.st.com>,
	Ludovic Barre <ludovic.barre@foss.st.com>,
	"Loic\ Pallardy" <loic.pallardy@foss.st.com>
Subject: Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request_resources to parent
Date: Thu, 12 May 2022 11:04:41 +0100	[thread overview]
Message-ID: <871qwz6p8m.wl-maz@kernel.org> (raw)
In-Reply-To: <87d059e62a54e2216acdf4ca3a6a81b8d771af42.camel@foss.st.com>

On Wed, 11 May 2022 15:55:03 +0100,
Antonio Borneo <antonio.borneo@foss.st.com> wrote:
> 
> On Tue, 2022-05-10 at 19:44 +0100, Marc Zyngier wrote:
> > On Tue, 10 May 2022 17:41:20 +0100,
> > Antonio Borneo <antonio.borneo@foss.st.com> wrote:
> > > 
> > > From: Pascal Paillet <p.paillet@foss.st.com>
> > > 
> > > Enhance stm32-exti driver to forward request_resources and
> > > release_resources_parent operations to parent.
> > > Do not use irq_request_resources_parent because it returns
> > > an error when the parent does not implement irq_request_resources.
> > > 
> > > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > > Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
> > > ---
> > >  drivers/irqchip/irq-stm32-exti.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/irqchip/irq-stm32-exti.c
> > > b/drivers/irqchip/irq-stm32-exti.c
> > > index c8003f4f0457..3f6d524a87fe 100644
> > > --- a/drivers/irqchip/irq-stm32-exti.c
> > > +++ b/drivers/irqchip/irq-stm32-exti.c
> > > @@ -550,6 +550,16 @@ static void stm32_exti_h_unmask(struct
> > > irq_data *d)
> > >                 irq_chip_unmask_parent(d);
> > >  }
> > >  
> > > +static int stm32_exti_h_request_resources(struct irq_data *data)
> > > +{
> > > +       data = data->parent_data;
> > > +
> > > +       if (data->chip->irq_request_resources)
> > > +               return data->chip->irq_request_resources(data);
> > > +
> > > +       return 0;
> > > +}
> > 
> > Why do you need to reinvent the whole thing? Why isn't it just:
> > 
> > static int stm32_exti_h_request_resources(struct irq_data *data)
> > {
> >         irq_chip_request_resources_parent(data);
> >         return 0;
> > }
> > 
> > And this really deserves a comment. I also wonder whether we should
> > change this behaviour to always return 0.
> 
> Marc,
> the stm32-exti sits in the middle of an irq hierarchy, exactly as the
> "Interrupt remapping controller" in the section "Hierarchy IRQ domain"
> in Documentation/core-api/irq/irq-domain.rst

Yes, I'm painfully aware of this.

> 
> When the "IOAPIC controller" runs a request_*_irq(), it causes calling
> irq_request_resources() of its parent, if the parent implements it.
> There is no automatic propagation in the hierarchy, so it's up to each
> irq_chip in the hierarchy to propagate this call to its parent.
> Using irq_chip_request_resources_parent() fits this use case.
> 
> At the end of the chain, the "Local APIC controller" is not obliged to
> implement the 'optional' irq_request_resources(). And here starts the
> pain:
> irq_chip_request_resources_parent() returns -ENOSYS if the parent does
> not implement the optional irq_request_resources().
> So we need to filter-out the error for unimplemented function, e.g.:
> 
> static int stm32_exti_h_request_resources(struct irq_data *data)
> {
> 	int ret;
> 	ret = irq_chip_request_resources_parent(data);
> 	/* not an error if parent does not implement it */
> 	return (ret == -ENOSYS) ? 0 : ret;
> }
> 
> but then we cannot discriminate if -ENOSYS comes from missing optional
> irq_request_resources() in parent, or from an error inside parent's
> irq_request_resources(). That's why this patch reimplements the wheel.

But you don't need to know the difference either. The only case you
would get this error is if some level doesn't implement it. If there
is an error number clash, this needs to be fixed.

> Shuldn't irq_chip_request_resources_parent() return 0 when the parent
> doesn't implements the optional method, as it's already the case inside
> kernel/irq/manage.c:1390 static int irq_request_resources(struct
> irq_desc *desc)
> ?

This is what I suggested above.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel