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

[Dmitry Torokhov]

On Mon, May 14, 2018 at 04:46:41PM -0700, Florian Fainelli wrote:
> 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  
> >> 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?

Florian, device_may_wakeup() is not constant (it may be changed via
sysfs), and when the driver was written we did not have
dev_pm_set_wake_irq() that can be used in probe() and handles wakeup
flag changing, so we had to arm wake IRQs in suspend. And even now, with
support of different interrupt triggers for normal operation and wakeup
action, we need to reprogram interrupts in suspend.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Mon, May 14, 2018 at 04:46:41PM -0700, Florian Fainelli wrote:
> 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  
> >> 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?

Florian, device_may_wakeup() is not constant (it may be changed via
sysfs), and when the driver was written we did not have
dev_pm_set_wake_irq() that can be used in probe() and handles wakeup
flag changing, so we had to arm wake IRQs in suspend. And even now, with
support of different interrupt triggers for normal operation and wakeup
action, we need to reprogram interrupts in suspend.

Thanks.

-- 
Dmitry

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

[Florian Fainelli]

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

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

[Florian Fainelli]

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

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

[Rafael J. Wysocki]

On Wednesday, April 25, 2018 9:29:59 PM CEST Grygorii Strashko wrote:
> 
> On 04/25/2018 02:10 PM, Grygorii Strashko wrote:
> > 
> > 
> > On 04/25/2018 01:57 PM, Florian Fainelli wrote:
> >> On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
> >>>
> >>>
> >>> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>  On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
> >
> >
> > On 04/24/2018 05:58 PM, Florian Fainelli 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.
> >
> > You can take a look at device_link_add() and Co.
> 
>  OK, though that requires a struct device references, so while I could
>  certainly resolve the device_node -> struct device that corresponds to
>  the GPIO provider , that poses a number of issues:
> 
>  - not all struct device_node have a corresponding struct device
>  reference (e.g: clock providers, interrupt controllers, and possibly
>  other custom drivers), though in this case, they most likely do have one
> 
>  - resolving a struct device associated with a struct device_node is
>  often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>  the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>  not work that easily
> 
>  I think this is what Dmitry just indicated in his email as well.
> 
> >
> > But it's little bit unclear what exactly you have issue with:
> > - shutdown
> > - suspend
> >
> > above are different (at least as it was before) and gpio-brcmstb.c
> > brcmstb_gpio_shutdown() should not be called as part of suspend !?
> > may be you mean brcmstb_gpio_suspend?
> 
>  The issue exists with shutdown (through the use of "poweroff"), that is
>  confirmed, but I cannot see how it does not exist with any suspend state
>  as well, for the same reason that the ordering is not strictly enforced.
> >>>
> >>> Sry, but it still required some clarification :( - poweroff calls
> >>> device_shutdown() which, in turn, should not call .suspend(), so
> >>> how have you got both .shutdown() and .suspend() callbacks called during
> >>> poweroff? Am I missing smth?
> >>
> >> You are missing me telling you the whole story, sorry I got confused,
> >> but you are absolutely right these are separate lists and on
> >> poweroff/shutdown only ->shutdown() is called. What I had missed in the
> >> report I was submitted was that there was a .shutdown() callback being
> >> added to gpio_keys.c, which of course, because it's an Android based
> >> project is not in the upstream Linux kernel.
> >>
> >> The problem does remain valid though AFAICT. Thanks Grygorii!
> >>
> > 
> > Thanks. But that means you should not see this problem :(
> > There is devices_kset_move_last() call in really_probe() which moves probed 
> > dev
> > at the end of kset, and gpio_keys should never be probed before 
> > gpio-brcmstb because
> > both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() 
> > expected to return
> > -EPROBE_DEFER otherwise.
> > 
> > Theoretically issue still might happen with suspend.
> > 
> 
> FYI https://lkml.org/lkml/2015/9/10/218

And also https://patchwork.kernel.org/patch/10334661/

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

[Rafael J. Wysocki]

On Wednesday, April 25, 2018 9:29:59 PM CEST Grygorii Strashko wrote:
> 
> On 04/25/2018 02:10 PM, Grygorii Strashko wrote:
> > 
> > 
> > On 04/25/2018 01:57 PM, Florian Fainelli wrote:
> >> On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
> >>>
> >>>
> >>> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>  On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
> >
> >
> > On 04/24/2018 05:58 PM, Florian Fainelli 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.
> >
> > You can take a look at device_link_add() and Co.
> 
>  OK, though that requires a struct device references, so while I could
>  certainly resolve the device_node -> struct device that corresponds to
>  the GPIO provider , that poses a number of issues:
> 
>  - not all struct device_node have a corresponding struct device
>  reference (e.g: clock providers, interrupt controllers, and possibly
>  other custom drivers), though in this case, they most likely do have one
> 
>  - resolving a struct device associated with a struct device_node is
>  often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>  the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>  not work that easily
> 
>  I think this is what Dmitry just indicated in his email as well.
> 
> >
> > But it's little bit unclear what exactly you have issue with:
> > - shutdown
> > - suspend
> >
> > above are different (at least as it was before) and gpio-brcmstb.c
> > brcmstb_gpio_shutdown() should not be called as part of suspend !?
> > may be you mean brcmstb_gpio_suspend?
> 
>  The issue exists with shutdown (through the use of "poweroff"), that is
>  confirmed, but I cannot see how it does not exist with any suspend state
>  as well, for the same reason that the ordering is not strictly enforced.
> >>>
> >>> Sry, but it still required some clarification :( - poweroff calls
> >>> device_shutdown() which, in turn, should not call .suspend(), so
> >>> how have you got both .shutdown() and .suspend() callbacks called during
> >>> poweroff? Am I missing smth?
> >>
> >> You are missing me telling you the whole story, sorry I got confused,
> >> but you are absolutely right these are separate lists and on
> >> poweroff/shutdown only ->shutdown() is called. What I had missed in the
> >> report I was submitted was that there was a .shutdown() callback being
> >> added to gpio_keys.c, which of course, because it's an Android based
> >> project is not in the upstream Linux kernel.
> >>
> >> The problem does remain valid though AFAICT. Thanks Grygorii!
> >>
> > 
> > Thanks. But that means you should not see this problem :(
> > There is devices_kset_move_last() call in really_probe() which moves probed 
> > dev
> > at the end of kset, and gpio_keys should never be probed before 
> > gpio-brcmstb because
> > both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() 
> > expected to return
> > -EPROBE_DEFER otherwise.
> > 
> > Theoretically issue still might happen with suspend.
> > 
> 
> FYI https://lkml.org/lkml/2015/9/10/218

And also https://patchwork.kernel.org/patch/10334661/

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

[Rafael J. Wysocki]

On Wednesday, April 25, 2018 8:14:35 PM CEST 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  
> > 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.

Not just to the end of dpm_list if shutdown is involved.

Also if you need runtime PM to follow the dependencies, this isn't
sufficient.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Wednesday, April 25, 2018 8:14:35 PM CEST 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  
> > 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.

Not just to the end of dpm_list if shutdown is involved.

Also if you need runtime PM to follow the dependencies, this isn't
sufficient.

Thanks,
Rafael

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

[Grygorii Strashko]

On 04/25/2018 02:10 PM, Grygorii Strashko wrote:



On 04/25/2018 01:57 PM, Florian Fainelli wrote:

On 04/25/2018 11:47 AM, Grygorii Strashko wrote:



On 04/25/2018 01:29 PM, Florian Fainelli wrote:

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:



On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.


OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.



But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
    brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?


The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.


Sry, but it still required some clarification :( - poweroff calls
device_shutdown() which, in turn, should not call .suspend(), so
how have you got both .shutdown() and .suspend() callbacks called during
poweroff? Am I missing smth?


You are missing me telling you the whole story, sorry I got confused,
but you are absolutely right these are separate lists and on
poweroff/shutdown only ->shutdown() is called. What I had missed in the
report I was submitted was that there was a .shutdown() callback being
added to gpio_keys.c, which of course, because it's an Android based
project is not in the upstream Linux kernel.

The problem does remain valid though AFAICT. Thanks Grygorii!



Thanks. But that means you should not see this problem :(
There is devices_kset_move_last() call in really_probe() which moves probed dev
at the end of kset, and gpio_keys should never be probed before gpio-brcmstb 
because
both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() expected to 
return
-EPROBE_DEFER otherwise.

Theoretically issue still might happen with suspend.



FYI https://lkml.org/lkml/2015/9/10/218

--
regards,
-grygorii

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

[Grygorii Strashko]

On 04/25/2018 02:10 PM, Grygorii Strashko wrote:



On 04/25/2018 01:57 PM, Florian Fainelli wrote:

On 04/25/2018 11:47 AM, Grygorii Strashko wrote:



On 04/25/2018 01:29 PM, Florian Fainelli wrote:

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:



On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.


OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.



But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
    brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?


The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.


Sry, but it still required some clarification :( - poweroff calls
device_shutdown() which, in turn, should not call .suspend(), so
how have you got both .shutdown() and .suspend() callbacks called during
poweroff? Am I missing smth?


You are missing me telling you the whole story, sorry I got confused,
but you are absolutely right these are separate lists and on
poweroff/shutdown only ->shutdown() is called. What I had missed in the
report I was submitted was that there was a .shutdown() callback being
added to gpio_keys.c, which of course, because it's an Android based
project is not in the upstream Linux kernel.

The problem does remain valid though AFAICT. Thanks Grygorii!



Thanks. But that means you should not see this problem :(
There is devices_kset_move_last() call in really_probe() which moves probed dev
at the end of kset, and gpio_keys should never be probed before gpio-brcmstb 
because
both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() expected to 
return
-EPROBE_DEFER otherwise.

Theoretically issue still might happen with suspend.



FYI https://lkml.org/lkml/2015/9/10/218

--
regards,
-grygorii

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

[Grygorii Strashko]

On 04/25/2018 01:57 PM, Florian Fainelli wrote:
> On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
>>
>>
>> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>>> On 04/25/2018 11:06 AM, Grygorii Strashko wrote:


 On 04/24/2018 05:58 PM, Florian Fainelli 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.

 You can take a look at device_link_add() and Co.
>>>
>>> OK, though that requires a struct device references, so while I could
>>> certainly resolve the device_node -> struct device that corresponds to
>>> the GPIO provider , that poses a number of issues:
>>>
>>> - not all struct device_node have a corresponding struct device
>>> reference (e.g: clock providers, interrupt controllers, and possibly
>>> other custom drivers), though in this case, they most likely do have one
>>>
>>> - resolving a struct device associated with a struct device_node is
>>> often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>>> the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>>> not work that easily
>>>
>>> I think this is what Dmitry just indicated in his email as well.
>>>

 But it's little bit unclear what exactly you have issue with:
 - shutdown
 - suspend

 above are different (at least as it was before) and gpio-brcmstb.c
    brcmstb_gpio_shutdown() should not be called as part of suspend !?
 may be you mean brcmstb_gpio_suspend?
>>>
>>> The issue exists with shutdown (through the use of "poweroff"), that is
>>> confirmed, but I cannot see how it does not exist with any suspend state
>>> as well, for the same reason that the ordering is not strictly enforced.
>>
>> Sry, but it still required some clarification :( - poweroff calls
>> device_shutdown() which, in turn, should not call .suspend(), so
>> how have you got both .shutdown() and .suspend() callbacks called during
>> poweroff? Am I missing smth?
> 
> You are missing me telling you the whole story, sorry I got confused,
> but you are absolutely right these are separate lists and on
> poweroff/shutdown only ->shutdown() is called. What I had missed in the
> report I was submitted was that there was a .shutdown() callback being
> added to gpio_keys.c, which of course, because it's an Android based
> project is not in the upstream Linux kernel.
> 
> The problem does remain valid though AFAICT. Thanks Grygorii!
> 

Thanks. But that means you should not see this problem :( 
There is devices_kset_move_last() call in really_probe() which moves probed dev
at the end of kset, and gpio_keys should never be probed before gpio-brcmstb 
because
both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() expected to 
return 
-EPROBE_DEFER otherwise.

Theoretically issue still might happen with suspend.

-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 04/25/2018 01:57 PM, Florian Fainelli wrote:
> On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
>>
>>
>> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>>> On 04/25/2018 11:06 AM, Grygorii Strashko wrote:


 On 04/24/2018 05:58 PM, Florian Fainelli 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.

 You can take a look at device_link_add() and Co.
>>>
>>> OK, though that requires a struct device references, so while I could
>>> certainly resolve the device_node -> struct device that corresponds to
>>> the GPIO provider , that poses a number of issues:
>>>
>>> - not all struct device_node have a corresponding struct device
>>> reference (e.g: clock providers, interrupt controllers, and possibly
>>> other custom drivers), though in this case, they most likely do have one
>>>
>>> - resolving a struct device associated with a struct device_node is
>>> often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>>> the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>>> not work that easily
>>>
>>> I think this is what Dmitry just indicated in his email as well.
>>>

 But it's little bit unclear what exactly you have issue with:
 - shutdown
 - suspend

 above are different (at least as it was before) and gpio-brcmstb.c
    brcmstb_gpio_shutdown() should not be called as part of suspend !?
 may be you mean brcmstb_gpio_suspend?
>>>
>>> The issue exists with shutdown (through the use of "poweroff"), that is
>>> confirmed, but I cannot see how it does not exist with any suspend state
>>> as well, for the same reason that the ordering is not strictly enforced.
>>
>> Sry, but it still required some clarification :( - poweroff calls
>> device_shutdown() which, in turn, should not call .suspend(), so
>> how have you got both .shutdown() and .suspend() callbacks called during
>> poweroff? Am I missing smth?
> 
> You are missing me telling you the whole story, sorry I got confused,
> but you are absolutely right these are separate lists and on
> poweroff/shutdown only ->shutdown() is called. What I had missed in the
> report I was submitted was that there was a .shutdown() callback being
> added to gpio_keys.c, which of course, because it's an Android based
> project is not in the upstream Linux kernel.
> 
> The problem does remain valid though AFAICT. Thanks Grygorii!
> 

Thanks. But that means you should not see this problem :( 
There is devices_kset_move_last() call in really_probe() which moves probed dev
at the end of kset, and gpio_keys should never be probed before gpio-brcmstb 
because
both devm_fwnode_get_gpiod_from_child() and devm_gpio_request_one() expected to 
return 
-EPROBE_DEFER otherwise.

Theoretically issue still might happen with suspend.

-- 
regards,
-grygorii

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

[Florian Fainelli]

On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
> 
> 
> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>> On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
>>>
>>>
>>> On 04/24/2018 05:58 PM, Florian Fainelli 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.
>>>
>>> You can take a look at device_link_add() and Co.
>>
>> OK, though that requires a struct device references, so while I could
>> certainly resolve the device_node -> struct device that corresponds to
>> the GPIO provider , that poses a number of issues:
>>
>> - not all struct device_node have a corresponding struct device
>> reference (e.g: clock providers, interrupt controllers, and possibly
>> other custom drivers), though in this case, they most likely do have one
>>
>> - resolving a struct device associated with a struct device_node is
>> often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>> the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>> not work that easily
>>
>> I think this is what Dmitry just indicated in his email as well.
>>
>>>
>>> But it's little bit unclear what exactly you have issue with:
>>> - shutdown
>>> - suspend
>>>
>>> above are different (at least as it was before) and gpio-brcmstb.c
>>>   brcmstb_gpio_shutdown() should not be called as part of suspend !?
>>> may be you mean brcmstb_gpio_suspend?
>>
>> The issue exists with shutdown (through the use of "poweroff"), that is
>> confirmed, but I cannot see how it does not exist with any suspend state
>> as well, for the same reason that the ordering is not strictly enforced.
> 
> Sry, but it still required some clarification :( - poweroff calls
> device_shutdown() which, in turn, should not call .suspend(), so
> how have you got both .shutdown() and .suspend() callbacks called during
> poweroff? Am I missing smth?

You are missing me telling you the whole story, sorry I got confused,
but you are absolutely right these are separate lists and on
poweroff/shutdown only ->shutdown() is called. What I had missed in the
report I was submitted was that there was a .shutdown() callback being
added to gpio_keys.c, which of course, because it's an Android based
project is not in the upstream Linux kernel.

The problem does remain valid though AFAICT. Thanks Grygorii!
-- 
Florian

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

[Florian Fainelli]

On 04/25/2018 11:47 AM, Grygorii Strashko wrote:
> 
> 
> On 04/25/2018 01:29 PM, Florian Fainelli wrote:
>> On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
>>>
>>>
>>> On 04/24/2018 05:58 PM, Florian Fainelli 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.
>>>
>>> You can take a look at device_link_add() and Co.
>>
>> OK, though that requires a struct device references, so while I could
>> certainly resolve the device_node -> struct device that corresponds to
>> the GPIO provider , that poses a number of issues:
>>
>> - not all struct device_node have a corresponding struct device
>> reference (e.g: clock providers, interrupt controllers, and possibly
>> other custom drivers), though in this case, they most likely do have one
>>
>> - resolving a struct device associated with a struct device_node is
>> often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
>> the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
>> not work that easily
>>
>> I think this is what Dmitry just indicated in his email as well.
>>
>>>
>>> But it's little bit unclear what exactly you have issue with:
>>> - shutdown
>>> - suspend
>>>
>>> above are different (at least as it was before) and gpio-brcmstb.c
>>>   brcmstb_gpio_shutdown() should not be called as part of suspend !?
>>> may be you mean brcmstb_gpio_suspend?
>>
>> The issue exists with shutdown (through the use of "poweroff"), that is
>> confirmed, but I cannot see how it does not exist with any suspend state
>> as well, for the same reason that the ordering is not strictly enforced.
> 
> Sry, but it still required some clarification :( - poweroff calls
> device_shutdown() which, in turn, should not call .suspend(), so
> how have you got both .shutdown() and .suspend() callbacks called during
> poweroff? Am I missing smth?

You are missing me telling you the whole story, sorry I got confused,
but you are absolutely right these are separate lists and on
poweroff/shutdown only ->shutdown() is called. What I had missed in the
report I was submitted was that there was a .shutdown() callback being
added to gpio_keys.c, which of course, because it's an Android based
project is not in the upstream Linux kernel.

The problem does remain valid though AFAICT. Thanks Grygorii!
-- 
Florian

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

[Grygorii Strashko]

On 04/25/2018 01:29 PM, Florian Fainelli wrote:

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:



On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.


OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.



But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
  brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?


The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.


Sry, but it still required some clarification :( - poweroff calls
device_shutdown() which, in turn, should not call .suspend(), so
how have you got both .shutdown() and .suspend() callbacks called during
poweroff? Am I missing smth?

Note. Suspend and shutdown uses different dev lists:
- shutdown uses kset
- suspend uses dpm_list

--
regards,
-grygorii

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

[Grygorii Strashko]

On 04/25/2018 01:29 PM, Florian Fainelli wrote:

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:



On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.


OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.



But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
  brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?


The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.


Sry, but it still required some clarification :( - poweroff calls
device_shutdown() which, in turn, should not call .suspend(), so
how have you got both .shutdown() and .suspend() callbacks called during
poweroff? Am I missing smth?

Note. Suspend and shutdown uses different dev lists:
- shutdown uses kset
- suspend uses dpm_list

--
regards,
-grygorii

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

[Florian Fainelli]

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
> 
> 
> On 04/24/2018 05:58 PM, Florian Fainelli 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.
> 
> You can take a look at device_link_add() and Co.

OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.

> 
> But it's little bit unclear what exactly you have issue with:
> - shutdown
> - suspend
> 
> above are different (at least as it was before) and gpio-brcmstb.c
>  brcmstb_gpio_shutdown() should not be called as part of suspend !?
> may be you mean brcmstb_gpio_suspend?

The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.
--
Florian

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

[Florian Fainelli]

On 04/25/2018 11:06 AM, Grygorii Strashko wrote:
> 
> 
> On 04/24/2018 05:58 PM, Florian Fainelli 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.
> 
> You can take a look at device_link_add() and Co.

OK, though that requires a struct device references, so while I could
certainly resolve the device_node -> struct device that corresponds to
the GPIO provider , that poses a number of issues:

- not all struct device_node have a corresponding struct device
reference (e.g: clock providers, interrupt controllers, and possibly
other custom drivers), though in this case, they most likely do have one

- resolving a struct device associated with a struct device_node is
often done in a "bus" specific way, e.g: of_find_device_by_node(), so if
the GPIO provider is e.g: i2c_device, pci_device etc. etc. this might
not work that easily

I think this is what Dmitry just indicated in his email as well.

> 
> But it's little bit unclear what exactly you have issue with:
> - shutdown
> - suspend
> 
> above are different (at least as it was before) and gpio-brcmstb.c
>  brcmstb_gpio_shutdown() should not be called as part of suspend !?
> may be you mean brcmstb_gpio_suspend?

The issue exists with shutdown (through the use of "poweroff"), that is
confirmed, but I cannot see how it does not exist with any suspend state
as well, for the same reason that the ordering is not strictly enforced.
--
Florian

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

[Dmitry Torokhov]

On Wed, Apr 25, 2018 at 10:00:31AM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli  
> 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.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Wed, Apr 25, 2018 at 10:00:31AM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli  
> 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.

Thanks.

-- 
Dmitry

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

[Grygorii Strashko]

On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.

But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
 brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?



--
regards,
-grygorii

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

[Grygorii Strashko]

On 04/24/2018 05:58 PM, Florian Fainelli 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.


You can take a look at device_link_add() and Co.

But it's little bit unclear what exactly you have issue with:
- shutdown
- suspend

above are different (at least as it was before) and gpio-brcmstb.c
 brcmstb_gpio_shutdown() should not be called as part of suspend !?
may be you mean brcmstb_gpio_suspend?



--
regards,
-grygorii

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

[Rob Herring]

On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli  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).

Rob

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

[Rob Herring]

On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli  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).

Rob

Lack of suspend/resume/shutdown ordering between GPIO providers and consumers

[Florian Fainelli]

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.

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.

Do you have any idea how to best solve that?

Thanks!
-- 
Florian

Lack of suspend/resume/shutdown ordering between GPIO providers and consumers

[Florian Fainelli]

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.

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.

Do you have any idea how to best solve that?

Thanks!
-- 
Florian