[PATCH v4 1/2] PTP: introduce new versions of IOCTLs

[PATCH v4 1/2] PTP: introduce new versions of IOCTLs

[Felipe Balbi]

The current version of the IOCTL have a small problem which prevents us
from extending the API by making use of reserved fields. In these new
IOCTLs, we are now making sure that flags and rsv fields are zero which
will allow us to extend the API in the future.

Reviewed-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---

Changes since v3:
	- Remove bogus bitwise negation

Changes since v2:
	- Define PTP_{PEROUT,EXTTS}_VALID_FLAGS
	- Fix comment above PTP_*_FLAGS

Changes since v1:
	- Add a blank line after memset()
	- Move memset(req) to the three places where it's needed
	- Fix the accidental removal of GETFUNC and SETFUNC

 drivers/ptp/ptp_chardev.c      | 63 ++++++++++++++++++++++++++++++++++
 include/uapi/linux/ptp_clock.h | 24 ++++++++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 18ffe449efdf..9c18476d8d10 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -126,7 +126,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	switch (cmd) {
 
 	case PTP_CLOCK_GETCAPS:
+	case PTP_CLOCK_GETCAPS2:
 		memset(&caps, 0, sizeof(caps));
+
 		caps.max_adj = ptp->info->max_adj;
 		caps.n_alarm = ptp->info->n_alarm;
 		caps.n_ext_ts = ptp->info->n_ext_ts;
@@ -139,11 +141,24 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_EXTTS_REQUEST:
+	case PTP_EXTTS_REQUEST2:
+		memset(&req, 0, sizeof(req));
+
 		if (copy_from_user(&req.extts, (void __user *)arg,
 				   sizeof(req.extts))) {
 			err = -EFAULT;
 			break;
 		}
+		if (((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
+			req.extts.rsv[0] || req.extts.rsv[1]) &&
+			cmd == PTP_EXTTS_REQUEST2) {
+			err = -EINVAL;
+			break;
+		} else if (cmd == PTP_EXTTS_REQUEST) {
+			req.extts.flags &= ~PTP_EXTTS_VALID_FLAGS;
+			req.extts.rsv[0] = 0;
+			req.extts.rsv[1] = 0;
+		}
 		if (req.extts.index >= ops->n_ext_ts) {
 			err = -EINVAL;
 			break;
@@ -154,11 +169,27 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_PEROUT_REQUEST:
+	case PTP_PEROUT_REQUEST2:
+		memset(&req, 0, sizeof(req));
+
 		if (copy_from_user(&req.perout, (void __user *)arg,
 				   sizeof(req.perout))) {
 			err = -EFAULT;
 			break;
 		}
+		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
+			req.perout.rsv[0] || req.perout.rsv[1] ||
+			req.perout.rsv[2] || req.perout.rsv[3]) &&
+			cmd == PTP_PEROUT_REQUEST2) {
+			err = -EINVAL;
+			break;
+		} else if (cmd == PTP_PEROUT_REQUEST) {
+			req.perout.flags &= ~PTP_PEROUT_VALID_FLAGS;
+			req.perout.rsv[0] = 0;
+			req.perout.rsv[1] = 0;
+			req.perout.rsv[2] = 0;
+			req.perout.rsv[3] = 0;
+		}
 		if (req.perout.index >= ops->n_per_out) {
 			err = -EINVAL;
 			break;
@@ -169,6 +200,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_ENABLE_PPS:
+	case PTP_ENABLE_PPS2:
+		memset(&req, 0, sizeof(req));
+
 		if (!capable(CAP_SYS_TIME))
 			return -EPERM;
 		req.type = PTP_CLK_REQ_PPS;
@@ -177,6 +211,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET_PRECISE:
+	case PTP_SYS_OFFSET_PRECISE2:
 		if (!ptp->info->getcrosststamp) {
 			err = -EOPNOTSUPP;
 			break;
@@ -201,6 +236,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET_EXTENDED:
+	case PTP_SYS_OFFSET_EXTENDED2:
 		if (!ptp->info->gettimex64) {
 			err = -EOPNOTSUPP;
 			break;
@@ -232,6 +268,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET:
+	case PTP_SYS_OFFSET2:
 		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
 		if (IS_ERR(sysoff)) {
 			err = PTR_ERR(sysoff);
@@ -266,10 +303,23 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_PIN_GETFUNC:
+	case PTP_PIN_GETFUNC2:
 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
 			err = -EFAULT;
 			break;
 		}
+		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
+				|| pd.rsv[3] || pd.rsv[4])
+			&& cmd == PTP_PIN_GETFUNC2) {
+			err = -EINVAL;
+			break;
+		} else if (cmd == PTP_PIN_GETFUNC) {
+			pd.rsv[0] = 0;
+			pd.rsv[1] = 0;
+			pd.rsv[2] = 0;
+			pd.rsv[3] = 0;
+			pd.rsv[4] = 0;
+		}
 		pin_index = pd.index;
 		if (pin_index >= ops->n_pins) {
 			err = -EINVAL;
@@ -285,10 +335,23 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_PIN_SETFUNC:
+	case PTP_PIN_SETFUNC2:
 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
 			err = -EFAULT;
 			break;
 		}
+		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
+				|| pd.rsv[3] || pd.rsv[4])
+			&& cmd == PTP_PIN_SETFUNC2) {
+			err = -EINVAL;
+			break;
+		} else if (cmd == PTP_PIN_SETFUNC) {
+			pd.rsv[0] = 0;
+			pd.rsv[1] = 0;
+			pd.rsv[2] = 0;
+			pd.rsv[3] = 0;
+			pd.rsv[4] = 0;
+		}
 		pin_index = pd.index;
 		if (pin_index >= ops->n_pins) {
 			err = -EINVAL;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1bc794ad957a..9a0af3511b68 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -25,10 +25,20 @@
 #include <linux/ioctl.h>
 #include <linux/types.h>
 
-/* PTP_xxx bits, for the flags field within the request structures. */
+/*
+ * Bits of the ptp_extts_request.flags field:
+ */
 #define PTP_ENABLE_FEATURE (1<<0)
 #define PTP_RISING_EDGE    (1<<1)
 #define PTP_FALLING_EDGE   (1<<2)
+#define PTP_EXTTS_VALID_FLAGS	(PTP_ENABLE_FEATURE |	\
+				 PTP_RISING_EDGE |	\
+				 PTP_FALLING_EDGE)
+
+/*
+ * Bits of the ptp_perout_request.flags field:
+ */
+#define PTP_PEROUT_VALID_FLAGS (0)
 
 /*
  * struct ptp_clock_time - represents a time value
@@ -149,6 +159,18 @@ struct ptp_pin_desc {
 #define PTP_SYS_OFFSET_EXTENDED \
 	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
 
+#define PTP_CLOCK_GETCAPS2  _IOR(PTP_CLK_MAGIC, 10, struct ptp_clock_caps)
+#define PTP_EXTTS_REQUEST2  _IOW(PTP_CLK_MAGIC, 11, struct ptp_extts_request)
+#define PTP_PEROUT_REQUEST2 _IOW(PTP_CLK_MAGIC, 12, struct ptp_perout_request)
+#define PTP_ENABLE_PPS2     _IOW(PTP_CLK_MAGIC, 13, int)
+#define PTP_SYS_OFFSET2     _IOW(PTP_CLK_MAGIC, 14, struct ptp_sys_offset)
+#define PTP_PIN_GETFUNC2    _IOWR(PTP_CLK_MAGIC, 15, struct ptp_pin_desc)
+#define PTP_PIN_SETFUNC2    _IOW(PTP_CLK_MAGIC, 16, struct ptp_pin_desc)
+#define PTP_SYS_OFFSET_PRECISE2 \
+	_IOWR(PTP_CLK_MAGIC, 17, struct ptp_sys_offset_precise)
+#define PTP_SYS_OFFSET_EXTENDED2 \
+	_IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
+
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
 	unsigned int index;      /* Which channel produced the event. */
