Skip to content
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

net: ptp: Misc clock sync bugs #75611

Merged
merged 2 commits into from
Aug 6, 2024
Merged

net: ptp: Misc clock sync bugs #75611

merged 2 commits into from
Aug 6, 2024
+13 −2

Conversation

kartben
Copy link
Collaborator

@kartben kartben commented Jul 8, 2024

  • Offset should be subtracted from current clock value, not added.
  • net_ptp_time artithmetic is wrong in ptp_clock_synchronize()

@kartben
net: ptp: Adjust clock using correct offset
9fad6b4 
Offset should be *subtracted* from current clock value, not added.
This was causing clock to accumulate error instead of actually
"converging".

Signed-off-by: Benjamin Cabé <[email protected]>
@kartben kartben added the bug The issue is a bug, or the PR is fixing a bug label Jul 8, 2024
@kartben kartben added this to the v3.7.0 milestone Jul 8, 2024
@kartben kartben requested review from jukkar, pdgendt and rlubos July 8, 2024 17:16
@kartben
Copy link
Collaborator Author

kartben commented Jul 8, 2024

BTW it looks like it would probably be a good thing to check the return values of the calls to ptp_clock_set and ptp_clock_adjust as my guess is the former probably fails when called with an invalid ts.nsec value (greater than NSEC_PER_SEC`) for most drivers.

awojasinski
awojasinski previously approved these changes Jul 9, 2024
View reviewed changes
rlubos
rlubos previously approved these changes Jul 9, 2024
View reviewed changes
@awojasinski
Copy link
Collaborator

BTW it looks like it would probably be a good thing to check the return values of the calls to ptp_clock_set and ptp_clock_adjust as my guess is the former probably fails when called with an invalid ts.nsec value (greater than NSEC_PER_SEC`) for most drivers.

@kartben first of all thank you for the contribution and time you spent on finding the bug. Regarding checking return values from ptp_clock driver, it could be added for debugging purposes (logging some message when it fails). I'm not sure if handling failure and passing this info to ptp_clock_synchronize() caller is necessary.

jukkar
jukkar requested changes Jul 10, 2024
View reviewed changes
subsys/net/lib/ptp/clock.c Outdated Show resolved Hide resolved
@nashif nashif removed this from the v3.7.0 milestone Jul 16, 2024
@nashif nashif added the backport v3.7-branch Request backport to the v3.7-branch label Jul 16, 2024
@awojasinski awojasinski requested a review from jukkar August 5, 2024 08:40
jukkar
jukkar reviewed Aug 5, 2024
View reviewed changes
subsys/net/lib/ptp/clock.c Outdated Show resolved Hide resolved
subsys/net/lib/ptp/clock.c Outdated Show resolved Hide resolved
@kartben @awojasinski
net: ptp: Properly handle second overflow/underflow
bbaacb1 
Fixes issues with net_ptp_time arithmetic where second
overflow/underflow would not be handled properly.

Signed-off-by: Benjamin Cabé <[email protected]>
Signed-off-by: Adam Wojasinski <[email protected]>
@awojasinski awojasinski dismissed stale reviews from rlubos and themself via bbaacb1 August 5, 2024 10:06
@fabiobaltieri fabiobaltieri merged commit 43b948f into zephyrproject-rtos:main Aug 6, 2024
24 checks passed
@zephyrbot zephyrbot mentioned this pull request Aug 6, 2024
Merged
@awojasinski awojasinski mentioned this pull request Aug 6, 2024
Closed
@kartben kartben deleted the more_ptp_fixes branch August 16, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar approved these changes

@rlubos rlubos rlubos approved these changes

@awojasinski awojasinski awojasinski approved these changes

@pdgendt pdgendt Awaiting requested review from pdgendt

Assignees

@awojasinski awojasinski

Labels
area: Networking backport v3.7-branch Request backport to the v3.7-branch bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kartben @awojasinski @jukkar @rlubos @nashif @fabiobaltieri @zephyrbot