Skip to content

Conversation

@FRASTM
Copy link
Contributor

@FRASTM FRASTM commented Jan 7, 2021

Fix Unintentional integer overflow in the calculation
Integer handling issues (OVERFLOW_BEFORE_WIDEN)
Potentially overflowing expression with a cast

Fixes #29700
Fixes #29697

Signed-off-by: Francois Ramu [email protected]

@github-actions github-actions bot added area: Timer Timer platform: STM32 ST Micro STM32 labels Jan 7, 2021
@r2r0
Copy link
Member

r2r0 commented Jan 8, 2021

Is there a reason why these calculations are done in two different ways (multiplication then division and division then multiplication)?
If we want to have the same results with the same inputs (and do not loose precision) then multiplication should be performed first using 64 bits arithmetic (casting) and then there should be made a division.

@FRASTM
Copy link
Contributor Author

FRASTM commented Jan 11, 2021

with this calculation on 64bit of returned time value, the timer tests can pass on nucleo_wb55rg
tests/kernel/timer/timer_monotonic/
tests/kernel/timer/timer_api

@erwango erwango requested a review from aurel32 January 13, 2021 11:18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look correct to me, but it was already the case before and not due to to you change. If the computation has to be done with precision as in the comment, it should be:
uint64_t ret = ((uint64_t)lp_time * sys_clock_hw_cycles_per_sec()) / LPTIM_CLOCK;

Basically just like the other change. Also note that sys_clock_hw_cycles_per_sec is equivalent to CONFIG_SYS_CLOCK_TICKS_PER_SEC when CONFIG_TIMER_READS_ITS_FREQUENCY_AT_RUNTIME is not defined, which is the case for this driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok,
I did not find any error when testing (10x) tests/kernel/timer/timer_api and tests/kernel/timer/timer_monotonic on nucleo stm32wb55 (when lptim enabled)

@r2r0
Copy link
Member

r2r0 commented Jan 27, 2021

As me and @aurel32 already wrote, dividing before multiplying causes lost of precision.
I.e. consider: sys_clock_hw_cycles_per_sec() = 64000000, LPTIM_CLOCK=32768, lp_time=32768 - the correct result is 64000000, but actual code gives: 63995904
Consider using expression as below, please.

uint64_t ret = ((uint64_t)lp_time * sys_clock_hw_cycles_per_sec()) / LPTIM_CLOCK;

Fix Unintentional integer overflow in the calculation
Integer handling issues (OVERFLOW_BEFORE_WIDEN)
Potentially overflowing expression

Signed-off-by: Francois Ramu <[email protected]>
@r2r0
Copy link
Member

r2r0 commented Jan 27, 2021

Thanks @FRASTM

@erwango erwango requested a review from aurel32 January 27, 2021 19:26
Copy link
Contributor

@aurel32 aurel32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, it looks good to me now.

@galak galak added this to the v2.5.0 milestone Jan 27, 2021
@nashif nashif merged commit 1280e98 into zephyrproject-rtos:master Jan 27, 2021
@FRASTM FRASTM deleted the lptim_coverity branch March 31, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Timer Timer platform: STM32 ST Micro STM32

Projects

None yet

6 participants