linux-kernel - Re: Lack of suspend/resume/shutdown ordering between GPIO providers and consumers

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: <299a564f-f2eb-aa5a-067e-688424b91e58@gmail.com>
Date:   Mon, 14 May 2018 16:46:41 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh@...nel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree@...r.kernel.org, Jeffy Chen <jeffy.chen@...k-chips.com>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Joseph Lo <josephl@...dia.com>,
        Doug Berger <opendmb@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: Lack of suspend/resume/shutdown ordering between GPIO providers
 and consumers

On 04/25/2018 11:14 AM, Dmitry Torokhov wrote:
> On Wed, Apr 25, 2018 at 10:00:31AM -0500, Rob Herring wrote:
>> On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>> Hi Linus, Rafael, all
>>>
>>> Our GPIO controller driver: gpio-brcmstb.c has a shutdown callback which
>>> gets invoked when the system is brought into poweroff aka S5. So far so
>>> good, except that we also wish to use gpio_keys.c as a possible wake-up
>>> source, so we may have a number of GPIO pins declared as gpio-keys that
>>> allow the system to wake-up from deep slumber.
>>>
>>> Recently we noticed that we could easily get into a state where
>>> gpio-brcmstb.c::brcmstb_gpio_shutdown() gets called first, and then
>>> gpio_keys.c::gpio_keys_suspend() gets called later, which is too late to
>>> have the enable_irq_wake() call do anything sensible since we have
>>> suspend its parent interrupt controller before. This is completely
>>> expected unfortunately because these two drivers are both platform
>>> device instances with no connection to one another except via Device
>>> Tree and the use of the GPIOLIB APIs.
>>>
>>> First solution is to make sure that gpio-keys nodes are declared in
>>> Device Tree *before* the GPIO controller. This works because Device Tree
>>> nodes are probed in the order in which they are declared in Device Tree
>>> and that directly influences the order in which platform devices are
>>> created. Problem with that is that this is easy to miss and it may not
>>> work with overlays, kexec reconstructing DT etc. etc.
>>
>> I'm going to make of_platform_populate randomize the order it creates devices...
>>
>>> Another possible solution would be have the GPIO controller nodes have
>>> the GPIO consumers nodes such as gpio-keys, gpio-leds etc., and that
>>> would allow the Linux device driver model to create an appropriate
>>> child/parent relationship. This would unfortunately require Device Tree
>>> changes everywhere to make that consistent, and it would be a special
>>> case, because not all GPIO consumers are eligible as child nodes of
>>> their parent GPIO controller, there are plenty of other consumers that
>>> are not suitable for being moved under a parent GPIO controller node.
>>> This would also mean that we need to "probe" GPIO controller nodes to
>>> populate their child nodes (e.g: of_platform_bus_populate).
>>>
>>> I am thinking a more generic solution might involve some more complex
>>> tracking of the provider <-> consumer, but there is room for breakage.
>>
>> That's what device connections are for. It probably just needs the
>> GPIO core to create the links. (but I've not looked into it at all).
> 
> Not all APIs accept device as parameter to easily create links. But I
> wonder, for cases like this, if we could not simply move the device to
> the end of the dpm list after successful binding it to a driver. The
> assumption that when GOPIs or other resources are not ready they'll
> return -EPROBE_DEFER and probing would fail.

Dmitry, do you see any reason why we are enabling the gpio_keys.c button
interrupts for wake-up during suspend/resume only, and not right from
the probe() function?

button->wakeup is effectively read-only past the probe function, if we
moved the logic to enable/disable the interrupts that would greatly
simplify things. I am assuming whomever added that functionality must
have been worried about spurious wake-up events somehow and wanted to do
it as late as possible?

Thanks
-- 
Florian