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;
Message-ID: <87d059e62a54e2216acdf4ca3a6a81b8d771af42.camel@foss.st.com>
Subject: Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request_resources
 to parent
From:   Antonio Borneo <antonio.borneo@foss.st.com>
To:     Marc Zyngier <maz@kernel.org>
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>
Date:   Wed, 11 May 2022 16:55:03 +0200
In-Reply-To: <875ymd6xdu.wl-maz@kernel.org>
References: <20220510164123.557921-1-antonio.borneo@foss.st.com>
         <20220510164123.557921-4-antonio.borneo@foss.st.com>
         <875ymd6xdu.wl-maz@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1 
MIME-Version: 1.0
Precedence: bulk

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:
> >=20
> > From: Pascal Paillet <p.paillet@foss.st.com>
> >=20
> > 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.
> >=20
> > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
> > ---
> > =C2=A0drivers/irqchip/irq-stm32-exti.c | 14 ++++++++++++++
> > =C2=A01 file changed, 14 insertions(+)
> >=20
> > 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)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0irq_chip_unmask_parent(d);
> > =C2=A0}
> > =C2=A0
> > +static int stm32_exti_h_request_resources(struct irq_data *data)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0data =3D data->parent_data;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (data->chip->irq_request_=
resources)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return data->chip->irq_request_resources(data);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> > +}
>=20
> Why do you need to reinvent the whole thing? Why isn't it just:
>=20
> static int stm32_exti_h_request_resources(struct irq_data *data)
> {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0irq_chip_request_resource=
s_parent(data);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> }
>=20
> 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

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 =3D irq_chip_request_resources_parent(data);
	/* not an error if parent does not implement it */
	return (ret =3D=3D -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.

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)
?

Regards,
Antonio