netdev - Re: [PATCH net-next 4/7] sfc: Add support for IEEE-1588 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20120720063102.GB2330@netboy.at.omicron.at>
Date:	Fri, 20 Jul 2012 08:31:02 +0200
From:	Richard Cochran <richardcochran@...il.com>
To:	Stuart Hodgson <smhodgson@...arflare.com>
Cc:	Ben Hutchings <bhutchings@...arflare.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	linux-net-drivers@...arflare.com,
	Andrew Jackson <ajackson@...arflare.com>
Subject: Re: [PATCH net-next 4/7] sfc: Add support for IEEE-1588 PTP

On Thu, Jul 19, 2012 at 04:29:51PM +0100, Stuart Hodgson wrote:
> On 19/07/12 15:25, Richard Cochran wrote:
> > On Wed, Jul 18, 2012 at 07:21:33PM +0100, Ben Hutchings wrote:
> >> Add PTP IEEE-1588 support and make accesible via the PHC subsystem.

...

> >> +/* Process times received from MC.
> >> + *
> >> + * Extract times from returned results, and establish the minimum value
> >> + * seen.  The minimum value represents the "best" possible time and events
> >> + * too much greater than this are rejected - the machine is, perhaps, too
> >> + * busy. A number of readings are taken so that, hopefully, at least one good
> >> + * synchronisation will be seen in the results.
> >> + */
> > 
> > This code looks like it is trying to find the offset between two
> > clocks. Is there some reason why you cannot use <linux/timecompare.h>
> > to accomplish this?
> 
> This is what the code is doing. <linux/timecompare.h> states
> 
> "the assumption is that reading the source
> time is slow and involves equal time for sending the request and
> receiving the reply"
> 
> While in our case event though it is slow we cannot guarantee the second
> assumption. The code above takes into account some of the particulars of the sfc
> hardware and gives us good results. 

Fair enough, but then maybe a comment mentioning how timecompare is
unsuitable would be nice to have.

> > I am trying to purge the whole SYS thing (only blackfin is left)
> > because there is a much better way to go about this, namely
> > synchronizing the system time to the PHC time via an internal PPS
> > signal.
> 
> This may be possible in future. But leads us to another problem
> where the PPS event that is generated by the PHC subsystem to the 
> PPS subsystem is stamped with the current system_time. That may
> be fine for a PPS signal generated from an interrupt but not when
> the internal PPS event has implicit jitter from the handler/event_queue
> that we have in the driver.

Yes, both your driver and timecompare**  suffer from the same
defect, namely that the synchronization times are determined by
network traffic, and not at some regular frequency like 1 Hz.

Why not just offer a 1 PPS signal using the standard method?  If you
cannot produce a real interrupt for this, you can at least use a
synthetic one using a periodically scheduled work.

** Actually, timecompare is a bit better as it ensures that the
   measurements occur no more than once per second.

> Calling pps_event directly does not seem possible with the current
> header/structure layout preventing access to ptp_clock->pps_source.
> Including ptp_private.h seems wrong.

I don't understand what the issue is here. Can't you just call
ptp_clock_event, like you already have...

> >> +static void efx_ptp_pps_worker(struct work_struct *work)
> >> +{
> >> +	struct efx_ptp_data *ptp =
> >> +		container_of(work, struct efx_ptp_data, pps_work);
> >> +	struct efx_nic *efx = ptp->channel->efx;
> >> +	struct timespec event_gen_time;
> >> +	struct ptp_clock_event ptp_pps_evt;
> >> +	ktime_t gen_time_host;
> >> +
> >> +	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
> >> +		return;
> >> +
> >> +	gen_time_host = ktime_sub(ptp->mc_base_time,
> >> +				  ptp->host_base_time);
> >> +	event_gen_time = ktime_to_timespec(gen_time_host);
> >> +
> >> +	ptp_pps_evt.type = PTP_CLOCK_EXTTS;
> >> +	ptp_pps_evt.timestamp = ktime_to_ns(gen_time_host);
> >> +	ptp_clock_event(ptp->phc_clock, &ptp_pps_evt);
> >> +}

... here?

> >> +static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
> >> +{

You can set the time here somehow by doing, T' = T + offset, and so...

> >> +}

> >> +static int efx_phc_settime(struct ptp_clock_info *ptp,
> >> +			   const struct timespec *e_ts)
> >> +{
> >> +	/* We must provide this function, but we cannot actually set the time */
> > 
> > Huh? You can adjtime, so must be able to settime, too, right?
> > 
> > If you have enough range in the RAW timestamp in the MC firmware (like
> > 64 bits of nanoseconds), and you allow settime, then you can spare the
> > system time synchronization code altogether.
> > 
> 
> You will have to elaborate further on this point.

... why can't you also just set the time?

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html