netdev - Re: [RFC 1/3] ptp: Implement timex esterror support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a17d3806-73cc-4fda-a1db-c516155982fc@machnikowski.net>
Date: Thu, 15 Aug 2024 11:33:29 +0200
From: Maciek Machnikowski <maciek@...hnikowski.net>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: netdev@...r.kernel.org, richardcochran@...il.com,
 jacob.e.keller@...el.com, darinzon@...zon.com, kuba@...nel.org
Subject: Re: [RFC 1/3] ptp: Implement timex esterror support



On 14/08/2024 11:29, Vadim Fedorenko wrote:
> On 13/08/2024 13:56, Maciek Machnikowski wrote:
>> !-------------------------------------------------------------------|
>>    This Message Is From an Untrusted Sender
>>    You have not previously corresponded with this sender.
>> |-------------------------------------------------------------------!
>>
>> The Timex structure returned by the clock_adjtime() POSIX API allows
>> the clock to return the estimated error. Implement getesterror
>> and setesterror functions in the ptp_clock_info to enable drivers
>> to interact with the hardware to get the error information.
>>
>> getesterror additionally implements returning hw_ts and sys_ts
>> to enable upper layers to estimate the maximum error of the clock
>> based on the last time of correction. This functionality is not
>> directly implemented in the clock_adjtime and will require
>> a separate interface in the future.
>>
>> Signed-off-by: Maciek Machnikowski <maciek@...hnikowski.net>
>> ---
>>   drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
>>   include/linux/ptp_clock_kernel.h | 11 +++++++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index c56cd0f63909..2cb1f6af60ea 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock
>> *pc, struct __kernel_timex *tx)
>>                 err = ops->adjphase(ops, offset);
>>           }
>> +    } else if (tx->modes & ADJ_ESTERROR) {
>> +        if (ops->setesterror)
>> +            if (tx->modes & ADJ_NANO)
>> +                err = ops->setesterror(ops, tx->esterror * 1000);
> 
> Looks like some miscoding here. The callback doc later says that
> setesterror expects nanoseconds. ADJ_NANO switches the structure to
> provide nanoseconds. But the code here expects tx->esterror to be in
> microseconds when ADJ_NANO is set, which is confusing.
> 
>> +            else
>> +                err = ops->setesterror(ops, tx->esterror);
>>       } else if (tx->modes == 0) {
>> +        long esterror;
>> +
>>           tx->freq = ptp->dialed_frequency;
>> -        err = 0;
>> +        if (ops->getesterror) {
>> +            err = ops->getesterror(ops, &esterror, NULL, NULL);
>> +            if (err)
>> +                return err;
>> +            tx->modes &= ADJ_NANO;
> 
> Here all the flags except possible ADJ_NANO is cleaned, but why?
> 
>> +            tx->esterror = esterror
> And here it doesn't matter what is set in flags, you return nanoseconds
> directly...
> 
You're right - seems I made an inverted logic. The PTP API should work
on nanoseconds and the layer above should convert different units to
nanoseconds.

>> +        } else {
>> +            err = 0;
>> +        }
>>       }
>>         return err;
>> diff --git a/include/linux/ptp_clock_kernel.h
>> b/include/linux/ptp_clock_kernel.h
>> index 6e4b8206c7d0..e78ea81fc4cf 100644
>> --- a/include/linux/ptp_clock_kernel.h
>> +++ b/include/linux/ptp_clock_kernel.h
>> @@ -136,6 +136,14 @@ struct ptp_system_timestamp {
>>    *                   parameter cts: Contains timestamp
>> (device,system) pair,
>>    *                   where system time is realtime and monotonic.
>>    *
>> + * @getesterror: Reads the current error estimate of the hardware clock.
>> + *               parameter phase: Holds the error estimate in
>> nanoseconds.
>> + *               parameter hw_ts: If not NULL, holds the timestamp of
>> the hardware clock.
>> + *               parameter sw_ts: If not NULL, holds the timestamp of
>> the CPU clock.
>> + *
>> + * @setesterror:  Set the error estimate of the hardware clock.
>> + *                parameter phase: Desired error estimate in
>> nanoseconds.
>> + *
> 
> The man pages says that esterror is in microseconds. It makes total
> sense to make it nanoseconds, but we have to adjust the documentation.
> 
>>    * @enable:   Request driver to enable or disable an ancillary feature.
>>    *            parameter request: Desired resource to enable or disable.
>>    *            parameter on: Caller passes one to enable or zero to
>> disable.
>> @@ -188,6 +196,9 @@ struct ptp_clock_info {
>>                   struct ptp_system_timestamp *sts);
>>       int (*getcrosscycles)(struct ptp_clock_info *ptp,
>>                     struct system_device_crosststamp *cts);
>> +    int (*getesterror)(struct ptp_clock_info *ptp, long *phase,
>> +               struct timespec64 *hw_ts, struct timespec64 *sys_ts);
>> +    int (*setesterror)(struct ptp_clock_info *ptp, long phase);
>>       int (*enable)(struct ptp_clock_info *ptp,
>>                 struct ptp_clock_request *request, int on);
>>       int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
>