-- 
2.23.0

[PATCH v4 2/2] PTP: add support for one-shot output

[Felipe Balbi]

Some controllers allow for a one-shot output pulse, in contrast to
periodic output. Now that we have extensible versions of our IOCTLs, we
can finally make use of the 'flags' field to pass a bit telling driver
that if we want one-shot pulse output.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---

Changes since v3:
	- Remove bogus bitwise negation

Changes since v2:
	- Add _PEROUT_ to bit macro

Changes since v1:
	- remove comment from .flags field

 include/uapi/linux/ptp_clock.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 9a0af3511b68..f16301015949 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -38,8 +38,8 @@
 /*
  * Bits of the ptp_perout_request.flags field:
  */
-#define PTP_PEROUT_VALID_FLAGS (0)
-
+#define PTP_PEROUT_ONE_SHOT (1<<0)
+#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
 /*
  * struct ptp_clock_time - represents a time value
  *
@@ -77,7 +77,7 @@ struct ptp_perout_request {
 	struct ptp_clock_time start;  /* Absolute start time. */
 	struct ptp_clock_time period; /* Desired period, zero means disable. */
 	unsigned int index;           /* Which channel to configure. */
-	unsigned int flags;           /* Reserved for future use. */
+	unsigned int flags;
 	unsigned int rsv[4];          /* Reserved for future use. */
 };
 
