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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Sep 2013 13:43:50 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
CC:	Mark Brown <broonie@...nel.org>,
	Lars Poeschel <poeschel@...onage.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Lars Poeschel <larsi@....tu-dresden.de>,
	Grant Likely <grant.likely@...aro.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ian.campbell@...rix.com>,
	Kumar Gala <galak@...eaurora.org>,
	Pawel Moll <pawel.moll@....com>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Enric Balletbo i Serra <eballetbo@...il.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>,
	Santosh Shilimkar <santosh.shilimkar@...com>,
	Kevin Hilman <khilman@...aro.org>,
	Balaji T K <balajitk@...com>,
	Tony Lindgren <tony@...mide.com>,
	Jon Hunter <jgchunter@...il.com>, joelf@...com
Subject: Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/