| 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
| ||
|
Message-ID: <CDCA1F64.1E87D%matthew.vick@intel.com>
Date: Tue, 28 May 2013 15:58:07 +0000
From: "Vick, Matthew" <matthew.vick@...el.com>
To: Richard Cochran <richardcochran@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: David Miller <davem@...emloft.net>,
"e1000-devel@...ts.sourceforge.net"
<e1000-devel@...ts.sourceforge.net>,
"Keller, Jacob E" <jacob.e.keller@...el.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
Subject: Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for
the i210.
On 5/27/13 2:21 AM, "Richard Cochran" <richardcochran@...il.com> wrote:
>The i210 device offers a number of special PTP Hardware Clock features on
>the Software Defined Pins (SDPs). This patch adds support for three of the
>possible functions, namely time stamping external events, a periodic
>output signal, and an internal PPS event for adjusting the kernel system
>time.
>
>A pair of module parameters lets the user choose which of the four SDPs
>will be used as the input signal and which as the output.
>
>Signed-off-by: Richard Cochran <richardcochran@...il.com>
>---
> drivers/net/ethernet/intel/igb/igb.h | 2 +
> drivers/net/ethernet/intel/igb/igb_main.c | 30 ++++++
> drivers/net/ethernet/intel/igb/igb_ptp.c | 157
>++++++++++++++++++++++++++++-
> 3 files changed, 188 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/igb/igb.h
>b/drivers/net/ethernet/intel/igb/igb.h
>index 15ea8dc..802b79c 100644
>--- a/drivers/net/ethernet/intel/igb/igb.h
>+++ b/drivers/net/ethernet/intel/igb/igb.h
>@@ -435,6 +435,8 @@ struct igb_adapter {
> struct timecounter tc;
> u32 tx_hwtstamp_timeouts;
> u32 rx_hwtstamp_cleared;
>+ struct timespec start;
>+ struct timespec period;
>
> char fw_version[32];
> #ifdef CONFIG_IGB_HWMON
>diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>b/drivers/net/ethernet/intel/igb/igb_main.c
>index d25a965..58120cd 100644
>--- a/drivers/net/ethernet/intel/igb/igb_main.c
>+++ b/drivers/net/ethernet/intel/igb/igb_main.c
>@@ -5038,12 +5038,42 @@ void igb_update_stats(struct igb_adapter *adapter,
> static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
>+ struct ptp_clock_event event;
> u32 tsicr = rd32(E1000_TSICR);
>
>+ if (tsicr & TSINTR_SYS_WRAP) {
>+ event.type = PTP_CLOCK_PPS;
>+ ptp_clock_event(adapter->ptp_clock, &event);
>+ }
>+
> if (tsicr & E1000_TSICR_TXTS) {
> /* retrieve hardware timestamp */
> schedule_work(&adapter->ptp_tx_work);
> }
>+
>+ if (tsicr & TSINTR_TT0) {
>+ struct timespec ts;
>+ u32 tsauxc;
>+ spin_lock(&adapter->tmreg_lock);
>+ ts = timespec_add(adapter->start, adapter->period);
>+ wr32(E1000_TRGTTIML0, ts.tv_nsec);
>+ wr32(E1000_TRGTTIMH0, ts.tv_sec);
>+ tsauxc = rd32(E1000_TSAUXC);
>+ tsauxc |= TSAUXC_EN_TT0;
>+ wr32(E1000_TSAUXC, tsauxc);
>+ adapter->start = ts;
>+ spin_unlock(&adapter->tmreg_lock);
>+ }
>+
>+ if (tsicr & TSINTR_AUTT0) {
>+ u32 sec, nsec;
>+ nsec = rd32(E1000_AUXSTMPL0);
>+ sec = rd32(E1000_AUXSTMPH0);
>+ event.type = PTP_CLOCK_EXTTS;
>+ event.index = 0;
>+ event.timestamp = sec * 1000000000ULL + nsec;
>+ ptp_clock_event(adapter->ptp_clock, &event);
>+ }
> }
I would prefer it if we did a MAC check before these two TSICR checks,
since we're making some assumptions about the hardware within the
interrupt cases. At the very least, a comment that these are only
applicable to I210/I211 would be nice.
>
> static irqreturn_t igb_msix_other(int irq, void *data)
>diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c
>b/drivers/net/ethernet/intel/igb/igb_ptp.c
>index 5944de0..8cf4b8a 100644
>--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
>+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
>@@ -23,6 +23,15 @@
>
> #include "igb.h"
>
>+static int igb_input_sdp = 0;
>+static int igb_output_sdp = 1;
>+module_param(igb_input_sdp, int, 0444);
>+module_param(igb_output_sdp, int, 0444);
>+MODULE_PARM_DESC(igb_input_sdp,
>+ "The SDP used as an input, to time stamp external events");
>+MODULE_PARM_DESC(igb_output_sdp,
>+ "The SDP used to output the programmable periodic signal");
>+
Is there any other mechanism we could use to control this? I would imagine
not, but I know module parameters are generally frowned upon.
> #define INCVALUE_MASK 0x7fffffff
> #define ISGN 0x80000000
>
>@@ -359,6 +368,86 @@ static int igb_ptp_settime_i210(struct
>ptp_clock_info *ptp,
> return 0;
> }
>
>+static int igb_ptp_enable_i210(struct ptp_clock_info *ptp,
>+ struct ptp_clock_request *rq, int on)
>+{
>+ struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
>+ ptp_caps);
>+ struct e1000_hw *hw = &igb->hw;
>+ unsigned long flags;
>+ struct timespec ts;
>+ u32 tsauxc, tsim;
>+ s64 ns;
>+
>+ switch (rq->type) {
>+ case PTP_CLK_REQ_EXTTS:
>+ if (rq->extts.index != 0)
>+ return -EINVAL;
>+ spin_lock_irqsave(&igb->tmreg_lock, flags);
>+ tsauxc = rd32(E1000_TSAUXC);
>+ tsim = rd32(E1000_TSIM);
>+ if (on) {
>+ tsauxc |= TSAUXC_EN_TS0;
>+ tsim |= TSINTR_AUTT0;
>+ } else {
>+ tsauxc &= ~TSAUXC_EN_TS0;
>+ tsim &= ~TSINTR_AUTT0;
>+ }
>+ wr32(E1000_TSAUXC, tsauxc);
>+ wr32(E1000_TSIM, tsim);
>+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
>+ return 0;
>+
>+ case PTP_CLK_REQ_PEROUT:
>+ if (rq->perout.index != 0)
>+ return -EINVAL;
>+
>+ ts.tv_sec = rq->perout.period.sec;
>+ ts.tv_nsec = rq->perout.period.nsec;
>+ ns = timespec_to_ns(&ts);
>+ ns = ns >> 1;
>+ if (on && ns < 500000LL) {
>+ /* 2k interrupts per second is an awful lot. */
>+ return -EINVAL;
>+ }
>+ ts = ns_to_timespec(ns);
>+
>+ spin_lock_irqsave(&igb->tmreg_lock, flags);
>+ tsauxc = rd32(E1000_TSAUXC);
>+ tsim = rd32(E1000_TSIM);
>+ if (on) {
>+ igb->start.tv_sec = rq->perout.start.sec;
>+ igb->start.tv_nsec = rq->perout.start.nsec;
>+ igb->period.tv_sec = ts.tv_sec;
>+ igb->period.tv_nsec = ts.tv_nsec;
>+ wr32(E1000_TRGTTIML0, rq->perout.start.sec);
>+ wr32(E1000_TRGTTIMH0, rq->perout.start.nsec);
>+ tsauxc |= TSAUXC_EN_TT0;
>+ tsim |= TSINTR_TT0;
>+ } else {
>+ tsauxc &= ~TSAUXC_EN_TT0;
>+ tsim &= ~TSINTR_TT0;
>+ }
>+ wr32(E1000_TSAUXC, tsauxc);
>+ wr32(E1000_TSIM, tsim);
>+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
>+ return 0;
>+
>+ case PTP_CLK_REQ_PPS:
>+ spin_lock_irqsave(&igb->tmreg_lock, flags);
>+ tsim = rd32(E1000_TSIM);
>+ if (on)
>+ tsim |= TSINTR_SYS_WRAP;
>+ else
>+ tsim &= ~TSINTR_SYS_WRAP;
>+ wr32(E1000_TSIM, tsim);
>+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
>+ return 0;
>+ }
>+
>+ return -EOPNOTSUPP;
>+}
>+
> static int igb_ptp_enable(struct ptp_clock_info *ptp,
> struct ptp_clock_request *rq, int on)
> {
>@@ -711,6 +800,68 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
> -EFAULT : 0;
> }
>
>+static int igb_sdp_init(struct igb_adapter *adapter)
>+{
>+ struct e1000_hw *hw = &adapter->hw;
>+ u32 ctrl, ctrl_ext, tssdp = 0;
>+
>+ if (igb_input_sdp == igb_output_sdp) {
>+ pr_err("SDP %d set as both input and output\n", igb_input_sdp);
>+ return -1;
Shouldn't this return -EINVAL?
>+ }
>+
>+ ctrl = rd32(E1000_CTRL);
>+ ctrl_ext = rd32(E1000_CTRL_EXT);
>+
>+ switch (igb_input_sdp) {
>+ case 0:
>+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP0;
>+ ctrl &= ~E1000_CTRL_SDP0_DIR;
>+ break;
>+ case 1:
>+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP1;
>+ ctrl &= ~E1000_CTRL_SDP1_DIR;
>+ break;
>+ case 2:
>+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP2;
>+ ctrl_ext &= ~E1000_CTRL_EXT_SDP2_DIR;
>+ break;
>+ case 3:
>+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP3;
>+ ctrl_ext &= ~E1000_CTRL_EXT_SDP3_DIR;
>+ break;
>+ default:
>+ pr_err("Input SDP %d out of range\n", igb_input_sdp);
Shouldn't this return -EINVAL as well?
>+ }
>+
>+ switch (igb_output_sdp) {
>+ case 0:
>+ tssdp |= TS_SDP0_SEL_TT0 | TS_SDP0_EN;
>+ ctrl |= E1000_CTRL_SDP0_DIR;
>+ break;
>+ case 1:
>+ tssdp |= TS_SDP1_SEL_TT0 | TS_SDP1_EN;
>+ ctrl |= E1000_CTRL_SDP1_DIR;
>+ break;
>+ case 2:
>+ tssdp |= TS_SDP2_SEL_TT0 | TS_SDP2_EN;
>+ ctrl_ext |= E1000_CTRL_EXT_SDP2_DIR;
>+ break;
>+ case 3:
>+ tssdp |= TS_SDP3_SEL_TT0 | TS_SDP3_EN;
>+ ctrl_ext |= E1000_CTRL_EXT_SDP3_DIR;
>+ break;
>+ default:
>+ pr_err("Output SDP %d out of range\n", igb_output_sdp);
Same here?
>+ }
>+
>+ wr32(E1000_TSSDP, tssdp);
>+ wr32(E1000_CTRL, ctrl);
>+ wr32(E1000_CTRL_EXT, ctrl_ext);
>+
>+ return 0;
>+}
>+
> void igb_ptp_init(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
>@@ -766,7 +917,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
> adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
> adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
> adapter->ptp_caps.settime = igb_ptp_settime_i210;
>- adapter->ptp_caps.enable = igb_ptp_enable;
>+ adapter->ptp_caps.enable = igb_ptp_enable_i210;
> /* Enable the timer functions by clearing bit 31. */
> wr32(E1000_TSAUXC, 0x0);
> break;
>@@ -785,6 +936,10 @@ void igb_ptp_init(struct igb_adapter *adapter)
> struct timespec ts = ktime_to_timespec(ktime_get_real());
>
> igb_ptp_settime_i210(&adapter->ptp_caps, &ts);
>+ igb_sdp_init(adapter);
What if igb_sdp_init fails?
>+ adapter->ptp_caps.n_ext_ts = 1;
>+ adapter->ptp_caps.n_per_out = 1;
>+ adapter->ptp_caps.pps = 1;
> } else {
> timecounter_init(&adapter->tc, &adapter->cc,
> ktime_to_ns(ktime_get_real()));
>--
>1.7.10.4
>
--
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
Powered by blists - more mailing lists