From: Pascal Paillet <[email protected]>
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 <[email protected]>
Signed-off-by: Antonio Borneo <[email protected]>
---
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;
+}
+
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,
--
2.36.0
On Tue, 10 May 2022 17:41:20 +0100,
Antonio Borneo <[email protected]> wrote:
>
> From: Pascal Paillet <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Antonio Borneo <[email protected]>
> ---
> 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.
--
Without deviation from the norm, progress is not possible.
On Tue, 2022-05-10 at 19:44 +0100, Marc Zyngier wrote:
> On Tue, 10 May 2022 17:41:20 +0100,
> Antonio Borneo <[email protected]> wrote:
> >
> > From: Pascal Paillet <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Antonio Borneo <[email protected]>
> > ---
> > 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