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

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
> On 09/11/2013 12:34 AM, Stephen Warren wrote:
>> On 09/10/2013 03:37 PM, Mark Brown wrote:
>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>>
>>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>>> hence prevent the driver's own gpio_request() from succeeding,
>>>> since the GPIO is already requested? If this is not a problem, it
>>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>>> for the GPIO.
>>>
>>> Or at the very least something that's likely to break in the
>>> future.
>>
>> Looking at the GPIO code, it already prevents double-requests:
>>
>>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>>                 desc_set_label(desc, label ? : "?");
>>>                 status = 0;
>>>         } else {
>>>                 status = -EBUSY;
>>>                 module_put(chip->owner);
>>>                 goto done;
>>>         }
>>
>> And I tested it in practice, and it really does fail.
>>
> 
> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
> double-requests mean that the use-case that you say that have not been covered
> by this patch can't actually happen?
> 
> I mean, if when using board files an explicit call to gpio_request() is made by
> platform code then a driver can't call gpio_request() for the same gpio. So this
> patch shouldn't cause any regression since is just auto-requesting a GPIO when
> is mapped as an IRQ in a DT which basically will be the same that was made by
> board files before.

I'm not familiar with the board file path; Linus describe this.

It sounds like that path is for the case where a driver /only/ cares
about using a pin as an IRQ, and hence the driver only calls
request_irq(). The board file is (earlier) calling gpio_request() in
order to set up that input pin to work correctly as an IRQ. Hence, there
is no double-call to gpio_request().

The case I said wouldn't work is:

* This patch calls gpio_request() in order to make the pin work as an IRQ.

* Driver uses the pin as both a GPIO and an IRQ, and hence calls
gpio_request() and request_irq().

So, there's a double-call to gpio_request(), which fails, and the driver
fails to probe.

I believe this situation is exactly what cause the original patch to the
OMAP driver to be reverted; that patch should have touched the HW
directly to solve the problem when the IRQ was requested, rather than
calling into the GPIO subsystem (which also has the side-effect of
touching the HW in the same way as desired).

> To give you an example of an use-case that this patch is trying to solve:
> 
> OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
> interface with Pseudo-SRAM devices such as ethernet controllers. So with board
> files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):
> ...

As we discussed on IRC (so mainly for the record in the mailing list
archive), I believe that if a driver wants to use a pin as an interrupt
and only an interrupt, even if the pin has the capability in HW to be a
GPIO (or absolutely anything else at all), then the only call in the
entire kernel (board code, DT core code, IRQ core, driver, ...) should
be a single request_irq(), and the IRQ chip driver needs to program the
HW appropriately to make that work.