Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On Wed, 2026-07-29 at 00:16 +, Arthur Kiyanovski wrote: > > > if (ptp->info->gettimexattrs64) > > err = ptp->info->gettimexattrs64(ptp->info, &ts, &sts, &att); > > else if (ptp->info->gettimex64) > > err = ptp->info->gettimex64(ptp->info, &ts, &sts); > > else > > return -EOPNOTSUPP; > > > > A userspace that respects the capability bits would refuse to call the > > new ioctl on drivers that would in fact serve it via the fallback. Is > > the fallback intended, and if so, should the caps advertise it? The > > commit message does not describe this behavior. > > > > The fallback is intentional. I'll widen the caps in v6 so they advertise it > (gettimexattrs64 || gettimex64, getcrosststampattrs || getcrosststamp) and > document the behavior in the commit message. Thanks. FWIW I kind of hate this pattern. I understand it's how PTP does things, so I'm not suggesting you change it now, but these are all *kernel* functions not userspace API, so why not just add the new argument to the existing function rather than adding a new function? In the linux-phc rework it basically all collapses down to *one* function that's needed to read a clock, with a bunch of optional arguments. (Which I guess means that in a very short period of time, I'll stop caring how awful it is in the PTP code...) smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On 2026-07-27 13:15:14-07:00, Jakub Kicinski wrote: > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > --- > ptp: Add ioctls for PHC timestamps with quality attributes > > This introduces PTP_SYS_OFFSET_EXTENDED_ATTRS and > PTP_SYS_OFFSET_PRECISE_ATTRS, extending the existing extended/precise > offset ioctls with clock quality attributes (error bound, timescale, > sync status) and raw system counter fields. New driver callbacks > gettimexattrs64 and getcrosststampattrs are added to ptp_clock_info. > > Link: https://lore.kernel.org/netdev/[email protected]/ > Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/ > > A few questions inline below. > > > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > > index dc23cd708cfe..d30ee4f51fbb 100644 > > --- a/drivers/ptp/ptp_chardev.c > > +++ b/drivers/ptp/ptp_chardev.c > > @@ -190,6 +190,8 @@ static long ptp_clock_getcaps(struct ptp_clock *ptp, > > void __user *arg) > > .cross_timestamping = ptp->info->getcrosststamp != NULL, > > .adjust_phase = ptp->info->adjphase != NULL && > > ptp->info->getmaxphase != NULL, > > + .extended_attrs = ptp->info->gettimexattrs64 != NULL, > > + .precise_attrs = ptp->info->getcrosststampattrs != > > NULL, > > }; > > [Medium] > Are the capability bits consistent with what the ioctls will actually > accept? > > The caps are set based only on the presence of the new *_attrs callbacks, > but the ioctl handlers fall back to gettimex64/getcrosststamp when the > *_attrs callback is absent: > > if (ptp->info->gettimexattrs64) > err = ptp->info->gettimexattrs64(ptp->info, &ts, &sts, &att); > else if (ptp->info->gettimex64) > err = ptp->info->gettimex64(ptp->info, &ts, &sts); > else > return -EOPNOTSUPP; > > A userspace that respects the capability bits would refuse to call the > new ioctl on drivers that would in fact serve it via the fallback. Is > the fallback intended, and if so, should the caps advertise it? The > commit message does not describe this behavior. > The fallback is intentional. I'll widen the caps in v6 so they advertise it (gettimexattrs64 || gettimex64, getcrosststampattrs || getcrosststamp) and document the behavior in the commit message. Thanks. > [ ... ] > > > @@ -404,6 +413,130 @@ static long ptp_sys_offset_extended(struct ptp_clock > > *ptp, void __user *arg, > > return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0; > > } > > > > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void > > __user *arg) > > +{ > > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL; > > + struct ptp_attrs_request request; > > + struct ptp_system_timestamp sts; > > [Medium] > Can this leak uninitialized stack contents to userspace? > > Only sts.clockid is initialized below, and sts is reused across loop > iterations. If any driver's gettimex64/gettimexattrs64 returns success > without touching pre_sts/post_sts on the sts pointer, sts.pre_sts.valid > is either uninitialized (first iteration) or stale (later iterations), > and the "if (!sts.pre_sts.valid || !sts.post_sts.valid)" gate can admit > a partially-populated snapshot. > > On the success path the code then copies pre_sts.cycles, pre_sts.cs_id, > pre_sts.monoraw, pre_sts.systime and the post_sts equivalents to > userspace. The pre-existing ptp_sys_offset_extended() has the same > shape but only copied systime, so the exposed surface is now wider. > Would something like: > > struct ptp_system_timestamp sts = {}; > > inside the loop (or at declaration) be safer? > Yes — I'll zero-initialize sts per loop iteration in v6, so nothing uninitialized or stale can reach userspace. Thanks. > > + unsigned int n_samples; > > + int err; > > + > > + if (copy_from_user(&request, arg, sizeof(request))) > > + return -EFAULT; > > + > > + if (request.valid || > > + request.num_samples > PTP_MAX_SAMPLES || > > + request.num_samples == 0) > > + return -EINVAL; > > [Medium] > Should this also reject non-zero request.rsv[]? > > The kernel-doc for struct ptp_attrs_request says "Reserved for future > use, must be zero", but rsv[3] is never validated. The neighboring > ptp_sys_offset_extended() enforces the same contract on its own > reserved fields: > > if (extoff->n_samples > PTP_MAX_SAMPLES || extoff->rsv[0] || > extoff->rsv[1]) > return -EINVAL; > > Without a similar check, existing userspace binaries can start sending > garbage in rsv[], which then blocks any future repurposing of those > bytes. > Agreed — v6 will reject a non-zero rsv[], matching ptp_sys_offset_extended(). Thanks. > > + > > + err = ptp_validate_sys_offset_clockid(request.clock_id); > > + if (err) > > + return
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On 2026-07-27 10:03:28+01:00, David Woodhouse wrote: > On Fri, 2026-07-17 at 06:59 +, Arthur Kiyanovski wrote: > > > Introduce two new ioctls that extend existing PTP timestamp interfaces > > with clock quality information: > > > > - PTP_SYS_OFFSET_EXTENDED_ATTRS: Extends PTP_SYS_OFFSET_EXTENDED > > - PTP_SYS_OFFSET_PRECISE_ATTRS: Extends PTP_SYS_OFFSET_PRECISE > > > > These ioctls provide quality attributes alongside timestamps: > > > > 1. error_bound: Maximum deviation from true time (nanoseconds), based > >on device's internal clock state > > 2. clock_status: Synchronization state (unknown, initializing, > >synchronized, free-running, unreliable) > > 3. timescale: Time reference (TAI, UTC, etc.) > > 4. counter_value: Raw system counter (e.g. TSC ticks) captured by the > >timekeeping core alongside each system timestamp > > 5. counter_id: Identifies the counter source (e.g. TSC, ARM arch counter) > > > > This supports three use cases: > > > > 1. Managed PHC devices (e.g., ENA, vmclock) that maintain their own > >synchronization and can report quality metrics directly to userspace > >without requiring ptp4l > > > > 2. Applications that need complete time quality information in a single > >call, regardless of how the PHC is synchronized > > > > 3. VMMs that need raw system counter values paired > >with PTP timestamps for feed-forward clock calibration, avoiding the > >feedback loop inherent in NTP-style synchronization > > > > Timescale definitions use a Continuity/Discipline framework to describe > > timeline properties and steering behavior consistently across all > > entries. > > > > This implementation is based on the original RFC and the UAPI design > > discussion linked below. > > > > Link: > > https://lore.kernel.org/netdev/[email protected]/ > > Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/ > > Signed-off-by: Amit Bernstein > > Signed-off-by: Arthur Kiyanovski > > ... > > > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void > > __user *arg) > > +{ > > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL; > > + struct ptp_attrs_request request; > > + struct ptp_system_timestamp sts; > > + unsigned int n_samples; > > + int err; > > + > > + if (copy_from_user(&request, arg, sizeof(request))) > > + return -EFAULT; > > + > > + if (request.valid || > > + request.num_samples > PTP_MAX_SAMPLES || > > + request.num_samples == 0) > > + return -EINVAL; > > + > > + err = ptp_validate_sys_offset_clockid(request.clock_id); > > + if (err) > > + return err; > > + > > + n_samples = request.num_samples; > > + sts.clockid = request.clock_id; > > + > > + data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->request.num_samples = n_samples; > > + > > + for (unsigned int i = 0; i < n_samples; i++) { > > + struct ptp_clock_attrs att = {}; > > + struct timespec64 ts; > > + > > + if (ptp->info->gettimexattrs64) > > + err = ptp->info->gettimexattrs64(ptp->info, &ts, > > +&sts, &att); > > + else if (ptp->info->gettimex64) > > + err = ptp->info->gettimex64(ptp->info, &ts, &sts); > > + else > > + return -EOPNOTSUPP; > > + > > + if (err) > > + return err; > > + > > + /* Filter out disabled or unavailable clocks */ > > + if (!sts.pre_sts.valid || !sts.post_sts.valid) > > + return -EINVAL; > > + > > + data->timestamps[i].pre_systime.sys_time = > > + ktime_to_ns(sts.pre_sts.systime); > > + data->timestamps[i].pre_systime.sys_rawtime = > > + ktime_to_ns(sts.pre_sts.monoraw); > > + data->timestamps[i].pre_systime.sys_counter = > > + sts.pre_sts.cycles; > > I hate all this line wrapping, btw. I'll defer to the net coding style > if they insist, but my preference would just be just to have longer > lines. Especially when it's a block of assignments like this, the > wrapped form is *much* harder to read. > Agreed — I'll reduce the wrapping in v6 (a local temp var + a small helper make that block read much better). Thanks. > > + data->timestamps[i].pre_systime.sys_counter_id = > > + sts.pre_sts.cs_id; > > You added PTP_COUNTER_* constants as we discussed¹... but didn't you > forget to *map* to them here? > > ¹ https://lore.kernel.org/all/877boqtg3g.ffs@tglx/ > Good catch — you're right, that's a bug. I'll add the csid→PTP_COUNTER_* mapping in v6. > > Arguably, if the timestamps have a hw_csid then you should be returning > that in place of the main one which will be kvmclock or hyperv > nonsense. You can do that here for extended_attrs but
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- ptp: Add ioctls for PHC timestamps with quality attributes This introduces PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS, extending the existing extended/precise offset ioctls with clock quality attributes (error bound, timescale, sync status) and raw system counter fields. New driver callbacks gettimexattrs64 and getcrosststampattrs are added to ptp_clock_info. Link: https://lore.kernel.org/netdev/[email protected]/ Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/ A few questions inline below. > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index dc23cd708cfe..d30ee4f51fbb 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -190,6 +190,8 @@ static long ptp_clock_getcaps(struct ptp_clock *ptp, void > __user *arg) > .cross_timestamping = ptp->info->getcrosststamp != NULL, > .adjust_phase = ptp->info->adjphase != NULL && > ptp->info->getmaxphase != NULL, > + .extended_attrs = ptp->info->gettimexattrs64 != NULL, > + .precise_attrs = ptp->info->getcrosststampattrs != > NULL, > }; [Medium] Are the capability bits consistent with what the ioctls will actually accept? The caps are set based only on the presence of the new *_attrs callbacks, but the ioctl handlers fall back to gettimex64/getcrosststamp when the *_attrs callback is absent: if (ptp->info->gettimexattrs64) err = ptp->info->gettimexattrs64(ptp->info, &ts, &sts, &att); else if (ptp->info->gettimex64) err = ptp->info->gettimex64(ptp->info, &ts, &sts); else return -EOPNOTSUPP; A userspace that respects the capability bits would refuse to call the new ioctl on drivers that would in fact serve it via the fallback. Is the fallback intended, and if so, should the caps advertise it? The commit message does not describe this behavior. [ ... ] > @@ -404,6 +413,130 @@ static long ptp_sys_offset_extended(struct ptp_clock > *ptp, void __user *arg, > return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0; > } > > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user > *arg) > +{ > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL; > + struct ptp_attrs_request request; > + struct ptp_system_timestamp sts; [Medium] Can this leak uninitialized stack contents to userspace? Only sts.clockid is initialized below, and sts is reused across loop iterations. If any driver's gettimex64/gettimexattrs64 returns success without touching pre_sts/post_sts on the sts pointer, sts.pre_sts.valid is either uninitialized (first iteration) or stale (later iterations), and the "if (!sts.pre_sts.valid || !sts.post_sts.valid)" gate can admit a partially-populated snapshot. On the success path the code then copies pre_sts.cycles, pre_sts.cs_id, pre_sts.monoraw, pre_sts.systime and the post_sts equivalents to userspace. The pre-existing ptp_sys_offset_extended() has the same shape but only copied systime, so the exposed surface is now wider. Would something like: struct ptp_system_timestamp sts = {}; inside the loop (or at declaration) be safer? > + unsigned int n_samples; > + int err; > + > + if (copy_from_user(&request, arg, sizeof(request))) > + return -EFAULT; > + > + if (request.valid || > + request.num_samples > PTP_MAX_SAMPLES || > + request.num_samples == 0) > + return -EINVAL; [Medium] Should this also reject non-zero request.rsv[]? The kernel-doc for struct ptp_attrs_request says "Reserved for future use, must be zero", but rsv[3] is never validated. The neighboring ptp_sys_offset_extended() enforces the same contract on its own reserved fields: if (extoff->n_samples > PTP_MAX_SAMPLES || extoff->rsv[0] || extoff->rsv[1]) return -EINVAL; Without a similar check, existing userspace binaries can start sending garbage in rsv[], which then blocks any future repurposing of those bytes. > + > + err = ptp_validate_sys_offset_clockid(request.clock_id); > + if (err) > + return err; > + > + n_samples = request.num_samples; > + sts.clockid = request.clock_id; > + > + data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->request.num_samples = n_samples; [Low] This is a _IOWR ioctl and the whole struct (including data->request) is copied back to userspace at the end, but only data->request.num_samples is populated. data->request.valid, data->request.clock_id and data->request.rsv[] therefore always come back as zero, silently overwriting the values userspace put in the request buffer. Is that intentional? It is not mentioned in
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On Mon, 27 Jul 2026 14:32:23 +0100 David Woodhouse wrote: > > > I hate all this line wrapping, btw. I'll defer to the net coding style > > > if they insist, but my preference would just be just to have longer > > > lines. Especially when it's a block of assignments like this, the > > > wrapped form is *much* harder to read. > > > > This is a long function, probably longer than the coding style > > suggests. So one option is to move the code within the for loop into a > > helper. That might then allow unwrapped lines? > > > > That is kind of the point of the line length limit, to make you break > > code up into lots of small functions which do one thing. > > Huh? Are we looking at the same function? The version of > ptp_sys_offset_extended_attrs() I'm looking at, having fixed up the > gratuitous line wrapping, is about 60 lines — none of which are wider > than 100 characters. It's a simple setup and then loop over n_samples > of data captures. A helper function would only serve to obfuscate it. 1. temporary variables for long deref chains help both readability and the compiler 2. our 80 char rule is obviously negotiable. But more often than not the best solution is to refactor the code.
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On Mon, 2026-07-27 at 15:15 +0200, Andrew Lunn wrote:
>
> > I hate all this line wrapping, btw. I'll defer to the net coding style
> > if they insist, but my preference would just be just to have longer
> > lines. Especially when it's a block of assignments like this, the
> > wrapped form is *much* harder to read.
>
> This is a long function, probably longer than the coding style
> suggests. So one option is to move the code within the for loop into a
> helper. That might then allow unwrapped lines?
>
> That is kind of the point of the line length limit, to make you break
> code up into lots of small functions which do one thing.
Huh? Are we looking at the same function? The version of
ptp_sys_offset_extended_attrs() I'm looking at, having fixed up the
gratuitous line wrapping, is about 60 lines — none of which are wider
than 100 characters. It's a simple setup and then loop over n_samples
of data captures. A helper function would only serve to obfuscate it.
static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user
*arg)
{
struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
struct ptp_attrs_request request;
struct ptp_system_timestamp sts;
unsigned int n_samples;
int err;
if (copy_from_user(&request, arg, sizeof(request)))
return -EFAULT;
if (request.valid ||
request.num_samples > PTP_MAX_SAMPLES ||
request.num_samples == 0)
return -EINVAL;
err = ptp_validate_sys_offset_clockid(request.clock_id);
if (err)
return err;
n_samples = request.num_samples;
sts.clockid = request.clock_id;
data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL);
if (!data)
return -ENOMEM;
data->request.num_samples = n_samples;
for (unsigned int i = 0; i < n_samples; i++) {
struct ptp_clock_attrs att = {};
struct timespec64 ts;
if (ptp->info->gettimexattrs64)
err = ptp->info->gettimexattrs64(ptp->info, &ts,
&sts, &att);
else if (ptp->info->gettimex64)
err = ptp->info->gettimex64(ptp->info, &ts, &sts);
else
return -EOPNOTSUPP;
if (err)
return err;
/* Filter out disabled or unavailable clocks */
if (!sts.pre_sts.valid || !sts.post_sts.valid)
return -EINVAL;
data->timestamps[i].pre_systime.sys_time =
ktime_to_ns(sts.pre_sts.systime);
data->timestamps[i].pre_systime.sys_rawtime =
ktime_to_ns(sts.pre_sts.monoraw);
data->timestamps[i].pre_systime.sys_counter =
sts.pre_sts.cycles;
data->timestamps[i].pre_systime.sys_counter_id =
sts.pre_sts.cs_id;
data->timestamps[i].devtime.device_time.sec = ts.tv_sec;
data->timestamps[i].devtime.device_time.nsec = ts.tv_nsec;
data->timestamps[i].devtime.attrs = att;
data->timestamps[i].post_systime.sys_time =
ktime_to_ns(sts.post_sts.systime);
data->timestamps[i].post_systime.sys_rawtime =
ktime_to_ns(sts.post_sts.monoraw);
data->timestamps[i].post_systime.sys_counter =
sts.post_sts.cycles;
data->timestamps[i].post_systime.sys_counter_id =
sts.post_sts.cs_id;
}
return copy_to_user(arg, data,
struct_size(data, timestamps, n_samples)) ? -EFAULT
: 0;
}
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
> > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void
> > __user *arg)
> > +{
> > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
> > + struct ptp_attrs_request request;
> > + struct ptp_system_timestamp sts;
> > + unsigned int n_samples;
> > + int err;
> > +
> > + if (copy_from_user(&request, arg, sizeof(request)))
> > + return -EFAULT;
> > +
> > + if (request.valid ||
> > + request.num_samples > PTP_MAX_SAMPLES ||
> > + request.num_samples == 0)
> > + return -EINVAL;
> > +
> > + err = ptp_validate_sys_offset_clockid(request.clock_id);
> > + if (err)
> > + return err;
> > +
> > + n_samples = request.num_samples;
> > + sts.clockid = request.clock_id;
> > +
> > + data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->request.num_samples = n_samples;
> > +
> > + for (unsigned int i = 0; i < n_samples; i++) {
> > + struct ptp_clock_attrs att = {};
> > + struct timespec64 ts;
> > +
> > + if (ptp->info->gettimexattrs64)
> > + err = ptp->info->gettimexattrs64(ptp->info, &ts,
> > +&sts, &att);
> > + else if (ptp->info->gettimex64)
> > + err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> > + else
> > + return -EOPNOTSUPP;
> > +
> > + if (err)
> > + return err;
> > +
> > + /* Filter out disabled or unavailable clocks */
> > + if (!sts.pre_sts.valid || !sts.post_sts.valid)
> > + return -EINVAL;
> > +
> > + data->timestamps[i].pre_systime.sys_time =
> > + ktime_to_ns(sts.pre_sts.systime);
> > + data->timestamps[i].pre_systime.sys_rawtime =
> > + ktime_to_ns(sts.pre_sts.monoraw);
> > + data->timestamps[i].pre_systime.sys_counter =
> > + sts.pre_sts.cycles;
>
> I hate all this line wrapping, btw. I'll defer to the net coding style
> if they insist, but my preference would just be just to have longer
> lines. Especially when it's a block of assignments like this, the
> wrapped form is *much* harder to read.
This is a long function, probably longer than the coding style
suggests. So one option is to move the code within the for loop into a
helper. That might then allow unwrapped lines?
That is kind of the point of the line length limit, to make you break
code up into lots of small functions which do one thing.
Andrew
Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes
On Fri, 2026-07-17 at 06:59 +, Arthur Kiyanovski wrote: > Introduce two new ioctls that extend existing PTP timestamp interfaces > with clock quality information: > > - PTP_SYS_OFFSET_EXTENDED_ATTRS: Extends PTP_SYS_OFFSET_EXTENDED > - PTP_SYS_OFFSET_PRECISE_ATTRS: Extends PTP_SYS_OFFSET_PRECISE > > These ioctls provide quality attributes alongside timestamps: > > 1. error_bound: Maximum deviation from true time (nanoseconds), based >on device's internal clock state > 2. clock_status: Synchronization state (unknown, initializing, >synchronized, free-running, unreliable) > 3. timescale: Time reference (TAI, UTC, etc.) > 4. counter_value: Raw system counter (e.g. TSC ticks) captured by the >timekeeping core alongside each system timestamp > 5. counter_id: Identifies the counter source (e.g. TSC, ARM arch counter) > > This supports three use cases: > > 1. Managed PHC devices (e.g., ENA, vmclock) that maintain their own >synchronization and can report quality metrics directly to userspace >without requiring ptp4l > > 2. Applications that need complete time quality information in a single >call, regardless of how the PHC is synchronized > > 3. VMMs that need raw system counter values paired >with PTP timestamps for feed-forward clock calibration, avoiding the >feedback loop inherent in NTP-style synchronization > > Timescale definitions use a Continuity/Discipline framework to describe > timeline properties and steering behavior consistently across all > entries. > > This implementation is based on the original RFC and the UAPI design > discussion linked below. > > Link: https://lore.kernel.org/netdev/[email protected]/ > Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/ > Signed-off-by: Amit Bernstein > Signed-off-by: Arthur Kiyanovski ... > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user > *arg) > +{ > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL; > + struct ptp_attrs_request request; > + struct ptp_system_timestamp sts; > + unsigned int n_samples; > + int err; > + > + if (copy_from_user(&request, arg, sizeof(request))) > + return -EFAULT; > + > + if (request.valid || > + request.num_samples > PTP_MAX_SAMPLES || > + request.num_samples == 0) > + return -EINVAL; > + > + err = ptp_validate_sys_offset_clockid(request.clock_id); > + if (err) > + return err; > + > + n_samples = request.num_samples; > + sts.clockid = request.clock_id; > + > + data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->request.num_samples = n_samples; > + > + for (unsigned int i = 0; i < n_samples; i++) { > + struct ptp_clock_attrs att = {}; > + struct timespec64 ts; > + > + if (ptp->info->gettimexattrs64) > + err = ptp->info->gettimexattrs64(ptp->info, &ts, > + &sts, &att); > + else if (ptp->info->gettimex64) > + err = ptp->info->gettimex64(ptp->info, &ts, &sts); > + else > + return -EOPNOTSUPP; > + > + if (err) > + return err; > + > + /* Filter out disabled or unavailable clocks */ > + if (!sts.pre_sts.valid || !sts.post_sts.valid) > + return -EINVAL; > + > + data->timestamps[i].pre_systime.sys_time = > + ktime_to_ns(sts.pre_sts.systime); > + data->timestamps[i].pre_systime.sys_rawtime = > + ktime_to_ns(sts.pre_sts.monoraw); > + data->timestamps[i].pre_systime.sys_counter = > + sts.pre_sts.cycles; I hate all this line wrapping, btw. I'll defer to the net coding style if they insist, but my preference would just be just to have longer lines. Especially when it's a block of assignments like this, the wrapped form is *much* harder to read. > + data->timestamps[i].pre_systime.sys_counter_id = > + sts.pre_sts.cs_id; You added PTP_COUNTER_* constants as we discussed¹... but didn't you forget to *map* to them here? ¹ https://lore.kernel.org/all/877boqtg3g.ffs@tglx/ Arguably, if the timestamps have a hw_csid then you should be returning that in place of the main one which will be kvmclock or hyperv nonsense. You can do that here for extended_attrs but precise_attrs will need a bit more work (as system_device_crosststamp doesn't have the hw_* fields). I won't randomise you further by asking you to handle that though. I'll do it in a follow-up. You can either map KVMCLOCK to unknown or give it a PTP_COUNTER_KVMCLOCK if you prefer. I suggest the former, as nobody who cares about time should be using kvmclock. And it means my promised follow-up will be providing counter data
