[PATCH] PM: fix calculation mistake in roll-over cases

[PATCH] PM: fix calculation mistake in roll-over cases

[vbyravarasu]

From: venu byravarasu <vbyravarasu@nvidia.com>

In case of jiffies roll over, delta is made zero.
Hence fixing it, after taking roll over into consideration.

Signed-off-by: venu byravarasu <vbyravarasu@nvidia.com>
---
When jiffies roll over, calculation of time spent in the
current state (stored in variable 'delta') is incorrect.
Hence fixing it, after taking roll over into consideration.

 drivers/base/power/runtime.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 1079e03..bd93fb0 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -30,12 +30,13 @@ static int rpm_suspend(struct device *dev, int rpmflags);
 void update_pm_runtime_accounting(struct device *dev)
 {
 	unsigned long now = jiffies;
-	int delta;
+	unsigned long delta;
+	unsigned long max_num = ~0;
 
 	delta = now - dev->power.accounting_timestamp;
 
-	if (delta < 0)
-		delta = 0;
+	if (now < dev->power.accounting_timestamp)
+		delta = max_num - dev->power.accounting_timestamp + now;
 
 	dev->power.accounting_timestamp = now;
 
-- 
1.7.1.1

Re: [PATCH] PM: fix calculation mistake in roll-over cases

[Rafael J. Wysocki]

On Tuesday, October 25, 2011, vbyravarasu@nvidia.com wrote:
> From: venu byravarasu <vbyravarasu@nvidia.com>
> 
> In case of jiffies roll over, delta is made zero.
> Hence fixing it, after taking roll over into consideration.
> 
> Signed-off-by: venu byravarasu <vbyravarasu@nvidia.com>
> ---
> When jiffies roll over, calculation of time spent in the
> current state (stored in variable 'delta') is incorrect.
> Hence fixing it, after taking roll over into consideration.
> 
>  drivers/base/power/runtime.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 1079e03..bd93fb0 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -30,12 +30,13 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>  void update_pm_runtime_accounting(struct device *dev)
>  {
>  	unsigned long now = jiffies;
> -	int delta;
> +	unsigned long delta;
> +	unsigned long max_num = ~0;
>  
>  	delta = now - dev->power.accounting_timestamp;
>  
> -	if (delta < 0)
> -		delta = 0;
> +	if (now < dev->power.accounting_timestamp)
> +		delta = max_num - dev->power.accounting_timestamp + now;
>  
>  	dev->power.accounting_timestamp = now;

I'm not sure how this is supposed to improve things.  Care to give more
details?

Rafael

RE: [PATCH] PM: fix calculation mistake in roll-over cases

[Venu Byravarasu]

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, October 25, 2011 6:39 PM
To: Venu Byravarasu
Cc: pavel@ucw.cz; len.brown@intel.com; gregkh@suse.de; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Alan Stern
Subject: Re: [PATCH] PM: fix calculation mistake in roll-over cases

On Tuesday, October 25, 2011, vbyravarasu@nvidia.com wrote:
> From: venu byravarasu <vbyravarasu@nvidia.com>
> 
> In case of jiffies roll over, delta is made zero.
> Hence fixing it, after taking roll over into consideration.
> 
> Signed-off-by: venu byravarasu <vbyravarasu@nvidia.com>
> ---
> When jiffies roll over, calculation of time spent in the
> current state (stored in variable 'delta') is incorrect.
> Hence fixing it, after taking roll over into consideration.
> 
>  drivers/base/power/runtime.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 1079e03..bd93fb0 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -30,12 +30,13 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>  void update_pm_runtime_accounting(struct device *dev)
>  {
>  	unsigned long now = jiffies;
> -	int delta;
> +	unsigned long delta;
> +	unsigned long max_num = ~0;
>  
>  	delta = now - dev->power.accounting_timestamp;
>  
> -	if (delta < 0)
> -		delta = 0;
> +	if (now < dev->power.accounting_timestamp)
> +		delta = max_num - dev->power.accounting_timestamp + now;
>  
>  	dev->power.accounting_timestamp = now;

> I'm not sure how this is supposed to improve things.  Care to give more
> details?
Below two items were taken care with this change:
1. Value of now is jiffies which is unsigned long.
As it is being stored in delta of 'int' type, for all values of now > 0x80000000,
Delta will be made 0 with the original if condition.
By changing delta to unsigned long, this is taken care.

2. Even if delta is made unsigned, in cases of jiffies roll over, delta will be zero.
That is also being taken care with the code added as part of if condition.

Plz let me know if you still have any questions.
> Rafael
Thanks,
Venu

RE: [PATCH] PM: fix calculation mistake in roll-over cases

[Alan Stern]

On Thu, 27 Oct 2011, Venu Byravarasu wrote:

> > From: venu byravarasu <vbyravarasu@nvidia.com>
> > 
> > In case of jiffies roll over, delta is made zero.
> > Hence fixing it, after taking roll over into consideration.
> > 
> > Signed-off-by: venu byravarasu <vbyravarasu@nvidia.com>
> > ---
> > When jiffies roll over, calculation of time spent in the
> > current state (stored in variable 'delta') is incorrect.
> > Hence fixing it, after taking roll over into consideration.
> > 
> >  drivers/base/power/runtime.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 1079e03..bd93fb0 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -30,12 +30,13 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> >  void update_pm_runtime_accounting(struct device *dev)
> >  {
> >  	unsigned long now = jiffies;
> > -	int delta;
> > +	unsigned long delta;

That change is correct.

> > +	unsigned long max_num = ~0;

max_num is not needed.

> >  
> >  	delta = now - dev->power.accounting_timestamp;
> >  
> > -	if (delta < 0)
> > -		delta = 0;

That change is correct.

> > +	if (now < dev->power.accounting_timestamp)
> > +		delta = max_num - dev->power.accounting_timestamp + now;

These two lines are not needed.

> >  
> >  	dev->power.accounting_timestamp = now;
> 
> > I'm not sure how this is supposed to improve things.  Care to give more
> > details?
> Below two items were taken care with this change:
> 1. Value of now is jiffies which is unsigned long.
> As it is being stored in delta of 'int' type, for all values of now > 0x80000000,
> Delta will be made 0 with the original if condition.
> By changing delta to unsigned long, this is taken care.

Yes, that's the right thing to do.

> 2. Even if delta is made unsigned, in cases of jiffies roll over, delta will be zero.
> That is also being taken care with the code added as part of if condition.

Since delta is now unsigned, the "if (delta < 0)" test can never
succeed.  Therefore it can be removed.

The new lines you added with max_num don't seem to serve any useful 
purpose.  All they do is recalculate the same value that delta had 
before, but with an off-by-one error.

Alan Stern

RE: [PATCH] PM: fix calculation mistake in roll-over cases

[Venu Byravarasu]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Friday, October 28, 2011 12:00 AM
> To: Venu Byravarasu
> Cc: Rafael J. Wysocki; pavel@ucw.cz; len.brown@intel.com; gregkh@suse.de;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Peter De Schrijver
> Subject: RE: [PATCH] PM: fix calculation mistake in roll-over cases
> 
> On Thu, 27 Oct 2011, Venu Byravarasu wrote:
> 
> > > From: venu byravarasu <vbyravarasu@nvidia.com>
> > >
> > > In case of jiffies roll over, delta is made zero.
> > > Hence fixing it, after taking roll over into consideration.
> > >
> > > Signed-off-by: venu byravarasu <vbyravarasu@nvidia.com>
> > > ---
> 
> > 2. Even if delta is made unsigned, in cases of jiffies roll over, delta will be zero.
> > That is also being taken care with the code added as part of if condition.
> 
> Since delta is now unsigned, the "if (delta < 0)" test can never
> succeed.  Therefore it can be removed.
> 
> The new lines you added with max_num don't seem to serve any useful
> purpose.  All they do is recalculate the same value that delta had
> before, but with an off-by-one error.
> 
Agree with you Alan.
Will remove max_num references and upload the change.

Thanks,
Venu