From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932933AbdBVO5W (ORCPT ); Wed, 22 Feb 2017 09:57:22 -0500 Received: from mail-oi0-f68.google.com ([209.85.218.68]:33442 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932642AbdBVO5N (ORCPT ); Wed, 22 Feb 2017 09:57:13 -0500 MIME-Version: 1.0 In-Reply-To: References: <1487622809-25127-1-git-send-email-geert+renesas@glider.be> <1487622809-25127-5-git-send-email-geert+renesas@glider.be> <20170221174844.GE8605@leverpostej> From: "Rafael J. Wysocki" Date: Wed, 22 Feb 2017 15:57:11 +0100 X-Google-Sender-Auth: KhJh5ye6iQ1tNSjoLAM-RP_W0fU Message-ID: Subject: Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power To: Geert Uytterhoeven Cc: Mark Rutland , Geert Uytterhoeven , Lorenzo Pieralisi , Sudeep Holla , Lina Iyer , John Stultz , Thomas Gleixner , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Rob Herring , Magnus Damm , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux-Renesas , Linux PM list , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven wrote: > Hi Mark, > > On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland wrote: >> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote: >>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state) >>> static int psci_system_suspend_enter(suspend_state_t state) >>> { >>> switch (state) { >>> + case PM_SUSPEND_MEM: >>> + if (!psci_system_suspend_is_power_down || >>> + !wakeup_source_available()) >>> + return cpu_suspend(0, psci_system_suspend); >>> + /* fall through */ >> >> I don't believe that this is the correct place to handle this. >> >> The wakeup_source_available() check *might* be ok, though even with that >> I'd rather we rejected the request rather than trying to fall back to a >> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression. > > If we reject the request here, I think the PM core has to be modified to > try again using a shallower state. Note that it would be better to reject > the state in the .valid() callback instead of in .enter(). > > You also have to consider this is dynamic not static. > I.e. the availability of other wake-up sources may change at runtime (cfr. > the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls > which states are available) is initialized from suspend_set_ops(), and not > changed later. > > Perhaps pm_sleep_states[] should be updated every time the wakeup_sources > list is changed? No, the definitions of sleep states *are* static. They have to be, or user space won't know what sleep state it is asking for. And, as I said in my last reply to Sudeep, the list of possible wakeup devices for the given state is part of that definition. The sysfs "wakeup" interface is on top of that, not the other way around (which seems seems to be what you would want). Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Date: Wed, 22 Feb 2017 15:57:11 +0100 Message-ID: References: <1487622809-25127-1-git-send-email-geert+renesas@glider.be> <1487622809-25127-5-git-send-email-geert+renesas@glider.be> <20170221174844.GE8605@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Geert Uytterhoeven Cc: Mark Rutland , Geert Uytterhoeven , Lorenzo Pieralisi , Sudeep Holla , Lina Iyer , John Stultz , Thomas Gleixner , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Rob Herring , Magnus Damm , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Linux-Renesas , Linux PM list , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven wrote: > Hi Mark, > > On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland wrote: >> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote: >>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state) >>> static int psci_system_suspend_enter(suspend_state_t state) >>> { >>> switch (state) { >>> + case PM_SUSPEND_MEM: >>> + if (!psci_system_suspend_is_power_down || >>> + !wakeup_source_available()) >>> + return cpu_suspend(0, psci_system_suspend); >>> + /* fall through */ >> >> I don't believe that this is the correct place to handle this. >> >> The wakeup_source_available() check *might* be ok, though even with that >> I'd rather we rejected the request rather than trying to fall back to a >> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression. > > If we reject the request here, I think the PM core has to be modified to > try again using a shallower state. Note that it would be better to reject > the state in the .valid() callback instead of in .enter(). > > You also have to consider this is dynamic not static. > I.e. the availability of other wake-up sources may change at runtime (cfr. > the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls > which states are available) is initialized from suspend_set_ops(), and not > changed later. > > Perhaps pm_sleep_states[] should be updated every time the wakeup_sources > list is changed? No, the definitions of sleep states *are* static. They have to be, or user space won't know what sleep state it is asking for. And, as I said in my last reply to Sudeep, the list of possible wakeup devices for the given state is part of that definition. The sysfs "wakeup" interface is on top of that, not the other way around (which seems seems to be what you would want). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1487622809-25127-1-git-send-email-geert+renesas@glider.be> <1487622809-25127-5-git-send-email-geert+renesas@glider.be> <20170221174844.GE8605@leverpostej> From: "Rafael J. Wysocki" Date: Wed, 22 Feb 2017 15:57:11 +0100 Message-ID: Subject: Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power To: Geert Uytterhoeven Cc: Mark Rutland , Geert Uytterhoeven , Lorenzo Pieralisi , Sudeep Holla , Lina Iyer , John Stultz , Thomas Gleixner , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Rob Herring , Magnus Damm , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux-Renesas , Linux PM list , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven wrote: > Hi Mark, > > On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland wrote: >> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote: >>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state) >>> static int psci_system_suspend_enter(suspend_state_t state) >>> { >>> switch (state) { >>> + case PM_SUSPEND_MEM: >>> + if (!psci_system_suspend_is_power_down || >>> + !wakeup_source_available()) >>> + return cpu_suspend(0, psci_system_suspend); >>> + /* fall through */ >> >> I don't believe that this is the correct place to handle this. >> >> The wakeup_source_available() check *might* be ok, though even with that >> I'd rather we rejected the request rather than trying to fall back to a >> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression. > > If we reject the request here, I think the PM core has to be modified to > try again using a shallower state. Note that it would be better to reject > the state in the .valid() callback instead of in .enter(). > > You also have to consider this is dynamic not static. > I.e. the availability of other wake-up sources may change at runtime (cfr. > the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls > which states are available) is initialized from suspend_set_ops(), and not > changed later. > > Perhaps pm_sleep_states[] should be updated every time the wakeup_sources > list is changed? No, the definitions of sleep states *are* static. They have to be, or user space won't know what sleep state it is asking for. And, as I said in my last reply to Sudeep, the list of possible wakeup devices for the given state is part of that definition. The sysfs "wakeup" interface is on top of that, not the other way around (which seems seems to be what you would want). Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 From: rafael@kernel.org (Rafael J. Wysocki) Date: Wed, 22 Feb 2017 15:57:11 +0100 Subject: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power In-Reply-To: References: <1487622809-25127-1-git-send-email-geert+renesas@glider.be> <1487622809-25127-5-git-send-email-geert+renesas@glider.be> <20170221174844.GE8605@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven wrote: > Hi Mark, > > On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland wrote: >> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote: >>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state) >>> static int psci_system_suspend_enter(suspend_state_t state) >>> { >>> switch (state) { >>> + case PM_SUSPEND_MEM: >>> + if (!psci_system_suspend_is_power_down || >>> + !wakeup_source_available()) >>> + return cpu_suspend(0, psci_system_suspend); >>> + /* fall through */ >> >> I don't believe that this is the correct place to handle this. >> >> The wakeup_source_available() check *might* be ok, though even with that >> I'd rather we rejected the request rather than trying to fall back to a >> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression. > > If we reject the request here, I think the PM core has to be modified to > try again using a shallower state. Note that it would be better to reject > the state in the .valid() callback instead of in .enter(). > > You also have to consider this is dynamic not static. > I.e. the availability of other wake-up sources may change at runtime (cfr. > the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls > which states are available) is initialized from suspend_set_ops(), and not > changed later. > > Perhaps pm_sleep_states[] should be updated every time the wakeup_sources > list is changed? No, the definitions of sleep states *are* static. They have to be, or user space won't know what sleep state it is asking for. And, as I said in my last reply to Sudeep, the list of possible wakeup devices for the given state is part of that definition. The sysfs "wakeup" interface is on top of that, not the other way around (which seems seems to be what you would want). Thanks, Rafael