Add all definitions from linux/ptp-clock.h #3559

folkertdev · 2024-01-25T15:23:19Z

I've taken over this change from @davidv1992 (who is my colleague). #3353 is the original PR which can now be closed.

I'm also assuming this can/should be merged into the libc-0.2 branch (and main will be rebased on top of that at some point?).

rustbot · 2024-01-25T15:23:24Z

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

folkertdev · 2024-01-25T16:28:40Z

libc-test/build.rs

+ (struct_ == "user_regs" && arm) ||
+ // `anonymous_1` is an anonymous union
+ (struct_ == "ptp_perout_request" && field == "anonymous_1") ||
+ // `anonymous_2` is an anonymous union
+ (struct_ == "ptp_perout_request" && field == "anonymous_2")


I'm not sure why but both this and block and the block above excluding these fields seem required for CI to be satisfied?!

JohnTitor · 2024-01-28T08:42:34Z

Could you resolve CI failure?
@rustbot author

folkertdev · 2024-01-28T14:27:18Z

well that was more work that expected. A bunch of the constants and one struct are defined only in newer kernel versions (this causes problems on the 0.2 branch on musl, which uses an older kernel version)

and then on powerpc a bunch of the constants have different values.

folkertdev · 2024-01-28T15:21:29Z

src/unix/linux_like/linux/mod.rs

+ pub const PTP_CLOCK_GETCAPS: ::c_uint = 0x40503d01;
+ pub const PTP_EXTTS_REQUEST: ::c_uint = 0x80103d02;
+ pub const PTP_PEROUT_REQUEST: ::c_uint = 0x80383d03;
+ pub const PTP_ENABLE_PPS: ::c_uint = 0x80043d04;
+ pub const PTP_SYS_OFFSET: ::c_uint = 0x83403d05;
+ pub const PTP_PIN_GETFUNC: ::c_uint = 0xc0603d06;
+ pub const PTP_PIN_SETFUNC: ::c_uint = 0x80603d07;
+ pub const PTP_SYS_OFFSET_PRECISE: ::c_uint = 0xc0403d08;


these are defined with some macros. There are rust versions here https://github.com/torvalds/linux/blob/8a696a29c6905594e4abf78eaafcb62165ac61f1/rust/kernel/ioctl.rs#L13

should those be used instead?

I think duplicating the macros from ioctl.h would be better here - these numbers are kind of difficult to verify by hand as-is.

folkertdev · 2024-03-07T13:26:53Z

@JohnTitor this is actually waiting for review, but I don't think I can update the label? (not sure how this workflow is supposed to work?)

folkertdev · 2024-03-07T13:30:47Z

ah, rustbot got more helpful in newer PRs

@rustbot review

tgross35 · 2024-08-21T05:09:11Z

libc-test/build.rs

@@ -3457,6 +3457,8 @@ fn test_linux(target: &str) {
 "linux/netlink.h",
 // FIXME: requires Linux >= 5.6:
 [!musl]: "linux/openat2.h",
+ "linux/ptp_clock.h",
+ // FIXME: requires Linux >= 5.6:


What is this referring to? uapi ptp_clock.h has been around since at least 4.0, not sure if this is intended for ptrace.h below but that doesn't make sense either.

#3559 (comment) goes into why. it's not that the file doesn't exist before 5.6, but some of the constants are only added in this later version, so for the full functionality, 5.6 is required. I believe the same is true for netlink.h above, a file that is also very old but its api changed over time, so that is why I copied over that comment too

I was asking mostly about why the FIXME is below ptp_clock.h rather than above, looks like it is referring to ptrace.h rather than ptp_clock.h - but I guess this didn't show up well on the review UI. That context is probably helpful though, maybe change the comment to some fields require Linux >= 5.6

libc-test/build.rs

tgross35 · 2024-08-21T05:15:18Z

src/unix/linux_like/linux/mod.rs

+ #[cfg(target_env = "gnu")]
+ pub adjust_phase: ::c_int,
+ #[cfg(target_env = "gnu")]
+ pub max_phase_adj: ::c_int,
+ #[cfg(target_env = "gnu")]
+ pub rsv: [::c_int; 11],
+ #[cfg(any(target_env = "musl", target_env = "ohos"))]
+ pub rsv: [::c_int; 13],


Could you add a note about how this is a kernel version difference that happens to be exposed by glibc / musl supported versions?

Also if you have links to relevant sources or docs, please add them in the PR description. I haven't been able to find anything relevant.

I'm confused by this now. I suspect that I came to the above after some fighting with CI. Here // https://github.com/torvalds/linux/blob/b311c1b497e51a628aa89e7cb954481e5f9dced2/include/uapi/linux/ptp_clock.h#L94
the max_phase_adj field clearly is defined. But then e.g. on godbolt, for both musl and gnu, that field does not exist. adjust_phase does though. I'll change it back and see what CI says

I'm confused by this now.

At least that makes two of us :)

Oh, I just realized this is targeting 0.2 rather than main. I would prefer this target main first since it uses some newer versions, then for the backport we can work around any kind of legacy weirdness.

Don't close this since it is still useful, but if it isn't too much trouble then it would be good if you could reapply this on top of main and open a new PR with that.

