diff mbox

[net-next,v2,1/9] ptp: introduce programmable pins.

Message ID 5faff8ea096044a48e4edcc6391844f8d428383f.1394975663.git.richardcochran@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Richard Cochran March 16, 2014, 1:29 p.m. UTC 
This patch adds a pair of new ioctls to the PTP Hardware Clock device
interface. Using the ioctls, user space programs can query each pin to
find out its current function and also reprogram a different function
if desired.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_chardev.c        |  133 +++++++++++++++++++++++++++++++++++++-
 drivers/ptp/ptp_clock.c          |   23 +++++++
 drivers/ptp/ptp_private.h        |    4 ++
 include/linux/ptp_clock_kernel.h |   33 ++++++++++
 include/uapi/linux/ptp_clock.h   |   39 ++++++++++-
 5 files changed, 230 insertions(+), 2 deletions(-)

Comments

David Miller March 18, 2014, 1:25 a.m. UTC | #1 
From: Richard Cochran <richardcochran@gmail.com>
Date: Sun, 16 Mar 2014 14:29:22 +0100

> +	/* Check to see if any other pin previously had this function. */
> +	if (mutex_lock_interruptible(&ptp->pincfg_mux))
> +		return -ERESTARTSYS;
> +	for (i = 0; i < info->n_pins; i++) {
> +		if (info->pin_config[i].func == func &&
> +		    info->pin_config[i].chan == chan) {
> +			pin1 = &info->pin_config[i];
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ptp->pincfg_mux);
 ...
> +	if (mutex_lock_interruptible(&ptp->pincfg_mux))
> +		return -ERESTARTSYS;
> +	pin2->func = func;
> +	pin2->chan = chan;
> +	if (pin1) {
> +		pin1->func = PTP_PF_NONE;
> +		pin1->chan = 0;
> +	}
> +	mutex_unlock(&ptp->pincfg_mux);
> +
> +	return 0;

This locking seems unnecessarily complex to me.  You should be able to
do the stateless sanity checks, take the mutex, then do all of the
rest of the operations until the end of the function before
dropping the lock.

So just take the lock once over the operations that need it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran March 20, 2014, 8:43 p.m. UTC | #2 
On Mon, Mar 17, 2014 at 09:25:34PM -0400, David Miller wrote:
> 
> This locking seems unnecessarily complex to me.  You should be able to
> do the stateless sanity checks, take the mutex, then do all of the
> rest of the operations until the end of the function before
> dropping the lock.
> 
> So just take the lock once over the operations that need it.

The idea was to avoid holding the mutex when invoking the driver
callbacks (.verify and .enable). Mostly this is my paranoia that some
bad driver will call back into the core via ptp_set_pinfunc().

But you are right that the result is overly complex. I'll make the
callers of ptp_set_pinfunc hold the mutex, and so the set path will
look just like the get path.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller March 20, 2014, 9:13 p.m. UTC | #3 
From: Richard Cochran <richardcochran@gmail.com>
Date: Thu, 20 Mar 2014 21:43:08 +0100

> On Mon, Mar 17, 2014 at 09:25:34PM -0400, David Miller wrote:
>> 
>> This locking seems unnecessarily complex to me.  You should be able to
>> do the stateless sanity checks, take the mutex, then do all of the
>> rest of the operations until the end of the function before
>> dropping the lock.
>> 
>> So just take the lock once over the operations that need it.
> 
> The idea was to avoid holding the mutex when invoking the driver
> callbacks (.verify and .enable). Mostly this is my paranoia that some
> bad driver will call back into the core via ptp_set_pinfunc().

During my review, I checked all the implementations of said methods
and they all universally adjust software state and return.

> But you are right that the result is overly complex. I'll make the
> callers of ptp_set_pinfunc hold the mutex, and so the set path will
> look just like the get path.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 34a0c60..bcbc930 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -25,6 +25,104 @@ 
 
 #include "ptp_private.h"
 
+static int ptp_disable_pinfunc(struct ptp_clock_info *ops,
+			       enum ptp_pin_function func, unsigned int chan)
+{
+	struct ptp_clock_request rq;
+	int err = 0;
+
+	memset(&rq, 0, sizeof(rq));
+
+	switch (func) {
+	case PTP_PF_NONE:
+		break;
+	case PTP_PF_EXTTS:
+		rq.type = PTP_CLK_REQ_EXTTS;
+		rq.extts.index = chan;
+		err = ops->enable(ops, &rq, 0);
+		break;
+	case PTP_PF_PEROUT:
+		rq.type = PTP_CLK_REQ_PEROUT;
+		rq.perout.index = chan;
+		err = ops->enable(ops, &rq, 0);
+		break;
+	case PTP_PF_PHYSYNC:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return err;
+}
+
+int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
+		    enum ptp_pin_function func, unsigned int chan)
+{
+	struct ptp_clock_info *info = ptp->info;
+	struct ptp_pin_desc *pin1 = NULL, *pin2 = &info->pin_config[pin];
+	unsigned int i;
+
+	/* Check to see if any other pin previously had this function. */
+	if (mutex_lock_interruptible(&ptp->pincfg_mux))
+		return -ERESTARTSYS;
+	for (i = 0; i < info->n_pins; i++) {
+		if (info->pin_config[i].func == func &&
+		    info->pin_config[i].chan == chan) {
+			pin1 = &info->pin_config[i];
+			break;
+		}
+	}
+	mutex_unlock(&ptp->pincfg_mux);
+	if (pin1 && i == pin)
+		return 0;
+
+	/* Check the desired function and channel. */
+	switch (func) {
+	case PTP_PF_NONE:
+		break;
+	case PTP_PF_EXTTS:
+		if (chan >= info->n_ext_ts)
+			return -EINVAL;
+		break;
+	case PTP_PF_PEROUT:
+		if (chan >= info->n_per_out)
+			return -EINVAL;
+		break;
+	case PTP_PF_PHYSYNC:
+		pr_err("sorry, cannot reassign the calibration pin\n");
+		return -EINVAL;
+	default:
+		return -EINVAL;
+	}
+
+	if (pin2->func == PTP_PF_PHYSYNC) {
+		pr_err("sorry, cannot reprogram the calibration pin\n");
+		return -EINVAL;
+	}
+
+	if (info->verify(info, pin, func, chan)) {
+		pr_err("driver cannot use function %u on pin %u\n", func, chan);
+		return -EOPNOTSUPP;
+	}
+
+	/* Disable whatever function was previously assigned. */
+	ptp_disable_pinfunc(info, pin2->func, pin2->chan);
+	if (pin1)
+		ptp_disable_pinfunc(info, func, chan);
+
+	if (mutex_lock_interruptible(&ptp->pincfg_mux))
+		return -ERESTARTSYS;
+	pin2->func = func;
+	pin2->chan = chan;
+	if (pin1) {
+		pin1->func = PTP_PF_NONE;
+		pin1->chan = 0;
+	}
+	mutex_unlock(&ptp->pincfg_mux);
+
+	return 0;
+}
+
 int ptp_open(struct posix_clock *pc, fmode_t fmode)
 {
 	return 0;
@@ -35,12 +133,13 @@  long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock_caps caps;
 	struct ptp_clock_request req;
 	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_pin_desc pd;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_clock_time *pct;
 	struct timespec ts;
 	int enable, err = 0;
-	unsigned int i;
+	unsigned int i, pin_index;
 
 	switch (cmd) {
 
@@ -51,6 +150,7 @@  long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		caps.n_ext_ts = ptp->info->n_ext_ts;
 		caps.n_per_out = ptp->info->n_per_out;
 		caps.pps = ptp->info->pps;
+		caps.n_pins = ptp->info->n_pins;
 		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
 			err = -EFAULT;
 		break;
@@ -126,6 +226,37 @@  long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 		break;
 
+	case PTP_PIN_GETFUNC:
+		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
+			err = -EFAULT;
+			break;
+		}
+		pin_index = pd.index;
+		if (pin_index >= ops->n_pins) {
+			err = -EINVAL;
+			break;
+		}
+		if (mutex_lock_interruptible(&ptp->pincfg_mux))
+			return -ERESTARTSYS;
+		pd = ops->pin_config[pin_index];
+		mutex_unlock(&ptp->pincfg_mux);
+		if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
+			err = -EFAULT;
+		break;
+
+	case PTP_PIN_SETFUNC:
+		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
+			err = -EFAULT;
+			break;
+		}
+		pin_index = pd.index;
+		if (pin_index >= ops->n_pins) {
+			err = -EINVAL;
+			break;
+		}
+		err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
+		break;
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index a8319b2..e25d2bc 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -169,6 +169,7 @@  static void delete_ptp_clock(struct posix_clock *pc)
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 
 	mutex_destroy(&ptp->tsevq_mux);
+	mutex_destroy(&ptp->pincfg_mux);
 	ida_simple_remove(&ptp_clocks_map, ptp->index);
 	kfree(ptp);
 }
