| Message ID | 20220510164123.557921-4-antonio.borneo@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] irqchip/stm32-exti: set_affinity return IRQ_SET_MASK_OK_DONE if no parent | expand |
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. M.
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
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.
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; +} + static int stm32_exti_h_set_type(struct irq_data *d, unsigned int type) { struct stm32_exti_chip_data *chip_data = irq_data_get_irq_chip_data(d); @@ -677,6 +687,8 @@ static struct irq_chip stm32_exti_h_chip = { .irq_eoi = stm32_exti_h_eoi, .irq_mask = stm32_exti_h_mask, .irq_unmask = stm32_exti_h_unmask, + .irq_request_resources = stm32_exti_h_request_resources, + .irq_release_resources = irq_chip_release_resources_parent, .irq_retrigger = stm32_exti_h_retrigger, .irq_set_type = stm32_exti_h_set_type, .irq_set_wake = stm32_exti_h_set_wake, @@ -690,6 +702,8 @@ static struct irq_chip stm32_exti_h_chip_direct = { .irq_ack = irq_chip_ack_parent, .irq_mask = stm32_exti_h_mask, .irq_unmask = stm32_exti_h_unmask, + .irq_request_resources = stm32_exti_h_request_resources, + .irq_release_resources = irq_chip_release_resources_parent, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = irq_chip_set_type_parent, .irq_set_wake = stm32_exti_h_set_wake,