| 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
| ||
|
Date: Wed, 14 Nov 2018 19:24:11 -0800
From: Richard Cochran <richardcochran@...il.com>
To: Dalon Westergreen <dwesterg@...il.com>
Cc: netdev@...r.kernel.org, dinguyen@...nel.org,
thor.thayer@...ux.intel.com,
Dalon Westergreen <dalon.westergreen@...el.com>
Subject: Re: [PATCH net-next 6/8] net: eth: altera: tse: add support for ptp
and timestamping
On Wed, Nov 14, 2018 at 04:50:45PM -0800, Dalon Westergreen wrote:
> From: Dalon Westergreen <dalon.westergreen@...el.com>
>
> Add support for the ptp clock used with the tse, and update
> the driver to support timestamping when enabled. We also
> enable debugfs entries for the ptp clock to allow some user
> control and interaction with the ptp clock.
>
> Signed-off-by: Dalon Westergreen <dalon.westergreen@...el.com>
> ---
> drivers/net/ethernet/altera/Kconfig | 1 +
> drivers/net/ethernet/altera/Makefile | 3 +-
> drivers/net/ethernet/altera/altera_ptp.c | 473 ++++++++++++++++++
> drivers/net/ethernet/altera/altera_ptp.h | 77 +++
Wouldn't it be nice if the PTP maintainer were included on CC for new
PTP drivers? One can always wish...
> diff --git a/drivers/net/ethernet/altera/Kconfig b/drivers/net/ethernet/altera/Kconfig
> index fdddba51473e..36aee0fc0b51 100644
> --- a/drivers/net/ethernet/altera/Kconfig
> +++ b/drivers/net/ethernet/altera/Kconfig
> @@ -2,6 +2,7 @@ config ALTERA_TSE
> tristate "Altera Triple-Speed Ethernet MAC support"
> depends on HAS_DMA
> select PHYLIB
> + select PTP_1588_CLOCK
We now use "imply" instead of "select".
> +/* A fine ToD HW clock offset adjustment.
> + * To perform the fine offset adjustment the AdjustPeriod register is used
> + * to replace the Period register for AdjustCount clock cycles in hardware.
> + */
> +static int fine_adjust_tod_clock(struct altera_ptp_private *priv,
> + u32 adjust_period, u32 adjust_count)
The function naming is really poor throughout this driver. You should
use a unique prefix that relates to your driver or HW.
> +static int adjust_fine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
This is named like a global function. Needs prefix.
> + struct altera_ptp_private *priv =
> + container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> + unsigned long flags;
> + int ret = 0;
> + u64 ppb;
> + u32 tod_period;
> + u32 tod_rem;
> + u32 tod_drift_adjust_fns;
> + u32 tod_drift_adjust_rate;
> + unsigned long rate;
Please put all like types together on one line, and use reverse
Christmas tree style.
> +
> + priv->scaled_ppm = scaled_ppm;
> +
> + /* only unlock if it is currently locked */
> + if (mutex_is_locked(&priv->ppm_mutex))
> + mutex_unlock(&priv->ppm_mutex);
> +
> + if (!priv->ptp_correct_freq)
> + goto out;
> +
> + rate = clk_get_rate(priv->tod_clk);
> +
> + /* From scaled_ppm_to_ppb */
> + ppb = 1 + scaled_ppm;
> + ppb *= 125;
> + ppb >>= 13;
> +
> + ppb += NOMINAL_PPB;
> +
> + tod_period = div_u64_rem(ppb << 16, rate, &tod_rem);
> + if (tod_period > TOD_PERIOD_MAX) {
> + ret = -ERANGE;
> + goto out;
> + }
> +
> + /* The drift of ToD adjusted periodically by adding a drift_adjust_fns
> + * correction value every drift_adjust_rate count of clock cycles.
> + */
> + tod_drift_adjust_fns = tod_rem / gcd(tod_rem, rate);
> + tod_drift_adjust_rate = rate / gcd(tod_rem, rate);
> +
> + while ((tod_drift_adjust_fns > TOD_DRIFT_ADJUST_FNS_MAX) |
> + (tod_drift_adjust_rate > TOD_DRIFT_ADJUST_RATE_MAX)) {
> + tod_drift_adjust_fns = tod_drift_adjust_fns >> 1;
> + tod_drift_adjust_rate = tod_drift_adjust_rate >> 1;
> + }
> +
> + if (tod_drift_adjust_fns == 0)
> + tod_drift_adjust_rate = 0;
> +
> + spin_lock_irqsave(&priv->tod_lock, flags);
> + csrwr32(tod_period, priv->tod_ctrl, tod_csroffs(period));
> + csrwr32(0, priv->tod_ctrl, tod_csroffs(adjust_period));
> + csrwr32(0, priv->tod_ctrl, tod_csroffs(adjust_count));
> + csrwr32(tod_drift_adjust_fns, priv->tod_ctrl,
> + tod_csroffs(drift_adjust));
> + csrwr32(tod_drift_adjust_rate, priv->tod_ctrl,
> + tod_csroffs(drift_adjust_rate));
> + spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +out:
> + return ret;
> +}
> +
> +static int adjust_time(struct ptp_clock_info *ptp, s64 delta)
> +{
> + struct altera_ptp_private *priv =
> + container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> + int ret = 0;
> + u64 abs_delta;
> + unsigned long flags;
> + u32 period;
> + u32 diff;
> + u64 count;
> + u32 rem;
Same here.
> + if (!priv->ptp_correct_offs)
> + goto out;
> +
> + if (delta < 0)
> + abs_delta = -delta;
> + else
> + abs_delta = delta;
> +
> + spin_lock_irqsave(&priv->tod_lock, flags);
> +
> + /* Get the maximum possible value of the Period register offset
> + * adjustment in nanoseconds scale. This depends on the current
> + * Period register settings and the maximum and minimum possible
> + * values of the Period register.
> + */
> + period = csrrd32(priv->tod_ctrl, tod_csroffs(period));
> +
> + if (delta < 0)
> + diff = (period - TOD_PERIOD_MIN) >> 16;
> + else
> + diff = (TOD_PERIOD_MAX - period) >> 16;
> +
> + /* Find out the least number of cycles needed for the Period register
> + * adjustment by delta nanoseconds.
> + */
> + while (diff) {
Ugh. How many 64 divisions are you going to do? Is there no better
way?
> + count = div_u64_rem(abs_delta, diff, &rem);
> + if (rem == 0)
> + break;
> + diff--;
> + }
> +
> + if (diff && count && count < TOD_ADJUST_COUNT_MAX) {
> + /* Perform the fine time offset adjustment */
> + if (delta < 0)
> + period -= (diff << 16);
> + else
> + period += (diff << 16);
> +
> + ret = fine_adjust_tod_clock(priv, period, count);
> +
> + } else {
> + /* Perform the coarse time offset adjustment */
> + ret = coarse_adjust_tod_clock(priv, delta);
> + }
> +
> + spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +out:
> + return ret;
> +}
> +
> +static int get_time(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> + struct altera_ptp_private *priv =
> + container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> + unsigned long flags;
> + u64 seconds;
> + u32 seconds_msb;
> + u32 seconds_lsb;
> + u32 nanosec;
> +
> + spin_lock_irqsave(&priv->tod_lock, flags);
> + nanosec = csrrd32(priv->tod_ctrl, tod_csroffs(nanosec));
> + seconds_lsb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_lsb));
> + seconds_msb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_msb));
> + spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> + seconds = (((u64)(seconds_msb & 0x0000ffff)) << 32) | seconds_lsb;
> +
> + ts->tv_nsec = nanosec;
> + ts->tv_sec = (__kernel_time_t)seconds;
> +
> + return 0;
> +}
> +
> +static int set_time(struct ptp_clock_info *ptp, const struct timespec64 *ts)
> +{
> + struct altera_ptp_private *priv =
> + container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> + unsigned long flags;
> + u32 seconds_msb = upper_32_bits(ts->tv_sec) & 0x0000ffff;
> + u32 seconds_lsb = lower_32_bits(ts->tv_sec);
> + u32 nanosec = lower_32_bits(ts->tv_nsec);
> +
> + spin_lock_irqsave(&priv->tod_lock, flags);
> + csrwr32(seconds_msb, priv->tod_ctrl, tod_csroffs(seconds_msb));
> + csrwr32(seconds_lsb, priv->tod_ctrl, tod_csroffs(seconds_lsb));
> + csrwr32(nanosec, priv->tod_ctrl, tod_csroffs(nanosec));
> + spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> + return 0;
> +}
> +
> +static int enable_feature(struct ptp_clock_info *ptp,
> + struct ptp_clock_request *request, int on)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info altera_ptp_clock_ops = {
> + .owner = THIS_MODULE,
> + .name = "altera_ptp",
> + .max_adj = 500000000,
> + .n_alarm = 0,
> + .n_ext_ts = 0,
> + .n_per_out = 0,
> + .pps = 0,
> + .adjfine = adjust_fine,
> + .adjtime = adjust_time,
> + .gettime64 = get_time,
> + .settime64 = set_time,
> + .enable = enable_feature,
Use driver-specific naming, please.
> +};
> +
> +/* ptp debugfs */
> +static ssize_t ptp_tod_read(struct file *filp, char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
Please drop this debugfs stuff. We can read the clock with clock_gettime().
> + struct altera_ptp_private *priv = filp->private_data;
> + unsigned long flags;
> + u32 seconds_msb;
> + u32 seconds_lsb;
> + u32 nanosec;
> +
> + if (*ppos != 0)
> + return 0;
> +
> + if (count < (sizeof(u32) * 3))
> + return -ENOSPC;
> +
> + spin_lock_irqsave(&priv->tod_lock, flags);
> + nanosec = csrrd32(priv->tod_ctrl, tod_csroffs(nanosec));
> + seconds_lsb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_lsb));
> + seconds_msb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_msb));
> + spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> + if (copy_to_user(buffer, &seconds_msb, sizeof(u32)))
> + return -EFAULT;
> + buffer += sizeof(u32);
> +
> + if (copy_to_user(buffer, &seconds_lsb, sizeof(u32)))
> + return -EFAULT;
> + buffer += sizeof(u32);
> +
> + if (copy_to_user(buffer, &nanosec, sizeof(u32)))
> + return -EFAULT;
> +
> + return (sizeof(u32) * 3);
> +}
> +
> +static const struct file_operations ptp_dbg_tod_ops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = ptp_tod_read,
> + .write = NULL,
> +};
> +
> +static ssize_t ptp_ppm_read(struct file *filp, char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + struct altera_ptp_private *priv = filp->private_data;
> +
> + if (*ppos != 0)
> + return 0;
> +
> + if (count < sizeof(long))
> + return -ENOSPC;
> +
> + if (mutex_lock_interruptible(&priv->ppm_mutex))
> + return -EAGAIN;
> +
> + if (copy_to_user(buffer, &priv->scaled_ppm, sizeof(long)))
> + return -EFAULT;
> +
> + return sizeof(long);
> +}
> +
> +static const struct file_operations ptp_dbg_ppm_ops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = ptp_ppm_read,
> + .write = NULL,
> +};
> +
> +int altera_ptp_dbg_init(struct altera_ptp_private *priv,
> + struct dentry *dfs_dir_root)
> +{
> + struct dentry *dfs_dir;
> +
> + dfs_dir = debugfs_create_dir("ptp", dfs_dir_root);
> +
> + /* Use u32 instead of bool */
> + if (!debugfs_create_u32("correct_offset", 0600, dfs_dir,
> + &priv->ptp_correct_offs))
> + return -ENOMEM;
> +
> + /* Use u32 instead of bool */
> + if (!debugfs_create_u32("correct_frequency", 0600, dfs_dir,
> + &priv->ptp_correct_freq))
> + return -ENOMEM;
> +
> + if (!debugfs_create_file("tod", 0400, dfs_dir,
> + (void *)priv, &ptp_dbg_tod_ops))
> + return -ENOMEM;
> +
> + if (!debugfs_create_file("scaled_ppm", 0400, dfs_dir,
> + (void *)priv, &ptp_dbg_ppm_ops))
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +/* Initialize PTP control block registers */
> +int altera_init_ptp(struct altera_ptp_private *priv)
> +{
So here we have a prefix, but "altera" is a poor choice. That is (or
was) a company name, not the name of a specific device. Imagine if a
MAC driver used "intel_init_mac()"!
Maybe "altera_tse_" ?
Thanks,
Richard
Powered by blists - more mailing lists