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

[Antonio Borneo]

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