netdev - Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

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: <CANr-f5zyLX1YAW+D4AJn2MBQ8g7e8F+KVDc0GuxL7s9K89Qx_A@mail.gmail.com>
Date:   Mon, 7 Mar 2022 18:54:19 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     yangbo.lu@....com, David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, mlichvar@...hat.com,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with
 additional free running time

> > > How can I cover my use case with the existing API? I had no idea so far.
> >
> > Okay, so 2 PHCs doesn't help, but still all you need is:
> >
> > 1. a different method to convert time stamps to vclock time base
> >
> > 2. a different method for vclocks' gettime
> >
> > So let me suggest a much smaller change to the phc/vclock api... stay tuned
>
> For #1:
>
> On the receive path, the stack calls ptp_convert_timestamp() if the
> socket has the SOF_TIMESTAMPING_RAW_HARDWARE option.  In that method,
> you need only get the raw cycle count if supported by the pclock.
>
> So instead of:
>
>         vclock = info_to_vclock(ptp->info);
>
>         ns = ktime_to_ns(hwtstamps->hwtstamp);
>
>         spin_lock_irqsave(&vclock->lock, flags);
>         ns = timecounter_cyc2time(&vclock->tc, ns);
>         spin_unlock_irqrestore(&vclock->lock, flags);
>
> something like this:
>
>         vclock = info_to_vclock(ptp->info);
>
>         cycles = pclock->ktime_to_cycles(hwtstamps->hwtstamp);
>
>         spin_lock_irqsave(&vclock->lock, flags);
>         ns = timecounter_cyc2time(&vclock->tc, cycles);
>         spin_unlock_irqrestore(&vclock->lock, flags);
>
> This new class method, ktime_to_cycles, can simply do ktime_to_ns() by
> default for all of the existing drivers, but your special driver can
> look up the hwtstamp in a cache of {hwtstamp, cycles} pairs.

ktime_to_cycles uses hwtstamp as key for the cache lookup. As long as
the PHC is monotonic, the key is unique. If the time of the PHC is set, then
the cache would be invalidated. I'm afraid that setting the PHC could lead to
wrong or missing timestamps. Setting the PHC in hardware, timestamp
generation in hardware, and cache invalidation in software would need to
be synchronized somehow.

Practically the PHC should be set only once. It would be also ok to use
vclocks for PTP after PHC has been set. So it should work. But it would
not be the generic PHC/vclock user space interface anymore.

> (No need to bloat skbuff by another eight bytes!)

I understand that bloating skbuff shall be prevented. Actually I need
to find a way
to generate the correct timestamp within ptp_convert_timestamp and without
bloating skbuff. The cache is one possibility. What do you think about the
following idea?

For TX it is known which timestamp is required. So I would have to find a way
to detect which timestamp shall be filled into hwtstamp.

For RX both timestamps are already available within skbuff, because they are
stored in front of the Ethernet header by the hardware. So I have to find a way
to detect the RX case and copy the right timestamp to hwtstamp.

This would prevent the cache and does not bloat skbuff.

> For #2:
>
> Similarly, add a new class method, say, pclock.get_cycles that does
>
>         if (ptp->info->gettimex64)
>                 ptp->info->gettimex64(ptp->info, &ts, NULL);
>         else
>                 ptp->info->gettime64(ptp->info, &ts);
>
> by default, but in your driver will read the special counter.

Looks better than my getfreeruntimex64.

Thanks for your suggestion!

Gerhard