kernel: sched: Use k_ticks_t in z_tick_sleep #29343
Conversation
|
@andyross any idea how to exercise this issue ? I reproduced the problem hardcoding |
c739e3e to
94804f7
Compare
kernel/sched.c
Outdated
| @@ -1258,7 +1262,7 @@ static int32_t z_tick_sleep(int32_t ticks) | |||
| timeout = Z_TIMEOUT_TICKS(ticks); | |||
| #else | |||
| ticks += _TICK_ALIGN; | |||
| timeout = (k_ticks_t) ticks; | |||
| timeout = ticks; | |||
There was a problem hiding this comment.
Should this also be Z_TIMEOUT_TICKS(ticks) rather than casting the int to a struct
There was a problem hiding this comment.
yep, I missed that we had it to legacy api, just changing it. Thanks for pointing it out.
There was a problem hiding this comment.
The initial change was correct. There was no casting to structure, because modified line was executed when CONFIG_LEGACY_TIMEOUT_API=y.
Fix in #30842.
z_tick_sleep was using int32_t what could cause a possible overflow when converting from k_ticks_t. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
|
@andyross could you take a look on it ? |
There was a problem hiding this comment.
We recently encountered this overflow issue in our application when using k_sleep(K_TIMEOUT_ABS_MS(...)); and I was very happy to find this PR. Thanks for the fix.
I tested it over the weekend an can confirm that it works. Just got one suggestion for more clear variable naming, so that we don't confuse timeouts with ticks.
Change a variable name to avoid confusion between time and ticks. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
| /* LOG subsys does not handle 64-bit values | ||
| * https://github.com/zephyrproject-rtos/zephyr/issues/26246 | ||
| */ | ||
| LOG_DBG("thread %p for %u ticks", _current, ticks); |
There was a problem hiding this comment.
Why bother with %u vs. %d? Both are lossy in this circumstance.
z_tick_sleep was using int32_t what could cause a possible overflow
when converting from k_ticks_t.
Fixes #29066