(If it's a PITA don't worry about it and I'll figure out how to get things in sync after)

worked out fine #3865

src/unix/linux_like/linux/mod.rs

tgross35 · 2024-08-21T05:24:56Z

src/unix/linux_like/linux/mod.rs

+ impl PartialEq for ptp_sys_offset {
+ fn eq(&self, other: &ptp_sys_offset) -> bool {
+ self.n_samples == other.n_samples &&
+ self.rsv == other.rsv &&
+ self.ts[..] == other.ts[..]
+ }
+ }
+ impl Eq for ptp_sys_offset {}
+ impl ::fmt::Debug for ptp_sys_offset {
+ fn fmt(&self, f: &mut ::fmt::Formatter) -> ::fmt::Result {
+ f.debug_struct("ptp_sys_offset")
+ .field("n_samples", &self.n_samples)
+ .field("rsv", &self.rsv)
+ .field("ts", &&self.ts[..])
+ .finish()
+ }
+ }
+ impl ::hash::Hash for ptp_sys_offset {
+ fn hash<H: ::hash::Hasher>(&self, state: &mut H) {
+ self.n_samples.hash(state);
+ self.rsv.hash(state);
+ self.ts[..].hash(state);
+ }
+ }


Why not just move this struct from s_no_extra_traits! to s! so it gets these derived? Same for ptp_pin_desc

tgross35 · 2024-08-21T05:29:11Z

src/unix/linux_like/linux/mod.rs

+ pub const PTP_CLOCK_GETCAPS: ::c_uint = 0x40503d01;
+ pub const PTP_EXTTS_REQUEST: ::c_uint = 0x80103d02;
+ pub const PTP_PEROUT_REQUEST: ::c_uint = 0x80383d03;
+ pub const PTP_ENABLE_PPS: ::c_uint = 0x80043d04;
+ pub const PTP_SYS_OFFSET: ::c_uint = 0x83403d05;
+ pub const PTP_PIN_GETFUNC: ::c_uint = 0xc0603d06;
+ pub const PTP_PIN_SETFUNC: ::c_uint = 0x80603d07;
+ pub const PTP_SYS_OFFSET_PRECISE: ::c_uint = 0xc0403d08;


I think duplicating the macros from ioctl.h would be better here - these numbers are kind of difficult to verify by hand as-is.

src/unix/linux_like/linux/mod.rs

tgross35 · 2024-08-21T05:30:37Z

Left a review above, mostly nits with some follow up questions.

@rustbot author

rustbot · 2024-08-21T22:04:35Z

Some changes occurred in OpenBSD module

cc @semarie

Some changes occurred in solarish module

cc @jclulow, @pfmooney

Some changes occurred in OpenBSD module

cc @semarie

Co-authored-by: Trevor Gross <[email protected]>

adopted from RFL

tgross35 · 2024-08-28T20:08:41Z

Considering there are some CI failures here and #3865 exists now, let's figure this out on main first then backport as applicable.

rustbot assigned JohnTitor Jan 25, 2024

rustbot added the S-waiting-on-review label Jan 25, 2024

folkertdev force-pushed the add-ptp-clock branch 4 times, most recently from b8f48dc to 543faf9 Compare January 25, 2024 16:27

folkertdev commented Jan 25, 2024

View reviewed changes

davidv1992 mentioned this pull request Jan 26, 2024

Added all definitions from linux/ptp-clock.h #3353

Closed

JohnTitor force-pushed the add-ptp-clock branch from 543faf9 to c57f452 Compare January 28, 2024 05:32

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Jan 28, 2024

folkertdev force-pushed the add-ptp-clock branch 3 times, most recently from b778e8f to 5ac4d4f Compare January 28, 2024 14:25

folkertdev commented Jan 28, 2024

View reviewed changes

folkertdev force-pushed the add-ptp-clock branch 3 times, most recently from d9c8b36 to b087c94 Compare January 28, 2024 17:18

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Mar 7, 2024

tgross35 reviewed Aug 21, 2024

View reviewed changes

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Aug 21, 2024

folkertdev force-pushed the add-ptp-clock branch from f74db2b to 2773e96 Compare August 21, 2024 22:04

davidv1992 and others added 6 commits August 22, 2024 00:05

Added all definitions from linux/ptp-clock.h

f66e60e

Apply suggestions from code review

aa5120f

Co-authored-by: Trevor Gross <[email protected]>

use PTP_MAX_SAMPLES in struct definitions

3073940

derive traits where possible

d561984

adjust ptp_clock_caps to older version

42d149b

use a _IOC const fn to generate constants

e23acab

adopted from RFL

folkertdev force-pushed the add-ptp-clock branch from 2773e96 to e23acab Compare August 21, 2024 22:05

folkertdev mentioned this pull request Aug 21, 2024

Add all definitions from linux/ptp-clock.h #3865

Open

tgross35 closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all definitions from linux/ptp-clock.h #3559

Add all definitions from linux/ptp-clock.h #3559

folkertdev commented Jan 25, 2024

rustbot commented Jan 25, 2024

folkertdev Jan 25, 2024

JohnTitor commented Jan 28, 2024

folkertdev commented Jan 28, 2024

folkertdev Jan 28, 2024

tgross35 Aug 21, 2024

folkertdev commented Mar 7, 2024

folkertdev commented Mar 7, 2024

tgross35 Aug 21, 2024

folkertdev Aug 21, 2024

tgross35 Aug 21, 2024

tgross35 Aug 21, 2024

folkertdev Aug 21, 2024

tgross35 Aug 21, 2024

tgross35 Aug 21, 2024

folkertdev Aug 21, 2024

tgross35 Aug 21, 2024

tgross35 Aug 21, 2024

tgross35 commented Aug 21, 2024

rustbot commented Aug 21, 2024

tgross35 commented Aug 28, 2024

Add all definitions from linux/ptp-clock.h #3559

Add all definitions from linux/ptp-clock.h #3559

Conversation

folkertdev commented Jan 25, 2024

rustbot commented Jan 25, 2024

Choose a reason for hiding this comment

JohnTitor commented Jan 28, 2024

folkertdev commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folkertdev commented Mar 7, 2024

folkertdev commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 commented Aug 21, 2024

rustbot commented Aug 21, 2024

tgross35 commented Aug 28, 2024