Hi there,
rtpm_status_show() is still there in the code. You found that I removed the
rtpm_status_show() in the patch because that after back port, there are two
rtpm_status_show() in the code. So I removed one of them to resolve the
conflict.
Please look at patched source code.
Yong
On Wed, Sep 15, 2010 at 6:33 AM, Leann Ogasawara <
leann.ogasaw...@canonical.com> wrote:
> On Tue, 2010-09-14 at 10:23 -0600, Tim Gardner wrote:
> > On 09/13/2010 12:31 AM, yong.s...@linaro.org wrote:
> > > From: Arjan van de Ven<ar...@linux.intel.com>
> > >
> > > In order for PowerTOP to be able to report how well the new runtime PM
> is
> > > working for the various drivers, the kernel needs to export some basic
> > > statistics in sysfs.
> > >
> > > This patch adds two sysfs files in the runtime PM domain that expose
> the
> > > total time a device has been active, and the time a device has been
> > > suspended.
> > >
> > > With this PowerTOP can compute the activity percentage
> > >
> > > Active %age = 100 * (delta active) / (delta active + delta suspended)
> > >
> > > and present the information to the user.
> > >
> > > I've written the PowerTOP code (slated for version 1.12) already, and
> the
> > > output looks like this:
> > >
> > > Runtime Device Power Management statistics
> > > Active Device name
> > > 10.0% 06:00.0 Ethernet controller: Realtek Semiconductor Co.,
> Ltd. RTL8101E/RTL8102E PCI Express Fast Ethernet controller
> > >
> > > [version 2: fix stat update bugs noticed by Alan Stern]
> > > [version 3: rebase to -next and move the sysfs declaration]
> > >
> > > Signed-off-by: Arjan van de Ven<ar...@linux.intel.com>
> > > Signed-off-by: Rafael J. Wysocki<r...@sisk.pl>
> > > Signed-off-by: Shen Yong<yong.s...@linaro.org>
> > > ---
> > > drivers/base/power/runtime.c | 54 ++++++++++++++++++++++++----
> > > drivers/base/power/sysfs.c | 83
> ++++++++++++++++++++++++++++++++----------
> > > include/linux/pm.h | 6 +++
> > > 3 files changed, 116 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/base/power/runtime.c
> b/drivers/base/power/runtime.c
> > > index b0ec0e9..b78c401 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -123,6 +123,45 @@ int pm_runtime_idle(struct device *dev)
> > > }
> > > EXPORT_SYMBOL_GPL(pm_runtime_idle);
> > >
> > > +
> > > +/**
> > > + * update_pm_runtime_accounting - Update the time accounting of power
> states
> > > + * @dev: Device to update the accounting for
> > > + *
> > > + * In order to be able to have time accounting of the various power
> states
> > > + * (as used by programs such as PowerTOP to show the effectiveness of
> runtime
> > > + * PM), we need to track the time spent in each state.
> > > + * update_pm_runtime_accounting must be called each time before the
> > > + * runtime_status field is updated, to account the time in the old
> state
> > > + * correctly.
> > > + */
> > > +void update_pm_runtime_accounting(struct device *dev)
> > > +{
> > > + unsigned long now = jiffies;
> > > + int delta;
> > > +
> > > + delta = now - dev->power.accounting_timestamp;
> > > +
> > > + if (delta< 0)
> > > + delta = 0;
> > > +
> > > + dev->power.accounting_timestamp = now;
> > > +
> > > + if (dev->power.disable_depth> 0)
> > > + return;
> > > +
> > > + if (dev->power.runtime_status == RPM_SUSPENDED)
> > > + dev->power.suspended_jiffies += delta;
> > > + else
> > > + dev->power.active_jiffies += delta;
> > > +}
> > > +
> > > +static void __update_runtime_status(struct device *dev, enum
> rpm_status status)
> > > +{
> > > + update_pm_runtime_accounting(dev);
> > > + dev->power.runtime_status = status;
> > > +}
> > > +
> > > /**
> > > * __pm_runtime_suspend - Carry out run-time suspend of given device.
> > > * @dev: Device to suspend.
> > > @@ -197,7 +236,7 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
> > > goto repeat;
> > > }
> > >
> > > - dev->power.runtime_status = RPM_SUSPENDING;
> > > + __update_runtime_status(dev, RPM_SUSPENDING);
> > > dev->power.deferred_resume = false;
> > >
> > > if (dev->bus&& dev->bus->pm&& dev->bus->pm->runtime_suspend) {
> > > @@ -228,7 +267,7 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
> > > }
> > >
> > > if (retval) {
> > > - dev->power.runtime_status = RPM_ACTIVE;
> > > + __update_runtime_status(dev, RPM_ACTIVE);
> > > if (retval == -EAGAIN || retval == -EBUSY) {
> > > if (dev->power.timer_expires == 0)
> > > notify = true;
> > > @@ -237,7 +276,7 @@ int __pm_runtime_suspend(struct device *dev, bool
> from_wq)
> > > pm_runtime_cancel_pending(dev);
> > > }
> > > } else {
> > > - dev->power.runtime_status = RPM_SUSPENDED;
> > > + __update_runtime_status(dev, RPM_SUSPENDED);
> > > pm_runtime_deactivate_timer(dev);
> > >
> > > if (dev->parent) {
> > > @@ -381,7 +420,7 @@ int __pm_runtime_resume(struct device *dev, bool
> from_wq)
> > > goto repeat;
> > > }
> > >
> > > - dev->power.runtime_status = RPM_RESUMING;
> > > + __update_runtime_status(dev, RPM_RESUMING);
> > >
> > > if (dev->bus&& dev->bus->pm&& dev->bus->pm->runtime_resume) {
> > > spin_unlock_irq(&dev->power.lock);
> > > @@ -411,10 +450,10 @@ int __pm_runtime_resume(struct device *dev, bool
> from_wq)
> > > }
> > >
> > > if (retval) {
> > > - dev->power.runtime_status = RPM_SUSPENDED;
> > > + __update_runtime_status(dev, RPM_SUSPENDED);
> > > pm_runtime_cancel_pending(dev);
> > > } else {
> > > - dev->power.runtime_status = RPM_ACTIVE;
> > > + __update_runtime_status(dev, RPM_ACTIVE);
> > > if (parent)
> > > atomic_inc(&parent->power.child_count);
> > > }
> > > @@ -848,7 +887,7 @@ int __pm_runtime_set_status(struct device *dev,
> unsigned int status)
> > > }
> > >
> > > out_set:
> > > - dev->power.runtime_status = status;
> > > + __update_runtime_status(dev, status);
> > > dev->power.runtime_error = 0;
> > > out:
> > > spin_unlock_irqrestore(&dev->power.lock, flags);
> > > @@ -1077,6 +1116,7 @@ void pm_runtime_init(struct device *dev)
> > > dev->power.request_pending = false;
> > > dev->power.request = RPM_REQ_NONE;
> > > dev->power.deferred_resume = false;
> > > + dev->power.accounting_timestamp = jiffies;
> > > INIT_WORK(&dev->power.work, pm_runtime_work);
> > >
> > > dev->power.timer_expires = 0;
> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > index a4c33bc..b831ec5 100644
> > > --- a/drivers/base/power/sysfs.c
> > > +++ b/drivers/base/power/sysfs.c
> > > @@ -6,6 +6,7 @@
> > > #include<linux/string.h>
> > > #include<linux/pm_runtime.h>
> > > #include<asm/atomic.h>
> > > +#include<linux/jiffies.h>
> > > #include "power.h"
> > >
> > > /*
> > > @@ -108,6 +109,65 @@ static ssize_t control_store(struct device * dev,
> struct device_attribute *attr,
> > > }
> > >
> > > static DEVICE_ATTR(control, 0644, control_show, control_store);
> > > +
> > > +static ssize_t rtpm_active_time_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + int ret;
> > > + spin_lock_irq(&dev->power.lock);
> > > + update_pm_runtime_accounting(dev);
> > > + ret = sprintf(buf, "%i\n",
> jiffies_to_msecs(dev->power.active_jiffies));
> > > + spin_unlock_irq(&dev->power.lock);
> > > + return ret;
> > > +}
> > > +
> > > +static DEVICE_ATTR(runtime_active_time, 0444, rtpm_active_time_show,
> NULL);
> > > +
> > > +static ssize_t rtpm_suspended_time_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + int ret;
> > > + spin_lock_irq(&dev->power.lock);
> > > + update_pm_runtime_accounting(dev);
> > > + ret = sprintf(buf, "%i\n",
> > > + jiffies_to_msecs(dev->power.suspended_jiffies));
> > > + spin_unlock_irq(&dev->power.lock);
> > > + return ret;
> > > +}
> > > +
> > > +static DEVICE_ATTR(runtime_suspended_time, 0444,
> rtpm_suspended_time_show, NULL);
> > > +
> > > +static ssize_t rtpm_status_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + const char *p;
> > > +
> > > + if (dev->power.runtime_error) {
> > > + p = "error\n";
> > > + } else if (dev->power.disable_depth) {
> > > + p = "unsupported\n";
> > > + } else {
> > > + switch (dev->power.runtime_status) {
> > > + case RPM_SUSPENDED:
> > > + p = "suspended\n";
> > > + break;
> > > + case RPM_SUSPENDING:
> > > + p = "suspending\n";
> > > + break;
> > > + case RPM_RESUMING:
> > > + p = "resuming\n";
> > > + break;
> > > + case RPM_ACTIVE:
> > > + p = "active\n";
> > > + break;
> > > + default:
> > > + return -EIO;
> > > + }
> > > + }
> > > + return sprintf(buf, p);
> > > +}
> > > +
> > > +static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL);
> > > #endif
> > >
> > > static ssize_t
> > > @@ -172,27 +232,8 @@ static ssize_t rtpm_enabled_show(struct device
> *dev,
> > > return sprintf(buf, "enabled\n");
> > > }
> > >
> > > -static ssize_t rtpm_status_show(struct device *dev,
> > > - struct device_attribute *attr, char *buf)
> > > -{
> > > - if (dev->power.runtime_error)
> > > - return sprintf(buf, "error\n");
> > > - switch (dev->power.runtime_status) {
> > > - case RPM_SUSPENDED:
> > > - return sprintf(buf, "suspended\n");
> > > - case RPM_SUSPENDING:
> > > - return sprintf(buf, "suspending\n");
> > > - case RPM_RESUMING:
> > > - return sprintf(buf, "resuming\n");
> > > - case RPM_ACTIVE:
> > > - return sprintf(buf, "active\n");
> > > - }
> > > - return -EIO;
> > > -}
> > > -
>
> Completely removing rtpm_status_show() doesn't appear correct, as this
> is not removed in the upstream version of the patch.
>
> > > static DEVICE_ATTR(runtime_usage, 0444, rtpm_usagecount_show, NULL);
> > > static DEVICE_ATTR(runtime_active_kids, 0444, rtpm_children_show,
> NULL);
> > > -static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL);
> > > static DEVICE_ATTR(runtime_enabled, 0444, rtpm_enabled_show, NULL);
> > >
> > > #endif
> > > @@ -228,6 +269,9 @@ static DEVICE_ATTR(async, 0644, async_show,
> async_store);
> > > static struct attribute * power_attrs[] = {
> > > #ifdef CONFIG_PM_RUNTIME
> > > &dev_attr_control.attr,
> > > + &dev_attr_runtime_status.attr,
> > > + &dev_attr_runtime_suspended_time.attr,
> > > + &dev_attr_runtime_active_time.attr,
> > > #endif
> > > &dev_attr_wakeup.attr,
> > > #ifdef CONFIG_PM_ADVANCED_DEBUG
> > > @@ -235,7 +279,6 @@ static struct attribute * power_attrs[] = {
> > > #ifdef CONFIG_PM_RUNTIME
> > > &dev_attr_runtime_usage.attr,
> > > &dev_attr_runtime_active_kids.attr,
> > > - &dev_attr_runtime_status.attr,
> > > &dev_attr_runtime_enabled.attr,
> > > #endif
> > > #endif
> > > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > > index 8e258c7..dca597f 100644
> > > --- a/include/linux/pm.h
> > > +++ b/include/linux/pm.h
> > > @@ -476,9 +476,15 @@ struct dev_pm_info {
> > > enum rpm_request request;
> > > enum rpm_status runtime_status;
> > > int runtime_error;
> > > + unsigned long active_jiffies;
> > > + unsigned long suspended_jiffies;
> > > + unsigned long accounting_timestamp;
> > > #endif
> > > };
> > >
> > > +extern void update_pm_runtime_accounting(struct device *dev);
> > > +
> > > +
> > > /*
> > > * The PM_EVENT_ messages are also used by drivers implementing the
> legacy
> > > * suspend framework, based on the ->suspend() and ->resume()
> callbacks common
> >
> > This isn't a faithful backport of
> > 8d4b9d1bfef117862a2889dec4dac227068544c9. What happened to
> > rtpm_status_show() ? Even if its not used I'd prefer to keep that
> > function in the patch so as to cause fewer conflicts if there are
> > upstream stable patches.
> >
> > I prefer the attached version.
>
> Tim's patch was on the right track to provide an updated
> rtpm_status_show() routine from what we currently have in Maverick, but
> this still didn't originate from upstream commit 8d4b9d1b.
>
> I've thus taken the path of least resistance and cleanly cherry-picked
> the following for the Ubuntu Maverick kernel:
>
> commit 0fcb4eef8294492c8f1de8236b1ed81f09e42922
> Author: Alan Stern <st...@rowland.harvard.edu>
> Date: Thu Jul 8 00:05:37 2010 +0200
>
> PM / Runtime: Make runtime_status attribute not debug-only (v. 2)
>
> commit 8d4b9d1bfef117862a2889dec4dac227068544c9
> Author: Arjan van de Ven <ar...@linux.intel.com>
> Date: Mon Jul 19 02:01:06 2010 +0200
>
> PM / Runtime: Add runtime PM statistics (v3)
>
> Thanks,
> Leann
>
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
