Regression in v5.0-rc1 with autosuspend hrtimers

Hi all,

Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") caused a regression on at least omap5-uevm where 8250 UART rx wake no longer works. I have not noticed this happening on others so far.

The devices I've tested all are using 8250 with dedicated wakeirqs configured for the rx pin. I can see the interrupt increase on omap5-uevm after some one or more keypresses, but then nothing. It seems that the uart just falls back asleep right away or something.

Any ideas what might be going wrong?

Regards,

Tony

Hi Tony,

Quoted 14 lines · Tony Lindgren

Hi all,

Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") caused a regression on at least omap5-uevm where 8250 UART rx wake no longer works. I have not noticed this happening on others so far.

The devices I've tested all are using 8250 with dedicated wakeirqs configured for the rx pin. I can see the interrupt increase on omap5-uevm after some one or more keypresses, but then nothing. It seems that the uart just falls back asleep right away or something.

Quoted 1 line

Any ideas what might be going wrong?

What is the autosuspend value ? Can it be that the autosuspend is set to a short value but was finally greater than 10-20ms on arm32. And now the autosuspend happens before and this has changed the sequence ?

Quoted 17 lines · Vincent Guittot

Hi Tony,

Quoted 14 lines · Tony Lindgren

Hi all,

Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") caused a regression on at least omap5-uevm where 8250 UART rx wake no longer works. I have not noticed this happening on others so far.

The devices I've tested all are using 8250 with dedicated wakeirqs configured for the rx pin. I can see the interrupt increase on omap5-uevm after some one or more keypresses, but then nothing. It seems that the uart just falls back asleep right away or something.

Quoted 5 lines

Quoted 1 line

Any ideas what might be going wrong?

What is the autosuspend value ? Can it be that the autosuspend is set to a short value but was finally greater than 10-20ms on arm32. And now the autosuspend happens before and this has changed the sequence ?

It's set to 3 seconds. The difference between let's say C-A9 pandaboard (that is working) compared to C-A15 omap5-uevm is that the C-A15 has arch_timer in use. Other than that things should behave more or less the same way.

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

Regards,

Tony

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

OK yeah looks like that part is working just fine.

Adding some printks and debugging over ssh, looks like omap8250_runtime_resume() gets called just fine based on a wakeirq, but then omap8250_runtime_suspend() runs immediately instead of waiting for the three second timeout.

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

Regards,

Tony

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

---
 drivers/base/power/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/runtime.c +1 -1

index 7062469..44c5c76 100644
@@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
 
 	last_busy = READ_ONCE(dev->power.last_busy);
 
-	expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+	expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
 	if (expires <= now)
 		expires = 0;	/* Already expired. */
 

Signature · 1 line

2.7.4

Quoted 4 lines

Regards,

Tony

Quoted 5 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

Quoted 3 lines

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Quoted 5 lines

Quoted 1 line

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Cool. Thanks for getting to the bottom of this!

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Cool. Thanks for getting to the bottom of this!

No problem.

One more thing I noticed: The 25% slack can get noticeable for larger values. For things like a 3 second uart console timeout slack of 750 ms is quite large variation.

Should we have a limit of max 100 ms for the slack?

Regards,

Tony

Quoted 26 lines · Tony Lindgren

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Cool. Thanks for getting to the bottom of this!

No problem.

One more thing I noticed: The 25% slack can get noticeable for larger values. For things like a 3 second uart console timeout slack of 750 ms is quite large variation.

Quoted 1 line

Should we have a limit of max 100 ms for the slack?

Keep in mind that when jiffies were used, expires was rounded to a full second when delay was greater than a second. So you could already have difference of up 990ms on arm before this patch
And i don't take into account the rework of timer infra which add another level of variation, something like up to 640 ms more when the timer is greater than 2880 ms for arm IIRC

Quoted 27 lines · Vincent Guittot

Quoted 26 lines · Tony Lindgren

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Cool. Thanks for getting to the bottom of this!

No problem.

One more thing I noticed: The 25% slack can get noticeable for larger values. For things like a 3 second uart console timeout slack of 750 ms is quite large variation.

Quoted 8 lines

Quoted 1 line

Should we have a limit of max 100 ms for the slack?

