Re: [RFC PATCH] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq()

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Jul 18, 2018 at 10:56 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:

> >> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
> >> is first requested and gpiod_direction_output() checks for that. So the
> >> only option is to free the interrupt and re-request it, which is painful.
> >
> > It's not the only option (hehe). If you study:
> > drivers/w1/masters/w1-gpio.c
> > you see that this uses gpiod_set_raw_value() which will ignore
> > all flags and protection and just drive the line.
> >
> > In the W1 case this is done to "recharge" the line.
> >
> > So when you know exactly what you're doing it's fine to just
> > drive the line.
>
> But the problem is not with setting the gpio, it's with changing the
> direction.

So I was thinking the only reason to change the direction from
input to output would be to drive a value on the output, so then
you could request the line as input and use the raw accessor to
anyways actively drive the input when it needs driving.

> I just noticed that there is a gpiod_direction_output_raw() as well
> that skips the IRQ test, but gpiod_direction_output() does more things
> than gpiod_direction_output_raw(), in particular it is calling
> gpio_set_drive_single_ended().

So what happens is that you want some of the semantic checks
in gpiolib and not all of them.

What we want to do is achieve a 1-1 mapping between the
semantics and your usecase.

I'm trying to figure out what is the general usecase that is lurking
behind this, because I hardly think it is restricted to CEC.

> It looks like I can work around this by:
>
> 1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended()
>    is called.
> 2) call gpiod_direction_input() so it is safe to add the interrupt.
> 3) call request_irq
> 4) If I now want to switch from input to output I can disable the irq
>    and call gpiod_direction_output_raw() and use gpiod_is_active_low() to
>    determine whether active is high or low (since I still need that).

Yeah that starts to bypass all the semantics in the core.

If this works I guess go for it, I'm just worried about that it
starts to look a bit duct-tape-y.

I think the core of your problem is really that the IRQchip
abstraction is not providing an easy way to unhinge the IRQ
on the line (including unlocking the line as IRQ in gpiolib) so
you can dynamically switch the IRQ on and off.

Are we trying to work around an irqchip shortcoming in gpiolib?

> I don't see where gpiod_set_raw_value() comes in.

I just inverted your usecase and made input the default mode
and output the (raw) exception instead of the other way around.
Have you tried this? Or is is counter-intuitive?

> In fact, it seems that
> gpiod_set_value() just changes the direction automatically and that there
> is no need to explicitly call gpiod_direction_output() at all,

gpiod_set_value() does not change the direction in the
general sense.

I guess you refer to the open drain code that will use direction
change to input as a way to drive the line high (through the
pull-up resistor on the line).

This does not happen if the chip has an explicit .set_config()
callback that can set a special bit (or so) to make the hardware
natively support open drain. Switching the line to input is
just open drain emulation by using the high impedance state
(input mode) as open drain.

> except once
> during initialization to ensure that gpio_set_drive_single_ended() is called.
> If true, then in point 4 above I can just call gpiod_set_value and drop the
> gpiod_direction_output()/_input() calls altogether.

This seems fragile. If you are relying on the "switch to input mode"
as described above, you are both exploiting gpiolib semantics
and the fact that your particular GPIO controller does not have
native open drain support.

> > This will not work other than with drivers that use GPIOLIB_IRQCHIP
> > right? Since there are a lot of drivers that don't used that,
> > this is not going to work.
>
> Correct. But it is this specific case that causes the problem.
> If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations,
> then those should be fixed there as well, i.e. by adding irq_enable/irq_disable
> hooks.
>
> Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver
> for cec-gpio might be the tegra driver.

Yeah ... but I think that if we add a semantic like that we need to
implement it in all chips. Users will start to expect that they
can always do this. Or you will soon see "why is cec-gpio not
working for me now!?" and "use another GPIO controller" is not
going to be a good answer to that. So it seems like a support
disaster. The CEC GPIO driver is supposed to be generic I
think.

> If the proposed work-around above works, then I'm happy to use that.
> Alternatively we could make a gpiod_direction_output_ignore_irq()
> call that just skips the FLAG_USED_AS_IRQ check.

It all seems pretty duct-tapey.

Isn't the problem that attaching/removing the IRQ from the GPIO
line is heavy/hard to do?

So you are essentially trying to work around a problem with
irqchip by duct-taping around with new semantics in gpiolib?

Maybe it is better if we discuss this with the irqchip maintainers
in that case?

I've been told to use the .irq_request_resources() and
.irq_release_resources() in struct irq_chip to call
.gpiochip_lock_as_irq() and .gpiochip_unlock_as_irq().

But when you look at these functions:
gpiochip_unlock_as_irq() and gpiochip_unlock_as_irq(),
there is nothing that requires slowpath in them. They can
be called with a spinlock held and IRQs disabled.

I think the real problem is that GPIOLIB_IRQCHIP, that
many irqchips use, use module_get()/put() which are slowpath:

static int gpiochip_irq_reqres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        if (!try_module_get(chip->gpiodev->owner))
                return -ENODEV;

        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,
                        "unable to lock HW IRQ %lu for IRQ\n",
                        d->hwirq);
                module_put(chip->gpiodev->owner);
                return -EINVAL;
        }
        return 0;
}

static void gpiochip_irq_relres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        gpiochip_unlock_as_irq(chip, d->hwirq);
        module_put(chip->gpiodev->owner);
}

This leads to the situtation that you can only attach/remove
an IRQ from a GPIO line by a whole cycle of [devm_]request_irq()
[devm_]free_irq().

You would probably think it's fine if you didn't have to do this
heavy lifting and could just request them on probe and
call enable_irq() and disable_irq() when needed, and have these
call down into the irq_chip .irq_enable() and .irq_disable() and
lock the GPIO line as input *there* instead, and then change
this everywhere (yes patch all gpio_chips with an irqchip
driver in that case).

As drivers have likely sometimes already assigned function
pointers to .irq_enable() and .irq_disable() these have to
be copied and stored in struct gpio_irq_chip() by
gpiochip_set_cascaded_irqchip() and called in sequence.
But it can be done.

What about changing the GPIOLIB_IRQCHIP code to just
do the module_get()/put() part on .irq_request_resources()
and move the locking to the .irq_enable()/.irq_disable()
callbacks so that we can enable and disable irqs on the fly
in a better way? (BIG WIN!)

What about going and reinvestigating this instead?

I'm sorry that I can't present any simple solution, but hey,
I presented a really complicated solution instead, isn't it
great! :D

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html