linux-kernel - Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d059e62a54e2216acdf4ca3a6a81b8d771af42.camel@foss.st.com>
Date:   Wed, 11 May 2022 16:55:03 +0200
From:   Antonio Borneo <antonio.borneo@...s.st.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        Pascal Paillet <p.paillet@...s.st.com>,
        Ludovic Barre <ludovic.barre@...s.st.com>,
        "Loic Pallardy" <loic.pallardy@...s.st.com>
Subject: Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request_resources
 to parent

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@...s.st.com> wrote:
> > 
> > From: Pascal Paillet <p.paillet@...s.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@...s.st.com>
> > Signed-off-by: Antonio Borneo <antonio.borneo@...s.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

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.

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