Keep in mind that when jiffies were used, expires was rounded to a full second when delay was greater than a second. So you could already have difference of up 990ms on arm before this patch
And i don't take into account the rework of timer infra which add another level of variation, something like up to 640 ms more when the timer is greater than 2880 ms for arm IIRC

I think it was rounded up earlier.

Don't we get rounded down now also?

Regards,

Tony

Quoted 38 lines · Tony Lindgren

Quoted 35 lines · Vincent Guittot

Quoted 20 lines · Tony Lindgren

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Could you try the patch below ?

Yup great thanks, that's it:

Tested-by Tony Lindgren <[email protected]>

Cool. Thanks for getting to the bottom of this!

Quoted 7 lines

No problem.

One more thing I noticed: The 25% slack can get noticeable for larger values. For things like a 3 second uart console timeout slack of 750 ms is quite large variation.

Should we have a limit of max 100 ms for the slack?

Keep in mind that when jiffies were used, expires was rounded to a full second when delay was greater than a second. So you could already have difference of up 990ms on arm before this patch
And i don't take into account the rework of timer infra which add another level of variation, something like up to 640 ms more when the timer is greater than 2880 ms for arm IIRC

Quoted 3 lines

I think it was rounded up earlier.

Don't we get rounded down now also?

We still round up. In hrtimer we have :
timer->_softexpires = time;
timer->node.expires = ktime_add_safe(time, delta); so the hrtimer will expire between "time" and "time+delta"

Quoted 11 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

OK yeah looks like that part is working just fine.

Quoted 8 lines

Adding some printks and debugging over ssh, looks like omap8250_runtime_resume() gets called just fine based on a wakeirq, but then omap8250_runtime_suspend() runs immediately instead of waiting for the three second timeout.

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

---
 drivers/base/power/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/runtime.c +0 -0

index 7062469..44c5c76 100644

Quoted 37 lines · Vincent Guittot

Quoted 11 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

OK yeah looks like that part is working just fine.

Quoted 8 lines

Adding some printks and debugging over ssh, looks like omap8250_runtime_resume() gets called just fine based on a wakeirq, but then omap8250_runtime_suspend() runs immediately instead of waiting for the three second timeout.

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

---
 drivers/base/power/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/runtime.c +0 -0

index 7062469..44c5c76 100644
@@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
 
      last_busy = READ_ONCE(dev->power.last_busy);
 

drivers/base/power/runtime.c u64 pm_runtime_autosuspend_expiration(struct device *dev) · line 146

Quoted 4 lines

drivers/base/power/runtime.c +1 -1

-     expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+     expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
      if (expires <= now)
              expires = 0;    /* Already expired. */

Hmm, comment above function states it returns "the expiration time in jiffies (adjusted to be nonzero)", so there's probably more to fix...

You can also consider change like this (still does not return jiffies):

drivers/base/power/runtime.c +7 -10

index 70624695b6d5..c72eaf21a61c 100644
@@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       ktime_t expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
+                              READ_ONCE(dev->power.last_busy));
+       if (expires <= ktime_get())
+               return 0;       /* Already expired. */
 
- out:
        return expires;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);

Regards,
ladis

Quoted 48 lines · Vincent Guittot

Quoted 46 lines · Ladislav Michl

Quoted 34 lines · Vincent Guittot

Quoted 11 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

OK yeah looks like that part is working just fine.

Quoted 8 lines

Adding some printks and debugging over ssh, looks like omap8250_runtime_resume() gets called just fine based on a wakeirq, but then omap8250_runtime_suspend() runs immediately instead of waiting for the three second timeout.

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

---
 drivers/base/power/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/runtime.c +0 -0

index 7062469..44c5c76 100644

drivers/base/power/runtime.c u64 pm_runtime_autosuspend_expiration(struct device *dev) · line 146

Quoted 7 lines

drivers/base/power/runtime.c +1 -1

@@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
 
      last_busy = READ_ONCE(dev->power.last_busy);
 
-     expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+     expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
      if (expires <= now)
              expires = 0;    /* Already expired. */

Hmm, comment above function states it returns "the expiration time in jiffies (adjusted to be nonzero)", so there's probably more to fix...

Quoted 2 lines

The comment is wrong and should be updated as commit 8234f6734c5d has moved on hrtimer and expires is now in raw ns unit

Yup. Who'll send a patch? Is it worth optimizing as bellow? I'm fine with doing

both.

Quoted 40 lines