@@ -203,6 +204,7 @@  struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	ptp->index = index;
 	spin_lock_init(&ptp->tsevq.lock);
 	mutex_init(&ptp->tsevq_mux);
+	mutex_init(&ptp->pincfg_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
 	/* Create a new device in our class. */
@@ -249,6 +251,7 @@  no_sysfs:
 	device_destroy(ptp_class, ptp->devid);
 no_device:
 	mutex_destroy(&ptp->tsevq_mux);
+	mutex_destroy(&ptp->pincfg_mux);
 no_slot:
 	kfree(ptp);
 no_memory:
@@ -305,6 +308,26 @@  int ptp_clock_index(struct ptp_clock *ptp)
 }
 EXPORT_SYMBOL(ptp_clock_index);
 
+int ptp_find_pin(struct ptp_clock *ptp,
+		 enum ptp_pin_function func, unsigned int chan)
+{
+	struct ptp_pin_desc *pin = NULL;
+	int i;
+
+	mutex_lock(&ptp->pincfg_mux);
+	for (i = 0; i < ptp->info->n_pins; i++) {
+		if (ptp->info->pin_config[i].func == func &&
+		    ptp->info->pin_config[i].chan == chan) {
+			pin = &ptp->info->pin_config[i];
+			break;
+		}
+	}
+	mutex_unlock(&ptp->pincfg_mux);
+
+	return pin ? i : -1;
+}
+EXPORT_SYMBOL(ptp_find_pin);
+
 /* module operations */
 
 static void __exit ptp_exit(void)
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index df03f2e..e36bf59 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -48,6 +48,7 @@  struct ptp_clock {
 	long dialed_frequency; /* remembers the frequency adjustment */
 	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
 	struct mutex tsevq_mux; /* one process at a time reading the fifo */
+	struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
 	wait_queue_head_t tsev_wq;
 	int defunct; /* tells readers to go away when clock is being removed */
 };
