Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sun, Jul 7, 2019 at 3:46 AM Brian Masney <masneyb@xxxxxxxxxxxxx> wrote:

> I got this working with spmi-gpio with two additional changes. See below
> for details. Hopefully I'll have time tomorrow evening (GMT-4) to finish
> cleaning up what I have so I can send out my series.

Awesome! No hurry because it is v5.4 material at this point but I'm
hoping to get to something you, Lina and Thierry can all use for early
merge and smoke test.

> > +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
> > +     .activate = gpiochip_irq_domain_activate,
> > +     .deactivate = gpiochip_irq_domain_deactivate,
> > +     .translate = gpiochip_hierarchy_irq_domain_translate,
> > +     .alloc = gpiochip_hierarchy_irq_domain_alloc,
> > +     .free = irq_domain_free_irqs_common,
> > +};
>
> spmi and ssbi gpio both need to subtract one from the hwirq in the
> translate function.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c#L956
>
> I'm going to optionally allow overriding the translate() function
> pointer as well.

Hm was more thinking to let gpiolib call out to an optional translate
function (on top of the template) but this is maybe cleaner.

> > +             /*
> > +              * 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 + i,
> > +                                 hwirq + i,
> > +                                 gc->irq.chip,
> > +                                 gc,
> > +                                 handle_bad_irq,
>                                     ^^^^^^^^^^
> In order to get this working, I had to change handle_bad_irq to
> handle_level_irq otherwise I get this attempted NULL pointer
> dereference:

Hmmmmmmmm that should not happen, we need to get to the
bottom of this.

> [    2.624430] [<c0372af4>] (irq_chip_ack_parent) from [<c0373f6c>] (__irq_do_set_handler+0x1b4/0x1bc)
> [    2.632584] [<c0373f6c>] (__irq_do_set_handler) from [<c0373fc0>] (__irq_set_handler+0x4c/0x78)
> [    2.641441] [<c0373fc0>] (__irq_set_handler) from [<c0375d44>] (irq_domain_set_info+0x38/0x4c)
> [    2.650126] [<c0375d44>] (irq_domain_set_info) from [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc+0x16c/0x22c)
> [    2.658808] [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc) from [<c0376bac>] (__irq_domain_alloc_irqs+0x12c/0x320)

I wonder why irq_chip_ack_parent() is called there.
Like there is some pending IRQ or something.

> The parent's irq_chip struct isn't populated yet and the error occurs
> here:
>
>     void irq_chip_ack_parent(struct irq_data *data)
>     {
>             data = data->parent_data;
>             data->chip->irq_ack(data);
>                   ^^^^
>
> We haven't called irq_domain_alloc_irqs_parent() yet, which is fine.
>
> __irq_do_set_handler() has a special check for handle_bad_irq():
>
> https://elixir.bootlin.com/linux/latest/source/kernel/irq/chip.c#L974
>
> I'm not sure what the proper fix is here and not going to dig into this
> anymore this evening.

I'm a bit puzzled too. :/

> > diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
> > index 1ce7fcd0f989..3099c7fbefdb 100644
> > --- a/Documentation/driver-api/gpio/driver.rst
> > +++ b/Documentation/driver-api/gpio/driver.rst
>
> I'm still on linux next-20190701. Does this patch series of yours
> require any other patches? I get a merge conflict against driver.rst.
> Everything else applies cleanly. I honestly haven't looked in detail
> about the conflicts.

I have some pending documentation patch I think.
Sorry about that.

Yours,
Linus Walleij