netdev - Re: [PATCH v2 net-next] ptp: Add clock driver for the OpenCompute TimeCard.

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: <20201204020037.jeyzulaqp4kd4pnv@bsd-mbp.dhcp.thefacebook.com>
Date:   Thu, 3 Dec 2020 18:00:37 -0800
From:   Jonathan Lemon <jonathan.lemon@...il.com>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, kernel-team@...com
Subject: Re: [PATCH v2 net-next] ptp: Add clock driver for the OpenCompute
 TimeCard.

On Thu, Dec 03, 2020 at 04:56:24PM -0800, Richard Cochran wrote:
> On Thu, Dec 03, 2020 at 10:29:25AM -0800, Jonathan Lemon wrote:
> > The OpenCompute time card is an atomic clock along with
> > a GPS receiver that provides a Grandmaster clock source
> > for a PTP enabled network.
> > 
> > More information is available at http://www.timingcard.com/
> > 
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> 
> What changed in v2?
> 
> (please include a change log for future patches)

test robot complained because it wasn't dependent on CONFIG_PCI.
Will add a cover letter for the next v3.

 
> > +static int
> > +ptp_ocp_gettimex(struct ptp_clock_info *ptp_info, struct timespec64 *ts,
> > +		 struct ptp_system_timestamp *sts)
> > +{
> 
> The name here is a bit confusing since "timex" has a special meaning
> in the NTP/PTP API.

The .gettimex64 call is used here so the time returned from the
clock can be correlated to the system time.


> > +static int
> > +ptp_ocp_settime(struct ptp_clock_info *ptp_info, const struct timespec64 *ts)
> > +{
> > +	struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> > +	unsigned long flags;
> > +
> > +	if (ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC)
> > +		return 0;
> > +
> > +	dev_info(&bp->pdev->dev, "settime to: %lld.%ld\n",
> > +		 ts->tv_sec, ts->tv_nsec);
> 
> No need for this dmesg spam.   Change to _debug if you really want to
> keep it.

Will remove.


> > +	spin_lock_irqsave(&bp->lock, flags);
> > +	__ptp_ocp_settime_locked(bp, ts);
> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> > +static int
> > +ptp_ocp_null_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
> > +{
> > +	struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> > +
> > +	if (scaled_ppm == 0)
> > +		return 0;
> > +
> > +	dev_info(&bp->pdev->dev, "adjfine, scaled by: %ld\n", scaled_ppm);
> 
> No need for this either.

Ditto.

 
> This driver looks fine, but I'm curious how you will use it.  Can it
> provide time stamping for network frames or other IO?

The card does have a PPS pulse output, so it can be wired to a network
card which takes an external PPS signal.

Right now, the current model (which certainly can be improved on) is using
phc2sys to discipline the system and NIC clocks.

I'll send a v3.  I also need to open a discussion on how this should
return the leap second changes to the user - there doesn't seem to be
anything in the current API for this.
-- 
Jonathan