Re: [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains

[Linus Walleij <linus.walleij@linaro.org>]

Cc:ing Neil Brown who might hit me on the fingers for
some statements here, let's see :)

On Tue, Dec 18, 2018 at 11:06 PM Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Dec 14, 2018 at 02:41:01PM +0100, Linus Walleij wrote:

> > > -       gpiochip->to_irq = gpiochip_to_irq;
> > > +       /*
> > > +        * Allow GPIO chips to override the ->to_irq() if they really need to.
> > > +        * This should only be very rarely needed, the majority should be fine
> > > +        * with gpiochip_to_irq().
> > > +        */
> > > +       if (!gpiochip->to_irq)
> > > +               gpiochip->to_irq = gpiochip_to_irq;
> >
> > And I see this is what your driver does, which leaves the default
> > domain hierarchy callback path unused.
>
> Actually this is only temporary until the patch that uses a sparse
> number space (with the valid masks). After the last patch in the series
> the need to override this goes away, so I could follow-up with a patch
> to revert this. Or alternatively we could squash the two Tegra patches.
> I think keeping them separate is slightly nicer.

OK then!

> Of course, I would even prefer to not move to the sparse number space,
> but you seemed to feel very strongly about it last time we discussed
> this.

Admittedly it is one of those things where I am working on intuition
and as such it may be wrong.

The main reason in this case is to keep interfaces simple.
Of course "simple" is a bit subjective. But keeping to a
certain predictable pattern makes things easier to understand
as long as the pattern is somewhat reasonable.

This pattern I think it is related to the irqdomains: in the
irqdomains a linear irqdomain with dynamically allocated
IRQ descriptors from a sparse number space is clearly
preferred.

By similarity and the fact that we are mapping GPIOs to
IRQs here, I think it is more intuitive and less confusing
to use a linear GPIO offset range and dynamically
allocated GPIO descriptors.

Today we do not allocate GPIO descriptors
dynamically, they are allocated as part of the gpiochip
struct. But I do see it as an end goal to allocate them
dynamically or at least not allocate GPIO descriptors for
the "holes" in the .valid_mask.

> > It's better if you indirect the pointer like we do with some other
> > irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq
> > is defined, we save that function pointer and call that at the
> > end of the gpiochip_to_irq() pointer, then we override it
> > with our own.
> >
> > Since the Tegra186 clearly .to_irq() clearly is mostly the
> > same plus some extra, this should work fine and give
> > lesser lines of code in that driver.
>
> That sounds an awful lot like a midlayer. I'm not a big fan of that at
> all. There are various reasons for this, but others have described it in
> much better detail than I could. See here for example:
>
>         https://lwn.net/Articles/336262/

Reading that article I do not think gpiolib qualifies as midlayer,
since it is after all optional and not every driver uses it, so
it is available on a case-by-case basis. The article even says:

"That common functionality which it is so tempting to put in a
midlayer should instead be provided as library routines which
can used, augmented, or ignored by each bottom level
driver independently."

Contrast that by the SCSI emulation in libata: is it even
possible to bypass? Not really, no. gpiolib is not like that.

> In this particular case one potential problem is that gpiochip_to_irq()
> might not always do the right things for everyone, and that it turn may
> incentivize people to work around it creatively. For example, one driver
> may want to perform some operation before gpiochip_to_irq() is called,
> while another driver may have to perform an operation after the call. If
> you move towards a midlayer there's no way to satisfy both, unless you
> go and add pre- and post-operations of some sort.

I don't really buy into that.

gpiochip_to_irq() is *ONLY* a translation mechanism. The main
abstraction is and should be irqchip.

In usecases like device tree the same device exposes a
gpiochip API and an irqchip API and those two are supposed
to be orthogonal. The Documentation/driver-api/gpio/driver.rst
says:

--------8<----------8<--------8<--------8<--------
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

So always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
been called first.

This orthogonality leads to ambiguities that we need to solve: if there is
competition inside the subsystem which side is using the resource (a certain
GPIO line and register for example) it needs to deny certain operations and
keep track of usage inside of the gpiolib subsystem. This is why the API
below exists.
--------8<----------8<--------8<--------8<--------

So now indeed it seems that your implementation actually
has a bug here. I didn't see it before: if we make use of
the irq off the irqchip without gpiochip_to_irq() having
been called before, will the (hierarchical) IRQ still work?

Much of the idea and reason behind GPIOLIB_IRQCHIP is
to make sure this can hold true.

So this hunk of your original patch:

+       if (irq_domain_is_hierarchy(domain)) {
+               struct irq_fwspec spec;
+
+               spec.fwnode = domain->fwnode;
+               spec.param_count = 2;
+               spec.param[0] = offset;
+               spec.param[1] = 0;
+
+               return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec);
+       }

Is probably misplaced: gpiochip_to_irq() should really
only contain irq_create_mapping() which is what will
also be done by the device tree (or ACPI, I hope) core
when providing an irq as a resource.

The snippet above needs to go somewhere else, if it
needs to happen before any of the irqchip callbacks
then just loop over all the IRQs in
gpiochip_add_irqchip() and allocate IRQs for all valid
IRQs from the start as a last resort I suppose.

A last alternative would be to assume that all other ways
an IRQ is mapped (besides using gpiochip_to_irq()) will
also do the same thing. E.g, you would have to go into
drivers/of/irq.c and patch irq_of_parse_and_map()
to do the same thing. This is the site that creates the
mapping when .to_irq() is not called in the DT case.

Maybe the DT or irqchip maintainers can give a better
answer to how this should happen. Relying on .to_irq()
to always be called is certainly not the answer. We already
did that mistake in the past :(

> I'd like to propose that we instead provide gpiochip_to_irq() as a
> helper that drivers can call into.

>        static int foo_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
>        {
>                /* do something before */
>                ...
>
>               err = gpiochip_to_irq(chip, offset);
>               if (err < 0)
>                       return err;
>
>               /* do something after */
>               ...
>       }

Under your assumptions that is fine, and the same reason as to
why we control pins and regulators and clock in callbacks from
say .suspend() instead of enforcing a certain order. It could be
necessary if the translation gets complicated enough.

But right now I am mostly worried that this implementation does
not cover for the fact that we have to make sure that hierarchical
irqchips in gpiolib work even if .to_irq() was not called first.

Yours,
Linus Walleij