Messages in this thread | | | Subject | Re: [PATCH 4/7] irqchip/stm32-exti: forward irq_request_resources to parent | From | Antonio Borneo <> | Date | Wed, 11 May 2022 16:55:03 +0200 |
| |
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
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
| |