* Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage
From: Sivaram Nair @ 2012-12-17 8:46 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: rafael.j.wysocki@intel.com, daniel.lezcano@linaro.org,
shuox.liu@intel.com, akpm@linux-foundation.org,
yanmin_zhang@intel.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <4160971.sv9ruOS3QC@vostro.rjw.lan>
On Mon, Dec 17, 2012 at 08:56:45AM +0100, Rafael J. Wysocki wrote:
> On Monday, December 17, 2012 09:38:15 AM Sivaram Nair wrote:
> > On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> > > On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > > > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > > > ops to output signed value instead of unsigned.
> > >
> > > What's actually wrong with printing it as an unsigned int?
> >
> > power_usage could have negative values (for example cpuidle/driver.c
> > inits this value to -1, -2 etc. when drv->power_specified is not set) and
> > these shows up badly in the sysfs output.
>
> Does "badly" mean "as big positive numbers"?
Yes (sorry for not being clearer).
>
> Should we actually print them at all in those case? Perhaps it'll be better to
> make the file appear empty then?
May be, but why is power_usage signed in the first place? I also noticed
Daniel Lezcano's patches that reduces the scope of this variable. So,
perhaps we can just ignore this change.
-Sivaram
^ permalink raw reply
* Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage
From: Rafael J. Wysocki @ 2012-12-17 7:56 UTC (permalink / raw)
To: Sivaram Nair
Cc: rafael.j.wysocki@intel.com, daniel.lezcano@linaro.org,
shuox.liu@intel.com, akpm@linux-foundation.org,
yanmin_zhang@intel.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20121217073815.GM10090@sivaramn-lnx>
On Monday, December 17, 2012 09:38:15 AM Sivaram Nair wrote:
> On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > > ops to output signed value instead of unsigned.
> >
> > What's actually wrong with printing it as an unsigned int?
>
> power_usage could have negative values (for example cpuidle/driver.c
> inits this value to -1, -2 etc. when drv->power_specified is not set) and
> these shows up badly in the sysfs output.
Does "badly" mean "as big positive numbers"?
Should we actually print them at all in those case? Perhaps it'll be better to
make the file appear empty then?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage
From: Sivaram Nair @ 2012-12-17 7:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: rafael.j.wysocki@intel.com, daniel.lezcano@linaro.org,
shuox.liu@intel.com, akpm@linux-foundation.org,
yanmin_zhang@intel.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <39891276.jhMMUpZpil@vostro.rjw.lan>
On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > ops to output signed value instead of unsigned.
>
> What's actually wrong with printing it as an unsigned int?
power_usage could have negative values (for example cpuidle/driver.c
inits this value to -1, -2 etc. when drv->power_specified is not set) and
these shows up badly in the sysfs output.
regards,
Sivaram
^ permalink raw reply
* Re: [PATCH] PNP: Simplify setting of resources
From: Witold Szczeponik @ 2012-12-16 20:43 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM list, ACPI Devel Maling List, LKML
In-Reply-To: <3252517.LSIXc8EikE@vostro.rjw.lan>
On 15/12/12 01:17, Rafael J. Wysocki wrote:
[snip]
> Both patches queued up for submission as v3.8 material later in the cycle.
Thanks!
--- Witold
>
> Thanks,
> Rafael
>
[snip]
^ permalink raw reply
* Re: [PATCH 2/2][V2] cpuidle - optimize the select function for the 'menu' governor
From: Francesco Lavra @ 2012-12-16 16:11 UTC (permalink / raw)
To: Daniel Lezcano
Cc: rjw, jwerner, linux-pm, deepthi, g.trinabh, linaro-dev, len.brown,
linux-kernel, akpm, snanda
In-Reply-To: <1355493455-30665-3-git-send-email-daniel.lezcano@linaro.org>
Hi Daniel,
On 12/14/2012 02:57 PM, Daniel Lezcano wrote:
> As the power is backward sorted in the states array and we are looking for
> the state consuming the little power as possible, instead of looking from
> the beginning of the array, we look from the end. That should save us some
> iterations in the loop each time we select a state at idle time.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpuidle/governors/menu.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index fe343a0..05b8998 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -367,24 +367,24 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> * Find the idle state with the lowest power while satisfying
> * our constraints.
> */
> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> + for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) {
> struct cpuidle_state *s = &drv->states[i];
> struct cpuidle_state_usage *su = &dev->states_usage[i];
>
> if (s->disabled || su->disable)
> continue;
> - if (s->target_residency > data->predicted_us) {
> - low_predicted = 1;
> - continue;
> - }
> if (s->exit_latency > latency_req)
> continue;
> + if (s->target_residency > data->predicted_us)
> + continue;
> if (s->exit_latency * multiplier > data->predicted_us)
> continue;
>
> + low_predicted = i - CPUIDLE_DRIVER_STATE_START;
You are altering the semantics of low_predicted, which is supposed to be
non-zero when the deepest C-state has not been chosen because its target
residency is more than the predicted residency.
It seems to me that the if block where target_residency is compared with
the predicted residency should be left as is.
> data->last_state_idx = i;
> data->exit_us = s->exit_latency;
> - }
> + break;
> + }
>
> /* not deepest C-state chosen for low predicted residency */
> if (low_predicted) {
--
Francesco
^ permalink raw reply
* Re: [PATCH] ACPI / PM: Do not apply ACPI_SUCCESS() to acpi_bus_get_device() result
From: Mika Westerberg @ 2012-12-16 15:59 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: ACPI Devel Maling List, LKML, Linux PM list
In-Reply-To: <1432424.eSUdEQ1VBT@vostro.rjw.lan>
On Sun, Dec 16, 2012 at 02:32:06PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since the return value of acpi_bus_get_device() is not of type
> acpi_status, ACPI_SUCCESS() should not be used for checking its
> return value. Fix that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/acpi/device_pm.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux/drivers/acpi/device_pm.c
> ===================================================================
> --- linux.orig/drivers/acpi/device_pm.c
> +++ linux/drivers/acpi/device_pm.c
> @@ -358,8 +358,7 @@ static struct acpi_device *acpi_dev_pm_g
> acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> struct acpi_device *adev;
>
> - return handle && ACPI_SUCCESS(acpi_bus_get_device(handle, &adev)) ?
> - adev : NULL;
> + return handle && !acpi_bus_get_device(handle, &adev) ? adev : NULL;
> }
>
> /**
^ permalink raw reply
* Re: [Resend][PATCH] PM: Move disabling/enabling runtime PM to late suspend/early resume
From: Alan Stern @ 2012-12-16 15:29 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Jiri Kosina, Linux PM list, LKML, Jan-Matthias Braun
In-Reply-To: <3558004.3QJ46yYCyI@vostro.rjw.lan>
On Sun, 16 Dec 2012, Rafael J. Wysocki wrote:
> On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote:
> > On Sat, 15 Dec 2012, Rafael J. Wysocki wrote:
> >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Currently, the PM core disables runtime PM for all devices right
> > > after executing subsystem/driver .suspend() callbacks for them
> > > and re-enables it right before executing subsystem/driver .resume()
> > > callbacks for them. This may lead to problems when there are
> > > two devices such that the .suspend() callback executed for one of
> > > them depends on runtime PM working for the other. In that case,
> > > if runtime PM has already been disabled for the second device,
> > > the first one's .suspend() won't work correctly (and analogously
> > > for resume).
> > >
> > > To make those issues go away, make the PM core disable runtime PM
> > > for devices right before executing subsystem/driver .suspend_late()
> > > callbacks for them and enable runtime PM for them right after
> > > executing subsystem/driver .resume_early() callbacks for them. This
> > > way the potential conflitcs between .suspend_late()/.resume_early()
> > > and their runtime PM counterparts are still prevented from happening,
> > > but the subtle ordering issues related to disabling/enabling runtime
> > > PM for devices during system suspend/resume are much easier to avoid.
> > >
> > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Hi Rafael,
> >
> > just curious what is the reason for resend? Do you want to gather more
> > Acks before pushing this upstream?
>
> Well, I thought that some people might actually look at it when they found it
> again in their mailboxes. :-)
I did look at it the first time it appeared. It seemed to be okay, but
I haven't tried any testing.
Alan Stern
^ permalink raw reply
* Re: [PATCH 1/3] cpufreq: Manage only online cpus
From: Viresh Kumar @ 2012-12-16 13:37 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linaro-dev, nicolas.pitre, amit.kucheria, mathieu.poirier,
linux-kernel, cpufreq, pdsw-power-team, linux-pm
In-Reply-To: <4137114.myBUIQvsg3@vostro.rjw.lan>
On 16 December 2012 18:34, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, December 16, 2012 11:20:08 AM Viresh Kumar wrote:
>> cpufreq core doesn't manage offline cpus and if driver->init() has returned
>> mask including offline cpus, it may result in unwanted behavior by cpufreq core
>> or governors.
>>
>> We need to get only online cpus in this mask. There are two places to fix this
>> mask, cpufreq core and cpufreq driver. It makes sense to do this at common place
>> and hence is done in core.
>
> Well, this series makes sense to me, but I'd like to hear what the other people
> think.
That sounds great :)
Some more information for others about how i reached to these issues
is present here:
https://lkml.org/lkml/2012/12/10/44
^ permalink raw reply
* [PATCH] ACPI / PM: Do not apply ACPI_SUCCESS() to acpi_bus_get_device() result
From: Rafael J. Wysocki @ 2012-12-16 13:32 UTC (permalink / raw)
To: ACPI Devel Maling List; +Cc: LKML, Linux PM list, Mika Westerberg
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since the return value of acpi_bus_get_device() is not of type
acpi_status, ACPI_SUCCESS() should not be used for checking its
return value. Fix that.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/device_pm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux/drivers/acpi/device_pm.c
===================================================================
--- linux.orig/drivers/acpi/device_pm.c
+++ linux/drivers/acpi/device_pm.c
@@ -358,8 +358,7 @@ static struct acpi_device *acpi_dev_pm_g
acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
struct acpi_device *adev;
- return handle && ACPI_SUCCESS(acpi_bus_get_device(handle, &adev)) ?
- adev : NULL;
+ return handle && !acpi_bus_get_device(handle, &adev) ? adev : NULL;
}
/**
^ permalink raw reply
* Re: [PATCH 1/3] cpufreq: Manage only online cpus
From: Rafael J. Wysocki @ 2012-12-16 13:04 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-dev, nicolas.pitre, amit.kucheria, mathieu.poirier,
linux-kernel, cpufreq, pdsw-power-team, linux-pm
In-Reply-To: <98330b2deb910453a356404b8cf774c94326bc42.1355636864.git.viresh.kumar@linaro.org>
On Sunday, December 16, 2012 11:20:08 AM Viresh Kumar wrote:
> cpufreq core doesn't manage offline cpus and if driver->init() has returned
> mask including offline cpus, it may result in unwanted behavior by cpufreq core
> or governors.
>
> We need to get only online cpus in this mask. There are two places to fix this
> mask, cpufreq core and cpufreq driver. It makes sense to do this at common place
> and hence is done in core.
Well, this series makes sense to me, but I'd like to hear what the other people
think.
Thanks,
Rafael
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1f93dbd..de99517 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -970,6 +970,13 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> pr_debug("initialization failed\n");
> goto err_unlock_policy;
> }
> +
> + /*
> + * affected cpus must always be the one, which are online. We aren't
> + * managing offline cpus here.
> + */
> + cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
> +
> policy->user_policy.min = policy->min;
> policy->user_policy.max = policy->max;
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH 3/3] cpufreq: Don't use cpu removed during cpufreq_driver_unregister
From: Viresh Kumar @ 2012-12-16 5:50 UTC (permalink / raw)
To: rjw, rafael.j.wysocki
Cc: linaro-dev, nicolas.pitre, amit.kucheria, mathieu.poirier,
linux-kernel, cpufreq, pdsw-power-team, linux-pm, Viresh Kumar
In-Reply-To: <98330b2deb910453a356404b8cf774c94326bc42.1355636864.git.viresh.kumar@linaro.org>
This is how the core works:
cpufreq_driver_unregister()
- subsys_interface_unregister()
- for_each_cpu() call cpufreq_remove_dev(), i.e. 0,1,2,3,4 when we
unregister.
cpufreq_remove_dev():
- Remove policy node
- Call cpufreq_add_dev() for next cpu, sharing mask with removed cpu.
i.e. When cpu 0 is removed, we call it for cpu 1. And when called for cpu 2,
we call it for cpu 3.
- cpufreq_add_dev() would call cpufreq_driver->init()
- init would return mask as AND of 2, 3 and 4 for cluster A7.
- cpufreq core would do online_cpu && policy->cpus
Here is the BUG(). Because cpu hasn't died but we have just unregistered
the cpufreq driver, online cpu would still have cpu 2 in it. And so thing
go bad again.
Solution: Keep cpumask of cpus that are registered with cpufreq core and clear
cpus when we get a call from subsys_interface_unregister() via
cpufreq_remove_dev().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a0a33bd..271d3be 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -47,6 +47,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
#endif
static DEFINE_SPINLOCK(cpufreq_driver_lock);
+/* Used when we unregister cpufreq driver */
+struct cpumask cpufreq_online_mask;
+
/*
* cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure
* all cpufreq/hotplug/workqueue/etc related lock issues.
@@ -981,6 +984,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
* managing offline cpus here.
*/
cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
+ cpumask_and(policy->cpus, policy->cpus, &cpufreq_online_mask);
policy->user_policy.min = policy->min;
policy->user_policy.max = policy->max;
@@ -1064,7 +1068,6 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
}
per_cpu(cpufreq_cpu_data, cpu) = NULL;
-
#ifdef CONFIG_SMP
/* if this isn't the CPU which is the parent of the kobj, we
* only need to unlink, put and exit
@@ -1185,6 +1188,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
if (unlikely(lock_policy_rwsem_write(cpu)))
BUG();
+ cpumask_clear_cpu(cpu, &cpufreq_online_mask);
retval = __cpufreq_remove_dev(dev, sif);
return retval;
}
@@ -1903,6 +1907,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
cpufreq_driver = driver_data;
spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ cpumask_setall(&cpufreq_online_mask);
+
ret = subsys_interface_register(&cpufreq_interface);
if (ret)
goto err_null_driver;
--
1.7.12.rc2.18.g61b472e
^ permalink raw reply related
* [PATCH 2/3] cpufreq: Notify governors when cpus are hot-[un]plugged
From: Viresh Kumar @ 2012-12-16 5:50 UTC (permalink / raw)
To: rjw, rafael.j.wysocki
Cc: linaro-dev, nicolas.pitre, amit.kucheria, mathieu.poirier,
linux-kernel, cpufreq, pdsw-power-team, linux-pm, Viresh Kumar
In-Reply-To: <98330b2deb910453a356404b8cf774c94326bc42.1355636864.git.viresh.kumar@linaro.org>
Because cpufreq core and governors worry only about the online cpus, if a cpu is
hot [un]plugged, we must notify governors about it, otherwise be ready to expect
something unexpected.
We already have notifiers in the form of CPUFREQ_GOV_START/CPUFREQ_GOV_STOP, we
just need to call them now.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index de99517..a0a33bd 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -751,11 +751,16 @@ static int cpufreq_add_dev_policy(unsigned int cpu,
return -EBUSY;
}
+ __cpufreq_governor(managed_policy, CPUFREQ_GOV_STOP);
+
spin_lock_irqsave(&cpufreq_driver_lock, flags);
cpumask_copy(managed_policy->cpus, policy->cpus);
per_cpu(cpufreq_cpu_data, cpu) = managed_policy;
spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ __cpufreq_governor(managed_policy, CPUFREQ_GOV_START);
+ __cpufreq_governor(managed_policy, CPUFREQ_GOV_LIMITS);
+
pr_debug("CPU already managed, adding link\n");
ret = sysfs_create_link(&dev->kobj,
&managed_policy->kobj,
@@ -1066,8 +1071,13 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
*/
if (unlikely(cpu != data->cpu)) {
pr_debug("removing link\n");
+ __cpufreq_governor(data, CPUFREQ_GOV_STOP);
cpumask_clear_cpu(cpu, data->cpus);
spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+ __cpufreq_governor(data, CPUFREQ_GOV_START);
+ __cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+
kobj = &dev->kobj;
cpufreq_cpu_put(data);
unlock_policy_rwsem_write(cpu);
--
1.7.12.rc2.18.g61b472e
^ permalink raw reply related
* [PATCH 1/3] cpufreq: Manage only online cpus
From: Viresh Kumar @ 2012-12-16 5:50 UTC (permalink / raw)
To: rjw, rafael.j.wysocki
Cc: linaro-dev, nicolas.pitre, amit.kucheria, mathieu.poirier,
linux-kernel, cpufreq, pdsw-power-team, linux-pm, Viresh Kumar
cpufreq core doesn't manage offline cpus and if driver->init() has returned
mask including offline cpus, it may result in unwanted behavior by cpufreq core
or governors.
We need to get only online cpus in this mask. There are two places to fix this
mask, cpufreq core and cpufreq driver. It makes sense to do this at common place
and hence is done in core.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1f93dbd..de99517 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -970,6 +970,13 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
pr_debug("initialization failed\n");
goto err_unlock_policy;
}
+
+ /*
+ * affected cpus must always be the one, which are online. We aren't
+ * managing offline cpus here.
+ */
+ cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
+
policy->user_policy.min = policy->min;
policy->user_policy.max = policy->max;
--
1.7.12.rc2.18.g61b472e
^ permalink raw reply related
* Re: [Resend][PATCH] PM: Move disabling/enabling runtime PM to late suspend/early resume
From: Rafael J. Wysocki @ 2012-12-16 1:30 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Linux PM list, LKML, Jan-Matthias Braun, Alan Stern
In-Reply-To: <alpine.LNX.2.00.1212152215300.3487@pobox.suse.cz>
On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote:
> On Sat, 15 Dec 2012, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Currently, the PM core disables runtime PM for all devices right
> > after executing subsystem/driver .suspend() callbacks for them
> > and re-enables it right before executing subsystem/driver .resume()
> > callbacks for them. This may lead to problems when there are
> > two devices such that the .suspend() callback executed for one of
> > them depends on runtime PM working for the other. In that case,
> > if runtime PM has already been disabled for the second device,
> > the first one's .suspend() won't work correctly (and analogously
> > for resume).
> >
> > To make those issues go away, make the PM core disable runtime PM
> > for devices right before executing subsystem/driver .suspend_late()
> > callbacks for them and enable runtime PM for them right after
> > executing subsystem/driver .resume_early() callbacks for them. This
> > way the potential conflitcs between .suspend_late()/.resume_early()
> > and their runtime PM counterparts are still prevented from happening,
> > but the subtle ordering issues related to disabling/enabling runtime
> > PM for devices during system suspend/resume are much easier to avoid.
> >
> > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Hi Rafael,
>
> just curious what is the reason for resend? Do you want to gather more
> Acks before pushing this upstream?
Well, I thought that some people might actually look at it when they found it
again in their mailboxes. :-)
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [Resend][PATCH] PM: Move disabling/enabling runtime PM to late suspend/early resume
From: Jiri Kosina @ 2012-12-15 21:16 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM list, LKML, Jan-Matthias Braun, Alan Stern
In-Reply-To: <3307221.dy6xvn45xe@vostro.rjw.lan>
On Sat, 15 Dec 2012, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Currently, the PM core disables runtime PM for all devices right
> after executing subsystem/driver .suspend() callbacks for them
> and re-enables it right before executing subsystem/driver .resume()
> callbacks for them. This may lead to problems when there are
> two devices such that the .suspend() callback executed for one of
> them depends on runtime PM working for the other. In that case,
> if runtime PM has already been disabled for the second device,
> the first one's .suspend() won't work correctly (and analogously
> for resume).
>
> To make those issues go away, make the PM core disable runtime PM
> for devices right before executing subsystem/driver .suspend_late()
> callbacks for them and enable runtime PM for them right after
> executing subsystem/driver .resume_early() callbacks for them. This
> way the potential conflitcs between .suspend_late()/.resume_early()
> and their runtime PM counterparts are still prevented from happening,
> but the subtle ordering issues related to disabling/enabling runtime
> PM for devices during system suspend/resume are much easier to avoid.
>
> Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Hi Rafael,
just curious what is the reason for resend? Do you want to gather more
Acks before pushing this upstream?
Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] cpufreq_stats: fix race between stats allocation and first usage
From: Rafael J. Wysocki @ 2012-12-15 11:22 UTC (permalink / raw)
To: Konstantin Khlebnikov; +Cc: linux-kernel, cpufreq, linux-pm
In-Reply-To: <50CC205D.4040106@openvz.org>
On Saturday, December 15, 2012 11:01:49 AM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Friday, December 14, 2012 02:59:21 PM Konstantin Khlebnikov wrote:
> >> This patch forces complete struct cpufreq_stats allocation for all cpus before
> >> registering CPUFREQ_TRANSITION_NOTIFIER notifier, otherwise in some conditions
> >> cpufreq_stat_notifier_trans() can be called in the middle of stats allocation,
> >> in this case cpufreq_stats_table already exists, but stat->freq_table is NULL.
> >
> > I'll queue it up for submission as v3.8 material.
> >
> > Does it need to be marked as -stable material too?
>
> It's very old and rare bug. I think you can leave it as is.
OK
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] cpufreq_stats: fix race between stats allocation and first usage
From: Konstantin Khlebnikov @ 2012-12-15 7:01 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-kernel, cpufreq, linux-pm
In-Reply-To: <4367394.4dMqb5NnJH@vostro.rjw.lan>
Rafael J. Wysocki wrote:
> On Friday, December 14, 2012 02:59:21 PM Konstantin Khlebnikov wrote:
>> This patch forces complete struct cpufreq_stats allocation for all cpus before
>> registering CPUFREQ_TRANSITION_NOTIFIER notifier, otherwise in some conditions
>> cpufreq_stat_notifier_trans() can be called in the middle of stats allocation,
>> in this case cpufreq_stats_table already exists, but stat->freq_table is NULL.
>
> I'll queue it up for submission as v3.8 material.
>
> Does it need to be marked as -stable material too?
It's very old and rare bug. I think you can leave it as is.
>
> Rafael
>
>
>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>> Cc: Rafael J. Wysocki<rjw@sisk.pl>
>> Cc: cpufreq<cpufreq@vger.kernel.org>
>> Cc: linux-pm<linux-pm@vger.kernel.org>
>>
>> ---
>>
>> <1>[ 363.116198] BUG: unable to handle kernel NULL pointer dereference at (null)
>> <1>[ 363.116668] IP: [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
>> <4>[ 363.116977] PGD 23177e067 PUD 2349c1067 PMD 0
>> <4>[ 363.117151] Oops: 0000 [#1] SMP
>> <4>[ 363.117151] last sysfs file: /sys/module/freq_table/initstate
>> <4>[ 363.117151] CPU 5
>> <4>[ 363.117151] Modules linked in: cpufreq_stats(+)(U) [a lot] [last unloaded: umc]
>> <4>[ 363.117151]
>> <4>[ 363.117151] Pid: 1690, comm: kondemand/5 veid: 0 Tainted: P WC --------------- T 2.6.32-279.5.1.el6-042stab061.7-vz #112 042stab061_7 System manufacturer System Product Name/Crosshair IV Formula
>> <4>[ 363.117151] RIP: 0010:[<ffffffffa11a70e4>] [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
>> <4>[ 363.117151] RSP: 0018:ffff880234281920 EFLAGS: 00010246
>> <4>[ 363.117151] RAX: 00000000001e12e8 RBX: 0000000000000000 RCX: 00000000002ab980
>> <4>[ 363.117151] RDX: 0000000000000004 RSI: 0000000000000000 RDI: 0000000000000005
>> <4>[ 363.117151] RBP: ffff880234281940 R08: 0000000000000000 R09: 0000000000000000
>> <4>[ 363.117151] R10: 0000000000000000 R11: 2222222222222222 R12: ffff880218ce7400
>> <4>[ 363.117151] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>> <4>[ 363.117151] FS: 00007f499ffe0700(0000) GS:ffff880031000000(0000) knlGS:0000000000000000
>> <4>[ 363.117151] CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
>> <4>[ 363.117151] CR2: 0000000000000000 CR3: 0000000230af7000 CR4: 00000000000006e0
>> <4>[ 363.117151] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> <4>[ 363.117151] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>> <4>[ 363.117151] Process kondemand/5 (pid: 1690, veid: 0, threadinfo ffff880234280000, task ffff8802330c48c0)
>> <4>[ 363.117151] Stack:
>> <4>[ 363.117151] ffffffff810cf4f3 0000000000000001 00000000ffffffff ffffffffa11a7ac0
>> <4>[ 363.117151]<d> ffff880234281990 ffffffff815454a8 ffff880234281c80 0000000000000000
>> <4>[ 363.117151]<d> ffff880234281a10 ffffffff833be978 ffffffff833be8e0 0000000000000001
>> <4>[ 363.117151] Call Trace:
>> <4>[ 363.117151] [<ffffffff810cf4f3>] ? is_module_text_address+0x23/0x30
>> <4>[ 363.117151] [<ffffffff815454a8>] notifier_call_chain+0x58/0xb0
>> <4>[ 363.117151] [<ffffffff810a5a8d>] __srcu_notifier_call_chain+0x5d/0x90
>> <4>[ 363.117151] [<ffffffff810a5ad6>] srcu_notifier_call_chain+0x16/0x20
>> <4>[ 363.117151] [<ffffffff81442a0a>] cpufreq_notify_transition+0x12a/0x190
>> <4>[ 363.117151] [<ffffffffa026df08>] powernowk8_target+0x628/0xb30 [powernow_k8]
>> <4>[ 363.117151] [<ffffffff8144289b>] __cpufreq_driver_target+0x8b/0x90
>> <4>[ 363.117151] [<ffffffffa0279388>] do_dbs_timer+0x3b8/0x3bc [cpufreq_ondemand]
>> <4>[ 363.117151] [<ffffffffa0278fd0>] ? do_dbs_timer+0x0/0x3bc [cpufreq_ondemand]
>> <4>[ 363.117151] [<ffffffff81097df4>] worker_thread+0x264/0x440
>> <4>[ 363.117151] [<ffffffff81097da3>] ? worker_thread+0x213/0x440
>> <4>[ 363.117151] [<ffffffff81097b90>] ? worker_thread+0x0/0x440
>> <4>[ 363.117151] [<ffffffff8109f050>] ? autoremove_wake_function+0x0/0x40
>> <4>[ 363.117151] [<ffffffff81097b90>] ? worker_thread+0x0/0x440
>> <4>[ 363.117151] [<ffffffff8109e986>] kthread+0x96/0xa0
>> <4>[ 363.117151] [<ffffffff8100c34a>] child_rip+0xa/0x20
>> <4>[ 363.117151] [<ffffffff8100bc90>] ? restore_args+0x0/0x30
>> <4>[ 363.117151] [<ffffffff8109e8f0>] ? kthread+0x0/0xa0
>> <4>[ 363.117151] [<ffffffff8100c340>] ? child_rip+0x0/0x20
>> <4>[ 363.117151] Code: 89 f9 48 8b 0c cd 20 53 9c 81 4c 8b 24 08 4d 85 e4 74 d3 8b 4a 08 41 8b 54 24 10 45 8b 6c 24 18 85 d2 74 22 49 8b 74 24 28 31 db<3b> 0e 75 10 eb 1a 66 0f 1f 44 00 00 48 63 c3 3b 0c 86 74 0c 83
>> <1>[ 363.117151] RIP [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
>> <4>[ 363.117151] RSP<ffff880234281920>
>> <4>[ 363.117151] CR2: 0000000000000000
>> ---
>> drivers/cpufreq/cpufreq_stats.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
>> index e40e508..9d7732b 100644
>> --- a/drivers/cpufreq/cpufreq_stats.c
>> +++ b/drivers/cpufreq/cpufreq_stats.c
>> @@ -364,18 +364,21 @@ static int __init cpufreq_stats_init(void)
>> if (ret)
>> return ret;
>>
>> + register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
>> + for_each_online_cpu(cpu)
>> + cpufreq_update_policy(cpu);
>> +
>> ret = cpufreq_register_notifier(¬ifier_trans_block,
>> CPUFREQ_TRANSITION_NOTIFIER);
>> if (ret) {
>> cpufreq_unregister_notifier(¬ifier_policy_block,
>> CPUFREQ_POLICY_NOTIFIER);
>> + unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
>> + for_each_online_cpu(cpu)
>> + cpufreq_stats_free_table(cpu);
>> return ret;
>> }
>>
>> - register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
>> - for_each_online_cpu(cpu) {
>> - cpufreq_update_policy(cpu);
>> - }
>> return 0;
>> }
>> static void __exit cpufreq_stats_exit(void)
>>
^ permalink raw reply
* [PATCH] PM / QoS: Rename local variable in dev_pm_qos_add_ancestor_request()
From: Rafael J. Wysocki @ 2012-12-15 0:26 UTC (permalink / raw)
To: Linux PM list; +Cc: LKML
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Local variable 'error' in dev_pm_qos_add_ancestor_request() need
not contain error codes only, so rename it to 'ret'.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/base/power/qos.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Index: linux/drivers/base/power/qos.c
===================================================================
--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -542,19 +542,19 @@ int dev_pm_qos_add_ancestor_request(stru
struct dev_pm_qos_request *req, s32 value)
{
struct device *ancestor = dev->parent;
- int error = -ENODEV;
+ int ret = -ENODEV;
while (ancestor && !ancestor->power.ignore_children)
ancestor = ancestor->parent;
if (ancestor)
- error = dev_pm_qos_add_request(ancestor, req,
- DEV_PM_QOS_LATENCY, value);
+ ret = dev_pm_qos_add_request(ancestor, req,
+ DEV_PM_QOS_LATENCY, value);
- if (error < 0)
+ if (ret < 0)
req->dev = NULL;
- return error;
+ return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_qos_add_ancestor_request);
^ permalink raw reply
* [Resend][PATCH] PM: Move disabling/enabling runtime PM to late suspend/early resume
From: Rafael J. Wysocki @ 2012-12-15 0:25 UTC (permalink / raw)
To: Linux PM list; +Cc: LKML, Jan-Matthias Braun, Jiri Kosina, Alan Stern
In-Reply-To: <1988667.JSHu2WyJuF@vostro.rjw.lan>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Currently, the PM core disables runtime PM for all devices right
after executing subsystem/driver .suspend() callbacks for them
and re-enables it right before executing subsystem/driver .resume()
callbacks for them. This may lead to problems when there are
two devices such that the .suspend() callback executed for one of
them depends on runtime PM working for the other. In that case,
if runtime PM has already been disabled for the second device,
the first one's .suspend() won't work correctly (and analogously
for resume).
To make those issues go away, make the PM core disable runtime PM
for devices right before executing subsystem/driver .suspend_late()
callbacks for them and enable runtime PM for them right after
executing subsystem/driver .resume_early() callbacks for them. This
way the potential conflitcs between .suspend_late()/.resume_early()
and their runtime PM counterparts are still prevented from happening,
but the subtle ordering issues related to disabling/enabling runtime
PM for devices during system suspend/resume are much easier to avoid.
Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Documentation/power/runtime_pm.txt | 9 +++++----
drivers/base/power/main.c | 9 ++++-----
2 files changed, 9 insertions(+), 9 deletions(-)
Index: linux/drivers/base/power/main.c
===================================================================
--- linux.orig/drivers/base/power/main.c
+++ linux/drivers/base/power/main.c
@@ -513,6 +513,8 @@ static int device_resume_early(struct de
Out:
TRACE_RESUME(error);
+
+ pm_runtime_enable(dev);
return error;
}
@@ -589,8 +591,6 @@ static int device_resume(struct device *
if (!dev->power.is_suspended)
goto Unlock;
- pm_runtime_enable(dev);
-
if (dev->pm_domain) {
info = "power domain ";
callback = pm_op(&dev->pm_domain->ops, state);
@@ -930,6 +930,8 @@ static int device_suspend_late(struct de
pm_callback_t callback = NULL;
char *info = NULL;
+ __pm_runtime_disable(dev, false);
+
if (dev->power.syscore)
return 0;
@@ -1133,11 +1135,8 @@ static int __device_suspend(struct devic
Complete:
complete_all(&dev->power.completion);
-
if (error)
async_error = error;
- else if (dev->power.is_suspended)
- __pm_runtime_disable(dev, false);
return error;
}
Index: linux/Documentation/power/runtime_pm.txt
===================================================================
--- linux.orig/Documentation/power/runtime_pm.txt
+++ linux/Documentation/power/runtime_pm.txt
@@ -642,12 +642,13 @@ out the following operations:
* During system suspend it calls pm_runtime_get_noresume() and
pm_runtime_barrier() for every device right before executing the
subsystem-level .suspend() callback for it. In addition to that it calls
- pm_runtime_disable() for every device right after executing the
- subsystem-level .suspend() callback for it.
+ __pm_runtime_disable() with 'false' as the second argument for every device
+ right before executing the subsystem-level .suspend_late() callback for it.
* During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
- for every device right before and right after executing the subsystem-level
- .resume() callback for it, respectively.
+ for every device right after executing the subsystem-level .resume_early()
+ callback and right after executing the subsystem-level .resume() callback
+ for it, respectively.
7. Generic subsystem callbacks
^ permalink raw reply
* Re: [PATCH] PNP: Simplify setting of resources
From: Rafael J. Wysocki @ 2012-12-15 0:17 UTC (permalink / raw)
To: Witold Szczeponik
Cc: Linus Torvalds, Len Brown, Linux PM list, ACPI Devel Maling List,
LKML
In-Reply-To: <50C8A391.3010900@gmx.net>
On Wednesday, December 12, 2012 04:32:33 PM Witold Szczeponik wrote:
> This patch factors out the setting of PNP resources into one function which is
> then reused for all PNP resource types. This makes the code more concise and
> avoids duplication. The parameters "type" and "flags" are not used at the
> moment but may be used by follow-up patches. Placeholders for these patches
> can be found in the comment lines that contain the "TBD" marker.
>
> As the code does not make any changes to the ABI, no regressions are expected.
>
> NB: While at it, support for bus type resources is added.
>
> The patch is applied against Linux 3.7 as well as linux-pm.git/master
> as of 2012-12-12.
Both patches queued up for submission as v3.8 material later in the cycle.
Thanks,
Rafael
> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>
>
> Index: linux/drivers/pnp/interface.c
> ===================================================================
> --- linux.orig/drivers/pnp/interface.c
> +++ linux/drivers/pnp/interface.c
> @@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource
> return ret;
> }
>
> +static char *pnp_get_resource_value(char *buf,
> + unsigned long type,
> + resource_size_t *start,
> + resource_size_t *end,
> + unsigned long *flags)
> +{
> + if (start)
> + *start = 0;
> + if (end)
> + *end = 0;
> + if (flags)
> + *flags = 0;
> +
> + /* TBD: allow for disabled resources */
> +
> + buf = skip_spaces(buf);
> + if (start) {
> + *start = simple_strtoull(buf, &buf, 0);
> + if (end) {
> + buf = skip_spaces(buf);
> + if (*buf == '-') {
> + buf = skip_spaces(buf + 1);
> + *end = simple_strtoull(buf, &buf, 0);
> + } else
> + *end = *start;
> + }
> + }
> +
> + /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */
> +
> + return buf;
> +}
> +
> static ssize_t pnp_set_current_resources(struct device *dmdev,
> struct device_attribute *attr,
> const char *ubuf, size_t count)
> @@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources
> struct pnp_dev *dev = to_pnp_dev(dmdev);
> char *buf = (void *)ubuf;
> int retval = 0;
> - resource_size_t start, end;
>
> if (dev->status & PNP_ATTACHED) {
> retval = -EBUSY;
> @@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources
> goto done;
> }
> if (!strnicmp(buf, "set", 3)) {
> + resource_size_t start;
> + resource_size_t end;
> + unsigned long flags;
> +
> if (dev->active)
> goto done;
> buf += 3;
> @@ -357,42 +393,37 @@ static ssize_t pnp_set_current_resources
> while (1) {
> buf = skip_spaces(buf);
> if (!strnicmp(buf, "io", 2)) {
> - buf = skip_spaces(buf + 2);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_io_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "mem", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_mem_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "irq", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_irq_resource(dev, start, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "dma", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_dma_resource(dev, start, 0);
> - continue;
> - }
> - break;
> + buf = pnp_get_resource_value(buf + 2,
> + IORESOURCE_IO,
> + &start, &end,
> + &flags);
> + pnp_add_io_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "mem", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_MEM,
> + &start, &end,
> + &flags);
> + pnp_add_mem_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "irq", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_IRQ,
> + &start, NULL,
> + &flags);
> + pnp_add_irq_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "dma", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_DMA,
> + &start, NULL,
> + &flags);
> + pnp_add_dma_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "bus", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_BUS,
> + &start, &end,
> + NULL);
> + pnp_add_bus_resource(dev, start, end);
> + } else
> + break;
> }
> mutex_unlock(&pnp_res_mutex);
> goto done;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] Longhaul: Disable driver by default
From: Rafael J. Wysocki @ 2012-12-15 0:06 UTC (permalink / raw)
To: Rafał Bilski; +Cc: cpufreq, linux-pm, Dave Jones
In-Reply-To: <1451067.pE2ohDYtjy@vostro.rjw.lan>
On Wednesday, November 28, 2012 12:44:54 AM Rafael J. Wysocki wrote:
> On Tuesday, November 27, 2012 11:38:01 PM Rafał Bilski wrote:
> >
> > > On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
> > >> This is only solution I can think of. User decides if he wants this
> > >> driver on his machine. I don't have enough knowledge and time to find
> > >> the reason why same code works on some machines and doesn't on others
> > >> which use same, or very similar, chipset and processor.
> > > I always have problems with patches like this one, because they are pretty much
> > > guaranteed to make someone complain.
> > >
> > > Is there any way to blacklist the affected machine you have?
> > >
> > > Rafael
> > No. Also problem seems to be larger than one machine. Also weirder.
> > One user claims his processor can't run below some frequency even
> > if it should be perfectly capable of doing so. System in question
> > freezes straight away. In past on good system I could change
> > frequency at least a couple of times without any protection. Just by
> > a chance. I tried to investigate "weird CPU 0 not listed by the BIOS"
> > message, but I have nothing. Also I seem to have far less time for
> > anything than in the past.
>
> OK, I'll take the patch, then, but I won't include it in my first v3.8 pull
> request.
I'm queuing it up for submission as v3.8 material later in the cycle.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage
From: Rafael J. Wysocki @ 2012-12-15 0:03 UTC (permalink / raw)
To: Sivaram Nair
Cc: rafael.j.wysocki, daniel.lezcano, shuox.liu, akpm, yanmin_zhang,
linux-pm, linux-kernel
In-Reply-To: <1355491060-970-2-git-send-email-sivaramn@nvidia.com>
On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> cpuidle_state->power_usage is signed; so change the corresponding sysfs
> ops to output signed value instead of unsigned.
What's actually wrong with printing it as an unsigned int?
Rafael
> Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
> ---
> drivers/cpuidle/sysfs.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 3409429..2fc79cd 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -232,6 +232,13 @@ static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
> static ssize_t show_state_##_name(struct cpuidle_state *state, \
> struct cpuidle_state_usage *state_usage, char *buf) \
> { \
> + return sprintf(buf, "%d\n", state->_name);\
> +}
> +
> +#define define_show_state_u_function(_name) \
> +static ssize_t show_state_##_name(struct cpuidle_state *state, \
> + struct cpuidle_state_usage *state_usage, char *buf) \
> +{ \
> return sprintf(buf, "%u\n", state->_name);\
> }
>
> @@ -270,7 +277,7 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
> return sprintf(buf, "%s\n", state->_name);\
> }
>
> -define_show_state_function(exit_latency)
> +define_show_state_u_function(exit_latency)
> define_show_state_function(power_usage)
> define_show_state_ull_function(usage)
> define_show_state_ull_function(time)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/2] cpuidle: fix finding state with min power_usage
From: Rafael J. Wysocki @ 2012-12-15 0:02 UTC (permalink / raw)
To: Sivaram Nair
Cc: rafael.j.wysocki, len.brown, daniel.lezcano, khilman, ccross,
riel, youquan.song, akpm, shuox.liu, linux-pm, linux-kernel
In-Reply-To: <1355491060-970-1-git-send-email-sivaramn@nvidia.com>
On Friday, December 14, 2012 03:17:36 PM Sivaram Nair wrote:
> Since cpuidle_state.power_usage is a signed value, use INT_MAX (instead
> of -1) to init the local copies so that functions that tries to find
> cpuidle states with minimum power usage works correctly even if they use
> non-negative values.
I'm queuing this up for submission as v3.8 material.
Thanks,
Rafael
> Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
> ---
> drivers/cpuidle/cpuidle.c | 2 +-
> drivers/cpuidle/governors/menu.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 8df53dd..fb4a7dd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -70,7 +70,7 @@ int cpuidle_play_dead(void)
> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> int i, dead_state = -1;
> - int power_usage = -1;
> + int power_usage = INT_MAX;
>
> if (!drv)
> return -ENODEV;
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index bd40b94..20ea33a 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -312,7 +312,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> {
> struct menu_device *data = &__get_cpu_var(menu_devices);
> int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
> - int power_usage = -1;
> + int power_usage = INT_MAX;
> int i;
> int multiplier;
> struct timespec t;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] PCIe/PM: Do not suspend port if any subordinate device need PME polling
From: Rafael J. Wysocki @ 2012-12-14 23:43 UTC (permalink / raw)
To: Huang Ying; +Cc: Bjorn Helgaas, linux-kernel, linux-pci, linux-pm
In-Reply-To: <1355453531-23348-1-git-send-email-ying.huang@intel.com>
On Friday, December 14, 2012 10:52:11 AM Huang Ying wrote:
> In
>
> http://www.mail-archive.com/linux-usb@vger.kernel.org/msg07976.html
>
> Ulrich reported that his USB3 cardreader does not work reliably when
> connected to the USB3 port. It turns out that USB3 controller failed
> to be waken up when plugging in the USB3 cardreader. Further
> experiment found that the USB3 host controller can only be waken up
> via polling, while not via PME interrupt. But if the PCIe port that
> the USB3 host controller is connected is suspended, we can not poll
> the USB3 host controller because its config space is not accessible if
> the PCIe port is put into low power state.
>
> To solve the issue, the PCIe port will not be suspended if any
> subordinate device need PME polling.
>
> Reported-by: Ulrich Eckhardt <usb@uli-eckhardt.de>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Tested-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> Cc: stable@vger.kernel.org # 3.6+
> ---
> drivers/pci/pcie/portdrv_pci.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -134,10 +134,26 @@ static int pcie_port_runtime_resume(stru
> return 0;
> }
>
> +static int pci_dev_pme_poll(struct pci_dev *pdev, void *data)
> +{
> + int *pme_poll = data;
> + *pme_poll = *pme_poll || pdev->pme_poll;
I would write that as
*pme_poll ||= pdev->pme_poll;
It is not a big deal, though.
> + return 0;
> +}
> +
> static int pcie_port_runtime_idle(struct device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int pme_poll = false;
> +
> + /*
> + * If any subordinate device needs pme poll, we should keep
> + * the port in D0, because we need port in D0 to poll it.
> + */
> + pci_walk_bus(pdev->subordinate, pci_dev_pme_poll, &pme_poll);
> /* Delay for a short while to prevent too frequent suspend/resume */
> - pm_schedule_suspend(dev, 10);
> + if (!pme_poll)
> + pm_schedule_suspend(dev, 10);
> return -EBUSY;
> }
> #else
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] cpufreq_stats: fix race between stats allocation and first usage
From: Rafael J. Wysocki @ 2012-12-14 23:33 UTC (permalink / raw)
To: Konstantin Khlebnikov; +Cc: linux-kernel, cpufreq, linux-pm
In-Reply-To: <20121214105921.5139.502.stgit@zurg>
On Friday, December 14, 2012 02:59:21 PM Konstantin Khlebnikov wrote:
> This patch forces complete struct cpufreq_stats allocation for all cpus before
> registering CPUFREQ_TRANSITION_NOTIFIER notifier, otherwise in some conditions
> cpufreq_stat_notifier_trans() can be called in the middle of stats allocation,
> in this case cpufreq_stats_table already exists, but stat->freq_table is NULL.
I'll queue it up for submission as v3.8 material.
Does it need to be marked as -stable material too?
Rafael
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: cpufreq <cpufreq@vger.kernel.org>
> Cc: linux-pm <linux-pm@vger.kernel.org>
>
> ---
>
> <1>[ 363.116198] BUG: unable to handle kernel NULL pointer dereference at (null)
> <1>[ 363.116668] IP: [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
> <4>[ 363.116977] PGD 23177e067 PUD 2349c1067 PMD 0
> <4>[ 363.117151] Oops: 0000 [#1] SMP
> <4>[ 363.117151] last sysfs file: /sys/module/freq_table/initstate
> <4>[ 363.117151] CPU 5
> <4>[ 363.117151] Modules linked in: cpufreq_stats(+)(U) [a lot] [last unloaded: umc]
> <4>[ 363.117151]
> <4>[ 363.117151] Pid: 1690, comm: kondemand/5 veid: 0 Tainted: P WC --------------- T 2.6.32-279.5.1.el6-042stab061.7-vz #112 042stab061_7 System manufacturer System Product Name/Crosshair IV Formula
> <4>[ 363.117151] RIP: 0010:[<ffffffffa11a70e4>] [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
> <4>[ 363.117151] RSP: 0018:ffff880234281920 EFLAGS: 00010246
> <4>[ 363.117151] RAX: 00000000001e12e8 RBX: 0000000000000000 RCX: 00000000002ab980
> <4>[ 363.117151] RDX: 0000000000000004 RSI: 0000000000000000 RDI: 0000000000000005
> <4>[ 363.117151] RBP: ffff880234281940 R08: 0000000000000000 R09: 0000000000000000
> <4>[ 363.117151] R10: 0000000000000000 R11: 2222222222222222 R12: ffff880218ce7400
> <4>[ 363.117151] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> <4>[ 363.117151] FS: 00007f499ffe0700(0000) GS:ffff880031000000(0000) knlGS:0000000000000000
> <4>[ 363.117151] CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> <4>[ 363.117151] CR2: 0000000000000000 CR3: 0000000230af7000 CR4: 00000000000006e0
> <4>[ 363.117151] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> <4>[ 363.117151] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> <4>[ 363.117151] Process kondemand/5 (pid: 1690, veid: 0, threadinfo ffff880234280000, task ffff8802330c48c0)
> <4>[ 363.117151] Stack:
> <4>[ 363.117151] ffffffff810cf4f3 0000000000000001 00000000ffffffff ffffffffa11a7ac0
> <4>[ 363.117151] <d> ffff880234281990 ffffffff815454a8 ffff880234281c80 0000000000000000
> <4>[ 363.117151] <d> ffff880234281a10 ffffffff833be978 ffffffff833be8e0 0000000000000001
> <4>[ 363.117151] Call Trace:
> <4>[ 363.117151] [<ffffffff810cf4f3>] ? is_module_text_address+0x23/0x30
> <4>[ 363.117151] [<ffffffff815454a8>] notifier_call_chain+0x58/0xb0
> <4>[ 363.117151] [<ffffffff810a5a8d>] __srcu_notifier_call_chain+0x5d/0x90
> <4>[ 363.117151] [<ffffffff810a5ad6>] srcu_notifier_call_chain+0x16/0x20
> <4>[ 363.117151] [<ffffffff81442a0a>] cpufreq_notify_transition+0x12a/0x190
> <4>[ 363.117151] [<ffffffffa026df08>] powernowk8_target+0x628/0xb30 [powernow_k8]
> <4>[ 363.117151] [<ffffffff8144289b>] __cpufreq_driver_target+0x8b/0x90
> <4>[ 363.117151] [<ffffffffa0279388>] do_dbs_timer+0x3b8/0x3bc [cpufreq_ondemand]
> <4>[ 363.117151] [<ffffffffa0278fd0>] ? do_dbs_timer+0x0/0x3bc [cpufreq_ondemand]
> <4>[ 363.117151] [<ffffffff81097df4>] worker_thread+0x264/0x440
> <4>[ 363.117151] [<ffffffff81097da3>] ? worker_thread+0x213/0x440
> <4>[ 363.117151] [<ffffffff81097b90>] ? worker_thread+0x0/0x440
> <4>[ 363.117151] [<ffffffff8109f050>] ? autoremove_wake_function+0x0/0x40
> <4>[ 363.117151] [<ffffffff81097b90>] ? worker_thread+0x0/0x440
> <4>[ 363.117151] [<ffffffff8109e986>] kthread+0x96/0xa0
> <4>[ 363.117151] [<ffffffff8100c34a>] child_rip+0xa/0x20
> <4>[ 363.117151] [<ffffffff8100bc90>] ? restore_args+0x0/0x30
> <4>[ 363.117151] [<ffffffff8109e8f0>] ? kthread+0x0/0xa0
> <4>[ 363.117151] [<ffffffff8100c340>] ? child_rip+0x0/0x20
> <4>[ 363.117151] Code: 89 f9 48 8b 0c cd 20 53 9c 81 4c 8b 24 08 4d 85 e4 74 d3 8b 4a 08 41 8b 54 24 10 45 8b 6c 24 18 85 d2 74 22 49 8b 74 24 28 31 db <3b> 0e 75 10 eb 1a 66 0f 1f 44 00 00 48 63 c3 3b 0c 86 74 0c 83
> <1>[ 363.117151] RIP [<ffffffffa11a70e4>] cpufreq_stat_notifier_trans+0x64/0xf0 [cpufreq_stats]
> <4>[ 363.117151] RSP <ffff880234281920>
> <4>[ 363.117151] CR2: 0000000000000000
> ---
> drivers/cpufreq/cpufreq_stats.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index e40e508..9d7732b 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -364,18 +364,21 @@ static int __init cpufreq_stats_init(void)
> if (ret)
> return ret;
>
> + register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
> + for_each_online_cpu(cpu)
> + cpufreq_update_policy(cpu);
> +
> ret = cpufreq_register_notifier(¬ifier_trans_block,
> CPUFREQ_TRANSITION_NOTIFIER);
> if (ret) {
> cpufreq_unregister_notifier(¬ifier_policy_block,
> CPUFREQ_POLICY_NOTIFIER);
> + unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
> + for_each_online_cpu(cpu)
> + cpufreq_stats_free_table(cpu);
> return ret;
> }
>
> - register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
> - for_each_online_cpu(cpu) {
> - cpufreq_update_policy(cpu);
> - }
> return 0;
> }
> static void __exit cpufreq_stats_exit(void)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ 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