* [PATCH v2 1/2] PTP: introduce new versions of IOCTLs @ 2019-08-29 9:58 Felipe Balbi 2019-08-29 9:58 ` [PATCH v2 2/2] PTP: add support for one-shot output Felipe Balbi 2019-08-29 17:21 ` [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Richard Cochran 0 siblings, 2 replies; 12+ messages in thread From: Felipe Balbi @ 2019-08-29 9:58 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel, 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 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 | 62 ++++++++++++++++++++++++++++++++++ include/uapi/linux/ptp_clock.h | 12 +++++++ 2 files changed, 74 insertions(+) diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 18ffe449efdf..98ec1395544e 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 || req.extts.rsv[0] || req.extts.rsv[1]) + && cmd == PTP_EXTTS_REQUEST2) { + err = -EINVAL; + break; + } else if (cmd == PTP_EXTTS_REQUEST) { + req.extts.flags = 0; + req.extts.rsv[0] = 0; + req.extts.rsv[1] = 0; + } + if (req.extts.index >= ops->n_ext_ts) { err = -EINVAL; break; @@ -154,11 +169,26 @@ 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 || 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 = 0; + 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 +199,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 +210,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 +235,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 +267,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 +302,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 +334,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..039cd62ec706 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -149,6 +149,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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-29 9:58 [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Felipe Balbi @ 2019-08-29 9:58 ` Felipe Balbi 2019-08-29 17:25 ` Richard Cochran 2019-08-29 17:21 ` [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Richard Cochran 1 sibling, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2019-08-29 9:58 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel, 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 v1: - remove comment from .flags field drivers/ptp/ptp_chardev.c | 5 ++--- include/uapi/linux/ptp_clock.h | 4 +++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 98ec1395544e..a407e5f76e2d 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) err = -EFAULT; break; } - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] - || req.perout.rsv[2] || req.perout.rsv[3]) - && cmd == PTP_PEROUT_REQUEST2) { + if ((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) { diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index 039cd62ec706..95840e5f5c53 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -67,7 +67,9 @@ 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. */ + +#define PTP_PEROUT_ONE_SHOT BIT(0) + unsigned int flags; unsigned int rsv[4]; /* Reserved for future use. */ }; -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-29 9:58 ` [PATCH v2 2/2] PTP: add support for one-shot output Felipe Balbi @ 2019-08-29 17:25 ` Richard Cochran 2019-08-29 17:28 ` Richard Cochran 0 siblings, 1 reply; 12+ messages in thread From: Richard Cochran @ 2019-08-29 17:25 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel On Thu, Aug 29, 2019 at 12:58:25PM +0300, Felipe Balbi wrote: > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index 98ec1395544e..a407e5f76e2d 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > err = -EFAULT; > break; > } > - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] > - || req.perout.rsv[2] || req.perout.rsv[3]) > - && cmd == PTP_PEROUT_REQUEST2) { > + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2] > + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) { Please check that the reserved bits of req.perout.flags, namely ~PTP_PEROUT_ONE_SHOT, are clear. > err = -EINVAL; > break; > } else if (cmd == PTP_PEROUT_REQUEST) { > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 039cd62ec706..95840e5f5c53 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -67,7 +67,9 @@ 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. */ > + > +#define PTP_PEROUT_ONE_SHOT BIT(0) > + unsigned int flags; @davem Any CodingStyle policy on #define within a struct? (Some maintainers won't allow it.) > unsigned int rsv[4]; /* Reserved for future use. */ > }; > > -- > 2.23.0 > Thanks, Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-29 17:25 ` Richard Cochran @ 2019-08-29 17:28 ` Richard Cochran 2019-08-30 8:00 ` Felipe Balbi 0 siblings, 1 reply; 12+ messages in thread From: Richard Cochran @ 2019-08-29 17:28 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem Adding davem onto CC... On Thu, Aug 29, 2019 at 12:58:25PM +0300, Felipe Balbi wrote: > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index 98ec1395544e..a407e5f76e2d 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > err = -EFAULT; > break; > } > - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] > - || req.perout.rsv[2] || req.perout.rsv[3]) > - && cmd == PTP_PEROUT_REQUEST2) { > + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2] > + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) { Please check that the reserved bits of req.perout.flags, namely ~PTP_PEROUT_ONE_SHOT, are clear. > err = -EINVAL; > break; > } else if (cmd == PTP_PEROUT_REQUEST) { > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 039cd62ec706..95840e5f5c53 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -67,7 +67,9 @@ 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. */ > + > +#define PTP_PEROUT_ONE_SHOT BIT(0) > + unsigned int flags; @davem Any CodingStyle policy on #define within a struct? (Some maintainers won't allow it.) > unsigned int rsv[4]; /* Reserved for future use. */ > }; > > -- > 2.23.0 > Thanks, Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-29 17:28 ` Richard Cochran @ 2019-08-30 8:00 ` Felipe Balbi 2019-08-31 14:47 ` Richard Cochran 2019-08-31 15:01 ` Richard Cochran 0 siblings, 2 replies; 12+ messages in thread From: Felipe Balbi @ 2019-08-30 8:00 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel, davem [-- Attachment #1: Type: text/plain, Size: 1969 bytes --] Hi, Richard Cochran <richardcochran@gmail.com> writes: > Adding davem onto CC... > > On Thu, Aug 29, 2019 at 12:58:25PM +0300, Felipe Balbi wrote: >> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c >> index 98ec1395544e..a407e5f76e2d 100644 >> --- a/drivers/ptp/ptp_chardev.c >> +++ b/drivers/ptp/ptp_chardev.c >> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) >> err = -EFAULT; >> break; >> } >> - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] >> - || req.perout.rsv[2] || req.perout.rsv[3]) >> - && cmd == PTP_PEROUT_REQUEST2) { >> + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2] >> + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) { > > Please check that the reserved bits of req.perout.flags, namely > ~PTP_PEROUT_ONE_SHOT, are clear. Actually, we should check more. PEROUT_FEATURE_ENABLE is still valid here, right? So are RISING and FALLING edges, no? > >> err = -EINVAL; >> break; >> } else if (cmd == PTP_PEROUT_REQUEST) { >> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h >> index 039cd62ec706..95840e5f5c53 100644 >> --- a/include/uapi/linux/ptp_clock.h >> +++ b/include/uapi/linux/ptp_clock.h >> @@ -67,7 +67,9 @@ 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. */ >> + >> +#define PTP_PEROUT_ONE_SHOT BIT(0) >> + unsigned int flags; > > @davem Any CodingStyle policy on #define within a struct? (Some > maintainers won't allow it.) seems like this should be defined together with the other flags? If that's the case, it seems like we would EXTTS and PEROUT masks. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-30 8:00 ` Felipe Balbi @ 2019-08-31 14:47 ` Richard Cochran 2019-09-05 10:03 ` Felipe Balbi 2019-08-31 15:01 ` Richard Cochran 1 sibling, 1 reply; 12+ messages in thread From: Richard Cochran @ 2019-08-31 14:47 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote: > >> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) > >> err = -EFAULT; > >> break; > >> } > >> - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] > >> - || req.perout.rsv[2] || req.perout.rsv[3]) > >> - && cmd == PTP_PEROUT_REQUEST2) { > >> + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2] > >> + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) { > > > > Please check that the reserved bits of req.perout.flags, namely > > ~PTP_PEROUT_ONE_SHOT, are clear. > > Actually, we should check more. PEROUT_FEATURE_ENABLE is still valid > here, right? So are RISING and FALLING edges, no? No. The ptp_extts_request.flags are indeed defined: struct ptp_extts_request { ... unsigned int flags; /* Bit field for PTP_xxx flags. */ ... }; But the ptp_perout_request.flags are reserved: struct ptp_perout_request { ... unsigned int flags; /* Reserved for future use. */ ... }; For this ioctl, the test for enable/disable is ptp_perout_request.period is zero: enable = req.perout.period.sec || req.perout.period.nsec; err = ops->enable(ops, &req, enable); The usage pattern here is taken from timer_settime(2). Thanks, Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-31 14:47 ` Richard Cochran @ 2019-09-05 10:03 ` Felipe Balbi 2019-09-06 5:35 ` Richard Cochran 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2019-09-05 10:03 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel, davem [-- Attachment #1: Type: text/plain, Size: 2004 bytes --] Hi, Richard Cochran <richardcochran@gmail.com> writes: > On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote: >> >> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) >> >> err = -EFAULT; >> >> break; >> >> } >> >> - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1] >> >> - || req.perout.rsv[2] || req.perout.rsv[3]) >> >> - && cmd == PTP_PEROUT_REQUEST2) { >> >> + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2] >> >> + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) { >> > >> > Please check that the reserved bits of req.perout.flags, namely >> > ~PTP_PEROUT_ONE_SHOT, are clear. >> >> Actually, we should check more. PEROUT_FEATURE_ENABLE is still valid >> here, right? So are RISING and FALLING edges, no? > > No. The ptp_extts_request.flags are indeed defined: > > struct ptp_extts_request { > ... > unsigned int flags; /* Bit field for PTP_xxx flags. */ > ... > }; > > But the ptp_perout_request.flags are reserved: > > struct ptp_perout_request { > ... > unsigned int flags; /* Reserved for future use. */ > ... > }; This a bit confusing, really. Specially when the comment right above those flags states: /* PTP_xxx bits, for the flags field within the request structures. */ The request "structures" include EXTTS and PEROUT: struct ptp_clock_request { enum { PTP_CLK_REQ_EXTTS, PTP_CLK_REQ_PEROUT, PTP_CLK_REQ_PPS, } type; union { struct ptp_extts_request extts; struct ptp_perout_request perout; }; }; Seems like we will, at least, make it clear which flags are valid for which request structures. > For this ioctl, the test for enable/disable is > ptp_perout_request.period is zero: > > enable = req.perout.period.sec || req.perout.period.nsec; > err = ops->enable(ops, &req, enable); > > The usage pattern here is taken from timer_settime(2). got it -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-09-05 10:03 ` Felipe Balbi @ 2019-09-06 5:35 ` Richard Cochran 0 siblings, 0 replies; 12+ messages in thread From: Richard Cochran @ 2019-09-06 5:35 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem On Thu, Sep 05, 2019 at 01:03:46PM +0300, Felipe Balbi wrote: > This a bit confusing, really. Specially when the comment right above > those flags states: > > /* PTP_xxx bits, for the flags field within the request structures. */ Agreed, it is confusing. Go ahead and remove this comment. > Seems like we will, at least, make it clear which flags are valid for > which request structures. Yes, please do make it as clear as you can. Thanks, Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-30 8:00 ` Felipe Balbi 2019-08-31 14:47 ` Richard Cochran @ 2019-08-31 15:01 ` Richard Cochran 2019-09-05 10:40 ` Felipe Balbi 1 sibling, 1 reply; 12+ messages in thread From: Richard Cochran @ 2019-08-31 15:01 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote: > seems like this should be defined together with the other flags? If > that's the case, it seems like we would EXTTS and PEROUT masks. Yes, let's make the meanings of the bit fields clear... --- ptp_clock.h --- /* * Bits of the ptp_extts_request.flags field: */ #define PTP_ENABLE_FEATURE BIT(0) #define PTP_RISING_EDGE BIT(1) #define PTP_FALLING_EDGE BIT(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_ONE_SHOT BIT(0) #define PTP_PEROUT_VALID_FLAGS (PTP_PEROUT_ONE_SHOT) struct ptp_extts_request { unsigned int flags; /* Bit field of PTP_EXTTS_VALID_FLAGS. */ }; struct ptp_perout_request { unsigned int flags; /* Bit field of PTP_PEROUT_VALID_FLAGS. */ }; Thanks, Richard ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] PTP: add support for one-shot output 2019-08-31 15:01 ` Richard Cochran @ 2019-09-05 10:40 ` Felipe Balbi 0 siblings, 0 replies; 12+ messages in thread From: Felipe Balbi @ 2019-09-05 10:40 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel, davem [-- Attachment #1: Type: text/plain, Size: 8864 bytes --] Hi, Richard Cochran <richardcochran@gmail.com> writes: > On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote: >> seems like this should be defined together with the other flags? If >> that's the case, it seems like we would EXTTS and PEROUT masks. > > Yes, let's make the meanings of the bit fields clear... > > --- ptp_clock.h --- > > /* > * Bits of the ptp_extts_request.flags field: > */ > #define PTP_ENABLE_FEATURE BIT(0) > #define PTP_RISING_EDGE BIT(1) > #define PTP_FALLING_EDGE BIT(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_ONE_SHOT BIT(0) > #define PTP_PEROUT_VALID_FLAGS (PTP_PEROUT_ONE_SHOT) > > struct ptp_extts_request { > unsigned int flags; /* Bit field of PTP_EXTTS_VALID_FLAGS. */ > }; > > struct ptp_perout_request { > unsigned int flags; /* Bit field of PTP_PEROUT_VALID_FLAGS. */ > }; Below you can find the combined patch. Locally, I have it split into two patches, but this lets us look at the full picture. I'll send it as v3 series of two patches on Monday if you like the result. Let me know if you prefer that I convert the flags to BIT() macro calls instead. 8<------------------------------------------------------------------------ From 633a8214c86a43dcf880d7aed33758b576933369 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <felipe.balbi@linux.intel.com> Date: Wed, 14 Aug 2019 10:31:08 +0300 Subject: [PATCH 1/5] PTP: introduce new versions of IOCTLs 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> --- drivers/ptp/ptp_chardev.c | 63 ++++++++++++++++++++++++++++++++++ include/uapi/linux/ptp_clock.h | 26 ++++++++++++-- 2 files changed, 87 insertions(+), 2 deletions(-) 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..cbdc0d97b471 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -25,11 +25,21 @@ #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_ONE_SHOT (1<<0) +#define PTP_PEROUT_VALID_FLAGS (~PTP_PEROUT_ONE_SHOT) /* * struct ptp_clock_time - represents a time value * @@ -67,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. */ }; @@ -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 -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PTP: introduce new versions of IOCTLs 2019-08-29 9:58 [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Felipe Balbi 2019-08-29 9:58 ` [PATCH v2 2/2] PTP: add support for one-shot output Felipe Balbi @ 2019-08-29 17:21 ` Richard Cochran 2019-08-30 7:57 ` Felipe Balbi 1 sibling, 1 reply; 12+ messages in thread From: Richard Cochran @ 2019-08-29 17:21 UTC (permalink / raw) To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel > @@ -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 || req.extts.rsv[0] || req.extts.rsv[1]) > + && cmd == PTP_EXTTS_REQUEST2) { > + err = -EINVAL; > + break; > + } else if (cmd == PTP_EXTTS_REQUEST) { > + req.extts.flags = 0; This still isn't quite right. Sorry that was my fault. The req.extts.flags can be (PTP_ENABLE_FEATURE | PTP_RISING_EDGE | PTP_FALLING_EDGE), and ENABLE is used immediately below in this case. Please #define those bits into a valid mask, and then: - for PTP_EXTTS_REQUEST2 check that ~mask is zero, and - for PTP_EXTTS_REQUEST clear the ~mask bits for the drivers. Thanks again for cleaning this up! Richard > + req.extts.rsv[0] = 0; > + req.extts.rsv[1] = 0; > + } > + > if (req.extts.index >= ops->n_ext_ts) { > err = -EINVAL; > break; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PTP: introduce new versions of IOCTLs 2019-08-29 17:21 ` [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Richard Cochran @ 2019-08-30 7:57 ` Felipe Balbi 0 siblings, 0 replies; 12+ messages in thread From: Felipe Balbi @ 2019-08-30 7:57 UTC (permalink / raw) To: Richard Cochran; +Cc: Christopher S Hall, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1165 bytes --] Hi, Richard Cochran <richardcochran@gmail.com> writes: >> @@ -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 || req.extts.rsv[0] || req.extts.rsv[1]) >> + && cmd == PTP_EXTTS_REQUEST2) { >> + err = -EINVAL; >> + break; >> + } else if (cmd == PTP_EXTTS_REQUEST) { >> + req.extts.flags = 0; > > This still isn't quite right. Sorry that was my fault. > > The req.extts.flags can be (PTP_ENABLE_FEATURE | PTP_RISING_EDGE | > PTP_FALLING_EDGE), and ENABLE is used immediately below in this case. > > Please #define those bits into a valid mask, and then: > > - for PTP_EXTTS_REQUEST2 check that ~mask is zero, and > - for PTP_EXTTS_REQUEST clear the ~mask bits for the drivers. > > Thanks again for cleaning this up! good point. This will actually reduce the size of the patch 2. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-06 5:36 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-29 9:58 [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Felipe Balbi 2019-08-29 9:58 ` [PATCH v2 2/2] PTP: add support for one-shot output Felipe Balbi 2019-08-29 17:25 ` Richard Cochran 2019-08-29 17:28 ` Richard Cochran 2019-08-30 8:00 ` Felipe Balbi 2019-08-31 14:47 ` Richard Cochran 2019-09-05 10:03 ` Felipe Balbi 2019-09-06 5:35 ` Richard Cochran 2019-08-31 15:01 ` Richard Cochran 2019-09-05 10:40 ` Felipe Balbi 2019-08-29 17:21 ` [PATCH v2 1/2] PTP: introduce new versions of IOCTLs Richard Cochran 2019-08-30 7:57 ` Felipe Balbi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).