netdev - Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210701161555.y4p6wz6l6e6ea2vg@bsd-mbp.dhcp.thefacebook.com>
Date:   Thu, 1 Jul 2021 09:15:55 -0700
From:   Jonathan Lemon <jonathan.lemon@...il.com>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event

On Thu, Jul 01, 2021 at 07:59:35AM -0700, Richard Cochran wrote:
> On Tue, Jun 29, 2021 at 08:50:31PM -0700, Jonathan Lemon wrote:
> 
> > Since these events are channel specific, I don't see why
> > this is problematic.
> 
> The problem is that the semantics of ptp_clock_event::data are not
> defined...
> 
> > The code blocks in question from my
> > upcoming patch (dependent on this) is:
> > 
> >     static irqreturn_t
> >     ptp_ocp_phase_irq(int irq, void *priv)
> >     {
> >             struct ptp_ocp_ext_src *ext = priv;
> >             struct ocp_phase_reg __iomem *reg = ext->mem;
> >             struct ptp_clock_event ev;
> >             u32 phase_error;
> > 
> >             phase_error = ioread32(&reg->phase_error);
> > 
> >             ev.type = PTP_CLOCK_EXTTSUSR;
> >             ev.index = ext->info->index;
> >             ev.data = phase_error;
> >             pps_get_ts(&ev.pps_times);
> 
> Here the time stamp is the system time, and the .data field is the
> mysterious "phase_error", but ...

Yes, the time here is not really relevant.


  
> >             ptp_clock_event(ext->bp->ptp, &ev);
> > 
> >             iowrite32(0, &reg->intr);
> > 
> >             return IRQ_HANDLED;
> >     }
> > 
> > and
> > 
> >     static irqreturn_t
> >     ptp_ocp_ts_irq(int irq, void *priv)
> >     {
> >             struct ptp_ocp_ext_src *ext = priv;
> >             struct ts_reg __iomem *reg = ext->mem;
> >             struct ptp_clock_event ev;
> > 
> >             ev.type = PTP_CLOCK_EXTTSUSR;
> >             ev.index = ext->info->index;
> >             ev.pps_times.ts_real.tv_sec = ioread32(&reg->time_sec);
> >             ev.pps_times.ts_real.tv_nsec = ioread32(&reg->time_ns);
> >             ev.data = ioread32(&reg->ts_count);
> 
> here the time stamp comes from the PHC device, apparently, and the
> .data field is a counter.
> 
> >             ptp_clock_event(ext->bp->ptp, &ev);
> > 
> >             iowrite32(1, &reg->intr);       /* write 1 to ack */
> > 
> >             return IRQ_HANDLED;
> >     }
> > 
> > 
> > I'm not seeing why this is controversial.
> 
> Simply put, it makes no sense to provide a new PTP_CLOCK_EXTTSUSR that
> has multiple, random meanings.  There has got to be a better way.

I agree with this.  I just talked to the HW folks, and they're willing
to change things so 2 PPS events are returned, which eliminates the need
for an internal comparator.  The returned time would come from a latched
value from a HW register (same as the ptp_ocp_ts_irq version above).

I'm still stuck with how to provide the sec/nsec from the hardware 
directly to the application, though, since the current code does:

static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
                                       struct ptp_clock_event *src)
{
        struct ptp_extts_event *dst;
        unsigned long flags;
        s64 seconds;
        u32 remainder;

        seconds = div_u64_rem(src->timestamp, 1000000000, &remainder);


It seems like there should be a way to use pps_times here instead
of needing to convert back and forth.
-- 
Jonathan