Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier <maz@kernel.org>]

[huh, missed this email...]

On Tue, 21 Sep 2021 19:23:04 +0100,
Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> On Tue, Sep 21, 2021 at 10:27 AM Marc Zyngier <maz@kernel.org> wrote:
> 
> > Linus: is there a reason why the gpiolib insist on setting its own
> > handler while building the hierarchy?
> 
> Is it this?
> 
>         /*
>          * We set handle_bad_irq because the .set_type() should
>          * always be invoked and set the right type of handler.
>          */
>         irq_domain_set_info(d,
>                             irq,
>                             hwirq,
>                             gc->irq.chip,
>                             gc,
>                             girq->handler,
>                             NULL, NULL);
>         irq_set_probe(irq);
> (...)

It is its relative position wrt to irq_domain_alloc_irqs_parent() that
has the potential for annoyance. irq_domain_set_info() will trigger an
irq startup, which will explode if the parent level hasn't been
initialised correctly.

> 
> IIUC it's because sometimes, on elder systems (such as ixp4xx) some machines
> are still using boardfiles, and drivers are not obtaining IRQs dynamically
> from device tree or ACPI, instead they are set up statically at machine
> init.
>
> I assume it would otherwise be done as part of ops->translate?

No, this is the right spot if you really need to set the handler. But
it should really be after the parent allocation (see below for
something totally untested).

Ultimately, setting the flow handler when there is a parent domain is
a bit odd, as you'd expect the root domain to be in charge of the
overall flow.

Thanks,

	M.

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index abfbf546d159..53221d54c4be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1103,19 +1103,6 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
 	}
 	chip_dbg(gc, "found parent hwirq %u\n", parent_hwirq);
 
-	/*
-	 * We set handle_bad_irq because the .set_type() should
-	 * always be invoked and set the right type of handler.
-	 */
-	irq_domain_set_info(d,
-			    irq,
-			    hwirq,
-			    gc->irq.chip,
-			    gc,
-			    girq->handler,
-			    NULL, NULL);
-	irq_set_probe(irq);
-
 	/* This parent only handles asserted level IRQs */
 	parent_arg = girq->populate_parent_alloc_arg(gc, parent_hwirq, parent_type);
 	if (!parent_arg)
@@ -1137,6 +1124,18 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
 			 parent_hwirq, hwirq);
 
 	kfree(parent_arg);
+
+	if (!ret) {
+		irq_domain_set_info(d,
+				    irq,
+				    hwirq,
+				    gc->irq.chip,
+				    gc,
+				    girq->handler,
+				    NULL, NULL);
+		irq_set_probe(irq);
+	}
+
 	return ret;
 }
 

-- 
Without deviation from the norm, progress is not possible.