* Re: [PATCH 6/6] cgroup: kill subsys->can_attach_task(), pre_attach() and attach_task()
From: Tejun Heo @ 2011-08-31 7:03 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: containers, lizf, linux-kernel, linux-pm, paul, kamezawa.hiroyu
In-Reply-To: <20110830201030.GC15953@somewhere.redhat.com>
Hello, Frederic.
On Tue, Aug 30, 2011 at 10:10:32PM +0200, Frederic Weisbecker wrote:
> In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
> the tasks that have failed to migrate should be removed from the iterator
> so that they are not included in the batch in ->attach().
I don't think that's a good approach. It breaks the symmetry when
calling different callbacks. What if ->can_attach() allocates
per-task resources and the task exits in the middle? I think we
better lock down fork/exit/exec. I'll send patches but I'm currently
moving / traveling w/ limited access to my toys so it might take some
time.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: MyungJoo Ham @ 2011-08-31 3:59 UTC (permalink / raw)
To: Kevin Hilman
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <871uw2lbls.fsf@ti.com>
On Wed, Aug 31, 2011 at 8:32 AM, Kevin Hilman <khilman@ti.com> wrote:
> MyungJoo Ham <myungjoo.ham@samsung.com> writes:
>
>> The patchset revision v8 has minor updates since v7 and v6.
>> - Allow governors to have their own sysfs interface and init/exit callbacks.
>>
>> The patches 1/5 (OPP notifier) and 2/5 (DEVFREQ core) have no changes since v7.
>> There has been reordering between "add common sysfs interfaces" patch
>> and "add basic governors" (3/5 and 5/5)
>> "add internal interfaces for governors (4/5)" patch has been newly
>> introduced at v8 patchset.
>>
>> For a usage example, please look at
>> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
>>
>> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
>> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
>> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
>> and other related clocks simply follow the determined DDR RAM clock.
>>
>> The DEVFREQ driver for Exynos4210 memory bus is at
>> /drivers/devfreq/exynos4210_memorybus.c in the git tree.
>
> Minor nit: you continue to use DEVFREQ (all caps) throughout subjects
> and changelogs etc. when it should be called devfreq since it's not an
> acryonym.
>
> Kevin
>
Fine, I'll regard devfreq as a common noun.
Cheers,
MyungJoo
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Kevin Hilman @ 2011-08-30 23:32 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-1-git-send-email-myungjoo.ham@samsung.com>
MyungJoo Ham <myungjoo.ham@samsung.com> writes:
> The patchset revision v8 has minor updates since v7 and v6.
> - Allow governors to have their own sysfs interface and init/exit callbacks.
>
> The patches 1/5 (OPP notifier) and 2/5 (DEVFREQ core) have no changes since v7.
> There has been reordering between "add common sysfs interfaces" patch
> and "add basic governors" (3/5 and 5/5)
> "add internal interfaces for governors (4/5)" patch has been newly
> introduced at v8 patchset.
>
> For a usage example, please look at
> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
>
> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
> and other related clocks simply follow the determined DDR RAM clock.
>
> The DEVFREQ driver for Exynos4210 memory bus is at
> /drivers/devfreq/exynos4210_memorybus.c in the git tree.
Minor nit: you continue to use DEVFREQ (all caps) throughout subjects
and changelogs etc. when it should be called devfreq since it's not an
acryonym.
Kevin
^ permalink raw reply
* [Update] LPC2011 Power Management Micro Conf
From: Rafael J. Wysocki @ 2011-08-30 22:39 UTC (permalink / raw)
To: linux-pm
Cc: Arnd Bergmann, Greg Kroah-Hartman, Stephen Boyd, LKML,
Grant Likely, MyungJoo Ham, Jean Pihet, Arjan van de Ven
In-Reply-To: <201107030929.28555.rjw@sisk.pl>
Hi,
On Sunday, July 03, 2011, Rafael J. Wysocki wrote:
> Hi,
>
> Some time ago I sent the following to linux-pm and wasn't really satisfied
> with the response, so here it goes again. It is slightly outdated, because
> we've already started to plan a PM meeting at the Kernel Summit, but I hope
> some of you will be able to participate in the LPC PM Mini Conf too.
>
> In the previous years we used to organize Power Management Mini Summits to
> discuss PM-related issues, the last of which took place during LinuxCon in
> Boston in 2010. Unfortunately, however, it wasn't particularly well attended
> and some of the participants generally felt that it had failed its purpose.
> The power management track at the last LPC, on the other hand, was generally
> regarded as interesting and successful, so there only seems to be room for
> one event devoted to power management a year. For this reason, there won't
> be a PM Mini Summit in 2011 and the LPC PM Mini Conference will play the role
> of it. To this end, in addition to the plenary sessions within the LPC PM
> track there will be a possibility to discuss specific problems related to
> power management in smaller groups.
>
> If there's anything you'd like to discuss or give a presentation on related to
> Power Management, please let me know, preferably by replying to this message.
>
> We have 1.5 hours allocated in the LPC schedule for the plenary session, but
> we will be able to move to another room (or a number of rooms if that's more
> suitable) afterwards to discuss things in detail.
The LPC planned schedule has recently changed so that the PM microconference
is now scheduled on Thursday, Sep. 8, 2011, at 4:45 PM. Since the LPC
organizers have no plans for Thursday evening, we can easily extend the
discussion part pretty much as far as we want.
Please review the updated agenda at
http://wiki.linuxplumbersconf.org/2011:power_management
and adjust your plans. Sorry for the inconveniences, if any.
Thanks,
Rafael
^ permalink raw reply
* Re: [stable] [regression] Re: [213/474] x86, hotplug: Use mwait to offline a processor, fix the legacy case
From: Greg KH @ 2011-08-30 22:34 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Len Brown, x86, Robert Scott, Greg KH, Frédéric Boiteux,
stable-review, Venkatesh Pallipadi, linux-pm, H. Peter Anvin,
stable
In-Reply-To: <20110828184004.GA7690@elie.gateway.2wire.net>
On Sun, Aug 28, 2011 at 01:40:04PM -0500, Jonathan Nieder wrote:
> Hi,
>
> On 2011-03-17, Greg KH wrote:
>
> > 2.6.33-longterm review patch. If anyone has any objections, please let us know.
> [...]
> > From: H. Peter Anvin <hpa@linux.intel.com>
> >
> > upstream ea53069231f9317062910d6e772cca4ce93de8c8
> > x86, hotplug: Use mwait to offline a processor, fix the legacy case
> >
> > Here included also some small follow-on patches to the same code:
> [...]
> > https://bugzilla.kernel.org/show_bug.cgi?id=5471
> >
> > Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> > Signed-off-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
> Sorry for the sloow response. Unfortunately this is reported to have
> broken hibernation on two machines: an EeePC 1002HA and an Ideapad S10-3.
> Frédéric writes:
>
> | I've observed that when resuming from hibernation, sometimes my computer was
> | returning to Grub2's menu (without any error message), and I had to do a
> | "normal" boot on my Debian Squeeze system (with file systems corrections),
> | loosing my hibernation's state.
> | The fail isn't automatic, but seems to happen more frequently after a while
> | my computer was disconnected from AC and battery.
>
> Noticed on Debian squeeze (which is based on v2.6.32.y), confirmed
> with unpatched v2.6.32.45 and v2.6.33.18. Backing out the patch
> mentioned above avoids trouble. A newer kernel (based on v2.6.39.y)
> does _not_ exhibit the same problem, so it looks like the problem was
> introduced in backporting. http://bugs.debian.org/622259 has
> details.
Thanks for letting me know, I'll go revert that patch for the next .32
and .33 longterm releases and let everyone else work out exactly what
the problem is, if they want to.
greg k-h
^ permalink raw reply
* [RFC][PATCH 5/5] PM / Domains: Add default power off governor function
From: Rafael J. Wysocki @ 2011-08-30 22:22 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Add a function deciding whether or not a given PM domain should
be powered off on the basis of that domain's devices' PM QoS
constraints.
---
drivers/base/power/domain_governor.c | 96 +++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 7 ++
2 files changed, 103 insertions(+)
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -49,6 +49,10 @@ struct generic_pm_domain {
int (*start_device)(struct device *dev);
int (*stop_device)(struct device *dev);
bool (*active_wakeup)(struct device *dev);
+ ktime_t power_off_latency;
+ ktime_t power_on_latency;
+ s64 break_even_ns;
+ s64 min_delta_ns;
};
static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
@@ -64,6 +68,9 @@ struct gpd_link {
};
struct gpd_gov_dev_data {
+ ktime_t start_latency;
+ ktime_t suspend_latency;
+ ktime_t resume_latency;
s64 break_even_ns;
};
Index: linux/drivers/base/power/domain_governor.c
===================================================================
--- linux.orig/drivers/base/power/domain_governor.c
+++ linux/drivers/base/power/domain_governor.c
@@ -35,6 +35,102 @@ static bool default_stop_ok(struct devic
return constraint_ns > gov_data->break_even_ns;
}
+/* This routine must be executed under the PM domain's lock. */
+static bool default_power_down_ok(struct dev_pm_domain *pd)
+{
+ struct generic_pm_domain *genpd = pd_to_genpd(pd);
+ struct gpd_link *link;
+ struct pm_domain_data *pdd;
+ ktime_t off_time, on_time;
+ s64 delta_ns, min_delta_ns;
+
+ on_time = genpd->power_on_latency;
+ /* Check if slave domains can be off for enough time. */
+ delta_ns = ktime_to_ns(ktime_add(genpd->power_off_latency, on_time));
+ min_delta_ns = 0;
+ /* All slave domains have been powered off at this point. */
+ list_for_each_entry(link, &genpd->master_links, master_node) {
+ if (delta_ns > link->slave->min_delta_ns)
+ return false;
+
+ delta_ns = link->slave->min_delta_ns - delta_ns;
+ if (delta_ns < min_delta_ns)
+ min_delta_ns = delta_ns;
+ }
+
+ genpd->min_delta_ns = min_delta_ns;
+
+ /* Compute the total time needed to power off the domain. */
+ off_time = ktime_set(0, 0);
+ /* All devices have been stopped at this point. */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ struct gpd_gov_dev_data *gov_data;
+
+ if (!pdd->dev->driver)
+ continue;
+
+ gov_data = to_gpd_data(pdd)->gov_data;
+ if (!gov_data)
+ continue;
+
+ off_time = ktime_add(off_time, gov_data->suspend_latency);
+ }
+ off_time = ktime_add(off_time, genpd->power_off_latency);
+
+ /*
+ * For each device in the domain compute the difference between the
+ * QoS value and the total time required to bring the device back
+ * assuming that the domain will be powered off and compute the minimum
+ * of those.
+ */
+ min_delta_ns = 0;
+ on_time = ktime_add(on_time, off_time);
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ struct gpd_gov_dev_data *gov_data;
+ struct device *dev = pdd->dev;
+ ktime_t dev_up_time;
+ s32 constraint;
+ s64 constraint_ns;
+
+ if (!dev->driver)
+ continue;
+
+ gov_data = to_gpd_data(pdd)->gov_data;
+ if (gov_data) {
+ dev_up_time = ktime_add(on_time,
+ gov_data->resume_latency);
+ dev_up_time = ktime_add(dev_up_time,
+ gov_data->start_latency);
+ } else {
+ dev_up_time = on_time;
+ }
+
+ constraint = dev_pm_qos_read_value(dev);
+ if (constraint < 0)
+ return false;
+ else if (constraint == 0) /* 0 means "don't care" */
+ continue;
+
+ constraint_ns = constraint;
+ constraint_ns *= NSEC_PER_USEC;
+ delta_ns = constraint_ns - ktime_to_ns(dev_up_time);
+ if (min_delta_ns > delta_ns)
+ min_delta_ns = delta_ns;
+ }
+
+ /* Compare the computed delta with the break even value. */
+ if (min_delta_ns < genpd->break_even_ns)
+ return false;
+
+ /* Store the computed value for the masters to use. */
+ if (genpd->min_delta_ns > min_delta_ns)
+ genpd->min_delta_ns = min_delta_ns;
+
+ /* The domain can be powered off. */
+ return true;
+}
+
struct dev_power_governor default_qos_governor = {
.stop_ok = default_stop_ok,
+ .power_down_ok = default_power_down_ok,
};
^ permalink raw reply
* [RFC][PATCH 4/5] PM / Domains: Add device stop governor function
From: Rafael J. Wysocki @ 2011-08-30 22:21 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Add a function deciding whether or not devices should be
stopped in pm_genpd_runtime_suspend() depending on their
PM QoS values.
---
arch/arm/mach-shmobile/pm-sh7372.c | 2 -
drivers/base/power/Makefile | 2 -
drivers/base/power/domain.c | 35 ++++++++++++++++++-----
drivers/base/power/domain_governor.c | 40 ++++++++++++++++++++++++++
include/linux/pm_domain.h | 52 ++++++++++++++++++++++++++++++-----
5 files changed, 115 insertions(+), 16 deletions(-)
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -21,6 +21,7 @@ enum gpd_status {
struct dev_power_governor {
bool (*power_down_ok)(struct dev_pm_domain *domain);
+ bool (*stop_ok)(struct device *dev);
};
struct generic_pm_domain {
@@ -62,8 +63,13 @@ struct gpd_link {
struct list_head slave_node;
};
+struct gpd_gov_dev_data {
+ s64 break_even_ns;
+};
+
struct generic_pm_domain_data {
struct pm_domain_data base;
+ struct gpd_gov_dev_data *gov_data;
bool need_restore;
};
@@ -73,18 +79,47 @@ static inline struct generic_pm_domain_d
}
#ifdef CONFIG_PM_GENERIC_DOMAINS
-extern int pm_genpd_add_device(struct generic_pm_domain *genpd,
- struct device *dev);
+extern struct dev_power_governor default_qos_governor;
+
+extern struct generic_pm_domain *dev_to_genpd(struct device *dev);
+extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
+ struct device *dev,
+ struct gpd_gov_dev_data *gov_data);
+
+static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
+ struct device *dev)
+{
+ return __pm_genpd_add_device(genpd, dev, NULL);
+}
+
extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
struct device *dev);
extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_subdomain);
extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *target);
-extern void pm_genpd_init(struct generic_pm_domain *genpd,
- struct dev_power_governor *gov, bool is_off);
+extern void __pm_genpd_init(struct generic_pm_domain *genpd,
+ struct dev_power_governor *gov, bool is_off);
+
+static inline void pm_genpd_init(struct generic_pm_domain *genpd, bool is_off)
+{
+ __pm_genpd_init(genpd, &default_qos_governor, is_off);
+}
+
extern int pm_genpd_poweron(struct generic_pm_domain *genpd);
+
#else
+
+static inline struct generic_pm_domain *dev_to_genpd(struct device *dev)
+{
+ return ERR_PTR(-ENOSYS);
+}
+static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
+ struct device *dev,
+ struct gpd_gov_dev_data *gov_data)
+{
+ return -ENOSYS;
+}
static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
struct device *dev)
{
@@ -105,8 +140,13 @@ static inline int pm_genpd_remove_subdom
{
return -ENOSYS;
}
-static inline void pm_genpd_init(struct generic_pm_domain *genpd,
- struct dev_power_governor *gov, bool is_off) {}
+static inline void __pm_genpd_init(struct generic_pm_domain *genpd,
+ struct dev_power_governor *gov, bool is_off)
+{
+}
+static inline void pm_genpd_init(struct generic_pm_domain *genpd, bool is_off)
+{
+}
static inline int pm_genpd_poweron(struct generic_pm_domain *genpd)
{
return -ENOSYS;
Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -21,7 +21,7 @@ static DEFINE_MUTEX(gpd_list_lock);
#ifdef CONFIG_PM
-static struct generic_pm_domain *dev_to_genpd(struct device *dev)
+struct generic_pm_domain *dev_to_genpd(struct device *dev)
{
if (IS_ERR_OR_NULL(dev->pm_domain))
return ERR_PTR(-EINVAL);
@@ -403,6 +403,22 @@ static void genpd_power_off_work_fn(stru
}
/**
+ * genpd_stop_dev - Stop a given device if that's beneficial.
+ * @genpd: PM domain the device belongs to.
+ * @dev: Device to stop.
+ */
+static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev)
+{
+ bool (*stop_ok)(struct device *dev);
+
+ stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;
+ if (stop_ok && !stop_ok(dev))
+ return -EBUSY;
+
+ return genpd->stop_device(dev);
+}
+
+/**
* pm_genpd_runtime_suspend - Suspend a device belonging to I/O PM domain.
* @dev: Device to suspend.
*
@@ -423,7 +439,7 @@ static int pm_genpd_runtime_suspend(stru
might_sleep_if(!genpd->dev_irq_safe);
if (genpd->stop_device) {
- int ret = genpd->stop_device(dev);
+ int ret = genpd_stop_dev(genpd, dev);
if (ret)
return ret;
}
@@ -495,7 +511,7 @@ static int pm_genpd_runtime_resume(struc
mutex_lock(&genpd->lock);
}
finish_wait(&genpd->status_wait_queue, &wait);
- __pm_genpd_restore_device(dev->power.subsys_data->domain_data, genpd);
+ __pm_genpd_restore_device(dev_to_psd(dev)->domain_data, genpd);
genpd->resume_count--;
genpd_set_active(genpd);
wake_up_all(&genpd->status_wait_queue);
@@ -1076,11 +1092,13 @@ static void pm_genpd_complete(struct dev
#endif /* CONFIG_PM_SLEEP */
/**
- * pm_genpd_add_device - Add a device to an I/O PM domain.
+ * __pm_genpd_add_device - Add a device to an I/O PM domain.
* @genpd: PM domain to add the device to.
* @dev: Device to be added.
+ * @gov_data: Set of PM QoS parameters to attach to the device.
*/
-int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
+int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+ struct gpd_gov_dev_data *gov_data)
{
struct generic_pm_domain_data *gpd_data;
struct pm_domain_data *pdd;
@@ -1123,6 +1141,7 @@ int pm_genpd_add_device(struct generic_p
gpd_data->base.dev = dev;
gpd_data->need_restore = false;
list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
+ gpd_data->gov_data = gov_data;
out:
genpd_release_lock(genpd);
@@ -1280,13 +1299,13 @@ int pm_genpd_remove_subdomain(struct gen
}
/**
- * pm_genpd_init - Initialize a generic I/O PM domain object.
+ * __pm_genpd_init - Initialize a generic I/O PM domain object.
* @genpd: PM domain object to initialize.
* @gov: PM domain governor to associate with the domain (may be NULL).
* @is_off: Initial value of the domain's power_is_off field.
*/
-void pm_genpd_init(struct generic_pm_domain *genpd,
- struct dev_power_governor *gov, bool is_off)
+void __pm_genpd_init(struct generic_pm_domain *genpd,
+ struct dev_power_governor *gov, bool is_off)
{
if (IS_ERR_OR_NULL(genpd))
return;
Index: linux/drivers/base/power/Makefile
===================================================================
--- linux.orig/drivers/base/power/Makefile
+++ linux/drivers/base/power/Makefile
@@ -3,7 +3,7 @@ obj-$(CONFIG_PM_SLEEP) += main.o wakeup.
obj-$(CONFIG_PM_RUNTIME) += runtime.o
obj-$(CONFIG_PM_TRACE_RTC) += trace.o
obj-$(CONFIG_PM_OPP) += opp.o
-obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
obj-$(CONFIG_HAVE_CLK) += clock_ops.o
ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
Index: linux/drivers/base/power/domain_governor.c
===================================================================
--- /dev/null
+++ linux/drivers/base/power/domain_governor.c
@@ -0,0 +1,40 @@
+/*
+ * drivers/base/power/domain_governor.c - Governors for device PM domains.
+ *
+ * Copyright (C) 2011 Rafael J. Wysocki <rjw@sisk.pl>, Renesas Electronics Corp.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_qos.h>
+
+static bool default_stop_ok(struct device *dev)
+{
+ struct gpd_gov_dev_data *gov_data;
+ s64 constraint_ns;
+ s32 constraint;
+
+ dev_dbg(dev, "%s()\n", __func__);
+
+ gov_data = to_gpd_data(dev_to_psd(dev)->domain_data)->gov_data;
+ if (!gov_data)
+ return true;
+
+ constraint = dev_pm_qos_read_value(dev);
+ if (constraint < 0)
+ return false;
+ else if (constraint == 0) /* 0 means "don't care" */
+ return true;
+
+ constraint_ns = constraint;
+ constraint_ns *= NSEC_PER_USEC;
+
+ return constraint_ns > gov_data->break_even_ns;
+}
+
+struct dev_power_governor default_qos_governor = {
+ .stop_ok = default_stop_ok,
+};
Index: linux/arch/arm/mach-shmobile/pm-sh7372.c
===================================================================
--- linux.orig/arch/arm/mach-shmobile/pm-sh7372.c
+++ linux/arch/arm/mach-shmobile/pm-sh7372.c
@@ -100,7 +100,7 @@ void sh7372_init_pm_domain(struct sh7372
{
struct generic_pm_domain *genpd = &sh7372_pd->genpd;
- pm_genpd_init(genpd, NULL, false);
+ pm_genpd_init(genpd, false);
genpd->stop_device = pm_clk_suspend;
genpd->start_device = pm_clk_resume;
genpd->dev_irq_safe = true;
^ permalink raw reply
* [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read_value()
From: Rafael J. Wysocki @ 2011-08-30 22:21 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
To read the current PM QoS value for a given device we need to
make sure that the device's power.constraints object won't be
removed while we're doing that. For this reason, put the
operation under dev->power.lock and acquire the lock
around the initialization and removal of power.constraints.
Moreover, since we're using the value of power.constraints to
determine whether or not the object is present, the
power.constraints_state field isn't necessary any more and may be
removed. However, dev_pm_qos_add_request() needs to check if the
device is being removed from the system before allocating a new
PM QoS constraints object for it, so it has to use device_pm_lock()
and the device PM QoS initialization and destruction should be done
under device_pm_lock() as well.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/main.c | 4 -
drivers/base/power/qos.c | 167 ++++++++++++++++++++++++++--------------------
include/linux/pm.h | 8 --
include/linux/pm_qos.h | 3
4 files changed, 101 insertions(+), 81 deletions(-)
Index: linux/drivers/base/power/qos.c
===================================================================
--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -30,15 +30,6 @@
* . To minimize the data usage by the per-device constraints, the data struct
* is only allocated at the first call to dev_pm_qos_add_request.
* . The data is later free'd when the device is removed from the system.
- * . The constraints_state variable from dev_pm_info tracks the data struct
- * allocation state:
- * DEV_PM_QOS_NO_DEVICE: No device present or device removed, no data
- * allocated,
- * DEV_PM_QOS_DEVICE_PRESENT: Device present, data not allocated and will be
- * allocated at the first call to dev_pm_qos_add_request,
- * DEV_PM_QOS_ALLOCATED: Device present, data allocated. The per-device
- * PM QoS constraints framework is operational and constraints can be
- * added, updated or removed using the dev_pm_qos_* API.
* . A global mutex protects the constraints users from the data being
* allocated and free'd.
*/
@@ -51,8 +42,30 @@
static DEFINE_MUTEX(dev_pm_qos_mtx);
+
static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
+/**
+ * dev_pm_qos_read_value - Get PM QoS constraint for a given device.
+ * @dev: Device to get the PM QoS constraint value for.
+ */
+s32 dev_pm_qos_read_value(struct device *dev)
+{
+ struct pm_qos_constraints *c;
+ unsigned long flags;
+ s32 ret = 0;
+
+ spin_lock_irqsave(&dev->power.lock, flags);
+
+ c = dev->power.constraints;
+ if (c)
+ ret = pm_qos_read_value(c);
+
+ spin_unlock_irqrestore(&dev->power.lock, flags);
+
+ return ret;
+}
+
/*
* apply_constraint
* @req: constraint request to apply
@@ -105,27 +118,37 @@ static int dev_pm_qos_constraints_alloca
}
BLOCKING_INIT_NOTIFIER_HEAD(n);
+ plist_head_init(&c->list);
+ c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+ c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+ c->type = PM_QOS_MIN;
+ c->notifiers = n;
+
+ spin_lock_irq(&dev->power.lock);
dev->power.constraints = c;
- plist_head_init(&dev->power.constraints->list);
- dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- dev->power.constraints->type = PM_QOS_MIN;
- dev->power.constraints->notifiers = n;
- dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
+ spin_unlock_irq(&dev->power.lock);
return 0;
}
+static void __dev_pm_qos_constraints_init(struct device *dev)
+{
+ spin_lock_irq(&dev->power.lock);
+ dev->power.constraints = NULL;
+ spin_unlock_irq(&dev->power.lock);
+}
+
/**
- * dev_pm_qos_constraints_init
+ * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
* @dev: target device
*
- * Called from the device PM subsystem at device insertion
+ * Called from the device PM subsystem at device insertion under
+ * device_pm_lock().
*/
void dev_pm_qos_constraints_init(struct device *dev)
{
mutex_lock(&dev_pm_qos_mtx);
- dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
+ dev->power.constraints = NULL;
mutex_unlock(&dev_pm_qos_mtx);
}
@@ -133,34 +156,35 @@ void dev_pm_qos_constraints_init(struct
* dev_pm_qos_constraints_destroy
* @dev: target device
*
- * Called from the device PM subsystem at device removal
+ * Called from the device PM subsystem at device removal under device_pm_lock().
*/
void dev_pm_qos_constraints_destroy(struct device *dev)
{
struct dev_pm_qos_request *req, *tmp;
+ struct pm_qos_constraints *c;
mutex_lock(&dev_pm_qos_mtx);
- if (dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
- /* Flush the constraints list for the device */
- plist_for_each_entry_safe(req, tmp,
- &dev->power.constraints->list,
- node) {
- /*
- * Update constraints list and call the notification
- * callbacks if needed
- */
- apply_constraint(req, PM_QOS_REMOVE_REQ,
- PM_QOS_DEFAULT_VALUE);
- memset(req, 0, sizeof(*req));
- }
+ c = dev->power.constraints;
+ if (!c)
+ goto out;
- kfree(dev->power.constraints->notifiers);
- kfree(dev->power.constraints);
- dev->power.constraints = NULL;
+ /* Flush the constraints list for the device */
+ plist_for_each_entry_safe(req, tmp, &c->list, node) {
+ /*
+ * Update constraints list and call the notification
+ * callbacks if needed
+ */
+ apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+ memset(req, 0, sizeof(*req));
}
- dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
+ __dev_pm_qos_constraints_init(dev);
+
+ kfree(c->notifiers);
+ kfree(c);
+
+ out:
mutex_unlock(&dev_pm_qos_mtx);
}
@@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
*
* Returns 1 if the aggregated constraint value has changed,
* 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures.
*/
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
s32 value)
@@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
return -EINVAL;
}
- mutex_lock(&dev_pm_qos_mtx);
req->dev = dev;
- /* Return if the device has been removed */
- if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
- ret = -ENODEV;
- goto out;
- }
+ device_pm_lock();
+ mutex_lock(&dev_pm_qos_mtx);
- /*
- * Allocate the constraints data on the first call to add_request,
- * i.e. only if the data is not already allocated and if the device has
- * not been removed
- */
- if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
- ret = dev_pm_qos_constraints_allocate(dev);
+ if (dev->power.constraints) {
+ device_pm_unlock();
+ } else {
+ if (list_empty(&dev->power.entry)) {
+ /* The device has been removed from the system. */
+ device_pm_unlock();
+ goto out;
+ } else {
+ device_pm_unlock();
+ /*
+ * Allocate the constraints data on the first call to
+ * add_request, i.e. only if the data is not already
+ * allocated and if the device has not been removed.
+ */
+ ret = dev_pm_qos_constraints_allocate(dev);
+ }
+ }
if (!ret)
ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
-out:
+ out:
mutex_unlock(&dev_pm_qos_mtx);
+
return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
@@ -252,13 +283,13 @@ int dev_pm_qos_update_request(struct dev
mutex_lock(&dev_pm_qos_mtx);
- if (req->dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
+ if (req->dev->power.constraints) {
if (new_value != req->node.prio)
ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
new_value);
} else {
/* Return if the device has been removed */
- ret = -ENODEV;
+ ret = -EINVAL;
}
mutex_unlock(&dev_pm_qos_mtx);
@@ -293,7 +324,7 @@ int dev_pm_qos_remove_request(struct dev
mutex_lock(&dev_pm_qos_mtx);
- if (req->dev->power.constraints_state == DEV_PM_QOS_ALLOCATED) {
+ if (req->dev->power.constraints) {
ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
@@ -323,15 +354,12 @@ int dev_pm_qos_add_notifier(struct devic
mutex_lock(&dev_pm_qos_mtx);
- /* Silently return if the device has been removed */
- if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
- goto out;
-
- retval = blocking_notifier_chain_register(
- dev->power.constraints->notifiers,
- notifier);
+ /* Silently return if the constraints object is not present. */
+ if (dev->power.constraints)
+ retval = blocking_notifier_chain_register(
+ dev->power.constraints->notifiers,
+ notifier);
-out:
mutex_unlock(&dev_pm_qos_mtx);
return retval;
}
@@ -354,15 +382,12 @@ int dev_pm_qos_remove_notifier(struct de
mutex_lock(&dev_pm_qos_mtx);
- /* Silently return if the device has been removed */
- if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
- goto out;
-
- retval = blocking_notifier_chain_unregister(
- dev->power.constraints->notifiers,
- notifier);
+ /* Silently return if the constraints object is not present. */
+ if (dev->power.constraints)
+ retval = blocking_notifier_chain_unregister(
+ dev->power.constraints->notifiers,
+ notifier);
-out:
mutex_unlock(&dev_pm_qos_mtx);
return retval;
}
Index: linux/include/linux/pm_qos.h
===================================================================
--- linux.orig/include/linux/pm_qos.h
+++ linux/include/linux/pm_qos.h
@@ -77,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_cl
int pm_qos_request_active(struct pm_qos_request *req);
s32 pm_qos_read_value(struct pm_qos_constraints *c);
+s32 dev_pm_qos_read_value(struct device *dev);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
s32 value);
int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
@@ -117,6 +118,8 @@ static inline int pm_qos_request_active(
static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
{ return 0; }
+static inline s32 dev_pm_qos_read_value(struct device *dev)
+ { return 0; }
static inline int dev_pm_qos_add_request(struct device *dev,
struct dev_pm_qos_request *req,
s32 value)
Index: linux/drivers/base/power/main.c
===================================================================
--- linux.orig/drivers/base/power/main.c
+++ linux/drivers/base/power/main.c
@@ -98,8 +98,8 @@ void device_pm_add(struct device *dev)
dev_warn(dev, "parent %s should not be sleeping\n",
dev_name(dev->parent));
list_add_tail(&dev->power.entry, &dpm_list);
- mutex_unlock(&dpm_list_mtx);
dev_pm_qos_constraints_init(dev);
+ mutex_unlock(&dpm_list_mtx);
}
/**
@@ -110,9 +110,9 @@ void device_pm_remove(struct device *dev
{
pr_debug("PM: Removing info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
- dev_pm_qos_constraints_destroy(dev);
complete_all(&dev->power.completion);
mutex_lock(&dpm_list_mtx);
+ dev_pm_qos_constraints_destroy(dev);
list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
device_wakeup_disable(dev);
Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -421,13 +421,6 @@ enum rpm_request {
RPM_REQ_RESUME,
};
-/* Per-device PM QoS constraints data struct state */
-enum dev_pm_qos_state {
- DEV_PM_QOS_NO_DEVICE, /* No device present */
- DEV_PM_QOS_DEVICE_PRESENT, /* Device present, data not allocated */
- DEV_PM_QOS_ALLOCATED, /* Device present, data allocated */
-};
-
struct wakeup_source;
struct pm_domain_data {
@@ -489,7 +482,6 @@ struct dev_pm_info {
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
struct pm_qos_constraints *constraints;
- enum dev_pm_qos_state constraints_state;
};
extern void update_pm_runtime_accounting(struct device *dev);
^ permalink raw reply
* [PATCH 2/5] PM / Runtime: Do not run callbacks under lock for power.irq_safe set
From: Rafael J. Wysocki @ 2011-08-30 22:20 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
The rpm_suspend() and rpm_resume() routines execute subsystem or PM
domain callbacks under power.lock if power.irq_safe is set for the
given device. This is inconsistent with that rpm_idle() does after
commit 02b2677 (PM / Runtime: Allow _put_sync() from
interrupts-disabled context) and is problematic for subsystems and PM
domains wanting to use power.lock for synchronization in their
runtime PM callbacks. For this reason, make runtime PM core functions
always release power.lock before invoking subsystem or PM domain
callbacks.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/runtime.c | 50 ++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 22 deletions(-)
Index: linux/drivers/base/power/runtime.c
===================================================================
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -155,6 +155,31 @@ static int rpm_check_suspend_allowed(str
}
/**
+ * __rpm_callback - Run a given runtime PM callback for a given device.
+ * @cb: Runtime PM callback to run.
+ * @dev: Device to run the callback for.
+ */
+static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
+ __releases(&dev->power.lock) __acquires(&dev->power.lock)
+{
+ int retval;
+
+ if (dev->power.irq_safe)
+ spin_unlock(&dev->power.lock);
+ else
+ spin_unlock_irq(&dev->power.lock);
+
+ retval = cb(dev);
+
+ if (dev->power.irq_safe)
+ spin_lock(&dev->power.lock);
+ else
+ spin_lock_irq(&dev->power.lock);
+
+ return retval;
+}
+
+/**
* rpm_idle - Notify device bus type if the device can be suspended.
* @dev: Device to notify the bus type about.
* @rpmflags: Flag bits.
@@ -225,19 +250,8 @@ static int rpm_idle(struct device *dev,
else
callback = NULL;
- if (callback) {
- if (dev->power.irq_safe)
- spin_unlock(&dev->power.lock);
- else
- spin_unlock_irq(&dev->power.lock);
-
- callback(dev);
-
- if (dev->power.irq_safe)
- spin_lock(&dev->power.lock);
- else
- spin_lock_irq(&dev->power.lock);
- }
+ if (callback)
+ __rpm_callback(callback, dev);
dev->power.idle_notification = false;
wake_up_all(&dev->power.wait_queue);
@@ -252,22 +266,14 @@ static int rpm_idle(struct device *dev,
* @dev: Device to run the callback for.
*/
static int rpm_callback(int (*cb)(struct device *), struct device *dev)
- __releases(&dev->power.lock) __acquires(&dev->power.lock)
{
int retval;
if (!cb)
return -ENOSYS;
- if (dev->power.irq_safe) {
- retval = cb(dev);
- } else {
- spin_unlock_irq(&dev->power.lock);
-
- retval = cb(dev);
+ retval = __rpm_callback(cb, dev);
- spin_lock_irq(&dev->power.lock);
- }
dev->power.runtime_error = retval;
return retval != -EACCES ? retval : -EIO;
}
^ permalink raw reply
* [PATCH 1/5] PM / Domains: Split device PM domain data into base and need_restore
From: Rafael J. Wysocki @ 2011-08-30 22:18 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
The struct pm_domain_data data type is defined in such a way that
adding new fields specific to the generic PM domains code will
require include/linux/pm.h to be modified. As a result, data types
used only by the generic PM domains code will be defined in two
headers, although they all should be defined in pm_domain.h and
pm.h will need to include more headers, which won't be very nice.
For this reason change the definition of struct pm_subsys_data
so that its domain_data member is a pointer, which will allow
struct pm_domain_data to be subclassed by various PM domains
implementations. Remove the need_restore member from
struct pm_domain_data and make the generic PM domains code
subclass it by adding the need_restore member to the new data type.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/domain.c | 28 +++++++++++++++++++---------
include/linux/pm.h | 3 +--
include/linux/pm_domain.h | 10 ++++++++++
3 files changed, 30 insertions(+), 11 deletions(-)
Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -433,7 +433,6 @@ struct wakeup_source;
struct pm_domain_data {
struct list_head list_node;
struct device *dev;
- bool need_restore;
};
struct pm_subsys_data {
@@ -443,7 +442,7 @@ struct pm_subsys_data {
struct list_head clock_list;
#endif
#ifdef CONFIG_PM_GENERIC_DOMAINS
- struct pm_domain_data domain_data;
+ struct pm_domain_data *domain_data;
#endif
};
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -62,6 +62,16 @@ struct gpd_link {
struct list_head slave_node;
};
+struct generic_pm_domain_data {
+ struct pm_domain_data base;
+ bool need_restore;
+};
+
+static inline struct generic_pm_domain_data *to_gpd_data(struct pm_domain_data *pdd)
+{
+ return container_of(pdd, struct generic_pm_domain_data, base);
+}
+
#ifdef CONFIG_PM_GENERIC_DOMAINS
extern int pm_genpd_add_device(struct generic_pm_domain *genpd,
struct device *dev);
Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -188,11 +188,12 @@ static int __pm_genpd_save_device(struct
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
+ struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
int ret = 0;
- if (pdd->need_restore)
+ if (gpd_data->need_restore)
return 0;
mutex_unlock(&genpd->lock);
@@ -210,7 +211,7 @@ static int __pm_genpd_save_device(struct
mutex_lock(&genpd->lock);
if (!ret)
- pdd->need_restore = true;
+ gpd_data->need_restore = true;
return ret;
}
@@ -224,10 +225,11 @@ static void __pm_genpd_restore_device(st
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
+ struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
- if (!pdd->need_restore)
+ if (!gpd_data->need_restore)
return;
mutex_unlock(&genpd->lock);
@@ -244,7 +246,7 @@ static void __pm_genpd_restore_device(st
mutex_lock(&genpd->lock);
- pdd->need_restore = false;
+ gpd_data->need_restore = false;
}
/**
@@ -493,7 +495,7 @@ static int pm_genpd_runtime_resume(struc
mutex_lock(&genpd->lock);
}
finish_wait(&genpd->status_wait_queue, &wait);
- __pm_genpd_restore_device(&dev->power.subsys_data->domain_data, genpd);
+ __pm_genpd_restore_device(dev->power.subsys_data->domain_data, genpd);
genpd->resume_count--;
genpd_set_active(genpd);
wake_up_all(&genpd->status_wait_queue);
@@ -1080,6 +1082,7 @@ static void pm_genpd_complete(struct dev
*/
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
{
+ struct generic_pm_domain_data *gpd_data;
struct pm_domain_data *pdd;
int ret = 0;
@@ -1106,14 +1109,20 @@ int pm_genpd_add_device(struct generic_p
goto out;
}
+ gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
+ if (!gpd_data) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
genpd->device_count++;
dev->pm_domain = &genpd->domain;
dev_pm_get_subsys_data(dev);
- pdd = &dev->power.subsys_data->domain_data;
- pdd->dev = dev;
- pdd->need_restore = false;
- list_add_tail(&pdd->list_node, &genpd->dev_list);
+ dev->power.subsys_data->domain_data = &gpd_data->base;
+ gpd_data->base.dev = dev;
+ gpd_data->need_restore = false;
+ list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
out:
genpd_release_lock(genpd);
@@ -1152,6 +1161,7 @@ int pm_genpd_remove_device(struct generi
pdd->dev = NULL;
dev_pm_put_subsys_data(dev);
dev->pm_domain = NULL;
+ kfree(to_gpd_data(pdd));
genpd->device_count--;
^ permalink raw reply
* [PATCH 0/5] PM: Generic PM domains and device PM QoS
From: Rafael J. Wysocki @ 2011-08-30 22:17 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
Hi,
This patchset illustrates how device PM QoS may be used along with
PM domains in my view.
Actually, it consists of two parts. Namely, patches [1-3/5] seem to be
suitable for 3.2, unless somebody hates them, but patches [4-5/5] are
total RFC. They haven't been tested, only compiled, so the use of them
is not encouraged (they may kill your dog or make your cat go wild, or
do something equally nasty, so beware). Their purpose is to illustrate
an idea that I'd like to discuss at the PM miniconference during the
LPC.
[1/5] - Split device PM domain data into a "base" and additional fields
(one need_restore field at the moment, but the subsequent patches
add more fields).
[2/5] - Make runtime PM always release power.lock if power.irq_safe is set.
[3/5] - Add function for reading device PM QoS values safely.
[4/5] - Add governor function for stopping devices.
[5/5] - Add generic PM domain power off governor function.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 6/6] cgroup: kill subsys->can_attach_task(), pre_attach() and attach_task()
From: Frederic Weisbecker @ 2011-08-30 20:10 UTC (permalink / raw)
To: Tejun Heo; +Cc: containers, lizf, linux-kernel, linux-pm, paul, kamezawa.hiroyu
In-Reply-To: <1314312192-26885-7-git-send-email-tj@kernel.org>
On Fri, Aug 26, 2011 at 12:43:12AM +0200, Tejun Heo wrote:
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f3c7f7a..ce765ec 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2075,7 +2064,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> {
> int retval, i, group_size, nr_migrating_tasks;
> struct cgroup_subsys *ss, *failed_ss = NULL;
> - bool cancel_failed_ss = false;
> /* guaranteed to be initialized later, but the compiler needs this */
> struct css_set *oldcg;
> struct cgroupfs_root *root = cgrp->root;
> @@ -2166,21 +2154,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> goto out_cancel_attach;
> }
> }
> - /* a callback to be run on every thread in the threadgroup. */
> - if (ss->can_attach_task) {
> - /* run on each task in the threadgroup. */
> - for (i = 0; i < group_size; i++) {
> - tc = flex_array_get(group, i);
> - if (tc->cgrp == cgrp)
> - continue;
> - retval = ss->can_attach_task(cgrp, tc->task);
> - if (retval) {
> - failed_ss = ss;
> - cancel_failed_ss = true;
> - goto out_cancel_attach;
> - }
> - }
> - }
> }
>
> /*
> @@ -2217,15 +2190,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> }
>
> /*
> - * step 3: now that we're guaranteed success wrt the css_sets, proceed
> - * to move all tasks to the new cgroup, calling ss->attach_task for each
> - * one along the way. there are no failure cases after here, so this is
> - * the commit point.
> + * step 3: now that we're guaranteed success wrt the css_sets,
> + * proceed to move all tasks to the new cgroup. There are no
> + * failure cases after here, so this is the commit point.
> */
> - for_each_subsys(root, ss) {
> - if (ss->pre_attach)
> - ss->pre_attach(cgrp);
> - }
> for (i = 0; i < group_size; i++) {
> tc = flex_array_get(group, i);
> /* leave current thread as it is if it's already there */
> @@ -2235,19 +2203,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> /* if the thread is PF_EXITING, it can just get skipped. */
> retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
> BUG_ON(retval != 0 && retval != -ESRCH);
> -
> - /* attach each task to each subsystem */
> - for_each_subsys(root, ss) {
> - if (ss->attach_task)
> - ss->attach_task(cgrp, tc->task);
> - }
In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
the tasks that have failed to migrate should be removed from the iterator
so that they are not included in the batch in ->attach().
> }
> /* nothing is sensitive to fork() after this point. */
>
> /*
> - * step 4: do expensive, non-thread-specific subsystem callbacks.
> - * TODO: if ever a subsystem needs to know the oldcgrp for each task
> - * being moved, this call will need to be reworked to communicate that.
> + * step 4: do subsystem attach callbacks.
> */
> for_each_subsys(root, ss) {
> if (ss->attach)
> @@ -2271,11 +2231,8 @@ out_cancel_attach:
> /* same deal as in cgroup_attach_task */
> if (retval) {
> for_each_subsys(root, ss) {
> - if (ss == failed_ss) {
> - if (cancel_failed_ss && ss->cancel_attach)
> - ss->cancel_attach(ss, cgrp, &tset);
> + if (ss == failed_ss)
> break;
> - }
> if (ss->cancel_attach)
> ss->cancel_attach(ss, cgrp, &tset);
> }
> --
> 1.7.6
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-30 17:10 UTC (permalink / raw)
To: myungjoo.ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJ0PZbQY0Li-uYQGxFqzjuuzCR4ViNy2AHE=8SiNCbADoG8z=g@mail.gmail.com>
On Mon, Aug 29, 2011 at 9:28 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@ti.com> wrote:
>>
>> Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
>> show_freq and restore_freq functions for the userspace governor do not
>> protect the private data accesses with a mutex. Looking a bit further
>> I see that they do call get_devfreq; I think the semantics for this
>> are wrong.
>>
>> get_devfreq only protects the list as long as it takes to do a lookup.
>> However neither struct devfreq or struct userspace_data have a mutex
>> associated with them, so concurrent operations are not protected.
>>
>> One heavy-handed way of solving this is to not have get_devfreq
>> release the mutex, and then have those functions call a new
>> put_devfreq which does release the mutex. However that is really bad
>> locking.
>>
>> What do you think about putting a mutex in struct devfreq? The rule
>> for governors can be that access to the governor-specific private data
>> requires taking that devfreq's mutex.
>
>
> A lock in private data at DEVFREQ framework won't be needed as it is a
> governor-specific issue; however, it appears that we need a locking
> mechanism for accessing struct devfreq in general for governors.
> Although we do not have any governor that writes something or reads
> multiple times on struct devfreq (except for the private data) out of
> get_target_freq ops, which does not need an additional lock, we may
> have one soon.
>
> Then, I will need to update the DEVFREQ core as well.
> There will be two locks in devfreq.c: the global devfreq_list_lock and
> per-devfreq devfreq_lock in struct devfreq.
> And the role of devfreq_lock will be protecting the access to struct
> devfreq (some functions in devfreq.c will use this lock and some usage
> of devfreq_list_lock will be removed).
>
> This will result in more general sysfs interface (or functions other
> than the standard ops) support in governors.
Sounds good. Also, please add a comment somewhere in devfreq.h
(probably above definition for struct devfreq) that the lock must also
be held by governors when accessing devfreq->data.
Regards,
Mike
> Thank you so much!
>
>
> Cheers,
> MyungJoo.
>
>>
>> Regards,
>> Mike
>>
>>> +
>>> +/**
>>> * devfreq_do() - Check the usage profile of a given device and configure
>>> * frequency and voltage accordingly
>>> * @devfreq: devfreq info of the given device
>>> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
>>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>>> error, devfreq->governor->name);
>>>
>>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>>> + &dev_attr_group);
>>> list_del(&devfreq->node);
>>> kfree(devfreq);
>>>
>>> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>>> queue_delayed_work(devfreq_wq, &devfreq_work,
>>> devfreq->next_polling);
>>> }
>>> +
>>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>>> out:
>>> mutex_unlock(&devfreq_list_lock);
>>>
>>> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
>>> goto out;
>>> }
>>>
>>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>>> +
>>> list_del(&devfreq->node);
>>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>>> kfree(devfreq);
>>> @@ -279,6 +303,132 @@ out:
>>> return 0;
>>> }
>>>
>>> +static ssize_t show_governor(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->governor)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%s\n", df->governor->name);
>>> +}
>>> +
>>> +static ssize_t show_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> +
>>> + return sprintf(buf, "%lu\n", df->previous_freq);
>>> +}
>>> +
>>> +static ssize_t show_max_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned long freq = ULONG_MAX;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_floor(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_min_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned long freq = 0;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_ceil(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_polling_interval(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->profile)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>>> +}
>>> +
>>> +static ssize_t store_polling_interval(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned int value;
>>> + int ret;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->profile)
>>> + return -EINVAL;
>>> +
>>> + ret = sscanf(buf, "%u", &value);
>>> + if (ret != 1)
>>> + return -EINVAL;
>>> +
>>> + df->profile->polling_ms = value;
>>> + df->next_polling = df->polling_jiffies
>>> + = msecs_to_jiffies(value);
>>> +
>>> + mutex_lock(&devfreq_list_lock);
>>> + if (df->next_polling > 0 && !polling) {
>>> + polling = true;
>>> + queue_delayed_work(devfreq_wq, &devfreq_work,
>>> + df->next_polling);
>>> + }
>>> + mutex_unlock(&devfreq_list_lock);
>>> +
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
>>> + store_polling_interval);
>>> +static struct attribute *dev_entries[] = {
>>> + &dev_attr_devfreq_governor.attr,
>>> + &dev_attr_devfreq_cur_freq.attr,
>>> + &dev_attr_devfreq_max_freq.attr,
>>> + &dev_attr_devfreq_min_freq.attr,
>>> + &dev_attr_devfreq_polling_interval.attr,
>>> + NULL,
>>> +};
>>> +static struct attribute_group dev_attr_group = {
>>> + .name = power_group_name,
>>> + .attrs = dev_entries,
>>> +};
>>> +
>>> /**
>>> * devfreq_init() - Initialize data structure for devfreq framework and
>>> * start polling registered devfreq devices.
>>> --
>>> 1.7.4.1
>>>
>>>
>>
>
>
>
> --
> MyungJoo Ham (함명주), Ph.D.
> Mobile Software Platform Lab,
> Digital Media and Communications (DMC) Business
> Samsung Electronics
> cell: 82-10-6714-2858
>
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: Turquette, Mike @ 2011-08-30 17:09 UTC (permalink / raw)
To: myungjoo.ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJ0PZbQcfg+e9D3Ne-U4kyAbR4p2-Ua0bJ=sq7Ko9sCkjooAbw@mail.gmail.com>
On Mon, Aug 29, 2011 at 9:19 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@ti.com> wrote:
>> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>> +What: /sys/devices/.../power/devfreq_userspace_set_freq
>>
>> How about just .../devfreq_set_freq? I think the userspace bit is
>> implied and the name is very long.
>>
>
> Umm.. as this entry became userspace specific, I though I need it be
> to distinguished from entries created by devfreq framework. However,
> this one is created only while userspace is being used (device may
> replace governors by calling remove and add); thus, there wouldn't be
> any duplicated name issues. So, I think removing userspace from the
> name should be fine. I will do so in the next revision.
It is your choice. You have a good point that it is specific to the
"userspace" governor. I hadn't thought of that at the time, so I
don't care either way.
Regards,
Mike
>>> +
>>> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct devfreq *devfreq = get_devfreq(dev);
>>> + struct userspace_data *data;
>>> + int err = 0;
>>> +
>>> + if (IS_ERR(devfreq)) {
>>> + err = PTR_ERR(devfreq);
>>> + goto out;
>>> + }
>>> + data = devfreq->data;
>>> +
>>> + if (data->valid)
>>> + err = sprintf(buf, "%lu\n", data->user_frequency);
>>> + else
>>> + err = sprintf(buf, "undefined\n");
>>> +out:
>>> + return err;
>>> +}
>>
>> Shouldn't accesses to devfreq->data be protected by a mutex?
>>
>> Regards,
>> Mike
>
>
> No, it doesn't need mutex here. Although generally, a mutex will be
> needed for simliar codes, in this specific case, devfreq->data does
> not need a mutex protection.
>
> There are two variables in data: "user_frequency" and "valid"
> - valid is initially false and becomes true at store_freq() and never changes.
> - store_freq() writes user_frequency and then writes valid as true.
> (no one writes false on valid)
> - user_frequency is read only when valid is true.
>
> Thus, the case where mutex protection serializes with some dictinction is:
> 1. Init. (valid = false)
> 2. store_freq() writes user_frequency = X
> 3. show_freq()/devfreq_userspace_func() reads valid (false)
> 4. store_freq() writes valid = true
> 5. show_freq()/devfreq_userspace_func() returns without reading
> devfreq_userspace_func()
> 6. store_freq() returns.
>
> into
> A.
> 1. Init (valid = false)
> 2. store_freq() starts and finishes
> 3. show_freq()/devfreq_userspace_func() starts and finishes
> B.
> 1. Init (valid = false)
> 2. show_freq()/devfreq_userspace_func() starts and finishes
> 3. store_freq() starts and finishes
>
> where B results in the same thing as non-mutex version does.
>
>
> If there is an operation that makes valid false, we will need a mutex
> (local to the governor) anyway.
>
>
>
> Anyway, I agree with you on the point that governors might need a
> locking mechanism in general; not just on the private data, but on the
> devfreq access. I'll put more on this issue on the other thread.
>
>
>
> Thank you.
>
>
> Cheers,
> MyungJoo
>
>
>>
>>> +
>>> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
>>> +static struct attribute *dev_entries[] = {
>>> + &dev_attr_devfreq_userspace_set_freq.attr,
>>> + NULL,
>>> +};
>>> +static struct attribute_group dev_attr_group = {
>>> + .name = power_group_name,
>>> + .attrs = dev_entries,
>>> +};
>>> +
>>> +static int userspace_init(struct devfreq *devfreq)
>>> +{
>>> + int err = 0;
>>> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
>>> + GFP_KERNEL);
>>> +
>>> + if (!data) {
>>> + err = -ENOMEM;
>>> + goto out;
>>> + }
>>> + data->valid = false;
>>> + devfreq->data = data;
>>> +
>>> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
>>> +out:
>>> + return err;
>>> +}
>>> +
>>> +static void userspace_exit(struct devfreq *devfreq)
>>> +{
>>> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
>>> + kfree(devfreq->data);
>>> + devfreq->data = NULL;
>>> +}
>>> +
>>> +struct devfreq_governor devfreq_userspace = {
>>> + .name = "userspace",
>>> + .get_target_freq = devfreq_userspace_func,
>>> + .init = userspace_init,
>>> + .exit = userspace_exit,
>>> +};
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index fdc6916..cbafcdf 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -13,6 +13,7 @@
>>> #ifndef __LINUX_DEVFREQ_H__
>>> #define __LINUX_DEVFREQ_H__
>>>
>>> +#include <linux/opp.h>
>>> #include <linux/notifier.h>
>>>
>>> #define DEVFREQ_NAME_LEN 16
>>> @@ -65,6 +66,8 @@ struct devfreq_governor {
>>> * "devfreq_monitor" executions to reevaluate
>>> * frequency/voltage of the device. Set by
>>> * profile's polling_ms interval.
>>> + * @user_set_freq User specified adequete frequency value (thru sysfs
>>> + * interface). Governors may and may not use this value.
>>> * @data Private data of the governor. The devfreq framework does not
>>> * touch this.
>>> *
>>> @@ -82,6 +85,7 @@ struct devfreq {
>>> unsigned long previous_freq;
>>> unsigned int next_polling;
>>>
>>> + unsigned long user_set_freq; /* governors may ignore this. */
>>> void *data; /* private data for governors */
>>> };
>>>
>>> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
>>> struct devfreq_governor *governor,
>>> void *data);
>>> extern int devfreq_remove_device(struct device *dev);
>>> +
>>> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
>>> +extern struct devfreq_governor devfreq_powersave;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
>>> +extern struct devfreq_governor devfreq_performance;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
>>> +extern struct devfreq_governor devfreq_userspace;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
>>> +extern struct devfreq_governor devfreq_simple_ondemand;
>>> +/**
>>> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
>>> + * and devfreq_add_device
>>> + * @ upthreshold If the load is over this value, the frequency jumps.
>>> + * Specify 0 to use the default. Valid value = 0 to 100.
>>> + * @ downdifferential If the load is under upthreshold - downdifferential,
>>> + * the governor may consider slowing the frequency down.
>>> + * Specify 0 to use the default. Valid value = 0 to 100.
>>> + * downdifferential < upthreshold must hold.
>>> + *
>>> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
>>> + * the governor uses the default values.
>>> + */
>>> +struct devfreq_simple_ondemand_data {
>>> + unsigned int upthreshold;
>>> + unsigned int downdifferential;
>>> +};
>>> +#endif
>>> +
>>> #else /* !CONFIG_PM_DEVFREQ */
>>> static int devfreq_add_device(struct device *dev,
>>> struct devfreq_dev_profile *profile,
>>> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
>>> {
>>> return 0;
>>> }
>>> +
>>> +#define devfreq_powersave NULL
>>> +#define devfreq_performance NULL
>>> +#define devfreq_userspace NULL
>>> +#define devfreq_simple_ondemand NULL
>>> +
>>> #endif /* CONFIG_PM_DEVFREQ */
>>>
>>> #endif /* __LINUX_DEVFREQ_H__ */
>>> --
>>> 1.7.4.1
>>>
>>>
>>
>
>
>
> --
> MyungJoo Ham (함명주), Ph.D.
> Mobile Software Platform Lab,
> Digital Media and Communications (DMC) Business
> Samsung Electronics
> cell: 82-10-6714-2858
>
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v2 4/4] bq24022: Add support for the bq2407x family
From: Mark Brown @ 2011-08-30 10:52 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291759.35396.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:59:35PM +0200, Heiko Stübner wrote:
> The bq2407x family of charger ICs simply supports another machine
> specific charging current above 500mA. To use this setting a
> second gpio pin is used while the rest of the pins stay the same.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH 2/4] bq24022: Use dev_err instead of dev_dbg for error messages
From: Mark Brown @ 2011-08-30 10:40 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291757.30189.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:57:29PM +0200, Heiko Stübner wrote:
> This makes error messages visible to the user,
> so he/she can eventually fix them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH v2 1/4] bq24022: Evaluate returns of gpio_direction_output-calls
From: Mark Brown @ 2011-08-30 10:39 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291756.17516.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:56:17PM +0200, Heiko Stübner wrote:
> It wasn't done before.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: MyungJoo Ham @ 2011-08-30 4:28 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJOA=zNHSC0kBMhRw1uf=8E7PE2Da6wXAWnG=mgh2M8-pa6w6g@mail.gmail.com>
On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@ti.com> wrote:
>
> Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
> show_freq and restore_freq functions for the userspace governor do not
> protect the private data accesses with a mutex. Looking a bit further
> I see that they do call get_devfreq; I think the semantics for this
> are wrong.
>
> get_devfreq only protects the list as long as it takes to do a lookup.
> However neither struct devfreq or struct userspace_data have a mutex
> associated with them, so concurrent operations are not protected.
>
> One heavy-handed way of solving this is to not have get_devfreq
> release the mutex, and then have those functions call a new
> put_devfreq which does release the mutex. However that is really bad
> locking.
>
> What do you think about putting a mutex in struct devfreq? The rule
> for governors can be that access to the governor-specific private data
> requires taking that devfreq's mutex.
A lock in private data at DEVFREQ framework won't be needed as it is a
governor-specific issue; however, it appears that we need a locking
mechanism for accessing struct devfreq in general for governors.
Although we do not have any governor that writes something or reads
multiple times on struct devfreq (except for the private data) out of
get_target_freq ops, which does not need an additional lock, we may
have one soon.
Then, I will need to update the DEVFREQ core as well.
There will be two locks in devfreq.c: the global devfreq_list_lock and
per-devfreq devfreq_lock in struct devfreq.
And the role of devfreq_lock will be protecting the access to struct
devfreq (some functions in devfreq.c will use this lock and some usage
of devfreq_list_lock will be removed).
This will result in more general sysfs interface (or functions other
than the standard ops) support in governors.
Thank you so much!
Cheers,
MyungJoo.
>
> Regards,
> Mike
>
>> +
>> +/**
>> * devfreq_do() - Check the usage profile of a given device and configure
>> * frequency and voltage accordingly
>> * @devfreq: devfreq info of the given device
>> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>> error, devfreq->governor->name);
>>
>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>> + &dev_attr_group);
>> list_del(&devfreq->node);
>> kfree(devfreq);
>>
>> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>> queue_delayed_work(devfreq_wq, &devfreq_work,
>> devfreq->next_polling);
>> }
>> +
>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>> out:
>> mutex_unlock(&devfreq_list_lock);
>>
>> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
>> goto out;
>> }
>>
>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>> +
>> list_del(&devfreq->node);
>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>> kfree(devfreq);
>> @@ -279,6 +303,132 @@ out:
>> return 0;
>> }
>>
>> +static ssize_t show_governor(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->governor)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%s\n", df->governor->name);
>> +}
>> +
>> +static ssize_t show_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> +
>> + return sprintf(buf, "%lu\n", df->previous_freq);
>> +}
>> +
>> +static ssize_t show_max_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = ULONG_MAX;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_floor(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_min_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = 0;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_ceil(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_polling_interval(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>> +}
>> +
>> +static ssize_t store_polling_interval(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned int value;
>> + int ret;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + ret = sscanf(buf, "%u", &value);
>> + if (ret != 1)
>> + return -EINVAL;
>> +
>> + df->profile->polling_ms = value;
>> + df->next_polling = df->polling_jiffies
>> + = msecs_to_jiffies(value);
>> +
>> + mutex_lock(&devfreq_list_lock);
>> + if (df->next_polling > 0 && !polling) {
>> + polling = true;
>> + queue_delayed_work(devfreq_wq, &devfreq_work,
>> + df->next_polling);
>> + }
>> + mutex_unlock(&devfreq_list_lock);
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
>> + store_polling_interval);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_governor.attr,
>> + &dev_attr_devfreq_cur_freq.attr,
>> + &dev_attr_devfreq_max_freq.attr,
>> + &dev_attr_devfreq_min_freq.attr,
>> + &dev_attr_devfreq_polling_interval.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> /**
>> * devfreq_init() - Initialize data structure for devfreq framework and
>> * start polling registered devfreq devices.
>> --
>> 1.7.4.1
>>
>>
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: MyungJoo Ham @ 2011-08-30 4:19 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJOA=zO6VUXEi1ymyi2sdv03VWgPFiq1Xus9V1-YHNjpnKRo2A@mail.gmail.com>
On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> +What: /sys/devices/.../power/devfreq_userspace_set_freq
>
> How about just .../devfreq_set_freq? I think the userspace bit is
> implied and the name is very long.
>
Umm.. as this entry became userspace specific, I though I need it be
to distinguished from entries created by devfreq framework. However,
this one is created only while userspace is being used (device may
replace governors by calling remove and add); thus, there wouldn't be
any duplicated name issues. So, I think removing userspace from the
name should be fine. I will do so in the next revision.
>> +
>> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct devfreq *devfreq = get_devfreq(dev);
>> + struct userspace_data *data;
>> + int err = 0;
>> +
>> + if (IS_ERR(devfreq)) {
>> + err = PTR_ERR(devfreq);
>> + goto out;
>> + }
>> + data = devfreq->data;
>> +
>> + if (data->valid)
>> + err = sprintf(buf, "%lu\n", data->user_frequency);
>> + else
>> + err = sprintf(buf, "undefined\n");
>> +out:
>> + return err;
>> +}
>
> Shouldn't accesses to devfreq->data be protected by a mutex?
>
> Regards,
> Mike
No, it doesn't need mutex here. Although generally, a mutex will be
needed for simliar codes, in this specific case, devfreq->data does
not need a mutex protection.
There are two variables in data: "user_frequency" and "valid"
- valid is initially false and becomes true at store_freq() and never changes.
- store_freq() writes user_frequency and then writes valid as true.
(no one writes false on valid)
- user_frequency is read only when valid is true.
Thus, the case where mutex protection serializes with some dictinction is:
1. Init. (valid = false)
2. store_freq() writes user_frequency = X
3. show_freq()/devfreq_userspace_func() reads valid (false)
4. store_freq() writes valid = true
5. show_freq()/devfreq_userspace_func() returns without reading
devfreq_userspace_func()
6. store_freq() returns.
into
A.
1. Init (valid = false)
2. store_freq() starts and finishes
3. show_freq()/devfreq_userspace_func() starts and finishes
B.
1. Init (valid = false)
2. show_freq()/devfreq_userspace_func() starts and finishes
3. store_freq() starts and finishes
where B results in the same thing as non-mutex version does.
If there is an operation that makes valid false, we will need a mutex
(local to the governor) anyway.
Anyway, I agree with you on the point that governors might need a
locking mechanism in general; not just on the private data, but on the
devfreq access. I'll put more on this issue on the other thread.
Thank you.
Cheers,
MyungJoo
>
>> +
>> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_userspace_set_freq.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> +static int userspace_init(struct devfreq *devfreq)
>> +{
>> + int err = 0;
>> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
>> + GFP_KERNEL);
>> +
>> + if (!data) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>> + data->valid = false;
>> + devfreq->data = data;
>> +
>> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
>> +out:
>> + return err;
>> +}
>> +
>> +static void userspace_exit(struct devfreq *devfreq)
>> +{
>> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
>> + kfree(devfreq->data);
>> + devfreq->data = NULL;
>> +}
>> +
>> +struct devfreq_governor devfreq_userspace = {
>> + .name = "userspace",
>> + .get_target_freq = devfreq_userspace_func,
>> + .init = userspace_init,
>> + .exit = userspace_exit,
>> +};
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index fdc6916..cbafcdf 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -13,6 +13,7 @@
>> #ifndef __LINUX_DEVFREQ_H__
>> #define __LINUX_DEVFREQ_H__
>>
>> +#include <linux/opp.h>
>> #include <linux/notifier.h>
>>
>> #define DEVFREQ_NAME_LEN 16
>> @@ -65,6 +66,8 @@ struct devfreq_governor {
>> * "devfreq_monitor" executions to reevaluate
>> * frequency/voltage of the device. Set by
>> * profile's polling_ms interval.
>> + * @user_set_freq User specified adequete frequency value (thru sysfs
>> + * interface). Governors may and may not use this value.
>> * @data Private data of the governor. The devfreq framework does not
>> * touch this.
>> *
>> @@ -82,6 +85,7 @@ struct devfreq {
>> unsigned long previous_freq;
>> unsigned int next_polling;
>>
>> + unsigned long user_set_freq; /* governors may ignore this. */
>> void *data; /* private data for governors */
>> };
>>
>> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
>> struct devfreq_governor *governor,
>> void *data);
>> extern int devfreq_remove_device(struct device *dev);
>> +
>> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
>> +extern struct devfreq_governor devfreq_powersave;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
>> +extern struct devfreq_governor devfreq_performance;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
>> +extern struct devfreq_governor devfreq_userspace;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +extern struct devfreq_governor devfreq_simple_ondemand;
>> +/**
>> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
>> + * and devfreq_add_device
>> + * @ upthreshold If the load is over this value, the frequency jumps.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * @ downdifferential If the load is under upthreshold - downdifferential,
>> + * the governor may consider slowing the frequency down.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * downdifferential < upthreshold must hold.
>> + *
>> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
>> + * the governor uses the default values.
>> + */
>> +struct devfreq_simple_ondemand_data {
>> + unsigned int upthreshold;
>> + unsigned int downdifferential;
>> +};
>> +#endif
>> +
>> #else /* !CONFIG_PM_DEVFREQ */
>> static int devfreq_add_device(struct device *dev,
>> struct devfreq_dev_profile *profile,
>> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
>> {
>> return 0;
>> }
>> +
>> +#define devfreq_powersave NULL
>> +#define devfreq_performance NULL
>> +#define devfreq_userspace NULL
>> +#define devfreq_simple_ondemand NULL
>> +
>> #endif /* CONFIG_PM_DEVFREQ */
>>
>> #endif /* __LINUX_DEVFREQ_H__ */
>> --
>> 1.7.4.1
>>
>>
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Rafael J. Wysocki @ 2011-08-29 20:55 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
linux-pm, Thomas Gleixner
In-Reply-To: <CAJOA=zN_TqAz5tyXp3uRe4MezxLDAF5Dhzb6kst2xWi9zzr5dg@mail.gmail.com>
Hi,
On Monday, August 29, 2011, Turquette, Mike wrote:
> Rafael,
>
> Locking doesn't seem safe in v8; I replied with my comments.
>
> I should be free this week to review the next round a little more
> quickly.
Cool, thanks a lot for your hard work as a reviewer.
> Last week was hectic.
Sorry about that.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Turquette, Mike @ 2011-08-29 19:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
linux-pm, Thomas Gleixner
In-Reply-To: <201108272235.39334.rjw@sisk.pl>
Rafael,
Locking doesn't seem safe in v8; I replied with my comments.
I should be free this week to review the next round a little more
quickly. Last week was hectic.
Regards,
Mike
2011/8/27 Rafael J. Wysocki <rjw@sisk.pl>:
> Mike, are patches [3-5/5] in this revision fine by you?
>
> Rafael
>
>
> On Wednesday, August 24, 2011, MyungJoo Ham wrote:
>> The patchset revision v8 has minor updates since v7 and v6.
>> - Allow governors to have their own sysfs interface and init/exit callbacks.
>>
>> The patches 1/5 (OPP notifier) and 2/5 (DEVFREQ core) have no changes since v7.
>> There has been reordering between "add common sysfs interfaces" patch
>> and "add basic governors" (3/5 and 5/5)
>> "add internal interfaces for governors (4/5)" patch has been newly
>> introduced at v8 patchset.
>>
>> For a usage example, please look at
>> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
>>
>> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
>> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
>> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
>> and other related clocks simply follow the determined DDR RAM clock.
>>
>> The DEVFREQ driver for Exynos4210 memory bus is at
>> /drivers/devfreq/exynos4210_memorybus.c in the git tree.
>>
>> In the dd (writing and reading 360MiB) test with NURI board, the memory
>> throughput was not changed (the performance is not deteriorated) while
>> the SoC power consumption has been reduced by 1%. When the memory access
>> is not that intense while the CPU is heavily used, the SoC power consumption
>> has been reduced by 6%. The power consumption has been compared with the
>> case using the conventional Exynos4210 CPUFREQ driver, which sets memory
>> bus frequency according to the CPU core frequency. Besides, when the CPU core
>> running slow and the memory access is intense, the performance (memory
>> throughput) has been increased by 11% (with higher SoC power consumption of
>> 5%). The tested governor is "simple-ondemand".
>>
>> MyungJoo Ham (5):
>> PM / OPP: Add OPP availability change notifier.
>> PM: Introduce DEVFREQ: generic DVFS framework with device-specific
>> OPPs
>> PM / DEVFREQ: add common sysfs interfaces
>> PM / DEVFREQ: add internal interfaces for governors
>> PM / DEVFREQ: add basic governors
>>
>> Documentation/ABI/testing/sysfs-devices-power | 46 +++
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/base/power/opp.c | 29 ++
>> drivers/devfreq/Kconfig | 75 ++++
>> drivers/devfreq/Makefile | 5 +
>> drivers/devfreq/devfreq.c | 463 +++++++++++++++++++++++++
>> drivers/devfreq/governor.h | 20 +
>> drivers/devfreq/governor_performance.c | 24 ++
>> drivers/devfreq/governor_powersave.c | 24 ++
>> drivers/devfreq/governor_simpleondemand.c | 88 +++++
>> drivers/devfreq/governor_userspace.c | 119 +++++++
>> include/linux/devfreq.h | 150 ++++++++
>> include/linux/opp.h | 12 +
>> 14 files changed, 1059 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/devfreq/Kconfig
>> create mode 100644 drivers/devfreq/Makefile
>> create mode 100644 drivers/devfreq/devfreq.c
>> create mode 100644 drivers/devfreq/governor.h
>> create mode 100644 drivers/devfreq/governor_performance.c
>> create mode 100644 drivers/devfreq/governor_powersave.c
>> create mode 100644 drivers/devfreq/governor_simpleondemand.c
>> create mode 100644 drivers/devfreq/governor_userspace.c
>> create mode 100644 include/linux/devfreq.h
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v8 4/5] PM / DEVFREQ: add internal interfaces for governors
From: Turquette, Mike @ 2011-08-29 19:21 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-5-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> - get_devfreq(): governors may convert struct dev -> struct devfreq
> - update_devfreq(): governors may notify DEVFREQ core to reevaluate
> frequencies.
> - Governors may have .init and .exit callbacks
>
> In order to use the internal interface, get_devfreq() and
> update_devfreq(), governor should include "governor.h"
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Please see patch 3 and 5 for my thoughts on locking in struct devfreq.
Also, this patch doesn't need to be separate, but can be rolled into
patch 3.
Regards,
Mike
> ---
> drivers/devfreq/devfreq.c | 32 ++++++++++++++++++++++++--------
> drivers/devfreq/governor.h | 20 ++++++++++++++++++++
> include/linux/devfreq.h | 2 ++
> 3 files changed, 46 insertions(+), 8 deletions(-)
> create mode 100644 drivers/devfreq/governor.h
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 5bbece6..8de3b21 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -65,10 +65,10 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>
> /**
> * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> - * with mutex protection.
> + * with mutex protection. exported for governors
> * @dev: device pointer used to lookup device devfreq.
> */
> -static struct devfreq *get_devfreq(struct device *dev)
> +struct devfreq *get_devfreq(struct device *dev)
> {
> struct devfreq *ret;
>
> @@ -113,17 +113,15 @@ static int devfreq_do(struct devfreq *devfreq)
> }
>
> /**
> - * devfreq_update() - Notify that the device OPP has been changed.
> - * @dev: the device whose OPP has been changed.
> + * update_devfreq() - Notify that the device OPP or frequency requirement
> + * has been changed. This function is exported for governors.
> + * @devfreq: the devfreq instance.
> */
> -static int devfreq_update(struct notifier_block *nb, unsigned long type,
> - void *devp)
> +int update_devfreq(struct devfreq *devfreq)
> {
> - struct devfreq *devfreq;
> int err = 0;
>
> mutex_lock(&devfreq_list_lock);
> - devfreq = container_of(nb, struct devfreq, nb);
> /* Reevaluate the proper frequency */
> err = devfreq_do(devfreq);
> mutex_unlock(&devfreq_list_lock);
> @@ -131,6 +129,18 @@ static int devfreq_update(struct notifier_block *nb, unsigned long type,
> }
>
> /**
> + * devfreq_update() - Notify that the device OPP has been changed.
> + * @dev: the device whose OPP has been changed.
> + */
> +static int devfreq_update(struct notifier_block *nb, unsigned long type,
> + void *devp)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> +
> + return update_devfreq(devfreq);
> +}
> +
> +/**
> * devfreq_monitor() - Periodically run devfreq_do()
> * @work: the work struct used to run devfreq_monitor periodically.
> *
> @@ -254,6 +264,9 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>
> list_add(&devfreq->node, &devfreq_list);
>
> + if (governor->init)
> + governor->init(devfreq);
> +
> if (devfreq_wq && devfreq->next_polling && !polling) {
> polling = true;
> queue_delayed_work(devfreq_wq, &devfreq_work,
> @@ -295,6 +308,9 @@ int devfreq_remove_device(struct device *dev)
>
> sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>
> + if (devfreq->governor->exit)
> + devfreq->governor->exit(devfreq);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> new file mode 100644
> index 0000000..bb5d964
> --- /dev/null
> +++ b/drivers/devfreq/governor.h
> @@ -0,0 +1,20 @@
> +/*
> + * governor.h - internal header for governors.
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@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.
> + *
> + * This header is for devfreq governors in drivers/devfreq/
> + */
> +
> +#ifndef _GOVERNOR_H
> +#define _GOVERNOR_H
> +
> +extern struct devfreq *get_devfreq(struct device *dev);
> +extern int update_devfreq(struct devfreq *devfreq);
> +
> +#endif /* _GOVERNOR_H */
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 8252238..fdc6916 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -46,6 +46,8 @@ struct devfreq_dev_profile {
> struct devfreq_governor {
> char name[DEVFREQ_NAME_LEN];
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
> + int (*init)(struct devfreq *this);
> + void (*exit)(struct devfreq *this);
> };
>
> /**
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-29 19:17 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-4-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Device specific sysfs interface /sys/devices/.../power/devfreq_*
> - governor R: name of governor
> - cur_freq R: current frequency
> - max_freq R: maximum operable frequency
> - min_freq R: minimum operable frequency
> - polling_interval R: polling interval in ms given with devfreq profile
> W: update polling interval.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> --
> Changed from v7
> - removed set_freq from the common devfreq interface
> - added get_devfreq, a mutex-protected wrapper for find_device_devfreq
> (for sysfs interfaces and later with governor-support)
> - corrected ABI documentation.
>
> Changed from v6
> - poling_interval is writable.
>
> Changed from v5
> - updated devferq_update usage.
>
> Changed from v4
> - removed system-wide sysfs interface
> - removed tickling sysfs interface
> - added set_freq for userspace governor (and any other governors that
> require user input)
>
> Changed from v3
> - corrected sysfs API usage
> - corrected error messages
> - moved sysfs entry location
> - added sysfs entries
>
> Changed from v2
> - add ABI entries for devfreq sysfs interface
> ---
> Documentation/ABI/testing/sysfs-devices-power | 37 ++++++
> drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++
> 2 files changed, 187 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 8ffbc25..57f4591 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -165,3 +165,40 @@ Description:
>
> Not all drivers support this attribute. If it isn't supported,
> attempts to read or write it will yield I/O errors.
> +
> +What: /sys/devices/.../power/devfreq_governor
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_governor shows the name
> + of the governor used by the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_cur_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_cur_freq shows the current
> + frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_max_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_max_freq shows the
> + maximum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_min_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_min_freq shows the
> + minimum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_polling_interval
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_polling_interval sets and
> + shows the requested polling interval of the corresponding
> + device. The values are represented in ms. If the value is less
> + than 1 jiffy, it is considered to be 0, which means no polling.
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index df63bdc..5bbece6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work;
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static struct attribute_group dev_attr_group;
> +
> /**
> * find_device_devfreq() - find devfreq struct using device pointer
> * @dev: device pointer used to lookup device devfreq.
> @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> }
>
> /**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection.
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +static struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
> +
> + return ret;
> +}
Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
show_freq and restore_freq functions for the userspace governor do not
protect the private data accesses with a mutex. Looking a bit further
I see that they do call get_devfreq; I think the semantics for this
are wrong.
get_devfreq only protects the list as long as it takes to do a lookup.
However neither struct devfreq or struct userspace_data have a mutex
associated with them, so concurrent operations are not protected.
One heavy-handed way of solving this is to not have get_devfreq
release the mutex, and then have those functions call a new
put_devfreq which does release the mutex. However that is really bad
locking.
What do you think about putting a mutex in struct devfreq? The rule
for governors can be that access to the governor-specific private data
requires taking that devfreq's mutex.
Regards,
Mike
> +
> +/**
> * devfreq_do() - Check the usage profile of a given device and configure
> * frequency and voltage accordingly
> * @devfreq: devfreq info of the given device
> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> error, devfreq->governor->name);
>
> + sysfs_unmerge_group(&devfreq->dev->kobj,
> + &dev_attr_group);
> list_del(&devfreq->node);
> kfree(devfreq);
>
> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
> queue_delayed_work(devfreq_wq, &devfreq_work,
> devfreq->next_polling);
> }
> +
> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
> out:
> mutex_unlock(&devfreq_list_lock);
>
> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
> goto out;
> }
>
> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> @@ -279,6 +303,132 @@ out:
> return 0;
> }
>
> +static ssize_t show_governor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->governor)
> + return -EINVAL;
> +
> + return sprintf(buf, "%s\n", df->governor->name);
> +}
> +
> +static ssize_t show_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> +
> + return sprintf(buf, "%lu\n", df->previous_freq);
> +}
> +
> +static ssize_t show_max_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = ULONG_MAX;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_floor(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_min_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = 0;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_ceil(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_polling_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + return sprintf(buf, "%d\n", df->profile->polling_ms);
> +}
> +
> +static ssize_t store_polling_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned int value;
> + int ret;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + ret = sscanf(buf, "%u", &value);
> + if (ret != 1)
> + return -EINVAL;
> +
> + df->profile->polling_ms = value;
> + df->next_polling = df->polling_jiffies
> + = msecs_to_jiffies(value);
> +
> + mutex_lock(&devfreq_list_lock);
> + if (df->next_polling > 0 && !polling) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work,
> + df->next_polling);
> + }
> + mutex_unlock(&devfreq_list_lock);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
> + store_polling_interval);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_governor.attr,
> + &dev_attr_devfreq_cur_freq.attr,
> + &dev_attr_devfreq_max_freq.attr,
> + &dev_attr_devfreq_min_freq.attr,
> + &dev_attr_devfreq_polling_interval.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> /**
> * devfreq_init() - Initialize data structure for devfreq framework and
> * start polling registered devfreq devices.
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: Turquette, Mike @ 2011-08-29 18:58 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-6-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Four CPUFREQ-like governors are provided as examples.
>
> powersave: use the lowest frequency possible. The user (device) should
> set the polling_ms as 0 because polling is useless for this governor.
>
> performance: use the highest freqeuncy possible. The user (device)
> should set the polling_ms as 0 because polling is useless for this
> governor.
>
> userspace: use the user specified frequency stored at
> devfreq.user_set_freq. With sysfs support in the following patch, a user
> may set the value with the sysfs interface.
>
> simple_ondemand: simplified version of CPUFREQ's ONDEMAND governor.
>
> When a user updates OPP entries (enable/disable/add), OPP framework
> automatically notifies DEVFREQ to update operating frequency
> accordingly. Thus, DEVFREQ users (device drivers) do not need to update
> DEVFREQ manually with OPP entry updates or set polling_ms for powersave
> , performance, userspace, or any other "static" governors.
>
> Note that these are given only as basic examples for governors and any
> devices with DEVFREQ may implement their own governors with the drivers
> and use them.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> ---
> Changed from v7
> - Userspace uses its own sysfs interface.
>
> Changed from v5
> - Seperated governor files from devfreq.c
> - Allow simple ondemand to be tuned for each device
> ---
> Documentation/ABI/testing/sysfs-devices-power | 9 ++
> drivers/devfreq/Kconfig | 36 ++++++++
> drivers/devfreq/Makefile | 4 +
> drivers/devfreq/governor_performance.c | 24 +++++
> drivers/devfreq/governor_powersave.c | 24 +++++
> drivers/devfreq/governor_simpleondemand.c | 88 ++++++++++++++++++
> drivers/devfreq/governor_userspace.c | 119 +++++++++++++++++++++++++
> include/linux/devfreq.h | 41 +++++++++
> 8 files changed, 345 insertions(+), 0 deletions(-)
> create mode 100644 drivers/devfreq/governor_performance.c
> create mode 100644 drivers/devfreq/governor_powersave.c
> create mode 100644 drivers/devfreq/governor_simpleondemand.c
> create mode 100644 drivers/devfreq/governor_userspace.c
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 57f4591..c7f6977 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -202,3 +202,12 @@ Description:
> shows the requested polling interval of the corresponding
> device. The values are represented in ms. If the value is less
> than 1 jiffy, it is considered to be 0, which means no polling.
> +
> +What: /sys/devices/.../power/devfreq_userspace_set_freq
How about just .../devfreq_set_freq? I think the userspace bit is
implied and the name is very long.
> +Date: August 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_userspace_set_freq sets
> + and shows the user specified frequency in kHz. This sysfs
> + entry is created and managed by userspace DEVFREQ governor.
> + If other governors are used, it won't be supported.
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 1fb42de..643b055 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -34,6 +34,42 @@ menuconfig PM_DEVFREQ
>
> if PM_DEVFREQ
>
> +comment "DEVFREQ Governors"
> +
> +config DEVFREQ_GOV_SIMPLE_ONDEMAND
> + bool "Simple Ondemand"
> + help
> + Chooses frequency based on the recent load on the device. Works
> + similar as ONDEMAND governor of CPUFREQ does. A device with
> + Simple-Ondemand should be able to provide busy/total counter
> + values that imply the usage rate. A device may provide tuned
> + values to the governor with data field at devfreq_add_device().
> +
> +config DEVFREQ_GOV_PERFORMANCE
> + bool "Performance"
> + help
> + Sets the frequency at the maximum available frequency.
> + This governor always returns UINT_MAX as frequency so that
> + the DEVFREQ framework returns the highest frequency available
> + at any time.
> +
> +config DEVFREQ_GOV_POWERSAVE
> + bool "Powersave"
> + help
> + Sets the frequency at the minimum available frequency.
> + This governor always returns 0 as frequency so that
> + the DEVFREQ framework returns the lowest frequency available
> + at any time.
> +
> +config DEVFREQ_GOV_USERSPACE
> + bool "Userspace"
> + help
> + Sets the frequency at the user specified one.
> + This governor returns the user configured frequency if there
> + has been an input to /sys/devices/.../power/devfreq_set_freq.
> + Otherwise, the governor does not change the frequnecy
> + given at the initialization.
> +
> comment "DEVFREQ Drivers"
>
> endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 168934a..4564a89 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -1 +1,5 @@
> obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
> +obj-$(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) += governor_simpleondemand.o
> +obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
> +obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> +obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
> new file mode 100644
> index 0000000..c47eff8
> --- /dev/null
> +++ b/drivers/devfreq/governor_performance.c
> @@ -0,0 +1,24 @@
> +/*
> + * linux/drivers/devfreq/governor_performance.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@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/devfreq.h>
> +
> +static int devfreq_performance_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + *freq = UINT_MAX; /* devfreq_do will run "floor" */
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_performance = {
> + .name = "performance",
> + .get_target_freq = devfreq_performance_func,
> +};
> diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
> new file mode 100644
> index 0000000..4f128d8
> --- /dev/null
> +++ b/drivers/devfreq/governor_powersave.c
> @@ -0,0 +1,24 @@
> +/*
> + * linux/drivers/devfreq/governor_powersave.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@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/devfreq.h>
> +
> +static int devfreq_powersave_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + *freq = 0; /* devfreq_do will run "ceiling" to 0 */
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_powersave = {
> + .name = "powersave",
> + .get_target_freq = devfreq_powersave_func,
> +};
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> new file mode 100644
> index 0000000..18fe8be
> --- /dev/null
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -0,0 +1,88 @@
> +/*
> + * linux/drivers/devfreq/governor_simpleondemand.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@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/errno.h>
> +#include <linux/devfreq.h>
> +#include <linux/math64.h>
> +
> +/* Default constants for DevFreq-Simple-Ondemand (DFSO) */
> +#define DFSO_UPTHRESHOLD (90)
> +#define DFSO_DOWNDIFFERENCTIAL (5)
> +static int devfreq_simple_ondemand_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + struct devfreq_dev_status stat;
> + int err = df->profile->get_dev_status(df->dev, &stat);
> + unsigned long long a, b;
> + unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
> + unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
> + struct devfreq_simple_ondemand_data *data = df->data;
> +
> + if (err)
> + return err;
> +
> + if (data) {
> + if (data->upthreshold)
> + dfso_upthreshold = data->upthreshold;
> + if (data->downdifferential)
> + dfso_downdifferential = data->downdifferential;
> + }
> + if (dfso_upthreshold > 100 ||
> + dfso_upthreshold < dfso_downdifferential)
> + return -EINVAL;
> +
> + /* Assume MAX if it is going to be divided by zero */
> + if (stat.total_time == 0) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Prevent overflow */
> + if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
> + stat.busy_time >>= 7;
> + stat.total_time >>= 7;
> + }
> +
> + /* Set MAX if it's busy enough */
> + if (stat.busy_time * 100 >
> + stat.total_time * dfso_upthreshold) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Set MAX if we do not know the initial frequency */
> + if (stat.current_frequency == 0) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Keep the current frequency */
> + if (stat.busy_time * 100 >
> + stat.total_time * (dfso_upthreshold - dfso_downdifferential)) {
> + *freq = stat.current_frequency;
> + return 0;
> + }
> +
> + /* Set the desired frequency based on the load */
> + a = stat.busy_time;
> + a *= stat.current_frequency;
> + b = div_u64(a, stat.total_time);
> + b *= 100;
> + b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> + *freq = (unsigned long) b;
> +
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_simple_ondemand = {
> + .name = "simple_ondemand",
> + .get_target_freq = devfreq_simple_ondemand_func,
> +};
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> new file mode 100644
> index 0000000..53a4574
> --- /dev/null
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -0,0 +1,119 @@
> +/*
> + * linux/drivers/devfreq/governor_simpleondemand.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@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/slab.h>
> +#include <linux/device.h>
> +#include <linux/devfreq.h>
> +#include <linux/pm.h>
> +#include "governor.h"
> +
> +struct userspace_data {
> + unsigned long user_frequency;
> + bool valid;
> +};
> +
> +static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
> +{
> + struct userspace_data *data = df->data;
> +
> + if (!data->valid)
> + *freq = df->previous_freq; /* No user freq specified yet */
> + else
> + *freq = data->user_frequency;
> + return 0;
> +}
> +
> +static ssize_t store_freq(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *devfreq = get_devfreq(dev);
> + struct userspace_data *data;
> + unsigned long wanted;
> + int err = 0;
> +
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> + data = devfreq->data;
> +
> + sscanf(buf, "%lu", &wanted);
> + data->user_frequency = wanted;
> + data->valid = true;
> + err = update_devfreq(devfreq);
> + if (err == 0)
> + err = count;
> +out:
> + return err;
> +}
> +
> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct devfreq *devfreq = get_devfreq(dev);
> + struct userspace_data *data;
> + int err = 0;
> +
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> + data = devfreq->data;
> +
> + if (data->valid)
> + err = sprintf(buf, "%lu\n", data->user_frequency);
> + else
> + err = sprintf(buf, "undefined\n");
> +out:
> + return err;
> +}
Shouldn't accesses to devfreq->data be protected by a mutex?
Regards,
Mike
> +
> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_userspace_set_freq.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> +static int userspace_init(struct devfreq *devfreq)
> +{
> + int err = 0;
> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
> + GFP_KERNEL);
> +
> + if (!data) {
> + err = -ENOMEM;
> + goto out;
> + }
> + data->valid = false;
> + devfreq->data = data;
> +
> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
> +out:
> + return err;
> +}
> +
> +static void userspace_exit(struct devfreq *devfreq)
> +{
> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
> + kfree(devfreq->data);
> + devfreq->data = NULL;
> +}
> +
> +struct devfreq_governor devfreq_userspace = {
> + .name = "userspace",
> + .get_target_freq = devfreq_userspace_func,
> + .init = userspace_init,
> + .exit = userspace_exit,
> +};
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fdc6916..cbafcdf 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -13,6 +13,7 @@
> #ifndef __LINUX_DEVFREQ_H__
> #define __LINUX_DEVFREQ_H__
>
> +#include <linux/opp.h>
> #include <linux/notifier.h>
>
> #define DEVFREQ_NAME_LEN 16
> @@ -65,6 +66,8 @@ struct devfreq_governor {
> * "devfreq_monitor" executions to reevaluate
> * frequency/voltage of the device. Set by
> * profile's polling_ms interval.
> + * @user_set_freq User specified adequete frequency value (thru sysfs
> + * interface). Governors may and may not use this value.
> * @data Private data of the governor. The devfreq framework does not
> * touch this.
> *
> @@ -82,6 +85,7 @@ struct devfreq {
> unsigned long previous_freq;
> unsigned int next_polling;
>
> + unsigned long user_set_freq; /* governors may ignore this. */
> void *data; /* private data for governors */
> };
>
> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
> struct devfreq_governor *governor,
> void *data);
> extern int devfreq_remove_device(struct device *dev);
> +
> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
> +extern struct devfreq_governor devfreq_powersave;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
> +extern struct devfreq_governor devfreq_performance;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
> +extern struct devfreq_governor devfreq_userspace;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
> +extern struct devfreq_governor devfreq_simple_ondemand;
> +/**
> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
> + * and devfreq_add_device
> + * @ upthreshold If the load is over this value, the frequency jumps.
> + * Specify 0 to use the default. Valid value = 0 to 100.
> + * @ downdifferential If the load is under upthreshold - downdifferential,
> + * the governor may consider slowing the frequency down.
> + * Specify 0 to use the default. Valid value = 0 to 100.
> + * downdifferential < upthreshold must hold.
> + *
> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
> + * the governor uses the default values.
> + */
> +struct devfreq_simple_ondemand_data {
> + unsigned int upthreshold;
> + unsigned int downdifferential;
> +};
> +#endif
> +
> #else /* !CONFIG_PM_DEVFREQ */
> static int devfreq_add_device(struct device *dev,
> struct devfreq_dev_profile *profile,
> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
> {
> return 0;
> }
> +
> +#define devfreq_powersave NULL
> +#define devfreq_performance NULL
> +#define devfreq_userspace NULL
> +#define devfreq_simple_ondemand NULL
> +
> #endif /* CONFIG_PM_DEVFREQ */
>
> #endif /* __LINUX_DEVFREQ_H__ */
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-29 18:49 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-4-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Device specific sysfs interface /sys/devices/.../power/devfreq_*
> - governor R: name of governor
> - cur_freq R: current frequency
> - max_freq R: maximum operable frequency
> - min_freq R: minimum operable frequency
> - polling_interval R: polling interval in ms given with devfreq profile
> W: update polling interval.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Looks good to me.
Reviewed-by: Mike Turquette <mturquette@ti.com>
Regards,
Mike
>
> --
> Changed from v7
> - removed set_freq from the common devfreq interface
> - added get_devfreq, a mutex-protected wrapper for find_device_devfreq
> (for sysfs interfaces and later with governor-support)
> - corrected ABI documentation.
>
> Changed from v6
> - poling_interval is writable.
>
> Changed from v5
> - updated devferq_update usage.
>
> Changed from v4
> - removed system-wide sysfs interface
> - removed tickling sysfs interface
> - added set_freq for userspace governor (and any other governors that
> require user input)
>
> Changed from v3
> - corrected sysfs API usage
> - corrected error messages
> - moved sysfs entry location
> - added sysfs entries
>
> Changed from v2
> - add ABI entries for devfreq sysfs interface
> ---
> Documentation/ABI/testing/sysfs-devices-power | 37 ++++++
> drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++
> 2 files changed, 187 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 8ffbc25..57f4591 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -165,3 +165,40 @@ Description:
>
> Not all drivers support this attribute. If it isn't supported,
> attempts to read or write it will yield I/O errors.
> +
> +What: /sys/devices/.../power/devfreq_governor
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_governor shows the name
> + of the governor used by the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_cur_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_cur_freq shows the current
> + frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_max_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_max_freq shows the
> + maximum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_min_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_min_freq shows the
> + minimum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_polling_interval
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_polling_interval sets and
> + shows the requested polling interval of the corresponding
> + device. The values are represented in ms. If the value is less
> + than 1 jiffy, it is considered to be 0, which means no polling.
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index df63bdc..5bbece6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work;
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static struct attribute_group dev_attr_group;
> +
> /**
> * find_device_devfreq() - find devfreq struct using device pointer
> * @dev: device pointer used to lookup device devfreq.
> @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> }
>
> /**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection.
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +static struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
> +
> + return ret;
> +}
> +
> +/**
> * devfreq_do() - Check the usage profile of a given device and configure
> * frequency and voltage accordingly
> * @devfreq: devfreq info of the given device
> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> error, devfreq->governor->name);
>
> + sysfs_unmerge_group(&devfreq->dev->kobj,
> + &dev_attr_group);
> list_del(&devfreq->node);
> kfree(devfreq);
>
> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
> queue_delayed_work(devfreq_wq, &devfreq_work,
> devfreq->next_polling);
> }
> +
> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
> out:
> mutex_unlock(&devfreq_list_lock);
>
> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
> goto out;
> }
>
> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> @@ -279,6 +303,132 @@ out:
> return 0;
> }
>
> +static ssize_t show_governor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->governor)
> + return -EINVAL;
> +
> + return sprintf(buf, "%s\n", df->governor->name);
> +}
> +
> +static ssize_t show_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> +
> + return sprintf(buf, "%lu\n", df->previous_freq);
> +}
> +
> +static ssize_t show_max_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = ULONG_MAX;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_floor(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_min_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = 0;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_ceil(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_polling_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + return sprintf(buf, "%d\n", df->profile->polling_ms);
> +}
> +
> +static ssize_t store_polling_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned int value;
> + int ret;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + ret = sscanf(buf, "%u", &value);
> + if (ret != 1)
> + return -EINVAL;
> +
> + df->profile->polling_ms = value;
> + df->next_polling = df->polling_jiffies
> + = msecs_to_jiffies(value);
> +
> + mutex_lock(&devfreq_list_lock);
> + if (df->next_polling > 0 && !polling) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work,
> + df->next_polling);
> + }
> + mutex_unlock(&devfreq_list_lock);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
> + store_polling_interval);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_governor.attr,
> + &dev_attr_devfreq_cur_freq.attr,
> + &dev_attr_devfreq_max_freq.attr,
> + &dev_attr_devfreq_min_freq.attr,
> + &dev_attr_devfreq_polling_interval.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> /**
> * devfreq_init() - Initialize data structure for devfreq framework and
> * start polling registered devfreq devices.
> --
> 1.7.4.1
>
>
^ 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