* Re: [PATCH] CPU C-state breakage with PM Qos change
From: mark gross @ 2012-02-05 3:50 UTC (permalink / raw)
To: Pihet-XID, Jean
Cc: Kevin Hilman, Venkatesh Pallipadi, markgross, linux-kernel,
Linux PM mailing list
In-Reply-To: <CAN5iSWo-n7OER4i1tPoeGBhY_RY-nX=7n4B3mFv1f0JK074bhQ@mail.gmail.com>
On Fri, Feb 03, 2012 at 03:04:43PM +0100, Pihet-XID, Jean wrote:
> Looping in linux-pm
>
> On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> > Looks like change "PM QoS: Move and rename the implementation files"
> > made pm_qos depend on CONFIG_PM which depends on
> > PM_SLEEP || PM_RUNTIME
> >
> > That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
> > to spend time in Polling loop idle instead of going into deep C-states,
> > consuming way way more power. This is with either acpi idle or intel idle
> > enabled.
> >
> > Either CONFIG_PM should be enabled with any pm_qos users or
> > the !CONFIG_PM pm_qos_request() should return sane defaults not to break
> > the existing users. Here's is the patch for the latter option.
> I think the real question is whether PM QoS should be functional in
> all cases (as is ACPI) or whether only if certain options are set
> (CONFIG_PM).
> In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
> provided as function stubs in order for the build to succeed.
>
> Rafael, Mark,
> What do you think? Should PM QoS be enabled in all cases? Are there
> any known dependencies with CONFIG_PM?
Yes I do think pm_qos interfaces should be enabled all the time and be
independent of CONFIG_PM. Also, I still am not a fan of the renaming
patch but, as the argument for and against renaming cannot be based on
quantifiable things I've chosen not to let it bother me.
I think Venki's change is a band aid and we should fix it right by not
having a dependency on config_pm for the interface to behave.
I'll take a look at why there is now a dependency before I have more to
say.
--mark
^ permalink raw reply
* Re: [PATCH 2/7] tboot: Add return values for tboot_sleep
From: Konrad Rzeszutek Wilk @ 2012-02-03 20:05 UTC (permalink / raw)
To: Cihula, Joseph
Cc: Brown, Len, x86@kernel.org, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org, tboot-devel@lists.sourceforge.net,
liang.tang@oracle.com, hpa@zytor.com,
linux-pm@lists.linux-foundation.org
In-Reply-To: <9F57BF860713DF4BA3EFA4F8C6DFEDAC2D6870EE@ORSMSX101.amr.corp.intel.com>
On Thu, Jan 26, 2012 at 12:16:39AM +0000, Cihula, Joseph wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > Sent: Thursday, January 12, 2012 12:23 PM
> eturn values for tboot_sleep
> >
> > On Thu, Jan 12, 2012 at 09:49:58AM -0800, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Jan 10, 2012 at 08:10:53PM +0000, Cihula, Joseph wrote:
> > > > ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which you
> > are not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its
> > errors through tboot_sleep().
> > >
> > > Hey Joe,
> > >
> > > Thanks for looking at the patches.
> > >
> > > Right now [with this patch applied] the code looks as so:
> > >
> > > 297
> > > 298 tboot_shutdown(acpi_shutdown_map[sleep_state]);
> > > 299 return 0;
> > > 300 }
> > >
> > >
> > > If we do make tboot_shutdown() return an int, there will be a need to
> > > modify other callers: native_machine_emergency_restart,
> > > native_machine_halt, native_machine_power_off, and native_play_dead as well.
> > >
> > > Perhaps that could be done in another patch and leave this one as is
> > > where the 'tboot_sleep' would just return 0 instead of 'return
> > > tboot_shutdown(...)' ?
> > >
> > > Perhaps another way to do this is to extract the common code of
> > > tboot_sleep and tboot_shutdown?
>
> I'd be happier to see the callers above changed to handle a tboot_shutdown() that returned an error than splitting this up and only checking the error in one path.
OK, so something like this then:
>From da60a441fd62aee83763758455c08c281893ae93 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 8 Dec 2011 17:14:08 +0800
Subject: [PATCH] tboot: Add return values for tboot_sleep and consequently
tboot_shutdown
.. as appropiately. As tboot_sleep now returns values.
remove tboot_sleep_wrapper.
Suggested-by: "Rafael J. Wysocki" <rjw@sisk.pl>
Acked-by: Joseph Cihula <joseph.cihula@intel.com>
[v1: Return -1/0/+1 instead of ACPI_xx values]
[v2: Also make tboot_shutdown return error code]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
arch/x86/kernel/reboot.c | 6 +++---
arch/x86/kernel/smpboot.c | 2 +-
arch/x86/kernel/tboot.c | 33 +++++++++++++++------------------
include/linux/tboot.h | 4 ++--
4 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 37a458b..b29326a 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -549,7 +549,7 @@ static void native_machine_emergency_restart(void)
if (reboot_emergency)
emergency_vmx_disable_all();
- tboot_shutdown(TB_SHUTDOWN_REBOOT);
+ WARN_ON(tboot_shutdown(TB_SHUTDOWN_REBOOT));
/* Tell the BIOS if we want cold or warm reboot */
*((unsigned short *)__va(0x472)) = reboot_mode;
@@ -684,7 +684,7 @@ static void native_machine_halt(void)
/* stop other cpus and apics */
machine_shutdown();
- tboot_shutdown(TB_SHUTDOWN_HALT);
+ WARN_ON(tboot_shutdown(TB_SHUTDOWN_HALT));
/* stop this cpu */
stop_this_cpu(NULL);
@@ -698,7 +698,7 @@ static void native_machine_power_off(void)
pm_power_off();
}
/* a fallback in case there is no PM info available */
- tboot_shutdown(TB_SHUTDOWN_HALT);
+ WARN_ON(tboot_shutdown(TB_SHUTDOWN_HALT));
}
struct machine_ops machine_ops = {
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 66d250c..9aa903a 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1419,7 +1419,7 @@ static inline void hlt_play_dead(void)
void native_play_dead(void)
{
play_dead_common();
- tboot_shutdown(TB_SHUTDOWN_WFS);
+ WARN_ON(tboot_shutdown(TB_SHUTDOWN_WFS));
mwait_play_dead(); /* Only returns on failure */
hlt_play_dead();
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 1a4ab7d..9504a7a 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -217,12 +217,12 @@ static int tboot_setup_sleep(void)
#endif
-void tboot_shutdown(u32 shutdown_type)
+int tboot_shutdown(u32 shutdown_type)
{
void (*shutdown)(void);
if (!tboot_enabled())
- return;
+ return 0;
/*
* if we're being called before the 1:1 mapping is set up then just
@@ -230,13 +230,14 @@ void tboot_shutdown(u32 shutdown_type)
* due to very early panic()
*/
if (!tboot_pg_dir)
- return;
+ return 0;
/* if this is S3 then set regions to MAC */
- if (shutdown_type == TB_SHUTDOWN_S3)
- if (tboot_setup_sleep())
- return;
-
+ if (shutdown_type == TB_SHUTDOWN_S3) {
+ int rc = tboot_setup_sleep();
+ if (rc)
+ return rc;
+ }
tboot->shutdown_type = shutdown_type;
switch_to_tboot_pt();
@@ -247,6 +248,8 @@ void tboot_shutdown(u32 shutdown_type)
/* should not reach here */
while (1)
halt();
+
+ return 0;
}
static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
@@ -272,7 +275,7 @@ static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
offsetof(struct acpi_table_facs, firmware_waking_vector);
}
-void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
+static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
{
static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
/* S0,1,2: */ -1, -1, -1,
@@ -281,7 +284,7 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
/* S5: */ TB_SHUTDOWN_S5 };
if (!tboot_enabled())
- return;
+ return 0;
tboot_copy_fadt(&acpi_gbl_FADT);
tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
@@ -292,16 +295,10 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
if (sleep_state >= ACPI_S_STATE_COUNT ||
acpi_shutdown_map[sleep_state] == -1) {
pr_warning("unsupported sleep state 0x%x\n", sleep_state);
- return;
+ return -1;
}
- tboot_shutdown(acpi_shutdown_map[sleep_state]);
-}
-static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
- u32 pm1b_control)
-{
- tboot_sleep(sleep_state, pm1a_control, pm1b_control);
- return 0;
+ return tboot_shutdown(acpi_shutdown_map[sleep_state]);
}
static atomic_t ap_wfs_count;
@@ -352,7 +349,7 @@ static __init int tboot_late_init(void)
atomic_set(&ap_wfs_count, 0);
register_hotcpu_notifier(&tboot_cpu_notifier);
- acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
+ acpi_os_set_prepare_sleep(&tboot_sleep);
return 0;
}
diff --git a/include/linux/tboot.h b/include/linux/tboot.h
index c75128b..8f14fe0 100644
--- a/include/linux/tboot.h
+++ b/include/linux/tboot.h
@@ -142,7 +142,7 @@ static inline int tboot_enabled(void)
}
extern void tboot_probe(void);
-extern void tboot_shutdown(u32 shutdown_type);
+extern int tboot_shutdown(u32 shutdown_type);
extern struct acpi_table_header *tboot_get_dmar_table(
struct acpi_table_header *dmar_tbl);
extern int tboot_force_iommu(void);
@@ -151,7 +151,7 @@ extern int tboot_force_iommu(void);
#define tboot_enabled() 0
#define tboot_probe() do { } while (0)
-#define tboot_shutdown(shutdown_type) do { } while (0)
+#define tboot_shutdown(shutdown_type) 0
#define tboot_sleep(sleep_state, pm1a_control, pm1b_control) \
do { } while (0)
#define tboot_get_dmar_table(dmar_tbl) (dmar_tbl)
--
1.7.7.5
^ permalink raw reply related
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Rafael J. Wysocki @ 2012-02-03 20:02 UTC (permalink / raw)
To: Pihet-XID, Jean
Cc: Kevin Hilman, Venkatesh Pallipadi, markgross, linux-kernel,
Linux PM mailing list
In-Reply-To: <CAN5iSWo-n7OER4i1tPoeGBhY_RY-nX=7n4B3mFv1f0JK074bhQ@mail.gmail.com>
On Friday, February 03, 2012, Pihet-XID, Jean wrote:
> Looping in linux-pm
>
> On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> > Looks like change "PM QoS: Move and rename the implementation files"
> > made pm_qos depend on CONFIG_PM which depends on
> > PM_SLEEP || PM_RUNTIME
> >
> > That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
> > to spend time in Polling loop idle instead of going into deep C-states,
> > consuming way way more power. This is with either acpi idle or intel idle
> > enabled.
> >
> > Either CONFIG_PM should be enabled with any pm_qos users or
> > the !CONFIG_PM pm_qos_request() should return sane defaults not to break
> > the existing users. Here's is the patch for the latter option.
> I think the real question is whether PM QoS should be functional in
> all cases (as is ACPI) or whether only if certain options are set
> (CONFIG_PM).
> In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
> provided as function stubs in order for the build to succeed.
>
> Rafael, Mark,
> What do you think? Should PM QoS be enabled in all cases? Are there
> any known dependencies with CONFIG_PM?
At least we should keep the current behavior to avoid breaking things
for now. We can change that in the next cycle, however, if everyone
agrees, but more carefully.
The patch has been applied to linux-pm/linux-next and will be pushed to Linus
early next week.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] CPU C-state breakage with PM Qos change
From: Pihet-XID, Jean @ 2012-02-03 14:04 UTC (permalink / raw)
To: Venkatesh Pallipadi, Rafael J. Wysocki, markgross
Cc: Kevin Hilman, Linux PM mailing list, linux-kernel
In-Reply-To: <1328228079-20716-1-git-send-email-venki@google.com>
Looping in linux-pm
On Fri, Feb 3, 2012 at 1:14 AM, Venkatesh Pallipadi <venki@google.com> wrote:
> Looks like change "PM QoS: Move and rename the implementation files"
> made pm_qos depend on CONFIG_PM which depends on
> PM_SLEEP || PM_RUNTIME
>
> That breaks CPU C-states with kernels not having these CONFIGs, causing CPUs
> to spend time in Polling loop idle instead of going into deep C-states,
> consuming way way more power. This is with either acpi idle or intel idle
> enabled.
>
> Either CONFIG_PM should be enabled with any pm_qos users or
> the !CONFIG_PM pm_qos_request() should return sane defaults not to break
> the existing users. Here's is the patch for the latter option.
I think the real question is whether PM QoS should be functional in
all cases (as is ACPI) or whether only if certain options are set
(CONFIG_PM).
In the current code if CONFIG_PM is not enabled, a dummy PM QoS API is
provided as function stubs in order for the build to succeed.
Rafael, Mark,
What do you think? Should PM QoS be enabled in all cases? Are there
any known dependencies with CONFIG_PM?
Regards,
Jean
^ permalink raw reply
* Re: [RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework
From: Amit Kachhap @ 2012-02-03 12:13 UTC (permalink / raw)
To: Zhang Rui
Cc: linux-pm, linux-kernel, mjg59, linux-acpi, lenb, linaro-dev,
patches
In-Reply-To: <1328253147.20438.19.camel@rui.sh.intel.com>
On 3 February 2012 12:42, Zhang Rui <rui.zhang@intel.com> wrote:
> Hi, sorry for the late response.
>
> On 四, 2012-01-19 at 14:47 +0530, Amit Kachhap wrote:
>> On 13 December 2011 20:43, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>> > PATCH 1) [thermal: Add a new trip type to use cooling device instance number]
>> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
>> > cooling device instance number and may be helpful for cpufreq cooling devices
>> > to take the correct cooling action.
>> >
> Sorry, I'm still not quite clear about how this will be used.
> Say, processor has P0~P3, then we need to register a thermal zone with
> three STATE_ACTIVE trip points, like the picture shown below?
>
> processor in full speed, i.e. P0
> ----------------------------
> state active trip point 1
> ----------------------------
> processor in P1
> ----------------------------
> state active trip point 2
> ----------------------------
> processor in P2
> ----------------------------
> state active trip point 3
> ----------------------------
> processor in P3
>
Thanks for the review. Your representation of the 3 PSTATES and trip
points are correct. Also remember the case that we have registered one
cpufreq cooling device and binded 3 instance of it with the 3 trip
points. So in the current patch set_cur_state will pass the instance
id also which will help in moving to the current P state.
This same behaviour is achieved through PASSIVE state but it iterates
through all the P states. so the current patch avoids the iteration
and goes to a single state directly.
>> > PATCH 2) [thermal: Add generic cpu cooling implementation]
>> > This patch adds generic cpu cooling low level implementation through frequency
>> > clipping and cpu hotplug. In future, other cpu related cooling devices may be
>> > added here. An ACPI version of this already exists(drivers/acpi/processor_thermal.c).
>> > But this will be useful for platforms like ARM using the generic thermal interface
>> > along with the generic cpu cooling devices. The cooling device registration API's
>> > return cooling device pointers which can be easily binded with the thermal zone
>> > trip points.
>> >
> It seems that we can convert the ACPI processor thermal driver to follow
> this generic cpu cooling implementation, right?
I am not sure if we can convert the processor thermal driver because
it performs other cpu cooling like throttling, idle etc and then ACPI
abstraction on top of it. I am thinking of exporting the generic low
level cooling implementation like this patch is doing from
processor_thermal.c file so that it can be usable with non-acpi
interface.
What is your opinion about it?
Thanks,
Amit Daniel
>
> thanks,
> rui
>>
>> Any comments on these patches? I submitted them quite long back.
>>
>> Regards,
>> Amit D
>>
>> >
>> > Amit Daniel Kachhap (2):
>> > thermal: Add a new trip type to use cooling device instance number
>> > thermal: Add generic cpu cooling implementation
>> >
>> > Documentation/thermal/cpu-cooling-api.txt | 52 +++++
>> > Documentation/thermal/sysfs-api.txt | 4 +-
>> > drivers/thermal/Kconfig | 11 +
>> > drivers/thermal/Makefile | 1 +
>> > drivers/thermal/cpu_cooling.c | 302 +++++++++++++++++++++++++++++
>> > drivers/thermal/thermal_sys.c | 27 +++-
>> > include/linux/cpu_cooling.h | 45 +++++
>> > include/linux/thermal.h | 1 +
>> > 8 files changed, 440 insertions(+), 3 deletions(-)
>> > create mode 100644 Documentation/thermal/cpu-cooling-api.txt
>> > create mode 100644 drivers/thermal/cpu_cooling.c
>> > create mode 100644 include/linux/cpu_cooling.h
>> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [Patch] JBD and JBD2 missing set_freezable()
From: Nigel Cunningham @ 2012-02-03 8:59 UTC (permalink / raw)
To: Linux PM mailing list
Hi all.
With the latest and greatest changes to the freezer, I started seeing
panics that were caused by jbd2 running post-process freezing and
hitting the canary BUG_ON for non-TuxOnIce I/O submission. I've traced
this back to a lack of set_freezable calls in both jbd and jbd2. Since
they're clearly meant to be frozen (there are tests for freezing()), I
submit the following patch to add the missing calls.
Signed-off-by: Nigel Cunningham <nigel@tuxonice.net>
diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index 59c09f9..89cd985 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -129,6 +129,8 @@ static int kjournald(void *arg)
setup_timer(&journal->j_commit_timer, commit_timeout,
(unsigned long)current);
+ set_freezable();
+
/* Record that the journal thread is running */
journal->j_task = current;
wake_up(&journal->j_wait_done_commit);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index c0a5f9f..663e47c 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -139,6 +139,8 @@ static int kjournald2(void *arg)
setup_timer(&journal->j_commit_timer, commit_timeout,
(unsigned long)current);
+ set_freezable();
+
/* Record that the journal thread is running */
journal->j_task = current;
wake_up(&journal->j_wait_done_commit);
--
Evolution (n): A hypothetical process whereby improbable
events occur with alarming frequency, order arises from chaos, and
no one is given credit.
^ permalink raw reply related
* Re: [RFC PATCH 0/2] thermal: Add generic cpu cooling devices according to thermal framework
From: Zhang Rui @ 2012-02-03 7:12 UTC (permalink / raw)
To: Amit Kachhap
Cc: linux-pm, linux-kernel, mjg59, linux-acpi, lenb, linaro-dev,
patches
In-Reply-To: <CAK44p22LmkGVo0W5eoT5Ms57pkP7PbWoxNS2c8bnx=1FhBvamA@mail.gmail.com>
Hi, sorry for the late response.
On 四, 2012-01-19 at 14:47 +0530, Amit Kachhap wrote:
> On 13 December 2011 20:43, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> > PATCH 1) [thermal: Add a new trip type to use cooling device instance number]
> > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE which passes
> > cooling device instance number and may be helpful for cpufreq cooling devices
> > to take the correct cooling action.
> >
Sorry, I'm still not quite clear about how this will be used.
Say, processor has P0~P3, then we need to register a thermal zone with
three STATE_ACTIVE trip points, like the picture shown below?
processor in full speed, i.e. P0
----------------------------
state active trip point 1
----------------------------
processor in P1
----------------------------
state active trip point 2
----------------------------
processor in P2
----------------------------
state active trip point 3
----------------------------
processor in P3
> > PATCH 2) [thermal: Add generic cpu cooling implementation]
> > This patch adds generic cpu cooling low level implementation through frequency
> > clipping and cpu hotplug. In future, other cpu related cooling devices may be
> > added here. An ACPI version of this already exists(drivers/acpi/processor_thermal.c).
> > But this will be useful for platforms like ARM using the generic thermal interface
> > along with the generic cpu cooling devices. The cooling device registration API's
> > return cooling device pointers which can be easily binded with the thermal zone
> > trip points.
> >
It seems that we can convert the ACPI processor thermal driver to follow
this generic cpu cooling implementation, right?
thanks,
rui
>
> Any comments on these patches? I submitted them quite long back.
>
> Regards,
> Amit D
>
> >
> > Amit Daniel Kachhap (2):
> > thermal: Add a new trip type to use cooling device instance number
> > thermal: Add generic cpu cooling implementation
> >
> > Documentation/thermal/cpu-cooling-api.txt | 52 +++++
> > Documentation/thermal/sysfs-api.txt | 4 +-
> > drivers/thermal/Kconfig | 11 +
> > drivers/thermal/Makefile | 1 +
> > drivers/thermal/cpu_cooling.c | 302 +++++++++++++++++++++++++++++
> > drivers/thermal/thermal_sys.c | 27 +++-
> > include/linux/cpu_cooling.h | 45 +++++
> > include/linux/thermal.h | 1 +
> > 8 files changed, 440 insertions(+), 3 deletions(-)
> > create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> > create mode 100644 drivers/thermal/cpu_cooling.c
> > create mode 100644 include/linux/cpu_cooling.h
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-02-03 1:19 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Kevin Hilman, Len Brown, linux-kernel@vger.kernel.org,
Amit Kucheria, linux-tegra@vger.kernel.org,
linux-pm@lists.linux-foundation.org, linux-omap@vger.kernel.org,
Arjan van de Ven, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20120201180705.GA20936@e102568-lin.cambridge.arm.com>
On Wed, Feb 1, 2012 at 10:07 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 01, 2012 at 05:30:15PM +0000, Colin Cross wrote:
>> On Wed, Feb 1, 2012 at 6:59 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Feb 01, 2012 at 12:13:26PM +0000, Vincent Guittot wrote:
>> >
>> > [...]
>> >
>> >> >> In your patch, you put in safe state (WFI for most of platform) the
>> >> >> cpus that become idle and these cpus are woken up each time a new cpu
>> >> >> of the cluster becomes idle. Then, the cluster state is chosen and the
>> >> >> cpus enter the selected C-state. On ux500, we are using another
>> >> >> behavior for synchronizing the cpus. The cpus are prepared to enter
>> >> >> the c-state that has been chosen by the governor and the last cpu,
>> >> >> that enters idle, chooses the final cluster state (according to cpus'
>> >> >> C-state). The main advantage of this solution is that you don't need
>> >> >> to wake other cpus to enter the C-state of a cluster. This can be
>> >> >> quite worth full when tasks mainly run on one cpu. Have you also think
>> >> >> about such behavior when developing the coupled cpuidle driver ? It
>> >> >> could be interesting to add such behavior.
>> >> >
>> >> > Waking up the cpus that are in the safe state is not done just to
>> >> > choose the target state, it's done to allow the cpus to take
>> >> > themselves to the target low power state. On ux500, are you saying
>> >> > you take the cpus directly from the safe state to a lower power state
>> >> > without ever going back to the active state? I once implemented Tegra
>> >>
>> >> yes it is
>> >
>> > But if there is a single power rail for the entire cluster, when a CPU
>> > is "prepared" for shutdown this means that you have to save the context and
>> > clean L1, maybe for nothing since if other CPUs are up and running the
>> > CPU going idle can just enter a simple standby wfi (clock-gated but power on).
>> >
>> > With Colin's approach, context is saved and L1 cleaned only when it is
>> > almost certain the cluster is powered off (so the CPUs).
>> >
>> > It is a trade-off, I am not saying one approach is better than the
>> > other; we just have to make sure that preparing the CPU for "possible" shutdown
>> > is better than sending IPIs to take CPUs out of wfi and synchronize
>> > them (this happens if and only if CPUs enter coupled C-states).
>> >
>> > As usual this will depend on use cases (and silicon implementations :) )
>> >
>> > It is definitely worth benchmarking them.
>> >
>>
>> I'm less worried about performance, and more worried about race
>> conditions. How do you deal with the following situation:
>> CPU0 goes to WFI, and saves its state
>> CPU1 goes idle, and selects a deep idle state that powers down CPU0
>> CPU1 saves is state, and is about to trigger the power down
>> CPU0 gets an interrupt, restores its state, and modifies state (maybe
>> takes a spinlock during boot)
>> CPU1 cuts the power to CPU0
>>
>> On OMAP4, the race is handled in hardware. When CPU1 tries to cut the
>> power to the blocks shared by CPU0 the hardware will ignore the
>> request if CPU0 is not in WFI. On Tegra2, there is no hardware
>> support and I had to handle it with a spinlock implemented in scratch
>> registers because CPU0 is out of coherency when it starts booting and
>> ldrex/strex don't work. I'm not convinced my implementation is
>> correct, and I'd be curious to see any other implementations.
>
> That's a problem you solved with coupled C-states (ie your example in
> the cover letter), where the primary waits for other CPUs to be reset
> before issuing the power down command, right ? At that point in time
> secondaries cannot wake up (?) and if wfi (ie power down) aborts you just
> take the secondaries out of reset and restart executing simultaneously,
> correct ? It mirrors the suspend behaviour, which is easier to deal with
> than completely random idle paths.
Yes, anything that supports hotplug and suspend should support coupled
cpuidle states fairly easily. The only thing required that is not
already used by hotplug/suspend is the ability to save and restore
context on cpu1, but most implementations end up doing that already.
> It is true that this should be managed by the PM HW; if HW is not
> capable of managing these situations things get nasty as you highlighted.
Yes - on some platforms, the HW is not designed to handle it. On
others, it is designed to, but due to HW bugs it cannot be used.
> And it is also true ldrex/strex on cacheable memory might not be available in
> those early warm-boot stages. I came up with a locking algorithm on
> strongly ordered memory to deal with that, but I am still not sure it is
> something we really really need.
I did the same, but with device memory.
> I will test coupled C-state code ASAP, and come back with feedback.
>
> Thanks,
> Lorenzo
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
From: Amit Kachhap @ 2012-02-02 9:16 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm
In-Reply-To: <20120201145707.GD30184@srcf.ucam.org>
On 1 February 2012 20:27, Matthew Garrett <mjg@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling and cpuhotplugg currently.
>
> We've been over this kind of thing in the past. cpu hotplug is a
> relatively expensive operation, so people have previously been
> enthusiastic about using the scheduler to simply avoid running anything
> on CPUs if they're overheating. Has any general consensus been reached
> on this?
yes you are right that cpuhotplug is an expensive process and it may
further heat up the system before turning off so the ideal way would
be to reduce the capacity of the cpu gradually. Anyway these patches
are only exporting those API's and the actual use of them depends on
the user. Although my bigger focus is on cpufreq as cooling devices so
I might remove cpuhotplug in the next version.
>
> I'm also not entirely thrilled at now having two ways to manage the cpu
> through the thermal layer. ACPI already plugs in via the passive trip
> points. If we're going to do this then I'd like to see the ACPI code
> merged in with the generic cpu cooling code.
Yeah I also agree that there is a kind of repetition and not entirely
sure where to place these codes. I will try adding them inside acpi.
Thanks for the suggestion.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number
From: Amit Kachhap @ 2012-02-02 7:16 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm
In-Reply-To: <20120201144931.GC30184@srcf.ucam.org>
On 1 February 2012 20:19, Matthew Garrett <mjg@redhat.com> wrote:
>
> I'm not really a fan of this as it stands - the name isn't very
> intuitive and the code's pretty difficult to read. Would the following
> (incomplete and obviously untested) not have the effect you want? Then
> you register multiple trip points with the same cooling device but
> different private values, and the state set does whatever you want it
> to. Or am I misunderstanding the problem you're trying to solve?
Thanks for the detailed review of the patch. Actually i tried to merge
the benefits of trip type ACTIVE and PASSIVE into one so this name.
This new trip type is just like ACTIVE but instead of OFF(0)/ON(1)
all values greater then 0 is on. Anyways I looked at your
implementation below but this will not solve the purpose as
thermal_cooling_device_register() need to be be called only once to
register a cooling device such cpu frequency based. However the same
cooling device may be binded many times to different trips.
Say,
1) thermal_zone_bind_cooling_device(tz_dev, 0, cdev);
2) thermal_zone_bind_cooling_device(tz_dev, 1, cdev);
3) thermal_zone_bind_cooling_device(tz_dev, 2, cdev);
0,1, 2 are nothing but trip points so the set_cur_state should be called like
set_cur_state(cdev, 0)
set_cur_state(cdev, 1)
set_cur_state(cdev, 2) when the trip point threshold are crossed.
Yeah I agree that implementation logic looks complex but this to
prevent the lower temp trip points cooling handlers to be called. I
will surely make this better in next version.
>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 220ce7e..817f2ba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
> char attr_name[THERMAL_NAME_LENGTH];
> struct device_attribute attr;
> struct list_head node;
> + unsigned long private;
> };
>
> static DEFINE_IDR(thermal_tz_idr);
> @@ -909,7 +910,8 @@ static struct class thermal_class = {
> * @ops: standard thermal cooling devices callbacks.
> */
> struct thermal_cooling_device *thermal_cooling_device_register(
> - char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
> + char *type, void *devdata, const struct thermal_cooling_device_ops *ops,
> + unsigned long private)
> {
> struct thermal_cooling_device *cdev;
> struct thermal_zone_device *pos;
> @@ -936,6 +938,7 @@ struct thermal_cooling_device *thermal_cooling_device_register(
> cdev->ops = ops;
> cdev->device.class = &thermal_class;
> cdev->devdata = devdata;
> + cdev->private = private;
> dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> result = device_register(&cdev->device);
> if (result) {
> @@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> continue;
>
> cdev = instance->cdev;
> -
> - if (temp >= trip_temp)
> - cdev->ops->set_cur_state(cdev, 1);
> - else
> - cdev->ops->set_cur_state(cdev, 0);
> + if (cdev->private) {
> + cdev->ops->set_cur_state(cdev, cdev->private);
> + } else {
> + if (temp >= trip_temp)
> + cdev->ops->set_cur_state(cdev, 1);
> + else
> + cdev->ops->set_cur_state(cdev, 0);
> + }
> }
> break;
> case THERMAL_TRIP_PASSIVE:
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..04aac09 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
> struct thermal_cooling_device *);
> void thermal_zone_device_update(struct thermal_zone_device *);
> struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
> - const struct thermal_cooling_device_ops *);
> + const struct thermal_cooling_device_ops *, unsigned long private);
> void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>
> #ifdef CONFIG_NET
>
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number
From: Amit Kachhap @ 2012-02-02 7:14 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linaro-dev, patches, linux-kernel, linux-acpi, linux-pm
In-Reply-To: <20120201144931.GC30184@srcf.ucam.org>
[-- Attachment #1.1: Type: text/plain, Size: 4626 bytes --]
On 1 February 2012 20:19, Matthew Garrett <mjg@redhat.com> wrote:
> I'm not really a fan of this as it stands - the name isn't very
> intuitive and the code's pretty difficult to read. Would the following
> (incomplete and obviously untested) not have the effect you want? Then
> you register multiple trip points with the same cooling device but
> different private values, and the state set does whatever you want it
> to. Or am I misunderstanding the problem you're trying to solve?
>
Thanks for the detailed review of the patch. Actually i tried to merge the
benefits of trip type ACTIVE and PASSIVE into one so this name. This new
trip type is just like ACTIVE but instead of OFF(0)/ON(1) all values
greater then 0 is on. Anyways I looked at your implementation below but
this will not solve the purpose as thermal_cooling_device_register() need
to be be called only once to register a cooling device such cpu frequency
based. However the same cooling device may be binded many times to
different trips.
Say,
1) thermal_zone_bind_cooling_device(tz_dev, 0, cdev);
2) thermal_zone_bind_cooling_device(tz_dev, 1, cdev);
3) thermal_zone_bind_cooling_device(tz_dev, 2, cdev);
0,1, 2 are nothing but trip points so the set_cur_state should be called
like
set_cur_state(cdev, 0)
set_cur_state(cdev, 1)
set_cur_state(cdev, 2) when the trip point threshold are crossed.
Yeah I agree that implementation logic looks complex but this to prevent
the lower temp trip points cooling handlers to be called. I will surely
make this better in next version.
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 220ce7e..817f2ba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
> char attr_name[THERMAL_NAME_LENGTH];
> struct device_attribute attr;
> struct list_head node;
> + unsigned long private;
> };
>
> static DEFINE_IDR(thermal_tz_idr);
> @@ -909,7 +910,8 @@ static struct class thermal_class = {
> * @ops: standard thermal cooling devices callbacks.
> */
> struct thermal_cooling_device *thermal_cooling_device_register(
> - char *type, void *devdata, const struct thermal_cooling_device_ops
> *ops)
> + char *type, void *devdata, const struct thermal_cooling_device_ops
> *ops,
> + unsigned long private)
> {
> struct thermal_cooling_device *cdev;
> struct thermal_zone_device *pos;
> @@ -936,6 +938,7 @@ struct thermal_cooling_device
> *thermal_cooling_device_register(
> cdev->ops = ops;
> cdev->device.class = &thermal_class;
> cdev->devdata = devdata;
> + cdev->private = private;
> dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> result = device_register(&cdev->device);
> if (result) {
> @@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct
> thermal_zone_device *tz)
> continue;
>
> cdev = instance->cdev;
> -
> - if (temp >= trip_temp)
> - cdev->ops->set_cur_state(cdev, 1);
> - else
> - cdev->ops->set_cur_state(cdev, 0);
> + if (cdev->private) {
> + cdev->ops->set_cur_state(cdev,
> cdev->private);
> + } else {
> + if (temp >= trip_temp)
> +
> cdev->ops->set_cur_state(cdev, 1);
> + else
> +
> cdev->ops->set_cur_state(cdev, 0);
> + }
> }
> break;
> case THERMAL_TRIP_PASSIVE:
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..04aac09 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct
> thermal_zone_device *, int,
> struct thermal_cooling_device *);
> void thermal_zone_device_update(struct thermal_zone_device *);
> struct thermal_cooling_device *thermal_cooling_device_register(char *,
> void *,
> - const struct thermal_cooling_device_ops *);
> + const struct thermal_cooling_device_ops *, unsigned
> long private);
> void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>
> #ifdef CONFIG_NET
>
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org
>
[-- Attachment #1.2: Type: text/html, Size: 5588 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/8] RFC: CPU frequency min/max as PM QoS params
From: Antti P Miettinen @ 2012-02-02 6:06 UTC (permalink / raw)
To: linux-pm; +Cc: cpufreq
In-Reply-To: <201201230043.46436.rjw@sisk.pl>
"Rafael J. Wysocki" <rjw@sisk.pl> writes:
> On Sunday, January 22, 2012, Antti P Miettinen wrote:
[..]
>> Seems that the device specific constraints are not yet in use in
>> 3.3-rc1, or am I not looking hard enough?
>
> They are in use through generic PM domains (drivers/base/power/domain*.c
> and friends) and ARM/shmobile uses those.
>
> Thanks,
> Rafael
Sorry for the delay - got pre-empted by other stuff. I took a look at
the per device constraints. Do I understand it correctly that the idea
is that there is only one constraint per device? If we want to make
frequency and latency per CPU I guess we'd need separate constraints
associated with the CPU device. Or do I misunderstand something?
Or would global CPU frequency be more in line with global CPU latency
after all?
--Antti
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Lorenzo Pieralisi @ 2012-02-01 18:07 UTC (permalink / raw)
To: Colin Cross
Cc: Kevin Hilman, Len Brown, linux-kernel@vger.kernel.org,
Amit Kucheria, linux-tegra@vger.kernel.org,
linux-pm@lists.linux-foundation.org, linux-omap@vger.kernel.org,
Arjan van de Ven, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAMbhsRTFzc8aGsSGUs2==4FiqBb7zOit4cO=6U698k+RCE+kqQ@mail.gmail.com>
On Wed, Feb 01, 2012 at 05:30:15PM +0000, Colin Cross wrote:
> On Wed, Feb 1, 2012 at 6:59 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Feb 01, 2012 at 12:13:26PM +0000, Vincent Guittot wrote:
> >
> > [...]
> >
> >> >> In your patch, you put in safe state (WFI for most of platform) the
> >> >> cpus that become idle and these cpus are woken up each time a new cpu
> >> >> of the cluster becomes idle. Then, the cluster state is chosen and the
> >> >> cpus enter the selected C-state. On ux500, we are using another
> >> >> behavior for synchronizing the cpus. The cpus are prepared to enter
> >> >> the c-state that has been chosen by the governor and the last cpu,
> >> >> that enters idle, chooses the final cluster state (according to cpus'
> >> >> C-state). The main advantage of this solution is that you don't need
> >> >> to wake other cpus to enter the C-state of a cluster. This can be
> >> >> quite worth full when tasks mainly run on one cpu. Have you also think
> >> >> about such behavior when developing the coupled cpuidle driver ? It
> >> >> could be interesting to add such behavior.
> >> >
> >> > Waking up the cpus that are in the safe state is not done just to
> >> > choose the target state, it's done to allow the cpus to take
> >> > themselves to the target low power state. On ux500, are you saying
> >> > you take the cpus directly from the safe state to a lower power state
> >> > without ever going back to the active state? I once implemented Tegra
> >>
> >> yes it is
> >
> > But if there is a single power rail for the entire cluster, when a CPU
> > is "prepared" for shutdown this means that you have to save the context and
> > clean L1, maybe for nothing since if other CPUs are up and running the
> > CPU going idle can just enter a simple standby wfi (clock-gated but power on).
> >
> > With Colin's approach, context is saved and L1 cleaned only when it is
> > almost certain the cluster is powered off (so the CPUs).
> >
> > It is a trade-off, I am not saying one approach is better than the
> > other; we just have to make sure that preparing the CPU for "possible" shutdown
> > is better than sending IPIs to take CPUs out of wfi and synchronize
> > them (this happens if and only if CPUs enter coupled C-states).
> >
> > As usual this will depend on use cases (and silicon implementations :) )
> >
> > It is definitely worth benchmarking them.
> >
>
> I'm less worried about performance, and more worried about race
> conditions. How do you deal with the following situation:
> CPU0 goes to WFI, and saves its state
> CPU1 goes idle, and selects a deep idle state that powers down CPU0
> CPU1 saves is state, and is about to trigger the power down
> CPU0 gets an interrupt, restores its state, and modifies state (maybe
> takes a spinlock during boot)
> CPU1 cuts the power to CPU0
>
> On OMAP4, the race is handled in hardware. When CPU1 tries to cut the
> power to the blocks shared by CPU0 the hardware will ignore the
> request if CPU0 is not in WFI. On Tegra2, there is no hardware
> support and I had to handle it with a spinlock implemented in scratch
> registers because CPU0 is out of coherency when it starts booting and
> ldrex/strex don't work. I'm not convinced my implementation is
> correct, and I'd be curious to see any other implementations.
That's a problem you solved with coupled C-states (ie your example in
the cover letter), where the primary waits for other CPUs to be reset
before issuing the power down command, right ? At that point in time
secondaries cannot wake up (?) and if wfi (ie power down) aborts you just
take the secondaries out of reset and restart executing simultaneously,
correct ? It mirrors the suspend behaviour, which is easier to deal with
than completely random idle paths.
It is true that this should be managed by the PM HW; if HW is not
capable of managing these situations things get nasty as you highlighted.
And it is also true ldrex/strex on cacheable memory might not be available in
those early warm-boot stages. I came up with a locking algorithm on
strongly ordered memory to deal with that, but I am still not sure it is
something we really really need.
I will test coupled C-state code ASAP, and come back with feedback.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [linux-pm] [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-02-01 17:30 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Vincent Guittot, Daniel Lezcano, Kevin Hilman, Len Brown,
linux-kernel@vger.kernel.org, Amit Kucheria,
linux-tegra@vger.kernel.org, linux-pm@lists.linux-foundation.org,
linux-omap@vger.kernel.org, Arjan van de Ven,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20120201145934.GA20421@e102568-lin.cambridge.arm.com>
On Wed, Feb 1, 2012 at 6:59 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 01, 2012 at 12:13:26PM +0000, Vincent Guittot wrote:
>
> [...]
>
>> >> In your patch, you put in safe state (WFI for most of platform) the
>> >> cpus that become idle and these cpus are woken up each time a new cpu
>> >> of the cluster becomes idle. Then, the cluster state is chosen and the
>> >> cpus enter the selected C-state. On ux500, we are using another
>> >> behavior for synchronizing the cpus. The cpus are prepared to enter
>> >> the c-state that has been chosen by the governor and the last cpu,
>> >> that enters idle, chooses the final cluster state (according to cpus'
>> >> C-state). The main advantage of this solution is that you don't need
>> >> to wake other cpus to enter the C-state of a cluster. This can be
>> >> quite worth full when tasks mainly run on one cpu. Have you also think
>> >> about such behavior when developing the coupled cpuidle driver ? It
>> >> could be interesting to add such behavior.
>> >
>> > Waking up the cpus that are in the safe state is not done just to
>> > choose the target state, it's done to allow the cpus to take
>> > themselves to the target low power state. On ux500, are you saying
>> > you take the cpus directly from the safe state to a lower power state
>> > without ever going back to the active state? I once implemented Tegra
>>
>> yes it is
>
> But if there is a single power rail for the entire cluster, when a CPU
> is "prepared" for shutdown this means that you have to save the context and
> clean L1, maybe for nothing since if other CPUs are up and running the
> CPU going idle can just enter a simple standby wfi (clock-gated but power on).
>
> With Colin's approach, context is saved and L1 cleaned only when it is
> almost certain the cluster is powered off (so the CPUs).
>
> It is a trade-off, I am not saying one approach is better than the
> other; we just have to make sure that preparing the CPU for "possible" shutdown
> is better than sending IPIs to take CPUs out of wfi and synchronize
> them (this happens if and only if CPUs enter coupled C-states).
>
> As usual this will depend on use cases (and silicon implementations :) )
>
> It is definitely worth benchmarking them.
>
I'm less worried about performance, and more worried about race
conditions. How do you deal with the following situation:
CPU0 goes to WFI, and saves its state
CPU1 goes idle, and selects a deep idle state that powers down CPU0
CPU1 saves is state, and is about to trigger the power down
CPU0 gets an interrupt, restores its state, and modifies state (maybe
takes a spinlock during boot)
CPU1 cuts the power to CPU0
On OMAP4, the race is handled in hardware. When CPU1 tries to cut the
power to the blocks shared by CPU0 the hardware will ignore the
request if CPU0 is not in WFI. On Tegra2, there is no hardware
support and I had to handle it with a spinlock implemented in scratch
registers because CPU0 is out of coherency when it starts booting and
ldrex/strex don't work. I'm not convinced my implementation is
correct, and I'd be curious to see any other implementations.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Lorenzo Pieralisi @ 2012-02-01 14:59 UTC (permalink / raw)
To: Vincent Guittot
Cc: Kevin Hilman, Len Brown, linux-kernel@vger.kernel.org,
Amit Kucheria, Colin Cross, linux-tegra@vger.kernel.org,
linux-pm@lists.linux-foundation.org, linux-omap@vger.kernel.org,
Arjan van de Ven, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAKfTPtB3Kcy41H9g9u3u81kEHJeypi=GvOfOjcpdztzqN991nA@mail.gmail.com>
On Wed, Feb 01, 2012 at 12:13:26PM +0000, Vincent Guittot wrote:
[...]
> >> In your patch, you put in safe state (WFI for most of platform) the
> >> cpus that become idle and these cpus are woken up each time a new cpu
> >> of the cluster becomes idle. Then, the cluster state is chosen and the
> >> cpus enter the selected C-state. On ux500, we are using another
> >> behavior for synchronizing the cpus. The cpus are prepared to enter
> >> the c-state that has been chosen by the governor and the last cpu,
> >> that enters idle, chooses the final cluster state (according to cpus'
> >> C-state). The main advantage of this solution is that you don't need
> >> to wake other cpus to enter the C-state of a cluster. This can be
> >> quite worth full when tasks mainly run on one cpu. Have you also think
> >> about such behavior when developing the coupled cpuidle driver ? It
> >> could be interesting to add such behavior.
> >
> > Waking up the cpus that are in the safe state is not done just to
> > choose the target state, it's done to allow the cpus to take
> > themselves to the target low power state. On ux500, are you saying
> > you take the cpus directly from the safe state to a lower power state
> > without ever going back to the active state? I once implemented Tegra
>
> yes it is
But if there is a single power rail for the entire cluster, when a CPU
is "prepared" for shutdown this means that you have to save the context and
clean L1, maybe for nothing since if other CPUs are up and running the
CPU going idle can just enter a simple standby wfi (clock-gated but power on).
With Colin's approach, context is saved and L1 cleaned only when it is
almost certain the cluster is powered off (so the CPUs).
It is a trade-off, I am not saying one approach is better than the
other; we just have to make sure that preparing the CPU for "possible" shutdown
is better than sending IPIs to take CPUs out of wfi and synchronize
them (this happens if and only if CPUs enter coupled C-states).
As usual this will depend on use cases (and silicon implementations :) )
It is definitely worth benchmarking them.
Lorenzo
^ permalink raw reply
* Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
From: Matthew Garrett @ 2012-02-01 14:57 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-pm, linux-kernel, linux-acpi, lenb, rui.zhang, linaro-dev,
patches
In-Reply-To: <1323789196-4942-3-git-send-email-amit.kachhap@linaro.org>
On Tue, Dec 13, 2011 at 08:43:16PM +0530, Amit Daniel Kachhap wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using frequency scaling and cpuhotplugg currently.
We've been over this kind of thing in the past. cpu hotplug is a
relatively expensive operation, so people have previously been
enthusiastic about using the scheduler to simply avoid running anything
on CPUs if they're overheating. Has any general consensus been reached
on this?
I'm also not entirely thrilled at now having two ways to manage the cpu
through the thermal layer. ACPI already plugs in via the passive trip
points. If we're going to do this then I'd like to see the ACPI code
merged in with the generic cpu cooling code.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number
From: Matthew Garrett @ 2012-02-01 14:49 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-pm, linux-kernel, linux-acpi, lenb, rui.zhang, linaro-dev,
patches
In-Reply-To: <1323789196-4942-2-git-send-email-amit.kachhap@linaro.org>
I'm not really a fan of this as it stands - the name isn't very
intuitive and the code's pretty difficult to read. Would the following
(incomplete and obviously untested) not have the effect you want? Then
you register multiple trip points with the same cooling device but
different private values, and the state set does whatever you want it
to. Or am I misunderstanding the problem you're trying to solve?
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 220ce7e..817f2ba 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
char attr_name[THERMAL_NAME_LENGTH];
struct device_attribute attr;
struct list_head node;
+ unsigned long private;
};
static DEFINE_IDR(thermal_tz_idr);
@@ -909,7 +910,8 @@ static struct class thermal_class = {
* @ops: standard thermal cooling devices callbacks.
*/
struct thermal_cooling_device *thermal_cooling_device_register(
- char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
+ char *type, void *devdata, const struct thermal_cooling_device_ops *ops,
+ unsigned long private)
{
struct thermal_cooling_device *cdev;
struct thermal_zone_device *pos;
@@ -936,6 +938,7 @@ struct thermal_cooling_device *thermal_cooling_device_register(
cdev->ops = ops;
cdev->device.class = &thermal_class;
cdev->devdata = devdata;
+ cdev->private = private;
dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
result = device_register(&cdev->device);
if (result) {
@@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
continue;
cdev = instance->cdev;
-
- if (temp >= trip_temp)
- cdev->ops->set_cur_state(cdev, 1);
- else
- cdev->ops->set_cur_state(cdev, 0);
+ if (cdev->private) {
+ cdev->ops->set_cur_state(cdev, cdev->private);
+ } else {
+ if (temp >= trip_temp)
+ cdev->ops->set_cur_state(cdev, 1);
+ else
+ cdev->ops->set_cur_state(cdev, 0);
+ }
}
break;
case THERMAL_TRIP_PASSIVE:
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 796f1ff..04aac09 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
struct thermal_cooling_device *);
void thermal_zone_device_update(struct thermal_zone_device *);
struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
- const struct thermal_cooling_device_ops *);
+ const struct thermal_cooling_device_ops *, unsigned long private);
void thermal_cooling_device_unregister(struct thermal_cooling_device *);
#ifdef CONFIG_NET
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply related
* Re: [linux-pm] [PATCH 0/3] coupled cpuidle state support
From: Vincent Guittot @ 2012-02-01 12:13 UTC (permalink / raw)
To: Colin Cross
Cc: Daniel Lezcano, Kevin Hilman, Len Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Amit Kucheria,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Arjan van de Ven,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <CAMbhsRQYVXTZ2pX4mBrM6=SyGTqn_GY8xsW4rt_e21zerEzctA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Colin,
Sorry for this late reply
On 27 January 2012 18:32, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, Jan 27, 2012 at 12:54 AM, Vincent Guittot
> <vincent.guittot-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 20 January 2012 21:40, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
>>> On Fri, Jan 20, 2012 at 12:46 AM, Daniel Lezcano
>>> <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> Hi Colin,
>>>>
>>>> this patchset could be interesting to resolve in a generic way the cpu
>>>> dependencies.
>>>> What is the status of this patchset ?
>>>
>>> I can't do much with it right now, because I don't have any devices
>>> that can do SMP idle with a v3.2 kernel. I've started working on an
>>> updated version that avoids the spinlock, but it might be a while
>>> before I can test and post it. I'm mostly looking for feedback on the
>>> approach taken in this patch, and whether it will be useful for other
>>> SoCs besides Tegra and OMAP4.
>>>
>>
>> Hi Colin,
>>
>> In your patch, you put in safe state (WFI for most of platform) the
>> cpus that become idle and these cpus are woken up each time a new cpu
>> of the cluster becomes idle. Then, the cluster state is chosen and the
>> cpus enter the selected C-state. On ux500, we are using another
>> behavior for synchronizing the cpus. The cpus are prepared to enter
>> the c-state that has been chosen by the governor and the last cpu,
>> that enters idle, chooses the final cluster state (according to cpus'
>> C-state). The main advantage of this solution is that you don't need
>> to wake other cpus to enter the C-state of a cluster. This can be
>> quite worth full when tasks mainly run on one cpu. Have you also think
>> about such behavior when developing the coupled cpuidle driver ? It
>> could be interesting to add such behavior.
>
> Waking up the cpus that are in the safe state is not done just to
> choose the target state, it's done to allow the cpus to take
> themselves to the target low power state. On ux500, are you saying
> you take the cpus directly from the safe state to a lower power state
> without ever going back to the active state? I once implemented Tegra
yes it is
> that way, and it required lots of nasty synchronization to prevent
> resetting the cpu at the same time that it was booting due to an
> interrupt, and I was later told that Tegra can't handle that sequence
> at all, although I haven't verified it yet.
you have to 2 main things to check :
- this cpu is the last one to enter an idle state
- other cpus are prepared to enter a cluster power state
- other cpus are in WFI
Vincent
>
> On platforms that can't turn the cpus off in a random order, or that
> can't take a cpu directly from the safe state to the target state,
> something like these coupled cpuidle patches are required. On
> platforms that can, the low power modes can be implemented without
> these patches, although it is very hard to do without race conditions.
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Daniel Lezcano @ 2012-01-31 14:13 UTC (permalink / raw)
To: Colin Cross
Cc: Daniel Lezcano, linux-kernel, linux-arm-kernel, linux-pm,
Len Brown, Kevin Hilman, Santosh Shilimkar, Amit Kucheria,
Arjan van de Ven, Trinabh Gupta, Deepthi Dharwar, linux-omap,
linux-tegra
In-Reply-To: <4F200BE2.2070301@free.fr>
On 01/25/2012 03:04 PM, Daniel Lezcano wrote:
> On 01/20/2012 09:40 PM, Colin Cross wrote:
>> On Fri, Jan 20, 2012 at 12:46 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> Hi Colin,
>>>
>>> this patchset could be interesting to resolve in a generic way the cpu
>>> dependencies.
>>> What is the status of this patchset ?
>> I can't do much with it right now, because I don't have any devices
>> that can do SMP idle with a v3.2 kernel. I've started working on an
>> updated version that avoids the spinlock, but it might be a while
>> before I can test and post it. I'm mostly looking for feedback on the
>> approach taken in this patch, and whether it will be useful for other
>> SoCs besides Tegra and OMAP4.
>
> Hi Colin,
>
> I will be happy to test your patchset. Do you have a pointer to a more
> recent kernel ?
Hi Colin,
sorry for bothering you. I am working on a cpuidle driver for an ARM SMP
board, I will be really happy to test your patchset and look how it fits
with the cpuidle driver.
Thanks
-- Daniel
^ permalink raw reply
* Re: [linux-pm] [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-01-27 17:32 UTC (permalink / raw)
To: Vincent Guittot
Cc: Daniel Lezcano, Kevin Hilman, Len Brown, linux-kernel,
Amit Kucheria, linux-tegra, linux-pm, linux-omap,
Arjan van de Ven, linux-arm-kernel
In-Reply-To: <CAKfTPtAsO8z_8AnR8zSGZ9cm_7orDNxB3r09JAFk+2jdezUVmQ@mail.gmail.com>
On Fri, Jan 27, 2012 at 12:54 AM, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
> On 20 January 2012 21:40, Colin Cross <ccross@android.com> wrote:
>> On Fri, Jan 20, 2012 at 12:46 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> Hi Colin,
>>>
>>> this patchset could be interesting to resolve in a generic way the cpu
>>> dependencies.
>>> What is the status of this patchset ?
>>
>> I can't do much with it right now, because I don't have any devices
>> that can do SMP idle with a v3.2 kernel. I've started working on an
>> updated version that avoids the spinlock, but it might be a while
>> before I can test and post it. I'm mostly looking for feedback on the
>> approach taken in this patch, and whether it will be useful for other
>> SoCs besides Tegra and OMAP4.
>>
>
> Hi Colin,
>
> In your patch, you put in safe state (WFI for most of platform) the
> cpus that become idle and these cpus are woken up each time a new cpu
> of the cluster becomes idle. Then, the cluster state is chosen and the
> cpus enter the selected C-state. On ux500, we are using another
> behavior for synchronizing the cpus. The cpus are prepared to enter
> the c-state that has been chosen by the governor and the last cpu,
> that enters idle, chooses the final cluster state (according to cpus'
> C-state). The main advantage of this solution is that you don't need
> to wake other cpus to enter the C-state of a cluster. This can be
> quite worth full when tasks mainly run on one cpu. Have you also think
> about such behavior when developing the coupled cpuidle driver ? It
> could be interesting to add such behavior.
Waking up the cpus that are in the safe state is not done just to
choose the target state, it's done to allow the cpus to take
themselves to the target low power state. On ux500, are you saying
you take the cpus directly from the safe state to a lower power state
without ever going back to the active state? I once implemented Tegra
that way, and it required lots of nasty synchronization to prevent
resetting the cpu at the same time that it was booting due to an
interrupt, and I was later told that Tegra can't handle that sequence
at all, although I haven't verified it yet.
On platforms that can't turn the cpus off in a random order, or that
can't take a cpu directly from the safe state to the target state,
something like these coupled cpuidle patches are required. On
platforms that can, the low power modes can be implemented without
these patches, although it is very hard to do without race conditions.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [linux-pm] [PATCH 0/3] coupled cpuidle state support
From: Vincent Guittot @ 2012-01-27 8:54 UTC (permalink / raw)
To: Colin Cross
Cc: Daniel Lezcano, Kevin Hilman, Len Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Amit Kucheria,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Arjan van de Ven,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <CAMbhsRTbVqV6p0py9NGZrpFPf2yvp-B0dr+o-m7qbdja_-Lfzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 20 January 2012 21:40, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
> On Fri, Jan 20, 2012 at 12:46 AM, Daniel Lezcano
> <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Hi Colin,
>>
>> this patchset could be interesting to resolve in a generic way the cpu
>> dependencies.
>> What is the status of this patchset ?
>
> I can't do much with it right now, because I don't have any devices
> that can do SMP idle with a v3.2 kernel. I've started working on an
> updated version that avoids the spinlock, but it might be a while
> before I can test and post it. I'm mostly looking for feedback on the
> approach taken in this patch, and whether it will be useful for other
> SoCs besides Tegra and OMAP4.
>
Hi Colin,
In your patch, you put in safe state (WFI for most of platform) the
cpus that become idle and these cpus are woken up each time a new cpu
of the cluster becomes idle. Then, the cluster state is chosen and the
cpus enter the selected C-state. On ux500, we are using another
behavior for synchronizing the cpus. The cpus are prepared to enter
the c-state that has been chosen by the governor and the last cpu,
that enters idle, chooses the final cluster state (according to cpus'
C-state). The main advantage of this solution is that you don't need
to wake other cpus to enter the C-state of a cluster. This can be
quite worth full when tasks mainly run on one cpu. Have you also think
about such behavior when developing the coupled cpuidle driver ? It
could be interesting to add such behavior.
Regards,
Vincent
>> Did you have the opportunity to measure the power consumption with and
>> without this patchset ?
>
> Power consumption will be very dependent on the specific SoC in
> question. The most important factors are the power savings of the
> independent cpuidle state (normally WFI) vs. the hotplug state
> (normally 1 cpu in OFF), and the workload being tested.
>
> On a very idle system, these patches result in the same total power as
> hotplugging one cpu and letting the other idle normally. On a 25%
> busy system, you might see a slight increase in power, as the best
> independent cpuidle state might be WFI, vs 1 cpu in OFF mode in
> hotplug. On OMAP4, that difference is small, on the order of 10 mW.
> Once you hit the threshold where a hotplug governor would have
> hotplugged in the second cpu (lets say 40%), the savings from these
> patches are enormous, as you can hit the lowest power state up to 60%
> of the time, where the hotplug solution would never be going below WFI
> on both cpus. On OMAP4, that can be well over 100 mW.
> _______________________________________________
> linux-pm mailing list
> linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: PM: cannot hibernate -- BUG at kernel/workqueue.c:3659
From: Rafael J. Wysocki @ 2012-01-27 1:10 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Jiri Slaby, LKML, Baohua.Song, Tejun Heo, Linux-pm mailing list,
Jiri Slaby
In-Reply-To: <4F21ABE2.1060302@linux.vnet.ibm.com>
On Thursday, January 26, 2012, Srivatsa S. Bhat wrote:
>
> Hi Rafael,
>
> On 01/25/2012 09:01 PM, Srivatsa S. Bhat wrote:
>
> >
> > Ok, I will need to quote a part of the userspace utility to explain the
> > problem.
> >
> > In suspend.c inside the suspend-utils userspace package, I see a loop such
> > as:
> >
> > error = freeze(snapshot_fd);
> > ...
> > attempts = 2;
> > do {
> > if (set_image_size(snapshot_fd, image_size)) {
> > error = errno;
> > break;
> > }
> > if (atomic_snapshot(snapshot_fd, &in_suspend)) {
> > error = errno;
> > break;
> > }
> > if (!in_suspend) {
> > /* first unblank the console, see console_codes(4) */
> > printf("\e[13]");
> > printf("%s: returned to userspace\n", my_name);
> > free_snapshot(snapshot_fd);
> > break;
> > }
> >
> > error = write_image(snapshot_fd, resume_fd, -1);
> > if (error) {
> > free_swap_pages(snapshot_fd);
> > free_snapshot(snapshot_fd);
> > image_size = 0;
> > error = -error;
> > if (error != ENOSPC)
> > break;
> > } else {
> > splash.progress(100);
> > #ifdef CONFIG_BOTH
> > if (s2ram_kms || s2ram) {
> > /* If we die (and allow system to continue)
> > * between now and reset_signature(), very bad
> > * things will happen. */
> > error = suspend_to_ram(snapshot_fd);
> > if (error)
> > goto Shutdown;
> > reset_signature(resume_fd);
> > free_swap_pages(snapshot_fd);
> > free_snapshot(snapshot_fd);
> > if (!s2ram_kms)
> > s2ram_resume();
>
>
> Your patch alters how SNAPSHOT_FREE (IOW, free_snapshot() in this utility) is
> handled. So, I was trying to see if there are any points of concern...
>
> In the above code, s2ram_resume() gets invoked after free_snapshot(). Will that
> pose any problems because kernel threads would have been thawed at that point,
> after applying your patch?
No, it shouldn't. s2ram_resume() only executes quirks needed to restore the
state of graphics if KMS is not being used. That shouldn't interfere with
any kernel threads.
> And other than that, do you foresee any problems arising from the change caused
> to SNAPSHOT_FREE by your patch? I mean, s2ram/s2disk/suspend-utils package are
> not the only userspace utilities after all... so I just wanted to ensure that
> we don't over-fit our solution to this particular utility and end up breaking
> others...
I'm quite sure they are the only package using the interface in
kernel/power/user.c. At least, I'm not aware of any other users. :-)
Thanks,
Rafael
^ permalink raw reply
* Re: PM: cannot hibernate -- BUG at kernel/workqueue.c:3659
From: Srivatsa S. Bhat @ 2012-01-26 19:39 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jiri Slaby, LKML, Baohua.Song, Tejun Heo, Linux-pm mailing list,
Jiri Slaby
In-Reply-To: <4F20204F.6040606@linux.vnet.ibm.com>
Hi Rafael,
On 01/25/2012 09:01 PM, Srivatsa S. Bhat wrote:
>
> Ok, I will need to quote a part of the userspace utility to explain the
> problem.
>
> In suspend.c inside the suspend-utils userspace package, I see a loop such
> as:
>
> error = freeze(snapshot_fd);
> ...
> attempts = 2;
> do {
> if (set_image_size(snapshot_fd, image_size)) {
> error = errno;
> break;
> }
> if (atomic_snapshot(snapshot_fd, &in_suspend)) {
> error = errno;
> break;
> }
> if (!in_suspend) {
> /* first unblank the console, see console_codes(4) */
> printf("\e[13]");
> printf("%s: returned to userspace\n", my_name);
> free_snapshot(snapshot_fd);
> break;
> }
>
> error = write_image(snapshot_fd, resume_fd, -1);
> if (error) {
> free_swap_pages(snapshot_fd);
> free_snapshot(snapshot_fd);
> image_size = 0;
> error = -error;
> if (error != ENOSPC)
> break;
> } else {
> splash.progress(100);
> #ifdef CONFIG_BOTH
> if (s2ram_kms || s2ram) {
> /* If we die (and allow system to continue)
> * between now and reset_signature(), very bad
> * things will happen. */
> error = suspend_to_ram(snapshot_fd);
> if (error)
> goto Shutdown;
> reset_signature(resume_fd);
> free_swap_pages(snapshot_fd);
> free_snapshot(snapshot_fd);
> if (!s2ram_kms)
> s2ram_resume();
Your patch alters how SNAPSHOT_FREE (IOW, free_snapshot() in this utility) is
handled. So, I was trying to see if there are any points of concern...
In the above code, s2ram_resume() gets invoked after free_snapshot(). Will that
pose any problems because kernel threads would have been thawed at that point,
after applying your patch?
And other than that, do you foresee any problems arising from the change caused
to SNAPSHOT_FREE by your patch? I mean, s2ram/s2disk/suspend-utils package are
not the only userspace utilities after all... so I just wanted to ensure that
we don't over-fit our solution to this particular utility and end up breaking
others...
Regards,
Srivatsa S. Bhat
> goto Unfreeze;
> }
> Shutdown:
> #endif
> close(resume_fd);
> suspend_shutdown(snapshot_fd);
> }
> } while (--attempts);
>
> ...
> Unfreeze:
> unfreeze(snapshot_fd);
>
^ permalink raw reply
* Re: Question regarding completion signal in S3 flow
From: Mansoor, Illyas @ 2012-01-26 17:07 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-pm@lists.linux-foundation.org
In-Reply-To: <Pine.LNX.4.44L0.1201261130000.1386-100000@iolanthe.rowland.org>
[-- Attachment #1.1: Type: text/plain, Size: 7150 bytes --]
Thank you Alan for your response, we will try reproducing this issue, this is a
very
rare case and we have just 2 such occurrences till now.
> On Thu, 26 Jan 2012, Mansoor, Illyas wrote:
>
> > > > I have question related to suspend flow and completion signal:
> > > >
> > > > Is it possible that a completion signal could be missed in S3 flow
> > > > after processes are frozen?
> > >
> > > What do you mean by "completion signal"?
> >
> > In our case we have an interrupt that signals completion of the
> > interrupt using
> > complete()
> > For a waiting thread.
>
> It sounds like you're saying you have an interrupt handler that calls
complete(),
> and the target thread is stuck in wait_for_completion().
Yes, that is correct, and the thread that is waiting for completion is the
suspend thread
which is about to enter suspend, and hence the CPU is still not frozen and can
receive
Completion.
>
> > Our interrupt is configured with IRQ_NO_SUSPEND so we expect
> > interrupts during Suspend also.
>
> IRQ_NO_SUSPEND no longer exists. Regardless, you can't receive interrupts
> while the system is in suspend, because by definition the CPU isn't running at
> that time.
IRQF_NO_SUSPEND is the exact flag that is used, and the wait_for_completion
happens
In suspend thread
dpm_suspend_start->...>pci_set_power_state->wait_for_completion
>
> > So here is what I'm seeing in the panic logs:
> >
> > <4>[ 7960.661939] Call Trace:
> > <4>[ 7960.661953] [<c12384a5>] ? sub_preempt_count+0x85/0xc0 <4>[
> > 7960.661965] [<c12735b5>] refrigerator+0xa5/0x160 <4>[ 7960.661977]
> > [<c125f64d>] get_signal_to_deliver+0x9ad/0xdc0 <4>[ 7960.661991]
> > [<c120257b>] do_signal+0x6b/0xa20 <4>[ 7960.662003] [<c186ffbd>] ?
> > schedule_hrtimeout_range+0x1cd/0x220
> > <4>[ 7960.662018] [<c122d4a4>] ? pmu_sc_irq+0x364/0x3d0
> > <==============This is where in the interrupt we signal completion
> > using complete()
>
> That is irrelevant. The '?' means this is merely a random value on the stack,
not
> the return address of a function call.
Okay, I see. 0xc122d4a4 is the exact instruction in pmu_sc_irq where it calls
complete()
So does this mean at least this (complete() ) happened some time during panic?
>
> > <4>[ 7960.662030] [<c18715f3>] ?
> > _raw_spin_unlock_irqrestore+0x23/0x50
> > <4>[ 7960.662043] [<c12384a5>] ? sub_preempt_count+0x85/0xc0 <4>[
> > 7960.662056] [<c134c162>] ? sys_epoll_wait+0x72/0x300 <4>[
> > 7960.662069] [<c1242c40>] ? default_wake_function+0x0/0x20 <4>[
> > 7960.662082] [<c1202f85>] do_notify_resume+0x55/0x90 <4>[
> > 7960.662094] [<c1871e99>] work_notifysig+0x9/0x1
>
> This stack trace appears to show that some thread attempted to send a signal
and
> got frozen while waiting.
pm_sc_irq is the interrupt handler that calls complete(), and this happens to be
listed in each of the threads call stack at least 8 such occurrences are there.
>
> > <6>[ 7960.380768] suspend R running 0 19 2 0x00000000
> > <4>[ 7960.380780] f78e3c94 00000000 c1204760 f78e3c8c c12047eb
> > f78e3c94
> > 00000000 f78e3c04
> > <4>[ 7960.380800] 00000004 00000000 00030002 c1852942 00000001
> > 00000001
> > 00000282 f78e3c5c
> > <4>[ 7960.380820] f78e3c30 c12384a5 00000282 f78e3c3c c18715f3
> > f7868000
> > f78e3c84 c186eebc
> > <4>[ 7960.380840] Call Trace:
> > <4>[ 7960.380850] [<c1204760>] ? do_invalid_op+0x0/0xb0 <4>[
> > 7960.380862] [<c12047eb>] ? do_invalid_op+0x8b/0xb0 <4>[ 7960.380875]
> > [<c1852942>] ? pmu_pci_set_power_state+0x322/0x6e0 <==== here is where
> > wait_for_completion_timeout call BUG() after timeout.
> > <4>[ 7960.380888] [<c12384a5>] ? sub_preempt_count+0x85/0xc0 <4>[
> > 7960.380901] [<c18715f3>] ? _raw_spin_unlock_irqrestore+0x23/0x50
> > <4>[ 7960.380913] [<c186eebc>] ? schedule_timeout+0x1dc/0x430 <4>[
> > 7960.380926] [<c1204760>] ? do_invalid_op+0x0/0xb0 <4>[ 7960.380937]
> > [<c14b916c>] ? trace_hardirqs_off_thunk+0xc/0x10 <4>[ 7960.380950]
> > [<c187244b>] ? error_code+0x6b/0x70 <4>[ 7960.380961] [<c186dd96>] ?
> > wait_for_common+0x96/0x120 <4>[ 7960.380973] [<c1204760>] ?
> > do_invalid_op+0x0/0xb0 <4>[ 7960.380985] [<c1852942>] ?
> > pmu_pci_set_power_state+0x322/0x6e0
> > <4>[ 7960.380998] [<c14b502a>] ? put_dec+0x2a/0xa0 <4>[ 7960.381011]
> > [<c14b502a>] ? put_dec+0x2a/0xa0 <4>[ 7960.381025] [<c12384a5>] ?
> > sub_preempt_count+0x85/0xc0 <4>[ 7960.381038] [<c14cee6e>] ?
> > pci_platform_power_transition+0x3e/0xa0
> > <4>[ 7960.381051] [<c18715f3>] ?
> > _raw_spin_unlock_irqrestore+0x23/0x50
> > <4>[ 7960.381064] [<c14cf5df>] ? pci_set_power_state+0x3f/0x2c0 <4>[
> > 7960.381077] [<c14ced7c>] ? pci_update_current_state+0x3c/0x50
> > <4>[ 7960.381090] [<c14d180e>] ? pci_pm_runtime_resume+0x5e/0xa0 <4>[
> > 7960.381102] [<c12384a5>] ? sub_preempt_count+0x85/0xc0 <4>[
> > 7960.381114] [<c14d17b0>] ? pci_pm_runtime_resume+0x0/0xa0 <4>[
> > 7960.381126] [<c1554d7b>] ? rpm_callback+0x3b/0x70 <4>[ 7960.381137]
> > [<c155577c>] ? rpm_resume+0x37c/0x5c0 <4>[ 7960.381150] [<c124923b>]
> > ? release_console_sem+0x37b/0x3c0 <4>[ 7960.381164] [<c1238593>] ?
> > add_preempt_count+0xb3/0xf0 <4>[ 7960.381176] [<c1556609>] ?
> > __pm_runtime_resume+0x49/0xc0 <4>[ 7960.381189] [<c14d1b71>] ?
> > pci_pm_prepare+0x21/0x60 <4>[ 7960.381200] [<c1553947>] ?
> > dpm_suspend_start+0x137/0x7d0 <4>[ 7960.381213] [<c12384a5>] ?
> > sub_preempt_count+0x85/0xc0 <4>[ 7960.381225] [<c18715f3>] ?
> > _raw_spin_unlock_irqrestore+0x23/0x50
> > <4>[ 7960.381237] [<c126f548>] ? up+0x28/0x40 <4>[ 7960.381250]
> > [<c1288f13>] ? suspend_devices_and_enter+0x73/0x1d0
> > <4>[ 7960.381262] [<c1289196>] ? enter_state+0x126/0x1e0 <4>[
> > 7960.381273] [<c1289277>] ? pm_suspend+0x27/0x70 <4>[ 7960.381285]
> > [<c128acba>] ? suspend+0x8a/0x160 <4>[ 7960.381296] [<c186e445>] ?
> > schedule+0x545/0x9e0 <4>[ 7960.381310] [<c12384a5>] ?
> > sub_preempt_count+0x85/0xc0 <4>[ 7960.381322] [<c1265833>] ?
> > worker_thread+0x123/0x2c0 <4>[ 7960.381333] [<c186e445>] ?
> > schedule+0x545/0x9e0 <4>[ 7960.381346] [<c128ac30>] ?
> > suspend+0x0/0x160 <4>[ 7960.381357] [<c12690d0>] ?
> > autoremove_wake_function+0x0/0x50 <4>[ 7960.381369] [<c1265710>] ?
> > worker_thread+0x0/0x2c0 <4>[ 7960.381381] [<c1268c34>] ?
> > kthread+0x74/0x80 <4>[ 7960.381393] [<c1268bc0>] ? kthread+0x0/0x80
> > <4>[ 7960.381405] [<c120357a>] ? kernel_thread_helper+0x6/0x1
>
> This is very difficult to understand. You should add some printk statements
to
> your code, so that you will know what is going on.
>
> > So my question, is it possible that the complete() called in interrupt
> > context can be missed During S3?
>
> I don't know what you mean by "missed". The complete() call will work, but
the
> target thread might not return from wait_for_completion() until after the
system
> returns to S0.
Since we know that the interrupt handler has called complete() and we have timed
out
Of wait_for_completion_timeout in the suspend flow before suspend, I assumed
completion
signal is missed because of ongoing S3, so either the interrupt just came in
while the wait_for_completioin
timed out, so we BUG() out?
-Illyas
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7212 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: Question regarding completion signal in S3 flow
From: Alan Stern @ 2012-01-26 16:49 UTC (permalink / raw)
To: Mansoor, Illyas; +Cc: linux-pm@lists.linux-foundation.org
In-Reply-To: <810586B7581CC8469141DADEBC371912057F41@BGSMSX102.gar.corp.intel.com>
On Thu, 26 Jan 2012, Mansoor, Illyas wrote:
> > > I have question related to suspend flow and completion signal:
> > >
> > > Is it possible that a completion signal could be missed in S3 flow
> > > after processes are frozen?
> >
> > What do you mean by "completion signal"?
>
> In our case we have an interrupt that signals completion of the interrupt using
> complete()
> For a waiting thread.
It sounds like you're saying you have an interrupt handler that calls
complete(), and the target thread is stuck in wait_for_completion().
> Our interrupt is configured with IRQ_NO_SUSPEND so we expect interrupts during
> Suspend also.
IRQ_NO_SUSPEND no longer exists. Regardless, you can't receive
interrupts while the system is in suspend, because by definition the
CPU isn't running at that time.
> So here is what I'm seeing in the panic logs:
>
> <4>[ 7960.661939] Call Trace:
> <4>[ 7960.661953] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.661965] [<c12735b5>] refrigerator+0xa5/0x160
> <4>[ 7960.661977] [<c125f64d>] get_signal_to_deliver+0x9ad/0xdc0
> <4>[ 7960.661991] [<c120257b>] do_signal+0x6b/0xa20
> <4>[ 7960.662003] [<c186ffbd>] ? schedule_hrtimeout_range+0x1cd/0x220
> <4>[ 7960.662018] [<c122d4a4>] ? pmu_sc_irq+0x364/0x3d0 <==============This is
> where in the interrupt we signal completion using complete()
That is irrelevant. The '?' means this is merely a random value on the
stack, not the return address of a function call.
> <4>[ 7960.662030] [<c18715f3>] ? _raw_spin_unlock_irqrestore+0x23/0x50
> <4>[ 7960.662043] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.662056] [<c134c162>] ? sys_epoll_wait+0x72/0x300
> <4>[ 7960.662069] [<c1242c40>] ? default_wake_function+0x0/0x20
> <4>[ 7960.662082] [<c1202f85>] do_notify_resume+0x55/0x90
> <4>[ 7960.662094] [<c1871e99>] work_notifysig+0x9/0x1
This stack trace appears to show that some thread attempted to send a
signal and got frozen while waiting.
> <6>[ 7960.380768] suspend R running 0 19 2 0x00000000
> <4>[ 7960.380780] f78e3c94 00000000 c1204760 f78e3c8c c12047eb f78e3c94
> 00000000 f78e3c04
> <4>[ 7960.380800] 00000004 00000000 00030002 c1852942 00000001 00000001
> 00000282 f78e3c5c
> <4>[ 7960.380820] f78e3c30 c12384a5 00000282 f78e3c3c c18715f3 f7868000
> f78e3c84 c186eebc
> <4>[ 7960.380840] Call Trace:
> <4>[ 7960.380850] [<c1204760>] ? do_invalid_op+0x0/0xb0
> <4>[ 7960.380862] [<c12047eb>] ? do_invalid_op+0x8b/0xb0
> <4>[ 7960.380875] [<c1852942>] ? pmu_pci_set_power_state+0x322/0x6e0 <==== here
> is where wait_for_completion_timeout call BUG() after timeout.
> <4>[ 7960.380888] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.380901] [<c18715f3>] ? _raw_spin_unlock_irqrestore+0x23/0x50
> <4>[ 7960.380913] [<c186eebc>] ? schedule_timeout+0x1dc/0x430
> <4>[ 7960.380926] [<c1204760>] ? do_invalid_op+0x0/0xb0
> <4>[ 7960.380937] [<c14b916c>] ? trace_hardirqs_off_thunk+0xc/0x10
> <4>[ 7960.380950] [<c187244b>] ? error_code+0x6b/0x70
> <4>[ 7960.380961] [<c186dd96>] ? wait_for_common+0x96/0x120
> <4>[ 7960.380973] [<c1204760>] ? do_invalid_op+0x0/0xb0
> <4>[ 7960.380985] [<c1852942>] ? pmu_pci_set_power_state+0x322/0x6e0
> <4>[ 7960.380998] [<c14b502a>] ? put_dec+0x2a/0xa0
> <4>[ 7960.381011] [<c14b502a>] ? put_dec+0x2a/0xa0
> <4>[ 7960.381025] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.381038] [<c14cee6e>] ? pci_platform_power_transition+0x3e/0xa0
> <4>[ 7960.381051] [<c18715f3>] ? _raw_spin_unlock_irqrestore+0x23/0x50
> <4>[ 7960.381064] [<c14cf5df>] ? pci_set_power_state+0x3f/0x2c0
> <4>[ 7960.381077] [<c14ced7c>] ? pci_update_current_state+0x3c/0x50
> <4>[ 7960.381090] [<c14d180e>] ? pci_pm_runtime_resume+0x5e/0xa0
> <4>[ 7960.381102] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.381114] [<c14d17b0>] ? pci_pm_runtime_resume+0x0/0xa0
> <4>[ 7960.381126] [<c1554d7b>] ? rpm_callback+0x3b/0x70
> <4>[ 7960.381137] [<c155577c>] ? rpm_resume+0x37c/0x5c0
> <4>[ 7960.381150] [<c124923b>] ? release_console_sem+0x37b/0x3c0
> <4>[ 7960.381164] [<c1238593>] ? add_preempt_count+0xb3/0xf0
> <4>[ 7960.381176] [<c1556609>] ? __pm_runtime_resume+0x49/0xc0
> <4>[ 7960.381189] [<c14d1b71>] ? pci_pm_prepare+0x21/0x60
> <4>[ 7960.381200] [<c1553947>] ? dpm_suspend_start+0x137/0x7d0
> <4>[ 7960.381213] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.381225] [<c18715f3>] ? _raw_spin_unlock_irqrestore+0x23/0x50
> <4>[ 7960.381237] [<c126f548>] ? up+0x28/0x40
> <4>[ 7960.381250] [<c1288f13>] ? suspend_devices_and_enter+0x73/0x1d0
> <4>[ 7960.381262] [<c1289196>] ? enter_state+0x126/0x1e0
> <4>[ 7960.381273] [<c1289277>] ? pm_suspend+0x27/0x70
> <4>[ 7960.381285] [<c128acba>] ? suspend+0x8a/0x160
> <4>[ 7960.381296] [<c186e445>] ? schedule+0x545/0x9e0
> <4>[ 7960.381310] [<c12384a5>] ? sub_preempt_count+0x85/0xc0
> <4>[ 7960.381322] [<c1265833>] ? worker_thread+0x123/0x2c0
> <4>[ 7960.381333] [<c186e445>] ? schedule+0x545/0x9e0
> <4>[ 7960.381346] [<c128ac30>] ? suspend+0x0/0x160
> <4>[ 7960.381357] [<c12690d0>] ? autoremove_wake_function+0x0/0x50
> <4>[ 7960.381369] [<c1265710>] ? worker_thread+0x0/0x2c0
> <4>[ 7960.381381] [<c1268c34>] ? kthread+0x74/0x80
> <4>[ 7960.381393] [<c1268bc0>] ? kthread+0x0/0x80
> <4>[ 7960.381405] [<c120357a>] ? kernel_thread_helper+0x6/0x1
This is very difficult to understand. You should add some printk
statements to your code, so that you will know what is going on.
> So my question, is it possible that the complete() called in interrupt context
> can be missed
> During S3?
I don't know what you mean by "missed". The complete() call will work,
but the target thread might not return from wait_for_completion()
until after the system returns to S0.
Alan Stern
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox