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: <20201204005624.GC18560@hoboy.vegasvil.org>
Date:   Thu, 3 Dec 2020 16:56:24 -0800
From:   Richard Cochran <richardcochran@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...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 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)

> +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.

Suggest plain old ptp_ocp_gettime();

> +	struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	err = __ptp_ocp_gettime_locked(bp, ts, sts);
> +	spin_unlock_irqrestore(&bp->lock, flags);
> +
> +	return err;
> +}
> +
> +static void
> +__ptp_ocp_settime_locked(struct ptp_ocp *bp, const struct timespec64 *ts)
> +{
> +	u32 ctrl, time_sec, time_ns;
> +	u32 select;
> +
> +	time_ns = ts->tv_nsec;
> +	time_sec = ts->tv_sec;
> +
> +	select = ioread32(&bp->reg->select);
> +	iowrite32(OCP_SELECT_CLK_REG, &bp->reg->select);
> +
> +	iowrite32(time_ns, &bp->reg->adjust_ns);
> +	iowrite32(time_sec, &bp->reg->adjust_sec);
> +
> +	ctrl = ioread32(&bp->reg->ctrl);
> +	ctrl |= OCP_CTRL_ADJUST_TIME;
> +	iowrite32(ctrl, &bp->reg->ctrl);
> +
> +	/* restore clock selection */
> +	iowrite32(select >> 16, &bp->reg->select);
> +}
> +
> +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.

> +	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.

> +	return -EOPNOTSUPP;

You can set the offset but not the frequency adjustment.  Makes sense
for an atomic clock.

> +}

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

If not, then that seems unfortunate.  Still you can regulate the Linux
system clock in software, but that of course introduces time error.

Thanks,
Richard