* Re: better oopsing when frozen
From: Rafael J. Wysocki @ 2011-07-29 20:29 UTC (permalink / raw)
To: Oliver Neukum; +Cc: linux-pm, linux-kernel
In-Reply-To: <201107292227.36424.oliver@neukum.org>
On Friday, July 29, 2011, Oliver Neukum wrote:
> Am Freitag, 29. Juli 2011, 21:54:30 schrieb Pavel Machek:
> > On Mon 2011-07-25 10:43:19, Oliver Neukum wrote:
> > > Hi Rafael,
> > >
> > > I had a problem with the kernel stopping the machine forever because I got an
> > > oops while tasks were frozen. It seems to me that we should thaw when this
> > > happens. How about this approach?
> >
> > Danger, Will Robinson. You must not write to the filesystems after
> > hibernation started. By thawing, you may do just that.
> >
> > It should be safe to thaw as long as final suspend signature is not
> > on disk and will not be written there.
>
> Yes. But this applies only if I use kernel-space hibernation, doesn't it?
> So I need a second flag for the signature being written. Any other problem?
Yes, please see my reply to Pavel.
Thanks,
Rafael
^ permalink raw reply
* Re: better oopsing when frozen
From: Oliver Neukum @ 2011-07-29 21:04 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Andrew Morton, linux-pm, linux-kernel
In-Reply-To: <201107292227.54948.rjw@sisk.pl>
Am Freitag, 29. Juli 2011, 22:27:54 schrieb Rafael J. Wysocki:
> However, there's another problem I didn't think of before. Namely,
> we cannot thaw tasks before resuming devices in case we've already
> suspended them, because that will defeat the very purpose of the
> freezing in the first place.
That purpose is already defeated. The machine cannot be deader than dead.
We could try to go through the device tree. But the machine just oopsed
quite likely doing just that, so this doesn't seem wise to me.
IMHO if we oops while frozen, chances are the system is going to die.
It certainly dies now. However, we must certainly not thaw if a valid
image is already on disk, lest we corrupt a filesystem.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH v3 1/2] Input: enable i8042-level wakeup control
From: Rafael J. Wysocki @ 2011-07-29 21:04 UTC (permalink / raw)
To: linux-pm; +Cc: linux-input, Daniel Drake, dtor, dmitry.torokhov, dilinger
In-Reply-To: <CAGq3pz6J2pma1TxcqUreQ-qi37Tk4VU15uAvH1=dwyAAvxAPLg@mail.gmail.com>
On Thursday, July 28, 2011, Daniel Drake wrote:
> On 28 July 2011 16:08, Alan Stern <stern@rowland.harvard.edu> wrote:
> > Aren't there? I have no idea. But if input devices are never
> > wakeup-capable, why do you name your function "input_may_wakeup()"?
>
> I guess i8042_may_wakeup() would be a better name for it.
Yes, it would.
> In this case, the only device in the hierarchy (i8042 -> serio -> input) that
> is marked wakeup capable is the i8042 device (and only via my patches). I'd
> welcome a better design.
>
> > I'm using a normal PC with PS/2 mouse and keyboard. It is set up with
> > the keyboard as a wakeup source but not the mouse. Of course, this
> > uses ACPI, which I assume isn't present on OLPC.
>
> It does depend on how the system is designed, yes. I suspect that the
> device that is actually marked as wakeup capable on your setup is not
> an input device, but rather another part of the /sys tree (perhaps
> part of the ACPI tree?).
If there's an ACPI object corresponding to the device and it allows the
device to be configured for wakeup, then device_can_wakeup() will be true
for the device's struct device object (not the ACPI one).
> Also, I suspect that if your system is woken up by the keyboard, it loses the
> keypress which was used to wake it up.
Yes, it does, because we don't even try to handle the ACPI case. One reason
is that on some systems keyboard wakeup is handled entirely by the BIOS behind
our back.
> I'm definitely open to better ways of doing this. The key considerations are:
> 1. We only have control over wakeups at the i8042 level; i.e. we
> cannot ask for keyboard wakeups but not mouse.
> 2. Our keyboard and mouse hardware is guaranteed to be powered
> throughout suspend
> 3. The key press or mouse movement or click that woke the system must
> not be lost during resume. This means that all 3 layers (i8042, serio,
> input) must not reset or really do anything with the device during
> suspend/resume.
Have you considered marking all of the devices in the chain as "wakeup
capable" in that case? Then, one would have to enable wakeup for all of
them for things to work, in analogy with the USB case (a USB mouse or
keyboard may be a wakeup-capable device, but you also need to enable the
controller it's attached to to wake up).
Thanks,
Rafael
^ permalink raw reply
* Re: better oopsing when frozen
From: Rafael J. Wysocki @ 2011-07-29 21:09 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Andrew Morton, linux-pm, linux-kernel
In-Reply-To: <201107292304.10738.oliver@neukum.org>
On Friday, July 29, 2011, Oliver Neukum wrote:
> Am Freitag, 29. Juli 2011, 22:27:54 schrieb Rafael J. Wysocki:
> > However, there's another problem I didn't think of before. Namely,
> > we cannot thaw tasks before resuming devices in case we've already
> > suspended them, because that will defeat the very purpose of the
> > freezing in the first place.
>
> That purpose is already defeated. The machine cannot be deader than dead.
OK, so what exactly is the purpose of the thawing, then?
Rafael
^ permalink raw reply
* Re: better oopsing when frozen
From: Oliver Neukum @ 2011-07-29 21:17 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Andrew Morton, linux-pm, linux-kernel
In-Reply-To: <201107292309.44877.rjw@sisk.pl>
Am Freitag, 29. Juli 2011, 23:09:44 schrieb Rafael J. Wysocki:
> On Friday, July 29, 2011, Oliver Neukum wrote:
> > Am Freitag, 29. Juli 2011, 22:27:54 schrieb Rafael J. Wysocki:
> > > However, there's another problem I didn't think of before. Namely,
> > > we cannot thaw tasks before resuming devices in case we've already
> > > suspended them, because that will defeat the very purpose of the
> > > freezing in the first place.
> >
> > That purpose is already defeated. The machine cannot be deader than dead.
>
> OK, so what exactly is the purpose of the thawing, then?
We may be lucky enough to get the oops written out to disk. The oops may happen
in a driver rarely used and be late in the sequence.
Regards
Oliver
^ permalink raw reply
* Re: [RFC/PATCH v3 00/13] PM QoS: add a per-device latency constraints class
From: Rafael J. Wysocki @ 2011-07-29 21:25 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-1-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 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.
>
> High level implementation:
>
> 1. Add a new PM QoS class for device wake-up constraints (PM_QOS_DEV_LATENCY).
> . Define a pm_qos_constraints struct for the storage of the constraints list
> and associated values (target_value, default_value, type ...).
> . Update the pm_qos_object struct with the information related to a PM QoS
> class: ptr to constraints list, notifer ptr, name ...
> . Each PM QoS class statically declare objects for pm_qos_object and
> pm_qos_constraints. The only exception is the devices constraints, cf. below.
> . The device constraints class is statically declaring a pm_qos_object. The
> pm_qos_constraints are per-device and so are embedded into the device struct.
>
> The new class is available from kernel drivers and shall be made available
> to user space through a per-device sysfs entry. User space API to come as a
> subsequent patch.
>
> 2. Added a notification of device insertion/removal from the device PM framework
> to PM QoS.
> This allows to init/de-init the per-device constraints list upon device insertion
> and removal.
> RFC state for comments and review, lightly tested
>
> 3. Make the pm_qos_add_request API more generic by using a
> struct pm_qos_parameters parameter. This allows easy extension in the future.
>
> 4. Upon a change of the aggregated constraint value in the PM_QOS_DEV_LATENCY class
> a notification chain mechanism is used to take action on the system.
> This is the proposed way to have PM QoS and the platform dependant code to
> interact with each other, cf. 5 below.
> The notification mechanism now passes the constraint request struct ptr in
> order for the notifier callback to have access to the full set of constraint
> data, e.g. the struct device ptr.
>
> 5. cpuidle interaction with the OMAP3 cpuidle handler
> Since cpuidle is a CPU centric framework it decides the MPU next power state
> based on the MPU exit_latency and target_residency figures.
>
> The rest of the power domains get their next power state programmed from
> the PM_QOS_DEV_LATENCY class of the PM QoS framework, via the device
> wake-up latency constraints callback to the OMAP_PM_CONSTRAINTS framework.
>
> Note: the exit_latency and target_residency figures of the MPU include the MPU
> itself and the peripherals needed for the MPU to execute instructions (e.g.
> main memory, caches, IRQ controller, MMU etc).
> Some of those peripherals can belong to other power domains than the MPU
> subsystem and so the corresponding latencies must be included in those figures.
>
> 6. Update the pm_qos_add_request callers to the generic API
>
> 7. Misc fixes to improve code readability:
> . rename of the PM QoS implementation file from pm_qos_params.[ch] to pm_qos.[ch]
> . rename of fields names (request, list, constraints, class),
> . simplification of the in-kernel PM QoS API implementation. The main logic part
> is now implemented in the update_target function.
>
> Questions:
> 1. per-device user-space API: since sysfs does not provide open/close
> callbacks it is not possible to support multiple and simultaneous users of
> the per-device sysfs entry. A user-space constraints aggregation application is
> needed in case of multiple constraints for a device. Is this the way to go?
I'd say so.
Thanks,
Rafael
^ permalink raw reply
* Re: better oopsing when frozen
From: Rafael J. Wysocki @ 2011-07-29 21:30 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Andrew Morton, linux-pm, linux-kernel
In-Reply-To: <201107292317.42737.oliver@neukum.org>
On Friday, July 29, 2011, Oliver Neukum wrote:
> Am Freitag, 29. Juli 2011, 23:09:44 schrieb Rafael J. Wysocki:
> > On Friday, July 29, 2011, Oliver Neukum wrote:
> > > Am Freitag, 29. Juli 2011, 22:27:54 schrieb Rafael J. Wysocki:
> > > > However, there's another problem I didn't think of before. Namely,
> > > > we cannot thaw tasks before resuming devices in case we've already
> > > > suspended them, because that will defeat the very purpose of the
> > > > freezing in the first place.
> > >
> > > That purpose is already defeated. The machine cannot be deader than dead.
> >
> > OK, so what exactly is the purpose of the thawing, then?
>
> We may be lucky enough to get the oops written out to disk. The oops may happen
> in a driver rarely used and be late in the sequence.
OK, so is there any guarantee that we won't corrupt things this way?
Rafael
^ permalink raw reply
* Re: [RFC/PATCH v3 00/13] PM QoS: add a per-device latency constraints class
From: Rafael J. Wysocki @ 2011-07-29 21:46 UTC (permalink / raw)
To: markgross; +Cc: broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <20110729142459.GA5509@gvim.org>
On Friday, July 29, 2011, mark gross wrote:
> On Fri, Jul 29, 2011 at 10:37:52AM +0200, Jean Pihet wrote:
> > Mark,
> >
> > On Thu, Jul 28, 2011 at 3:14 PM, mark gross <markgross@thegnar.org> wrote:
> > > On Thu, Jul 28, 2011 at 10:30:07AM +0200, jean.pihet@newoldbits.com wrote:
> > >> From: Jean Pihet <j-pihet@ti.com>
> > >>
> > >> This patch set is in an RFC state, for review and comments.
> > >>
> > >> High level implementation:
> > >>
> > ...
> >
> > >> 7. Misc fixes to improve code readability:
> > >> . rename of the PM QoS implementation file from pm_qos_params.[ch] to pm_qos.[ch]
> > > I picked the name for the file as pm_qos_params over pm_qos because I
> > > wanted to make it implicitly clear that this was not an full QOS
> > > implementation. True QOS carries expectations similar to real time and
> > > as the infrastructure is closer to "good intentioned" than even "best
> > > effort" and offers no notification when the QOS request is not able to
> > > be met and really doesn't implement a true QOS at all, (it just provides
> > > the parameter interface for part of one its missing the notification
> > > interface when the service level is not met and I think a few other
> > > things.) So I wanted to have it named a bit different from just pm_qos.
> > >
> > > This said I'm not supper attached to the naming of the modules. If
> > > folks want to change it I wouldn't complain (too much anyway;).
> > Ok got the idea. I do not know what name to chose though. As suggested
> > previously the name pm_qos_params does not reflect the implementation,
> > that is why I renamed it.
>
> I must have missed the part where the name doesn't reflect the
> implementation was talked about. I look at the interface and I see
> parameters all over the place and a small bit of notification.
The interface is for specifying PM QoS requirements or constraints that
may or may not be regarded as "parameters", depending on ones definition
of the last term. Moreover, the code not only implements the interface,
but also defines data structures and API to be used throughout the kernel
which is quite a bit more than just "parameters".
In my not so humble opinion the name isn't good and the .c file should be
in kernel/power in the first place. I would call it simply qos.c (the fact
that _right_ _now_ it's not a full QoS implementation doesn't matter, because
it may become one in the future).
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 01/13] PM: QoS: rename pm_qos_params files to pm_qos
From: Rafael J. Wysocki @ 2011-07-29 21:57 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-2-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> The PM QoS implementation files are better named
> kernel/pm_qos.c and include/linux/pm_qos.h.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> arch/arm/mach-msm/clock.c | 2 +-
> drivers/acpi/processor_idle.c | 2 +-
> drivers/cpuidle/cpuidle.c | 2 +-
> drivers/cpuidle/governors/ladder.c | 2 +-
> drivers/cpuidle/governors/menu.c | 2 +-
> drivers/media/video/via-camera.c | 2 +-
> drivers/net/e1000e/netdev.c | 2 +-
> drivers/net/wireless/ipw2x00/ipw2100.c | 2 +-
> drivers/staging/msm/lcdc.c | 2 +-
> drivers/staging/msm/tvenc.c | 2 +-
> include/linux/netdevice.h | 2 +-
> include/linux/pm_qos.h | 38 +++
> include/linux/pm_qos_params.h | 38 ---
> include/sound/pcm.h | 2 +-
> kernel/Makefile | 2 +-
> kernel/pm_qos.c | 481 ++++++++++++++++++++++++++++++++
> kernel/pm_qos_params.c | 481 --------------------------------
> net/mac80211/main.c | 2 +-
> net/mac80211/mlme.c | 2 +-
> net/mac80211/scan.c | 2 +-
> sound/core/pcm_native.c | 2 +-
> 21 files changed, 536 insertions(+), 536 deletions(-)
> create mode 100644 include/linux/pm_qos.h
> delete mode 100644 include/linux/pm_qos_params.h
That I agree with.
> create mode 100644 kernel/pm_qos.c
> delete mode 100644 kernel/pm_qos_params.c
As I said, please move that file to kernel/power and call it qos.c.
That said the device interface should be located in drivers/base/power
to follow our current conventions.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 02/13] PM: add a per-device wake-up latency constraints plist
From: Rafael J. Wysocki @ 2011-07-29 21:58 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-3-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Add the field latency_constraints in the struct dev_pm_info
> and the initialization of the plist in device_pm_init.
>
> This enables the implementation of per-device constraints in
> PM QoS.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
This one looks good to me.
Thanks,
Rafael
> ---
> drivers/base/power/main.c | 1 +
> include/linux/pm.h | 2 ++
> 2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 06f09bf..dad2eb9 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -97,6 +97,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);
> + plist_head_init(&dev->power.latency_constraints, &dev->power.lock);
> }
>
> /**
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 411e4f4..23c85f1 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -22,6 +22,7 @@
> #define _LINUX_PM_H
>
> #include <linux/list.h>
> +#include <linux/plist.h>
> #include <linux/workqueue.h>
> #include <linux/spinlock.h>
> #include <linux/wait.h>
> @@ -463,6 +464,7 @@ struct dev_pm_info {
> unsigned long accounting_timestamp;
> void *subsys_data; /* Owned by the subsystem. */
> #endif
> + struct plist_head latency_constraints;
> };
>
> extern void update_pm_runtime_accounting(struct device *dev);
>
^ permalink raw reply
* Re: [PATCH 03/13] PM: QoS: extend the in-kernel API with per-device latency constraints
From: Rafael J. Wysocki @ 2011-07-29 22:55 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-4-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Extend the PM QoS kernel API:
> - add a new PM QoS class PM_QOS_DEV_LATENCY for device wake-up latency
> constraints
> - make the pm_qos_add_request API more generic by using a parameter of
> type struct pm_qos_parameters
> - minor clean-ups and rename of struct fields:
> . rename pm_qos_class to class and pm_qos_req to req in internal code
> . consistenly use req and params as the API parameters
> . rename struct pm_qos_request_list to struct pm_qos_request
> - update the in-kernel API callers to the new API
There should be more about the motivation in the changelog. It says
what the patch is doing, but it doesn't say a word of _why_ it is done in
the first place.
Second, you're renaming a lot of things in the same patch that makes
functional changes. This is confusing and makes it very difficult to
understand what's going on. Please use separate patches to rename
things, when possible, and avoid renaming things that don't need to be
renamed.
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> arch/arm/plat-omap/i2c.c | 20 -----
> drivers/i2c/busses/i2c-omap.c | 35 ++++++---
> drivers/media/video/via-camera.c | 7 +-
> drivers/net/e1000e/netdev.c | 9 ++-
> drivers/net/wireless/ipw2x00/ipw2100.c | 8 +-
> include/linux/netdevice.h | 2 +-
> include/linux/pm_qos.h | 39 ++++++----
> include/sound/pcm.h | 2 +-
> kernel/pm_qos.c | 130 +++++++++++++++++--------------
> sound/core/pcm_native.c | 8 ++-
> 10 files changed, 141 insertions(+), 119 deletions(-)
I'm going to comment the core changes this time.
...
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 9cc0224..a2e4409 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -8,31 +8,40 @@
> #include <linux/notifier.h>
> #include <linux/miscdevice.h>
>
> -#define PM_QOS_RESERVED 0
> -#define PM_QOS_CPU_DMA_LATENCY 1
> -#define PM_QOS_NETWORK_LATENCY 2
> -#define PM_QOS_NETWORK_THROUGHPUT 3
> +#define PM_QOS_RESERVED 0
> +#define PM_QOS_CPU_DMA_LATENCY 1
> +#define PM_QOS_DEV_LATENCY 2
> +#define PM_QOS_NETWORK_LATENCY 3
> +#define PM_QOS_NETWORK_THROUGHPUT 4
>
> -#define PM_QOS_NUM_CLASSES 4
> +#define PM_QOS_NUM_CLASSES 5
> #define PM_QOS_DEFAULT_VALUE -1
>
> #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> +#define PM_QOS_DEV_LAT_DEFAULT_VALUE 0
> #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 pm_qos_request {
This renaming should go in a separate patch, really.
> struct plist_node list;
> - int pm_qos_class;
> + int class;
This renaming doesn't seem to be necessary. Moreover, it's confusing,
because there already is struct class (for device classes) and a member
called "class" in struct device and they aren't related to this one.
> + struct device *dev;
> };
>
> -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,
> - s32 new_value);
> -void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
> +struct pm_qos_parameters {
> + int class;
> + struct device *dev;
> + s32 value;
> +};
What exactly is the "dev" member needed for?
> +
> +void pm_qos_add_request(struct pm_qos_request *req,
> + struct pm_qos_parameters *params);
> +void pm_qos_update_request(struct pm_qos_request *req, s32 new_value);
> +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(int class);
> +int pm_qos_add_notifier(int class, struct notifier_block *notifier);
> +int pm_qos_remove_notifier(int class, struct notifier_block *notifier);
> +int pm_qos_request_active(struct pm_qos_request *req);
>
> #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/pm_qos.c b/kernel/pm_qos.c
> index 3bf69f1..4ede3cd 100644
> --- a/kernel/pm_qos.c
> +++ b/kernel/pm_qos.c
> @@ -82,6 +82,16 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> .type = PM_QOS_MIN,
> };
>
> +static BLOCKING_NOTIFIER_HEAD(dev_lat_notifier);
> +static struct pm_qos_object dev_pm_qos = {
> + .requests = PLIST_HEAD_INIT(dev_pm_qos.requests, pm_qos_lock),
> + .notifiers = &dev_lat_notifier,
> + .name = "dev_latency",
> + .target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> + .default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> + .type = PM_QOS_MIN,
> +};
> +
You seem to be confusing things here. Since devices will have their own lists
of requests now (as per the previous patch), the .requests member above is not
necessary. Moreover, it seems to be used incorrectly below.
> 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),
> @@ -107,6 +117,7 @@ static struct pm_qos_object network_throughput_pm_qos = {
> static struct pm_qos_object *pm_qos_array[] = {
> &null_pm_qos,
> &cpu_dma_pm_qos,
> + &dev_pm_qos,
> &network_lat_pm_qos,
> &network_throughput_pm_qos
> };
> @@ -212,132 +223,132 @@ static int find_pm_qos_object_by_minor(int minor)
>
> /**
> * pm_qos_request - returns current system wide qos expectation
> - * @pm_qos_class: identification of which qos value is requested
> + * @class: identification of which qos value is requested
> *
> * This function returns the current target value.
> */
> -int pm_qos_request(int pm_qos_class)
> +int pm_qos_request(int class)
> {
> - return pm_qos_read_value(pm_qos_array[pm_qos_class]);
> + return pm_qos_read_value(pm_qos_array[class]);
> }
> EXPORT_SYMBOL_GPL(pm_qos_request);
As I said, it is completely unnecessary (and confusing) to rename
"pm_qos_class" to "class".
> -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;
> + return req->class != 0;
> }
> EXPORT_SYMBOL_GPL(pm_qos_request_active);
>
> /**
> * pm_qos_add_request - inserts new qos request into the list
> - * @dep: pointer to a preallocated handle
> - * @pm_qos_class: identifies which list of qos request to use
> - * @value: defines the qos request
> + * @req: pointer to a preallocated handle
> + * @params: request parameters
> *
> - * This function inserts a new entry in the pm_qos_class list of requested qos
> + * This function inserts a new entry in the 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 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,
> - int pm_qos_class, s32 value)
> +void pm_qos_add_request(struct pm_qos_request *req,
> + struct pm_qos_parameters *params)
> {
> - struct pm_qos_object *o = pm_qos_array[pm_qos_class];
> + struct pm_qos_object *o = pm_qos_array[params->class];
> int new_value;
>
> - if (pm_qos_request_active(dep)) {
> - WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
> + 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)
> + if (params->value == PM_QOS_DEFAULT_VALUE)
> 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);
> + new_value = params->value;
> + plist_node_init(&req->list, new_value);
> + req->class = params->class;
> + req->dev = params->dev;
> + update_target(o, &req->list, 0, PM_QOS_DEFAULT_VALUE);
This causes update_target() to use the .requests member of dev_pm_qos
as a list of requests for every device, which doesn't seem to be correct.
> }
> 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
> - * with updating the target pm_qos_class value.
> + * Updates an existing qos request for the class of parameters along
> + * with updating the target class value.
> *
> * 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,
> - s32 new_value)
> +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->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->list.prio)
> + update_target(o, &req->list, 0, temp);
Same here.
> }
> 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
> - * recompute the current target value for the pm_qos_class. Call this
> + * recompute the current target value for the 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->class];
> + update_target(o, &req->list, 1, PM_QOS_DEFAULT_VALUE);
Same here.
> + memset(req, 0, sizeof(*req));
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>
> /**
> * pm_qos_add_notifier - sets notification entry for changes to target value
> - * @pm_qos_class: identifies which qos target changes should be notified.
> + * @class: identifies which qos target changes should be notified.
> * @notifier: notifier block managed by caller.
> *
> * will register the notifier into a notification chain that gets called
> - * upon changes to the pm_qos_class target value.
> + * upon changes to the class target value.
> */
> -int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
> +int pm_qos_add_notifier(int class, struct notifier_block *notifier)
> {
> int retval;
>
> retval = blocking_notifier_chain_register(
> - pm_qos_array[pm_qos_class]->notifiers, notifier);
> + pm_qos_array[class]->notifiers, notifier);
Now, there's a question if we want to have one notifier chain for all
devices or if it's better to have a per-device notifier chain. Both
approaches have their own advantages and drawbacks, but in the latter
case this code will have to be reworked.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v4 0/3] DEVFREQ, DVFS framework for non-CPU devices
From: Turquette, Mike @ 2011-07-30 1:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
Thomas Gleixner, linux-pm
In-Reply-To: <201107291110.41801.rjw@sisk.pl>
On Fri, Jul 29, 2011 at 2:10 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, July 29, 2011, Turquette, Mike wrote:
>> On Thu, Jul 28, 2011 at 3:10 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Friday, July 15, 2011, MyungJoo Ham wrote:
>> >> For a usage example, please look at
>> >> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
>> >>
>> >> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
>> >> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
>> >> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
>> >> and other related clocks simply follow the determined DDR RAM clock.
>> >>
>> >> The DEVFREQ driver for Exynos4210 memory bus is at
>> >> /arch/arm/mach-exynos4/devfreq_bus.c in the git tree.
>> >>
>> >> MyungJoo Ham (3):
>> >> PM: Introduce DEVFREQ: generic DVFS framework with device-specific
>> >> OPPs
>> >> PM / DEVFREQ: add example governors
>> >> PM / DEVFREQ: add sysfs interface (including user tickling)
>> >
>> > OK, I'm going to take the patches for 3.2.
>>
>> Have any other platforms signed up to use this mechanism to manage
>> their peripheral DVFS?
>
> Not that I know of, but one initial user is sufficient for me.
> So if you have anything _against_ the patches, please speak up.
I do have some concerns. Let me start by saying that I'm defining a
"governor" as some active piece of executing code, probably a looping
workqueue that inspects activity/idleness of a device and then makes a
determination regarding clock frequency.
devfreq seems to be good framework for creating DVFS governors.
However I think that most scalable devices on an SoC do *not* need a
governor, and many scalable devices won't have performance counters or
any other way to implement such introspection.
Some examples include a MMC controller, which might change its clock
rate depending on the class of card that the user has inserted. Or
even a "smartish" device like a GPU lacking performance counters; it's
driver will ramp up frequency when there is work to be done and kick
off a timeout. If no new work comes in before the timeout then the
driver will drop the frequency.
A governor is not required in these cases (as they are event driven)
and devfreq is quite heavyweight for such applications. What is
needed is a QoS-style software layer that allows throughput requests
to be made from an initiator device towards a target device. This
layer should aggregate requests since many initator devices may make
requests to the same target device. This layer I'm describing, which
does not exist today, should be where the actual DVFS transition takes
place. That could take the form of a clk_set_rate call in the clock
framework (as described by Colin in V1 of this series), or some other
not-yet-realized dvfs_set_opp ,or something like Jean Pihet's
per-device PM QoS patches or whatever. For the purposes of this email
I don't really care which framework implements the QoS request
aggregation.
The point of describing this non-existant API is that devfreq should
really be just another input into it. A governor that can measure bus
saturation is really cool, but it may not yield optimal results
compared to several drivers which make QoS-style requests and insure
that performance is guaranteed for their particular needs during their
transactions. The good news is that we don't have to choose between
performance counter introspection and software QoS requests: both the
driver requests and the governor should all feed as inputs into the
QoS-style DVFS mechanism.
Taking that logic to its inevitable conclusion, tickle doesn't belong
inside the governor at all. If some device X wants to ramp up the
frequency of device Y, it should just make a QoS-style throughput
request towards device Y, possibly with a timeout (keeping the
original idea of tickle intact). This is entirely a separate idea
from a governor's introspective workqueue loop.
For userspace, a sysfs entry for tickle would also not feed into the
governor, but some dummy struct device *user would probably be the
initiator device and it would simply call the QoS-style throughput
API.
In summary my objections to this series are:
1) devfreq should not be the *final* software layer to invoke a DVFS
transition as it has not taken all constraints into account.
2) a devfreq governor represents just one constraint out of many to be
considered for any given scalable device.
My objection to these patches getting merged is that I think they are
a bit ahead of their time. We need to know what the real DVFS API
looks like underneath devfreq first, since devfreq should really be
built on top of it.
Regards,
Mike
> Thanks,
> Rafael
>
^ permalink raw reply
* [PATCH -next] x86/platform: mrst/pmu.c needs module.h
From: Randy Dunlap @ 2011-07-30 4:14 UTC (permalink / raw)
To: Stephen Rothwell, Len Brown, linux-pm
Cc: platform-driver-x86, akpm, linux-next, LKML, Matthew Garrett
In-Reply-To: <20110729173827.716f57ff6e00095a3efc6449@canb.auug.org.au>
From: Randy Dunlap <rdunlap@xenotime.net>
mrst/pmu.c uses interfaces from linux/module.h, so it should
include that file. This fixes build errors.
Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
arch/x86/platform/mrst/pmu.c | 1 +
1 file changed, 1 insertion(+)
--- linux-next-20110729.orig/arch/x86/platform/mrst/pmu.c
+++ linux-next-20110729/arch/x86/platform/mrst/pmu.c
@@ -21,6 +21,7 @@
#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
+#include <linux/module.h>
#include <linux/pci.h>
#include <linux/seq_file.h>
#include <linux/sfi.h>
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Pavel Machek @ 2011-07-30 20:37 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <201107292152.27066.rjw@sisk.pl>
Hi!
> > Well, auto suspending when screensaver is active would still be
> > useful.
> >
> > (And IIRC some machines kept screen on when in S-state unless driver
> > powered it down... but that might be S1.
> >
> > > The reason why you can't enter ACPI S-states from CPUidle is because you
> > > need to go out of the idle loop to execute some ACPI-specific stuff. Which
> > > is not even specific to Intel chips, but to ACPI in general.
> >
> > The code was little tricky/unclean, but it "worked" for me at one
> > point... I called it "sleepy linux".
>
> Yes, you can find a system where it might kind of work (just because
> _PTS is empty or something like this). Is it going to work in general?
> No way.
IIRC I solved it by just calling _PTS when sleepy Linux was
enabled. It had side effect of lighting up moon icon, but otherwise
seemed to work ok.
I do not think ACPI says what can and can not be done after _PTS...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Rafael J. Wysocki @ 2011-07-30 21:05 UTC (permalink / raw)
To: Pavel Machek; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <20110730203737.GA8762@elf.ucw.cz>
On Saturday, July 30, 2011, Pavel Machek wrote:
> Hi!
>
> > > Well, auto suspending when screensaver is active would still be
> > > useful.
> > >
> > > (And IIRC some machines kept screen on when in S-state unless driver
> > > powered it down... but that might be S1.
> > >
> > > > The reason why you can't enter ACPI S-states from CPUidle is because you
> > > > need to go out of the idle loop to execute some ACPI-specific stuff. Which
> > > > is not even specific to Intel chips, but to ACPI in general.
> > >
> > > The code was little tricky/unclean, but it "worked" for me at one
> > > point... I called it "sleepy linux".
> >
> > Yes, you can find a system where it might kind of work (just because
> > _PTS is empty or something like this). Is it going to work in general?
> > No way.
>
> IIRC I solved it by just calling _PTS when sleepy Linux was
> enabled. It had side effect of lighting up moon icon, but otherwise
> seemed to work ok.
>
> I do not think ACPI says what can and can not be done after _PTS...
Yes, it does.
Thanks,
Rafael
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Rafael J. Wysocki @ 2011-07-30 21:09 UTC (permalink / raw)
To: Pavel Machek; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <201107302305.00212.rjw@sisk.pl>
On Saturday, July 30, 2011, Rafael J. Wysocki wrote:
> On Saturday, July 30, 2011, Pavel Machek wrote:
> > Hi!
> >
> > > > Well, auto suspending when screensaver is active would still be
> > > > useful.
> > > >
> > > > (And IIRC some machines kept screen on when in S-state unless driver
> > > > powered it down... but that might be S1.
> > > >
> > > > > The reason why you can't enter ACPI S-states from CPUidle is because you
> > > > > need to go out of the idle loop to execute some ACPI-specific stuff. Which
> > > > > is not even specific to Intel chips, but to ACPI in general.
> > > >
> > > > The code was little tricky/unclean, but it "worked" for me at one
> > > > point... I called it "sleepy linux".
> > >
> > > Yes, you can find a system where it might kind of work (just because
> > > _PTS is empty or something like this). Is it going to work in general?
> > > No way.
> >
> > IIRC I solved it by just calling _PTS when sleepy Linux was
> > enabled. It had side effect of lighting up moon icon, but otherwise
> > seemed to work ok.
> >
> > I do not think ACPI says what can and can not be done after _PTS...
>
> Yes, it does.
And even if _PTS will work, you're certainly not supposed to execute _WAK
with interrupts off.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v4 0/3] DEVFREQ, DVFS framework for non-CPU devices
From: Rafael J. Wysocki @ 2011-07-30 21:23 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
Thomas Gleixner, linux-pm
In-Reply-To: <CAJOA=zPL_DUBb2miytFH6s3AKqroGDBsOPts=SdCb3p0c=cP=A@mail.gmail.com>
On Saturday, July 30, 2011, Turquette, Mike wrote:
> On Fri, Jul 29, 2011 at 2:10 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, July 29, 2011, Turquette, Mike wrote:
> >> On Thu, Jul 28, 2011 at 3:10 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Friday, July 15, 2011, MyungJoo Ham wrote:
> >> >> For a usage example, please look at
> >> >> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
> >> >>
> >> >> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
> >> >> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
> >> >> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
> >> >> and other related clocks simply follow the determined DDR RAM clock.
> >> >>
> >> >> The DEVFREQ driver for Exynos4210 memory bus is at
> >> >> /arch/arm/mach-exynos4/devfreq_bus.c in the git tree.
> >> >>
> >> >> MyungJoo Ham (3):
> >> >> PM: Introduce DEVFREQ: generic DVFS framework with device-specific
> >> >> OPPs
> >> >> PM / DEVFREQ: add example governors
> >> >> PM / DEVFREQ: add sysfs interface (including user tickling)
> >> >
> >> > OK, I'm going to take the patches for 3.2.
> >>
> >> Have any other platforms signed up to use this mechanism to manage
> >> their peripheral DVFS?
> >
> > Not that I know of, but one initial user is sufficient for me.
> > So if you have anything _against_ the patches, please speak up.
>
> I do have some concerns. Let me start by saying that I'm defining a
> "governor" as some active piece of executing code, probably a looping
> workqueue that inspects activity/idleness of a device and then makes a
> determination regarding clock frequency.
>
> devfreq seems to be good framework for creating DVFS governors.
> However I think that most scalable devices on an SoC do *not* need a
> governor, and many scalable devices won't have performance counters or
> any other way to implement such introspection.
OK, so I'd like to see what the author of the patch series has to say
in the face of your comments below.
> Some examples include a MMC controller, which might change its clock
> rate depending on the class of card that the user has inserted. Or
> even a "smartish" device like a GPU lacking performance counters; it's
> driver will ramp up frequency when there is work to be done and kick
> off a timeout. If no new work comes in before the timeout then the
> driver will drop the frequency.
>
> A governor is not required in these cases (as they are event driven)
> and devfreq is quite heavyweight for such applications. What is
> needed is a QoS-style software layer that allows throughput requests
> to be made from an initiator device towards a target device. This
> layer should aggregate requests since many initator devices may make
> requests to the same target device. This layer I'm describing, which
> does not exist today, should be where the actual DVFS transition takes
> place. That could take the form of a clk_set_rate call in the clock
> framework (as described by Colin in V1 of this series), or some other
> not-yet-realized dvfs_set_opp ,or something like Jean Pihet's
> per-device PM QoS patches or whatever. For the purposes of this email
> I don't really care which framework implements the QoS request
> aggregation.
>
> The point of describing this non-existant API is that devfreq should
> really be just another input into it. A governor that can measure bus
> saturation is really cool, but it may not yield optimal results
> compared to several drivers which make QoS-style requests and insure
> that performance is guaranteed for their particular needs during their
> transactions. The good news is that we don't have to choose between
> performance counter introspection and software QoS requests: both the
> driver requests and the governor should all feed as inputs into the
> QoS-style DVFS mechanism.
>
> Taking that logic to its inevitable conclusion, tickle doesn't belong
> inside the governor at all. If some device X wants to ramp up the
> frequency of device Y, it should just make a QoS-style throughput
> request towards device Y, possibly with a timeout (keeping the
> original idea of tickle intact). This is entirely a separate idea
> from a governor's introspective workqueue loop.
>
> For userspace, a sysfs entry for tickle would also not feed into the
> governor, but some dummy struct device *user would probably be the
> initiator device and it would simply call the QoS-style throughput
> API.
>
> In summary my objections to this series are:
> 1) devfreq should not be the *final* software layer to invoke a DVFS
> transition as it has not taken all constraints into account.
> 2) a devfreq governor represents just one constraint out of many to be
> considered for any given scalable device.
>
> My objection to these patches getting merged is that I think they are
> a bit ahead of their time.
Still, we're merging quite a number of patches being ahead of their time.
The resulting code is then modified as we learn what's wrong with it or
how to improve it.
Why exactly do you think this approach will not work in this particular case?
> We need to know what the real DVFS API looks like underneath devfreq first,
> since devfreq should really be built on top of it.
Well, quite frankly, if we generally adopted that point of view, much of the
useful functionality we have in the kernel right now wouldn't be merged at all.
Think of the USB subsystem, for one example, that has been rewritten from
scratch no fewer that three times.
The code appears to be reasonably isolated and simple enough to merge.
There is a subsystem wanting to use it and I don't see anyone forcing
anybody else to adopt it. If it's not suitable to you, you won't be using it,
plain and simple. And if you come up with some better code to replace it,
I won't have any problems with taking that too.
Thanks,
Rafael
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Pavel Machek @ 2011-07-30 21:25 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <201107302309.19169.rjw@sisk.pl>
> > > IIRC I solved it by just calling _PTS when sleepy Linux was
> > > enabled. It had side effect of lighting up moon icon, but otherwise
> > > seemed to work ok.
> > >
> > > I do not think ACPI says what can and can not be done after _PTS...
> >
> > Yes, it does.
>
> And even if _PTS will work, you're certainly not supposed to execute _WAK
> with interrupts off.
It should be ok to execute _WAK in process context once system is
resumed. ... or maybe not executing _WAK at all. User can probably
live with moon icon lighted up.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: where is /sys/class/power_supply/ADP1 ?
From: Rafael J. Wysocki @ 2011-07-30 21:28 UTC (permalink / raw)
To: J. Bakshi; +Cc: Linux PM mailing list, linux-kernel
In-Reply-To: <201107301330.p6UDUrt8004286@dcnode-01.unlimitedmail.net>
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.
Thanks,
Rafael
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Rafael J. Wysocki @ 2011-07-30 21:36 UTC (permalink / raw)
To: Pavel Machek; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <20110730212523.GA10450@elf.ucw.cz>
On Saturday, July 30, 2011, Pavel Machek wrote:
>
> > > > IIRC I solved it by just calling _PTS when sleepy Linux was
> > > > enabled. It had side effect of lighting up moon icon, but otherwise
> > > > seemed to work ok.
> > > >
> > > > I do not think ACPI says what can and can not be done after _PTS...
> > >
> > > Yes, it does.
> >
> > And even if _PTS will work, you're certainly not supposed to execute _WAK
> > with interrupts off.
>
> It should be ok to execute _WAK in process context once system is
> resumed. ... or maybe not executing _WAK at all. User can probably
> live with moon icon lighted up.
But I don't think the user can live with certain functionality missing,
like battery status or such things.
This is completely unrealistic to think that your prototype can be safely
implemented on _every_ ACPI-based system. Period.
And that's even without taking SMP into account, BTW.
Thanks,
Rafael
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Pavel Machek @ 2011-07-30 22:09 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <201107302305.00212.rjw@sisk.pl>
Hi!
> > IIRC I solved it by just calling _PTS when sleepy Linux was
> > enabled. It had side effect of lighting up moon icon, but otherwise
> > seemed to work ok.
> >
> > I do not think ACPI says what can and can not be done after _PTS...
>
> Yes, it does.
I have 2.0 spec here; it explains how _PTS can be called long time
before actual sleep and may not power down devices...
> > It should be ok to execute _WAK in process context once system is
> > resumed. ... or maybe not executing _WAK at all. User can probably
> > live with moon icon lighted up.
>
> But I don't think the user can live with certain functionality missing,
> like battery status or such things.
Yep; you'd probably want to execute it before calling other ACPI methods...
> This is completely unrealistic to think that your prototype can be safely
> implemented on _every_ ACPI-based system. Period.
My prototype would probably not be safe on any system, period. It was
quick hack to prove the concept.
> And that's even without taking SMP into account, BTW.
Idea was to do hot CPU unplug before enabling sleepy state. Sleepy
probably should be enabled when screen is turned off by
screensaver (if machine is sufficiently idle).
I'm not saying that task is easy, merely that is doable. (Doing it in
clean way will be harder still). Basically prepare all the devices to
sleep when machine gets "idle enough", along with _PTS, but keep
running, including userspace. Then, if next wake up is "long enough"
into future, set up RTC alarm and push machine to sleep.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH 04/13] PM: QoS: implement the per-device latency constraints
From: Rafael J. Wysocki @ 2011-07-30 22:30 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-5-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> Re-design the PM QoS implementation to support the per-device
> constraints:
Well, I guess I should have reviewed this patch before [03/13].
> - Define a pm_qos_constraints struct for the storage of the constraints
> list and associated values (target_value, default_value, type ...).
>
> - Update the pm_qos_object struct with the information related to a
> PM QoS class: ptr to constraints list, notifer ptr, name ...
>
> - Each PM QoS class statically declare objects for pm_qos_object and
> pm_qos_constraints. The only exception is the devices constraints, cf. below.
>
> - The device constraints class is statically declaring a pm_qos_object. The
> pm_qos_constraints are per-device and so are embedded into the device struct.
>
> - The PM QoS internal functions are updated to operate on the pm_qos_constraints
> structs for the constraints management, and on pm_qos_object for other the PM QoS
> functionality (notifiers, export as misc devices...).
>
> - The PM QoS events notification callbacks are passing the full constraint
> request data in order for the callees to have access to it. The current use
> is for the platform low-level code to access the target device of the constraint
>
> - User space API: the PM QoS classes -excepted PM_QOS_DEV_LATENCY- are exporting
> a MISC entry in /dev. PM_QOS_DEV_LATENCY shall export a per-device sysfs entry, this
> support is coming as a subsequent patch.
>
> - Misc fixes to improve code readability:
> . rename of fields names (request, list, constraints, class),
Please avoid doing renames along with functional changes. It makes reviewing
extremely hard.
> . simplification of the in-kernel API implementation. The main logic part is
> now implemented in the update_target function.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> drivers/base/power/main.c | 7 +-
> include/linux/pm.h | 3 +-
> include/linux/pm_qos.h | 23 ++++-
> kernel/pm_qos.c | 248 +++++++++++++++++++++++++-------------------
> 4 files changed, 170 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index dad2eb9..360c2c0 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -97,7 +97,12 @@ void device_pm_add(struct device *dev)
> dev_name(dev->parent));
> list_add_tail(&dev->power.entry, &dpm_list);
> mutex_unlock(&dpm_list_mtx);
> - plist_head_init(&dev->power.latency_constraints, &dev->power.lock);
> + plist_head_init(&dev->power.latency_constraints.list, &dev->power.lock);
> + dev->power.latency_constraints.target_value =
> + PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + dev->power.latency_constraints.default_value =
> + PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + dev->power.latency_constraints.type = PM_QOS_MIN;
Perhaps add a helper doing these assignments?
> }
>
> /**
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 23c85f1..35e48a3 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -28,6 +28,7 @@
> #include <linux/wait.h>
> #include <linux/timer.h>
> #include <linux/completion.h>
> +#include <linux/pm_qos.h>
>
> /*
> * Callbacks for platform drivers to implement.
> @@ -464,7 +465,7 @@ struct dev_pm_info {
> unsigned long accounting_timestamp;
> void *subsys_data; /* Owned by the subsystem. */
> #endif
> - struct plist_head latency_constraints;
> + struct pm_qos_constraints latency_constraints;
Why don't you simply call it "qos"? The data type provides the information
about what it's for now.
> };
>
> extern void update_pm_runtime_accounting(struct device *dev);
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index a2e4409..d72b16b 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -6,7 +6,6 @@
> */
> #include <linux/plist.h>
> #include <linux/notifier.h>
> -#include <linux/miscdevice.h>
>
> #define PM_QOS_RESERVED 0
> #define PM_QOS_CPU_DMA_LATENCY 1
> @@ -22,8 +21,28 @@
> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
>
> +enum pm_qos_type {
> + PM_QOS_MAX, /* return the largest value */
> + PM_QOS_MIN /* return the smallest value */
> +};
> +
> +struct pm_qos_constraints {
> + struct plist_head list;
> + /*
> + * Do not change target_value to 64 bit in order to guarantee
> + * accesses atomicity
> + */
The comment doesn't belong here. Please put it outside of the structure
definition or after the field name (or both, in which case the "inline"
one may be shorter, like "must not be 64-bit").
> + s32 target_value;
> + s32 default_value;
> + enum pm_qos_type type;
> +};
> +
> +/*
> + * Struct that is pre-allocated by the caller.
> + * The handle is kept for future use (update, removal)
> + */
> struct pm_qos_request {
> - struct plist_node list;
> + struct plist_node node;
Please avoid doing such things along with functional changes.
> int class;
> struct device *dev;
> };
> diff --git a/kernel/pm_qos.c b/kernel/pm_qos.c
> index 4ede3cd..7edc6d0 100644
> --- a/kernel/pm_qos.c
> +++ b/kernel/pm_qos.c
> @@ -49,71 +49,81 @@
> * 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 */
> +
> +enum pm_qos_req_action {
> + PM_QOS_ADD_REQ,
> + PM_QOS_UPDATE_REQ,
> + PM_QOS_REMOVE_REQ
> };
A comment describing the meaning of these would be helpful.
> -/*
> - * 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 requests;
> + struct pm_qos_constraints *constraints;
> struct blocking_notifier_head *notifiers;
> 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;
> +
> +/* CPU/DMA latency constraints PM QoS object */
> +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,
> +};
> 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 = &cpu_dma_constraints,
> .notifiers = &cpu_dma_lat_notifier,
> .name = "cpu_dma_latency",
> - .target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
> - .default_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
> - .type = PM_QOS_MIN,
> };
>
> +/*
> + * Per-device latency constraints PM QoS object
> + *
> + * The constraints are stored in the device struct data.
> + * No misc device is exported to /dev, instead the user space API
> + * shall use a per-device /sysfs entry.
> + */
> static BLOCKING_NOTIFIER_HEAD(dev_lat_notifier);
> static struct pm_qos_object dev_pm_qos = {
> - .requests = PLIST_HEAD_INIT(dev_pm_qos.requests, pm_qos_lock),
> + .constraints = NULL,
> .notifiers = &dev_lat_notifier,
> + .pm_qos_power_miscdev = { .minor = -1 },
> .name = "dev_latency",
> - .target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> - .default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> - .type = PM_QOS_MIN,
> };
>
> +/* Network latency constraints PM QoS object */
> +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
> +};
> 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 = &network_lat_constraints,
> .notifiers = &network_lat_notifier,
> .name = "network_latency",
> - .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .type = PM_QOS_MIN
> };
>
> -
> +/* Network throughput constraints PM QoS object */
> +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,
> +};
> 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 = &network_tput_constraints,
> .notifiers = &network_throughput_notifier,
> .name = "network_throughput",
> - .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> - .default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> - .type = PM_QOS_MAX,
> };
>
> -
> static struct pm_qos_object *pm_qos_array[] = {
> &null_pm_qos,
> &cpu_dma_pm_qos,
> @@ -138,17 +148,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)
I'd remove the "inline" if you're at it. It's only a hint if the kernel
is not built with "always inline" and the compiler should do the inlining
anyway if that's a better choice.
> {
> - if (plist_head_empty(&o->requests))
> - return o->default_value;
> + if (plist_head_empty(&c->list))
> + return c->default_value;
>
> - switch (o->type) {
> + switch (c->type) {
> case PM_QOS_MIN:
> - return plist_first(&o->requests)->prio;
> + return plist_first(&c->list)->prio;
>
> case PM_QOS_MAX:
> - return plist_last(&o->requests)->prio;
> + return plist_last(&c->list)->prio;
>
> default:
> /* runtime check for not using enum */
> @@ -156,69 +166,92 @@ static inline int pm_qos_get_value(struct pm_qos_object *o)
> }
> }
>
> -static inline s32 pm_qos_read_value(struct pm_qos_object *o)
> +/*
> + * pm_qos_read_value atomically reads and returns target_value.
We have a standard for writing kerneldoc comments, please follow it.
> + * target_value is updated upon update of the constraints list, using
> + * pm_qos_set_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
You should say "data types" rather than "quantities" here.
> + */
> +static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
> {
> - return o->target_value;
> + if (c)
> + return c->target_value;
> + else
> + return 0;
> }
>
> -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->target_value = value;
> + c->target_value = value;
> }
Well, I'm not sure that this function is necessary at all. You might as well
simply remove it as far as I'm concerned.
> -static void update_target(struct pm_qos_object *o, struct plist_node *node,
> - int del, int value)
> +static void update_target(struct pm_qos_request *req,
> + enum pm_qos_req_action action, int value)
> {
> unsigned long flags;
> - int prev_value, curr_value;
> + int prev_value, curr_value, new_value;
> + struct pm_qos_object *o = pm_qos_array[req->class];
> + struct pm_qos_constraints *c;
> +
> + switch (req->class) {
> + case PM_QOS_DEV_LATENCY:
> + if (!req->dev) {
> + WARN(1, KERN_ERR "PM QoS API called with NULL dev\n");
> + return;
> + }
> + c = &req->dev->power.latency_constraints;
> + break;
> + case PM_QOS_CPU_DMA_LATENCY:
> + case PM_QOS_NETWORK_LATENCY:
> + case PM_QOS_NETWORK_THROUGHPUT:
> + c = o->constraints;
> + break;
> + case PM_QOS_RESERVED:
> + default:
> + WARN(1, KERN_ERR "PM QoS API called with wrong class %d, "
> + "req 0x%p\n", req->class, req);
> + return;
> + }
Do we _really_ need that switch()?
What about introducing dev_pm_qos_add_request() and friends specifically
for devices, such that they will take the target device object (dev) as
their first argument? Then, you could keep pm_qos_add_request() pretty
much as is, right?
>
> 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;
What about writing that as
new_value = value != PM_QOS_DEFAULT_VALUE ? value : c->default_value;
> +
> + switch (action) {
> + case PM_QOS_REMOVE_REQ:
> + plist_del(&req->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->requests);
> - plist_node_init(node, value);
> - plist_add(node, &o->requests);
> - } else if (del) {
> - plist_del(node, &o->requests);
> - } else {
> - plist_add(node, &o->requests);
> + plist_del(&req->node, &c->list);
> + case PM_QOS_ADD_REQ:
> + plist_node_init(&req->node, new_value);
> + plist_add(&req->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->notifiers,
That's why I'm thinking that it would be helpful to have a pointer
to the notifier list from struct pm_qos_constraints .
Besides, you can use "pm_qos_array[req->class]->notifiers" instead of
"o->notifiers".
> (unsigned long)curr_value,
> - 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;
> + req);
> }
>
> /**
> @@ -229,7 +262,7 @@ static int find_pm_qos_object_by_minor(int minor)
> */
> int pm_qos_request(int class)
> {
> - return pm_qos_read_value(pm_qos_array[class]);
> + return pm_qos_read_value(pm_qos_array[class]->constraints);
> }
> EXPORT_SYMBOL_GPL(pm_qos_request);
>
> @@ -254,22 +287,15 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active);
> void pm_qos_add_request(struct pm_qos_request *req,
> struct pm_qos_parameters *params)
> {
> - struct pm_qos_object *o = pm_qos_array[params->class];
> - int new_value;
> -
> if (pm_qos_request_active(req)) {
> WARN(1, KERN_ERR "pm_qos_add_request() called for already "
> "added request\n");
> return;
> }
> - if (params->value == PM_QOS_DEFAULT_VALUE)
> - new_value = o->default_value;
> - else
> - new_value = params->value;
> - plist_node_init(&req->list, new_value);
> +
> req->class = params->class;
> req->dev = params->dev;
> - update_target(o, &req->list, 0, PM_QOS_DEFAULT_VALUE);
> + update_target(req, PM_QOS_ADD_REQ, params->value);
> }
> EXPORT_SYMBOL_GPL(pm_qos_add_request);
>
> @@ -285,9 +311,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;
>
> @@ -296,15 +319,8 @@ void pm_qos_update_request(struct pm_qos_request *req, s32 new_value)
> return;
> }
>
> - o = pm_qos_array[req->class];
> -
> - if (new_value == PM_QOS_DEFAULT_VALUE)
> - temp = o->default_value;
> - else
> - temp = new_value;
> -
> - if (temp != req->list.prio)
> - update_target(o, &req->list, 0, temp);
> + if (new_value != req->node.prio)
> + update_target(req, PM_QOS_UPDATE_REQ, new_value);
> }
> EXPORT_SYMBOL_GPL(pm_qos_update_request);
>
> @@ -318,8 +334,6 @@ 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)
> return;
> /* silent return to keep pcm code cleaner */
> @@ -329,8 +343,8 @@ void pm_qos_remove_request(struct pm_qos_request *req)
> return;
> }
>
> - o = pm_qos_array[req->class];
> - update_target(o, &req->list, 1, PM_QOS_DEFAULT_VALUE);
> + update_target(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> +
> memset(req, 0, sizeof(*req));
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_request);
> @@ -373,6 +387,28 @@ int pm_qos_remove_notifier(int class, struct notifier_block *notifier)
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>
> +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;
> +}
This function doesn't seem to be used anywhere, what's the purpose of it?
> +
> static int pm_qos_power_open(struct inode *inode, struct file *filp)
> {
> struct pm_qos_parameters pm_qos_params;
> @@ -410,7 +446,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)
> @@ -418,9 +453,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->class];
> spin_lock_irqsave(&pm_qos_lock, flags);
> - value = pm_qos_get_value(o);
> + value = pm_qos_get_value(pm_qos_array[req->class]->constraints);
> spin_unlock_irqrestore(&pm_qos_lock, flags);
>
> return simple_read_from_buffer(buf, count, f_pos, &value, sizeof(s32));
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 05/13] PM: QoS: support the dynamic insertion and removal of devices
From: Rafael J. Wysocki @ 2011-07-30 22:38 UTC (permalink / raw)
To: jean.pihet
Cc: markgross, broonie, Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <1311841821-10252-6-git-send-email-j-pihet@ti.com>
On Thursday, July 28, 2011, jean.pihet@newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> The devices latency constraints class of PM QoS is storing the
> constraints list in the device dev_pm_info struct.
>
> This patch adds the init and de-init of the per-device constraints
> list in order to support the dynamic insertion and removal
> of the devices in the system.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> drivers/base/power/main.c | 10 ++++------
> include/linux/pm.h | 1 +
> include/linux/pm_qos.h | 2 ++
> kernel/pm_qos.c | 30 ++++++++++++++++++++++++++++++
> 4 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 360c2c0..c86f97c 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -97,12 +97,8 @@ void device_pm_add(struct device *dev)
> dev_name(dev->parent));
> list_add_tail(&dev->power.entry, &dpm_list);
> mutex_unlock(&dpm_list_mtx);
> - plist_head_init(&dev->power.latency_constraints.list, &dev->power.lock);
> - dev->power.latency_constraints.target_value =
> - PM_QOS_DEV_LAT_DEFAULT_VALUE;
> - dev->power.latency_constraints.default_value =
> - PM_QOS_DEV_LAT_DEFAULT_VALUE;
> - dev->power.latency_constraints.type = PM_QOS_MIN;
> + /* Call PM QoS to init the per-device latency constraints */
> + pm_qos_dev_constraints_init(dev);
> }
>
> /**
> @@ -113,6 +109,8 @@ 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));
> + /* Call PM QoS to de-init the per-device latency constraints */
> + pm_qos_dev_constraints_deinit(dev);
I'd call this function "dev_pm_qos_constraints_destroy()" (and the previous
one "dev_pm_qos_constraints_init()" for consistency).
> complete_all(&dev->power.completion);
> mutex_lock(&dpm_list_mtx);
> list_del_init(&dev->power.entry);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 35e48a3..3ed53be 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -466,6 +466,7 @@ struct dev_pm_info {
> void *subsys_data; /* Owned by the subsystem. */
> #endif
> struct pm_qos_constraints latency_constraints;
> + int latency_constraints_init;
> };
>
> extern void update_pm_runtime_accounting(struct device *dev);
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index d72b16b..4d36537 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -63,4 +63,6 @@ int pm_qos_add_notifier(int class, struct notifier_block *notifier);
> int pm_qos_remove_notifier(int class, struct notifier_block *notifier);
> int pm_qos_request_active(struct pm_qos_request *req);
>
> +void pm_qos_dev_constraints_init(struct device *dev);
> +void pm_qos_dev_constraints_deinit(struct device *dev);
> #endif
> diff --git a/kernel/pm_qos.c b/kernel/pm_qos.c
> index 7edc6d0..361fc3f 100644
> --- a/kernel/pm_qos.c
> +++ b/kernel/pm_qos.c
> @@ -202,6 +202,9 @@ static void update_target(struct pm_qos_request *req,
> WARN(1, KERN_ERR "PM QoS API called with NULL dev\n");
> return;
> }
> + /* Silently return if the device is being released */
> + if (!req->dev->power.latency_constraints_init)
> + return;
> c = &req->dev->power.latency_constraints;
> break;
> case PM_QOS_CPU_DMA_LATENCY:
> @@ -387,6 +390,33 @@ int pm_qos_remove_notifier(int class, struct notifier_block *notifier)
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>
> +/* Called from the device PM subsystem at device init */
> +void pm_qos_dev_constraints_init(struct device *dev)
> +{
> + plist_head_init(&dev->power.latency_constraints.list, &dev->power.lock);
> + dev->power.latency_constraints.target_value =
> + PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + dev->power.latency_constraints.default_value =
> + PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + dev->power.latency_constraints.type = PM_QOS_MIN;
> + dev->power.latency_constraints_init = 1;
You could avoid adding this field if there were a PM_QOS_UNINITIALIZED
(or PM_QOS_UNKNOWN) type.
And if you _really_ want to have a separate field, why don't you put it
into latency_constraints ?
> +}
> +
> +/* Called from the device PM subsystem at device release */
> +void pm_qos_dev_constraints_deinit(struct device *dev)
> +{
> + struct pm_qos_request *req, *tmp;
> +
> + dev->power.latency_constraints_init = 0;
> +
> + /* Flush the constraints list for the device */
> + plist_for_each_entry_safe(req, tmp,
> + &dev->power.latency_constraints.list,
> + node)
> + update_target(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> + plist_head_init(&dev->power.latency_constraints.list, &dev->power.lock);
> +}
> +
> static int register_pm_qos_misc(struct pm_qos_object *qos)
> {
> qos->pm_qos_power_miscdev.minor = MISC_DYNAMIC_MINOR;
>
Thanks,
Rafael
^ permalink raw reply
* Re: Runtime PM discussion notes
From: Rafael J. Wysocki @ 2011-07-30 22:41 UTC (permalink / raw)
To: Pavel Machek; +Cc: Mark Brown, linux-pm, linux-omap
In-Reply-To: <20110730220953.GB10450@elf.ucw.cz>
On Sunday, July 31, 2011, Pavel Machek wrote:
> Hi!
>
> > > IIRC I solved it by just calling _PTS when sleepy Linux was
> > > enabled. It had side effect of lighting up moon icon, but otherwise
> > > seemed to work ok.
> > >
> > > I do not think ACPI says what can and can not be done after _PTS...
> >
> > Yes, it does.
>
> I have 2.0 spec here; it explains how _PTS can be called long time
> before actual sleep and may not power down devices...
>
> > > It should be ok to execute _WAK in process context once system is
> > > resumed. ... or maybe not executing _WAK at all. User can probably
> > > live with moon icon lighted up.
> >
> > But I don't think the user can live with certain functionality missing,
> > like battery status or such things.
>
> Yep; you'd probably want to execute it before calling other ACPI methods...
>
> > This is completely unrealistic to think that your prototype can be safely
> > implemented on _every_ ACPI-based system. Period.
>
> My prototype would probably not be safe on any system, period. It was
> quick hack to prove the concept.
>
> > And that's even without taking SMP into account, BTW.
>
> Idea was to do hot CPU unplug before enabling sleepy state. Sleepy
> probably should be enabled when screen is turned off by
> screensaver (if machine is sufficiently idle).
>
> I'm not saying that task is easy, merely that is doable. (Doing it in
> clean way will be harder still). Basically prepare all the devices to
> sleep when machine gets "idle enough", along with _PTS, but keep
> running, including userspace. Then, if next wake up is "long enough"
> into future, set up RTC alarm and push machine to sleep.
You're supposed to do all that from within the idle loop on one
of the CPUs (at least in the context of this discussion). I don't
really think it's realistically doable.
Thanks,
Rafael
^ permalink raw reply
* [git pull] cpupowerutils
From: Dominik Brodowski @ 2011-07-31 8:51 UTC (permalink / raw)
To: torvalds, akpm; +Cc: Ingo Molnar, linux-kernel, cpufreq, linux-acpi, linux-pm
Linus,
the git tree
git://git.kernel.org/pub/scm/linux/kernel/git/brodo/cpupowerutils.git master
contains a new utility called "cpupowerutils" which is laregely based on
the well known "cpufrequtils", but extended to provide much more information
about other power-related features of modern CPUs, such as idle states.
Users and Developers want to have *one* tool to get an overview what
their system supports and to monitor and debug CPU power management
in detail. The tool should compile and work on as many architectures
as possible.
To reach these goals, Thomas Renninger suggested -- and implemented most
of the parts -- to convert the external tool cpufrequtils to a userspace
tool residing in tools/power/cpupower/ .
Once this stabilizes, it is intended to replace cpufrequtils and the
Intel-specific tools in tools/power/x86 .
A list of patches and the diffstat are appended to this messages.
Best,
Dominik
Dominik Brodowski (10):
cpupowerutils - cpufrequtils extended with quite some features
cpupowerutils: use COPYING, CREDITS from top-level directory
cpupowerutils: remove ccdv, use kernel quiet/verbose mechanism
cpupowerutils: do not update po files on each and every compile
cpupowerutils: bench - ConfigStyle bugfixes
cpupowerutils: lib - ConfigStyle bugfixes
cpupowerutils: idle_monitor - ConfigStyle bugfixes
cpupowerutils: helpers - ConfigStyle bugfixes
cpupowerutils: utils - ConfigStyle bugfixes
cpupowerutils: use kernel version-derived version string
Roman Vasiyarov (1):
cpupowerutils: increase MAX_LINE_LEN
Thomas Renninger (4):
cpupowerutils: Rename: libcpufreq->libcpupower
cpupower: Rename package from cpupowerutils to cpupower
cpupower: Show Intel turbo ratio support via ./cpupower frequency-info
cpupower: Do detect IDA (opportunistic processor performance) via cpuid
CREDITS | 17 +-
MAINTAINERS | 6 +
tools/power/cpupower/.gitignore | 22 +
tools/power/cpupower/Makefile | 279 ++++++
tools/power/cpupower/README | 49 +
tools/power/cpupower/ToDo | 11 +
tools/power/cpupower/bench/Makefile | 29 +
tools/power/cpupower/bench/README-BENCH | 124 +++
tools/power/cpupower/bench/benchmark.c | 194 ++++
tools/power/cpupower/bench/benchmark.h | 29 +
tools/power/cpupower/bench/config.h | 36 +
tools/power/cpupower/bench/cpufreq-bench_plot.sh | 104 +++
tools/power/cpupower/bench/cpufreq-bench_script.sh | 101 ++
tools/power/cpupower/bench/example.cfg | 11 +
tools/power/cpupower/bench/main.c | 202 ++++
tools/power/cpupower/bench/parse.c | 225 +++++
tools/power/cpupower/bench/parse.h | 53 ++
tools/power/cpupower/bench/system.c | 191 ++++
tools/power/cpupower/bench/system.h | 29 +
tools/power/cpupower/debug/i386/Makefile | 20 +
tools/power/cpupower/debug/i386/centrino-decode.c | 113 +++
tools/power/cpupower/debug/i386/dump_psb.c | 196 ++++
tools/power/cpupower/debug/i386/intel_gsic.c | 78 ++
.../power/cpupower/debug/i386/powernow-k8-decode.c | 96 ++
tools/power/cpupower/debug/kernel/Makefile | 23 +
.../power/cpupower/debug/kernel/cpufreq-test_tsc.c | 113 +++
tools/power/cpupower/debug/x86_64/Makefile | 14 +
.../power/cpupower/debug/x86_64/centrino-decode.c | 1 +
.../cpupower/debug/x86_64/powernow-k8-decode.c | 1 +
tools/power/cpupower/lib/cpufreq.c | 208 +++++
tools/power/cpupower/lib/cpufreq.h | 223 +++++
tools/power/cpupower/lib/sysfs.c | 672 ++++++++++++++
tools/power/cpupower/lib/sysfs.h | 31 +
tools/power/cpupower/man/cpupower-frequency-info.1 | 76 ++
tools/power/cpupower/man/cpupower-frequency-set.1 | 54 ++
tools/power/cpupower/man/cpupower-info.1 | 19 +
tools/power/cpupower/man/cpupower-monitor.1 | 179 ++++
tools/power/cpupower/man/cpupower-set.1 | 103 +++
tools/power/cpupower/man/cpupower.1 | 72 ++
tools/power/cpupower/po/cs.po | 944 +++++++++++++++++++
tools/power/cpupower/po/de.po | 961 ++++++++++++++++++++
tools/power/cpupower/po/fr.po | 947 +++++++++++++++++++
tools/power/cpupower/po/it.po | 961 ++++++++++++++++++++
tools/power/cpupower/po/pt.po | 957 +++++++++++++++++++
tools/power/cpupower/utils/builtin.h | 18 +
tools/power/cpupower/utils/cpufreq-info.c | 708 ++++++++++++++
tools/power/cpupower/utils/cpufreq-set.c | 358 ++++++++
tools/power/cpupower/utils/cpuidle-info.c | 244 +++++
tools/power/cpupower/utils/cpupower-info.c | 153 ++++
tools/power/cpupower/utils/cpupower-set.c | 153 ++++
tools/power/cpupower/utils/cpupower.c | 203 ++++
tools/power/cpupower/utils/helpers/amd.c | 137 +++
tools/power/cpupower/utils/helpers/bitmask.c | 292 ++++++
tools/power/cpupower/utils/helpers/bitmask.h | 33 +
tools/power/cpupower/utils/helpers/cpuid.c | 176 ++++
tools/power/cpupower/utils/helpers/helpers.h | 178 ++++
tools/power/cpupower/utils/helpers/misc.c | 27 +
tools/power/cpupower/utils/helpers/msr.c | 115 +++
tools/power/cpupower/utils/helpers/pci.c | 44 +
tools/power/cpupower/utils/helpers/sysfs.c | 358 ++++++++
tools/power/cpupower/utils/helpers/sysfs.h | 28 +
tools/power/cpupower/utils/helpers/topology.c | 108 +++
.../cpupower/utils/idle_monitor/amd_fam14h_idle.c | 338 +++++++
.../cpupower/utils/idle_monitor/cpuidle_sysfs.c | 196 ++++
.../cpupower/utils/idle_monitor/cpupower-monitor.c | 448 +++++++++
.../cpupower/utils/idle_monitor/cpupower-monitor.h | 68 ++
.../cpupower/utils/idle_monitor/idle_monitors.def | 7 +
.../cpupower/utils/idle_monitor/idle_monitors.h | 18 +
.../cpupower/utils/idle_monitor/mperf_monitor.c | 255 ++++++
tools/power/cpupower/utils/idle_monitor/nhm_idle.c | 216 +++++
tools/power/cpupower/utils/idle_monitor/snb_idle.c | 190 ++++
tools/power/cpupower/utils/version-gen.sh | 35 +
72 files changed, 13877 insertions(+), 1 deletions(-)
create mode 100644 tools/power/cpupower/.gitignore
create mode 100644 tools/power/cpupower/Makefile
create mode 100644 tools/power/cpupower/README
create mode 100644 tools/power/cpupower/ToDo
create mode 100644 tools/power/cpupower/bench/Makefile
create mode 100644 tools/power/cpupower/bench/README-BENCH
create mode 100644 tools/power/cpupower/bench/benchmark.c
create mode 100644 tools/power/cpupower/bench/benchmark.h
create mode 100644 tools/power/cpupower/bench/config.h
create mode 100644 tools/power/cpupower/bench/cpufreq-bench_plot.sh
create mode 100644 tools/power/cpupower/bench/cpufreq-bench_script.sh
create mode 100644 tools/power/cpupower/bench/example.cfg
create mode 100644 tools/power/cpupower/bench/main.c
create mode 100644 tools/power/cpupower/bench/parse.c
create mode 100644 tools/power/cpupower/bench/parse.h
create mode 100644 tools/power/cpupower/bench/system.c
create mode 100644 tools/power/cpupower/bench/system.h
create mode 100644 tools/power/cpupower/debug/i386/Makefile
create mode 100644 tools/power/cpupower/debug/i386/centrino-decode.c
create mode 100644 tools/power/cpupower/debug/i386/dump_psb.c
create mode 100644 tools/power/cpupower/debug/i386/intel_gsic.c
create mode 100644 tools/power/cpupower/debug/i386/powernow-k8-decode.c
create mode 100644 tools/power/cpupower/debug/kernel/Makefile
create mode 100644 tools/power/cpupower/debug/kernel/cpufreq-test_tsc.c
create mode 100644 tools/power/cpupower/debug/x86_64/Makefile
create mode 120000 tools/power/cpupower/debug/x86_64/centrino-decode.c
create mode 120000 tools/power/cpupower/debug/x86_64/powernow-k8-decode.c
create mode 100644 tools/power/cpupower/lib/cpufreq.c
create mode 100644 tools/power/cpupower/lib/cpufreq.h
create mode 100644 tools/power/cpupower/lib/sysfs.c
create mode 100644 tools/power/cpupower/lib/sysfs.h
create mode 100644 tools/power/cpupower/man/cpupower-frequency-info.1
create mode 100644 tools/power/cpupower/man/cpupower-frequency-set.1
create mode 100644 tools/power/cpupower/man/cpupower-info.1
create mode 100644 tools/power/cpupower/man/cpupower-monitor.1
create mode 100644 tools/power/cpupower/man/cpupower-set.1
create mode 100644 tools/power/cpupower/man/cpupower.1
create mode 100644 tools/power/cpupower/po/cs.po
create mode 100644 tools/power/cpupower/po/de.po
create mode 100644 tools/power/cpupower/po/fr.po
create mode 100644 tools/power/cpupower/po/it.po
create mode 100644 tools/power/cpupower/po/pt.po
create mode 100644 tools/power/cpupower/utils/builtin.h
create mode 100644 tools/power/cpupower/utils/cpufreq-info.c
create mode 100644 tools/power/cpupower/utils/cpufreq-set.c
create mode 100644 tools/power/cpupower/utils/cpuidle-info.c
create mode 100644 tools/power/cpupower/utils/cpupower-info.c
create mode 100644 tools/power/cpupower/utils/cpupower-set.c
create mode 100644 tools/power/cpupower/utils/cpupower.c
create mode 100644 tools/power/cpupower/utils/helpers/amd.c
create mode 100644 tools/power/cpupower/utils/helpers/bitmask.c
create mode 100644 tools/power/cpupower/utils/helpers/bitmask.h
create mode 100644 tools/power/cpupower/utils/helpers/cpuid.c
create mode 100644 tools/power/cpupower/utils/helpers/helpers.h
create mode 100644 tools/power/cpupower/utils/helpers/misc.c
create mode 100644 tools/power/cpupower/utils/helpers/msr.c
create mode 100644 tools/power/cpupower/utils/helpers/pci.c
create mode 100644 tools/power/cpupower/utils/helpers/sysfs.c
create mode 100644 tools/power/cpupower/utils/helpers/sysfs.h
create mode 100644 tools/power/cpupower/utils/helpers/topology.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/amd_fam14h_idle.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/cpupower-monitor.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/cpupower-monitor.h
create mode 100644 tools/power/cpupower/utils/idle_monitor/idle_monitors.def
create mode 100644 tools/power/cpupower/utils/idle_monitor/idle_monitors.h
create mode 100644 tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/nhm_idle.c
create mode 100644 tools/power/cpupower/utils/idle_monitor/snb_idle.c
create mode 100755 tools/power/cpupower/utils/version-gen.sh
^ 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