* 3.1-rc1-git9: Reported regressions 2.6.39 -> 3.0
From: Rafael J. Wysocki @ 2011-08-14 19:02 UTC (permalink / raw)
To: Linux Kernel Mailing List
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, DRI, Florian Mickler, Andrew Morton,
Kernel Testers List, Linus Torvalds, Linux PM List,
Maciej Rutecki
[NOTE:
We already have a bug entry for tracking regressions from 3.0:
http://bugzilla.kernel.org/show_bug.cgi?id=40982
but there are no reports linked to it, mostly because Maciej is on vacation,
but also because the frequency of reporting regressions has been low
recently. So, if you're seeing a regression from 3.0, please let us know.]
This message contains a list of some post-2.6.39 regressions introduced before
3.0, for which there are no fixes in the mainline known to the tracking team.
If any of them have been fixed already, please let us know.
If you know of any other unresolved post-2.6.39 regressions, please let us know
either and we'll add them to the list. Also, please let us know if any
of the entries below are invalid.
Each entry from the list will be sent additionally in an automatic reply to
this message with CCs to the people involved in reporting and handling the
issue.
Listed regressions statistics:
Date Total Pending Unresolved
----------------------------------------
2011-08-14 63 22 19
Unresolved regressions
----------------------
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=40282
Subject : IP forwarding regression since 3.0-rc6
Submitter : Stephan Seitz <stse+lkml@fsing.rootsland.net>
Date : 2011-07-25 12:51 (21 days old)
Message-ID : <20110725T141145.GA.2ae38.stse@fsing.rootsland.net>
References : http://marc.info/?l=linux-kernel&m=131159880004908&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=40172
Subject : lockdep trace from post 3.0.
Submitter : Dave Jones <davej@redhat.com>
Date : 2011-07-24 1:32 (22 days old)
Message-ID : <20110724013257.GA6777@redhat.com>
References : http://marc.info/?l=linux-kernel&m=131147120206819&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=40092
Subject : RCU stall in linux-3.0.0
Submitter : Philip Armstrong <phil@kantaka.co.uk>
Date : 2011-07-25 21:44 (21 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=40072
Subject : suspend/resume problems w/ kernel 3.0 and a *docked* ThinkPad T400, unless iwlagn unloaded
Submitter : Toralf Förster <toralf.foerster@gmx.de>
Date : 2011-07-23 19:27 (23 days old)
Message-ID : <201107232127.34255.toralf.foerster@gmx.de>
References : http://marc.info/?l=linux-kernel&m=131144928023465&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39972
Subject : [regression] Radeon multi-screen
Submitter : Robert Piasek <dagger@gentoo.org>
Date : 2011-07-25 14:04 (21 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39922
Subject : WIFI becomes EXTREMELY slow ( ath9k ) making the connection unusable
Submitter : Lukasz Olszewski <lcpak@epoczta.pl>
Date : 2011-07-24 09:20 (22 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39912
Subject : WARNING: at drivers/tty/tty_ldisc.c:766 tty_ldisc_reinit+0x7d/0x90()
Submitter : Vegard Nossum <vegard.nossum@gmail.com>
Date : 2011-07-19 11:33 (27 days old)
Message-ID : <CAOMGZ=HS2EPkYD=5HfkSPpwPqHB5V3q0vaFmoZ8Hh+pMM9phrQ@mail.gmail.com>
References : http://marc.info/?l=linux-kernel&m=131107522914558&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39902
Subject : ath: Unable to reset channel (2412 MHz), reset status -5
Submitter : Justin P. Mattock <justinmattock@gmail.com>
Date : 2011-07-19 6:13 (27 days old)
Message-ID : <4E252076.1050309@gmail.com>
References : http://marc.info/?l=linux-kernel&m=131105603428323&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39832
Subject : HDA ATI HDMI: no sound with 3.0 - 2.6.39.3 works
Submitter : Andreas Steinmetz <ast@domdv.de>
Date : 2011-07-23 01:36 (23 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39732
Subject : JBD: Spotted dirty metadata buffer (dev = dm-2, blocknr = 2512725). There's a risk of filesystem corruption in case of system crash.
Submitter : Witold Baryluk <baryluk@smp.if.uj.edu.pl>
Date : 2011-07-22 04:22 (24 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39412
Subject : Win Vista and Win2K8 guests' network breaks down
Submitter : Jay Ren <yongjie.ren@intel.com>
Date : 2011-07-15 15:31 (31 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39342
Subject : [3.0.0-rc6-git7] possible recursive locking at cache_alloc_refill
Submitter : Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date : 2011-07-11 12:37 (35 days old)
Message-ID : <201107112137.FAD00545.SHtLOFOJOMFQFV@I-love.SAKURA.ne.jp>
References : http://marc.info/?l=linux-kernel&m=131038788902151&w=2
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39192
Subject : firmware_install fails with parallel make
Submitter : Ambroz Bizjak <ambrop7@gmail.com>
Date : 2011-07-12 09:35 (34 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39132
Subject : Starting with 3.0.0-rc6, masquerading seems to be broken.
Submitter : David Hill <hilld@binarystorm.net>
Date : 2011-07-10 19:45 (36 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=38922
Subject : Lenovo T420 (Sandy Bridge) Crashes on SD Card gpt partition writing and io errors on insert
Submitter : Peter Vollmer <vollmerpeter@googlemail.com>
Date : 2011-07-07 17:33 (39 days old)
First-Bad-Commit: http://git.kernel.org/linus/a3c7778f8153b9e4eceea6738973280b9e63c618
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=38622
Subject : [radeon cayman regresion] artefacts on screen
Submitter : Marek Hobler, 'neutrinus' <kernellist@neutrinus.com>
Date : 2011-07-01 09:35 (45 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=38612
Subject : Boot time warnings in APIC
Submitter : Bill Huey <bill.huey@gmail.com>
Date : 2011-07-01 06:44 (45 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=38582
Subject : T510 43495KG won't resume with 32bit installation
Submitter : Marc Koschewski <marc@osknowledge.org>
Date : 2011-06-30 22:39 (46 days old)
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=37432
Subject : sdhci-pci fails on 3.0.0-rc1 on Dell E6510
Submitter : Oliver Hartkopp <socketcan@hartkopp.net>
Date : 2011-06-07 20:06 (69 days old)
First-Bad-Commit: http://git.kernel.org/linus/da7822e5ad71ec9b745b412639f1e5e0ba795a20
References : http://article.gmane.org/gmane.linux.kernel.mmc/8442
https://lkml.org/lkml/2011/6/23/660
Regressions with patches
------------------------
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=40062
Subject : USB related "unable to handle kernel paging request" in 3.0.0-rc7
Submitter : Tino Keitel <tino.keitel@tikei.de>
Date : 2011-07-22 19:27 (24 days old)
Message-ID : <20110722192722.GA9369@x61.home>
References : http://marc.info/?l=linux-kernel&m=131136334621623&w=2
Patch : https://lkml.org/lkml/2011/8/2/174
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=39882
Subject : Kernel oops when turning bluetooth mouse on
Submitter : Lamarque V. Souza <lamarque@gmail.com>
Date : 2011-07-24 02:09 (22 days old)
Patch : https://bugzilla.kernel.org/attachment.cgi?id=66632
Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=37872
Subject : CPU lockup during boot
Submitter : Bruno Wolff III <bruno@wolff.to>
Date : 2011-06-19 15:00 (57 days old)
Patch : https://bugzilla.kernel.org/show_bug.cgi?id=37872#c16
For details, please visit the bug entries and follow the links given in
references.
As you can see, there is a Bugzilla entry for each of the listed regressions.
There also is a Bugzilla entry used for tracking the regressions introduced
between 2.6.39 and 3.0, unresolved as well as resolved, at:
http://bugzilla.kernel.org/show_bug.cgi?id=36912
Please let the tracking team know if there are any Bugzilla entries that
should be added to the list in there.
Thanks!
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* [Update x2][PATCH 9/9] ARM / shmobile: Make A3RV be a subdomain of A4LC on SH7372
From: Rafael J. Wysocki @ 2011-08-14 14:07 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, linux-sh
In-Reply-To: <201108132143.47649.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ARM / shmobile: Make A3RV be a subdomain of A4LC on SH7372
Instead of coding the undocumented dependencies between power domains
A3RV and A4LC on SH7372 directly into the low-level power up/down
routines, make A3RV be a subdomain of A4LC, which will cause the
same dependecies to hold.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
It also needs to build for CONFIG_PM unset.
Thanks,
Rafael
---
arch/arm/mach-shmobile/include/mach/sh7372.h | 3 +
arch/arm/mach-shmobile/pm-sh7372.c | 48 ++++-----------------------
arch/arm/mach-shmobile/setup-sh7372.c | 3 +
3 files changed, 14 insertions(+), 40 deletions(-)
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
@@ -91,35 +91,6 @@ static int pd_power_up(struct generic_pm
return ret;
}
-static int pd_power_up_a3rv(struct generic_pm_domain *genpd)
-{
- int ret = pd_power_up(genpd);
-
- /* force A4LC on after A3RV has been requested on */
- pm_genpd_poweron(&sh7372_a4lc.genpd);
-
- return ret;
-}
-
-static int pd_power_down_a3rv(struct generic_pm_domain *genpd)
-{
- int ret = pd_power_down(genpd);
-
- /* try to power down A4LC after A3RV is requested off */
- genpd_queue_power_off_work(&sh7372_a4lc.genpd);
-
- return ret;
-}
-
-static int pd_power_down_a4lc(struct generic_pm_domain *genpd)
-{
- /* only power down A4LC if A3RV is off */
- if (!(__raw_readl(PSTR) & (1 << sh7372_a3rv.bit_shift)))
- return pd_power_down(genpd);
-
- return -EBUSY;
-}
-
static bool pd_active_wakeup(struct device *dev)
{
return true;
@@ -133,17 +104,8 @@ void sh7372_init_pm_domain(struct sh7372
genpd->stop_device = pm_clk_suspend;
genpd->start_device = pm_clk_resume;
genpd->active_wakeup = pd_active_wakeup;
-
- if (sh7372_pd == &sh7372_a4lc) {
- genpd->power_off = pd_power_down_a4lc;
- genpd->power_on = pd_power_up;
- } else if (sh7372_pd == &sh7372_a3rv) {
- genpd->power_off = pd_power_down_a3rv;
- genpd->power_on = pd_power_up_a3rv;
- } else {
- genpd->power_off = pd_power_down;
- genpd->power_on = pd_power_up;
- }
+ genpd->power_off = pd_power_down;
+ genpd->power_on = pd_power_up;
genpd->power_on(&sh7372_pd->genpd);
}
@@ -159,6 +121,12 @@ void sh7372_add_device_to_domain(struct
pm_genpd_add_device(&sh7372_pd->genpd, dev);
}
+void sh7372_pm_add_subdomain(struct sh7372_pm_domain *sh7372_pd,
+ struct sh7372_pm_domain *sh7372_sd)
+{
+ pm_genpd_add_subdomain(&sh7372_pd->genpd, &sh7372_sd->genpd);
+}
+
struct sh7372_pm_domain sh7372_a4lc = {
.bit_shift = 1,
};
Index: linux/arch/arm/mach-shmobile/setup-sh7372.c
===================================================================
--- linux.orig/arch/arm/mach-shmobile/setup-sh7372.c
+++ linux/arch/arm/mach-shmobile/setup-sh7372.c
@@ -30,6 +30,7 @@
#include <linux/sh_dma.h>
#include <linux/sh_intc.h>
#include <linux/sh_timer.h>
+#include <linux/pm_domain.h>
#include <mach/hardware.h>
#include <mach/sh7372.h>
#include <asm/mach-types.h>
@@ -848,6 +849,8 @@ void __init sh7372_add_standard_devices(
sh7372_init_pm_domain(&sh7372_a3ri);
sh7372_init_pm_domain(&sh7372_a3sg);
+ sh7372_pm_add_subdomain(&sh7372_a4lc, &sh7372_a3rv);
+
platform_add_devices(sh7372_early_devices,
ARRAY_SIZE(sh7372_early_devices));
Index: linux/arch/arm/mach-shmobile/include/mach/sh7372.h
===================================================================
--- linux.orig/arch/arm/mach-shmobile/include/mach/sh7372.h
+++ linux/arch/arm/mach-shmobile/include/mach/sh7372.h
@@ -494,9 +494,12 @@ extern struct sh7372_pm_domain sh7372_a3
extern void sh7372_init_pm_domain(struct sh7372_pm_domain *sh7372_pd);
extern void sh7372_add_device_to_domain(struct sh7372_pm_domain *sh7372_pd,
struct platform_device *pdev);
+extern void sh7372_pm_add_subdomain(struct sh7372_pm_domain *sh7372_pd,
+ struct sh7372_pm_domain *sh7372_sd);
#else
#define sh7372_init_pm_domain(pd) do { } while(0)
#define sh7372_add_device_to_domain(pd, pdev) do { } while(0)
+#define sh7372_pm_add_subdomain(pd, sd) do { } while(0)
#endif /* CONFIG_PM */
#endif /* __ASM_SH7372_H__ */
^ permalink raw reply
* [PATCH] PM / Domains: Fix build for CONFIG_PM_RUNTIME unset
From: Rafael J. Wysocki @ 2011-08-14 14:04 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, linux-sh
From: Rafael J. Wysocki <rjw@sisk.pl>
Function genpd_queue_power_off_work() is not defined for
CONFIG_PM_RUNTIME, so pm_genpd_poweroff_unused() causes a build
error to happen in that case. Fix the problem by making
pm_genpd_poweroff_unused() depend on CONFIG_PM_RUNTIME too.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
This is 3.1 material if no one objects.
Thanks,
Rafael
---
drivers/base/power/domain.c | 30 +++++++++++++++---------------
include/linux/pm_domain.h | 10 +++++++---
kernel/power/Kconfig | 4 ++++
3 files changed, 26 insertions(+), 18 deletions(-)
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -72,8 +72,6 @@ extern int pm_genpd_remove_subdomain(str
extern void pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
extern int pm_genpd_poweron(struct generic_pm_domain *genpd);
-extern void pm_genpd_poweroff_unused(void);
-extern void genpd_queue_power_off_work(struct generic_pm_domain *genpd);
#else
static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
struct device *dev)
@@ -101,8 +99,14 @@ static inline int pm_genpd_poweron(struc
{
return -ENOSYS;
}
-static inline void pm_genpd_poweroff_unused(void) {}
+#endif
+
+#ifdef CONFIG_PM_GENERIC_DOMAINS_RUNTIME
+extern void genpd_queue_power_off_work(struct generic_pm_domain *genpd);
+extern void pm_genpd_poweroff_unused(void);
+#else
static inline void genpd_queue_power_off_work(struct generic_pm_domain *gpd) {}
+static inline void pm_genpd_poweroff_unused(void) {}
#endif
#endif /* _LINUX_PM_DOMAIN_H */
Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -460,6 +460,21 @@ static int pm_genpd_runtime_resume(struc
return 0;
}
+/**
+ * pm_genpd_poweroff_unused - Power off all PM domains with no devices in use.
+ */
+void pm_genpd_poweroff_unused(void)
+{
+ struct generic_pm_domain *genpd;
+
+ mutex_lock(&gpd_list_lock);
+
+ list_for_each_entry(genpd, &gpd_list, gpd_list_node)
+ genpd_queue_power_off_work(genpd);
+
+ mutex_unlock(&gpd_list_lock);
+}
+
#else
static inline void genpd_power_off_work_fn(struct work_struct *work) {}
@@ -1255,18 +1270,3 @@ void pm_genpd_init(struct generic_pm_dom
list_add(&genpd->gpd_list_node, &gpd_list);
mutex_unlock(&gpd_list_lock);
}
-
-/**
- * pm_genpd_poweroff_unused - Power off all PM domains with no devices in use.
- */
-void pm_genpd_poweroff_unused(void)
-{
- struct generic_pm_domain *genpd;
-
- mutex_lock(&gpd_list_lock);
-
- list_for_each_entry(genpd, &gpd_list, gpd_list_node)
- genpd_queue_power_off_work(genpd);
-
- mutex_unlock(&gpd_list_lock);
-}
Index: linux/kernel/power/Kconfig
===================================================================
--- linux.orig/kernel/power/Kconfig
+++ linux/kernel/power/Kconfig
@@ -231,3 +231,7 @@ config PM_CLK
config PM_GENERIC_DOMAINS
bool
depends on PM
+
+config PM_GENERIC_DOMAINS_RUNTIME
+ def_bool y
+ depends on PM_RUNTIME && PM_GENERIC_DOMAINS
^ permalink raw reply
* Re: [PATCH v4 00/15] PM QoS: add a per-device latency constraints class
From: Rafael J. Wysocki @ 2011-08-14 13:53 UTC (permalink / raw)
To: Jean Pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <CAORVsuU2sph=oKDk1btSmEY=tKPUucXuQdapru=Z6EXvuPqARA@mail.gmail.com>
On Sunday, August 14, 2011, Jean Pihet wrote:
> Rafael,
>
> On Fri, Aug 12, 2011 at 11:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Hi,
> >
> > On Friday, August 12, 2011, Jean Pihet wrote:
> >> Hi Rafael,
> >>
> >> 2011/8/12 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
> >> >> From: Jean Pihet <j-pihet@ti.com>
> >> >>
> >> >> This patch set is in an RFC state, for review and comments.
> >> >>
> >> ...
> >> >>
> >> >>
> >> >> Questions:
> >> >> 1. the user space API is still under discussions on linux-omap and linux-pm MLs,
> >> >> cf. [1]. The idea is to add a user-space API for the devices constratins
> >> >> PM QoS, using a sysfs entry per device
> >> >>
> >> >> [1] http://marc.info/?l=linux-omap&m=131232344503327&w=2
> >> >>
> >> >> ToDo:
> >> >> 1. write Documentation for the new PM QoS class, once the RFC is agreed on
> >> >> 2. validate the constraints framework on OMAP4 HW (done on OMAP3)
> >> >> 3. Need testing on platforms other than OMAP
> >> >> 4. refine the power domains wake-up latency and the cpuidle figures
> >> >> 5. re-visit the OMAP power domains states initialization procedure. Currently
> >> >> the power states that have been changed from the constraints API which were
> >> >> applied before the initialization of the power domains are lost
> >> >>
> >> >>
> >> >> Based on the master branch of the linux-omap git tree (3.0.0-rc7). Compile
> >> >> tested using OMAP and x86 generic defconfigs.
> >> >>
> >> >> Lightly tested on OMAP3 Beagleboard (ES2.x).
> >> >> Need testing on platforms other than OMAP, because of the impact on the
> >> >> device insertion/removal in device_pm_add/remove
> >> >
> >> > The patchset looks really good to me, I don't think I have any major
> >> > complaints about this version.
> >> Ok good to hear it! I tried to address all comments and concerns in
> >> this release.
> >>
> >> >
> >> > The only thing I'd like to ask at the moment is whether or not the
> >> > compilation of drivers/base/power/qos.c should depend on
> >> > CONFIG_PM_RUNTIME. Do you think it will be used by system suspend code on any
> >> > platforms?
> >> I would say it should only depend on CONFIG_PM because the dev PM QoS
> >> API can be used from any kernel code, being runtime PM code or not.
> >> I leave the decision to the PM framework experts.
> >>
> >> >
> >> > Also, I'd like to take the final patchset for 3.2,
> >> Ok good!
> >>
> >> > but I don't feel
> >> > confident enough about the OMAP patches.
> >> The OMAP patches have been reviewed a few times already and the
> >> comments have been taken into account. Also i has been tested
> >> correctly on OMAP3.
> >>
> >> > If you want me to take them too,
> >> > please make sure they are ACKed by the OMAP maintainers.
> >> For sure I need the Acks. I guess I now need to annoy OMAP folks about it ;p
> >> In the case the Acks are not gathered on time the generic patches
> >> could be merged in, then the OMAP generic code. Do you think it is a
> >> viable option?
> >
> > Yes, it is. I can take patches [1-7/15] alone.
> >
> >> The only concern I have is about the on-going OMAP PM initialization
> >> clean-up task, cf. ToDo list:
> >> >> 5. re-visit the OMAP power domains states initialization
> >> procedure. Currently
> >> >> the power states that have been changed from the constraints
> >> API which were
> >> >> applied before the initialization of the power domains are lost
> >>
> >> On the other hand some testing is needed on platforms other than OMAP,
> >> because of the impact on the device insertion/removal in
> >> device_pm_add/remove functions. I tested the SD card insertion/removal
> >> on OMAP3.
> >
> > OK, so are you going to make any more changes to patches [1-7/15]?
> I am now reworking [06/15] after your comments.
> Is that OK timewise?
It should be fine. I still need to have a closer look at [7/15] later today,
I'll let you know if there's anything I'd like to change in there.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 06/15] PM QoS: implement the per-device PM QoS constraints
From: Rafael J. Wysocki @ 2011-08-14 13:51 UTC (permalink / raw)
To: Jean Pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <CAORVsuVZjrqY0qhEvCpRRvt5RDcG-YT5SJo6Ejhyr-_fTyk9HQ@mail.gmail.com>
Hi,
On Sunday, August 14, 2011, Jean Pihet wrote:
...
> >> +
> >> + if (dev_pm_qos_request_active(req)) {
> >> + WARN(1, KERN_ERR "dev_pm_qos_add_request() called for already "
> >> + "added request\n");
> >> + return;
> >> + }
> >> + req->dev = dev;
> >> +
> >> + /* Allocate the constraints struct on the first call to add_request */
> >> + if (req->dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
> >> + dev_pm_qos_constraints_allocate(dev);
> >
> > Why not to do
> >
> > + if (!req->dev->power.constraints)
> > + dev_pm_qos_constraints_allocate(dev);
>
> Cf. my comments at the end of this message for the data structs
> alloc/free and the locking.
>
> >
> >> +
> >> + /* Silently return if the device has been removed */
> >> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> >> + return;
> >> +
> >
> > Hmm. What will happen if two callers run dev_pm_qos_add_request()
> > concurrently for the same device?
> update_target is using the power.lock to protect the constraints lists changes.
I was referring to the scenario in particular:
Suppose that there are no constraits for dev initially.
A -> calls dev_pm_qos_add_request(dev)
B -> calls dev_pm_qos_add_request(dev)
A -> sees power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT
and calls dev_pm_qos_constraints_allocate(dev)
B -> sees power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT
and calls dev_pm_qos_constraints_allocate(dev)
As a result, the structure allocated by A is leaked.
...
>
> Your remarks are inline with the concerns I had about the adata
> structs alloc/free and the locking.
>
> 1) data structs alloc/free
> As described in the changelog:
> >> 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.
> A basic state machine is needed in order to allocate the data at the
> first call to add_request and free it when the device is removed.
> Another option is to allocate the data when the device is added to the
> system and free it when the device is removed. That would make the
> code simpler but it is using a lot of memory for unneeded data.
That's fine. You simply need to be more careful about the possible
race conditions when the constraints objects are created and destroyed.
> 2) Race conditions
> I will add a lock to isolate the API callers from
> dev_pm_qos_constraints_destroy().
> The power.lock is already used by update_target so another lock is
> needed to protect the data allocation/free.
OK
> I will rework this tomorrow and re-submit asap when it is ready.
> Is that OK?
Yes, it is.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code
From: Rafael J. Wysocki @ 2011-08-14 13:37 UTC (permalink / raw)
To: Jean Pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <CAORVsuUz9qXFvUTsYOs4Z7rLQFXROKOcWVi=bZWTKJuA2Xu1Og@mail.gmail.com>
On Sunday, August 14, 2011, Jean Pihet wrote:
> Hi Rafael, Mark,
>
> On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, August 13, 2011, mark gross wrote:
> >> On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:
> >> > From: Jean Pihet <j-pihet@ti.com>
> >> >
> >> > In preparation for the per-device constratins support:
> >> > - rename update_target to pm_qos_update_target
> >> > - generalize and export pm_qos_update_target for usage by the upcoming
> >> > per-device latency constraints framework:
> >> > . operate on struct pm_qos_constraints for constraints management,
> >> > . introduce an 'action' parameter for constraints add/update/remove,
> >> > . the return value indicates if the aggregated constraint value has
> >> > changed,
> >> > - update the internal code to operate on struct pm_qos_constraints
> >> > - add a NULL pointer check in the API functions
> >> >
> >> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ...
> >> > +/* Action requested to pm_qos_update_target */
> >> > +enum pm_qos_req_action {
> >> > + PM_QOS_ADD_REQ, /* Add a new request */
> >> > + PM_QOS_UPDATE_REQ, /* Update an existing request */
> >> > + PM_QOS_REMOVE_REQ /* Remove an existing request */
> >> > +};
> >> > +
> >>
> >> What do you need this enum for? The function names *_update_*, *_add_*,
> >> and *_remove_* seem to be pretty redundant if you have to pass an enum
> >> that could possibly conflict with the function name.
> >>
> >> > #ifdef CONFIG_PM
> >> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> >> > + enum pm_qos_req_action action, int value);
> >> The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or
> >> there is something strange going on.... BTW what shold this function do
> >> if the pm_qos_req_action was *not* the UPDATE one?
>
> The meaning of pm_qos_update_target is 'update the PM QoS target
> constraints lists'. As described in the changelog the intention of
> this patch is to implement the constraints lists management logic in
> update_target and simplify the API functions (add/update/remove). It
> is also exported for the upcoming (patch 06/15]) to use it as well.
The enums are fine by me and they allow us to simplify the code
quite a bit.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 04/15] PM QoS: re-organize data structs
From: Rafael J. Wysocki @ 2011-08-14 13:34 UTC (permalink / raw)
To: Jean Pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <CAORVsuWHCf3EuQfpCGGSxxUc-K5e1dM2KDf8z5avPpQ=+V1DFw@mail.gmail.com>
On Sunday, August 14, 2011, Jean Pihet wrote:
> Rafael, Mark,
>
> On Sat, Aug 13, 2011 at 10:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, August 13, 2011, mark gross wrote:
> >> On Thu, Aug 11, 2011 at 05:06:41PM +0200, jean.pihet@newoldbits.com wrote:
> >> > From: Jean Pihet <j-pihet@ti.com>
> >> >
> >> > In preparation for the per-device constratins support, re-organize
> >> > the data strctures:
> >> > - add a struct pm_qos_constraints which contains the constraints
> >> > related data
> >> > - update struct pm_qos_object contents to the PM QoS internal object
> >> > data. Add a pointer to struct pm_qos_constraints
> >> > - update the internal code to use the new data structs.
> >> >
> >> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
> >> > ---
> >> > include/linux/pm_qos.h | 19 ++++++++++
> >> > kernel/power/qos.c | 90 ++++++++++++++++++++++-------------------------
> >> > 2 files changed, 61 insertions(+), 48 deletions(-)
> >> >
> >> > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> >> > index 6b0968f..9772311 100644
> >> > --- a/include/linux/pm_qos.h
> >> > +++ b/include/linux/pm_qos.h
> >> > @@ -25,6 +25,25 @@ struct pm_qos_request {
> >> > int pm_qos_class;
> >> > };
> >> >
> >> > +enum pm_qos_type {
> >> > + PM_QOS_UNITIALIZED,
> >> what is this for?
> >
> > I seem to remember discussing that previously, but I can't recall what
> > it's for. Jean?
> >
>
> Sorry it is a left over from the previous version, it has been used to
> detect non initialized data structs.
> It is still used to detect an error in pm_qos_get_value and so by its
> callers pm_qos_update_target and pm_qos_power_read. I have to admit
> the usefulness is quite limited.
>
> Is the removal of PM_QOS_UNITIALIZED needed? I would say no.
I agree.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v4 00/15] PM QoS: add a per-device latency constraints class
From: Jean Pihet @ 2011-08-14 8:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <201108122356.02983.rjw@sisk.pl>
Rafael,
On Fri, Aug 12, 2011 at 11:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Hi,
>
> On Friday, August 12, 2011, Jean Pihet wrote:
>> Hi Rafael,
>>
>> 2011/8/12 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
>> >> From: Jean Pihet <j-pihet@ti.com>
>> >>
>> >> This patch set is in an RFC state, for review and comments.
>> >>
>> ...
>> >>
>> >>
>> >> Questions:
>> >> 1. the user space API is still under discussions on linux-omap and linux-pm MLs,
>> >> cf. [1]. The idea is to add a user-space API for the devices constratins
>> >> PM QoS, using a sysfs entry per device
>> >>
>> >> [1] http://marc.info/?l=linux-omap&m=131232344503327&w=2
>> >>
>> >> ToDo:
>> >> 1. write Documentation for the new PM QoS class, once the RFC is agreed on
>> >> 2. validate the constraints framework on OMAP4 HW (done on OMAP3)
>> >> 3. Need testing on platforms other than OMAP
>> >> 4. refine the power domains wake-up latency and the cpuidle figures
>> >> 5. re-visit the OMAP power domains states initialization procedure. Currently
>> >> the power states that have been changed from the constraints API which were
>> >> applied before the initialization of the power domains are lost
>> >>
>> >>
>> >> Based on the master branch of the linux-omap git tree (3.0.0-rc7). Compile
>> >> tested using OMAP and x86 generic defconfigs.
>> >>
>> >> Lightly tested on OMAP3 Beagleboard (ES2.x).
>> >> Need testing on platforms other than OMAP, because of the impact on the
>> >> device insertion/removal in device_pm_add/remove
>> >
>> > The patchset looks really good to me, I don't think I have any major
>> > complaints about this version.
>> Ok good to hear it! I tried to address all comments and concerns in
>> this release.
>>
>> >
>> > The only thing I'd like to ask at the moment is whether or not the
>> > compilation of drivers/base/power/qos.c should depend on
>> > CONFIG_PM_RUNTIME. Do you think it will be used by system suspend code on any
>> > platforms?
>> I would say it should only depend on CONFIG_PM because the dev PM QoS
>> API can be used from any kernel code, being runtime PM code or not.
>> I leave the decision to the PM framework experts.
>>
>> >
>> > Also, I'd like to take the final patchset for 3.2,
>> Ok good!
>>
>> > but I don't feel
>> > confident enough about the OMAP patches.
>> The OMAP patches have been reviewed a few times already and the
>> comments have been taken into account. Also i has been tested
>> correctly on OMAP3.
>>
>> > If you want me to take them too,
>> > please make sure they are ACKed by the OMAP maintainers.
>> For sure I need the Acks. I guess I now need to annoy OMAP folks about it ;p
>> In the case the Acks are not gathered on time the generic patches
>> could be merged in, then the OMAP generic code. Do you think it is a
>> viable option?
>
> Yes, it is. I can take patches [1-7/15] alone.
>
>> The only concern I have is about the on-going OMAP PM initialization
>> clean-up task, cf. ToDo list:
>> >> 5. re-visit the OMAP power domains states initialization
>> procedure. Currently
>> >> the power states that have been changed from the constraints
>> API which were
>> >> applied before the initialization of the power domains are lost
>>
>> On the other hand some testing is needed on platforms other than OMAP,
>> because of the impact on the device insertion/removal in
>> device_pm_add/remove functions. I tested the SD card insertion/removal
>> on OMAP3.
>
> OK, so are you going to make any more changes to patches [1-7/15]?
I am now reworking [06/15] after your comments.
Is that OK timewise?
>
> Rafael
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 06/15] PM QoS: implement the per-device PM QoS constraints
From: Jean Pihet @ 2011-08-14 8:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <201108132308.26265.rjw@sisk.pl>
Rafael,
2011/8/13 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> Well, it looks like I should have reviewed this one more carefully.
>
> On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Implement the per-device PM QoS constraints by creating a device
>> PM QoS API, which calls the PM QoS constraints management core code.
>>
>> The per-device latency constraints data strctures are stored
>> in the device dev_pm_info struct.
>>
>> The device PM code calls the init and destroy of the per-device constraints
>> data struct in order to support the dynamic insertion and removal of the
>> devices in the system.
>>
>> 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.
>>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> ---
...
>> +/*#define DEBUG*/
>
> Please remove this.
Ok
>
>> +
>> +#include <linux/pm_qos.h>
>> +#include <linux/sched.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/slab.h>
>> +#include <linux/time.h>
>> +#include <linux/fs.h>
>> +#include <linux/device.h>
>> +#include <linux/string.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>
> Are you sure all of the headers are necessary?
No. I can check that.
>
>> +
>> +
>> +static void dev_pm_qos_constraints_allocate(struct device *dev);
>> +
>> +int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
>> +{
>> + return req->dev != 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
>
> That should be a static inline in a header.
Ok
>
>> +
>> +/**
>> + * dev_pm_qos_add_request - inserts new qos request into the list
>> + * @req: pointer to a preallocated handle
>> + * @dev: target device for the constraint
>> + * @value: defines the qos request
>> + *
>> + * This function inserts a new entry in the device constraints list of
>> + * requested qos performance characteristics. It recomputes the aggregate
>> + * QoS expectations of parameters and initializes the dev_pm_qos_request
>> + * handle. Caller needs to save this handle for later use in updates and
>> + * removal.
>> + */
>> +void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
>> + s32 value)
>
> I'd use a different ordering of arguments, namely (dev, req, value).
Ok
>
>> +{
>> + if (!req) /*guard against callers passing in null */
>> + return;
>
> Why not to check for !dev too?
Ok that makes sense
>
>> +
>> + if (dev_pm_qos_request_active(req)) {
>> + WARN(1, KERN_ERR "dev_pm_qos_add_request() called for already "
>> + "added request\n");
>> + return;
>> + }
>> + req->dev = dev;
>> +
>> + /* Allocate the constraints struct on the first call to add_request */
>> + if (req->dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
>> + dev_pm_qos_constraints_allocate(dev);
>
> Why not to do
>
> + if (!req->dev->power.constraints)
> + dev_pm_qos_constraints_allocate(dev);
Cf. my comments at the end of this message for the data structs
alloc/free and the locking.
>
>> +
>> + /* Silently return if the device has been removed */
>> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> + return;
>> +
>
> Hmm. What will happen if two callers run dev_pm_qos_add_request()
> concurrently for the same device?
update_target is using the power.lock to protect the constraints lists changes.
>
>> + pm_qos_update_target(dev->power.constraints,
>> + &req->node, PM_QOS_ADD_REQ, value);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
>> +
>> +/**
>> + * dev_pm_qos_update_request - modifies an existing qos request
>> + * @req : handle to list element holding a dev_pm_qos request to use
>> + * @value: defines the qos request
>> + *
>> + * Updates an existing dev PM qos request along with updating the
>> + * target value.
>> + *
>> + * Attempts are made to make this code callable on hot code paths.
>> + */
>> +void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
>> + s32 new_value)
>> +{
>> + if (!req) /*guard against callers passing in null */
>> + return;
>> +
>> + if (!dev_pm_qos_request_active(req)) {
>> + WARN(1, KERN_ERR "dev_pm_qos_update_request() called for "
>> + "unknown object\n");
>> + return;
>> + }
>> +
>> + /* Silently return if the device has been removed */
>> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> + return;
>> +
>> + if (new_value != req->node.prio)
>> + pm_qos_update_target(
>> + req->dev->power.constraints,
>> + &req->node, PM_QOS_UPDATE_REQ, new_value);
>
> I'm not sure what prevents dev_pm_qos_constraints_destroy() from removing
> the list from under us while we're executing this?
Cf. my comments at the end of this message for the data structs
alloc/free and the locking.
>
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
>> +
>> +/**
>> + * dev_pm_qos_remove_request - modifies an existing qos request
>> + * @req: handle to request list element
>> + *
>> + * Will remove pm qos request from the list of constraints and
>> + * recompute the current target value. Call this on slow code paths.
>> + */
>> +void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
>> +{
>> + if (!req) /*guard against callers passing in null */
>> + return;
>> +
>> + if (!dev_pm_qos_request_active(req)) {
>> + WARN(1, KERN_ERR "dev_pm_qos_remove_request() called for "
>> + "unknown object\n");
>> + return;
>> + }
>> +
>> + /* Silently return if the device has been removed */
>> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> + return;
>> +
>> + pm_qos_update_target(req->dev->power.constraints,
>> + &req->node, PM_QOS_REMOVE_REQ,
>> + PM_QOS_DEFAULT_VALUE);
>
> Same here.
>
>> + memset(req, 0, sizeof(*req));
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
>> +
>> +/**
>> + * dev_pm_qos_add_notifier - sets notification entry for changes to target value
>> + * of per-device PM QoS constraints
>> + *
>> + * @dev: target device for the constraint
>> + * @notifier: notifier block managed by caller.
>> + *
>> + * Will register the notifier into a notification chain that gets called
>> + * upon changes to the target value for the device.
>> + */
>> +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
>> +{
>> + int retval = 0;
>> +
>> + /* Silently return if the device has been removed */
>> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> + return retval;
>> +
>> + retval = blocking_notifier_chain_register(
>> + dev->power.constraints->notifiers,
>> + notifier);
>
> That may be racing with dev_pm_qos_constraints_destroy() too.
>
>> +
>> + return retval;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
>> +
>> +/**
>> + * dev_pm_qos_remove_notifier - deletes notification for changes to target value
>> + * of per-device PM QoS constraints
>> + *
>> + * @dev: target device for the constraint
>> + * @notifier: notifier block to be removed.
>> + *
>> + * Will remove the notifier from the notification chain that gets called
>> + * upon changes to the target value.
>> + */
>> +int dev_pm_qos_remove_notifier(struct device *dev,
>> + struct notifier_block *notifier)
>> +{
>> + int retval = 0;
>> +
>> + /* Silently return if the device has been removed */
>> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> + return retval;
>> +
>> + retval = blocking_notifier_chain_unregister(
>> + dev->power.constraints->notifiers,
>> + notifier);
>
> Same here.
>
>> +
>> + return retval;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
>> +
>> +/* Called at the first call to add_request, for constraint data allocation */
>> +static void dev_pm_qos_constraints_allocate(struct device *dev)
>> +{
>> + struct pm_qos_constraints *c;
>> + struct blocking_notifier_head *n;
>> +
>> + c = kzalloc(sizeof(*c), GFP_KERNEL);
>> + if (!c)
>> + return;
>> +
>> + n = kzalloc(sizeof(*n), GFP_KERNEL);
>> + if (!n) {
>> + kfree(c);
>> + return;
>> + }
>> + BLOCKING_INIT_NOTIFIER_HEAD(n);
>> +
>> + dev->power.constraints = c;
>> + plist_head_init(&dev->power.constraints->list, &dev->power.lock);
>> + 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;
>> +}
>> +
>> +/* Called from the device PM subsystem at device insertion */
>> +void dev_pm_qos_constraints_init(struct device *dev)
>> +{
>> + dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
>
> Hmm. Is it insufficient to check if "constraints" is not NULL?
>
>> +}
>> +
>> +/* Called from the device PM subsystem at device removal */
>> +void dev_pm_qos_constraints_destroy(struct device *dev)
>> +{
>> + struct dev_pm_qos_request *req, *tmp;
>> + enum dev_pm_qos_state constraints_state = dev->power.constraints_state;
>> +
>> + dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
>
> I'm not sure what the purpose of this is. I'd simply check if "constraints"
> is not NULL and I'd flush the list if not.
>
> Moreover, that has to go under a lock so that it doesn't race with
> the other functions above. I think you can reuse power.lock for that.
>
Your remarks are inline with the concerns I had about the adata
structs alloc/free and the locking.
1) data structs alloc/free
As described in the changelog:
>> 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.
A basic state machine is needed in order to allocate the data at the
first call to add_request and free it when the device is removed.
Another option is to allocate the data when the device is added to the
system and free it when the device is removed. That would make the
code simpler but it is using a lot of memory for unneeded data.
2) Race conditions
I will add a lock to isolate the API callers from
dev_pm_qos_constraints_destroy().
The power.lock is already used by update_target so another lock is
needed to protect the data allocation/free.
I will rework this tomorrow and re-submit asap when it is ready.
Is that OK?
>
> Thanks,
> Rafael
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 04/15] PM QoS: re-organize data structs
From: Jean Pihet @ 2011-08-14 8:29 UTC (permalink / raw)
To: Rafael J. Wysocki, markgross
Cc: Mark Brown, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <201108132258.49283.rjw@sisk.pl>
Rafael, Mark,
On Sat, Aug 13, 2011 at 10:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, August 13, 2011, mark gross wrote:
>> On Thu, Aug 11, 2011 at 05:06:41PM +0200, jean.pihet@newoldbits.com wrote:
>> > From: Jean Pihet <j-pihet@ti.com>
>> >
>> > In preparation for the per-device constratins support, re-organize
>> > the data strctures:
>> > - add a struct pm_qos_constraints which contains the constraints
>> > related data
>> > - update struct pm_qos_object contents to the PM QoS internal object
>> > data. Add a pointer to struct pm_qos_constraints
>> > - update the internal code to use the new data structs.
>> >
>> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> > ---
>> > include/linux/pm_qos.h | 19 ++++++++++
>> > kernel/power/qos.c | 90 ++++++++++++++++++++++-------------------------
>> > 2 files changed, 61 insertions(+), 48 deletions(-)
>> >
>> > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
>> > index 6b0968f..9772311 100644
>> > --- a/include/linux/pm_qos.h
>> > +++ b/include/linux/pm_qos.h
>> > @@ -25,6 +25,25 @@ struct pm_qos_request {
>> > int pm_qos_class;
>> > };
>> >
>> > +enum pm_qos_type {
>> > + PM_QOS_UNITIALIZED,
>> what is this for?
>
> I seem to remember discussing that previously, but I can't recall what
> it's for. Jean?
>
Sorry it is a left over from the previous version, it has been used to
detect non initialized data structs.
It is still used to detect an error in pm_qos_get_value and so by its
callers pm_qos_update_target and pm_qos_power_read. I have to admit
the usefulness is quite limited.
Is the removal of PM_QOS_UNITIALIZED needed? I would say no.
Regards,
Jean
^ permalink raw reply
* Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code
From: Jean Pihet @ 2011-08-14 8:25 UTC (permalink / raw)
To: Rafael J. Wysocki, markgross
Cc: Mark Brown, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <201108132234.17377.rjw@sisk.pl>
Hi Rafael, Mark,
On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, August 13, 2011, mark gross wrote:
>> On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:
>> > From: Jean Pihet <j-pihet@ti.com>
>> >
>> > In preparation for the per-device constratins support:
>> > - rename update_target to pm_qos_update_target
>> > - generalize and export pm_qos_update_target for usage by the upcoming
>> > per-device latency constraints framework:
>> > . operate on struct pm_qos_constraints for constraints management,
>> > . introduce an 'action' parameter for constraints add/update/remove,
>> > . the return value indicates if the aggregated constraint value has
>> > changed,
>> > - update the internal code to operate on struct pm_qos_constraints
>> > - add a NULL pointer check in the API functions
>> >
>> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
...
>> > +/* Action requested to pm_qos_update_target */
>> > +enum pm_qos_req_action {
>> > + PM_QOS_ADD_REQ, /* Add a new request */
>> > + PM_QOS_UPDATE_REQ, /* Update an existing request */
>> > + PM_QOS_REMOVE_REQ /* Remove an existing request */
>> > +};
>> > +
>>
>> What do you need this enum for? The function names *_update_*, *_add_*,
>> and *_remove_* seem to be pretty redundant if you have to pass an enum
>> that could possibly conflict with the function name.
>>
>> > #ifdef CONFIG_PM
>> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
>> > + enum pm_qos_req_action action, int value);
>> The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or
>> there is something strange going on.... BTW what shold this function do
>> if the pm_qos_req_action was *not* the UPDATE one?
The meaning of pm_qos_update_target is 'update the PM QoS target
constraints lists'. As described in the changelog the intention of
this patch is to implement the constraints lists management logic in
update_target and simplify the API functions (add/update/remove). It
is also exported for the upcoming (patch 06/15]) to use it as well.
...
>> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
>> > + enum pm_qos_req_action action, int value)
>> > {
>> > unsigned long flags;
>> > - int prev_value, curr_value;
>> > + int prev_value, curr_value, new_value;
>> >
>> > spin_lock_irqsave(&pm_qos_lock, flags);
>> > - prev_value = pm_qos_get_value(o);
>> > - /* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
>> > - if (value != PM_QOS_DEFAULT_VALUE) {
>> > + prev_value = pm_qos_get_value(c);
>> > + if (value == PM_QOS_DEFAULT_VALUE)
>> > + new_value = c->default_value;
>> > + else
>> > + new_value = value;
>> > +
>> > + switch (action) {
>> > + case PM_QOS_REMOVE_REQ:
>> We have a remove request API already. This overloading of this
>> interface feels wrong to me.
>>
>> > + plist_del(node, &c->list);
>> > + break;
>> > + case PM_QOS_UPDATE_REQ:
>> > /*
>> > * to change the list, we atomically remove, reinit
>> > * with new value and add, then see if the extremal
>> > * changed
>> > */
>> > - plist_del(node, &o->constraints->list);
>> > - plist_node_init(node, value);
>> > - plist_add(node, &o->constraints->list);
>> > - } else if (del) {
>> > - plist_del(node, &o->constraints->list);
>> > - } else {
>> > - plist_add(node, &o->constraints->list);
>> > + plist_del(node, &c->list);
>> > + case PM_QOS_ADD_REQ:
>> Don't we have an API for adding a request? if you want to overload
>> update like this then either we lose the add API or this shouldn't be
>> here.
>>
...
>> > @@ -216,20 +242,16 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
>> > void pm_qos_add_request(struct pm_qos_request *req,
>> > int pm_qos_class, s32 value)
>> > {
>> > - struct pm_qos_object *o = pm_qos_array[pm_qos_class];
>> > - int new_value;
>> > + if (!req) /*guard against callers passing in null */
>> > + return;
>> >
>> > if (pm_qos_request_active(req)) {
>> > WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
>> > return;
>> > }
>> > - if (value == PM_QOS_DEFAULT_VALUE)
>> > - new_value = o->constraints->default_value;
>> > - else
>> > - new_value = value;
>> > - plist_node_init(&req->node, new_value);
>> > req->pm_qos_class = pm_qos_class;
>> > - update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE);
>> > + pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
>> > + &req->node, PM_QOS_ADD_REQ, value);
>>
>> Ok, using pm_qos_update_target to reduce the LOC is ok but I don't think
>> this function and the enum should be exported outside of pm_qos.c
>
> They are used by the next patches adding the per-device QoS.
Exactly
>
> Thanks,
> Rafael
>
Jean
^ permalink raw reply
* Re: where is /sys/class/power_supply/ADP1 ?
From: J. Bakshi @ 2011-08-14 4:00 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM mailing list, linux-kernel
In-Reply-To: <201107302328.55921.rjw@sisk.pl>
On Sat, 30 Jul 2011 23:28:55 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> On Saturday, July 30, 2011, J. Bakshi wrote:
> > Hello kernel gurus,
> >
> > This is my very first email to this list, so greetings to all of you.
> >
> > I have compiled the latest 3.0 kernel. But I have not found the
> >
> > /sys/class/power_supply/ADP1 and sys/class/power_supply/BAT0 any more
> >
> > are they still supported in 3.0 ?
>
> What's there in your /sys/class/power_supply/ with 3.0? I surely have
> those links on my test systems.
>
Problem solved with the new version of laptop-mode-tools 1.58
This version is designed to work with kernel 3.0
THNX
^ permalink raw reply
* Re: [PATCH] PM / Freezer: Freeze filesystems while freezing processes (v2)
From: Rafael J. Wysocki @ 2011-08-14 0:16 UTC (permalink / raw)
To: Dave Chinner
Cc: Christoph, Theodore Ts'o, LKML, xfs, Christoph Hellwig,
linux-fsdevel, Linux PM mailing list, linux-ext4
In-Reply-To: <20110807001446.GI3162@dastard>
On Sunday, August 07, 2011, Dave Chinner wrote:
> On Sat, Aug 06, 2011 at 11:17:18PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
...
> > + /*
> > + * Freeze in reverse order so filesystems depending on others are
> > + * frozen in the right order (eg. loopback on ext3).
> > + */
> > + list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> > + if (!sb->s_root || !sb->s_bdev ||
> > + (sb->s_frozen == SB_FREEZE_TRANS) ||
> > + (sb->s_flags & MS_RDONLY))
> > + continue;
> > +
> > + freeze_bdev(sb->s_bdev);
> > + sb->s_flags |= MS_FROZEN;
> > + }
>
> AFAIK, that won't work for btrfs - you have to call freeze_super()
> directly for btrfs because it has a special relationship with
> sb->s_bdev. And besides, all freeze_bdev does is get an active
> reference on the superblock and call freeze_super().
>
> Also, that's traversing the list of superblock with locking and
> dereferencing the superblock without properly checking that the
> superblock is not being torn down. You should probably use
> iterate_supers (or at least copy the code), with a function that
> drops the s_umount read lock befor calling freeze_super() and then
> picks it back up afterwards.
So, what about the patch below? It appears to work on my test boxes.
Thanks,
Rafael
---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Freezer: Freeze filesystems while freezing processes (v3)
Freeze all filesystems during the freezing of tasks by calling
freeze_super() for all superblocks and thaw them during the thawing
of tasks with the help of thaw_super().
This is needed by hibernation, because some filesystems (e.g. XFS)
deadlock with the preallocation of memory used by it if the memory
pressure caused by it is too heavy.
The additional benefit of this change is that, if something goes
wrong after filesystems have been frozen, they will stay in a
consistent state and journal replays won't be necessary (e.g. after
a failing suspend or resume). In particular, this should help to
solve a long-standing issue that in some cases during resume from
hibernation the boot loader causes the journal to be replied for the
filesystem containing the kernel image and initrd causing it to
become inconsistent with the information stored in the hibernation
image.
This change is based on earlier work by Nigel Cunningham.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
fs/super.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 3 ++
kernel/power/process.c | 9 +++++-
3 files changed, 81 insertions(+), 1 deletion(-)
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -211,6 +211,7 @@ struct inodes_stat_t {
#define MS_KERNMOUNT (1<<22) /* this is a kern_mount call */
#define MS_I_VERSION (1<<23) /* Update inode I_version field */
#define MS_STRICTATIME (1<<24) /* Always perform atime updates */
+#define MS_FROZEN (1<<25) /* Frozen filesystem */
#define MS_NOSEC (1<<28)
#define MS_BORN (1<<29)
#define MS_ACTIVE (1<<30)
@@ -2497,6 +2498,8 @@ extern void drop_super(struct super_bloc
extern void iterate_supers(void (*)(struct super_block *, void *), void *);
extern void iterate_supers_type(struct file_system_type *,
void (*)(struct super_block *, void *), void *);
+extern int freeze_supers(void);
+extern void thaw_supers(void);
extern int dcache_dir_open(struct inode *, struct file *);
extern int dcache_dir_close(struct inode *, struct file *);
Index: linux/kernel/power/process.c
===================================================================
--- linux.orig/kernel/power/process.c
+++ linux/kernel/power/process.c
@@ -12,10 +12,10 @@
#include <linux/oom.h>
#include <linux/suspend.h>
#include <linux/module.h>
-#include <linux/syscalls.h>
#include <linux/freezer.h>
#include <linux/delay.h>
#include <linux/workqueue.h>
+#include <linux/fs.h>
/*
* Timeout for stopping processes
@@ -147,6 +147,12 @@ int freeze_processes(void)
goto Exit;
printk("done.\n");
+ printk("Freezing filesystems ... ");
+ error = freeze_supers();
+ if (error)
+ goto Exit;
+ printk("done.\n");
+
printk("Freezing remaining freezable tasks ... ");
error = try_to_freeze_tasks(false);
if (error)
@@ -188,6 +194,7 @@ void thaw_processes(void)
printk("Restarting tasks ... ");
thaw_workqueues();
thaw_tasks(true);
+ thaw_supers();
thaw_tasks(false);
schedule();
printk("done.\n");
Index: linux/fs/super.c
===================================================================
--- linux.orig/fs/super.c
+++ linux/fs/super.c
@@ -590,6 +590,76 @@ void iterate_supers_type(struct file_sys
EXPORT_SYMBOL(iterate_supers_type);
/**
+ * freeze_supers - call freeze_super() for all superblocks
+ */
+int freeze_supers(void)
+{
+ struct super_block *sb, *p = NULL;
+ int error = 0;
+
+ spin_lock(&sb_lock);
+ /*
+ * Freeze in reverse order so filesystems depending on others are
+ * frozen in the right order (eg. loopback on ext3).
+ */
+ list_for_each_entry_reverse(sb, &super_blocks, s_list) {
+ if (list_empty(&sb->s_instances))
+ continue;
+ sb->s_count++;
+ spin_unlock(&sb_lock);
+
+ if (sb->s_root && sb->s_frozen != SB_FREEZE_TRANS
+ && !(sb->s_flags & MS_RDONLY)) {
+ error = freeze_super(sb);
+ if (!error)
+ sb->s_flags |= MS_FROZEN;
+ }
+
+ spin_lock(&sb_lock);
+ if (error)
+ break;
+ if (p)
+ __put_super(p);
+ p = sb;
+ }
+ if (p)
+ __put_super(p);
+ spin_unlock(&sb_lock);
+
+ return error;
+}
+
+/**
+ * thaw_supers - call thaw_super() for all superblocks
+ */
+void thaw_supers(void)
+{
+ struct super_block *sb, *p = NULL;
+
+ spin_lock(&sb_lock);
+ list_for_each_entry(sb, &super_blocks, s_list) {
+ if (list_empty(&sb->s_instances))
+ continue;
+ sb->s_count++;
+ spin_unlock(&sb_lock);
+
+ if (sb->s_flags & MS_FROZEN) {
+ thaw_super(sb);
+ sb->s_flags &= ~MS_FROZEN;
+ }
+
+ spin_lock(&sb_lock);
+ if (p)
+ __put_super(p);
+ p = sb;
+ }
+ if (p)
+ __put_super(p);
+ spin_unlock(&sb_lock);
+}
+
+
+/**
* get_super - get the superblock of a device
* @bdev: device to get the superblock for
*
^ permalink raw reply
* Re: [PATCH 0/4] ARM: EXYNOS4: Support generic Power domain framework for EXYNOS4210
From: Rafael J. Wysocki @ 2011-08-13 23:39 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-samsung-soc, Kyungmin Park, Kukjin Kim, linux-pm,
linux-arm-kernel
In-Reply-To: <20110813213601.GA25866@n2100.arm.linux.org.uk>
On Saturday, August 13, 2011, Russell King - ARM Linux wrote:
> On Sat, Aug 13, 2011 at 11:24:07PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, August 11, 2011, Chanwoo Choi wrote:
> > > The following patch set use the generic Power domain Framework instead of
> > > power domain code depend of Samsung SoC.
> > >
> > > Chanwoo Choi (4):
> > > ARM: EXYNOS4: Support for generic I/O power domains on EXYNOS4210
> > > ARM: EXYNOS4: Support for generic Clock manipulation PM callbacks
> > > ARM: EXYNOS4: Delete the power-domain code depend on Samsung SoC
> > > ARM: EXYNOS4: Add power domain to use generic Power domain Framework
> > >
> > > arch/arm/mach-exynos4/Kconfig | 10 +-
> > > arch/arm/mach-exynos4/Makefile | 4 +-
> > > arch/arm/mach-exynos4/dev-pd.c | 139 --------------
> > > arch/arm/mach-exynos4/include/mach/pm-exynos4210.h | 52 ++++++
> > > arch/arm/mach-exynos4/include/mach/regs-clock.h | 8 +
> > > arch/arm/mach-exynos4/mach-nuri.c | 21 ++-
> > > arch/arm/mach-exynos4/mach-smdkc210.c | 26 ++-
> > > arch/arm/mach-exynos4/mach-smdkv310.c | 23 ++-
> > > arch/arm/mach-exynos4/mach-universal_c210.c | 21 ++-
> > > arch/arm/mach-exynos4/pm-exynos4210.c | 189 ++++++++++++++++++++
> > > arch/arm/mach-exynos4/pm-runtime.c | 56 ++++++
> > > arch/arm/plat-samsung/Kconfig | 8 -
> > > arch/arm/plat-samsung/Makefile | 4 -
> > > arch/arm/plat-samsung/include/plat/pd.h | 30 ---
> > > arch/arm/plat-samsung/pd.c | 95 ----------
> > > 15 files changed, 377 insertions(+), 309 deletions(-)
> > > delete mode 100644 arch/arm/mach-exynos4/dev-pd.c
> > > create mode 100644 arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
> > > create mode 100644 arch/arm/mach-exynos4/pm-exynos4210.c
> > > create mode 100644 arch/arm/mach-exynos4/pm-runtime.c
> > > delete mode 100644 arch/arm/plat-samsung/include/plat/pd.h
> > > delete mode 100644 arch/arm/plat-samsung/pd.c
> >
> > The patchset looks good to me, but please note that some code it
> > is based on will most likely change in 3.2 due to this patchset:
> >
> > https://lkml.org/lkml/2011/8/8/420
>
> Err, isn't all that pm_clk stuff just duplicating what the clk API does?
I'm not sure it's duplicating anything. Maybe it does, but it came into
being by moving some code that were duplicated in a few places throughout
the ARM and sh trees into one place.
> IOW, drivers _can_ (and should be) calling clk_disable() when they don't
> need the clock running.
Drivers may not know about what to do in a given situation. For example,
if the system has power domains, it may be better to switch a power domain
off instead of or in addition to disabling the clock and the driver usually
doesn't know about that.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 0/4] ARM: EXYNOS4: Support generic Power domain framework for EXYNOS4210
From: Russell King - ARM Linux @ 2011-08-13 21:36 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-samsung-soc, Kyungmin Park, Kukjin Kim, linux-pm,
linux-arm-kernel
In-Reply-To: <201108132324.08137.rjw@sisk.pl>
On Sat, Aug 13, 2011 at 11:24:07PM +0200, Rafael J. Wysocki wrote:
> On Thursday, August 11, 2011, Chanwoo Choi wrote:
> > The following patch set use the generic Power domain Framework instead of
> > power domain code depend of Samsung SoC.
> >
> > Chanwoo Choi (4):
> > ARM: EXYNOS4: Support for generic I/O power domains on EXYNOS4210
> > ARM: EXYNOS4: Support for generic Clock manipulation PM callbacks
> > ARM: EXYNOS4: Delete the power-domain code depend on Samsung SoC
> > ARM: EXYNOS4: Add power domain to use generic Power domain Framework
> >
> > arch/arm/mach-exynos4/Kconfig | 10 +-
> > arch/arm/mach-exynos4/Makefile | 4 +-
> > arch/arm/mach-exynos4/dev-pd.c | 139 --------------
> > arch/arm/mach-exynos4/include/mach/pm-exynos4210.h | 52 ++++++
> > arch/arm/mach-exynos4/include/mach/regs-clock.h | 8 +
> > arch/arm/mach-exynos4/mach-nuri.c | 21 ++-
> > arch/arm/mach-exynos4/mach-smdkc210.c | 26 ++-
> > arch/arm/mach-exynos4/mach-smdkv310.c | 23 ++-
> > arch/arm/mach-exynos4/mach-universal_c210.c | 21 ++-
> > arch/arm/mach-exynos4/pm-exynos4210.c | 189 ++++++++++++++++++++
> > arch/arm/mach-exynos4/pm-runtime.c | 56 ++++++
> > arch/arm/plat-samsung/Kconfig | 8 -
> > arch/arm/plat-samsung/Makefile | 4 -
> > arch/arm/plat-samsung/include/plat/pd.h | 30 ---
> > arch/arm/plat-samsung/pd.c | 95 ----------
> > 15 files changed, 377 insertions(+), 309 deletions(-)
> > delete mode 100644 arch/arm/mach-exynos4/dev-pd.c
> > create mode 100644 arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
> > create mode 100644 arch/arm/mach-exynos4/pm-exynos4210.c
> > create mode 100644 arch/arm/mach-exynos4/pm-runtime.c
> > delete mode 100644 arch/arm/plat-samsung/include/plat/pd.h
> > delete mode 100644 arch/arm/plat-samsung/pd.c
>
> The patchset looks good to me, but please note that some code it
> is based on will most likely change in 3.2 due to this patchset:
>
> https://lkml.org/lkml/2011/8/8/420
Err, isn't all that pm_clk stuff just duplicating what the clk API does?
IOW, drivers _can_ (and should be) calling clk_disable() when they don't
need the clock running.
^ permalink raw reply
* Re: [PATCH 0/4] ARM: EXYNOS4: Support generic Power domain framework for EXYNOS4210
From: Rafael J. Wysocki @ 2011-08-13 21:24 UTC (permalink / raw)
To: Chanwoo Choi
Cc: linux-samsung-soc, Russell King - ARM Linux, Kyungmin Park,
Kukjin Kim, linux-pm, linux-arm-kernel
In-Reply-To: <4E4360B2.2060508@samsung.com>
On Thursday, August 11, 2011, Chanwoo Choi wrote:
> The following patch set use the generic Power domain Framework instead of
> power domain code depend of Samsung SoC.
>
> Chanwoo Choi (4):
> ARM: EXYNOS4: Support for generic I/O power domains on EXYNOS4210
> ARM: EXYNOS4: Support for generic Clock manipulation PM callbacks
> ARM: EXYNOS4: Delete the power-domain code depend on Samsung SoC
> ARM: EXYNOS4: Add power domain to use generic Power domain Framework
>
> arch/arm/mach-exynos4/Kconfig | 10 +-
> arch/arm/mach-exynos4/Makefile | 4 +-
> arch/arm/mach-exynos4/dev-pd.c | 139 --------------
> arch/arm/mach-exynos4/include/mach/pm-exynos4210.h | 52 ++++++
> arch/arm/mach-exynos4/include/mach/regs-clock.h | 8 +
> arch/arm/mach-exynos4/mach-nuri.c | 21 ++-
> arch/arm/mach-exynos4/mach-smdkc210.c | 26 ++-
> arch/arm/mach-exynos4/mach-smdkv310.c | 23 ++-
> arch/arm/mach-exynos4/mach-universal_c210.c | 21 ++-
> arch/arm/mach-exynos4/pm-exynos4210.c | 189 ++++++++++++++++++++
> arch/arm/mach-exynos4/pm-runtime.c | 56 ++++++
> arch/arm/plat-samsung/Kconfig | 8 -
> arch/arm/plat-samsung/Makefile | 4 -
> arch/arm/plat-samsung/include/plat/pd.h | 30 ---
> arch/arm/plat-samsung/pd.c | 95 ----------
> 15 files changed, 377 insertions(+), 309 deletions(-)
> delete mode 100644 arch/arm/mach-exynos4/dev-pd.c
> create mode 100644 arch/arm/mach-exynos4/include/mach/pm-exynos4210.h
> create mode 100644 arch/arm/mach-exynos4/pm-exynos4210.c
> create mode 100644 arch/arm/mach-exynos4/pm-runtime.c
> delete mode 100644 arch/arm/plat-samsung/include/plat/pd.h
> delete mode 100644 arch/arm/plat-samsung/pd.c
The patchset looks good to me, but please note that some code it
is based on will most likely change in 3.2 due to this patchset:
https://lkml.org/lkml/2011/8/8/420
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 06/15] PM QoS: implement the per-device PM QoS constraints
From: Rafael J. Wysocki @ 2011-08-13 21:08 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-7-git-send-email-j-pihet@ti.com>
Hi,
Well, it looks like I should have reviewed this one more carefully.
On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Implement the per-device PM QoS constraints by creating a device
> PM QoS API, which calls the PM QoS constraints management core code.
>
> The per-device latency constraints data strctures are stored
> in the device dev_pm_info struct.
>
> The device PM code calls the init and destroy of the per-device constraints
> data struct in order to support the dynamic insertion and removal of the
> devices in the system.
>
> 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.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> drivers/base/power/Makefile | 4 +-
> drivers/base/power/main.c | 3 +
> drivers/base/power/qos.c | 262 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm.h | 9 ++
> include/linux/pm_qos.h | 39 +++++++
> 5 files changed, 315 insertions(+), 2 deletions(-)
> create mode 100644 drivers/base/power/qos.c
>
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index 3647e11..1a61f89 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,8 +1,8 @@
> -obj-$(CONFIG_PM) += sysfs.o generic_ops.o
> +obj-$(CONFIG_PM) += sysfs.o generic_ops.o qos.o
> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> obj-$(CONFIG_PM_RUNTIME) += runtime.o
> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> obj-$(CONFIG_PM_OPP) += opp.o
> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
>
> -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> \ No newline at end of file
> +ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 06f09bf..f5c0e0e 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -22,6 +22,7 @@
> #include <linux/mutex.h>
> #include <linux/pm.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pm_qos.h>
> #include <linux/resume-trace.h>
> #include <linux/interrupt.h>
> #include <linux/sched.h>
> @@ -97,6 +98,7 @@ void device_pm_add(struct device *dev)
> dev_name(dev->parent));
> list_add_tail(&dev->power.entry, &dpm_list);
> mutex_unlock(&dpm_list_mtx);
> + dev_pm_qos_constraints_init(dev);
> }
>
> /**
> @@ -107,6 +109,7 @@ 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);
> list_del_init(&dev->power.entry);
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> new file mode 100644
> index 0000000..465e419
> --- /dev/null
> +++ b/drivers/base/power/qos.c
> @@ -0,0 +1,262 @@
> +/*
> + * This module exposes the interface to kernel space for specifying
> + * per-device PM QoS dependencies. It provides infrastructure for registration
> + * of:
> + *
> + * Dependents on a QoS value : register requests
> + * Watchers of QoS value : get notified when target QoS value changes
> + *
> + * This QoS design is best effort based. Dependents register their QoS needs.
> + * Watchers register to keep track of the current QoS needs of the system.
> + *
> + * Note about the per-device constraint data struct allocation:
> + * . The per-device constraints data struct ptr is tored into the device
> + * dev_pm_info.
> + * . 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.
> + */
> +
> +/*#define DEBUG*/
Please remove this.
> +
> +#include <linux/pm_qos.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +#include <linux/fs.h>
> +#include <linux/device.h>
> +#include <linux/string.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
Are you sure all of the headers are necessary?
> +
> +
> +static void dev_pm_qos_constraints_allocate(struct device *dev);
> +
> +int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
> +{
> + return req->dev != 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
That should be a static inline in a header.
> +
> +/**
> + * dev_pm_qos_add_request - inserts new qos request into the list
> + * @req: pointer to a preallocated handle
> + * @dev: target device for the constraint
> + * @value: defines the qos request
> + *
> + * This function inserts a new entry in the device constraints list of
> + * requested qos performance characteristics. It recomputes the aggregate
> + * QoS expectations of parameters and initializes the dev_pm_qos_request
> + * handle. Caller needs to save this handle for later use in updates and
> + * removal.
> + */
> +void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
> + s32 value)
I'd use a different ordering of arguments, namely (dev, req, value).
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
Why not to check for !dev too?
> +
> + if (dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_add_request() called for already "
> + "added request\n");
> + return;
> + }
> + req->dev = dev;
> +
> + /* Allocate the constraints struct on the first call to add_request */
> + if (req->dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
> + dev_pm_qos_constraints_allocate(dev);
Why not to do
+ if (!req->dev->power.constraints)
+ dev_pm_qos_constraints_allocate(dev);
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
Hmm. What will happen if two callers run dev_pm_qos_add_request()
concurrently for the same device?
> + pm_qos_update_target(dev->power.constraints,
> + &req->node, PM_QOS_ADD_REQ, value);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
> +
> +/**
> + * dev_pm_qos_update_request - modifies an existing qos request
> + * @req : handle to list element holding a dev_pm_qos request to use
> + * @value: defines the qos request
> + *
> + * Updates an existing dev PM qos request along with updating the
> + * target value.
> + *
> + * Attempts are made to make this code callable on hot code paths.
> + */
> +void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
> + s32 new_value)
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
> +
> + if (!dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_update_request() called for "
> + "unknown object\n");
> + return;
> + }
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
> + if (new_value != req->node.prio)
> + pm_qos_update_target(
> + req->dev->power.constraints,
> + &req->node, PM_QOS_UPDATE_REQ, new_value);
I'm not sure what prevents dev_pm_qos_constraints_destroy() from removing
the list from under us while we're executing this?
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
> +
> +/**
> + * dev_pm_qos_remove_request - modifies an existing qos request
> + * @req: handle to request list element
> + *
> + * Will remove pm qos request from the list of constraints and
> + * recompute the current target value. Call this on slow code paths.
> + */
> +void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
> +
> + if (!dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_remove_request() called for "
> + "unknown object\n");
> + return;
> + }
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
> + pm_qos_update_target(req->dev->power.constraints,
> + &req->node, PM_QOS_REMOVE_REQ,
> + PM_QOS_DEFAULT_VALUE);
Same here.
> + memset(req, 0, sizeof(*req));
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
> +
> +/**
> + * dev_pm_qos_add_notifier - sets notification entry for changes to target value
> + * of per-device PM QoS constraints
> + *
> + * @dev: target device for the constraint
> + * @notifier: notifier block managed by caller.
> + *
> + * Will register the notifier into a notification chain that gets called
> + * upon changes to the target value for the device.
> + */
> +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
> +{
> + int retval = 0;
> +
> + /* Silently return if the device has been removed */
> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return retval;
> +
> + retval = blocking_notifier_chain_register(
> + dev->power.constraints->notifiers,
> + notifier);
That may be racing with dev_pm_qos_constraints_destroy() too.
> +
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
> +
> +/**
> + * dev_pm_qos_remove_notifier - deletes notification for changes to target value
> + * of per-device PM QoS constraints
> + *
> + * @dev: target device for the constraint
> + * @notifier: notifier block to be removed.
> + *
> + * Will remove the notifier from the notification chain that gets called
> + * upon changes to the target value.
> + */
> +int dev_pm_qos_remove_notifier(struct device *dev,
> + struct notifier_block *notifier)
> +{
> + int retval = 0;
> +
> + /* Silently return if the device has been removed */
> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return retval;
> +
> + retval = blocking_notifier_chain_unregister(
> + dev->power.constraints->notifiers,
> + notifier);
Same here.
> +
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
> +
> +/* Called at the first call to add_request, for constraint data allocation */
> +static void dev_pm_qos_constraints_allocate(struct device *dev)
> +{
> + struct pm_qos_constraints *c;
> + struct blocking_notifier_head *n;
> +
> + c = kzalloc(sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return;
> +
> + n = kzalloc(sizeof(*n), GFP_KERNEL);
> + if (!n) {
> + kfree(c);
> + return;
> + }
> + BLOCKING_INIT_NOTIFIER_HEAD(n);
> +
> + dev->power.constraints = c;
> + plist_head_init(&dev->power.constraints->list, &dev->power.lock);
> + 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;
> +}
> +
> +/* Called from the device PM subsystem at device insertion */
> +void dev_pm_qos_constraints_init(struct device *dev)
> +{
> + dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
Hmm. Is it insufficient to check if "constraints" is not NULL?
> +}
> +
> +/* Called from the device PM subsystem at device removal */
> +void dev_pm_qos_constraints_destroy(struct device *dev)
> +{
> + struct dev_pm_qos_request *req, *tmp;
> + enum dev_pm_qos_state constraints_state = dev->power.constraints_state;
> +
> + dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
I'm not sure what the purpose of this is. I'd simply check if "constraints"
is not NULL and I'd flush the list if not.
Moreover, that has to go under a lock so that it doesn't race with
the other functions above. I think you can reuse power.lock for that.
> + if (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 per-device
> + * callbacks if needed
> + */
> + pm_qos_update_target(req->dev->power.constraints,
> + &req->node, PM_QOS_REMOVE_REQ,
> + PM_QOS_DEFAULT_VALUE);
> +
> + kfree(dev->power.constraints->notifiers);
> + kfree(dev->power.constraints);
> + dev->power.constraints = NULL;
> + }
> +}
> +
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 04/15] PM QoS: re-organize data structs
From: Rafael J. Wysocki @ 2011-08-13 20:58 UTC (permalink / raw)
To: markgross, Jean Pihet; +Cc: Mark Brown, Linux PM mailing list, linux-omap
In-Reply-To: <20110813025629.GD639@gvim.org>
On Saturday, August 13, 2011, mark gross wrote:
> On Thu, Aug 11, 2011 at 05:06:41PM +0200, jean.pihet@newoldbits.com wrote:
> > From: Jean Pihet <j-pihet@ti.com>
> >
> > In preparation for the per-device constratins support, re-organize
> > the data strctures:
> > - add a struct pm_qos_constraints which contains the constraints
> > related data
> > - update struct pm_qos_object contents to the PM QoS internal object
> > data. Add a pointer to struct pm_qos_constraints
> > - update the internal code to use the new data structs.
> >
> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
> > ---
> > include/linux/pm_qos.h | 19 ++++++++++
> > kernel/power/qos.c | 90 ++++++++++++++++++++++-------------------------
> > 2 files changed, 61 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> > index 6b0968f..9772311 100644
> > --- a/include/linux/pm_qos.h
> > +++ b/include/linux/pm_qos.h
> > @@ -25,6 +25,25 @@ struct pm_qos_request {
> > int pm_qos_class;
> > };
> >
> > +enum pm_qos_type {
> > + PM_QOS_UNITIALIZED,
> what is this for?
I seem to remember discussing that previously, but I can't recall what
it's for. Jean?
^ permalink raw reply
* Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code
From: Rafael J. Wysocki @ 2011-08-13 20:34 UTC (permalink / raw)
To: markgross; +Cc: Mark Brown, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <20110813030937.GE639@gvim.org>
On Saturday, August 13, 2011, mark gross wrote:
> On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:
> > From: Jean Pihet <j-pihet@ti.com>
> >
> > In preparation for the per-device constratins support:
> > - rename update_target to pm_qos_update_target
> > - generalize and export pm_qos_update_target for usage by the upcoming
> > per-device latency constraints framework:
> > . operate on struct pm_qos_constraints for constraints management,
> > . introduce an 'action' parameter for constraints add/update/remove,
> > . the return value indicates if the aggregated constraint value has
> > changed,
> > - update the internal code to operate on struct pm_qos_constraints
> > - add a NULL pointer check in the API functions
> >
> > Signed-off-by: Jean Pihet <j-pihet@ti.com>
> >
> > ---
> > include/linux/pm_qos.h | 14 ++++++
> > kernel/power/qos.c | 123 ++++++++++++++++++++++++++----------------------
> > 2 files changed, 81 insertions(+), 56 deletions(-)
> >
> > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> > index 9772311..84aa150 100644
> > --- a/include/linux/pm_qos.h
> > +++ b/include/linux/pm_qos.h
> > @@ -44,7 +44,16 @@ struct pm_qos_constraints {
> > struct blocking_notifier_head *notifiers;
> > };
> >
> > +/* Action requested to pm_qos_update_target */
> > +enum pm_qos_req_action {
> > + PM_QOS_ADD_REQ, /* Add a new request */
> > + PM_QOS_UPDATE_REQ, /* Update an existing request */
> > + PM_QOS_REMOVE_REQ /* Remove an existing request */
> > +};
> > +
>
> What do you need this enum for? The function names *_update_*, *_add_*,
> and *_remove_* seem to be pretty redundant if you have to pass an enum
> that could possibly conflict with the function name.
>
> > #ifdef CONFIG_PM
> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> > + enum pm_qos_req_action action, int value);
> The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or
> there is something strange going on.... BTW what shold this function do
> if the pm_qos_req_action was *not* the UPDATE one?
>
>
> pm_qos_update_target should be a static to the C- file along with the
> enum pm_qos_req_action.
>
>
> > void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
> > s32 value);
> > void pm_qos_update_request(struct pm_qos_request *req,
> > @@ -56,6 +65,11 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
> > int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> > int pm_qos_request_active(struct pm_qos_request *req);
> > #else
> > +static inline int pm_qos_update_target(struct pm_qos_constraints *c,
> > + struct plist_node *node,
> > + enum pm_qos_req_action action,
> > + int value)
> > + { return 0; }
> > static inline void pm_qos_add_request(struct pm_qos_request *req,
> > int pm_qos_class, s32 value)
> > { return; }
> > diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> > index 66e8d6f..fc60f96 100644
> > --- a/kernel/power/qos.c
> > +++ b/kernel/power/qos.c
> > @@ -121,17 +121,17 @@ static const struct file_operations pm_qos_power_fops = {
> > };
> >
> > /* unlocked internal variant */
> > -static inline int pm_qos_get_value(struct pm_qos_object *o)
> > +static inline int pm_qos_get_value(struct pm_qos_constraints *c)
> > {
> > - if (plist_head_empty(&o->constraints->list))
> > - return o->constraints->default_value;
> > + if (plist_head_empty(&c->list))
> > + return c->default_value;
> >
> > - switch (o->constraints->type) {
> > + switch (c->type) {
> > case PM_QOS_MIN:
> > - return plist_first(&o->constraints->list)->prio;
> > + return plist_first(&c->list)->prio;
> >
> > case PM_QOS_MAX:
> > - return plist_last(&o->constraints->list)->prio;
> > + return plist_last(&c->list)->prio;
> >
> > default:
> > /* runtime check for not using enum */
> > @@ -139,47 +139,73 @@ static inline int pm_qos_get_value(struct pm_qos_object *o)
> > }
> > }
> >
> > -static inline s32 pm_qos_read_value(struct pm_qos_object *o)
> > +static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
> > {
> > - return o->constraints->target_value;
> > + return c->target_value;
> > }
> >
> > -static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
> > +static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
> > {
> > - o->constraints->target_value = value;
> > + c->target_value = value;
> > }
> >
> > -static void update_target(struct pm_qos_object *o, struct plist_node *node,
> > - int del, int value)
> > +/**
> > + * pm_qos_update_target - manages the constraints list and calls the notifiers
> > + * if needed
> > + * @c: constraints data struct
> > + * @node: request to add to the list, to update or to remove
> > + * @action: action to take on the constraints list
> > + * @value: value of the request to add or update
> > + *
> > + * This function returns 1 if the aggregated constraint value has changed, 0
> > + * otherwise.
> > + */
> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> > + enum pm_qos_req_action action, int value)
> > {
> > unsigned long flags;
> > - int prev_value, curr_value;
> > + int prev_value, curr_value, new_value;
> >
> > spin_lock_irqsave(&pm_qos_lock, flags);
> > - prev_value = pm_qos_get_value(o);
> > - /* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
> > - if (value != PM_QOS_DEFAULT_VALUE) {
> > + prev_value = pm_qos_get_value(c);
> > + if (value == PM_QOS_DEFAULT_VALUE)
> > + new_value = c->default_value;
> > + else
> > + new_value = value;
> > +
> > + switch (action) {
> > + case PM_QOS_REMOVE_REQ:
> We have a remove request API already. This overloading of this
> interface feels wrong to me.
>
> > + plist_del(node, &c->list);
> > + break;
> > + case PM_QOS_UPDATE_REQ:
> > /*
> > * to change the list, we atomically remove, reinit
> > * with new value and add, then see if the extremal
> > * changed
> > */
> > - plist_del(node, &o->constraints->list);
> > - plist_node_init(node, value);
> > - plist_add(node, &o->constraints->list);
> > - } else if (del) {
> > - plist_del(node, &o->constraints->list);
> > - } else {
> > - plist_add(node, &o->constraints->list);
> > + plist_del(node, &c->list);
> > + case PM_QOS_ADD_REQ:
> Don't we have an API for adding a request? if you want to overload
> update like this then either we lose the add API or this shouldn't be
> here.
>
>
> > + plist_node_init(node, new_value);
> > + plist_add(node, &c->list);
> > + break;
> > + default:
> > + /* no action */
> > + ;
> > }
> > - curr_value = pm_qos_get_value(o);
> > - pm_qos_set_value(o, curr_value);
> > +
> > + curr_value = pm_qos_get_value(c);
> > + pm_qos_set_value(c, curr_value);
> > +
> > spin_unlock_irqrestore(&pm_qos_lock, flags);
> >
> > - if (prev_value != curr_value)
> > - blocking_notifier_call_chain(o->constraints->notifiers,
> > + if (prev_value != curr_value) {
> > + blocking_notifier_call_chain(c->notifiers,
> > (unsigned long)curr_value,
> > NULL);
> > + return 1;
> > + } else {
> > + return 0;
> > + }
> > }
> >
> > /**
> > @@ -190,7 +216,7 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
> > */
> > int pm_qos_request(int pm_qos_class)
> > {
> > - return pm_qos_read_value(pm_qos_array[pm_qos_class]);
> > + return pm_qos_read_value(pm_qos_array[pm_qos_class]->constraints);
> > }
> > EXPORT_SYMBOL_GPL(pm_qos_request);
> >
> > @@ -216,20 +242,16 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
> > void pm_qos_add_request(struct pm_qos_request *req,
> > int pm_qos_class, s32 value)
> > {
> > - struct pm_qos_object *o = pm_qos_array[pm_qos_class];
> > - int new_value;
> > + if (!req) /*guard against callers passing in null */
> > + return;
> >
> > if (pm_qos_request_active(req)) {
> > WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
> > return;
> > }
> > - if (value == PM_QOS_DEFAULT_VALUE)
> > - new_value = o->constraints->default_value;
> > - else
> > - new_value = value;
> > - plist_node_init(&req->node, new_value);
> > req->pm_qos_class = pm_qos_class;
> > - update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE);
> > + pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
> > + &req->node, PM_QOS_ADD_REQ, value);
>
> Ok, using pm_qos_update_target to reduce the LOC is ok but I don't think
> this function and the enum should be exported outside of pm_qos.c
They are used by the next patches adding the per-device QoS.
Thanks,
Rafael
^ permalink raw reply
* [Update][PATCH 9/9] ARM / shmobile: Make A3RV be a subdomain of A4LC on SH7372
From: Rafael J. Wysocki @ 2011-08-13 19:43 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, linux-sh
In-Reply-To: <201107311952.51969.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Instead of coding the undocumented dependencies between power domains
A3RV and A4LC on SH7372 directly into the low-level power up/down
routines, make A3RV be a subdomain of A4LC, which will cause the
same dependecies to hold.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
Well, the patch should actually do what's advertised in the changelog
(ie. make A3RV, _not_ A3SG, a subdomain of A4LC).
---
arch/arm/mach-shmobile/pm-sh7372.c | 42 +---------------------------------
arch/arm/mach-shmobile/setup-sh7372.c | 3 ++
2 files changed, 5 insertions(+), 40 deletions(-)
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
@@ -91,35 +91,6 @@ static int pd_power_up(struct generic_pm
return ret;
}
-static int pd_power_up_a3rv(struct generic_pm_domain *genpd)
-{
- int ret = pd_power_up(genpd);
-
- /* force A4LC on after A3RV has been requested on */
- pm_genpd_poweron(&sh7372_a4lc.genpd);
-
- return ret;
-}
-
-static int pd_power_down_a3rv(struct generic_pm_domain *genpd)
-{
- int ret = pd_power_down(genpd);
-
- /* try to power down A4LC after A3RV is requested off */
- genpd_queue_power_off_work(&sh7372_a4lc.genpd);
-
- return ret;
-}
-
-static int pd_power_down_a4lc(struct generic_pm_domain *genpd)
-{
- /* only power down A4LC if A3RV is off */
- if (!(__raw_readl(PSTR) & (1 << sh7372_a3rv.bit_shift)))
- return pd_power_down(genpd);
-
- return -EBUSY;
-}
-
static bool pd_active_wakeup(struct device *dev)
{
return true;
@@ -133,17 +104,8 @@ void sh7372_init_pm_domain(struct sh7372
genpd->stop_device = pm_clk_suspend;
genpd->start_device = pm_clk_resume;
genpd->active_wakeup = pd_active_wakeup;
-
- if (sh7372_pd == &sh7372_a4lc) {
- genpd->power_off = pd_power_down_a4lc;
- genpd->power_on = pd_power_up;
- } else if (sh7372_pd == &sh7372_a3rv) {
- genpd->power_off = pd_power_down_a3rv;
- genpd->power_on = pd_power_up_a3rv;
- } else {
- genpd->power_off = pd_power_down;
- genpd->power_on = pd_power_up;
- }
+ genpd->power_off = pd_power_down;
+ genpd->power_on = pd_power_up;
genpd->power_on(&sh7372_pd->genpd);
}
Index: linux/arch/arm/mach-shmobile/setup-sh7372.c
===================================================================
--- linux.orig/arch/arm/mach-shmobile/setup-sh7372.c
+++ linux/arch/arm/mach-shmobile/setup-sh7372.c
@@ -30,6 +30,7 @@
#include <linux/sh_dma.h>
#include <linux/sh_intc.h>
#include <linux/sh_timer.h>
+#include <linux/pm_domain.h>
#include <mach/hardware.h>
#include <mach/sh7372.h>
#include <asm/mach-types.h>
@@ -848,6 +849,8 @@ void __init sh7372_add_standard_devices(
sh7372_init_pm_domain(&sh7372_a3ri);
sh7372_init_pm_domain(&sh7372_a3sg);
+ pm_genpd_add_subdomain(&sh7372_a4lc.genpd, &sh7372_a3rv.genpd);
+
platform_add_devices(sh7372_early_devices,
ARRAY_SIZE(sh7372_early_devices));
^ permalink raw reply
* Re: [PATCH 06/15] PM QoS: implement the per-device PM QoS constraints
From: mark gross @ 2011-08-13 3:16 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-7-git-send-email-j-pihet@ti.com>
On Thu, Aug 11, 2011 at 05:06:43PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Implement the per-device PM QoS constraints by creating a device
> PM QoS API, which calls the PM QoS constraints management core code.
>
> The per-device latency constraints data strctures are stored
> in the device dev_pm_info struct.
>
> The device PM code calls the init and destroy of the per-device constraints
> data struct in order to support the dynamic insertion and removal of the
> devices in the system.
>
> 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.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> drivers/base/power/Makefile | 4 +-
> drivers/base/power/main.c | 3 +
> drivers/base/power/qos.c | 262 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm.h | 9 ++
> include/linux/pm_qos.h | 39 +++++++
> 5 files changed, 315 insertions(+), 2 deletions(-)
> create mode 100644 drivers/base/power/qos.c
>
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index 3647e11..1a61f89 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,8 +1,8 @@
> -obj-$(CONFIG_PM) += sysfs.o generic_ops.o
> +obj-$(CONFIG_PM) += sysfs.o generic_ops.o qos.o
> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> obj-$(CONFIG_PM_RUNTIME) += runtime.o
> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> obj-$(CONFIG_PM_OPP) += opp.o
> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
>
> -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> \ No newline at end of file
> +ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 06f09bf..f5c0e0e 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -22,6 +22,7 @@
> #include <linux/mutex.h>
> #include <linux/pm.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pm_qos.h>
> #include <linux/resume-trace.h>
> #include <linux/interrupt.h>
> #include <linux/sched.h>
> @@ -97,6 +98,7 @@ void device_pm_add(struct device *dev)
> dev_name(dev->parent));
> list_add_tail(&dev->power.entry, &dpm_list);
> mutex_unlock(&dpm_list_mtx);
> + dev_pm_qos_constraints_init(dev);
> }
>
> /**
> @@ -107,6 +109,7 @@ 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);
> list_del_init(&dev->power.entry);
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> new file mode 100644
> index 0000000..465e419
> --- /dev/null
> +++ b/drivers/base/power/qos.c
> @@ -0,0 +1,262 @@
> +/*
> + * This module exposes the interface to kernel space for specifying
> + * per-device PM QoS dependencies. It provides infrastructure for registration
> + * of:
> + *
> + * Dependents on a QoS value : register requests
> + * Watchers of QoS value : get notified when target QoS value changes
> + *
> + * This QoS design is best effort based. Dependents register their QoS needs.
> + * Watchers register to keep track of the current QoS needs of the system.
> + *
> + * Note about the per-device constraint data struct allocation:
> + * . The per-device constraints data struct ptr is tored into the device
> + * dev_pm_info.
> + * . 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.
> + */
> +
> +/*#define DEBUG*/
> +
> +#include <linux/pm_qos.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +#include <linux/fs.h>
> +#include <linux/device.h>
> +#include <linux/string.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +
> +
> +static void dev_pm_qos_constraints_allocate(struct device *dev);
> +
> +int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
> +{
> + return req->dev != 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
> +
> +/**
> + * dev_pm_qos_add_request - inserts new qos request into the list
> + * @req: pointer to a preallocated handle
> + * @dev: target device for the constraint
> + * @value: defines the qos request
> + *
> + * This function inserts a new entry in the device constraints list of
> + * requested qos performance characteristics. It recomputes the aggregate
> + * QoS expectations of parameters and initializes the dev_pm_qos_request
> + * handle. Caller needs to save this handle for later use in updates and
> + * removal.
> + */
> +void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
> + s32 value)
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
> +
> + if (dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_add_request() called for already "
> + "added request\n");
> + return;
> + }
> + req->dev = dev;
> +
> + /* Allocate the constraints struct on the first call to add_request */
> + if (req->dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
> + dev_pm_qos_constraints_allocate(dev);
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
> + pm_qos_update_target(dev->power.constraints,
> + &req->node, PM_QOS_ADD_REQ, value);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
> +
> +/**
> + * dev_pm_qos_update_request - modifies an existing qos request
> + * @req : handle to list element holding a dev_pm_qos request to use
> + * @value: defines the qos request
> + *
> + * Updates an existing dev PM qos request along with updating the
> + * target value.
> + *
> + * Attempts are made to make this code callable on hot code paths.
> + */
> +void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
> + s32 new_value)
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
> +
> + if (!dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_update_request() called for "
> + "unknown object\n");
> + return;
> + }
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
> + if (new_value != req->node.prio)
> + pm_qos_update_target(
> + req->dev->power.constraints,
> + &req->node, PM_QOS_UPDATE_REQ, new_value);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
> +
> +/**
> + * dev_pm_qos_remove_request - modifies an existing qos request
> + * @req: handle to request list element
> + *
> + * Will remove pm qos request from the list of constraints and
> + * recompute the current target value. Call this on slow code paths.
> + */
> +void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
> +{
> + if (!req) /*guard against callers passing in null */
> + return;
> +
> + if (!dev_pm_qos_request_active(req)) {
> + WARN(1, KERN_ERR "dev_pm_qos_remove_request() called for "
> + "unknown object\n");
> + return;
> + }
> +
> + /* Silently return if the device has been removed */
> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return;
> +
> + pm_qos_update_target(req->dev->power.constraints,
> + &req->node, PM_QOS_REMOVE_REQ,
> + PM_QOS_DEFAULT_VALUE);
> + memset(req, 0, sizeof(*req));
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
> +
> +/**
> + * dev_pm_qos_add_notifier - sets notification entry for changes to target value
> + * of per-device PM QoS constraints
> + *
> + * @dev: target device for the constraint
> + * @notifier: notifier block managed by caller.
> + *
> + * Will register the notifier into a notification chain that gets called
> + * upon changes to the target value for the device.
> + */
> +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
> +{
> + int retval = 0;
> +
> + /* Silently return if the device has been removed */
> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return retval;
> +
> + retval = blocking_notifier_chain_register(
> + dev->power.constraints->notifiers,
> + notifier);
> +
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
> +
> +/**
> + * dev_pm_qos_remove_notifier - deletes notification for changes to target value
> + * of per-device PM QoS constraints
> + *
> + * @dev: target device for the constraint
> + * @notifier: notifier block to be removed.
> + *
> + * Will remove the notifier from the notification chain that gets called
> + * upon changes to the target value.
> + */
> +int dev_pm_qos_remove_notifier(struct device *dev,
> + struct notifier_block *notifier)
> +{
> + int retval = 0;
> +
> + /* Silently return if the device has been removed */
> + if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> + return retval;
> +
> + retval = blocking_notifier_chain_unregister(
> + dev->power.constraints->notifiers,
> + notifier);
> +
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
> +
> +/* Called at the first call to add_request, for constraint data allocation */
> +static void dev_pm_qos_constraints_allocate(struct device *dev)
> +{
> + struct pm_qos_constraints *c;
> + struct blocking_notifier_head *n;
> +
> + c = kzalloc(sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return;
> +
> + n = kzalloc(sizeof(*n), GFP_KERNEL);
> + if (!n) {
> + kfree(c);
> + return;
> + }
> + BLOCKING_INIT_NOTIFIER_HEAD(n);
> +
> + dev->power.constraints = c;
> + plist_head_init(&dev->power.constraints->list, &dev->power.lock);
> + 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;
> +}
> +
> +/* Called from the device PM subsystem at device insertion */
> +void dev_pm_qos_constraints_init(struct device *dev)
> +{
> + dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
> +}
> +
> +/* Called from the device PM subsystem at device removal */
> +void dev_pm_qos_constraints_destroy(struct device *dev)
> +{
> + struct dev_pm_qos_request *req, *tmp;
> + enum dev_pm_qos_state constraints_state = dev->power.constraints_state;
> +
> + dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
> + if (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 per-device
> + * callbacks if needed
> + */
> + pm_qos_update_target(req->dev->power.constraints,
> + &req->node, PM_QOS_REMOVE_REQ,
> + PM_QOS_DEFAULT_VALUE);
> +
> + kfree(dev->power.constraints->notifiers);
> + kfree(dev->power.constraints);
> + dev->power.constraints = NULL;
> + }
> +}
> +
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 411e4f4..aa6dc53 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -421,6 +421,13 @@ enum rpm_request {
>
> struct wakeup_source;
>
> +/* 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 dev_pm_info {
> pm_message_t power_state;
> unsigned int can_wakeup:1;
> @@ -463,6 +470,8 @@ struct dev_pm_info {
> unsigned long accounting_timestamp;
> void *subsys_data; /* Owned by the subsystem. */
> #endif
> + struct pm_qos_constraints *constraints;
> + enum dev_pm_qos_state constraints_state;
> };
>
> extern void update_pm_runtime_accounting(struct device *dev);
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 84aa150..178eaa1 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -19,12 +19,18 @@
> #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
> +#define PM_QOS_DEV_LAT_DEFAULT_VALUE 0
>
> struct pm_qos_request {
> struct plist_node node;
> int pm_qos_class;
> };
>
> +struct dev_pm_qos_request {
> + struct plist_node node;
> + struct device *dev;
> +};
> +
> enum pm_qos_type {
> PM_QOS_UNITIALIZED,
> PM_QOS_MAX, /* return the largest value */
> @@ -64,6 +70,18 @@ int pm_qos_request(int pm_qos_class);
> int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_request_active(struct pm_qos_request *req);
> +
> +int dev_pm_qos_request_active(struct dev_pm_qos_request *req);
> +void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
> + s32 value);
> +void dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
> +void dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
> +int dev_pm_qos_add_notifier(struct device *dev,
> + struct notifier_block *notifier);
> +int dev_pm_qos_remove_notifier(struct device *dev,
> + struct notifier_block *notifier);
> +void dev_pm_qos_constraints_init(struct device *dev);
> +void dev_pm_qos_constraints_destroy(struct device *dev);
> #else
> static inline int pm_qos_update_target(struct pm_qos_constraints *c,
> struct plist_node *node,
> @@ -89,6 +107,27 @@ static inline int pm_qos_remove_notifier(int pm_qos_class,
> { return 0; }
> static inline int pm_qos_request_active(struct pm_qos_request *req)
> { return 0; }
> +
> +static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
> + { return 0; }
> +static inline void dev_pm_qos_add_request(struct dev_pm_qos_request *req,
> + struct device *dev, s32 value)
> + { return; }
> +static inline void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
> + s32 new_value)
> + { return; }
> +static inline void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
> + { return; }
> +static inline int dev_pm_qos_add_notifier(struct device *dev,
> + struct notifier_block *notifier)
> + { return 0; }
> +static inline int dev_pm_qos_remove_notifier(struct device *dev,
> + struct notifier_block *notifier)
> + { return 0; }
> +static inline void dev_pm_qos_constraints_init(struct device *dev)
> + { return; }
> +static inline void dev_pm_qos_constraints_destroy(struct device *dev)
> + { return; }
> #endif
>
> #endif
> --
> 1.7.2.5
>
looks good (except for the use of those enums I don't care for)
--mark
^ permalink raw reply
* Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code
From: mark gross @ 2011-08-13 3:09 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-6-git-send-email-j-pihet@ti.com>
On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> In preparation for the per-device constratins support:
> - rename update_target to pm_qos_update_target
> - generalize and export pm_qos_update_target for usage by the upcoming
> per-device latency constraints framework:
> . operate on struct pm_qos_constraints for constraints management,
> . introduce an 'action' parameter for constraints add/update/remove,
> . the return value indicates if the aggregated constraint value has
> changed,
> - update the internal code to operate on struct pm_qos_constraints
> - add a NULL pointer check in the API functions
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>
> ---
> include/linux/pm_qos.h | 14 ++++++
> kernel/power/qos.c | 123 ++++++++++++++++++++++++++----------------------
> 2 files changed, 81 insertions(+), 56 deletions(-)
>
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 9772311..84aa150 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -44,7 +44,16 @@ struct pm_qos_constraints {
> struct blocking_notifier_head *notifiers;
> };
>
> +/* Action requested to pm_qos_update_target */
> +enum pm_qos_req_action {
> + PM_QOS_ADD_REQ, /* Add a new request */
> + PM_QOS_UPDATE_REQ, /* Update an existing request */
> + PM_QOS_REMOVE_REQ /* Remove an existing request */
> +};
> +
What do you need this enum for? The function names *_update_*, *_add_*,
and *_remove_* seem to be pretty redundant if you have to pass an enum
that could possibly conflict with the function name.
> #ifdef CONFIG_PM
> +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> + enum pm_qos_req_action action, int value);
The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or
there is something strange going on.... BTW what shold this function do
if the pm_qos_req_action was *not* the UPDATE one?
pm_qos_update_target should be a static to the C- file along with the
enum pm_qos_req_action.
> void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
> s32 value);
> void pm_qos_update_request(struct pm_qos_request *req,
> @@ -56,6 +65,11 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_request_active(struct pm_qos_request *req);
> #else
> +static inline int pm_qos_update_target(struct pm_qos_constraints *c,
> + struct plist_node *node,
> + enum pm_qos_req_action action,
> + int value)
> + { return 0; }
> static inline void pm_qos_add_request(struct pm_qos_request *req,
> int pm_qos_class, s32 value)
> { return; }
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 66e8d6f..fc60f96 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -121,17 +121,17 @@ static const struct file_operations pm_qos_power_fops = {
> };
>
> /* unlocked internal variant */
> -static inline int pm_qos_get_value(struct pm_qos_object *o)
> +static inline int pm_qos_get_value(struct pm_qos_constraints *c)
> {
> - if (plist_head_empty(&o->constraints->list))
> - return o->constraints->default_value;
> + if (plist_head_empty(&c->list))
> + return c->default_value;
>
> - switch (o->constraints->type) {
> + switch (c->type) {
> case PM_QOS_MIN:
> - return plist_first(&o->constraints->list)->prio;
> + return plist_first(&c->list)->prio;
>
> case PM_QOS_MAX:
> - return plist_last(&o->constraints->list)->prio;
> + return plist_last(&c->list)->prio;
>
> default:
> /* runtime check for not using enum */
> @@ -139,47 +139,73 @@ static inline int pm_qos_get_value(struct pm_qos_object *o)
> }
> }
>
> -static inline s32 pm_qos_read_value(struct pm_qos_object *o)
> +static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
> {
> - return o->constraints->target_value;
> + return c->target_value;
> }
>
> -static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
> +static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
> {
> - o->constraints->target_value = value;
> + c->target_value = value;
> }
>
> -static void update_target(struct pm_qos_object *o, struct plist_node *node,
> - int del, int value)
> +/**
> + * pm_qos_update_target - manages the constraints list and calls the notifiers
> + * if needed
> + * @c: constraints data struct
> + * @node: request to add to the list, to update or to remove
> + * @action: action to take on the constraints list
> + * @value: value of the request to add or update
> + *
> + * This function returns 1 if the aggregated constraint value has changed, 0
> + * otherwise.
> + */
> +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> + enum pm_qos_req_action action, int value)
> {
> unsigned long flags;
> - int prev_value, curr_value;
> + int prev_value, curr_value, new_value;
>
> spin_lock_irqsave(&pm_qos_lock, flags);
> - prev_value = pm_qos_get_value(o);
> - /* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
> - if (value != PM_QOS_DEFAULT_VALUE) {
> + prev_value = pm_qos_get_value(c);
> + if (value == PM_QOS_DEFAULT_VALUE)
> + new_value = c->default_value;
> + else
> + new_value = value;
> +
> + switch (action) {
> + case PM_QOS_REMOVE_REQ:
We have a remove request API already. This overloading of this
interface feels wrong to me.
> + plist_del(node, &c->list);
> + break;
> + case PM_QOS_UPDATE_REQ:
> /*
> * to change the list, we atomically remove, reinit
> * with new value and add, then see if the extremal
> * changed
> */
> - plist_del(node, &o->constraints->list);
> - plist_node_init(node, value);
> - plist_add(node, &o->constraints->list);
> - } else if (del) {
> - plist_del(node, &o->constraints->list);
> - } else {
> - plist_add(node, &o->constraints->list);
> + plist_del(node, &c->list);
> + case PM_QOS_ADD_REQ:
Don't we have an API for adding a request? if you want to overload
update like this then either we lose the add API or this shouldn't be
here.
> + plist_node_init(node, new_value);
> + plist_add(node, &c->list);
> + break;
> + default:
> + /* no action */
> + ;
> }
> - curr_value = pm_qos_get_value(o);
> - pm_qos_set_value(o, curr_value);
> +
> + curr_value = pm_qos_get_value(c);
> + pm_qos_set_value(c, curr_value);
> +
> spin_unlock_irqrestore(&pm_qos_lock, flags);
>
> - if (prev_value != curr_value)
> - blocking_notifier_call_chain(o->constraints->notifiers,
> + if (prev_value != curr_value) {
> + blocking_notifier_call_chain(c->notifiers,
> (unsigned long)curr_value,
> NULL);
> + return 1;
> + } else {
> + return 0;
> + }
> }
>
> /**
> @@ -190,7 +216,7 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
> */
> int pm_qos_request(int pm_qos_class)
> {
> - return pm_qos_read_value(pm_qos_array[pm_qos_class]);
> + return pm_qos_read_value(pm_qos_array[pm_qos_class]->constraints);
> }
> EXPORT_SYMBOL_GPL(pm_qos_request);
>
> @@ -216,20 +242,16 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
> void pm_qos_add_request(struct pm_qos_request *req,
> int pm_qos_class, s32 value)
> {
> - struct pm_qos_object *o = pm_qos_array[pm_qos_class];
> - int new_value;
> + if (!req) /*guard against callers passing in null */
> + return;
>
> if (pm_qos_request_active(req)) {
> WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
> return;
> }
> - if (value == PM_QOS_DEFAULT_VALUE)
> - new_value = o->constraints->default_value;
> - else
> - new_value = value;
> - plist_node_init(&req->node, new_value);
> req->pm_qos_class = pm_qos_class;
> - update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE);
> + pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
> + &req->node, PM_QOS_ADD_REQ, value);
Ok, using pm_qos_update_target to reduce the LOC is ok but I don't think
this function and the enum should be exported outside of pm_qos.c
--mark
> }
> EXPORT_SYMBOL_GPL(pm_qos_add_request);
>
> @@ -246,9 +268,6 @@ EXPORT_SYMBOL_GPL(pm_qos_add_request);
> void pm_qos_update_request(struct pm_qos_request *req,
> s32 new_value)
> {
> - s32 temp;
> - struct pm_qos_object *o;
> -
> if (!req) /*guard against callers passing in null */
> return;
>
> @@ -257,15 +276,10 @@ void pm_qos_update_request(struct pm_qos_request *req,
> return;
> }
>
> - o = pm_qos_array[req->pm_qos_class];
> -
> - if (new_value == PM_QOS_DEFAULT_VALUE)
> - temp = o->constraints->default_value;
> - else
> - temp = new_value;
> -
> - if (temp != req->node.prio)
> - update_target(o, &req->node, 0, temp);
> + if (new_value != req->node.prio)
> + pm_qos_update_target(
> + pm_qos_array[req->pm_qos_class]->constraints,
> + &req->node, PM_QOS_UPDATE_REQ, new_value);
> }
> EXPORT_SYMBOL_GPL(pm_qos_update_request);
>
> @@ -279,9 +293,7 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
> */
> void pm_qos_remove_request(struct pm_qos_request *req)
> {
> - struct pm_qos_object *o;
> -
> - if (req == NULL)
> + if (!req) /*guard against callers passing in null */
> return;
> /* silent return to keep pcm code cleaner */
>
> @@ -290,8 +302,9 @@ void pm_qos_remove_request(struct pm_qos_request *req)
> return;
> }
>
> - o = pm_qos_array[req->pm_qos_class];
> - update_target(o, &req->node, 1, PM_QOS_DEFAULT_VALUE);
> + pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
> + &req->node, PM_QOS_REMOVE_REQ,
> + PM_QOS_DEFAULT_VALUE);
> memset(req, 0, sizeof(*req));
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_request);
> @@ -395,7 +408,6 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
> {
> s32 value;
> unsigned long flags;
> - struct pm_qos_object *o;
> struct pm_qos_request *req = filp->private_data;
>
> if (!req)
> @@ -403,9 +415,8 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
> if (!pm_qos_request_active(req))
> return -EINVAL;
>
> - o = pm_qos_array[req->pm_qos_class];
> spin_lock_irqsave(&pm_qos_lock, flags);
> - value = pm_qos_get_value(o);
> + value = pm_qos_get_value(pm_qos_array[req->pm_qos_class]->constraints);
> spin_unlock_irqrestore(&pm_qos_lock, flags);
>
> return simple_read_from_buffer(buf, count, f_pos, &value, sizeof(s32));
> --
> 1.7.2.5
>
^ permalink raw reply
* Re: [PATCH 04/15] PM QoS: re-organize data structs
From: mark gross @ 2011-08-13 2:56 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-5-git-send-email-j-pihet@ti.com>
On Thu, Aug 11, 2011 at 05:06:41PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> In preparation for the per-device constratins support, re-organize
> the data strctures:
> - add a struct pm_qos_constraints which contains the constraints
> related data
> - update struct pm_qos_object contents to the PM QoS internal object
> data. Add a pointer to struct pm_qos_constraints
> - update the internal code to use the new data structs.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> include/linux/pm_qos.h | 19 ++++++++++
> kernel/power/qos.c | 90 ++++++++++++++++++++++-------------------------
> 2 files changed, 61 insertions(+), 48 deletions(-)
>
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 6b0968f..9772311 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -25,6 +25,25 @@ struct pm_qos_request {
> int pm_qos_class;
> };
>
> +enum pm_qos_type {
> + PM_QOS_UNITIALIZED,
what is this for?
> + PM_QOS_MAX, /* return the largest value */
> + PM_QOS_MIN /* return the smallest value */
> +};
> +
> +/*
> + * Note: The lockless read path depends on the CPU accessing
> + * target_value atomically. Atomic access is only guaranteed on all CPU
> + * types linux supports for 32 bit quantites
> + */
> +struct pm_qos_constraints {
> + struct plist_head list;
> + s32 target_value; /* Do not change to 64 bit */
> + s32 default_value;
> + enum pm_qos_type type;
> + struct blocking_notifier_head *notifiers;
> +};
> +
> #ifdef CONFIG_PM
> void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
> s32 value);
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index fa9a090..66e8d6f 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -49,62 +49,54 @@
> * or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
> * held, taken with _irqsave. One lock to rule them all
> */
> -enum pm_qos_type {
> - PM_QOS_MAX, /* return the largest value */
> - PM_QOS_MIN /* return the smallest value */
> -};
> -
> -/*
> - * Note: The lockless read path depends on the CPU accessing
> - * target_value atomically. Atomic access is only guaranteed on all CPU
> - * types linux supports for 32 bit quantites
> - */
> struct pm_qos_object {
> - struct plist_head constraints;
> - struct blocking_notifier_head *notifiers;
> + struct pm_qos_constraints *constraints;
> struct miscdevice pm_qos_power_miscdev;
> char *name;
> - s32 target_value; /* Do not change to 64 bit */
> - s32 default_value;
> - enum pm_qos_type type;
> };
>
> static DEFINE_SPINLOCK(pm_qos_lock);
>
> static struct pm_qos_object null_pm_qos;
> +
> static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> -static struct pm_qos_object cpu_dma_pm_qos = {
> - .constraints = PLIST_HEAD_INIT(cpu_dma_pm_qos.constraints, pm_qos_lock),
> - .notifiers = &cpu_dma_lat_notifier,
> - .name = "cpu_dma_latency",
> +static struct pm_qos_constraints cpu_dma_constraints = {
> + .list = PLIST_HEAD_INIT(cpu_dma_constraints.list, pm_qos_lock),
> .target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
> .default_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
> .type = PM_QOS_MIN,
> + .notifiers = &cpu_dma_lat_notifier,
> +};
> +static struct pm_qos_object cpu_dma_pm_qos = {
> + .constraints = &cpu_dma_constraints,
> + .name = "cpu_dma_latency",
> };
>
> static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> -static struct pm_qos_object network_lat_pm_qos = {
> - .constraints = PLIST_HEAD_INIT(network_lat_pm_qos.constraints,
> - pm_qos_lock),
> - .notifiers = &network_lat_notifier,
> - .name = "network_latency",
> +static struct pm_qos_constraints network_lat_constraints = {
> + .list = PLIST_HEAD_INIT(network_lat_constraints.list, pm_qos_lock),
> .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> .default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .type = PM_QOS_MIN
> + .type = PM_QOS_MIN,
> + .notifiers = &network_lat_notifier,
> +};
> +static struct pm_qos_object network_lat_pm_qos = {
> + .constraints = &network_lat_constraints,
> + .name = "network_latency",
> };
> -
>
> static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> -static struct pm_qos_object network_throughput_pm_qos = {
> - .constraints = PLIST_HEAD_INIT(network_throughput_pm_qos.constraints,
> - pm_qos_lock),
> - .notifiers = &network_throughput_notifier,
> - .name = "network_throughput",
> +static struct pm_qos_constraints network_tput_constraints = {
> + .list = PLIST_HEAD_INIT(network_tput_constraints.list, pm_qos_lock),
> .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> .default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> .type = PM_QOS_MAX,
> + .notifiers = &network_throughput_notifier,
> +};
> +static struct pm_qos_object network_throughput_pm_qos = {
> + .constraints = &network_tput_constraints,
> + .name = "network_throughput",
> };
> -
>
> static struct pm_qos_object *pm_qos_array[] = {
> &null_pm_qos,
> @@ -131,15 +123,15 @@ static const struct file_operations pm_qos_power_fops = {
> /* unlocked internal variant */
> static inline int pm_qos_get_value(struct pm_qos_object *o)
> {
> - if (plist_head_empty(&o->constraints))
> - return o->default_value;
> + if (plist_head_empty(&o->constraints->list))
> + return o->constraints->default_value;
>
> - switch (o->type) {
> + switch (o->constraints->type) {
> case PM_QOS_MIN:
> - return plist_first(&o->constraints)->prio;
> + return plist_first(&o->constraints->list)->prio;
>
> case PM_QOS_MAX:
> - return plist_last(&o->constraints)->prio;
> + return plist_last(&o->constraints->list)->prio;
>
> default:
> /* runtime check for not using enum */
> @@ -149,12 +141,12 @@ static inline int pm_qos_get_value(struct pm_qos_object *o)
>
> static inline s32 pm_qos_read_value(struct pm_qos_object *o)
> {
> - return o->target_value;
> + return o->constraints->target_value;
> }
>
> static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
> {
> - o->target_value = value;
> + o->constraints->target_value = value;
> }
>
> static void update_target(struct pm_qos_object *o, struct plist_node *node,
> @@ -172,20 +164,20 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
> * with new value and add, then see if the extremal
> * changed
> */
> - plist_del(node, &o->constraints);
> + plist_del(node, &o->constraints->list);
> plist_node_init(node, value);
> - plist_add(node, &o->constraints);
> + plist_add(node, &o->constraints->list);
> } else if (del) {
> - plist_del(node, &o->constraints);
> + plist_del(node, &o->constraints->list);
> } else {
> - plist_add(node, &o->constraints);
> + plist_add(node, &o->constraints->list);
> }
> curr_value = pm_qos_get_value(o);
> pm_qos_set_value(o, curr_value);
> spin_unlock_irqrestore(&pm_qos_lock, flags);
>
> if (prev_value != curr_value)
> - blocking_notifier_call_chain(o->notifiers,
> + blocking_notifier_call_chain(o->constraints->notifiers,
> (unsigned long)curr_value,
> NULL);
> }
> @@ -232,7 +224,7 @@ void pm_qos_add_request(struct pm_qos_request *req,
> return;
> }
> if (value == PM_QOS_DEFAULT_VALUE)
> - new_value = o->default_value;
> + new_value = o->constraints->default_value;
> else
> new_value = value;
> plist_node_init(&req->node, new_value);
> @@ -268,7 +260,7 @@ void pm_qos_update_request(struct pm_qos_request *req,
> o = pm_qos_array[req->pm_qos_class];
>
> if (new_value == PM_QOS_DEFAULT_VALUE)
> - temp = o->default_value;
> + temp = o->constraints->default_value;
> else
> temp = new_value;
>
> @@ -317,7 +309,8 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
> int retval;
>
> retval = blocking_notifier_chain_register(
> - pm_qos_array[pm_qos_class]->notifiers, notifier);
> + pm_qos_array[pm_qos_class]->constraints->notifiers,
> + notifier);
>
> return retval;
> }
> @@ -336,7 +329,8 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
> int retval;
>
> retval = blocking_notifier_chain_unregister(
> - pm_qos_array[pm_qos_class]->notifiers, notifier);
> + pm_qos_array[pm_qos_class]->constraints->notifiers,
> + notifier);
>
> return retval;
> }
> --
> 1.7.2.5
>
looks ok but I'm wondering what the uninititalized qos type is for.
--mark
^ permalink raw reply
* Re: [PATCH 03/15] PM QoS: code re-organization
From: mark gross @ 2011-08-13 2:50 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-4-git-send-email-j-pihet@ti.com>
On Thu, Aug 11, 2011 at 05:06:40PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Move around the PM QoS misc devices management code
> for better readability.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> kernel/power/qos.c | 45 +++++++++++++++++++++++----------------------
> 1 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 61275f2..fa9a090 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -190,28 +190,6 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
> NULL);
> }
>
> -static int register_pm_qos_misc(struct pm_qos_object *qos)
> -{
> - qos->pm_qos_power_miscdev.minor = MISC_DYNAMIC_MINOR;
> - qos->pm_qos_power_miscdev.name = qos->name;
> - qos->pm_qos_power_miscdev.fops = &pm_qos_power_fops;
> -
> - return misc_register(&qos->pm_qos_power_miscdev);
> -}
> -
> -static int find_pm_qos_object_by_minor(int minor)
> -{
> - int pm_qos_class;
> -
> - for (pm_qos_class = 0;
> - pm_qos_class < PM_QOS_NUM_CLASSES; pm_qos_class++) {
> - if (minor ==
> - pm_qos_array[pm_qos_class]->pm_qos_power_miscdev.minor)
> - return pm_qos_class;
> - }
> - return -1;
> -}
> -
> /**
> * pm_qos_request - returns current system wide qos expectation
> * @pm_qos_class: identification of which qos value is requested
> @@ -364,6 +342,29 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>
> +/* User space interface to PM QoS classes via misc devices */
> +static int register_pm_qos_misc(struct pm_qos_object *qos)
> +{
> + qos->pm_qos_power_miscdev.minor = MISC_DYNAMIC_MINOR;
> + qos->pm_qos_power_miscdev.name = qos->name;
> + qos->pm_qos_power_miscdev.fops = &pm_qos_power_fops;
> +
> + return misc_register(&qos->pm_qos_power_miscdev);
> +}
> +
> +static int find_pm_qos_object_by_minor(int minor)
> +{
> + int pm_qos_class;
> +
> + for (pm_qos_class = 0;
> + pm_qos_class < PM_QOS_NUM_CLASSES; pm_qos_class++) {
> + if (minor ==
> + pm_qos_array[pm_qos_class]->pm_qos_power_miscdev.minor)
> + return pm_qos_class;
> + }
> + return -1;
> +}
> +
> static int pm_qos_power_open(struct inode *inode, struct file *filp)
> {
> long pm_qos_class;
> --
> 1.7.2.5
>
I don't see the point of this but its ok.
Acked-by:markgross <markgross@thegnar.org>
^ permalink raw reply
* Re: [PATCH 02/15] PM QoS: minor clean-ups
From: mark gross @ 2011-08-13 2:48 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, Mark Brown, Linux PM mailing list, linux-omap,
Jean Pihet
In-Reply-To: <1313075212-8366-3-git-send-email-j-pihet@ti.com>
On Thu, Aug 11, 2011 at 05:06:39PM +0200, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> - Misc fixes to improve code readability:
> . rename struct pm_qos_request_list to struct pm_qos_request,
> . rename pm_qos_req parameter to req in internal code,
> consistenly use req in the API parameters,
> . update the in-kernel API callers to the new parameters names,
> . rename of fields names (requests, list, node, constraints)
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> drivers/media/video/via-camera.c | 2 +-
> drivers/net/wireless/ipw2x00/ipw2100.c | 2 +-
> include/linux/netdevice.h | 2 +-
> include/linux/pm_qos.h | 22 ++++----
> include/sound/pcm.h | 2 +-
> kernel/power/qos.c | 90 ++++++++++++++++---------------
> 6 files changed, 61 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/media/video/via-camera.c b/drivers/media/video/via-camera.c
> index b3ca389..fba6c64 100644
> --- a/drivers/media/video/via-camera.c
> +++ b/drivers/media/video/via-camera.c
> @@ -69,7 +69,7 @@ struct via_camera {
> struct mutex lock;
> enum viacam_opstate opstate;
> unsigned long flags;
> - struct pm_qos_request_list qos_request;
> + struct pm_qos_request qos_request;
> /*
> * GPIO info for power/reset management
> */
> diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c b/drivers/net/wireless/ipw2x00/ipw2100.c
> index d9df575..f323ec0 100644
> --- a/drivers/net/wireless/ipw2x00/ipw2100.c
> +++ b/drivers/net/wireless/ipw2x00/ipw2100.c
> @@ -174,7 +174,7 @@ that only one external action is invoked at a time.
> #define DRV_DESCRIPTION "Intel(R) PRO/Wireless 2100 Network Driver"
> #define DRV_COPYRIGHT "Copyright(c) 2003-2006 Intel Corporation"
>
> -static struct pm_qos_request_list ipw2100_pm_qos_req;
> +static struct pm_qos_request ipw2100_pm_qos_req;
>
> /* Debugging stuff */
> #ifdef CONFIG_IPW2100_DEBUG
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index cc1eb9e..82f01d9 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -999,7 +999,7 @@ struct net_device {
> */
> char name[IFNAMSIZ];
>
> - struct pm_qos_request_list pm_qos_req;
> + struct pm_qos_request pm_qos_req;
>
> /* device name hash chain */
> struct hlist_node name_hlist;
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 7ba67541..6b0968f 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -20,30 +20,30 @@
> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
>
> -struct pm_qos_request_list {
> - struct plist_node list;
> +struct pm_qos_request {
> + struct plist_node node;
> int pm_qos_class;
> };
>
> #ifdef CONFIG_PM
> -void pm_qos_add_request(struct pm_qos_request_list *l,
> - int pm_qos_class, s32 value);
> -void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> +void pm_qos_add_request(struct pm_qos_request *req, int pm_qos_class,
> + s32 value);
> +void pm_qos_update_request(struct pm_qos_request *req,
> s32 new_value);
> -void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
> +void pm_qos_remove_request(struct pm_qos_request *req);
>
> int pm_qos_request(int pm_qos_class);
> int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
> int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
> -int pm_qos_request_active(struct pm_qos_request_list *req);
> +int pm_qos_request_active(struct pm_qos_request *req);
> #else
> -static inline void pm_qos_add_request(struct pm_qos_request_list *l,
> +static inline void pm_qos_add_request(struct pm_qos_request *req,
> int pm_qos_class, s32 value)
> { return; }
> -static inline void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> +static inline void pm_qos_update_request(struct pm_qos_request *req,
> s32 new_value)
> { return; }
> -static inline void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
> +static inline void pm_qos_remove_request(struct pm_qos_request *req)
> { return; }
>
> static inline int pm_qos_request(int pm_qos_class)
> @@ -54,7 +54,7 @@ static inline int pm_qos_add_notifier(int pm_qos_class,
> static inline int pm_qos_remove_notifier(int pm_qos_class,
> struct notifier_block *notifier)
> { return 0; }
> -static inline int pm_qos_request_active(struct pm_qos_request_list *req)
> +static inline int pm_qos_request_active(struct pm_qos_request *req)
> { return 0; }
> #endif
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 1204f17..d3b068f 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -373,7 +373,7 @@ struct snd_pcm_substream {
> int number;
> char name[32]; /* substream name */
> int stream; /* stream (direction) */
> - struct pm_qos_request_list latency_pm_qos_req; /* pm_qos request */
> + struct pm_qos_request latency_pm_qos_req; /* pm_qos request */
> size_t buffer_bytes_max; /* limit ring buffer size */
> struct snd_dma_buffer dma_buffer;
> unsigned int dma_buf_id;
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 3bf69f1..61275f2 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -45,7 +45,7 @@
> #include <linux/uaccess.h>
>
> /*
> - * locking rule: all changes to requests or notifiers lists
> + * locking rule: all changes to constraints or notifiers lists
> * or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
> * held, taken with _irqsave. One lock to rule them all
> */
> @@ -60,7 +60,7 @@ enum pm_qos_type {
> * types linux supports for 32 bit quantites
> */
> struct pm_qos_object {
> - struct plist_head requests;
> + struct plist_head constraints;
> struct blocking_notifier_head *notifiers;
> struct miscdevice pm_qos_power_miscdev;
> char *name;
> @@ -74,7 +74,7 @@ static DEFINE_SPINLOCK(pm_qos_lock);
> static struct pm_qos_object null_pm_qos;
> static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> static struct pm_qos_object cpu_dma_pm_qos = {
> - .requests = PLIST_HEAD_INIT(cpu_dma_pm_qos.requests, pm_qos_lock),
> + .constraints = PLIST_HEAD_INIT(cpu_dma_pm_qos.constraints, pm_qos_lock),
> .notifiers = &cpu_dma_lat_notifier,
> .name = "cpu_dma_latency",
> .target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
> @@ -84,7 +84,8 @@ static struct pm_qos_object cpu_dma_pm_qos = {
>
> static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> static struct pm_qos_object network_lat_pm_qos = {
> - .requests = PLIST_HEAD_INIT(network_lat_pm_qos.requests, pm_qos_lock),
> + .constraints = PLIST_HEAD_INIT(network_lat_pm_qos.constraints,
> + pm_qos_lock),
> .notifiers = &network_lat_notifier,
> .name = "network_latency",
> .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> @@ -95,7 +96,8 @@ static struct pm_qos_object network_lat_pm_qos = {
>
> static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> static struct pm_qos_object network_throughput_pm_qos = {
> - .requests = PLIST_HEAD_INIT(network_throughput_pm_qos.requests, pm_qos_lock),
> + .constraints = PLIST_HEAD_INIT(network_throughput_pm_qos.constraints,
> + pm_qos_lock),
> .notifiers = &network_throughput_notifier,
> .name = "network_throughput",
> .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> @@ -129,15 +131,15 @@ static const struct file_operations pm_qos_power_fops = {
> /* unlocked internal variant */
> static inline int pm_qos_get_value(struct pm_qos_object *o)
> {
> - if (plist_head_empty(&o->requests))
> + if (plist_head_empty(&o->constraints))
> return o->default_value;
>
> switch (o->type) {
> case PM_QOS_MIN:
> - return plist_first(&o->requests)->prio;
> + return plist_first(&o->constraints)->prio;
>
> case PM_QOS_MAX:
> - return plist_last(&o->requests)->prio;
> + return plist_last(&o->constraints)->prio;
>
> default:
> /* runtime check for not using enum */
> @@ -170,13 +172,13 @@ static void update_target(struct pm_qos_object *o, struct plist_node *node,
> * with new value and add, then see if the extremal
> * changed
> */
> - plist_del(node, &o->requests);
> + plist_del(node, &o->constraints);
> plist_node_init(node, value);
> - plist_add(node, &o->requests);
> + plist_add(node, &o->constraints);
> } else if (del) {
> - plist_del(node, &o->requests);
> + plist_del(node, &o->constraints);
> } else {
> - plist_add(node, &o->requests);
> + plist_add(node, &o->constraints);
> }
> curr_value = pm_qos_get_value(o);
> pm_qos_set_value(o, curr_value);
> @@ -222,7 +224,7 @@ int pm_qos_request(int pm_qos_class)
> }
> EXPORT_SYMBOL_GPL(pm_qos_request);
>
> -int pm_qos_request_active(struct pm_qos_request_list *req)
> +int pm_qos_request_active(struct pm_qos_request *req)
> {
> return req->pm_qos_class != 0;
> }
> @@ -230,24 +232,24 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
>
> /**
> * pm_qos_add_request - inserts new qos request into the list
> - * @dep: pointer to a preallocated handle
> + * @req: pointer to a preallocated handle
> * @pm_qos_class: identifies which list of qos request to use
> * @value: defines the qos request
> *
> * This function inserts a new entry in the pm_qos_class list of requested qos
> * performance characteristics. It recomputes the aggregate QoS expectations
> - * for the pm_qos_class of parameters and initializes the pm_qos_request_list
> + * for the pm_qos_class of parameters and initializes the pm_qos_request
> * handle. Caller needs to save this handle for later use in updates and
> * removal.
> */
>
> -void pm_qos_add_request(struct pm_qos_request_list *dep,
> +void pm_qos_add_request(struct pm_qos_request *req,
> int pm_qos_class, s32 value)
> {
> struct pm_qos_object *o = pm_qos_array[pm_qos_class];
> int new_value;
>
> - if (pm_qos_request_active(dep)) {
> + if (pm_qos_request_active(req)) {
> WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
> return;
> }
> @@ -255,15 +257,15 @@ void pm_qos_add_request(struct pm_qos_request_list *dep,
> new_value = o->default_value;
> else
> new_value = value;
> - plist_node_init(&dep->list, new_value);
> - dep->pm_qos_class = pm_qos_class;
> - update_target(o, &dep->list, 0, PM_QOS_DEFAULT_VALUE);
> + plist_node_init(&req->node, new_value);
> + req->pm_qos_class = pm_qos_class;
> + update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE);
> }
> EXPORT_SYMBOL_GPL(pm_qos_add_request);
>
> /**
> * pm_qos_update_request - modifies an existing qos request
> - * @pm_qos_req : handle to list element holding a pm_qos request to use
> + * @req : handle to list element holding a pm_qos request to use
> * @value: defines the qos request
> *
> * Updates an existing qos request for the pm_qos_class of parameters along
> @@ -271,56 +273,56 @@ EXPORT_SYMBOL_GPL(pm_qos_add_request);
> *
> * Attempts are made to make this code callable on hot code paths.
> */
> -void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> +void pm_qos_update_request(struct pm_qos_request *req,
> s32 new_value)
> {
> s32 temp;
> struct pm_qos_object *o;
>
> - if (!pm_qos_req) /*guard against callers passing in null */
> + if (!req) /*guard against callers passing in null */
> return;
>
> - if (!pm_qos_request_active(pm_qos_req)) {
> + if (!pm_qos_request_active(req)) {
> WARN(1, KERN_ERR "pm_qos_update_request() called for unknown object\n");
> return;
> }
>
> - o = pm_qos_array[pm_qos_req->pm_qos_class];
> + o = pm_qos_array[req->pm_qos_class];
>
> if (new_value == PM_QOS_DEFAULT_VALUE)
> temp = o->default_value;
> else
> temp = new_value;
>
> - if (temp != pm_qos_req->list.prio)
> - update_target(o, &pm_qos_req->list, 0, temp);
> + if (temp != req->node.prio)
> + update_target(o, &req->node, 0, temp);
> }
> EXPORT_SYMBOL_GPL(pm_qos_update_request);
>
> /**
> * pm_qos_remove_request - modifies an existing qos request
> - * @pm_qos_req: handle to request list element
> + * @req: handle to request list element
> *
> - * Will remove pm qos request from the list of requests and
> + * Will remove pm qos request from the list of constraints and
> * recompute the current target value for the pm_qos_class. Call this
> * on slow code paths.
> */
> -void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
> +void pm_qos_remove_request(struct pm_qos_request *req)
> {
> struct pm_qos_object *o;
>
> - if (pm_qos_req == NULL)
> + if (req == NULL)
> return;
> /* silent return to keep pcm code cleaner */
>
> - if (!pm_qos_request_active(pm_qos_req)) {
> + if (!pm_qos_request_active(req)) {
> WARN(1, KERN_ERR "pm_qos_remove_request() called for unknown object\n");
> return;
> }
>
> - o = pm_qos_array[pm_qos_req->pm_qos_class];
> - update_target(o, &pm_qos_req->list, 1, PM_QOS_DEFAULT_VALUE);
> - memset(pm_qos_req, 0, sizeof(*pm_qos_req));
> + o = pm_qos_array[req->pm_qos_class];
> + update_target(o, &req->node, 1, PM_QOS_DEFAULT_VALUE);
> + memset(req, 0, sizeof(*req));
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>
> @@ -368,7 +370,7 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>
> pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
> if (pm_qos_class >= 0) {
> - struct pm_qos_request_list *req = kzalloc(sizeof(*req), GFP_KERNEL);
> + struct pm_qos_request *req = kzalloc(sizeof(*req), GFP_KERNEL);
> if (!req)
> return -ENOMEM;
>
> @@ -383,7 +385,7 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>
> static int pm_qos_power_release(struct inode *inode, struct file *filp)
> {
> - struct pm_qos_request_list *req;
> + struct pm_qos_request *req;
>
> req = filp->private_data;
> pm_qos_remove_request(req);
> @@ -399,14 +401,14 @@ static ssize_t pm_qos_power_read(struct file *filp, char __user *buf,
> s32 value;
> unsigned long flags;
> struct pm_qos_object *o;
> - struct pm_qos_request_list *pm_qos_req = filp->private_data;
> + struct pm_qos_request *req = filp->private_data;
>
> - if (!pm_qos_req)
> + if (!req)
> return -EINVAL;
> - if (!pm_qos_request_active(pm_qos_req))
> + if (!pm_qos_request_active(req))
> return -EINVAL;
>
> - o = pm_qos_array[pm_qos_req->pm_qos_class];
> + o = pm_qos_array[req->pm_qos_class];
> spin_lock_irqsave(&pm_qos_lock, flags);
> value = pm_qos_get_value(o);
> spin_unlock_irqrestore(&pm_qos_lock, flags);
> @@ -418,7 +420,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> size_t count, loff_t *f_pos)
> {
> s32 value;
> - struct pm_qos_request_list *pm_qos_req;
> + struct pm_qos_request *req;
>
> if (count == sizeof(s32)) {
> if (copy_from_user(&value, buf, sizeof(s32)))
> @@ -449,8 +451,8 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> return -EINVAL;
> }
>
> - pm_qos_req = filp->private_data;
> - pm_qos_update_request(pm_qos_req, value);
> + req = filp->private_data;
> + pm_qos_update_request(req, value);
>
> return count;
> }
> --
> 1.7.2.5
>
Acked-by: markgross<markgross@thegnar.org>
^ 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