@@ -69,6 +70,9 @@  static inline int queue_cnt(struct timestamp_event_queue *q)
  * see ptp_chardev.c
  */
 
+int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
+		    enum ptp_pin_function func, unsigned int chan);
+
 long ptp_ioctl(struct posix_clock *pc,
 	       unsigned int cmd, unsigned long arg);
 
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 38a9935..0d8ff3f 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -49,7 +49,11 @@  struct ptp_clock_request {
  * @n_alarm:   The number of programmable alarms.
  * @n_ext_ts:  The number of external time stamp channels.
  * @n_per_out: The number of programmable periodic signals.
+ * @n_pins:    The number of programmable pins.
  * @pps:       Indicates whether the clock supports a PPS callback.
+ * @pin_config: Array of length 'n_pins'. If the number of
+ *              programmable pins is nonzero, then drivers must
+ *              allocate and initialize this array.
  *
  * clock operations
  *
@@ -70,6 +74,18 @@  struct ptp_clock_request {
  *            parameter request: Desired resource to enable or disable.
  *            parameter on: Caller passes one to enable or zero to disable.
  *
+ * @verify:   Confirm that a pin can perform a given function. The PTP
+ *            Hardware Clock subsystem maintains the 'pin_config'
+ *            array on behalf of the drivers, but the PHC subsystem
+ *            assumes that every pin can perform every function. This
+ *            hook gives drivers a way of telling the core about
+ *            limitations on specific pins. This function must return
+ *            zero if the function can be assigned to this pin, and
+ *            nonzero otherwise.
+ *            parameter pin: index of the pin in question.
+ *            parameter func: the desired function to use.
+ *            parameter chan: the function channel index to use.
+ *
  * Drivers should embed their ptp_clock_info within a private
  * structure, obtaining a reference to it using container_of().
  *
@@ -83,13 +99,17 @@  struct ptp_clock_info {
 	int n_alarm;
 	int n_ext_ts;
 	int n_per_out;
+	int n_pins;
 	int pps;
+	struct ptp_pin_desc *pin_config;
 	int (*adjfreq)(struct ptp_clock_info *ptp, s32 delta);
 	int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
 	int (*gettime)(struct ptp_clock_info *ptp, struct timespec *ts);
 	int (*settime)(struct ptp_clock_info *ptp, const struct timespec *ts);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
+	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
+		      enum ptp_pin_function func, unsigned int chan);
 };
 
 struct ptp_clock;
@@ -156,4 +176,17 @@  extern void ptp_clock_event(struct ptp_clock *ptp,
 
 extern int ptp_clock_index(struct ptp_clock *ptp);
 
+/**
+ * ptp_find_pin() - obtain the pin index of a given auxiliary function
+ *
+ * @ptp:    The clock obtained from ptp_clock_register().
+ * @func:   One of the ptp_pin_function enumerated values.
+ * @chan:   The particular functional channel to find.
+ * Return:  Pin index in the range of zero to ptp_clock_caps.n_pins - 1,
+ *          or -1 if the auxiliary function cannot be found.
+ */
+
+int ptp_find_pin(struct ptp_clock *ptp,
+		 enum ptp_pin_function func, unsigned int chan);
+
 #endif
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index b65c834..f0b7bfe 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -50,7 +50,8 @@  struct ptp_clock_caps {
 	int n_ext_ts;  /* Number of external time stamp channels. */
 	int n_per_out; /* Number of programmable periodic signals. */
 	int pps;       /* Whether the clock supports a PPS callback. */
-	int rsv[15];   /* Reserved for future use. */
+	int n_pins;    /* Number of input/output pins. */
+	int rsv[14];   /* Reserved for future use. */
 };
 
 struct ptp_extts_request {
@@ -80,6 +81,40 @@  struct ptp_sys_offset {
 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
 };
 
+enum ptp_pin_function {
+	PTP_PF_NONE,
+	PTP_PF_EXTTS,
+	PTP_PF_PEROUT,
+	PTP_PF_PHYSYNC,
+};
+
+struct ptp_pin_desc {
+	/*
+	 * Hardware specific human readable pin name. This field is
+	 * set by the kernel during the PTP_PIN_GETFUNC ioctl and is
+	 * ignored for the PTP_PIN_SETFUNC ioctl.
+	 */
+	char name[64];
+	/*
+	 * Pin index in the range of zero to ptp_clock_caps.n_pins - 1.
+	 */
+	unsigned int index;
+	/*
+	 * Which of the PTP_PF_xxx functions to use on this pin.
+	 */
+	unsigned int func;
+	/*
+	 * The specific channel to use for this function.
+	 * This corresponds to the 'index' field of the
+	 * PTP_EXTTS_REQUEST and PTP_PEROUT_REQUEST ioctls.
+	 */
+	unsigned int chan;
+	/*
+	 * Reserved for future use.
+	 */
+	unsigned int rsv[5];
+};
+
 #define PTP_CLK_MAGIC '='
 
 #define PTP_CLOCK_GETCAPS  _IOR(PTP_CLK_MAGIC, 1, struct ptp_clock_caps)
@@ -87,6 +122,8 @@  struct ptp_sys_offset {
 #define PTP_PEROUT_REQUEST _IOW(PTP_CLK_MAGIC, 3, struct ptp_perout_request)
 #define PTP_ENABLE_PPS     _IOW(PTP_CLK_MAGIC, 4, int)
 #define PTP_SYS_OFFSET     _IOW(PTP_CLK_MAGIC, 5, struct ptp_sys_offset)
+#define PTP_PIN_GETFUNC    _IOWR(PTP_CLK_MAGIC, 6, struct ptp_pin_desc)
+#define PTP_PIN_SETFUNC    _IOW(PTP_CLK_MAGIC, 7, struct ptp_pin_desc)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */