Re: [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
From: Richard Cochran <richardcochran@gmail.com>
Date: 2018-10-31 11:19:52
Also in:
intel-wired-lan
On Fri, Oct 26, 2018 at 06:27:39PM +0200, Miroslav Lichvar wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 2012551d93e0..1a04c437fd4f 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c@@ -124,11 +124,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_sys_offset_extended *sysoff_extended = NULL;
How about a more succinct name, like 'extoff' ?
struct ptp_sys_offset_precise precise_offset; 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 ptp_system_timestamp sts; struct timespec64 ts; struct system_device_crosststamp xtstamp; int enable, err = 0;
This collection of automatic variables is getting ugly. May I ask you to prefix a patch that puts them into reverse Christmas tree before your changes? (Patch below)
quoted hunk ↗ jump to hunk
@@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) err = -EFAULT; break; + case PTP_SYS_OFFSET_EXTENDED: + if (!ptp->info->gettimex64) { + err = -EOPNOTSUPP; + break; + } + sysoff_extended = memdup_user((void __user *)arg, + sizeof(*sysoff_extended));
As pointed out before, this needs to be freed, and
+ if (IS_ERR(sysoff_extended)) {
+ err = PTR_ERR(sysoff_extended);
+ sysoff = NULL;here you meant, sysoff_extended = NULL;
quoted hunk ↗ jump to hunk
+ break; + } + if (sysoff_extended->n_samples > PTP_MAX_SAMPLES) { + err = -EINVAL; + break; + } + + pct = &sysoff_extended->ts[0]; + for (i = 0; i < sysoff_extended->n_samples; i++) { + err = ptp->info->gettimex64(ptp->info, &sts); + if (err) + break; + pct->sec = sts.sys_ts1.tv_sec; + pct->nsec = sts.sys_ts1.tv_nsec; + pct++; + pct->sec = sts.phc_ts.tv_sec; + pct->nsec = sts.phc_ts.tv_nsec; + pct++; + pct->sec = sts.sys_ts2.tv_sec; + pct->nsec = sts.sys_ts2.tv_nsec; + pct++; + } + if (copy_to_user((void __user *)arg, sysoff_extended, + sizeof(*sysoff_extended))) + err = -EFAULT; + break; + case PTP_SYS_OFFSET: sysoff = memdup_user((void __user *)arg, sizeof(*sysoff)); if (IS_ERR(sysoff)) {diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index 51349d124ee5..79321d929925 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h@@ -39,6 +39,13 @@ struct ptp_clock_request { }; struct system_device_crosststamp; +
KernelDoc please for this:
+struct ptp_system_timestamp {
+ struct timespec64 sys_ts1;
+ struct timespec64 phc_ts;
+ struct timespec64 sys_ts2;
+};
+Thanks, Richard