-- 
2.23.0

Re: [PATCH v4 2/2] PTP: add support for one-shot output

[Richard Cochran]

On Wed, Sep 11, 2019 at 09:16:22AM +0300, Felipe Balbi wrote:
> Some controllers allow for a one-shot output pulse, in contrast to
> periodic output. Now that we have extensible versions of our IOCTLs, we
> can finally make use of the 'flags' field to pass a bit telling driver
> that if we want one-shot pulse output.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> 
> Changes since v3:
> 	- Remove bogus bitwise negation
> 
> Changes since v2:
> 	- Add _PEROUT_ to bit macro
> 
> Changes since v1:
> 	- remove comment from .flags field

Reviewed-by: Richard Cochran <richardcochran@gmail.com>

@davem, these two are good to go!

Thanks,
Richard

Re: [PATCH v4 2/2] PTP: add support for one-shot output

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Thu, 12 Sep 2019 09:56:09 -0700

> On Wed, Sep 11, 2019 at 09:16:22AM +0300, Felipe Balbi wrote:
>> Some controllers allow for a one-shot output pulse, in contrast to
>> periodic output. Now that we have extensible versions of our IOCTLs, we
>> can finally make use of the 'flags' field to pass a bit telling driver
>> that if we want one-shot pulse output.
>> 
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>> 
>> Changes since v3:
>> 	- Remove bogus bitwise negation
>> 
>> Changes since v2:
>> 	- Add _PEROUT_ to bit macro
>> 
>> Changes since v1:
>> 	- remove comment from .flags field
> 
> Reviewed-by: Richard Cochran <richardcochran@gmail.com>
> 
> @davem, these two are good to go!

Ok, thanks for reviewing.

Re: [PATCH v4 1/2] PTP: introduce new versions of IOCTLs

[David Miller]

From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed, 11 Sep 2019 09:16:21 +0300

> The current version of the IOCTL have a small problem which prevents us
> from extending the API by making use of reserved fields. In these new
> IOCTLs, we are now making sure that flags and rsv fields are zero which
> will allow us to extend the API in the future.
> 
> Reviewed-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

Applied.

Re: [PATCH v4 2/2] PTP: add support for one-shot output

[David Miller]

From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed, 11 Sep 2019 09:16:22 +0300

> Some controllers allow for a one-shot output pulse, in contrast to
> periodic output. Now that we have extensible versions of our IOCTLs, we
> can finally make use of the 'flags' field to pass a bit telling driver
> that if we want one-shot pulse output.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

Applied.

RE: [PATCH v4 2/2] PTP: add support for one-shot output

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Felipe Balbi
> Sent: Tuesday, September 10, 2019 11:16 PM
> To: Richard Cochran <richardcochran@gmail.com>
> Cc: Hall, Christopher S <christopher.s.hall@intel.com>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Felipe Balbi <felipe.balbi@linux.intel.com>
> Subject: [PATCH v4 2/2] PTP: add support for one-shot output
> 
> Some controllers allow for a one-shot output pulse, in contrast to
> periodic output. Now that we have extensible versions of our IOCTLs, we
> can finally make use of the 'flags' field to pass a bit telling driver
> that if we want one-shot pulse output.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> 
> Changes since v3:
> 	- Remove bogus bitwise negation
> 
> Changes since v2:
> 	- Add _PEROUT_ to bit macro
> 
> Changes since v1:
> 	- remove comment from .flags field
> 
>  include/uapi/linux/ptp_clock.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 9a0af3511b68..f16301015949 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -38,8 +38,8 @@
>  /*
>   * Bits of the ptp_perout_request.flags field:
>   */
> -#define PTP_PEROUT_VALID_FLAGS (0)
> -
> +#define PTP_PEROUT_ONE_SHOT (1<<0)
> +#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
>  /*
>   * struct ptp_clock_time - represents a time value
>   *
> @@ -77,7 +77,7 @@ struct ptp_perout_request {
>  	struct ptp_clock_time start;  /* Absolute start time. */
>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
> -	unsigned int flags;           /* Reserved for future use. */
> +	unsigned int flags;
>  	unsigned int rsv[4];          /* Reserved for future use. */
>  };
> 
> --
> 2.23.0

Hi Felipe,

Do you have any examples for how you envision using this? I don't see any drivers or other code on the list for doing so.

