Skip to contentSkip to conversation

ptp: feature enhancements

PATCH In review Richard Cochran 3 participants 12 messages Sep 20, 2011 – Oct 20, 2011

Subsystems (5)
Networking Network drivers Ethernet PHY PTP hardware clock support +1 more subsystems
lore
Share & tools
lore

Richard Cochran

author maintainer
#0 lore raw

cover ptp: feature enhancements

This series adds one driver specific enhancement and one new feature to the PTP Hardware Clock subsystem. The first patch enables more of the phyter's IO capabilities. The second patch introduces PTP one-step support for Sync messages at the driver level. The third patch implements the one-step flag in the phyter.

Richard Cochran (3):
dp83640: enable six external events and one periodic output
net: introduce ptp one step time stamp mode for sync packets
dp83640: add time stamp insertion for sync messages

 drivers/net/phy/dp83640.c  |  205 +++++++++++++++++++++++++++++++++++++-------
 include/linux/net_tstamp.h |    9 ++
 2 files changed, 182 insertions(+), 32 deletions(-)
Signature · 1 line
1.7.2.5
author maintainer
#1 lore raw

PATCH 1/3 dp83640: enable six external events and one periodic output

This patch enables six external event channels and one periodic output. One GPIO is reserved for synchronizing multiple PHYs. The assignment of GPIO functions can be changed via a module parameter.

The code supports multiple simultaneous events by inducing a PTP clock event for every channel marked in the PHY's extended status word.

Signed-off-by Richard Cochran <[email protected]>
---
 drivers/net/phy/dp83640.c |  135 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 116 insertions(+), 19 deletions(-)
drivers/net/phy/dp83640.c +116 -19
index edd7304..d3c6a2e 100644
@@ -35,16 +35,15 @@
 #define LAYER4		0x02
 #define LAYER2		0x01
 #define MAX_RXTS	64
-#define N_EXT_TS	1
+#define N_EXT_TS	6
 #define PSF_PTPVER	2
 #define PSF_EVNT	0x4000
 #define PSF_RX		0x2000
 #define PSF_TX		0x1000
 #define EXT_EVENT	1
-#define EXT_GPIO	1
-#define CAL_EVENT	2
-#define CAL_GPIO	9
-#define CAL_TRIGGER	2
+#define CAL_EVENT	7
+#define CAL_TRIGGER	7
+#define PER_TRIGGER	6
 
 /* phyter seems to miss the mark by 16 ns */
 #define ADJTIME_FIX	16

