Skip to content

Revert 69705 and fix #72344 #72345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 6, 2024
Merged

Revert 69705 and fix #72344 #72345

merged 4 commits into from
May 6, 2024

Conversation

@aescolar
Copy link
Member

@aescolar aescolar commented May 6, 2024 •
edited
Loading

#69705
Introduced a regression in main in which tests/subsys/logging/log_timestamp started failing.

Let's revert the PR. It can be submitted after with the issue fixed.

Fixes #72344

aescolar added 4 commits May 6, 2024 11:00
@aescolar
Revert "kernel: retrieve system timer clock frequency at runtime or s…
c8102e1 
…tatic"

This reverts commit 7c03e5d.

zephyrproject-rtos#69705
Introduced a regression in main in which
tests/subsys/logging/log_timestamp
started failing. (See
zephyrproject-rtos#72344
for more info).
Let's revert the PR. It can be submitted after with the issue
fixed.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar
Revert "board: x86: enable system timer clock frequency at run time"
49e653a 
This reverts commit 8eabf80.

zephyrproject-rtos#69705
Introduced a regression in main in which
tests/subsys/logging/log_timestamp
started failing. (See
zephyrproject-rtos#72344
for more info).
Let's revert the PR. It can be submitted after with the issue
fixed.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar
Revert "tests: kernel: update test case with sys_clock_hw_cycles_per_…
d3bacd3 
…sec"

This reverts commit fd8ca83.

zephyrproject-rtos#69705
Introduced a regression in main in which
tests/subsys/logging/log_timestamp
started failing. (See
zephyrproject-rtos#72344
for more info).
Let's revert the PR. It can be submitted after with the issue
fixed.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar
Revert "drivers: timer: tsc: retrieve clock frequency of system timer…
0a88d64 
… at runtime"

This reverts commit d82f4a9.

zephyrproject-rtos#69705
Introduced a regression in main in which
tests/subsys/logging/log_timestamp
started failing. (See
zephyrproject-rtos#72344
for more info).
Let's revert the PR. It can be submitted after with the issue
fixed.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar aescolar added the Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. label May 6, 2024
@aescolar aescolar requested a review from jhedberg May 6, 2024 09:05
@jhedberg
Copy link
Member

jhedberg commented May 6, 2024

@aescolar did you consider reverting just the one commit that @fabiobaltieri identified as the culprit? I was able to verify that simply reverting it also fixes the issue.

@aescolar
Copy link
Member Author

aescolar commented May 6, 2024

@aescolar did you consider reverting just the one commit that @fabiobaltieri identified as the culprit? I was able to verify that simply reverting it also fixes the issue.

I did, but I was concerned that reverting only that one I would leave something else broken. It just seemed the cleaner and safer thing to do was revert the whole PR; And once the author identifies the cause behind the issue the PR can be resubmitted with it fixed.

jhedberg
jhedberg approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

@aescolar did you consider reverting just the one commit that @fabiobaltieri identified as the culprit? I was able to verify that simply reverting it also fixes the issue.

I did, but I was concerned that reverting only that one I would leave something else broken. It just seemed the cleaner and safer thing to do was revert the whole PR; And once the author identifies the cause behind the issue the PR can be resubmitted with it fixed.

Ok. Fair enough.

@najumon1980
Copy link
Contributor

najumon1980 commented May 6, 2024

The regression is due to following reason:
sys_clock_hw_cycles_per_sec() used in log_core_init() which invoked during cstart (EARLY init) but HPET driver init is PRE_KERNEL_2. So the z_clock_hw_cycles_per_sec need to init with some default CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC (though it is not right) or ideally need to init z_clock_hw_cycles_per_sec run time during cstart (EARLY init).

@aescolar
Copy link
Member Author

aescolar commented May 6, 2024

Thanks @najumon1980 , does that mean you will be able to have a patch ready and passing soon? (in a matter of an hour or so). Otherwise we can just merge the revert and the original PR can be resubmitted without time pressure.

@najumon1980
Copy link
Contributor

najumon1980 commented May 6, 2024

let see I can provide a fix for this

@najumon1980
Copy link
Contributor

najumon1980 commented May 6, 2024

Provided following fix for the regression:
#72352

@aescolar
Copy link
Member Author

aescolar commented May 6, 2024

@najumon1980 The fix in #72352 does not seem too trivial. I'm quite inclined to merge this revert PR and unblock main.
If you don't say otherwise in the next few minutes I'll go ahead and merge this one.

@jhedberg jhedberg assigned jhedberg and unassigned andyross May 6, 2024
@jhedberg
Copy link
Member

jhedberg commented May 6, 2024

@najumon1980 The fix in #72352 does not seem too trivial. I'm quite inclined to merge this revert PR and unblock main. If you don't say otherwise in the next few minutes I'll go ahead and merge this one.

Yes, I think it's better to give the correct fix some more time for review, especially from @andyross, so let's merge the revert first.

@jhedberg jhedberg merged commit 0b87cb1 into zephyrproject-rtos:main May 6, 2024
@aescolar aescolar deleted the revert_69705 branch May 6, 2024 11:52
@andyross
Copy link
Contributor

andyross commented May 6, 2024

Random aside: this is the second time in a week we've seen core tests regress from PR's that didn't see them in CI? Is something getting skipped? Basically how was this not caught?

kartben added a commit to kartben/zephyr that referenced this pull request Dec 4, 2025
@kartben
MAINTAINERS: remove najumon1980 due to inactivity
e52b01e 
Last activity: 18 months ago (review)

Recent activities:

- review: Review on 'x86: ia32: linker: move bss/noinit sections after data
  section'
  zephyrproject-rtos#73044
  (2024-05-22T06:47:31Z)
- pr: kernel: timer: fixes for log_timestamp test case failer due to timer
  PR regression
  zephyrproject-rtos#72352
  (2024-05-06T10:47:53Z)
- review: Review on 'Revert 69705 and fix zephyrproject-rtos#72344'
  zephyrproject-rtos#72345
  (2024-05-06T09:53:31Z)

Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@jhedberg jhedberg jhedberg approved these changes

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@najumon1980 najumon1980 najumon1980 approved these changes

@andyross andyross Awaiting requested review from andyross

@ceolin ceolin Awaiting requested review from ceolin

@cfriedt cfriedt Awaiting requested review from cfriedt

@dcpleung dcpleung Awaiting requested review from dcpleung

@laurenmurphyx64 laurenmurphyx64 Awaiting requested review from laurenmurphyx64

@nashif nashif Awaiting requested review from nashif

@npitre npitre Awaiting requested review from npitre

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@TaiJuWu TaiJuWu Awaiting requested review from TaiJuWu

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@teburd teburd Awaiting requested review from teburd

Assignees

@jhedberg jhedberg

Labels

area: Kernel area: Timer Timer Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. platform: X86 x86 and x86-64

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tests/subsys/logging/log_timestamp/ failing on qemu_x86[_64] on main

7 participants

@aescolar @jhedberg @najumon1980 @andyross @fabiobaltieri @henrikbrixandersen @zephyrbot