* Re: [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
From: Greg KH @ 2012-03-12 17:22 UTC (permalink / raw)
To: ShuoX Liu
Cc: Yanmin Zhang, H. Peter Anvin, Valentin, Eduardo,
Henrique de Moraes Holschuh, Brown, Len, Thomas Gleixner,
Andrew Morton, Ingo Molnar, Kleen, Andi,
linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org
In-Reply-To: <4F5DBFA8.3080300@intel.com>
On Mon, Mar 12, 2012 at 05:19:36PM +0800, ShuoX Liu wrote:
> I created a series to do this.
>
> [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
> [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state
> for debug purpose.
> [PATCH 3/3] cpupower: Update the cpupower tool for new debugfs
> entries of cpuidle.
>
> Below are patches.
The patch is line-wrapped and can not be applied :(
> --- /dev/null
> +++ b/drivers/cpuidle/debugfs.c
> @@ -0,0 +1,176 @@
> +/*
> + * debugfs.c - debugfs support
> + *
> + * (C) 2006-2007 Shaohua Li <shaohua.li@intel.com>
> + * (C) 2012 ShuoX Liu <shuox.liu@intel.com>
> + *
> + * This code is licenced under the GPL.
What version of the GPL?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Mark Brown @ 2012-03-12 18:11 UTC (permalink / raw)
To: Greg KH
Cc: Brown, Len, Yanmin Zhang, Andrew Morton, ShuoX Liu,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, andi.kleen, Ingo Molnar,
linux-pm@lists.linux-foundation.org, Thomas Gleixner
In-Reply-To: <20120306143935.GA23346@kroah.com>
On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> Do you know of any tools using these files? I have never heard of them,
> and I was told we should move these files years ago. So I don't think
> there should be any api issues.
powertop uses them.
^ permalink raw reply
* Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Greg KH @ 2012-03-12 19:29 UTC (permalink / raw)
To: Mark Brown
Cc: Yanmin Zhang, Brown, Len, ShuoX Liu, Ingo Molnar,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, andi.kleen, Thomas Gleixner,
linux-pm@lists.linux-foundation.org, Andrew Morton
In-Reply-To: <20120312181151.GE14020@sirena.org.uk>
On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
>
> > Do you know of any tools using these files? I have never heard of them,
> > and I was told we should move these files years ago. So I don't think
> > there should be any api issues.
>
> powertop uses them.
Ok, then we can't move them all.
We should then just move the ones that have multiple lines, as I'm
pretty sure powertop doesn't use them, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for debug purpose.
From: Matthew Garrett @ 2012-03-12 20:46 UTC (permalink / raw)
To: ShuoX Liu
Cc: Brown, Len, Yanmin Zhang, Greg KH, Ingo Molnar,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, Kleen, Andi, Thomas Gleixner,
linux-pm@lists.linux-foundation.org, Andrew Morton
In-Reply-To: <4F5DC01D.7040006@intel.com>
On Mon, Mar 12, 2012 at 05:21:33PM +0800, ShuoX Liu wrote:
> In addition, C state might have much impact on performance tuning,
> as it takes
> much time to enter/exit C states, which might delay interrupt
> processing. With
> the new debug option, developers could check if a deep C state could impact
> performance and how much impact it could cause.
Would anyone doing performance tuning not be using the cpu_dma_latency
interface?
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Henrique de Moraes Holschuh @ 2012-03-13 0:42 UTC (permalink / raw)
To: Yanmin Zhang
Cc: Brown, Len, ShuoX Liu, linux-kernel@vger.kernel.org,
Thomas Gleixner, H. Peter Anvin, Andrew Morton,
linux-pm@lists.linux-foundation.org, Ingo Molnar
In-Reply-To: <1330995854.1916.39.camel@ymzhang>
On Tue, 06 Mar 2012, Yanmin Zhang wrote:
> On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 05 Mar 2012, ShuoX Liu wrote:
> > > @@ -45,6 +46,7 @@ total 0
> > > /sys/devices/system/cpu/cpu0/cpuidle/state1:
> > > total 0
> > > -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> > > +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> > > -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> > > -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> > > -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> >
> > ...
> >
> > > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> > > index 3fe41fe..1eae29a 100644
> > > --- a/drivers/cpuidle/sysfs.c
> > > +++ b/drivers/cpuidle/sysfs.c
> > > @@ -222,6 +222,9 @@ struct cpuidle_state_attr {
> > > #define define_one_state_ro(_name, show) \
> > > static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0444,
> > > show, NULL)
> > >
> > > +#define define_one_state_rw(_name, show, store) \
> > > +static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644,
> > > show, store)
> > > +
> > > #define define_show_state_function(_name) \
> > > static ssize_t show_state_##_name(struct cpuidle_state *state, \
> > > struct cpuidle_state_usage *state_usage, char *buf) \
> > > @@ -229,6 +232,19 @@ static ssize_t show_state_##_name(struct
> > > cpuidle_state *state, \
> > > return sprintf(buf, "%u\n", state->_name);\
> > > }
> > >
> > > +#define define_store_state_function(_name) \
> > > +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> > > + const char *buf, size_t size) \
> > > +{ \
> > > + int value; \
> > > + sscanf(buf, "%d", &value); \
> > > + if (value) \
> > > + state->disable = 1; \
> > > + else \
> > > + state->disable = 0; \
> > > + return size; \
> > > +}
> >
> > Isn't this missing a check for capabilities? Disabling cpuidle states is
> > not something random Joe (and IMHO that does mean random capability-
> > restricted Joe root) should be doing...
> Sorry. Could you elaborate it?
Sure. Should any user be able to disable a C state, therefore causing
the system to consume more power?
I am pretty sure the answer is NO, in which case you should check for
the appropriate user credentials before you allow a write to these
"debug" controls to succeed. "capability" here is one of the CAP_*
capabilities tested through capable(), which are supposed to limit even
root.
> > Also, maybe it would be best to use one of the lib helpers to parse that
> > value, so that it will be less annoying to userspace (trim blanks, complain
> > if there is trailing junk after trimming, etc)?
> We would use strict_strtol to parse the value in next version.
Thanks!
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
^ permalink raw reply
* Re: [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Yanmin Zhang @ 2012-03-13 1:18 UTC (permalink / raw)
To: Henrique de Moraes Holschuh
Cc: ShuoX Liu, Deepthi Dharwar, Andrew Morton,
linux-kernel@vger.kernel.org, Brown, Len, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin,
linux-pm@lists.linux-foundation.org
In-Reply-To: <20120313004229.GA21111@khazad-dum.debian.net>
On Mon, 2012-03-12 at 21:42 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 06 Mar 2012, Yanmin Zhang wrote:
> > On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh wrote:
> > > On Mon, 05 Mar 2012, ShuoX Liu wrote:
> > > > @@ -45,6 +46,7 @@ total 0
> > > > /sys/devices/system/cpu/cpu0/cpuidle/state1:
> > > > total 0
> > > > -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> > > > +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> > > > -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> > > > -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> > > > -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> > >
> > > ...
> > >
> > > > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> > > > index 3fe41fe..1eae29a 100644
> > > > --- a/drivers/cpuidle/sysfs.c
> > > > +++ b/drivers/cpuidle/sysfs.c
> > > > @@ -222,6 +222,9 @@ struct cpuidle_state_attr {
> > > > #define define_one_state_ro(_name, show) \
> > > > static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0444,
> > > > show, NULL)
> > > >
> > > > +#define define_one_state_rw(_name, show, store) \
> > > > +static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644,
> > > > show, store)
> > > > +
> > > > #define define_show_state_function(_name) \
> > > > static ssize_t show_state_##_name(struct cpuidle_state *state, \
> > > > struct cpuidle_state_usage *state_usage, char *buf) \
> > > > @@ -229,6 +232,19 @@ static ssize_t show_state_##_name(struct
> > > > cpuidle_state *state, \
> > > > return sprintf(buf, "%u\n", state->_name);\
> > > > }
> > > >
> > > > +#define define_store_state_function(_name) \
> > > > +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> > > > + const char *buf, size_t size) \
> > > > +{ \
> > > > + int value; \
> > > > + sscanf(buf, "%d", &value); \
> > > > + if (value) \
> > > > + state->disable = 1; \
> > > > + else \
> > > > + state->disable = 0; \
> > > > + return size; \
> > > > +}
> > >
> > > Isn't this missing a check for capabilities? Disabling cpuidle states is
> > > not something random Joe (and IMHO that does mean random capability-
> > > restricted Joe root) should be doing...
> > Sorry. Could you elaborate it?
>
> Sure. Should any user be able to disable a C state, therefore causing
> the system to consume more power?
Here we use the simple way to check access. Only root could change it.
>
> I am pretty sure the answer is NO, in which case you should check for
> the appropriate user credentials before you allow a write to these
> "debug" controls to succeed. "capability" here is one of the CAP_*
> capabilities tested through capable(), which are supposed to limit even
> root.
We would add below check.
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
>
> > > Also, maybe it would be best to use one of the lib helpers to parse that
> > > value, so that it will be less annoying to userspace (trim blanks, complain
> > > if there is trailing junk after trimming, etc)?
> > We would use strict_strtol to parse the value in next version.
>
> Thanks!
>
^ permalink raw reply
* Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Yanmin Zhang @ 2012-03-13 1:36 UTC (permalink / raw)
To: Greg KH
Cc: Mark Brown, Brown, Len, ShuoX Liu, Ingo Molnar,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, andi.kleen, Thomas Gleixner,
linux-pm@lists.linux-foundation.org, Andrew Morton
In-Reply-To: <20120312192936.GC24873@kroah.com>
On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> >
> > > Do you know of any tools using these files? I have never heard of them,
> > > and I was told we should move these files years ago. So I don't think
> > > there should be any api issues.
> >
> > powertop uses them.
>
> Ok, then we can't move them all.
>
> We should then just move the ones that have multiple lines, as I'm
> pretty sure powertop doesn't use them, right?
All sys files under cpu/cpuXXX/cpuidle have single line. If we move
some files to debugfs and keep others under sysfs, users might be confused.
Should we go back to the 1st version which just adds the new entry to
sysfs?
In addition, should we move powertop to tools/?
Yanmin
^ permalink raw reply
* Re: [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
From: ShuoX Liu @ 2012-03-13 2:07 UTC (permalink / raw)
To: Greg KH
Cc: Yanmin Zhang, H. Peter Anvin, Valentin, Eduardo,
Henrique de Moraes Holschuh, Brown, Len, Thomas Gleixner,
Andrew Morton, Ingo Molnar, Kleen, Andi,
linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org
In-Reply-To: <20120312172258.GA29938@kroah.com>
On 2012年03月13日 01:22, Greg KH wrote:
> On Mon, Mar 12, 2012 at 05:19:36PM +0800, ShuoX Liu wrote:
>> I created a series to do this.
>>
>> [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
>> [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state
>> for debug purpose.
>> [PATCH 3/3] cpupower: Update the cpupower tool for new debugfs
>> entries of cpuidle.
>>
>> Below are patches.
>
> The patch is line-wrapped and can not be applied :(
Sorry that i made a mistake. I sent email with fresh email-client which
has the text-wrapping default. I will configure it.
I will send the patch in good format when needed.
>> --- /dev/null
>> +++ b/drivers/cpuidle/debugfs.c
>> @@ -0,0 +1,176 @@
>> +/*
>> + * debugfs.c - debugfs support
>> + *
>> + * (C) 2006-2007 Shaohua Li <shaohua.li@intel.com>
>> + * (C) 2012 ShuoX Liu <shuox.liu@intel.com>
>> + *
>> + * This code is licenced under the GPL.
>
> What version of the GPL?
GPLv2
>
> thanks,
>
> greg k-h
thanks,
Shuo
^ permalink raw reply
* Re: [PATCH 1/4] thermal: exynos: Add thermal interface support for linux thermal layer
From: Amit Kachhap @ 2012-03-13 4:22 UTC (permalink / raw)
To: R, Durgadoss
Cc: linux-pm@lists.linux-foundation.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
mjg59@srcf.ucam.org, linux-acpi@vger.kernel.org, lenb@kernel.org,
linaro-dev@lists.linaro.org, lm-sensors@lm-sensors.org,
eduardo.valentin@ti.com, patches@linaro.org
In-Reply-To: <4D68720C2E767A4AA6A8796D42C8EB5904B13D@BGSMSX101.gar.corp.intel.com>
Hi Durgadoss,
Thanks for the detailed review.
On 12 March 2012 16:21, R, Durgadoss <durgadoss.r@intel.com> wrote:
> Hi Amit,
>
> Thanks for keeping this up. And Sorry for late reply.
>
>> -----Original Message-----
>> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel
>> Kachhap
>> Sent: Saturday, March 03, 2012 4:36 PM
>> To: linux-pm@lists.linux-foundation.org; linux-samsung-soc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux-
>> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; lm-
>> sensors@lm-sensors.org; amit.kachhap@linaro.org; eduardo.valentin@ti.com; R,
>> Durgadoss; patches@linaro.org
>> Subject: [PATCH 1/4] thermal: exynos: Add thermal interface support for linux
>> thermal layer
>>
>> This codes uses the generic linux thermal layer and creates a bridge
>> between temperature sensors, linux thermal framework and cooling devices
>> for samsung exynos platform. This layer recieves or monitor the
>> temperature from the sensor and informs the generic thermal layer to take
>> the necessary cooling action.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>> ---
>> drivers/thermal/Kconfig | 8 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/exynos_thermal.c | 272 ++++++++++++++++++++++++++++++++++++++
>> include/linux/exynos_thermal.h | 72 ++++++++++
>> 4 files changed, 353 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/thermal/exynos_thermal.c
>> create mode 100644 include/linux/exynos_thermal.h
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 298c1cd..4e8df56 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -29,3 +29,11 @@ config CPU_THERMAL
>> This will be useful for platforms using the generic thermal interface
>> and not the ACPI interface.
>> If you want this support, you should say Y or M here.
>> +
>> +config SAMSUNG_THERMAL_INTERFACE
>> + bool "Samsung Thermal interface support"
>> + depends on THERMAL && CPU_THERMAL
>> + help
>> + This is a samsung thermal interface which will be used as
>> + a link between sensors and cooling devices with linux thermal
>> + framework.
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 655cbc4..c67b6b2 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -4,3 +4,4 @@
>>
>> obj-$(CONFIG_THERMAL) += thermal_sys.o
>> obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
>> +obj-$(CONFIG_SAMSUNG_THERMAL_INTERFACE) += exynos_thermal.o
>> diff --git a/drivers/thermal/exynos_thermal.c
>> b/drivers/thermal/exynos_thermal.c
>> new file mode 100644
>> index 0000000..878d45c
>> --- /dev/null
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -0,0 +1,272 @@
>> +/* linux/drivers/thermal/exynos_thermal.c
>> + *
>> + * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/cpu_cooling.h>
>> +#include <linux/exynos_thermal.h>
>> +
>> +#define MAX_COOLING_DEVICE 4
>> +struct exynos4_thermal_zone {
>> + unsigned int idle_interval;
>> + unsigned int active_interval;
>> + struct thermal_zone_device *therm_dev;
>> + struct thermal_cooling_device *cool_dev[MAX_COOLING_DEVICE];
>> + unsigned int cool_dev_size;
>> + struct platform_device *exynos4_dev;
>> + struct thermal_sensor_conf *sensor_conf;
>> +};
>> +
>> +static struct exynos4_thermal_zone *th_zone;
>> +
>> +static int exynos4_get_mode(struct thermal_zone_device *thermal,
>> + enum thermal_device_mode *mode)
>> +{
>> + if (th_zone->sensor_conf) {
>> + pr_info("Temperature sensor not initialised\n");
>> + *mode = THERMAL_DEVICE_DISABLED;
>> + } else
>> + *mode = THERMAL_DEVICE_ENABLED;
>> + return 0;
>> +}
>> +
>> +static int exynos4_set_mode(struct thermal_zone_device *thermal,
>> + enum thermal_device_mode mode)
>> +{
>> + if (!th_zone->therm_dev) {
>> + pr_notice("thermal zone not registered\n");
>> + return 0;
>> + }
>> + if (mode == THERMAL_DEVICE_ENABLED)
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->active_interval*1000;
>> + else
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->idle_interval*1000;
>
> If it is 'DISABLED' mode, shouldn't the polling delay be just 0 ?
Yes Ideally this should be zero. But I wanted thermal monitoring to
always happen with some long interval in case of error scenarios.
Anyway I will check this again.
>
>> +
>> + thermal_zone_device_update(th_zone->therm_dev);
>> + pr_info("thermal polling set for duration=%d sec\n",
>> + th_zone->therm_dev->polling_delay/1000);
>> + return 0;
>> +}
>> +
>> +/*This may be called from interrupt based temperature sensor*/
>> +void exynos4_report_trigger(void)
>> +{
>> + unsigned int monitor_temp;
>> +
>> + if (!th_zone || !th_zone->therm_dev)
>> + return;
>> +
>> + monitor_temp = th_zone->sensor_conf->trip_data.trip_val[0];
>
> Why are we checking only against the 0-th trip point ?
> Why not for other trip_val[i] also ?
Yes correct. Actually the trip temperatures are arranged in ascending order.
>
>> +
>> + thermal_zone_device_update(th_zone->therm_dev);
>> +
>> + mutex_lock(&th_zone->therm_dev->lock);
>> + if (th_zone->therm_dev->last_temperature > monitor_temp)
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->active_interval*1000;
>> + else
>> + th_zone->therm_dev->polling_delay =
>> + th_zone->idle_interval*1000;
>> +
>> + kobject_uevent(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE);
>
> Wouldn't it make more sense to pass the trip point id also as an 'env'
> parameter ? This way, the user space can easily figure out which trip
> point has been crossed.
Its a good suggestion. I will check if some uevent property allows that.
>
>> + mutex_unlock(&th_zone->therm_dev->lock);
>> +}
>> +
>> +static int exynos4_get_trip_type(struct thermal_zone_device *thermal, int
>> trip,
>> + enum thermal_trip_type *type)
>> +{
>> + if (trip == 0 || trip == 1)
>> + *type = THERMAL_TRIP_STATE_ACTIVE;
>> + else if (trip == 2)
>> + *type = THERMAL_TRIP_CRITICAL;
>> + else
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos4_get_trip_temp(struct thermal_zone_device *thermal, int
>> trip,
>> + unsigned long *temp)
>> +{
>> + /*Monitor zone*/
>> + if (trip == 0)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[0];
>> + /*Warn zone*/
>> + else if (trip == 1)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[1];
>> + /*Panic zone*/
>> + else if (trip == 2)
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[2];
>> + else
>> + return -EINVAL;
>> + /*convert the temperature into millicelsius*/
>> + *temp = *temp * 1000;
>> +
>> + return 0;
>> +}
>
> This could be:
>
> if (trip < 0 || trip >2) return -EINVAL;
> *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
> return 0;
Agreed. Will apply.
>
>> +
>> +static int exynos4_get_crit_temp(struct thermal_zone_device *thermal,
>> + unsigned long *temp)
>> +{
>> + /*Panic zone*/
>> + *temp = th_zone->sensor_conf->trip_data.trip_val[2];
>> + /*convert the temperature into millicelsius*/
>> + *temp = *temp * 1000;
>> + return 0;
>> +}
>
> Why not make it, exynos4_get_trip_temp(thermal, 2, temp) ?
OK.
>
>> +
>> +static int exynos4_bind(struct thermal_zone_device *thermal,
>> + struct thermal_cooling_device *cdev)
>> +{
>> + /* if the cooling device is the one from exynos4 bind it */
>> + if (cdev != th_zone->cool_dev[0])
>> + return 0;
>> +
>> + if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
>> + pr_err("error binding cooling dev\n");
>> + return -EINVAL;
>> + }
>> + if (thermal_zone_bind_cooling_device(thermal, 1, cdev)) {
>> + pr_err("error binding cooling dev\n");
>> + return -EINVAL;
>
> If we fail here, do you want to remove the earlier 'binding' also ?
Yes. Missed this error handling.
>
>> + }
>> +
>> + return 0;
>> +
>> +}
>> +
>> +static int exynos4_unbind(struct thermal_zone_device *thermal,
>> + struct thermal_cooling_device *cdev)
>> +{
>> + if (cdev != th_zone->cool_dev[0])
>> + return 0;
>> +
>> + if (thermal_zone_unbind_cooling_device(thermal, 0, cdev)) {
>> + pr_err("error unbinding cooling dev\n");
>> + return -EINVAL;
>
> I think we should still go ahead and try to 'unbind' the other one.
Yes. Missed this error handling.
>
>> + }
>> + if (thermal_zone_unbind_cooling_device(thermal, 1, cdev)) {
>> + pr_err("error unbinding cooling dev\n");
>> + return -EINVAL;
>> + }
>> + return 0;
>> +
>> +}
>> +
>> +static int exynos4_get_temp(struct thermal_zone_device *thermal,
>> + unsigned long *temp)
>> +{
>> + void *data;
>> +
>> + if (!th_zone->sensor_conf) {
>> + pr_info("Temperature sensor not initialised\n");
>> + return -EINVAL;
>> + }
>> + data = th_zone->sensor_conf->private_data;
>> + *temp = th_zone->sensor_conf->read_temperature(data);
>> + /*convert the temperature into millicelsius*/
>> + *temp = *temp * 1000;
>> + return 0;
>> +}
>> +
>> +/* bind callback functions to thermalzone */
>> +static struct thermal_zone_device_ops exynos4_dev_ops = {
>> + .bind = exynos4_bind,
>> + .unbind = exynos4_unbind,
>> + .get_temp = exynos4_get_temp,
>> + .get_mode = exynos4_get_mode,
>> + .set_mode = exynos4_set_mode,
>> + .get_trip_type = exynos4_get_trip_type,
>> + .get_trip_temp = exynos4_get_trip_temp,
>> + .get_crit_temp = exynos4_get_crit_temp,
>> +};
>> +
>> +int exynos4_register_thermal(struct thermal_sensor_conf *sensor_conf)
>> +{
>> + int ret, count, tab_size;
>> + struct freq_pctg_table *tab_ptr;
>> +
>> + if (!sensor_conf || !sensor_conf->read_temperature) {
>> + pr_err("Temperature sensor not initialised\n");
>> + return -EINVAL;
>> + }
>> +
>> + th_zone = kzalloc(sizeof(struct exynos4_thermal_zone), GFP_KERNEL);
>> + if (!th_zone) {
>> + ret = -ENOMEM;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->sensor_conf = sensor_conf;
>> +
>> + tab_ptr = (struct freq_pctg_table *)sensor_conf->cooling_data.freq_data;
>> + tab_size = sensor_conf->cooling_data.freq_pctg_count;
>> +
>> + /*Register the cpufreq cooling device*/
>> + th_zone->cool_dev_size = 1;
>> + count = 0;
>> + th_zone->cool_dev[count] = cpufreq_cooling_register(
>> + (struct freq_pctg_table *)&(tab_ptr[count]),
>> + tab_size, cpumask_of(0));
>> +
>> + if (IS_ERR(th_zone->cool_dev[count])) {
>> + pr_err("Failed to register cpufreq cooling device\n");
>> + ret = -EINVAL;
>> + th_zone->cool_dev_size = 0;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
>> + 3, NULL, &exynos4_dev_ops, 0, 0, 0, 1000);
>> + if (IS_ERR(th_zone->therm_dev)) {
>> + pr_err("Failed to register thermal zone device\n");
>> + ret = -EINVAL;
>> + goto err_unregister;
>> + }
>> +
>> + th_zone->active_interval = 1;
>> + th_zone->idle_interval = 10;
>> +
>> + exynos4_set_mode(th_zone->therm_dev, THERMAL_DEVICE_DISABLED);
>> +
>> + pr_info("Exynos: Kernel Thermal management registered\n");
>> +
>> + return 0;
>> +
>> +err_unregister:
>> + exynos4_unregister_thermal();
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(exynos4_register_thermal);
>> +
>> +void exynos4_unregister_thermal(void)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < th_zone->cool_dev_size; i++) {
>> + if (th_zone && th_zone->cool_dev[i])
>> + cpufreq_cooling_unregister(th_zone->cool_dev[i]);
>> + }
>> +
>> + if (th_zone && th_zone->therm_dev)
>> + thermal_zone_device_unregister(th_zone->therm_dev);
>> +
>> + kfree(th_zone);
>> +
>> + pr_info("Exynos: Kernel Thermal management unregistered\n");
>> +}
>> +EXPORT_SYMBOL(exynos4_unregister_thermal);
>> diff --git a/include/linux/exynos_thermal.h b/include/linux/exynos_thermal.h
>> new file mode 100644
>> index 0000000..186e409
>> --- /dev/null
>> +++ b/include/linux/exynos_thermal.h
>> @@ -0,0 +1,72 @@
>> +/* linux/include/linux/exynos_thermal.h
>> + *
>> + * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#ifndef THERMAL_INTERFACE_H
>> +#define THERMAL_INTERFACE_H
>> +/* CPU Zone information */
>> +
>> +#define SENSOR_NAME_LEN 16
>> +#define MAX_TRIP_COUNT 8
>> +
>> +#define PANIC_ZONE 4
>> +#define WARN_ZONE 3
>> +#define MONITOR_ZONE 2
>> +#define SAFE_ZONE 1
>> +#define NO_ACTION 0
>
> I don't get why we need two separate SAFE and NO_ACTION
> zones..To me, both should be the same.
Yes not needed.
>
>> +
>> +
>> +struct thermal_trip_point_conf {
>> + int trip_val[MAX_TRIP_COUNT];
>> + int trip_count;
>> +};
>> +
>> +struct thermal_cooling_conf {
>> + struct freq_pctg_table freq_data[MAX_TRIP_COUNT];
>> + int freq_pctg_count;
>> +};
>> +
>> +/**
>> + * struct exynos4_tmu_platform_data
>> + * @name: name of the temperature sensor
>> + * @read_temperature: A function pointer to read temperature info
>> + * @private_data: Temperature sensor private data
>> + * @sensor_data: Sensor specific information like trigger temperature, level
>> + */
>> +struct thermal_sensor_conf {
>> + char name[SENSOR_NAME_LEN];
>> + int (*read_temperature)(void *data);
>> + struct thermal_trip_point_conf trip_data;
>> + struct thermal_cooling_conf cooling_data;
>
> Since both trip_count and freq_pctg_count are same at all
> times (please correct me if I am wrong) I think it's better
> to have a single 'count' variable inside this structure and move
> trip_val and freq_data to this structure directly.
No trip_count and pctg_count are different.
>
> One General Concern:
> Why do we even need this exynos_thermal layer between Thermal Framework
> and the Thermal Sensor Drivers ? I think the thermal sensor driver (in
> this case exynos_tmu.c) can directly register with the Thermal framework.
> This means, we will soon have platformXXX_thermal.c files for each
> platform, which is not really a good way to go IMHO.
> I also understand that the framework does not have alarm attributes,
> notification support etc..But We can add them if needed.
Yes even comments from Guenter Roeck and Mark Brown also point in the
same direction. Actually I wanted to separate the sensor h/w
implementation independent from thermal management algorithm so its
future modification may be clean. But looks like the agreement is to
merge them.
>
> I have reviewed your two sets of patches independently. My only
> request to you would be to post the next versions of both the patch
> sets at the same time, so that it becomes easier to understand and test.
Thanks even i also think this a good way.
>
> Thanks,
> Durga
>
>> + void *private_data;
>> +};
>> +
>> +/**
>> + * exynos4_register_thermal: Register to the exynos thermal interface.
>> + * @sensor_conf: Structure containing temperature sensor information
>> + *
>> + * returns zero on success, else negative errno.
>> + */
>> +int exynos4_register_thermal(struct thermal_sensor_conf *sensor_conf);
>> +
>> +/**
>> + * exynos4_unregister_thermal: Un-register from the exynos thermal interface.
>> + *
>> + * return not applicable.
>> + */
>> +void exynos4_unregister_thermal(void);
>> +
>> +/**
>> + * exynos4_report_trigger: Report any trigger level crossed in the
>> + * temperature sensor. This may be useful to take any cooling action.
>> + *
>> + * return not applicable.
>> + */
>> +extern void exynos4_report_trigger(void);
>> +#endif
>> --
>> 1.7.1
>
--
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 1/4] thermal: exynos: Add thermal interface support for linux thermal layer
From: R, Durgadoss @ 2012-03-13 4:48 UTC (permalink / raw)
To: Amit Kachhap
Cc: linux-samsung-soc@vger.kernel.org, linaro-dev@lists.linaro.org,
patches@linaro.org, linux-kernel@vger.kernel.org,
lm-sensors@lm-sensors.org, linux-acpi@vger.kernel.org,
linux-pm@lists.linux-foundation.org
In-Reply-To: <CAK44p23NwjPe3Ku0tfFXCQzi+2fdgEipFf+oCVKk99sC9qVMBA@mail.gmail.com>
Hi Amit,
[snip.]
> >> +
> >> + kobject_uevent(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE);
> >
> > Wouldn't it make more sense to pass the trip point id also as an 'env'
> > parameter ? This way, the user space can easily figure out which trip
> > point has been crossed.
> Its a good suggestion. I will check if some uevent property allows that.
The kobject_uevent_env(...) call allows that. The third argument takes a
char *envp[]. For example, We could pass "trip=0" to indicate the trip point.
I should have mentioned this in my previous reply itself..missed it..
Thanks,
Durga
^ permalink raw reply
* Re: [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Amit Kachhap @ 2012-03-13 5:07 UTC (permalink / raw)
To: Sundar
Cc: len.brown, linaro-dev, patches, linux-kernel, linux-acpi,
linux-pm, rob.lee
In-Reply-To: <CAMHCuPUOnftKYaGjEpA9rZFc1yjH3U-EkEEUWqcFapSKROTopQ@mail.gmail.com>
On 11 March 2012 09:41, Sundar <sunder.svit@gmail.com> wrote:
> Hi Amit,
>
> I am new here; so please bear with my questions/doubts :)
>
> On Wed, Feb 22, 2012 at 3:44 PM, Amit Daniel Kachhap
> <amit.kachhap@linaro.org> wrote:
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using frequency scaling up/down based on the request
>> from user. Different cpu related cooling devices can be registered by the
>> user and the binding of these cooling devices to the corresponding
>
> "Different cpu related cooling devices": Do you mean cooling devices
> for different CPUs (num_cpus) or are you referring to different
> customers aka consumer drivers who could use this framework and impose
> the cooling.
Correct but the consumer driver only has to use the other
thermal-sys.c functions. Only register cpu cooling devices is
implemented in this work.
>
>> trip points can be easily done as the registration API's return the
>> cooling device pointer. The user of these api's are responsible for
>> passing clipping frequency in percentages.
>
> Why do you want to pass the clipping frequency in percentages? Wouldnt
> it be simpler for any platform sensor driver to just pass the
> frequency it wants to throttle the CPU?
Yes i also agree.
>
>> +
>> + This interface function registers the cpufreq cooling device with the name
>> + "thermal-cpufreq-%x". This api can support multiple instance of cpufreq cooling
>> + devices.
>
> When you refer to cooling devices, is it the number of instances
> per-CPU that is supported? Sorry I am unable to follow.
CPU cooling apis can be used as below
1)per cpus if different frequency clipping is needed. so multiple
instance of this api can be called.
2)for all the cpus as provided with mask when same frequency clipping
is needed. In this case single instance is fine.
>
>> + .polling_interval: polling interval for this cooling state.
>> + tab_size: the total number of cooling state.
>
> By cooling_state, I assume you are referring to the thermal state for
> the platform? or only for the CPU?
Yes thermal state of the platform.
>
>> + mask_val: all the allowed cpu's where frequency clipping can happen.
>
> Why should this be a new variable? The policy->affected_cpus should be
> the same as this IMO?
yes this should be same. I will check this.
>
>> + help
>> + This implements the generic cpu cooling mechanism through frequency
>> + reduction, cpu hotplug and any other ways of reducing temperature. An
>
> Apart from reducing the CPU frequency, (probably) CPU hotplug, what
> other means of reducing CPU temperature? Or are you referring to any
> platform temperature controls?
No only CPU related. Other control ways I thought was cpuidle with low
power states and cpu_power factor modifications used in the schedular.
>
> It isnt very clear from the documentation (at least to me) if this is
> only for CPU cooling or generic platform cooling.
>
> Cheers!
> --
> ---------
> The views expressed in this email are personal and do not necessarily
> echo my employers'.
^ permalink raw reply
* Re: mmc: sdhci-pci: why no .shutdown() implemented
From: Adrian Hunter @ 2012-03-13 5:51 UTC (permalink / raw)
To: Mansoor, Illyas
Cc: linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org
In-Reply-To: <810586B7581CC8469141DADEBC3719120AF0A2@BGSMSX102.gar.corp.intel.com>
On 12/03/12 16:30, Mansoor, Illyas wrote:
>>> On our platform we implemented a BUG in pci_set_power_state callback
>>> when the shutdown Is in progress, and we caught sdhci-pci doing
>>> pci_set_power_state(D0) when shutdown was Already in progress.
>>>
>>> My question, why doesn't sdhci-pci.c implement a .shutdown() callback
>>> and close the device After doing sys_sync()?
>>>
>>> Is there some reason behind it not doing a graceful shutdown.
>>
>> In general, the kernel does not know how to do a graceful shutdown. There may
>> be any number of housekeeping activities that userspace wishes to do before
>> shutting down.
>
> I see some storage drivers do implement .shutdown() callbacks do you think
> sdhci-pci
> could also do the same?
Certainly.
>
> immansoo@immansoo-desktop:~/Linux_kernel/linux-stable/drivers/mmc$ find . -name
> \*.c -exec grep \.shutdown -H {} \;
> ./host/s3cmci.c:static void s3cmci_shutdown(struct platform_device *pdev)
> ./host/s3cmci.c: s3cmci_shutdown(pdev);
> ./host/s3cmci.c: .shutdown = s3cmci_shutdown,
>
>>
>> If file systems are not getting sync'd that is a userspace problem, not a
> kernel
>> problem.
>
> I agree the issue needs to be fixed in user-space, so it does not request
> Storage device during shutdown.
>
> Shouldn't we also make sure we don't open a device during shutdown, that way we
> make
> sure at least we don't stay in device power'd up state during power-off.
>
> It's not that the file system is not getting sync'd, but issue is bringing up
> the device
> during shutdown, because once the shutdown starts there is no way to stop it
> and the device may be left in undefined state.
Power-off is not an undefined state.
What exactly is the problem?
>
>
>>
>>>
>>> I think it could cause corruption, since there is no guarantee when
>>> the system will power-off/reboot/halt
>>>
>>> I just implemented a patch to fix this, if it's a good fix I'll submit.
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pci.c
>>> b/drivers/mmc/host/sdhci-pci.c index f5fe05c..e0818c9 100644
>>> --- a/drivers/mmc/host/sdhci-pci.c
>>> +++ b/drivers/mmc/host/sdhci-pci.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/regulator/consumer.h> #include <linux/pm_runtime.h>
>>> #include <linux/async.h>
>>> +#include <linux/syscalls.h>
>>>
>>> #include <asm/scatterlist.h>
>>> #include <asm/io.h>
>>> @@ -1342,6 +1343,34 @@ err:
>>> return ret;
>>> }
>>>
>>> +static void sdhci_pci_shutdown(struct pci_dev *pdev) {
>>> + int i;
>>> + struct sdhci_pci_chip *chip;
>>> +
>>> + printk(KERN_INFO "%s: Syncing filesystems ... ", __func__);
>>> + sys_sync();
>>> + printk("done.\n");
>>> +
>>> + pm_runtime_get_sync(&pdev->dev);
>>> +
>>> + chip = pci_get_drvdata(pdev);
>>> +
>>> + if (chip) {
>>> + for (i = 0;i < chip->num_slots; i++)
>>> + sdhci_pci_remove_slot(chip->slots[i]);
>>> +
>>> + pci_set_drvdata(pdev, NULL);
>>> + kfree(chip);
>>> + }
>>> +
>>> + pci_disable_device(pdev);
>>> +
>>> + pm_runtime_put_noidle(&pdev->dev);
>>> + pm_runtime_forbid(&pdev->dev);
>>> + pm_runtime_disable(&pdev->dev); }
>>> +
>>> static void __devexit sdhci_pci_remove(struct pci_dev *pdev) {
>>> int i;
>>> @@ -1473,6 +1502,7 @@ static struct pci_driver sdhci_driver = {
>>> .id_table = pci_ids,
>>> .probe = sdhci_pci_probe,
>>> .remove = __devexit_p(sdhci_pci_remove),
>>> + .shutdown = sdhci_pci_shutdown,
>>>
>>>
>>> -Illyas
>
^ permalink raw reply
* Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Sundar @ 2012-03-13 9:45 UTC (permalink / raw)
To: Amit Kachhap
Cc: mark.gross, len.brown, linux-pm, linaro-dev, patches,
linux-kernel, linux-acpi, rob.lee
In-Reply-To: <CAK44p23nZ0kgdieasMkChV8B51Rjt=KrmypN+ksV+nym_e0Cqw@mail.gmail.com>
Hi Amit,
Thanks for the replies. One more query
On Tue, Mar 13, 2012 at 10:37 AM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>> "Different cpu related cooling devices": Do you mean cooling devices
>> for different CPUs (num_cpus) or are you referring to different
>> customers aka consumer drivers who could use this framework and impose
>> the cooling.
> Correct but the consumer driver only has to use the other
> thermal-sys.c functions. Only register cpu cooling devices is
> implemented in this work.
Am i right in noting that this framework then doesnt handle other
devices for cooling like Graphics, LCD etc?
Cheers!
--
---------
The views expressed in this email are personal and do not necessarily
echo my employers.
^ permalink raw reply
* Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Amit Kucheria @ 2012-03-13 10:00 UTC (permalink / raw)
To: Sundar
Cc: Amit Kachhap, len.brown, linaro-dev, patches, linux-kernel,
linux-acpi, mark.gross, linux-pm
In-Reply-To: <CAMHCuPXSCZjiyW28aPPjO4zF0vgabZUrqtUK5nMXLwNzSWUjQA@mail.gmail.com>
Sundar,
On Tue, Mar 13, 2012 at 11:45 AM, Sundar <sunder.svit@gmail.com> wrote:
> Hi Amit,
>
> Thanks for the replies. One more query
>
> On Tue, Mar 13, 2012 at 10:37 AM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>
>>> "Different cpu related cooling devices": Do you mean cooling devices
>>> for different CPUs (num_cpus) or are you referring to different
>>> customers aka consumer drivers who could use this framework and impose
>>> the cooling.
>> Correct but the consumer driver only has to use the other
>> thermal-sys.c functions. Only register cpu cooling devices is
>> implemented in this work.
>
> Am i right in noting that this framework then doesnt handle other
> devices for cooling like Graphics, LCD etc?
At the moment it doesn't. But there was some discussion around
creating something that will work with devfreq. This would allow
peripheral drivers to be plugged in as well. Amit is investigating
that at present.
/Amit
^ permalink raw reply
* Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Sundar @ 2012-03-13 10:14 UTC (permalink / raw)
To: Amit Kucheria
Cc: Amit Kachhap, len.brown, linaro-dev, patches, linux-kernel,
linux-acpi, mark.gross, linux-pm
In-Reply-To: <CAP245DUqz-N3nG+bc-3b=4YgYk3LMJTWk_wB7Db1ZBm0fBQOSQ@mail.gmail.com>
On Tue, Mar 13, 2012 at 3:30 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> Sundar,
Hi Amit,
> At the moment it doesn't. But there was some discussion around
> creating something that will work with devfreq. This would allow
> peripheral drivers to be plugged in as well. Amit is investigating
> that at present.
What if we work towards a generic constraint framework which models
thermals as a performance constraint.
Drivers can register to this constraint; platform code can then decide
to issue restrictions either to the CPU or other power-hungry
peripherals based on the platform conditions.
That also allows to model CPU frequency as a generic constraint but
via an actual consumer, say the thermal driver.
Cheers!
--
---------
The views expressed in this email are personal and do not necessarily
echo my employers.
^ permalink raw reply
* Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Amit Kachhap @ 2012-03-13 10:52 UTC (permalink / raw)
To: Sundar
Cc: Amit Kucheria, len.brown, linaro-dev, patches, linux-kernel,
linux-acpi, mark.gross, linux-pm
In-Reply-To: <CAMHCuPUQSPgt8mP2FY7hxi8345ZbLYi9NNbo5P24CDpZp50=_A@mail.gmail.com>
On 13 March 2012 15:44, Sundar <sunder.svit@gmail.com> wrote:
> On Tue, Mar 13, 2012 at 3:30 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>> Sundar,
>
> Hi Amit,
>
>> At the moment it doesn't. But there was some discussion around
>> creating something that will work with devfreq. This would allow
>> peripheral drivers to be plugged in as well. Amit is investigating
>> that at present.
>
> What if we work towards a generic constraint framework which models
> thermals as a performance constraint.
>
> Drivers can register to this constraint; platform code can then decide
> to issue restrictions either to the CPU or other power-hungry
> peripherals based on the platform conditions.
>
> That also allows to model CPU frequency as a generic constraint but
> via an actual consumer, say the thermal driver.
Yes that should be helpful. Even the things your are suggesting are
somewhat same with some patches submitted which sets cpufreq min/max
constraint.
>
> Cheers!
>
> --
> ---------
> The views expressed in this email are personal and do not necessarily
> echo my employers.
^ permalink raw reply
* Re: [linux-pm] [PATCH 2/4] thermal: Add generic cpufreq cooling implementation
From: Sundar @ 2012-03-13 11:43 UTC (permalink / raw)
To: Amit Kachhap
Cc: Amit Kucheria, len.brown, linaro-dev, patches, linux-kernel,
linux-acpi, mark.gross, linux-pm
In-Reply-To: <CAK44p224FTbnBXOo34TPgB2dKjT1x-rGbDB+kPitvNhoXy35Tg@mail.gmail.com>
On Tue, Mar 13, 2012 at 4:22 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
> Yes that should be helpful. Even the things your are suggesting are
> somewhat same with some patches submitted which sets cpufreq min/max
> constraint.
Hmm..let me see how soon can I code up a rough implementation!
Cheers!
--
---------
The views expressed in this email are personal and do not necessarily
echo my employers.
^ permalink raw reply
* Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Greg KH @ 2012-03-13 19:29 UTC (permalink / raw)
To: Yanmin Zhang
Cc: Mark Brown, Brown, Len, ShuoX Liu, Ingo Molnar,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, andi.kleen, Thomas Gleixner,
linux-pm@lists.linux-foundation.org, Andrew Morton
In-Reply-To: <1331602594.1916.155.camel@ymzhang>
On Tue, Mar 13, 2012 at 09:36:34AM +0800, Yanmin Zhang wrote:
> On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> > On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > >
> > > > Do you know of any tools using these files? I have never heard of them,
> > > > and I was told we should move these files years ago. So I don't think
> > > > there should be any api issues.
> > >
> > > powertop uses them.
> >
> > Ok, then we can't move them all.
> >
> > We should then just move the ones that have multiple lines, as I'm
> > pretty sure powertop doesn't use them, right?
> All sys files under cpu/cpuXXX/cpuidle have single line. If we move
> some files to debugfs and keep others under sysfs, users might be confused.
>
> Should we go back to the 1st version which just adds the new entry to
> sysfs?
Wait, I thought this whole thing started when we wanted to move the
files that had multiple lines out of sysfs?
If none of these do, and they all are being used by tools already, then
fine, they should stay.
But for some reason, I thought there was a problem here. Perhaps that
was in the cpufreq code?
confused,
greg k-h
^ permalink raw reply
* Re: [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Henrique de Moraes Holschuh @ 2012-03-13 20:49 UTC (permalink / raw)
To: Yanmin Zhang
Cc: Brown, Len, ShuoX Liu, linux-kernel@vger.kernel.org,
Thomas Gleixner, H. Peter Anvin, Andrew Morton,
linux-pm@lists.linux-foundation.org, Ingo Molnar
In-Reply-To: <1331601480.1916.149.camel@ymzhang>
On Tue, 13 Mar 2012, Yanmin Zhang wrote:
> > > > > +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> > > > > + const char *buf, size_t size) \
> > > > > +{ \
> > > > > + int value; \
> > > > > + sscanf(buf, "%d", &value); \
> > > > > + if (value) \
> > > > > + state->disable = 1; \
> > > > > + else \
> > > > > + state->disable = 0; \
> > > > > + return size; \
> > > > > +}
> > > >
> > > > Isn't this missing a check for capabilities? Disabling cpuidle states is
> > > > not something random Joe (and IMHO that does mean random capability-
> > > > restricted Joe root) should be doing...
> > > Sorry. Could you elaborate it?
> >
> > Sure. Should any user be able to disable a C state, therefore causing
> > the system to consume more power?
> Here we use the simple way to check access. Only root could change it.
Yea, but capabilities are supposed to constrain root as well :-)
> > I am pretty sure the answer is NO, in which case you should check for
> > the appropriate user credentials before you allow a write to these
> > "debug" controls to succeed. "capability" here is one of the CAP_*
> > capabilities tested through capable(), which are supposed to limit even
> > root.
> We would add below check.
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
Looks good.
Thanks!
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Kevin Hilman @ 2012-03-13 23:52 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Trinabh Gupta, Deepthi Dharwar, Daniel Lezcano,
linux-kernel, Amit Kucheria, Santosh Shilimkar, linux-tegra,
linux-pm, linux-omap, Arjan van de Ven, linux-arm-kernel
In-Reply-To: <4F1929E9.7070707@linaro.org>
Hi Colin,
On 12/21/2011 01:09 AM, Colin Cross wrote:
> To use coupled cpuidle states, a cpuidle driver must:
[...]
> Provide a struct cpuidle_state.enter function for each state
> that affects multiple cpus. This function is guaranteed to be
> called on all cpus at approximately the same time. The driver
> should ensure that the cpus all abort together if any cpu tries
> to abort once the function is called.
I've discoved the last sentence above is crucial, and in order to catch
all the corner cases I found it useful to have the struct
cpuidle_coupled in cpuidle.h so that the driver can check ready_count
itself (patch below, on top of $SUBJECT series.)
As you know, on OMAP4, when entering the coupled state, CPU0 has to wait
for CPU1 to enter its low power state before entering itself. The first
pass at implementing this was to just spin waiting for the powerdomain
of CPU1 to hit off. That works... most of the time.
If CPU1 wakes up immediately (or before CPU0 starts checking), or more
likely, fails to hit the low-power state because of other hardware
"conditions", CPU0 will end up stuck in the loop waiting for CPU1.
To solve this, in addition to checking the power state of CPU1, I also
check if (coupled->ready_count != cpumask_weight(&coupled->alive_coupled_cpus)).
If true, it means that CPU1 has already exited/aborted so CPU0 had
better abort as well.
Checking the ready_count seemed like an easy way to do this, but did you
have any other mechanisms in mind for CPUs to communicate that they've
exited/aborted?
Kevin
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index bf34d67..16e00cc 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -32,15 +32,6 @@ extern void cpuidle_remove_state_sysfs(struct cpuidle_device *device);
extern int cpuidle_add_sysfs(struct device *dev);
extern void cpuidle_remove_sysfs(struct device *dev);
-/* coupled states */
-struct cpuidle_coupled {
- spinlock_t lock;
- int requested_state[NR_CPUS];
- int ready_count;
- cpumask_t alive_coupled_cpus;
- int refcnt;
-};
-
#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int state);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 71f2fba..dbc1f2e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -51,6 +51,15 @@ struct cpuidle_state {
int index);
};
+/* coupled states */
+struct cpuidle_coupled {
+ spinlock_t lock;
+ int requested_state[NR_CPUS];
+ int ready_count;
+ cpumask_t alive_coupled_cpus;
+ int refcnt;
+};
+
/* Idle State Flags */
#define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */
#define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */
^ permalink raw reply related
* Re: [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-03-14 0:28 UTC (permalink / raw)
To: Kevin Hilman
Cc: Daniel Lezcano, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Len Brown,
Santosh Shilimkar, Amit Kucheria, Arjan van de Ven, Trinabh Gupta,
Deepthi Dharwar, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <8762e8kqi6.fsf-l0cyMroinI0@public.gmane.org>
On Tue, Mar 13, 2012 at 4:52 PM, Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org> wrote:
> Hi Colin,
>
> On 12/21/2011 01:09 AM, Colin Cross wrote:
>
>> To use coupled cpuidle states, a cpuidle driver must:
>
> [...]
>
>> Provide a struct cpuidle_state.enter function for each state
>> that affects multiple cpus. This function is guaranteed to be
>> called on all cpus at approximately the same time. The driver
>> should ensure that the cpus all abort together if any cpu tries
>> to abort once the function is called.
>
> I've discoved the last sentence above is crucial, and in order to catch
> all the corner cases I found it useful to have the struct
> cpuidle_coupled in cpuidle.h so that the driver can check ready_count
> itself (patch below, on top of $SUBJECT series.)
ready_count is an internal state of core coupled code, and will change
significantly in the next version of the patches. Drivers cannot
depend on it.
> As you know, on OMAP4, when entering the coupled state, CPU0 has to wait
> for CPU1 to enter its low power state before entering itself. The first
> pass at implementing this was to just spin waiting for the powerdomain
> of CPU1 to hit off. That works... most of the time.
>
> If CPU1 wakes up immediately (or before CPU0 starts checking), or more
> likely, fails to hit the low-power state because of other hardware
> "conditions", CPU0 will end up stuck in the loop waiting for CPU1.
>
> To solve this, in addition to checking the power state of CPU1, I also
> check if (coupled->ready_count != cpumask_weight(&coupled->alive_coupled_cpus)).
> If true, it means that CPU1 has already exited/aborted so CPU0 had
> better abort as well.
>
> Checking the ready_count seemed like an easy way to do this, but did you
> have any other mechanisms in mind for CPUs to communicate that they've
> exited/aborted?
Why not set a flag from CPU1 when it exits the low power state, and
have CPU0 spin on the powerdomain register or the flag? You can then
use the parallel barrier function to ensure both cpus have seen the
flag and reset it to 0 before returning.
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-03-14 0:39 UTC (permalink / raw)
To: Arjan van de Ven
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Len Brown,
Kevin Hilman, Santosh Shilimkar, Amit Kucheria, Trinabh Gupta,
Deepthi Dharwar, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4EF1CD49.9020800-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On Wed, Dec 21, 2011 at 4:12 AM, Arjan van de Ven <arjan-VuQAYsv1562vlLw7lfJaRg@public.gmane.orgm> wrote:
> On 12/21/2011 10:55 AM, Colin Cross wrote:
>> On Wed, Dec 21, 2011 at 1:44 AM, Arjan van de Ven <arjan-VuQAYsv1560/9W2qTSuDoA@public.gmane.org.com> wrote:
>>> On 12/21/2011 10:40 AM, Colin Cross wrote:
>>>
>>>>> this smells fundamentally racey to me; you can get an interrupt one
>>>>> cycle after you think you're done, but before the last guy enters WFI...
>>>>>
>>>>> how do you solve that issue ?
>>>>
>>>> All the cpus have interrupts off when they increment the counter, so
>>>> they cannot receive an interrupt. If an interrupt is pending on one
>>>> of those cpus, it will be handled later when WFI aborts due to the
>>>> pending interrupt.
>>>
>>> ... but this leads to cases where you're aborting before other cpus are
>>> entering..... so your "last guy in" doesn't really work, since while cpu
>>> 0 thinks it's the last guy, cpu 1 is already on the way out/out
>>> already... (heck it might already be going back to sleep if your idle
>>> code can run fast, like in the size of a cache miss)
>>
>> Once a cpu has incremented the counter, it has no way out unless either
>> 1: another cpu (that hasn't incremented the counter yet) receives an
>> interrupt, aborts idle, and clears its idle flag
>> or
>> 2: all cpus enter the ready counter, and call the cpuidle driver's
>> enter function.
>
> .. or it enters WFI, and a physical device sends it an interrupt,
> at which point it exits.
Rereading this, I think I understand what you meant. I misunderstood
because of your reference to WFI, which is not entered at this point.
WFI generally refers to the shallowest idle state, which only affects
a single cpu. For deeper states, the cpu may or may not execute the
WFI instruction to trigger the transition (depending on the platform),
but it's not really entering WFI, and the cpu will generally not ever
execute the next instruction (again, depending on the platform).
For coupled cpus, only one cpu is capable of booting first, so all
interrupts should be masked on cpu1 by the time it gets to its low
power state, so it should never abort. If it does abort (like the
example Kevin Hillman just posted to the end of this thread), cpu1
needs to set a flag to say it is not going to get to idle, and cpu0
needs to wait for cpu1 to be in idle (by polling a power controller
register), or for the flag to be set. Afterwards, the cpus need to
resynchronize and reset the flag, which will be easy with the parallel
barrier helper function that will be included in the next patch set.
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Colin Cross @ 2012-03-14 0:47 UTC (permalink / raw)
To: Kevin Hilman
Cc: Len Brown, linux-kernel, Amit Kucheria, linux-tegra, linux-pm,
linux-omap, Arjan van de Ven, linux-arm-kernel
In-Reply-To: <CAMbhsRRZ+xt179RN32-+eBWngbRow=MpKV1XcLvG-sHGF6VOJA@mail.gmail.com>
On Tue, Mar 13, 2012 at 5:28 PM, Colin Cross <ccross@android.com> wrote:
> On Tue, Mar 13, 2012 at 4:52 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Hi Colin,
>>
>> On 12/21/2011 01:09 AM, Colin Cross wrote:
>>
>>> To use coupled cpuidle states, a cpuidle driver must:
>>
>> [...]
>>
>>> Provide a struct cpuidle_state.enter function for each state
>>> that affects multiple cpus. This function is guaranteed to be
>>> called on all cpus at approximately the same time. The driver
>>> should ensure that the cpus all abort together if any cpu tries
>>> to abort once the function is called.
>>
>> I've discoved the last sentence above is crucial, and in order to catch
>> all the corner cases I found it useful to have the struct
>> cpuidle_coupled in cpuidle.h so that the driver can check ready_count
>> itself (patch below, on top of $SUBJECT series.)
>
> ready_count is an internal state of core coupled code, and will change
> significantly in the next version of the patches. Drivers cannot
> depend on it.
>
>> As you know, on OMAP4, when entering the coupled state, CPU0 has to wait
>> for CPU1 to enter its low power state before entering itself. The first
>> pass at implementing this was to just spin waiting for the powerdomain
>> of CPU1 to hit off. That works... most of the time.
>>
>> If CPU1 wakes up immediately (or before CPU0 starts checking), or more
>> likely, fails to hit the low-power state because of other hardware
>> "conditions", CPU0 will end up stuck in the loop waiting for CPU1.
>>
>> To solve this, in addition to checking the power state of CPU1, I also
>> check if (coupled->ready_count != cpumask_weight(&coupled->alive_coupled_cpus)).
>> If true, it means that CPU1 has already exited/aborted so CPU0 had
>> better abort as well.
>>
>> Checking the ready_count seemed like an easy way to do this, but did you
>> have any other mechanisms in mind for CPUs to communicate that they've
>> exited/aborted?
>
> Why not set a flag from CPU1 when it exits the low power state, and
> have CPU0 spin on the powerdomain register or the flag? You can then
> use the parallel barrier function to ensure both cpus have seen the
> flag and reset it to 0 before returning.
I realized the parallel barrier helper was not included in the patch
set I posted, it will be in the next patch set. Short version, no
caller to cpuidle_coupled_parallel_barrier will return before all cpus
in the coupled set have called it. It allows you to resynchronize the
cpus after an abort to ensure they have all seen the abort flag before
clearing it and returning, leaving everything in the correct state for
the next idle attempt.
^ permalink raw reply
* Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
From: Yanmin Zhang @ 2012-03-14 0:55 UTC (permalink / raw)
To: Greg KH
Cc: Mark Brown, Brown, Len, ShuoX Liu, Ingo Molnar,
linux-kernel@vger.kernel.org, Henrique de Moraes Holschuh,
H. Peter Anvin, andi.kleen, Thomas Gleixner,
linux-pm@lists.linux-foundation.org, Andrew Morton
In-Reply-To: <20120313192949.GA8568@kroah.com>
On Tue, 2012-03-13 at 12:29 -0700, Greg KH wrote:
> On Tue, Mar 13, 2012 at 09:36:34AM +0800, Yanmin Zhang wrote:
> > On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> > > On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > > > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > > >
> > > > > Do you know of any tools using these files? I have never heard of them,
> > > > > and I was told we should move these files years ago. So I don't think
> > > > > there should be any api issues.
> > > >
> > > > powertop uses them.
> > >
> > > Ok, then we can't move them all.
> > >
> > > We should then just move the ones that have multiple lines, as I'm
> > > pretty sure powertop doesn't use them, right?
> > All sys files under cpu/cpuXXX/cpuidle have single line. If we move
> > some files to debugfs and keep others under sysfs, users might be confused.
> >
> > Should we go back to the 1st version which just adds the new entry to
> > sysfs?
>
> Wait, I thought this whole thing started when we wanted to move the
> files that had multiple lines out of sysfs?
No. Liu Shuo's patch adds a new entry under cpu/cpuXXX/cpuidle and users
can disable specific C state.
A gentleman raised that if we should move it to debugfs, then you suggested
to move all files under cpu/cpuXXX/cpuidle to debugfs.
>
> If none of these do, and they all are being used by tools already, then
> fine, they should stay.
Agree.
>
> But for some reason, I thought there was a problem here. Perhaps that
> was in the cpufreq code?
I checked cpufreq quickly. Every file has single-line, but some have
multiple-fields.
We would send a new patch based on sysfs as the new entry has
single line.
^ permalink raw reply
* Re: [PATCH 0/3] coupled cpuidle state support
From: Arjan van de Ven @ 2012-03-14 2:04 UTC (permalink / raw)
To: Kevin Hilman
Cc: Len Brown, linux-kernel, Amit Kucheria, Colin Cross, linux-tegra,
linux-pm, linux-omap, linux-arm-kernel
In-Reply-To: <8762e8kqi6.fsf@ti.com>
On 3/13/2012 4:52 PM, Kevin Hilman wrote:
> Checking the ready_count seemed like an easy way to do this, but did you
> have any other mechanisms in mind for CPUs to communicate that they've
> exited/aborted?
this indeed is the tricky part (which I warned about earlier);
I've spent quite a lot of time (weeks) to get this provably working for
an Intel system with similar requirements... and it's extremely unfunny,
and needed firmware support to close some of the race conditions.
I sure hope that hardware with these requirements is on the way out...
it's not very OS friendly.
^ 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