Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/23/2013 02:01 PM, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Put another way, I don't believe
>> there's any rule when writing DT bindings that states that bindings must
>> not describe the same pin as both a GPIO and an IRQ, although admittedly
>> that may be unusual.
> 
> Actually I think you've won me over here.
> 
> But I do not think that this shall be done at the expense of letting
> DT-based drivers do nasty things like setting up the same GPIO
> line as an IRQ (hammering the hardware to do that) and then
> request the same line to be an output line and drive it, for
> example.
> 
> So the state of the GPIO line has to be tracked: if it is set as
> input and tied to an IRQ it should *not* be possible for the
> other code path to request it and set it as output. But it should
> be possible to set it as input again and read the value.
> 
> Laurent deascribed exactly the latter usecase, which is OK.
> 
>>> I agree with you that it would be the best if the only call would be
>>> request_irq and the chip driver programs the HW appropriately. It would be a
>>> dream, but unfortunately this is not possible at the moment. This is something
>>> that Linus pointed out very very early in this whole discussion. The gpio and
>>> irq frameworks don't share any information. The irq framework has no chance to
>>> program the HW, because it will never find the related gpio.
>>> For this to work the frameworks have to change (and possibly all drivers/board
>>> files/whatever using request_irq() and/or request_gpio()) have to change.
>>> That is something that I do not dare to do alone.
>>
>> This is a controller-specific issue, and has nothing to do with the GPIO
>> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
>> needs to program the HW when the IRQ is requested or set up. The Tegra
>> driver already works this way, so there's actual proof that it is
>> possible to do this in practice.
> 
> And how to you block the same line from being gpio_request()ed
> and set as output?

To be honest, I really don't think this problem is terribly likely to
occur, so I'm really not convinced that it's worth putting a lot of
effort into solving it.

If the problem does occur, it's trivial to see that this has happened by
looking at /proc/interrupts and /sys/kernel/debug/gpio, assuming the
system doesn't hang when this happens due to an interrupt storm, and if
it does, it should be pretty easy to track down the problematic register
write.

That said, I'm not outright against the kernel checking for this
situation. I think it'd work as follows:

The only code that knows the correlation between the IRQ and GPIO in
question is the combined gpio_chip/irq_chip driver. Everything /has/ to
be driven through that driver. In some cases, an IRQ may not be related
to any GPIO at all; there are certainly IRQ controllers which take
inputs directly from outside the chip, but have no GPIO functionality at
all on those signals.

That driver needs to maintain some state that indicates which of its
IRQs have been requested. Any time a GPIO request (I mean e.g.
set_output() not request_gpio)) comes in, the request needs to be
validated against that IRQ usage state. If there's a conflict, deny the
GPIO request.

Now, it's quite possible that the code to maintain this state and
perform the checks will be similar/common across multiple drivers. If
so, by all means implement the code somewhere common, and have the
various irq_chip/gpio_chip drivers call into it.

The main thing is that all of this has to be driven/controlled/enabled
by the gpio_chip/irq_chip driver itself, not as some global/blanket
feature of the GPIO or IRQ subsystems.

Perhaps rather than having the gpio_chip/irq_chip drivers physically
implement a function which calls this common code, they could set some
flags/data/... in the struct gpio_chip/irq_chip indicating that they
desire the core code that implements the error-checking to be enabled.
That might save some levels of stack. But the key is still that the
whole scenario is controlled by the end driver, not always assumed to
apply by the core code.