Quoted 40 lines

You can also consider change like this (still does not return jiffies):

drivers/base/power/runtime.c +7 -10

index 70624695b6d5..c72eaf21a61c 100644
@@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       ktime_t expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
+                              READ_ONCE(dev->power.last_busy));
+       if (expires <= ktime_get())
+               return 0;       /* Already expired. */
 
- out:
        return expires;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);

Regards,
ladis

Quoted 54 lines · Vincent Guittot

Quoted 53 lines · Ladislav Michl

Quoted 50 lines · Vincent Guittot

Quoted 39 lines · Ladislav Michl

Quoted 37 lines · Vincent Guittot

Quoted 11 lines · Tony Lindgren

Quoted 7 lines · Vincent Guittot

Quoted 3 lines · Tony Lindgren

Hmm so could it be that we now rely on timers that that may not be capable of waking up the system from idle states with hrtimer?

With nohz and hrtimer enabled, timer relies on hrtimer to generate the tick so you should use the same interrupt.

OK yeah looks like that part is working just fine.

Quoted 8 lines

Adding some printks and debugging over ssh, looks like omap8250_runtime_resume() gets called just fine based on a wakeirq, but then omap8250_runtime_suspend() runs immediately instead of waiting for the three second timeout.

Lowering the autosuspend_delay_ms to 2100 ms makes things work again. Anything higher than 2200 ms seems to somehow time out immediately now :)

This is quite close to the max ns of an int on arm 32bits

Could you try the patch below ?

---
 drivers/base/power/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/runtime.c +0 -0

index 7062469..44c5c76 100644
@@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
 
      last_busy = READ_ONCE(dev->power.last_busy);
 

Quoted 7 lines

drivers/base/power/runtime.c u64 pm_runtime_autosuspend_expiration(struct device *dev) · line 146

Quoted 4 lines

drivers/base/power/runtime.c +1 -1

-     expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+     expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
      if (expires <= now)
              expires = 0;    /* Already expired. */

Hmm, comment above function states it returns "the expiration time in jiffies (adjusted to be nonzero)", so there's probably more to fix...

The comment is wrong and should be updated as commit 8234f6734c5d has moved on hrtimer and expires is now in raw ns unit

Quoted 8 lines

Quoted 1 line

Yup. Who'll send a patch? Is it worth optimizing as bellow? I'm fine with doing

You can send a patch for updating the comment.

Quoted 1 line

both.

Regarding proposal below:
- pm_runtime_autosuspend_expiration() returns u64 and not ktime_t

Well, that's matter of adding ktime_to_ns (which is noop).

Quoted 3 lines

- use helper ktime_before/after to compare ktime_t value

Using ktime helper function makes the code less readable and the

That why I avoided it.

Quoted 7 lines

proposal make the function more difficult to read IMHO when you compare
expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
with
expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),

                              READ_ONCE(dev->power.last_busy));

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

Quoted 6 lines

or when you compare
if (expires <= now)
with
if (ktime_after(now, expires))

Quoted 1 line

And I'm not sure that this will optimize the code at the end

Checked generated code on ARM and x86 and gcc does pretty good job here.

Quoted 1 line

Only the replacement of the "goto out" by return 0 would make sense IMO

Well, main concern was not to call ktime_get at the beginning of function as it is not too cheap.

Quoted 44 lines

Regards,
Vincent

Quoted 40 lines

Quoted 40 lines

Quoted 40 lines

You can also consider change like this (still does not return jiffies):

drivers/base/power/runtime.c +7 -10

index 70624695b6d5..c72eaf21a61c 100644
@@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       ktime_t expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
+                              READ_ONCE(dev->power.last_busy));
+       if (expires <= ktime_get())
+               return 0;       /* Already expired. */
 
- out:
        return expires;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);

Regards,
ladis

Please keep all thread list when replying :-)

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Quoted 2 lines

Well, main concern was not to call ktime_get at the beginning of function as it is not too cheap.

Doesn't the compiler optimize that to just before the test ?

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 6 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Fair enough, so let's go back to:

drivers/base/power/runtime.c +9 -12

index 70624695b6d5..e61ee9b47a26 100644
@@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
  * Compute the autosuspend-delay expiration time based on the device's
  * power.last_busy time.  If the delay has already expired or is disabled
  * (negative) or the power.use_autosuspend flag isn't set, return 0.
- * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
+ * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
  *
  * This function may be called either with or without dev->power.lock held.
  * Either way it can be racy, since power.last_busy may be updated at any time.

@@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       u64 expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires  = READ_ONCE(dev->power.last_busy);
+       expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
+       if (expires > ktime_to_ns(ktime_get()))
+               return expires; /* Expires in the future */
 
- out:
-       return expires;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
 

Which gives for arm:
00000480 <pm_runtime_autosuspend_expiration>:

     480:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     484:       e5d030d1        ldrb    r3, [r0, #209]  ; 0xd1
     488:       e3130004        tst     r3, #4
     48c:       0a00000c        beq     4c4 <pm_runtime_autosuspend_expiration+0x44>
     490:       e59030e4        ldr     r3, [r0, #228]  ; 0xe4
     494:       e3530000        cmp     r3, #0
     498:       ba000009        blt     4c4 <pm_runtime_autosuspend_expiration+0x44>
     49c:       e28050e8        add     r5, r0, #232    ; 0xe8
     4a0:       e8950030        ldm     r5, {r4, r5}
     4a4:       e59f2030        ldr     r2, [pc, #48]   ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
     4a8:       e0e54392        smlal   r4, r5, r2, r3
     4ac:       ebfffffe        bl      0 <ktime_get>
     4b0:       e1550001        cmp     r5, r1
     4b4:       01540000        cmpeq   r4, r0
     4b8:       e1a06004        mov     r6, r4
     4bc:       e1a07005        mov     r7, r5
     4c0:       8a000001        bhi     4cc <pm_runtime_autosuspend_expiration+0x4c>
     4c4:       e3a06000        mov     r6, #0
     4c8:       e3a07000        mov     r7, #0
     4cc:       e1a00006        mov     r0, r6
     4d0:       e1a01007        mov     r1, r7
     4d4:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     4d8:       e12fff1e        bx      lr
     4dc:       000f4240        .word   0x000f4240

And x86:
0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:

     230:       8b 87 a4 01 00 00       mov    0x1a4(%rdi),%eax
     236:       53                      push   %rbx
     237:       85 c0                   test   %eax,%eax
     239:       78 1e                   js     259 <pm_runtime_autosuspend_expiration.part.0+0x29>
     23b:       48 63 d8                movslq %eax,%rbx
     23e:       48 8b 97 a8 01 00 00    mov    0x1a8(%rdi),%rdx
     245:       48 69 db 40 42 0f 00    imul   $0xf4240,%rbx,%rbx
     24c:       48 01 d3                add    %rdx,%rbx
     24f:       e8 00 00 00 00          callq  254 <pm_runtime_autosuspend_expiration.part.0+0x24>
     254:       48 39 c3                cmp    %rax,%rbx
     257:       77 02                   ja     25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
     259:       31 db                   xor    %ebx,%ebx
     25b:       48 89 d8                mov    %rbx,%rax
     25e:       5b                      pop    %rbx
     25f:       c3                      retq

00000000000002a0 <pm_runtime_autosuspend_expiration>:

     2a0:       f6 87 91 01 00 00 04    testb  $0x4,0x191(%rdi)
     2a7:       74 02                   je     2ab <pm_runtime_autosuspend_expiration+0xb>
     2a9:       eb 85                   jmp    230 <pm_runtime_autosuspend_expiration.part.0>
     2ab:       31 c0                   xor    %eax,%eax
     2ad:       c3                      retq
     2ae:       66 90                   xchg   %ax,%ax

[snip]

Quoted 4 lines

Quoted 2 lines

Well, main concern was not to call ktime_get at the beginning of function as it is not too cheap.

Doesn't the compiler optimize that to just before the test ?

No (compare with above). And it is also almost certain that someone will run script and send "...do not needlesly initialize..." patch :)

gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)

00000110 <pm_runtime_autosuspend_expiration>:

     110:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     114:       e1a06000        mov     r6, r0
     118:       ebfffffe        bl      0 <ktime_get>
     11c:       e5d630d1        ldrb    r3, [r6, #209]  ; 0xd1
     120:       e3130004        tst     r3, #4
     124:       0a00000d        beq     160 <pm_runtime_autosuspend_expiration+0x50>
     128:       e596c0e4        ldr     ip, [r6, #228]  ; 0xe4
     12c:       e35c0000        cmp     ip, #0
     130:       ba00000a        blt     160 <pm_runtime_autosuspend_expiration+0x50>
     134:       e28630e8        add     r3, r6, #232    ; 0xe8
     138:       e893000c        ldm     r3, {r2, r3}
     13c:       e1a05001        mov     r5, r1
     140:       e1a04000        mov     r4, r0
     144:       e59f002c        ldr     r0, [pc, #44]   ; 178 <pm_runtime_autosuspend_expiration+0x68>
     148:       e0010c90        mul     r1, r0, ip
     14c:       e0926001        adds    r6, r2, r1
     150:       e0a37fc1        adc     r7, r3, r1, asr #31
     154:       e1550007        cmp     r5, r7
     158:       01540006        cmpeq   r4, r6
     15c:       3a000001        bcc     168 <pm_runtime_autosuspend_expiration+0x58>
     160:       e3a06000        mov     r6, #0
     164:       e3a07000        mov     r7, #0
     168:       e1a00006        mov     r0, r6
     16c:       e1a01007        mov     r1, r7
     170:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     174:       e12fff1e        bx      lr
     178:       000f4240        .word   0x000f4240

Quoted 10 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 4 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

Quoted 4 lines

Quoted 2 lines

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Fair enough, so let's go back to:

This one looks good for me

Quoted 139 lines

drivers/base/power/runtime.c +9 -12

index 70624695b6d5..e61ee9b47a26 100644
@@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
  * Compute the autosuspend-delay expiration time based on the device's
  * power.last_busy time.  If the delay has already expired or is disabled
  * (negative) or the power.use_autosuspend flag isn't set, return 0.
- * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
+ * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
  *
  * This function may be called either with or without dev->power.lock held.
  * Either way it can be racy, since power.last_busy may be updated at any time.

@@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       u64 expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires  = READ_ONCE(dev->power.last_busy);
+       expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
+       if (expires > ktime_to_ns(ktime_get()))
+               return expires; /* Expires in the future */
 
- out:
-       return expires;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
 

Which gives for arm:
00000480 <pm_runtime_autosuspend_expiration>:

     480:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     484:       e5d030d1        ldrb    r3, [r0, #209]  ; 0xd1
     488:       e3130004        tst     r3, #4
     48c:       0a00000c        beq     4c4 <pm_runtime_autosuspend_expiration+0x44>
     490:       e59030e4        ldr     r3, [r0, #228]  ; 0xe4
     494:       e3530000        cmp     r3, #0
     498:       ba000009        blt     4c4 <pm_runtime_autosuspend_expiration+0x44>
     49c:       e28050e8        add     r5, r0, #232    ; 0xe8
     4a0:       e8950030        ldm     r5, {r4, r5}
     4a4:       e59f2030        ldr     r2, [pc, #48]   ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
     4a8:       e0e54392        smlal   r4, r5, r2, r3
     4ac:       ebfffffe        bl      0 <ktime_get>
     4b0:       e1550001        cmp     r5, r1
     4b4:       01540000        cmpeq   r4, r0
     4b8:       e1a06004        mov     r6, r4
     4bc:       e1a07005        mov     r7, r5
     4c0:       8a000001        bhi     4cc <pm_runtime_autosuspend_expiration+0x4c>
     4c4:       e3a06000        mov     r6, #0
     4c8:       e3a07000        mov     r7, #0
     4cc:       e1a00006        mov     r0, r6
     4d0:       e1a01007        mov     r1, r7
     4d4:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     4d8:       e12fff1e        bx      lr
     4dc:       000f4240        .word   0x000f4240

And x86:
0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:

     230:       8b 87 a4 01 00 00       mov    0x1a4(%rdi),%eax
     236:       53                      push   %rbx
     237:       85 c0                   test   %eax,%eax
     239:       78 1e                   js     259 <pm_runtime_autosuspend_expiration.part.0+0x29>
     23b:       48 63 d8                movslq %eax,%rbx
     23e:       48 8b 97 a8 01 00 00    mov    0x1a8(%rdi),%rdx
     245:       48 69 db 40 42 0f 00    imul   $0xf4240,%rbx,%rbx
     24c:       48 01 d3                add    %rdx,%rbx
     24f:       e8 00 00 00 00          callq  254 <pm_runtime_autosuspend_expiration.part.0+0x24>
     254:       48 39 c3                cmp    %rax,%rbx
     257:       77 02                   ja     25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
     259:       31 db                   xor    %ebx,%ebx
     25b:       48 89 d8                mov    %rbx,%rax
     25e:       5b                      pop    %rbx
     25f:       c3                      retq

00000000000002a0 <pm_runtime_autosuspend_expiration>:

     2a0:       f6 87 91 01 00 00 04    testb  $0x4,0x191(%rdi)
     2a7:       74 02                   je     2ab <pm_runtime_autosuspend_expiration+0xb>
     2a9:       eb 85                   jmp    230 <pm_runtime_autosuspend_expiration.part.0>
     2ab:       31 c0                   xor    %eax,%eax
     2ad:       c3                      retq
     2ae:       66 90                   xchg   %ax,%ax

[snip]

Quoted 4 lines

Quoted 2 lines

Well, main concern was not to call ktime_get at the beginning of function as it is not too cheap.

Doesn't the compiler optimize that to just before the test ?

No (compare with above). And it is also almost certain that someone will run script and send "...do not needlesly initialize..." patch :)

gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)

00000110 <pm_runtime_autosuspend_expiration>:

     110:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     114:       e1a06000        mov     r6, r0
     118:       ebfffffe        bl      0 <ktime_get>
     11c:       e5d630d1        ldrb    r3, [r6, #209]  ; 0xd1
     120:       e3130004        tst     r3, #4
     124:       0a00000d        beq     160 <pm_runtime_autosuspend_expiration+0x50>
     128:       e596c0e4        ldr     ip, [r6, #228]  ; 0xe4
     12c:       e35c0000        cmp     ip, #0
     130:       ba00000a        blt     160 <pm_runtime_autosuspend_expiration+0x50>
     134:       e28630e8        add     r3, r6, #232    ; 0xe8
     138:       e893000c        ldm     r3, {r2, r3}
     13c:       e1a05001        mov     r5, r1
     140:       e1a04000        mov     r4, r0
     144:       e59f002c        ldr     r0, [pc, #44]   ; 178 <pm_runtime_autosuspend_expiration+0x68>
     148:       e0010c90        mul     r1, r0, ip
     14c:       e0926001        adds    r6, r2, r1
     150:       e0a37fc1        adc     r7, r3, r1, asr #31
     154:       e1550007        cmp     r5, r7
     158:       01540006        cmpeq   r4, r6
     15c:       3a000001        bcc     168 <pm_runtime_autosuspend_expiration+0x58>
     160:       e3a06000        mov     r6, #0
     164:       e3a07000        mov     r7, #0
     168:       e1a00006        mov     r0, r6
     16c:       e1a01007        mov     r1, r7
     170:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     174:       e12fff1e        bx      lr
     178:       000f4240        .word   0x000f4240

Quoted 14 lines · Vincent Guittot

Quoted 13 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 6 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Quoted 3 lines

Quoted 1 line

Fair enough, so let's go back to:

This one looks good for me

Lets split is for 'comments fix' patch, which was just send and 'the rest'. Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on 32bits arch" or will wait for 5.1. You decide :)

Quoted 140 lines

Quoted 140 lines

drivers/base/power/runtime.c +9 -12

index 70624695b6d5..e61ee9b47a26 100644
@@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
  * Compute the autosuspend-delay expiration time based on the device's
  * power.last_busy time.  If the delay has already expired or is disabled
  * (negative) or the power.use_autosuspend flag isn't set, return 0.
- * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
+ * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
  *
  * This function may be called either with or without dev->power.lock held.
  * Either way it can be racy, since power.last_busy may be updated at any time.

@@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
        int autosuspend_delay;
-       u64 last_busy, expires = 0;
-       u64 now = ktime_to_ns(ktime_get());
+       u64 expires;
 
        if (!dev->power.use_autosuspend)
-               goto out;
+               return 0;
 
        autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
        if (autosuspend_delay < 0)
-               goto out;
-
-       last_busy = READ_ONCE(dev->power.last_busy);
+               return 0;
 
-       expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
-       if (expires <= now)
-               expires = 0;    /* Already expired. */
+       expires  = READ_ONCE(dev->power.last_busy);
+       expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
+       if (expires > ktime_to_ns(ktime_get()))
+               return expires; /* Expires in the future */
 
- out:
-       return expires;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
 

Which gives for arm:
00000480 <pm_runtime_autosuspend_expiration>:

     480:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     484:       e5d030d1        ldrb    r3, [r0, #209]  ; 0xd1
     488:       e3130004        tst     r3, #4
     48c:       0a00000c        beq     4c4 <pm_runtime_autosuspend_expiration+0x44>
     490:       e59030e4        ldr     r3, [r0, #228]  ; 0xe4
     494:       e3530000        cmp     r3, #0
     498:       ba000009        blt     4c4 <pm_runtime_autosuspend_expiration+0x44>
     49c:       e28050e8        add     r5, r0, #232    ; 0xe8
     4a0:       e8950030        ldm     r5, {r4, r5}
     4a4:       e59f2030        ldr     r2, [pc, #48]   ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
     4a8:       e0e54392        smlal   r4, r5, r2, r3
     4ac:       ebfffffe        bl      0 <ktime_get>
     4b0:       e1550001        cmp     r5, r1
     4b4:       01540000        cmpeq   r4, r0
     4b8:       e1a06004        mov     r6, r4
     4bc:       e1a07005        mov     r7, r5
     4c0:       8a000001        bhi     4cc <pm_runtime_autosuspend_expiration+0x4c>
     4c4:       e3a06000        mov     r6, #0
     4c8:       e3a07000        mov     r7, #0
     4cc:       e1a00006        mov     r0, r6
     4d0:       e1a01007        mov     r1, r7
     4d4:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     4d8:       e12fff1e        bx      lr
     4dc:       000f4240        .word   0x000f4240

And x86:
0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:

     230:       8b 87 a4 01 00 00       mov    0x1a4(%rdi),%eax
     236:       53                      push   %rbx
     237:       85 c0                   test   %eax,%eax
     239:       78 1e                   js     259 <pm_runtime_autosuspend_expiration.part.0+0x29>
     23b:       48 63 d8                movslq %eax,%rbx
     23e:       48 8b 97 a8 01 00 00    mov    0x1a8(%rdi),%rdx
     245:       48 69 db 40 42 0f 00    imul   $0xf4240,%rbx,%rbx
     24c:       48 01 d3                add    %rdx,%rbx
     24f:       e8 00 00 00 00          callq  254 <pm_runtime_autosuspend_expiration.part.0+0x24>
     254:       48 39 c3                cmp    %rax,%rbx
     257:       77 02                   ja     25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
     259:       31 db                   xor    %ebx,%ebx
     25b:       48 89 d8                mov    %rbx,%rax
     25e:       5b                      pop    %rbx
     25f:       c3                      retq

00000000000002a0 <pm_runtime_autosuspend_expiration>:

     2a0:       f6 87 91 01 00 00 04    testb  $0x4,0x191(%rdi)
     2a7:       74 02                   je     2ab <pm_runtime_autosuspend_expiration+0xb>
     2a9:       eb 85                   jmp    230 <pm_runtime_autosuspend_expiration.part.0>
     2ab:       31 c0                   xor    %eax,%eax
     2ad:       c3                      retq
     2ae:       66 90                   xchg   %ax,%ax

[snip]

Quoted 4 lines

Quoted 2 lines

Well, main concern was not to call ktime_get at the beginning of function as it is not too cheap.

Doesn't the compiler optimize that to just before the test ?

No (compare with above). And it is also almost certain that someone will run script and send "...do not needlesly initialize..." patch :)

gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)

00000110 <pm_runtime_autosuspend_expiration>:

     110:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
     114:       e1a06000        mov     r6, r0
     118:       ebfffffe        bl      0 <ktime_get>
     11c:       e5d630d1        ldrb    r3, [r6, #209]  ; 0xd1
     120:       e3130004        tst     r3, #4
     124:       0a00000d        beq     160 <pm_runtime_autosuspend_expiration+0x50>
     128:       e596c0e4        ldr     ip, [r6, #228]  ; 0xe4
     12c:       e35c0000        cmp     ip, #0
     130:       ba00000a        blt     160 <pm_runtime_autosuspend_expiration+0x50>
     134:       e28630e8        add     r3, r6, #232    ; 0xe8
     138:       e893000c        ldm     r3, {r2, r3}
     13c:       e1a05001        mov     r5, r1
     140:       e1a04000        mov     r4, r0
     144:       e59f002c        ldr     r0, [pc, #44]   ; 178 <pm_runtime_autosuspend_expiration+0x68>
     148:       e0010c90        mul     r1, r0, ip
     14c:       e0926001        adds    r6, r2, r1
     150:       e0a37fc1        adc     r7, r3, r1, asr #31
     154:       e1550007        cmp     r5, r7
     158:       01540006        cmpeq   r4, r6
     15c:       3a000001        bcc     168 <pm_runtime_autosuspend_expiration+0x58>
     160:       e3a06000        mov     r6, #0
     164:       e3a07000        mov     r7, #0
     168:       e1a00006        mov     r0, r6
     16c:       e1a01007        mov     r1, r7
     170:       e8bd41f0        pop     {r4, r5, r6, r7, r8, lr}
     174:       e12fff1e        bx      lr
     178:       000f4240        .word   0x000f4240

Quoted 16 lines · Ladislav Michl

Quoted 14 lines · Vincent Guittot

Quoted 13 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 6 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Fair enough, so let's go back to:

This one looks good for me

Lets split is for 'comments fix' patch, which was just send and 'the rest'. Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on 32bits arch" or will wait for 5.1. You decide :)

I don't really mind.

Rafael,
Do you prefer to only take the fix for u64 casting problem or do you prefer to also take the optimization below in one single patch ?

Quoted 26 lines · Vincent Guittot

Quoted 23 lines · Ladislav Michl

Quoted 11 lines · Vincent Guittot

Quoted 10 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 4 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Fair enough, so let's go back to:

This one looks good for me

Lets split is for 'comments fix' patch, which was just send and 'the rest'. Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on 32bits arch" or will wait for 5.1. You decide :)

Quoted 5 lines

I don't really mind.

Rafael,
Do you prefer to only take the fix for u64 casting problem or do you prefer to also take the optimization below in one single patch ?

The casting problem is a bug and the optimization is next release material in my view. Please send the optimization separately.

Quoted 34 lines · Rafael J. Wysocki

Quoted 31 lines · Vincent Guittot

Quoted 16 lines · Ladislav Michl

Quoted 14 lines · Vincent Guittot

Quoted 13 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 6 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Fair enough, so let's go back to:

This one looks good for me

Lets split is for 'comments fix' patch, which was just send and 'the rest'. Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on 32bits arch" or will wait for 5.1. You decide :)

I don't really mind.

Rafael,
Do you prefer to only take the fix for u64 casting problem or do you prefer to also take the optimization below in one single patch ?

Quoted 2 lines

The casting problem is a bug and the optimization is next release material in my view. Please send the optimization separately.

Ok, will send that separately in a few moments...
Btw, u64 casting problem seems to be still present in rpm_suspend while computing slack for autosuspend delay greater than 25% of 2^31 (2^33) ns.

Not sure if that triggers any real bug.

Quoted 36 lines · Ladislav Michl

Quoted 34 lines · Rafael J. Wysocki

Quoted 31 lines · Vincent Guittot

Quoted 16 lines · Ladislav Michl

Quoted 14 lines · Vincent Guittot

Quoted 13 lines · Ladislav Michl

Quoted 1 line · Vincent Guittot

Please keep all thread list when replying :-)

Ahh, sorry for that...
[snip]

Quoted 6 lines · Ladislav Michl

Quoted 2 lines

I agree, but it doea all the magic correctly, so you won't need to touch that code is ktime_t changes (I know, unlikely..)

But the current implementation doesn't care of any changes in ktime_t as it uses raw ns

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Fair enough, so let's go back to:

This one looks good for me

Lets split is for 'comments fix' patch, which was just send and 'the rest'. Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on 32bits arch" or will wait for 5.1. You decide :)

I don't really mind.

Rafael,
Do you prefer to only take the fix for u64 casting problem or do you prefer to also take the optimization below in one single patch ?

Quoted 6 lines

Quoted 2 lines

The casting problem is a bug and the optimization is next release material in my view. Please send the optimization separately.

Ok, will send that separately in a few moments...
Btw, u64 casting problem seems to be still present in rpm_suspend while computing slack for autosuspend delay greater than 25% of 2^31 (2^33) ns.

Good point. I will add it to the fix as well

Quoted 1 line

Not sure if that triggers any real bug.