Additionally, it seems weird because we do not have support for specifying the pulse width. I guess you leave that up to driver choice?

Thanks,
Jake

RE: [PATCH v4 2/2] PTP: add support for one-shot output

[Hall, Christopher S]

> -----Original Message-----
> From: Keller, Jacob E
> Sent: Tuesday, September 24, 2019 12:23 PM
> To: Felipe Balbi <felipe.balbi@linux.intel.com>; Richard Cochran
> <richardcochran@gmail.com>
> Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v4 2/2] PTP: add support for one-shot output
> 
> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On
> > Behalf Of Felipe Balbi
> > Sent: Tuesday, September 10, 2019 11:16 PM
> > To: Richard Cochran <richardcochran@gmail.com>
> > Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Felipe Balbi
> <felipe.balbi@linux.intel.com>
> > Subject: [PATCH v4 2/2] PTP: add support for one-shot output
> >
> > Some controllers allow for a one-shot output pulse, in contrast to
> > periodic output. Now that we have extensible versions of our IOCTLs, we
> > can finally make use of the 'flags' field to pass a bit telling driver
> > that if we want one-shot pulse output.
> >
> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > ---
> >
> > Changes since v3:
> > 	- Remove bogus bitwise negation
> >
> > Changes since v2:
> > 	- Add _PEROUT_ to bit macro
> >
> > Changes since v1:
> > 	- remove comment from .flags field
> >
> >  include/uapi/linux/ptp_clock.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/uapi/linux/ptp_clock.h
> b/include/uapi/linux/ptp_clock.h
> > index 9a0af3511b68..f16301015949 100644
> > --- a/include/uapi/linux/ptp_clock.h
> > +++ b/include/uapi/linux/ptp_clock.h
> > @@ -38,8 +38,8 @@
> >  /*
> >   * Bits of the ptp_perout_request.flags field:
> >   */
> > -#define PTP_PEROUT_VALID_FLAGS (0)
> > -
> > +#define PTP_PEROUT_ONE_SHOT (1<<0)
> > +#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
> >  /*
> >   * struct ptp_clock_time - represents a time value
> >   *
> > @@ -77,7 +77,7 @@ struct ptp_perout_request {
> >  	struct ptp_clock_time start;  /* Absolute start time. */
> >  	struct ptp_clock_time period; /* Desired period, zero means disable.
> */
> >  	unsigned int index;           /* Which channel to configure. */
> > -	unsigned int flags;           /* Reserved for future use. */
> > +	unsigned int flags;
> >  	unsigned int rsv[4];          /* Reserved for future use. */
> >  };
> >
> > --
> > 2.23.0
> 
> Hi Felipe,
> 
> Do you have any examples for how you envision using this? I don't see any
> drivers or other code on the list for doing so.
> 
> Additionally, it seems weird because we do not have support for specifying
> the pulse width. I guess you leave that up to driver choice?
> 
> Thanks,
> Jake

Jake,

Good catch on the terminology. This is an API that produces edges not pulses.
This flag causes the PEROUT ioctl to ignore the period argument and produce a
single edge. Currently, the igb driver implements the same function, but uses
a "magic" invalid period specification to signal that the period argument
should be ignored (use_freq == 0):

		if (on && ((ns <= 70000000LL) || (ns == 125000000LL) ||
			   (ns == 250000000LL) || (ns == 500000000LL))) {
			if (ns < 8LL)
				return -EINVAL;
			use_freq = 1;
		}

The proposal is to support this function without magic period specifications
using an explicit flag instead. An example use case is pulse-per-second
output. While PPS is periodic, time-aware GPIO is driven by (an
unadjustable) Always Running Timer (ART). It's necessary to schedule each
edge in software to produce PPS synced with system time.

Chris

RE: [PATCH v4 2/2] PTP: add support for one-shot output

[Keller, Jacob E]

> -----Original Message-----
> From: Hall, Christopher S
> Sent: Tuesday, September 24, 2019 1:24 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Felipe Balbi
> <felipe.balbi@linux.intel.com>; Richard Cochran <richardcochran@gmail.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v4 2/2] PTP: add support for one-shot output
> 
> Good catch on the terminology. This is an API that produces edges not pulses.
> This flag causes the PEROUT ioctl to ignore the period argument and produce a
> single edge. Currently, the igb driver implements the same function, but uses
> a "magic" invalid period specification to signal that the period argument
> should be ignored (use_freq == 0):
> 
> 		if (on && ((ns <= 70000000LL) || (ns == 125000000LL) ||
> 			   (ns == 250000000LL) || (ns == 500000000LL))) {
> 			if (ns < 8LL)
> 				return -EINVAL;
> 			use_freq = 1;
> 		}

