[Linuxptp-devel] Issues with mutex locking and ptp_find_pin

[Andrew Symington]

Hi there

I just modified the Linux TI CPTS driver to support external hardware time
stamping. In my driver's .enable function callback on the *ptp_clock_info*
struct I made use of the *ptp_find_pin* to map a (function,channel) to a
(pin). Using the testptp.c app I am able to successfully switch to func=1
(EXTTS) mode and time stamping works. However, when I try and switch back
to func=0 (NONE) the testptp application hangs, and I get a kernel panic
(slow mutex).

To repeat:

***@arm:~# testptp -l
name HW1_TS_PUSH index 0 func 0 chan 0
name HW2_TS_PUSH index 1 func 0 chan 1
name HW3_TS_PUSH index 2 func 0 chan 2
name HW4_TS_PUSH index 3 func 0 chan 3
***@arm:~# testptp -i 3 -L 3,1
set pin function okay
***@arm:~# testptp -i 3 -e 1
external time stamp request okay
event index 3 at 946685182.188502679
***@arm:~#
***@arm:~# testptp -i 3 -L 3,0
...
(testptp application hangs)

I believe that the underlying reason for this is that *ptp_find_pin* function
attempts to perform a mutex lock on &ptp->pincfg_mux, which is already
locked by the calling function (see the mutex lock
<https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/ptp/ptp_chardev.c?id=refs/tags/v4.4#n244>
in the *ptp_ioctl* function in /drivers/ptp/ptp_chardev.c).

Perhaps it isn't a good practice to call *ptp_find_pin *from within the
enable function callback in driver code, as done in
/drivers/net/ethernet/intel/igb/igb_ptp.c.

Andrew Symington

[Richard Cochran]

Post by Andrew Symington
I just modified the Linux TI CPTS driver to support external hardware time
stamping. In my driver's .enable function callback on the *ptp_clock_info*
struct I made use of the *ptp_find_pin* to map a (function,channel) to a
(pin). Using the testptp.c app I am able to successfully switch to func=1
(EXTTS) mode and time stamping works. However, when I try and switch back
to func=0 (NONE) the testptp application hangs, and I get a kernel panic
(slow mutex).

It is hard to figure out what went wrong without seeing the code.
Please show us your diff.

Post by Andrew Symington
Perhaps it isn't a good practice to call *ptp_find_pin *from within the
enable function callback in driver code, as done in
/drivers/net/ethernet/intel/igb/igb_ptp.c.

I think igb is working just fine:

# testptp -l
name SDP0 index 0 func 0 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0

# testptp -L 0,1
set pin function okay

# testptp -l
name SDP0 index 0 func 1 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0

# testptp -L 0,0
set pin function okay

# testptp -l
name SDP0 index 0 func 0 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0

Thanks,
Richard

[Andrew Symington]

Hi Richard

Thanks for verifying the behavior on your side.

Having just looked more closely at the igb driver code, I notice that the
*ptp_find_pin* is only ever called when enabling a pin (the "on" argument
must be true). This explains why my code hangs while their code doesn't.

As a temporary measure I have now changed my code to simply lock the pin
number to a channel with the same number, negating the need to call
*ptp_find_pin.*

My original problematic code follow below.

Regards
Andrew


----------

--- KERNEL-CLEAN/drivers/net/ethernet/ti/cpts.c 2015-11-13
12:56:51.000000000 -0800
+++ KERNEL/drivers/net/ethernet/ti/cpts.c 2016-01-14 17:10:10.507652309
-0800
@@ -46,6 +46,11 @@
return (event->high >> EVENT_TYPE_SHIFT) & EVENT_TYPE_MASK;
}

+static int event_port(struct cpts_event *event)
+{
+ return (event->high >> PORT_NUMBER_SHIFT) & PORT_NUMBER_MASK;
+}
+
static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
{
u32 r = cpts_read32(cpts, intstat_raw);
@@ -67,7 +72,7 @@
int i, type = -1;
u32 hi, lo;
struct cpts_event *event;
-
+ struct ptp_clock_event pevent;
for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
if (cpts_fifo_pop(cpts, &hi, &lo))
break;
@@ -81,6 +86,12 @@
event->low = lo;
type = event_type(event);
switch (type) {
+ case CPTS_EV_HW:
+ pevent.timestamp = timecounter_cyc2time(&cpts->tc, event->low);
+ pevent.type = PTP_CLOCK_EXTTS;
+ pevent.index = event_port(event) - 1;
+ ptp_clock_event(cpts->clock, &pevent);
+ break;
case CPTS_EV_PUSH:
case CPTS_EV_RX:
case CPTS_EV_TX:
@@ -89,7 +100,6 @@
break;
case CPTS_EV_ROLL:
case CPTS_EV_HALF:
- case CPTS_EV_HW:
break;
default:
pr_err("cpts: unknown event type\n");
@@ -183,7 +193,7 @@
}

static int cpts_ptp_settime(struct ptp_clock_info *ptp,
- const struct timespec64 *ts)
+ const struct timespec64 *ts)
{
u64 ns;
unsigned long flags;
@@ -198,32 +208,113 @@
return 0;
}

