linuxptp-devel

[Linuxptp-devel] Issues with mutex locking and ptp_find_pin

[Andrew S. <and...@gm...>]

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:

root@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
root@arm:~# testptp -i 3 -L 3,1
set pin function okay
root@arm:~# testptp -i 3 -e 1
external time stamp request okay
event index 3 at 946685182.188502679
root@arm:~#
root@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

Re: [Linuxptp-devel] Issues with mutex locking and ptp_find_pin

[Richard C. <ric...@gm...>]

On Thu, Jan 14, 2016 at 01:57:02PM -0800, Andrew Symington wrote:
> 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.

> 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

Re: [Linuxptp-devel] Issues with mutex locking and ptp_find_pin

[Andrew S. <and...@gm...>]

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


On Thu, Jan 14, 2016 at 3:01 PM, Richard Cochran <ric...@gm...>
wrote:

> On Thu, Jan 14, 2016 at 01:57:02PM -0800, Andrew Symington wrote:
> > 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.
>
> > 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
>