@@ -131,16 +130,30 @@ struct dp83640_clock {
 
 /* globals */
 
+enum {
+	CALIBRATE_GPIO,
+	PEROUT_GPIO,
+	EXTTS0_GPIO,
+	EXTTS1_GPIO,
+	EXTTS2_GPIO,
+	EXTTS3_GPIO,
+	EXTTS4_GPIO,
+	EXTTS5_GPIO,
+	GPIO_TABLE_SIZE
+};
+
 static int chosen_phy = -1;
-static ushort cal_gpio = 4;
+static ushort gpio_tab[GPIO_TABLE_SIZE] = {
+	1, 2, 3, 4, 8, 9, 10, 11
+};
 
 module_param(chosen_phy, int, 0444);
-module_param(cal_gpio, ushort, 0444);
+module_param_array(gpio_tab, ushort, NULL, 0444);
 
 MODULE_PARM_DESC(chosen_phy, \
 	"The address of the PHY to use for the ancillary clock features");
-MODULE_PARM_DESC(cal_gpio, \
-	"Which GPIO line to use for synchronizing multiple PHYs");
+MODULE_PARM_DESC(gpio_tab, \
+	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
 
 /* a list of clocks and a mutex to protect it */
 static LIST_HEAD(phyter_clocks);

@@ -235,6 +248,61 @@ static u64 phy2txts(struct phy_txts *p)
 	return ns;
 }
 
+static void periodic_output(struct dp83640_clock *clock,
+			    struct ptp_clock_request *clkreq, bool on)
+{
+	struct dp83640_private *dp83640 = clock->chosen;
+	struct phy_device *phydev = dp83640->phydev;
+	u32 sec, nsec, period;
+	u16 gpio, ptp_trig, trigger, val;
+
+	gpio = on ? gpio_tab[PEROUT_GPIO] : 0;
+	trigger = PER_TRIGGER;
+
+	ptp_trig = TRIG_WR |
+		(trigger & TRIG_CSEL_MASK) << TRIG_CSEL_SHIFT |
+		(gpio & TRIG_GPIO_MASK) << TRIG_GPIO_SHIFT |
+		TRIG_PER |
+		TRIG_PULSE;
+
+	val = (trigger & TRIG_SEL_MASK) << TRIG_SEL_SHIFT;
+
+	if (!on) {
+		val |= TRIG_DIS;
+		mutex_lock(&clock->extreg_lock);
+		ext_write(0, phydev, PAGE5, PTP_TRIG, ptp_trig);
+		ext_write(0, phydev, PAGE4, PTP_CTL, val);
+		mutex_unlock(&clock->extreg_lock);
+		return;
+	}
+
+	sec = clkreq->perout.start.sec;
+	nsec = clkreq->perout.start.nsec;
+	period = clkreq->perout.period.sec * 1000000000UL;
+	period += clkreq->perout.period.nsec;
+
+	mutex_lock(&clock->extreg_lock);
+
+	ext_write(0, phydev, PAGE5, PTP_TRIG, ptp_trig);
+
+	/*load trigger*/
+	val |= TRIG_LOAD;
+	ext_write(0, phydev, PAGE4, PTP_CTL, val);
+	ext_write(0, phydev, PAGE4, PTP_TDR, nsec & 0xffff);   /* ns[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, nsec >> 16);      /* ns[31:16] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, sec & 0xffff);    /* sec[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, sec >> 16);       /* sec[31:16] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, period & 0xffff); /* ns[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, period >> 16);    /* ns[31:16] */
+
+	/*enable trigger*/
+	val &= ~TRIG_LOAD;
+	val |= TRIG_EN;
+	ext_write(0, phydev, PAGE4, PTP_CTL, val);
+
+	mutex_unlock(&clock->extreg_lock);
+}
+
 /* ptp clock methods */
 
 static int ptp_dp83640_adjfreq(struct ptp_clock_info *ptp, s32 ppb)

@@ -338,19 +406,30 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp,
 	struct dp83640_clock *clock =
 		container_of(ptp, struct dp83640_clock, caps);
 	struct phy_device *phydev = clock->chosen->phydev;
-	u16 evnt;
+	int index;
+	u16 evnt, event_num, gpio_num;
 
 	switch (rq->type) {
 	case PTP_CLK_REQ_EXTTS:
-		if (rq->extts.index != 0)
+		index = rq->extts.index;
+		if (index < 0 || index >= N_EXT_TS)
 			return -EINVAL;
-		evnt = EVNT_WR | (EXT_EVENT & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
+		event_num = EXT_EVENT + index;
+		evnt = EVNT_WR | (event_num & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
 		if (on) {
-			evnt |= (EXT_GPIO & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
+			gpio_num = gpio_tab[EXTTS0_GPIO + index];
+			evnt |= (gpio_num & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
 			evnt |= EVNT_RISE;
 		}
 		ext_write(0, phydev, PAGE5, PTP_EVNT, evnt);
 		return 0;
+
+	case PTP_CLK_REQ_PEROUT:
+		if (rq->perout.index != 0)
+			return -EINVAL;
+		periodic_output(clock, rq, on);
+		return 0;
+
 	default:
 		break;
 	}

@@ -441,9 +520,10 @@ static void recalibrate(struct dp83640_clock *clock)
 	struct list_head *this;
 	struct dp83640_private *tmp;
 	struct phy_device *master = clock->chosen->phydev;
-	u16 cfg0, evnt, ptp_trig, trigger, val;
+	u16 cal_gpio, cfg0, evnt, ptp_trig, trigger, val;
 
 	trigger = CAL_TRIGGER;
+	cal_gpio = gpio_tab[CALIBRATE_GPIO];
 
 	mutex_lock(&clock->extreg_lock);
 

@@ -542,11 +622,17 @@ static void recalibrate(struct dp83640_clock *clock)
 
 /* time stamping methods */
 
+static inline u16 exts_chan_to_edata(int ch)
+{
+	return 1 << ((ch + EXT_EVENT) * 2);
+}
+
 static int decode_evnt(struct dp83640_private *dp83640,
 		       void *data, u16 ests)
 {
 	struct phy_txts *phy_txts;
 	struct ptp_clock_event event;
+	int i, parsed;
 	int words = (ests >> EVNT_TS_LEN_SHIFT) & EVNT_TS_LEN_MASK;
 	u16 ext_status = 0;
 

@@ -568,14 +654,25 @@ static int decode_evnt(struct dp83640_private *dp83640,
 		dp83640->edata.ns_lo = phy_txts->ns_lo;
 	}
 
+	if (ext_status) {
+		parsed = words + 2;
+	} else {
+		parsed = words + 1;
+		i = ((ests >> EVNT_NUM_SHIFT) & EVNT_NUM_MASK) - EXT_EVENT;
+		ext_status = exts_chan_to_edata(i);
+	}
+
 	event.type = PTP_CLOCK_EXTTS;
-	event.index = 0;
 	event.timestamp = phy2txts(&dp83640->edata);
 
-	ptp_clock_event(dp83640->clock->ptp_clock, &event);
+	for (i = 0; i < N_EXT_TS; i++) {
+		if (ext_status & exts_chan_to_edata(i)) {
+			event.index = i;
+			ptp_clock_event(dp83640->clock->ptp_clock, &event);
+		}
+	}
 
-	words = ext_status ? words + 2 : words + 1;
-	return words * sizeof(u16);
+	return parsed * sizeof(u16);
 }
 
 static void decode_rxts(struct dp83640_private *dp83640,

@@ -740,7 +837,7 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	clock->caps.max_adj	= 1953124;
 	clock->caps.n_alarm	= 0;
 	clock->caps.n_ext_ts	= N_EXT_TS;
-	clock->caps.n_per_out	= 0;
+	clock->caps.n_per_out	= 1;
 	clock->caps.pps		= 0;
 	clock->caps.adjfreq	= ptp_dp83640_adjfreq;
 	clock->caps.adjtime	= ptp_dp83640_adjtime;
Signature · 1 line
1.7.2.5
author maintainer
#2 lore raw

PATCH 2/3 net: introduce ptp one step time stamp mode for sync packets

In reply to Richard Cochran

The IEEE 1588 standard (PTP) has a provision for a "one step" mode, where time stamps on outgoing event packets are inserted into the packet by the hardware on the fly. This patch adds a new flag for the SIOCSHWTSTAMP ioctl that lets user space programs request this mode.

Signed-off-by Richard Cochran <[email protected]>
---
 include/linux/net_tstamp.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
include/linux/net_tstamp.h +9 -0
index a3b8546..3df0984 100644
@@ -60,6 +60,15 @@ enum {
 	 * before sending the packet.
 	 */
 	HWTSTAMP_TX_ON,
+
+	/*
+	 * Enables time stamping for outgoing packets just as
+	 * HWTSTAMP_TX_ON does, but also enables time stamp insertion
+	 * directly into Sync packets. In this case, transmitted Sync
+	 * packets will not received a time stamp via the socket error
+	 * queue.
+	 */
+	HWTSTAMP_TX_ONESTEP_SYNC,
 };
 
 /* possible values for hwtstamp_config->rx_filter */
Signature · 1 line
1.7.2.5
author maintainer
#3 lore raw

PATCH 3/3 dp83640: add time stamp insertion for sync messages

In reply to Richard Cochran

This commit adds one step support to the phyter. When enabled, the hardware does not provide time stamps for transmitted sync messages but instead inserts the stamp into the outgoing packet.

Signed-off-by Richard Cochran <[email protected]>
---
 drivers/net/phy/dp83640.c |   70 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 57 insertions(+), 13 deletions(-)
drivers/net/phy/dp83640.c +57 -13
index d3c6a2e..c588a16 100644
@@ -761,6 +761,41 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 	}
 }
 
+static int is_sync(struct sk_buff *skb, int type)
+{
+	u8 *data = skb->data, *msgtype;
+	unsigned int offset = 0;
+
+	switch (type) {
+	case PTP_CLASS_V1_IPV4:
+	case PTP_CLASS_V2_IPV4:
+		offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
+		break;
+	case PTP_CLASS_V1_IPV6:
+	case PTP_CLASS_V2_IPV6:
+		offset = OFF_PTP6;
+		break;
+	case PTP_CLASS_V2_L2:
+		offset = ETH_HLEN;
+		break;
+	case PTP_CLASS_V2_VLAN:
+		offset = ETH_HLEN + VLAN_HLEN;
+		break;
+	default:
+		return 0;
+	}
+
+	if (type & PTP_CLASS_V1)
+		offset += OFF_PTP_CONTROL;
+
+	if (skb->len < offset + 1)
+		return 0;
+
+	msgtype = data + offset;
+
+	return (*msgtype & 0xf) == 0;
+}
+
 static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
 {
 	u16 *seqid;

@@ -1010,16 +1045,10 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 	if (cfg.flags) /* reserved for future extensions */
 		return -EINVAL;
 
-	switch (cfg.tx_type) {
-	case HWTSTAMP_TX_OFF:
-		dp83640->hwts_tx_en = 0;
-		break;
-	case HWTSTAMP_TX_ON:
-		dp83640->hwts_tx_en = 1;
-		break;
-	default:
+	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
 		return -ERANGE;
-	}
+
+	dp83640->hwts_tx_en = cfg.tx_type;
 
 	switch (cfg.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:

@@ -1074,6 +1103,9 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 	if (dp83640->hwts_tx_en)
 		txcfg0 |= TX_TS_EN;
 
+	if (dp83640->hwts_tx_en == HWTSTAMP_TX_ONESTEP_SYNC)
+		txcfg0 |= SYNC_1STEP | CHK_1STEP;
+
 	if (dp83640->hwts_rx_en)
 		rxcfg0 |= RX_TS_EN;
 

@@ -1156,12 +1188,24 @@ static void dp83640_txtstamp(struct phy_device *phydev,
 {
 	struct dp83640_private *dp83640 = phydev->priv;
 
-	if (!dp83640->hwts_tx_en) {
+	switch (dp83640->hwts_tx_en) {
+
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (is_sync(skb, type)) {
+			kfree_skb(skb);
+			return;
+		}
+		/* fall through */
+	case HWTSTAMP_TX_ON:
+		skb_queue_tail(&dp83640->tx_queue, skb);
+		schedule_work(&dp83640->ts_work);
+		break;
+
+	case HWTSTAMP_TX_OFF:
+	default:
 		kfree_skb(skb);
-		return;
+		break;
 	}
-	skb_queue_tail(&dp83640->tx_queue, skb);
-	schedule_work(&dp83640->ts_work);
 }
 
 static struct phy_driver dp83640_driver = {
Signature · 1 line
1.7.2.5
maintainer
#4 lore raw
In reply to Richard Cochran

From: Richard Cochran <[email protected]>
Date: Tue, 20 Sep 2011 13:43:13 +0200

Quoted 10 lines

This series adds one driver specific enhancement and one new feature to the PTP Hardware Clock subsystem. The first patch enables more of the phyter's IO capabilities. The second patch introduces PTP one-step support for Sync messages at the driver level. The third patch implements the one-step flag in the phyter.

Richard Cochran (3):
dp83640: enable six external events and one periodic output
net: introduce ptp one step time stamp mode for sync packets
dp83640: add time stamp insertion for sync messages

All applied to net-next, thanks.

Could you please add a test at the point where we pull in the ->tx_type value from the user, to make sure it is one of the validly defined HWTSTAMP_TX_FOO values?

Thanks.

author maintainer
#5 lore raw
Quoted 3 lines · David Miller

Could you please add a test at the point where we pull in the ->tx_type value from the user, to make sure it is one of the validly defined HWTSTAMP_TX_FOO values?

Okay, will do. Any chance of getting the bug fixes in for 3.1?

Thanks,
Richard

maintainer
#6 lore raw

From: Richard Cochran <[email protected]>
Date: Wed, 28 Sep 2011 10:00:24 +0200

Quoted 6 lines · David Miller
Quoted 3 lines

Could you please add a test at the point where we pull in the ->tx_type value from the user, to make sure it is one of the validly defined HWTSTAMP_TX_FOO values?

Okay, will do. Any chance of getting the bug fixes in for 3.1?

I plan to try to get them into the next round.

author maintainer
#7 lore raw

cover net: validate HWTSTAMP ioctl parameters

In reply to David Miller

Okay, here is a patch to check the ioctl.

I took the liberty to check both the Rx and Tx fields. I coded the check rather pedantically, using a switch/case over the enumerated values. The advantage of this style is that the compiler will warn if someone adds a new enum into net_tstamp.h without fixing the checking function in dev.c.

Richard Cochran (1):
net: validate HWTSTAMP ioctl parameters

 include/linux/net_tstamp.h |    4 +-
 net/core/dev.c             |   58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.2.5
author maintainer
#8 lore raw

PATCH 1/1 net: validate HWTSTAMP ioctl parameters

This patch adds a sanity check on the values provided by user space for the hardware time stamping configuration. If the values lie outside of the absolute limits, then the ioctl request will be denied.

Signed-off-by Richard Cochran <[email protected]>
---
 include/linux/net_tstamp.h |    4 +-
 net/core/dev.c             |   58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)
include/linux/net_tstamp.h +2 -2
index 3df0984..ae5df12 100644
@@ -45,7 +45,7 @@ struct hwtstamp_config {
 };
 
 /* possible values for hwtstamp_config->tx_type */
-enum {
+enum hwtstamp_tx_types {
 	/*
 	 * No outgoing packet will need hardware time stamping;
 	 * should a packet arrive which asks for it, no hardware

@@ -72,7 +72,7 @@ enum {
 };
 
 /* possible values for hwtstamp_config->rx_filter */
-enum {
+enum hwtstamp_rx_filters {
 	/* time stamp no incoming packet at all */
 	HWTSTAMP_FILTER_NONE,
net/core/dev.c +58 -0

76 lines not on this page · raw message

Signature · 1 line
1.7.2.5
maintainer Applied
#9 lore raw

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

From: Richard Cochran <[email protected]>
Date: Fri, 14 Oct 2011 11:37:48 +0200

Quoted 5 lines

This patch adds a sanity check on the values provided by user space for the hardware time stamping configuration. If the values lie outside of the absolute limits, then the ioctl request will be denied.

Signed-off-by Richard Cochran <[email protected]>

Thanks a lot for following up on this.

Applied, thanks again Richard.

#10 lore raw

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

In reply to Richard Cochran
Quoted 3 lines · Richard Cochran

This patch adds a sanity check on the values provided by user space for the hardware time stamping configuration. If the values lie outside of the absolute limits, then the ioctl request will be denied.

[...]

What does this validation buy us? The driver still has to copy the values into kernel space again, at which point they may have been changed to be invalid. Depending on how the driver uses them (perhaps as array indices), it may have to validate them again to avoid a security vulnerability.

I think that either SIOCHWTSTAMP should be handled through a discrete device operation (not ndo_ioctl) which receives a pointer to the validated structure in kernel memory, or a validation function should be exported to drivers so that they can call it from their ndo_ioctl implementations after copying the structure into kernel memory.

Ben.

Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
author maintainer
#11 lore raw

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

Quoted 11 lines · Ben Hutchings
Quoted 3 lines · Richard Cochran

This patch adds a sanity check on the values provided by user space for the hardware time stamping configuration. If the values lie outside of the absolute limits, then the ioctl request will be denied.

[...]

What does this validation buy us? The driver still has to copy the values into kernel space again, at which point they may have been changed to be invalid. Depending on how the driver uses them (perhaps as array indices), it may have to validate them again to avoid a security vulnerability.

Oops, you are right.

The drivers will really need to check the configuration again in any case, since no driver will support every option.

I understood David's request as simply a sanity check on the absolute limits.

Quoted 3 lines

I think that either SIOCHWTSTAMP should be handled through a discrete device operation (not ndo_ioctl) which receives a pointer to the validated structure in kernel memory,

Okay, I'll take a stab at this.

Quoted 3 lines

or a validation function should be
exported to drivers so that they can call it from their ndo_ioctl implementations after copying the structure into kernel memory.

I think it better to do the sanity check in one place, to guard against lazy or sloppy drivers.

Thanks,
Richard