linux-kernel - Re: [RFC] Potential issue with GPIO/IRQ flags

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]

Message-ID: <CAL_JsqKm2ttQ-rikuZYJ7=qVj+-z+Xf8PyiP16=Rh=GoBW9Zhg@mail.gmail.com>
Date:	Thu, 17 Sep 2015 12:20:33 -0500
From:	Rob Herring <robh@...nel.org>
To:	"Andrew F. Davis" <afd@...com>
Cc:	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Stephen Warren <swarren@...dia.com>,
	Andreas Fenkart <afenkart@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [RFC] Potential issue with GPIO/IRQ flags

On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@...com> wrote:
> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>
>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@...com> wrote:
>>>
>>> Hello all,
>>>
>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>> in:
>>>
>>> include/dt-bindings/gpio/gpio.h
>>>
>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>
>>> include/dt-bindings/interrupt-controller/irq.h
>>>
>>> and are flags for IRQ pins.
>>
>>
>> It is perfectly valid for the meaning of the field to be defined by
>> the interrupt controller, and gpio interrupts could do something
>> different. We've tried to standardize this though.
>>
>
> Sure, but in this case these are not what the interrupt controller
> is expecting.

Understood. I was talking generally, not this specific case.

>>> These seem to have been mixed up in a few places, take for example:
>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>> correct usage, but just before on line 1384 we see the issue.
>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>> this IRQ was not hard-coded with the correct edge in the driver
>>> this would not work. What the author probably wanted was
>>> IRQ_TYPE_LEVEL_HIGH.
>>>
>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>
>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>> instances of this. I found this by using one of these files as an
>>> example and giving myself a lot of problems, so I would like to fix
>>> this before it spreads anymore.
>>>
>>> I have a couple of ideas of how to go at this, first would be to
>>> just replace the incorrect flags with what was intended, but for
>>> some of these I don't know what was intended and do not have the
>>> board to test.
>>>
>>> My other solution would be to just change all instances of the GPIO
>>> flags to their value corresponding IRQ flags:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>
>>> this would not make any functional change as the defines would
>>> still evaluate to the same value, but would make it obvious where
>>> a problem may be and that they should probably be checked and
>>> corrected, maybe we could even put a comment after:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>
>>> Well, what do you think?
>>
>>
>> This seems fine. It is no less wrong.
>>
>
> I'm not sure what you mean here.

In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
IRQ_TYPE_EDGE_FALLING if the original text was correct in its
intentions (but broken in implementation). Since the change you
propose doesn't change the actual dtb at all, if it was wrong before
it will still be wrong.

Rob
--
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/