Re: [PATCH 1/2] PTP: introduce new versions of IOCTLs
From: Joe Perches
Date: Sun Aug 18 2019 - 18:07:28 EST
On Sun, 2019-08-18 at 13:11 -0700, Richard Cochran wrote:
> On Sat, Aug 17, 2019 at 09:17:20AM -0700, Joe Perches wrote:
> > Is there a case where this initialization is
> > unnecessary such that it impacts performance
> > given the use in ptp_ioctl?
>
> None of these ioctls are sensitive WRT performance. They are all
> setup or configuration, or in the case of the OFFSET ioctls, the tiny
> extra delay before the actual measurement will not affect the result.
>
> Thanks,
> Richard
Still, my preference would be to move the struct declarations
into the switch/case blocks where they are used instead of
having a large declaration block at the top of the function.
This minimizes stack use and makes the declarations use {}
Also the original patch deletes 2 case entries for
PTP_PIN_GETFUNC and PTP_PIN_SETFUNC and converts them to
PTP_PIN_GETFUNC2 and PTP_PIN_SETFUNC2 but still uses tests
for the deleted case label entries making part of the case
code block unreachable.
That's at least a defect:
- case PTP_PIN_GETFUNC:
+ case PTP_PIN_GETFUNC2:
and
- case PTP_PIN_SETFUNC:
+ case PTP_PIN_SETFUNC2:
Anyway, leaving aside that nominal defect, which
should probably leave the original case labels in place,
I suggest:
---
drivers/ptp/ptp_chardev.c | 106 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 91 insertions(+), 15 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 18ffe449efdf..a77f12e6326b 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -110,23 +110,17 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
{
struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
struct ptp_sys_offset_extended *extoff = NULL;
- struct ptp_sys_offset_precise precise_offset;
- struct system_device_crosststamp xtstamp;
- struct ptp_clock_info *ops = ptp->info;
struct ptp_sys_offset *sysoff = NULL;
- struct ptp_system_timestamp sts;
- struct ptp_clock_request req;
- struct ptp_clock_caps caps;
- struct ptp_clock_time *pct;
+ struct ptp_clock_info *ops = ptp->info;
unsigned int i, pin_index;
- struct ptp_pin_desc pd;
- struct timespec64 ts;
int enable, err = 0;
switch (cmd) {
case PTP_CLOCK_GETCAPS:
- memset(&caps, 0, sizeof(caps));
+ case PTP_CLOCK_GETCAPS2: {
+ struct ptp_clock_caps caps = {};
+
caps.max_adj = ptp->info->max_adj;
caps.n_alarm = ptp->info->n_alarm;
caps.n_ext_ts = ptp->info->n_ext_ts;
@@ -137,13 +131,28 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
err = -EFAULT;
break;
+ }
case PTP_EXTTS_REQUEST:
+ case PTP_EXTTS_REQUEST2: {
+ struct ptp_clock_request req = {};
+
if (copy_from_user(&req.extts, (void __user *)arg,
sizeof(req.extts))) {
err = -EFAULT;
break;
}
+ if (cmd == PTP_EXTTS_REQUEST2 &&
+ (req.extts.flags || req.extts.rsv[0] || req.extts.rsv[1])) {
+ err = -EINVAL;
+ break;
+ }
+ 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;
@@ -152,13 +161,30 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
err = ops->enable(ops, &req, enable);
break;
+ }
case PTP_PEROUT_REQUEST:
+ case PTP_PEROUT_REQUEST2: {
+ struct ptp_clock_request req = {};
+
if (copy_from_user(&req.perout, (void __user *)arg,
sizeof(req.perout))) {
err = -EFAULT;
break;
}
+ if (cmd == PTP_PEROUT_REQUEST2 &&
+ (req.perout.flags ||
+ req.perout.rsv[0] || req.perout.rsv[1] ||
+ req.perout.rsv[2] || req.perout.rsv[3])) {
+ 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;
@@ -167,16 +193,26 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
enable = req.perout.period.sec || req.perout.period.nsec;
err = ops->enable(ops, &req, enable);
break;
+ }
case PTP_ENABLE_PPS:
+ case PTP_ENABLE_PPS2: {
+ struct ptp_clock_request req = {};
+
if (!capable(CAP_SYS_TIME))
return -EPERM;
req.type = PTP_CLK_REQ_PPS;
enable = arg ? 1 : 0;
err = ops->enable(ops, &req, enable);
break;
+ }
case PTP_SYS_OFFSET_PRECISE:
+ case PTP_SYS_OFFSET_PRECISE2: {
+ struct ptp_sys_offset_precise precise_offset = {};
+ struct system_device_crosststamp xtstamp;
+ struct timespec64 ts;
+
if (!ptp->info->getcrosststamp) {
err = -EOPNOTSUPP;
break;
@@ -185,7 +221,6 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
if (err)
break;
- memset(&precise_offset, 0, sizeof(precise_offset));
ts = ktime_to_timespec64(xtstamp.device);
precise_offset.device.sec = ts.tv_sec;
precise_offset.device.nsec = ts.tv_nsec;
@@ -199,8 +234,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
sizeof(precise_offset)))
err = -EFAULT;
break;
+ }
case PTP_SYS_OFFSET_EXTENDED:
+ case PTP_SYS_OFFSET_EXTENDED2: {
+ struct ptp_system_timestamp sts;
+ struct timespec64 ts;
+
if (!ptp->info->gettimex64) {
err = -EOPNOTSUPP;
break;
@@ -211,8 +251,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
extoff = NULL;
break;
}
- if (extoff->n_samples > PTP_MAX_SAMPLES
- || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+ if (extoff->n_samples > PTP_MAX_SAMPLES ||
+ extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
err = -EINVAL;
break;
}
@@ -230,8 +270,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
if (copy_to_user((void __user *)arg, extoff, sizeof(*extoff)))
err = -EFAULT;
break;
+ }
case PTP_SYS_OFFSET:
+ case PTP_SYS_OFFSET2: {
+ struct timespec64 ts;
+ struct ptp_clock_time *pct;
+
sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
if (IS_ERR(sysoff)) {
err = PTR_ERR(sysoff);
@@ -264,12 +309,27 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
err = -EFAULT;
break;
+ }
+
+ case PTP_PIN_GETFUNC2: {
+ struct ptp_pin_desc pd;
- case PTP_PIN_GETFUNC:
if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
err = -EFAULT;
break;
}
+ if (cmd == PTP_PIN_GETFUNC2 &&
+ (pd.rsv[0] || pd.rsv[1] || pd.rsv[2] || pd.rsv[3] ||
+ pd.rsv[4])) {
+ 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;
@@ -283,12 +343,27 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
err = -EFAULT;
break;
+ }
+
+ case PTP_PIN_SETFUNC2: {
+ struct ptp_pin_desc pd;
- case PTP_PIN_SETFUNC:
if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
err = -EFAULT;
break;
}
+ if (cmd == PTP_PIN_SETFUNC2 &&
+ (pd.rsv[0] || pd.rsv[1] || pd.rsv[2] || pd.rsv[3] ||
+ pd.rsv[4])) {
+ 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;
@@ -300,6 +375,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
mutex_unlock(&ptp->pincfg_mux);
break;
+ }
default:
err = -ENOTTY;