+static int cpts_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ /* Check number of pins */
+ if (pin >= ptp->n_pins || !ptp->pin_config)
+ return -EINVAL;
+
+ /* Lock the channel */
+ if (chan != ptp->pin_config[pin].chan)
+ return -EINVAL;
+
+ /* Check function */
+ switch (func) {
+ case PTP_PF_NONE:
+ case PTP_PF_EXTTS:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int cpts_ptp_enable(struct ptp_clock_info *ptp,
- struct ptp_clock_request *rq, int on)
+ struct ptp_clock_request *rq, int on)
{
+ int pin;
+ u32 ctrl, mask;
+ struct cpts *cpts = container_of(ptp, struct cpts, info);
+ switch(rq->type) {
+ case PTP_CLK_REQ_EXTTS:
+ pin = ptp_find_pin(cpts->clock, PTP_PF_EXTTS, rq->extts.index);
+ if (pin < 0)
+ return -EBUSY;
+ switch (pin) {
+ case 0: mask = HW1_TS_PUSH_EN; break;
+ case 1: mask = HW2_TS_PUSH_EN; break;
+ case 2: mask = HW3_TS_PUSH_EN; break;
+ case 3: mask = HW4_TS_PUSH_EN; break;
+ default:
+ return -EINVAL;
+ }
+ ctrl = cpts_read32(cpts, control);
+ if (on)
+ ctrl |= mask;
+ else
+ ctrl &= ~mask;
+ cpts_write32(cpts, ctrl, control);
+ return 0;
+ default:
+ break;
+ }
return -EOPNOTSUPP;
}

+static struct ptp_pin_desc cpts_pins[4] = {
+ {
+ .name = "HW1_TS_PUSH",
+ .index = 0,
+ .func = PTP_PF_NONE,
+ .chan = 0
+ },
+ {
+ .name = "HW2_TS_PUSH",
+ .index = 1,
+ .func = PTP_PF_NONE,
+ .chan = 1
+ },
+ {
+ .name = "HW3_TS_PUSH",
+ .index = 2,
+ .func = PTP_PF_NONE,
+ .chan = 2
+ },
+ {
+ .name = "HW4_TS_PUSH",
+ .index = 3,
+ .func = PTP_PF_NONE,
+ .chan = 3
+ }
+};
+
static struct ptp_clock_info cpts_info = {
.owner = THIS_MODULE,
.name = "CTPS timer",
.max_adj = 1000000,
- .n_ext_ts = 0,
- .n_pins = 0,
+ .n_alarm = 0,
+ .n_ext_ts = 4,
+ .n_pins = 4,
.pps = 0,
+ .pin_config = cpts_pins,
.adjfreq = cpts_ptp_adjfreq,
.adjtime = cpts_ptp_adjtime,
.gettime64 = cpts_ptp_gettime,
.settime64 = cpts_ptp_settime,
.enable = cpts_ptp_enable,
+ .verify = cpts_ptp_verify,
};

static void cpts_overflow_check(struct work_struct *work)
{
+ u32 ctrl;
struct timespec64 ts;
struct cpts *cpts = container_of(work, struct cpts, overflow_work.work);
-
- cpts_write32(cpts, CPTS_EN, control);
+ ctrl = cpts_read32(cpts, control);
+ ctrl |= CPTS_EN;
+ cpts_write32(cpts, ctrl, control);
cpts_write32(cpts, TS_PEND_EN, int_enable);
cpts_ptp_gettime(&cpts->info, &ts);
pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
@@ -247,7 +338,7 @@
}

static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
- u16 ts_seqid, u8 ts_msgtype)
+ u16 ts_seqid, u8 ts_msgtype)
{
u16 *seqid;
unsigned int offset = 0;
@@ -308,7 +399,7 @@
mtype = (event->high >> MESSAGE_TYPE_SHIFT) & MESSAGE_TYPE_MASK;
seqid = (event->high >> SEQUENCE_ID_SHIFT) & SEQUENCE_ID_MASK;
if (ev_type == event_type(event) &&
- cpts_match(skb, class, seqid, mtype)) {
+ cpts_match(skb, class, seqid, mtype)) {
ns = timecounter_cyc2time(&cpts->tc, event->low);
list_del_init(&event->list);
list_add(&event->list, &cpts->pool);
@@ -353,7 +444,7 @@
#endif /*CONFIG_TI_CPTS*/

int cpts_register(struct device *dev, struct cpts *cpts,
- u32 mult, u32 shift)
+ u32 mult, u32 shift)
{
#ifdef CONFIG_TI_CPTS
int err, i;
@@ -405,4 +496,4 @@
if (cpts->refclk)
cpts_clk_release(cpts);
#endif
-}
+}
\ No newline at end of file

Post by Andrew Symington

Post by Andrew Symington
I just modified the Linux TI CPTS driver to support external hardware

time

Post by Andrew Symington
stamping. In my driver's .enable function callback on the

*ptp_clock_info*

Post by Andrew Symington
struct I made use of the *ptp_find_pin* to map a (function,channel) to a
(pin). Using the testptp.c app I am able to successfully switch to func=1
(EXTTS) mode and time stamping works. However, when I try and switch back
to func=0 (NONE) the testptp application hangs, and I get a kernel panic
(slow mutex).

It is hard to figure out what went wrong without seeing the code.
Please show us your diff.

Post by Andrew Symington
Perhaps it isn't a good practice to call *ptp_find_pin *from within the
enable function callback in driver code, as done in
/drivers/net/ethernet/intel/igb/igb_ptp.c.

# testptp -l
name SDP0 index 0 func 0 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0
# testptp -L 0,1
set pin function okay
# testptp -l
name SDP0 index 0 func 1 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0
# testptp -L 0,0
set pin function okay
# testptp -l
name SDP0 index 0 func 0 chan 0
name SDP1 index 1 func 0 chan 0
name SDP2 index 2 func 0 chan 0
name SDP3 index 3 func 0 chan 0
Thanks,
Richard