From my understanding, the use_freq = 0 is intended to perform a clock using the target time registers with an interrupt to re-trigger the next toggle.

If you use a frequency not supported by freqout, it will result in an interrupt that re-toggles the target time, not a single edge.

> 
> The proposal is to support this function without magic period specifications
> using an explicit flag instead. An example use case is pulse-per-second
> output. While PPS is periodic, time-aware GPIO is driven by (an
> unadjustable) Always Running Timer (ART). It's necessary to schedule each
> edge in software to produce PPS synced with system time.
> 
> Chris

Oh, so "one shot" will simply toggle the clock output once. I see.

So this won't really work for generating a pulse per second, and we would possibly still want an API for that?

Thanks,
Jake

RE: [PATCH v4 2/2] PTP: add support for one-shot output

[Keller, Jacob E]

> -----Original Message-----
> From: Hall, Christopher S
> Sent: Tuesday, September 24, 2019 1:24 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Felipe Balbi
> <felipe.balbi@linux.intel.com>; Richard Cochran <richardcochran@gmail.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v4 2/2] PTP: add support for one-shot output
> 
> > -----Original Message-----
> > From: Keller, Jacob E
> > Sent: Tuesday, September 24, 2019 12:23 PM
> > To: Felipe Balbi <felipe.balbi@linux.intel.com>; Richard Cochran
> > <richardcochran@gmail.com>
> > Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH v4 2/2] PTP: add support for one-shot output
> >
> >
> >
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> > On
> > > Behalf Of Felipe Balbi
> > > Sent: Tuesday, September 10, 2019 11:16 PM
> > > To: Richard Cochran <richardcochran@gmail.com>
> > > Cc: Hall, Christopher S <christopher.s.hall@intel.com>;
> > netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Felipe Balbi
> > <felipe.balbi@linux.intel.com>
> > > Subject: [PATCH v4 2/2] PTP: add support for one-shot output
> > >
> > > Some controllers allow for a one-shot output pulse, in contrast to
> > > periodic output. Now that we have extensible versions of our IOCTLs, we
> > > can finally make use of the 'flags' field to pass a bit telling driver
> > > that if we want one-shot pulse output.
> > >
> > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > ---
> > >
> > > Changes since v3:
> > > 	- Remove bogus bitwise negation
> > >
> > > Changes since v2:
> > > 	- Add _PEROUT_ to bit macro
> > >
> > > Changes since v1:
> > > 	- remove comment from .flags field
> > >
> > >  include/uapi/linux/ptp_clock.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/ptp_clock.h
> > b/include/uapi/linux/ptp_clock.h
> > > index 9a0af3511b68..f16301015949 100644
> > > --- a/include/uapi/linux/ptp_clock.h
> > > +++ b/include/uapi/linux/ptp_clock.h
> > > @@ -38,8 +38,8 @@
> > >  /*
> > >   * Bits of the ptp_perout_request.flags field:
> > >   */
> > > -#define PTP_PEROUT_VALID_FLAGS (0)
> > > -
> > > +#define PTP_PEROUT_ONE_SHOT (1<<0)
> > > +#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
> > >  /*
> > >   * struct ptp_clock_time - represents a time value
> > >   *
> > > @@ -77,7 +77,7 @@ struct ptp_perout_request {
> > >  	struct ptp_clock_time start;  /* Absolute start time. */
> > >  	struct ptp_clock_time period; /* Desired period, zero means disable.
> > */
> > >  	unsigned int index;           /* Which channel to configure. */
> > > -	unsigned int flags;           /* Reserved for future use. */
> > > +	unsigned int flags;
> > >  	unsigned int rsv[4];          /* Reserved for future use. */
> > >  };
> > >
> > > --
> > > 2.23.0
> >
> > Hi Felipe,
> >
> > Do you have any examples for how you envision using this? I don't see any
> > drivers or other code on the list for doing so.
> >
> > Additionally, it seems weird because we do not have support for specifying
> > the pulse width. I guess you leave that up to driver choice?
> >
> > Thanks,
> > Jake
> 

Also a quick note/question:

Is there a spot where flags are explicitly checked and rejected? I don't see any driver which would reject this as "not an acceptable configuration".

I.e. if a function calls the PEROUT_REQUEST2 ioctl, they will pass the flag through, and drivers today don't seem to bother checking flags at all.

I think we also need a patch so that all drivers are updated to reject non-zero flags, ensuring that they do not attempt to configure a request incorrectly.

Thanks,
Jake