Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 22/08/2024 13:26, Vadim Fedorenko wrote:
> On 02/05/2024 22:10, Mahesh Bandewar wrote:
>> The ability to read the PHC (Physical Hardware Clock) alongside
>> multiple system clocks is currently dependent on the specific
>> hardware architecture. This limitation restricts the use of
>> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
>>
>> The generic soultion which would work across all architectures
>> is to read the PHC along with the latency to perform PHC-read as
>> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
>> timestamps.  However, these timestamps are currently limited
>> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
>> by NTP (or similar time synchronization services), it can
>> experience significant jumps forward or backward. This hinders
>> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
>> is designed to provide.
>>
>> This problem could be addressed by supporting MONOTONIC_RAW
>> timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
>> or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
>> by NTP adjustments.
>>
>> This enhancement can be implemented by utilizing one of the three
>> reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
>> the clock-id for timestamps.  The current behavior aligns with
>> clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
>> backward compatibility of the UAPI.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> 
> Hi Mahesh!
> 
> Any chance there will be v5 soon with the comments addressed?
> I don't see any strong objections to the patch, and it looks
> like we can benefit from having MONOTONIC_RAW clock for our
> project too.
> 
> Thanks,
> Vadim
> 

Hi Mahesh!

It's a gentle ping mail...
If you don't have cycles to work on this patch anymore I can jump in and
address the feedback to finally merge the feature.

Thanks,
Vadim



>> ---
>> v1 -> v2
>>     * Code-style fixes.
>> v2 -> v3
>>     * Reword commit log
>>     * Fix the compilation issue by using __kernel_clockid instead of 
>> clockid_t
>>       which has kernel only scope.
>> v3 -> v4
>>     * Typo/comment fixes.
>>
>>   drivers/ptp/ptp_chardev.c        |  7 +++++--
>>   include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
>>   include/uapi/linux/ptp_clock.h   | 27 +++++++++++++++++++++------
>>   3 files changed, 52 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
>> index 7513018c9f9a..c109109c9e8e 100644
>> --- a/drivers/ptp/ptp_chardev.c
>> +++ b/drivers/ptp/ptp_chardev.c
>> @@ -358,11 +358,14 @@ long ptp_ioctl(struct posix_clock_context 
>> *pccontext, unsigned int cmd,
>>               extoff = NULL;
>>               break;
>>           }
>> -        if (extoff->n_samples > PTP_MAX_SAMPLES
>> -            || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
>> +        if (extoff->n_samples > PTP_MAX_SAMPLES ||
>> +            extoff->rsv[0] || extoff->rsv[1] ||
>> +            (extoff->clockid != CLOCK_REALTIME &&
>> +             extoff->clockid != CLOCK_MONOTONIC_RAW)) {
>>               err = -EINVAL;
>>               break;
>>           }
>> +        sts.clockid = extoff->clockid;
>>           for (i = 0; i < extoff->n_samples; i++) {
>>               err = ptp->info->gettimex64(ptp->info, &ts, &sts);
>>               if (err)
>> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ 
>> ptp_clock_kernel.h
>> index 6e4b8206c7d0..74ded5f95d95 100644
>> --- a/include/linux/ptp_clock_kernel.h
>> +++ b/include/linux/ptp_clock_kernel.h
>> @@ -47,10 +47,12 @@ struct system_device_crosststamp;
>>    * struct ptp_system_timestamp - system time corresponding to a PHC 
>> timestamp
>>    * @pre_ts: system timestamp before capturing PHC
>>    * @post_ts: system timestamp after capturing PHC
>> + * @clockid: clock-base used for capturing the system timestamps
>>    */
>>   struct ptp_system_timestamp {
>>       struct timespec64 pre_ts;
>>       struct timespec64 post_ts;
>> +    clockid_t clockid;
>>   };
>>   /**
>> @@ -457,14 +459,34 @@ static inline ktime_t 
>> ptp_convert_timestamp(const ktime_t *hwtstamp,
>>   static inline void ptp_read_system_prets(struct ptp_system_timestamp 
>> *sts)
>>   {
>> -    if (sts)
>> -        ktime_get_real_ts64(&sts->pre_ts);
>> +    if (sts) {
>> +        switch (sts->clockid) {
>> +        case CLOCK_REALTIME:
>> +            ktime_get_real_ts64(&sts->pre_ts);
>> +            break;
>> +        case CLOCK_MONOTONIC_RAW:
>> +            ktime_get_raw_ts64(&sts->pre_ts);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>>   }
>>   static inline void ptp_read_system_postts(struct 
>> ptp_system_timestamp *sts)
>>   {
>> -    if (sts)
>> -        ktime_get_real_ts64(&sts->post_ts);
>> +    if (sts) {
>> +        switch (sts->clockid) {
>> +        case CLOCK_REALTIME:
>> +            ktime_get_real_ts64(&sts->post_ts);
>> +            break;
>> +        case CLOCK_MONOTONIC_RAW:
>> +            ktime_get_raw_ts64(&sts->post_ts);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>>   }
>>   #endif
>> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ 
>> ptp_clock.h
>> index 053b40d642de..5e3d70fbc499 100644
>> --- a/include/uapi/linux/ptp_clock.h
>> +++ b/include/uapi/linux/ptp_clock.h
>> @@ -155,13 +155,28 @@ struct ptp_sys_offset {
>>       struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>>   };
>> +/*
>> + * ptp_sys_offset_extended - data structure for IOCTL operation
>> + *                 PTP_SYS_OFFSET_EXTENDED
>> + *
>> + * @n_samples:    Desired number of measurements.
>> + * @clockid:    clockid of a clock-base used for pre/post timestamps.
>> + * @rsv:    Reserved for future use.
>> + * @ts:        Array of samples in the form [pre-TS, PHC, post-TS]. The
>> + *        kernel provides @n_samples.
>> + *
>> + * History:
>> + * v1: Initial implementation.
>> + *
>> + * v2: Use the first word of the reserved-field for @clockid. That's
>> + *     backward compatible since v1 expects all three reserved words
>> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
>> + *     CLOCK_REALTIME is '0'.
>> + */
>>   struct ptp_sys_offset_extended {
>> -    unsigned int n_samples; /* Desired number of measurements. */
>> -    unsigned int rsv[3];    /* Reserved for future use. */
>> -    /*
>> -     * Array of [system, phc, system] time stamps. The kernel will 
>> provide
>> -     * 3*n_samples time stamps.
>> -     */
>> +    unsigned int n_samples;
>> +    __kernel_clockid_t clockid;
>> +    unsigned int rsv[2];
>>       struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
>>   };
>