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

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> Hi Lars, Linus,
> 
> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
>>
>> In the non-DT probe path it is clear that the GPIO number has to
>> be passed to the consuming device, and if it is to be used as
>> an interrupt, the consumer shall performa a gpio_to_irq() mapping
>> and request the resulting IRQ number.
>>
>> In the DT boot path consumers may have been given one or more
>> interrupts from the interrupt-controller side of the GPIO chip
>> in an abstract way, such that the driver is not aware of the
>> fact that a GPIO chip is backing this interrupt, and the GPIO
>> side of the same line is never requested with gpio_request().
>> A typical case for this is ethernet chips which just take some
>> interrupt line which may be a "hard" interrupt line (such as an
>> external interrupt line from an SoC) or a cascaded interrupt
>> connected to a GPIO line.
>>
>> This has the following undesired effects:
>>
>> - The GPIOlib subsystem is not aware that the line is in use
>>   and willingly lets some other consumer perform gpio_request()
>>   on it, leading to a complex resource conflict if it occurs.
>>
>> - The GPIO debugfs file claims this GPIO line is "free".
>>
>> - The line direction of the interrupt GPIO line is not
>>   explicitly set as input, even though it is obvious that such
>>   a line need to be set up in this way, often making the system
>>   depend on boot-on defaults for this kind of settings.

That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.

>> To solve this dilemma, perform an interrupt consistency check
>> when adding a GPIO chip: if the chip is both gpio-controller and
>> interrupt-controller, walk all children of the device tree,

It seems a little odd to solve this only for DT. What about the non-DT case?

>> check if these in turn reference the interrupt-controller, and
>> if they do, loop over the interrupts used by that child and
>> perform gpio_request() and gpio_direction_input() on these,
>> making them unreachable from the GPIO side.

What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.

Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?

That would cover both the exceptions I pointed out above.

I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...

>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

>> +static void of_gpio_scan_irq_lines(const struct device_node *const

>> +		for (i = 0; i < intlen; i += intsize) {
>> +			/*
>> +			 * Find out the local IRQ number. This corresponds to
>> +			 * the GPIO line offset for a GPIO chip.
> 
> I'm still not convinced that this assumption is correct. This code will
> behave erraticaly in cases where it is not true, requesting innocent GPIO
> pins.
> 
>> +			 */
>> +			if (irq_domain && irq_domain->ops->xlate)
>> +				irq_domain->ops->xlate(irq_domain, gcn,
>> +						       intspec + i, intsize,
>> +						       &hwirq, &type);
>> +			else
>> +				hwirq = intspec[0];
> 
> Is it a correct fallback when irq_domain is NULL?

Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.

> 
>> +
>> +			hwirq = be32_to_cpu(hwirq);
> 
> Is this conversion correct? I don't think hwirq could be big endian here
> (unless running on a big endian CPU).

I think that should be inside the else branch above.