Re: [PATCH] irq: Consider a negative return value of irq_startup() as an error

[Linus Walleij]

On Thu, Feb 27, 2014 at 6:15 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Tue, 25 Feb 2014, Jean-Jacques Hiblot wrote:
>
> The whole drviers/gpio directory is full of this gpio_lock_as_irq()
> called from the guts of irq_chip callbacks braindamage.

OK then we need to figure out where to place this call to lock
the lines as used by IRQs properly.

The whole thing came out as a result of the resolution of a
discussion about semantic dependencies in drivers/gpio, as drivers
were not handling gpio_* calls to gpiolib and irqchip stuff
orthogonally instead relying on implicit handling of call
dependencies.

Anyway, using a GPIO line as an IRQ makes some other usecases
(like setting it to output and bitbanging it as an I2C) impossible,
and the gpiolib definately needs to be aware that it is used as
an IRQ. And some usecases (like the line being an output)
are impossible so the use as IRQ need to be denied.

> The comment above gpiod_lock_as_irq() is so wrong it's not even funny
> anymore.

Sent a patch to fix this.

> The whole idiocy starts with commit d468bf9e, which tells people to
> call this from irq_startup/enable. But it fails to tell, that this is
> pointless because it wont prevent the interrupt from starting up.

The call is still useful for the gpiolib intrinsics, we would be better off
even if the error to lock the line as IRQ was just printed and then
ignored, as then atleast the error message would tell users that they
are trying to do something that is impossible. Before they had no clue
and were debugging their GPIO irqs for days...

> So 11 SoC lemmings followed and added this completely pointless
> nonsense to their drivers.

Maybe for the irq subsystem, but the gpio subsystem is very
happy about now knowing if a GPIO is used as IRQ or not.
It has implications for the (horrid) sysfs API to GPIO for example.

> Dammit, I told you folks often enough that workarounds in some
> subsystem do not actually cure a shortcoming in the core code. When
> the core code was written, the GPIO case which might actually fail was
> definitly not thought of. But that's not a reason for adding
> tasteless and useless workarounds to the GPIO subsystem.

So while it is true that the situation where the locking to IRQ
fails was not handled properly, this was not added to workaround
shortcomings in other subsystems, but rather to make gpiolib
aware that a certain line is used for IRQs so as to stop people
from shooting themselves in the foot using the gpiolib.

> Of course you encouraged people to copy that nonsense all over the
> place.

Not Jean-Jacques! I take the sole honor for that :-)

> All startup functions do the same thing:
>
>     if (gpio_lock_as_irq() < 0)
>        dev_err("BLA");
>     unmask_irq();
>
> Plus the shutdown functions having the gpio_unlock_as_irq() +
> mask_irq() sequence. Sigh.
>
> So the proper solution to the problem if we want to use irq_startup()
> is:
>
>   Change the return value of the irq_startup() callback to int and
>   fixup all existing implementations. This wants to be done with a
>   coccinelle script. If you hurry up, I might squeeze it into 3.14 as
>   there is no risk at all. Otherwise this is going to happen after the
>   3.15 merge window closes.

I think Jean-Jacques says he's onto this so great.

>   After that's done, remove all the copies of startup/shutdown
>   nonsense in drivers/gpio and force all gpio chips to call
>   gpio_set_irq_chip*() instead of irq_set_irq_chip*() and add generic
>   irq_startup/shutdown callbacks which are implemented in the gpiolib
>   core code.

We probably need to mirror irq_set_chip_and_handler() too since
most drivers use this, but I like this idea.

> There is a less intrusive alternative solution:
>
>   Mark the interrupt as GPIO based in the core and let the core call
>   conditonally the gpio_[un]lock_as_irq functions.

I think the first one is better actually.

I'll see what Jean-Jacques comes up with and take it from there unless
he's interested in taking it all the way.

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