| Message ID | 20230928133544.3642650-4-reibax@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Support for multiple filtered timestamp event queue readers | expand |
Xabier Marquiegui <reibax@gmail.com> writes: > Implement ioctl to support filtering of external timestamp event queue > channels per reader based on the process PID accessing the timestamp > queue. > > Can be tested using testptp test binary. Use lsof to figure out readers > of the DUT. LSB of the timestamp channel mask is channel 0. > > eg: To view all current users of the device: > ``` > # testptp -F /dev/ptp0 > (USER PID) TSEVQ FILTER ID:MASK > (3234) 1:0x00000001 > (3692) 2:0xFFFFFFFF > (3792) 3:0xFFFFFFFF > (8713) 4:0xFFFFFFFF > ``` > > eg: To allow ID 1 to access only ts channel 0: > ``` > # testptp -F 1,0x1 > ``` > > eg: To allow ID 1 to access any channel: > ``` > # testptp -F 1,0xFFFFFFFF > ``` > > Signed-off-by: Xabier Marquiegui <reibax@gmail.com> > Suggested-by: Richard Cochran <richardcochran@gmail.com> > --- > v3: > - filter application by object id, aided by process id > - friendlier testptp implementation of event queue channel filters > v2: https://lore.kernel.org/netdev/20230912220217.2008895-3-reibax@gmail.com/ > - fix testptp compilation error: unknown type name 'pid_t' > - rename mask variable for easier code traceability > - more detailed commit message with two examples > v1: https://lore.kernel.org/netdev/20230906104754.1324412-4-reibax@gmail.com/ > > drivers/ptp/ptp_chardev.c | 85 +++++++++++++- > drivers/ptp/ptp_clock.c | 4 +- > drivers/ptp/ptp_private.h | 1 + > include/uapi/linux/ptp_clock.h | 12 ++ > tools/testing/selftests/ptp/testptp.c | 158 ++++++++++++++++++++------ > 5 files changed, 221 insertions(+), 39 deletions(-) > > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index 65e7acaa40a9..14b5bd7e7ca2 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -114,6 +114,7 @@ int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode) > if (!queue) > return -EINVAL; > queue->close_req = false; > + queue->mask = 0xFFFFFFFF; > queue->reader_pid = task_pid_nr(current); > spin_lock_init(&queue->lock); > queue->ida = ida; > @@ -169,19 +170,28 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > { > struct ptp_clock *ptp = > container_of(pcuser->clk, struct ptp_clock, clock); > + struct ptp_tsfilter tsfilter_set, *tsfilter_get = NULL; > 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 timestamp_event_queue *tsevq; > struct ptp_system_timestamp sts; > struct ptp_clock_request req; > struct ptp_clock_caps caps; > struct ptp_clock_time *pct; > + int lsize, enable, err = 0; > unsigned int i, pin_index; > struct ptp_pin_desc pd; > struct timespec64 ts; > - int enable, err = 0; > + > + tsevq = pcuser->private_clkdata; > + > + if (tsevq->close_req) { > + err = -EPIPE; > + return err; > + } > > switch (cmd) { > > @@ -481,6 +491,79 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > mutex_unlock(&ptp->pincfg_mux); > break; > > + case PTP_FILTERCOUNT_REQUEST: > + /* Calculate amount of device users */ > + if (tsevq) { > + lsize = list_count_nodes(&tsevq->qlist); > + if (copy_to_user((void __user *)arg, &lsize, > + sizeof(lsize))) > + err = -EFAULT; > + } > + break; > + case PTP_FILTERTS_GET_REQUEST: > + /* Read operation */ > + /* Read amount of entries expected */ > + if (copy_from_user(&tsfilter_set, (void __user *)arg, > + sizeof(tsfilter_set))) { > + err = -EFAULT; > + break; > + } > + if (tsfilter_set.ndevusers <= 0) { > + err = -EINVAL; > + break; > + } > + /* Allocate the necessary memory space to dump the requested filter > + * list > + */ > + tsfilter_get = kzalloc(tsfilter_set.ndevusers * > + sizeof(struct ptp_tsfilter), > + GFP_KERNEL); > + if (!tsfilter_get) { > + err = -ENOMEM; > + break; > + } > + if (!tsevq) { > + err = -EFAULT; > + break; > + } > + /* Set the whole region to 0 in case the current list is shorter than > + * anticipated > + */ > + memset(tsfilter_get, 0, > + tsfilter_set.ndevusers * sizeof(struct ptp_tsfilter)); > + i = 0; > + /* Format data */ > + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > + tsfilter_get[i].reader_rpid = tsevq->reader_pid; > + tsfilter_get[i].reader_oid = tsevq->oid; > + tsfilter_get[i].mask = tsevq->mask; > + i++; > + /* Current list is longer than anticipated */ > + if (i >= tsfilter_set.ndevusers) > + break; > + } > + /* Dump data */ > + if (copy_to_user((void __user *)arg, tsfilter_get, > + tsfilter_set.ndevusers * > + sizeof(struct ptp_tsfilter))) > + err = -EFAULT; > + break; > + > + case PTP_FILTERTS_SET_REQUEST: > + /* Write Operation */ > + if (copy_from_user(&tsfilter_set, (void __user *)arg, > + sizeof(tsfilter_set))) { > + err = -EFAULT; > + break; > + } > + if (tsevq) { > + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > + if (tsevq->oid == tsfilter_set.reader_oid) > + tsevq->mask = tsfilter_set.mask; > + } > + } > + break; > + > default: > err = -ENOTTY; > break; > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index 9e271ad66933..6284eaad5f53 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -280,6 +280,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, > if (!queue) > goto no_memory_queue; > queue->close_req = false; > + queue->mask = 0xFFFFFFFF; > queue->ida = kzalloc(sizeof(*queue->ida), GFP_KERNEL); > if (!queue->ida) > goto no_memory_queue; > @@ -449,7 +450,8 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) > case PTP_CLOCK_EXTTS: > /* Enqueue timestamp on all other queues */ > list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > - enqueue_external_timestamp(tsevq, event); > + if (tsevq->mask & (0x1 << event->index)) > + enqueue_external_timestamp(tsevq, event); > } > wake_up_interruptible(&ptp->tsev_wq); > break; > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > index 529d3d421ba0..c8ff2272f837 100644 > --- a/drivers/ptp/ptp_private.h > +++ b/drivers/ptp/ptp_private.h > @@ -32,6 +32,7 @@ struct timestamp_event_queue { > pid_t reader_pid; > struct ida *ida; > int oid; > + int mask; > bool close_req; > }; > > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 05cc35fc94ac..6bbf11dc4a05 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -105,6 +105,15 @@ struct ptp_extts_request { > unsigned int rsv[2]; /* Reserved for future use. */ > }; > > +struct ptp_tsfilter { > + union { > + unsigned int reader_rpid; /* PID of device user */ > + unsigned int ndevusers; /* Device user count */ > + }; > + int reader_oid; /* Object ID of the timestamp event queue */ > + unsigned int mask; /* Channel mask. LSB = channel 0 */ > +}; > + > struct ptp_perout_request { > union { > /* > @@ -224,6 +233,9 @@ struct ptp_pin_desc { > _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) > +#define PTP_FILTERTS_SET_REQUEST _IOW(PTP_CLK_MAGIC, 19, struct ptp_tsfilter) > +#define PTP_FILTERCOUNT_REQUEST _IOR(PTP_CLK_MAGIC, 20, int) > +#define PTP_FILTERTS_GET_REQUEST _IOWR(PTP_CLK_MAGIC, 21, struct ptp_tsfilter) > Looking below, at the usability of the API, it feels too complicated, I was trying to think, "how an application would change the mask for itself": first it would need to know the PID of the process that created the fd, then it would have to find the OID associated with that PID, and then build the request. And it has the problem of being error prone, for example, it's easy for an application to override the mask of another, either by mistake or else. My suggestion is to keep things simple, the "SET" only receives the 'mask', and it only changes the mask for that particular fd (which you already did the hard work of allowing that). Seems to be less error prone. At least in my mental model, I don't think much else is needed (we expose only a "SET" operation), at least from the UAPI side of things. For "debugging", i.e. discovering which applications have what masks, then perhaps we could do it "on the side", for example, a debugfs entry that lists all open file descriptors and their masks. Just an idea. What do you think? > struct ptp_extts_event { > struct ptp_clock_time t; /* Time event occured. */ > diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c > index c9f6cca4feb4..e7ff22d60d63 100644 > --- a/tools/testing/selftests/ptp/testptp.c > +++ b/tools/testing/selftests/ptp/testptp.c > @@ -22,6 +22,7 @@ > #include <sys/types.h> > #include <time.h> > #include <unistd.h> > +#include <stdbool.h> > > #include <linux/ptp_clock.h> > > @@ -117,35 +118,36 @@ static void usage(char *progname) > { > fprintf(stderr, > "usage: %s [options]\n" > - " -c query the ptp clock's capabilities\n" > - " -d name device to open\n" > - " -e val read 'val' external time stamp events\n" > - " -f val adjust the ptp clock frequency by 'val' ppb\n" > - " -g get the ptp clock time\n" > - " -h prints this message\n" > - " -i val index for event/trigger\n" > - " -k val measure the time offset between system and phc clock\n" > - " for 'val' times (Maximum 25)\n" > - " -l list the current pin configuration\n" > - " -L pin,val configure pin index 'pin' with function 'val'\n" > - " the channel index is taken from the '-i' option\n" > - " 'val' specifies the auxiliary function:\n" > - " 0 - none\n" > - " 1 - external time stamp\n" > - " 2 - periodic output\n" > - " -n val shift the ptp clock time by 'val' nanoseconds\n" > - " -o val phase offset (in nanoseconds) to be provided to the PHC servo\n" > - " -p val enable output with a period of 'val' nanoseconds\n" > - " -H val set output phase to 'val' nanoseconds (requires -p)\n" > - " -w val set output pulse width to 'val' nanoseconds (requires -p)\n" > - " -P val enable or disable (val=1|0) the system clock PPS\n" > - " -s set the ptp clock time from the system time\n" > - " -S set the system time from the ptp clock time\n" > - " -t val shift the ptp clock time by 'val' seconds\n" > - " -T val set the ptp clock time to 'val' seconds\n" > - " -x val get an extended ptp clock time with the desired number of samples (up to %d)\n" > - " -X get a ptp clock cross timestamp\n" > - " -z test combinations of rising/falling external time stamp flags\n", > + " -c query the ptp clock's capabilities\n" > + " -d name device to open\n" > + " -e val read 'val' external time stamp events\n" > + " -f val adjust the ptp clock frequency by 'val' ppb\n" > + " -F [oid,msk] with no arguments, list the users of the device\n" > + " -g get the ptp clock time\n" > + " -h prints this message\n" > + " -i val index for event/trigger\n" > + " -k val measure the time offset between system and phc clock\n" > + " for 'val' times (Maximum 25)\n" > + " -l list the current pin configuration\n" > + " -L pin,val configure pin index 'pin' with function 'val'\n" > + " the channel index is taken from the '-i' option\n" > + " 'val' specifies the auxiliary function:\n" > + " 0 - none\n" > + " 1 - external time stamp\n" > + " 2 - periodic output\n" > + " -n val shift the ptp clock time by 'val' nanoseconds\n" > + " -o val phase offset (in nanoseconds) to be provided to the PHC servo\n" > + " -p val enable output with a period of 'val' nanoseconds\n" > + " -H val set output phase to 'val' nanoseconds (requires -p)\n" > + " -w val set output pulse width to 'val' nanoseconds (requires -p)\n" > + " -P val enable or disable (val=1|0) the system clock PPS\n" > + " -s set the ptp clock time from the system time\n" > + " -S set the system time from the ptp clock time\n" > + " -t val shift the ptp clock time by 'val' seconds\n" > + " -T val set the ptp clock time to 'val' seconds\n" > + " -x val get an extended ptp clock time with the desired number of samples (up to %d)\n" > + " -X get a ptp clock cross timestamp\n" > + " -z test combinations of rising/falling external time stamp flags\n", > progname, PTP_MAX_SAMPLES); > } > > @@ -162,6 +164,7 @@ int main(int argc, char *argv[]) > struct ptp_sys_offset *sysoff; > struct ptp_sys_offset_extended *soe; > struct ptp_sys_offset_precise *xts; > + struct ptp_tsfilter tsfilter, *tsfilter_read; > > char *progname; > unsigned int i; > @@ -187,6 +190,7 @@ int main(int argc, char *argv[]) > int pps = -1; > int seconds = 0; > int settime = 0; > + int rvalue = 0; > > int64_t t1, t2, tp; > int64_t interval, offset; > @@ -194,9 +198,17 @@ int main(int argc, char *argv[]) > int64_t pulsewidth = -1; > int64_t perout = -1; > > + tsfilter_read = NULL; > + tsfilter.ndevusers = 0; > + tsfilter.reader_oid = 0; > + tsfilter.mask = 0xFFFFFFFF; > + bool opt_tsfilter = false; > + > progname = strrchr(argv[0], '/'); > progname = progname ? 1+progname : argv[0]; > - while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) { > + while (EOF != > + (c = getopt(argc, argv, > + "cd:e:f:F:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) { > switch (c) { > case 'c': > capabilities = 1; > @@ -210,6 +222,15 @@ int main(int argc, char *argv[]) > case 'f': > adjfreq = atoi(optarg); > break; > + case 'F': > + opt_tsfilter = true; > + cnt = sscanf(optarg, "%d,%X", &tsfilter.reader_oid, > + &tsfilter.mask); > + if (cnt != 2 && cnt != 0) { > + usage(progname); > + return -1; > + } > + break; > case 'g': > gettime = 1; > break; > @@ -295,7 +316,8 @@ int main(int argc, char *argv[]) > clkid = get_clockid(fd); > if (CLOCK_INVALID == clkid) { > fprintf(stderr, "failed to read clock id\n"); > - return -1; > + rvalue = -1; > + goto exit; > } > > if (capabilities) { > @@ -464,18 +486,21 @@ int main(int argc, char *argv[]) > > if (pulsewidth >= 0 && perout < 0) { > puts("-w can only be specified together with -p"); > - return -1; > + rvalue = -1; > + goto exit; > } > > if (perout_phase >= 0 && perout < 0) { > puts("-H can only be specified together with -p"); > - return -1; > + rvalue = -1; > + goto exit; > } > > if (perout >= 0) { > if (clock_gettime(clkid, &ts)) { > perror("clock_gettime"); > - return -1; > + rvalue = -1; > + goto exit; > } > memset(&perout_request, 0, sizeof(perout_request)); > perout_request.index = index; > @@ -516,13 +541,15 @@ int main(int argc, char *argv[]) > if (n_samples <= 0 || n_samples > 25) { > puts("n_samples should be between 1 and 25"); > usage(progname); > - return -1; > + rvalue = -1; > + goto exit; > } > > sysoff = calloc(1, sizeof(*sysoff)); > if (!sysoff) { > perror("calloc"); > - return -1; > + rvalue = -1; > + goto exit; > } > sysoff->n_samples = n_samples; > > @@ -604,6 +631,63 @@ int main(int argc, char *argv[]) > free(xts); > } > > + if (opt_tsfilter) { > + if (tsfilter.reader_oid) { > + /* Set a filter for a specific object id */ > + if (ioctl(fd, PTP_FILTERTS_SET_REQUEST, &tsfilter)) { > + perror("PTP_FILTERTS_SET_REQUEST"); > + rvalue = -1; > + goto exit; > + } > + printf("Timestamp event queue mask 0x%X applied to reader with oid: %d\n", > + (int)tsfilter.mask, tsfilter.reader_oid); > + > + } else { > + /* List all filters */ > + if (ioctl(fd, PTP_FILTERCOUNT_REQUEST, > + &tsfilter.ndevusers)) { > + perror("PTP_FILTERTS_SET_REQUEST"); > + rvalue = -1; > + goto exit; > + } > + tsfilter_read = calloc(tsfilter.ndevusers, > + sizeof(*tsfilter_read)); > + /* > + * Get a variable length result from the IOCTL. We use a value > + * inside the structure we are willing to read to communicate the > + * IOCTL how many elements we are expecting to get. > + * It's ok if the size of the list changed between these two operations, > + * this is just an approximation to be able to test the concept. > + */ > + tsfilter_read[0].ndevusers = tsfilter.ndevusers; > + if (!tsfilter_read) { > + perror("tsfilter_read calloc"); > + rvalue = -1; > + goto exit; > + } > + if (ioctl(fd, PTP_FILTERTS_GET_REQUEST, > + tsfilter_read)) { > + perror("PTP_FILTERTS_GET_REQUEST"); > + rvalue = -1; > + goto exit; > + } > + printf("(USER PID)\tTSEVQ FILTER ID:MASK\n"); > + for (i = 0; i < tsfilter.ndevusers; i++) { > + if (tsfilter_read[i].reader_oid) > + printf("(%d)\t\t%5d:0x%08X\n", > + tsfilter_read[i].reader_rpid, > + tsfilter_read[i].reader_oid, > + tsfilter_read[i].mask); > + } > + } > + } > + > +exit: > + if (tsfilter_read) { > + free(tsfilter_read); > + tsfilter_read = NULL; > + } > + > close(fd); > - return 0; > + return rvalue; > } > -- > 2.34.1 >
Vinicius Costa Gomes <vinicius.gomes@intel.com> writes: > Looking below, at the usability of the API, it feels too complicated, I > was trying to think, "how an application would change the mask for > itself": first it would need to know the PID of the process that created > the fd, then it would have to find the OID associated with that PID, and > then build the request. > > And it has the problem of being error prone, for example, it's easy for > an application to override the mask of another, either by mistake or > else. > > My suggestion is to keep things simple, the "SET" only receives the > 'mask', and it only changes the mask for that particular fd (which you > already did the hard work of allowing that). Seems to be less error prone. > > At least in my mental model, I don't think much else is needed (we > expose only a "SET" operation), at least from the UAPI side of things. > > For "debugging", i.e. discovering which applications have what masks, > then perhaps we could do it "on the side", for example, a debugfs entry > that lists all open file descriptors and their masks. Just an idea. > > What do you think? Thank you very much for your input Vinicius. I really appreciate it. I totally agree with your observations. I had already thought about that angle myself, but I decided to go this route anyway because it was the only way I could think of meeting all of Richard's requirements at that time. Even if being error prone, being able to externally manipulate the channel masks is the only way I can think of to make this feature backwards compatible with existing software. One example of a piece of software that would need to be updated to support multiple channels is linuxptp. If you try to start ts2phc with multiple channels enabled and no masks, it refuses to work stating that unwanted channels are present. This would be easy to fix, incorporating the SET operation you mention, but it is still something that needs to be changed. Now that I think of it, it is true that nothing prevents us from having both methods available: the simple and safe, and the complicated and unsafe. Even with that option, I also think that going exclusively with the safe and simple route is better. So, I wonder: Can we just do it and require changes in software that relies on this driver, or should we maintain compatibility at all cost? Thank you very much for sharing your knowledge and experience.
On Thu, Sep 28, 2023 at 03:35:44PM +0200, Xabier Marquiegui wrote: > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 05cc35fc94ac..6bbf11dc4a05 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -105,6 +105,15 @@ struct ptp_extts_request { > unsigned int rsv[2]; /* Reserved for future use. */ > }; > > +struct ptp_tsfilter { > + union { > + unsigned int reader_rpid; /* PID of device user */ > + unsigned int ndevusers; /* Device user count */ > + }; > + int reader_oid; /* Object ID of the timestamp event queue */ > + unsigned int mask; /* Channel mask. LSB = channel 0 */ > +}; This is WAY too complicated. Just let the user pass a bit mask (say 32 x uint64_t = 2048 channels) of what they want to receive. OR: - ioctl to clear mask - ioctl to set one channel in mask After opening the character device, the default is that the user will receive events from all channels. This is an established API, and you cannot change it. If the user is only interested in specific channels, then they can set the mask after calling open(). Thanks, Richard
On Thu, Sep 28, 2023 at 03:35:44PM +0200, Xabier Marquiegui wrote: > Implement ioctl to support filtering of external timestamp event queue > channels per reader based on the process PID accessing the timestamp > queue. > > Can be tested using testptp test binary. Use lsof to figure out readers > of the DUT. LSB of the timestamp channel mask is channel 0. > > eg: To view all current users of the device: > ``` > # testptp -F /dev/ptp0 > (USER PID) TSEVQ FILTER ID:MASK > (3234) 1:0x00000001 > (3692) 2:0xFFFFFFFF > (3792) 3:0xFFFFFFFF > (8713) 4:0xFFFFFFFF > ``` > > eg: To allow ID 1 to access only ts channel 0: > ``` > # testptp -F 1,0x1 > ``` > > eg: To allow ID 1 to access any channel: > ``` > # testptp -F 1,0xFFFFFFFF > ``` > > Signed-off-by: Xabier Marquiegui <reibax@gmail.com> > Suggested-by: Richard Cochran <richardcochran@gmail.com> Hi Xabier, please find some more feedback from Smatch inline. ... > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c ... > @@ -169,19 +170,28 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > { > struct ptp_clock *ptp = > container_of(pcuser->clk, struct ptp_clock, clock); > + struct ptp_tsfilter tsfilter_set, *tsfilter_get = NULL; > 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 timestamp_event_queue *tsevq; > struct ptp_system_timestamp sts; > struct ptp_clock_request req; > struct ptp_clock_caps caps; > struct ptp_clock_time *pct; > + int lsize, enable, err = 0; > unsigned int i, pin_index; > struct ptp_pin_desc pd; > struct timespec64 ts; > - int enable, err = 0; > + > + tsevq = pcuser->private_clkdata; > + > + if (tsevq->close_req) { > + err = -EPIPE; > + return err; > + } Here tseqv is dereferenced unconditionally... > > switch (cmd) { > > @@ -481,6 +491,79 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > mutex_unlock(&ptp->pincfg_mux); > break; > > + case PTP_FILTERCOUNT_REQUEST: > + /* Calculate amount of device users */ > + if (tsevq) { ... but here it is assumed that tseqv might be NULL. As flagged by Smatch. > + lsize = list_count_nodes(&tsevq->qlist); > + if (copy_to_user((void __user *)arg, &lsize, > + sizeof(lsize))) > + err = -EFAULT; > + } > + break; > + case PTP_FILTERTS_GET_REQUEST: > + /* Read operation */ > + /* Read amount of entries expected */ > + if (copy_from_user(&tsfilter_set, (void __user *)arg, > + sizeof(tsfilter_set))) { > + err = -EFAULT; > + break; > + } > + if (tsfilter_set.ndevusers <= 0) { > + err = -EINVAL; > + break; > + } > + /* Allocate the necessary memory space to dump the requested filter > + * list > + */ > + tsfilter_get = kzalloc(tsfilter_set.ndevusers * > + sizeof(struct ptp_tsfilter), > + GFP_KERNEL); > + if (!tsfilter_get) { > + err = -ENOMEM; > + break; > + } > + if (!tsevq) { Ditto. > + err = -EFAULT; > + break; > + } > + /* Set the whole region to 0 in case the current list is shorter than > + * anticipated > + */ > + memset(tsfilter_get, 0, > + tsfilter_set.ndevusers * sizeof(struct ptp_tsfilter)); > + i = 0; > + /* Format data */ > + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > + tsfilter_get[i].reader_rpid = tsevq->reader_pid; > + tsfilter_get[i].reader_oid = tsevq->oid; > + tsfilter_get[i].mask = tsevq->mask; > + i++; > + /* Current list is longer than anticipated */ > + if (i >= tsfilter_set.ndevusers) > + break; > + } > + /* Dump data */ > + if (copy_to_user((void __user *)arg, tsfilter_get, > + tsfilter_set.ndevusers * > + sizeof(struct ptp_tsfilter))) > + err = -EFAULT; > + break; > + > + case PTP_FILTERTS_SET_REQUEST: > + /* Write Operation */ > + if (copy_from_user(&tsfilter_set, (void __user *)arg, > + sizeof(tsfilter_set))) { > + err = -EFAULT; > + break; > + } > + if (tsevq) { Ditto. > + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > + if (tsevq->oid == tsfilter_set.reader_oid) > + tsevq->mask = tsfilter_set.mask; > + } > + } > + break; > + > default: > err = -ENOTTY; > break; ...
On Sun, Oct 01, 2023 at 05:12:02PM +0200, Simon Horman wrote: > > @@ -169,19 +170,28 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > > { > > struct ptp_clock *ptp = > > container_of(pcuser->clk, struct ptp_clock, clock); > > + struct ptp_tsfilter tsfilter_set, *tsfilter_get = NULL; > > 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 timestamp_event_queue *tsevq; > > struct ptp_system_timestamp sts; > > struct ptp_clock_request req; > > struct ptp_clock_caps caps; > > struct ptp_clock_time *pct; > > + int lsize, enable, err = 0; > > unsigned int i, pin_index; > > struct ptp_pin_desc pd; > > struct timespec64 ts; > > - int enable, err = 0; > > + > > + tsevq = pcuser->private_clkdata; > > + > > + if (tsevq->close_req) { > > + err = -EPIPE; > > + return err; > > + } > > Here tseqv is dereferenced unconditionally... Which is correct because the pointer is always set during open(). > > > > > switch (cmd) { > > > > @@ -481,6 +491,79 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, > > mutex_unlock(&ptp->pincfg_mux); > > break; > > > > + case PTP_FILTERCOUNT_REQUEST: > > + /* Calculate amount of device users */ > > + if (tsevq) { > > ... but here it is assumed that tseqv might be NULL. Which is incorrect. The test is pointless. Thanks, Richard
Xabier Marquiegui <reibax@gmail.com> writes: > Vinicius Costa Gomes <vinicius.gomes@intel.com> writes: > >> Looking below, at the usability of the API, it feels too complicated, I >> was trying to think, "how an application would change the mask for >> itself": first it would need to know the PID of the process that created >> the fd, then it would have to find the OID associated with that PID, and >> then build the request. >> >> And it has the problem of being error prone, for example, it's easy for >> an application to override the mask of another, either by mistake or >> else. >> >> My suggestion is to keep things simple, the "SET" only receives the >> 'mask', and it only changes the mask for that particular fd (which you >> already did the hard work of allowing that). Seems to be less error prone. >> >> At least in my mental model, I don't think much else is needed (we >> expose only a "SET" operation), at least from the UAPI side of things. >> >> For "debugging", i.e. discovering which applications have what masks, >> then perhaps we could do it "on the side", for example, a debugfs entry >> that lists all open file descriptors and their masks. Just an idea. >> >> What do you think? > > Thank you very much for your input Vinicius. I really appreciate it. > > I totally agree with your observations. I had already thought about that angle > myself, but I decided to go this route anyway because it was the only way I > could think of meeting all of Richard's requirements at that time. > > Even if being error prone, being able to externally manipulate the channel > masks is the only way I can think of to make this feature backwards compatible > with existing software. One example of a piece of software that would need to > be updated to support multiple channels is linuxptp. If you try to start ts2phc > with multiple channels enabled and no masks, it refuses to work stating that > unwanted channels are present. This would be easy to fix, incorporating the > SET operation you mention, but it is still something that needs to be changed. > I never looked at this a lot, so, as always, I could be missing stuff. But from the way I see things, the solution that seems better has two parts: 1. Fix ts2phc to ignore events from channels that it cannot/doesn't want to handle. (Is this possible?) 2. Add the "set mask ioctl/alternative ideas, is then more like a optimization, to avoid waking up applications that don't want some events; So we have 'ts2phc' working on "old" kernels and on "new" kernels it is "just" more efficient. > Now that I think of it, it is true that nothing prevents us from having both > methods available: the simple and safe, and the complicated and unsafe. > > Even with that option, I also think that going exclusively with the safe > and simple route is better. > > So, I wonder: Can we just do it and require changes in software that relies > on this driver, or should we maintain compatibility at all cost? > > Thank you very much for sharing your knowledge and experience. Cheers,
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 65e7acaa40a9..14b5bd7e7ca2 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -114,6 +114,7 @@ int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode) if (!queue) return -EINVAL; queue->close_req = false; + queue->mask = 0xFFFFFFFF; queue->reader_pid = task_pid_nr(current); spin_lock_init(&queue->lock); queue->ida = ida; @@ -169,19 +170,28 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, { struct ptp_clock *ptp = container_of(pcuser->clk, struct ptp_clock, clock); + struct ptp_tsfilter tsfilter_set, *tsfilter_get = NULL; 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 timestamp_event_queue *tsevq; struct ptp_system_timestamp sts; struct ptp_clock_request req; struct ptp_clock_caps caps; struct ptp_clock_time *pct; + int lsize, enable, err = 0; unsigned int i, pin_index; struct ptp_pin_desc pd; struct timespec64 ts; - int enable, err = 0; + + tsevq = pcuser->private_clkdata; + + if (tsevq->close_req) { + err = -EPIPE; + return err; + } switch (cmd) { @@ -481,6 +491,79 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd, mutex_unlock(&ptp->pincfg_mux); break; + case PTP_FILTERCOUNT_REQUEST: + /* Calculate amount of device users */ + if (tsevq) { + lsize = list_count_nodes(&tsevq->qlist); + if (copy_to_user((void __user *)arg, &lsize, + sizeof(lsize))) + err = -EFAULT; + } + break; + case PTP_FILTERTS_GET_REQUEST: + /* Read operation */ + /* Read amount of entries expected */ + if (copy_from_user(&tsfilter_set, (void __user *)arg, + sizeof(tsfilter_set))) { + err = -EFAULT; + break; + } + if (tsfilter_set.ndevusers <= 0) { + err = -EINVAL; + break; + } + /* Allocate the necessary memory space to dump the requested filter + * list + */ + tsfilter_get = kzalloc(tsfilter_set.ndevusers * + sizeof(struct ptp_tsfilter), + GFP_KERNEL); + if (!tsfilter_get) { + err = -ENOMEM; + break; + } + if (!tsevq) { + err = -EFAULT; + break; + } + /* Set the whole region to 0 in case the current list is shorter than + * anticipated + */ + memset(tsfilter_get, 0, + tsfilter_set.ndevusers * sizeof(struct ptp_tsfilter)); + i = 0; + /* Format data */ + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { + tsfilter_get[i].reader_rpid = tsevq->reader_pid; + tsfilter_get[i].reader_oid = tsevq->oid; + tsfilter_get[i].mask = tsevq->mask; + i++; + /* Current list is longer than anticipated */ + if (i >= tsfilter_set.ndevusers) + break; + } + /* Dump data */ + if (copy_to_user((void __user *)arg, tsfilter_get, + tsfilter_set.ndevusers * + sizeof(struct ptp_tsfilter))) + err = -EFAULT; + break; + + case PTP_FILTERTS_SET_REQUEST: + /* Write Operation */ + if (copy_from_user(&tsfilter_set, (void __user *)arg, + sizeof(tsfilter_set))) { + err = -EFAULT; + break; + } + if (tsevq) { + list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { + if (tsevq->oid == tsfilter_set.reader_oid) + tsevq->mask = tsfilter_set.mask; + } + } + break; + default: err = -ENOTTY; break; diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 9e271ad66933..6284eaad5f53 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -280,6 +280,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, if (!queue) goto no_memory_queue; queue->close_req = false; + queue->mask = 0xFFFFFFFF; queue->ida = kzalloc(sizeof(*queue->ida), GFP_KERNEL); if (!queue->ida) goto no_memory_queue; @@ -449,7 +450,8 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) case PTP_CLOCK_EXTTS: /* Enqueue timestamp on all other queues */ list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { - enqueue_external_timestamp(tsevq, event); + if (tsevq->mask & (0x1 << event->index)) + enqueue_external_timestamp(tsevq, event); } wake_up_interruptible(&ptp->tsev_wq); break; diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 529d3d421ba0..c8ff2272f837 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -32,6 +32,7 @@ struct timestamp_event_queue { pid_t reader_pid; struct ida *ida; int oid; + int mask; bool close_req; }; diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index 05cc35fc94ac..6bbf11dc4a05 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -105,6 +105,15 @@ struct ptp_extts_request { unsigned int rsv[2]; /* Reserved for future use. */ }; +struct ptp_tsfilter { + union { + unsigned int reader_rpid; /* PID of device user */ + unsigned int ndevusers; /* Device user count */ + }; + int reader_oid; /* Object ID of the timestamp event queue */ + unsigned int mask; /* Channel mask. LSB = channel 0 */ +}; + struct ptp_perout_request { union { /* @@ -224,6 +233,9 @@ struct ptp_pin_desc { _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) +#define PTP_FILTERTS_SET_REQUEST _IOW(PTP_CLK_MAGIC, 19, struct ptp_tsfilter) +#define PTP_FILTERCOUNT_REQUEST _IOR(PTP_CLK_MAGIC, 20, int) +#define PTP_FILTERTS_GET_REQUEST _IOWR(PTP_CLK_MAGIC, 21, struct ptp_tsfilter) struct ptp_extts_event { struct ptp_clock_time t; /* Time event occured. */ diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c index c9f6cca4feb4..e7ff22d60d63 100644 --- a/tools/testing/selftests/ptp/testptp.c +++ b/tools/testing/selftests/ptp/testptp.c @@ -22,6 +22,7 @@ #include <sys/types.h> #include <time.h> #include <unistd.h> +#include <stdbool.h> #include <linux/ptp_clock.h> @@ -117,35 +118,36 @@ static void usage(char *progname) { fprintf(stderr, "usage: %s [options]\n" - " -c query the ptp clock's capabilities\n" - " -d name device to open\n" - " -e val read 'val' external time stamp events\n" - " -f val adjust the ptp clock frequency by 'val' ppb\n" - " -g get the ptp clock time\n" - " -h prints this message\n" - " -i val index for event/trigger\n" - " -k val measure the time offset between system and phc clock\n" - " for 'val' times (Maximum 25)\n" - " -l list the current pin configuration\n" - " -L pin,val configure pin index 'pin' with function 'val'\n" - " the channel index is taken from the '-i' option\n" - " 'val' specifies the auxiliary function:\n" - " 0 - none\n" - " 1 - external time stamp\n" - " 2 - periodic output\n" - " -n val shift the ptp clock time by 'val' nanoseconds\n" - " -o val phase offset (in nanoseconds) to be provided to the PHC servo\n" - " -p val enable output with a period of 'val' nanoseconds\n" - " -H val set output phase to 'val' nanoseconds (requires -p)\n" - " -w val set output pulse width to 'val' nanoseconds (requires -p)\n" - " -P val enable or disable (val=1|0) the system clock PPS\n" - " -s set the ptp clock time from the system time\n" - " -S set the system time from the ptp clock time\n" - " -t val shift the ptp clock time by 'val' seconds\n" - " -T val set the ptp clock time to 'val' seconds\n" - " -x val get an extended ptp clock time with the desired number of samples (up to %d)\n" - " -X get a ptp clock cross timestamp\n" - " -z test combinations of rising/falling external time stamp flags\n", + " -c query the ptp clock's capabilities\n" + " -d name device to open\n" + " -e val read 'val' external time stamp events\n" + " -f val adjust the ptp clock frequency by 'val' ppb\n" + " -F [oid,msk] with no arguments, list the users of the device\n" + " -g get the ptp clock time\n" + " -h prints this message\n" + " -i val index for event/trigger\n" + " -k val measure the time offset between system and phc clock\n" + " for 'val' times (Maximum 25)\n" + " -l list the current pin configuration\n" + " -L pin,val configure pin index 'pin' with function 'val'\n" + " the channel index is taken from the '-i' option\n" + " 'val' specifies the auxiliary function:\n" + " 0 - none\n" + " 1 - external time stamp\n" + " 2 - periodic output\n" + " -n val shift the ptp clock time by 'val' nanoseconds\n" + " -o val phase offset (in nanoseconds) to be provided to the PHC servo\n" + " -p val enable output with a period of 'val' nanoseconds\n" + " -H val set output phase to 'val' nanoseconds (requires -p)\n" + " -w val set output pulse width to 'val' nanoseconds (requires -p)\n" + " -P val enable or disable (val=1|0) the system clock PPS\n" + " -s set the ptp clock time from the system time\n" + " -S set the system time from the ptp clock time\n" + " -t val shift the ptp clock time by 'val' seconds\n" + " -T val set the ptp clock time to 'val' seconds\n" + " -x val get an extended ptp clock time with the desired number of samples (up to %d)\n" + " -X get a ptp clock cross timestamp\n" + " -z test combinations of rising/falling external time stamp flags\n", progname, PTP_MAX_SAMPLES); } @@ -162,6 +164,7 @@ int main(int argc, char *argv[]) struct ptp_sys_offset *sysoff; struct ptp_sys_offset_extended *soe; struct ptp_sys_offset_precise *xts; + struct ptp_tsfilter tsfilter, *tsfilter_read; char *progname; unsigned int i; @@ -187,6 +190,7 @@ int main(int argc, char *argv[]) int pps = -1; int seconds = 0; int settime = 0; + int rvalue = 0; int64_t t1, t2, tp; int64_t interval, offset; @@ -194,9 +198,17 @@ int main(int argc, char *argv[]) int64_t pulsewidth = -1; int64_t perout = -1; + tsfilter_read = NULL; + tsfilter.ndevusers = 0; + tsfilter.reader_oid = 0; + tsfilter.mask = 0xFFFFFFFF; + bool opt_tsfilter = false; + progname = strrchr(argv[0], '/'); progname = progname ? 1+progname : argv[0]; - while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) { + while (EOF != + (c = getopt(argc, argv, + "cd:e:f:F:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) { switch (c) { case 'c': capabilities = 1; @@ -210,6 +222,15 @@ int main(int argc, char *argv[]) case 'f': adjfreq = atoi(optarg); break; + case 'F': + opt_tsfilter = true; + cnt = sscanf(optarg, "%d,%X", &tsfilter.reader_oid, + &tsfilter.mask); + if (cnt != 2 && cnt != 0) { + usage(progname); + return -1; + } + break; case 'g': gettime = 1; break; @@ -295,7 +316,8 @@ int main(int argc, char *argv[]) clkid = get_clockid(fd); if (CLOCK_INVALID == clkid) { fprintf(stderr, "failed to read clock id\n"); - return -1; + rvalue = -1; + goto exit; } if (capabilities) { @@ -464,18 +486,21 @@ int main(int argc, char *argv[]) if (pulsewidth >= 0 && perout < 0) { puts("-w can only be specified together with -p"); - return -1; + rvalue = -1; + goto exit; } if (perout_phase >= 0 && perout < 0) { puts("-H can only be specified together with -p"); - return -1; + rvalue = -1; + goto exit; } if (perout >= 0) { if (clock_gettime(clkid, &ts)) { perror("clock_gettime"); - return -1; + rvalue = -1; + goto exit; } memset(&perout_request, 0, sizeof(perout_request)); perout_request.index = index; @@ -516,13 +541,15 @@ int main(int argc, char *argv[]) if (n_samples <= 0 || n_samples > 25) { puts("n_samples should be between 1 and 25"); usage(progname); - return -1; + rvalue = -1; + goto exit; } sysoff = calloc(1, sizeof(*sysoff)); if (!sysoff) { perror("calloc"); - return -1; + rvalue = -1; + goto exit; } sysoff->n_samples = n_samples; @@ -604,6 +631,63 @@ int main(int argc, char *argv[]) free(xts); } + if (opt_tsfilter) { + if (tsfilter.reader_oid) { + /* Set a filter for a specific object id */ + if (ioctl(fd, PTP_FILTERTS_SET_REQUEST, &tsfilter)) { + perror("PTP_FILTERTS_SET_REQUEST"); + rvalue = -1; + goto exit; + } + printf("Timestamp event queue mask 0x%X applied to reader with oid: %d\n", + (int)tsfilter.mask, tsfilter.reader_oid); + + } else { + /* List all filters */ + if (ioctl(fd, PTP_FILTERCOUNT_REQUEST, + &tsfilter.ndevusers)) { + perror("PTP_FILTERTS_SET_REQUEST"); + rvalue = -1; + goto exit; + } + tsfilter_read = calloc(tsfilter.ndevusers, + sizeof(*tsfilter_read)); + /* + * Get a variable length result from the IOCTL. We use a value + * inside the structure we are willing to read to communicate the + * IOCTL how many elements we are expecting to get. + * It's ok if the size of the list changed between these two operations, + * this is just an approximation to be able to test the concept. + */ + tsfilter_read[0].ndevusers = tsfilter.ndevusers; + if (!tsfilter_read) { + perror("tsfilter_read calloc"); + rvalue = -1; + goto exit; + } + if (ioctl(fd, PTP_FILTERTS_GET_REQUEST, + tsfilter_read)) { + perror("PTP_FILTERTS_GET_REQUEST"); + rvalue = -1; + goto exit; + } + printf("(USER PID)\tTSEVQ FILTER ID:MASK\n"); + for (i = 0; i < tsfilter.ndevusers; i++) { + if (tsfilter_read[i].reader_oid) + printf("(%d)\t\t%5d:0x%08X\n", + tsfilter_read[i].reader_rpid, + tsfilter_read[i].reader_oid, + tsfilter_read[i].mask); + } + } + } + +exit: + if (tsfilter_read) { + free(tsfilter_read); + tsfilter_read = NULL; + } + close(fd); - return 0; + return rvalue; }
Implement ioctl to support filtering of external timestamp event queue channels per reader based on the process PID accessing the timestamp queue. Can be tested using testptp test binary. Use lsof to figure out readers of the DUT. LSB of the timestamp channel mask is channel 0. eg: To view all current users of the device: ``` # testptp -F /dev/ptp0 (USER PID) TSEVQ FILTER ID:MASK (3234) 1:0x00000001 (3692) 2:0xFFFFFFFF (3792) 3:0xFFFFFFFF (8713) 4:0xFFFFFFFF ``` eg: To allow ID 1 to access only ts channel 0: ``` # testptp -F 1,0x1 ``` eg: To allow ID 1 to access any channel: ``` # testptp -F 1,0xFFFFFFFF ``` Signed-off-by: Xabier Marquiegui <reibax@gmail.com> Suggested-by: Richard Cochran <richardcochran@gmail.com> --- v3: - filter application by object id, aided by process id - friendlier testptp implementation of event queue channel filters v2: https://lore.kernel.org/netdev/20230912220217.2008895-3-reibax@gmail.com/ - fix testptp compilation error: unknown type name 'pid_t' - rename mask variable for easier code traceability - more detailed commit message with two examples v1: https://lore.kernel.org/netdev/20230906104754.1324412-4-reibax@gmail.com/ drivers/ptp/ptp_chardev.c | 85 +++++++++++++- drivers/ptp/ptp_clock.c | 4 +- drivers/ptp/ptp_private.h | 1 + include/uapi/linux/ptp_clock.h | 12 ++ tools/testing/selftests/ptp/testptp.c | 158 ++++++++++++++++++++------ 5 files changed, 221 insertions(+), 39 deletions(-)