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  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]
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

Powered by blists - more mailing lists