[PATCH net-next v3 0/3] ptp: Support for multiple filtered timestamp event queue readers

[PATCH net-next v3 0/3] ptp: Support for multiple filtered timestamp event queue readers

[Xabier Marquiegui]

On systems with multiple timestamp event channels, there can be scenarios where
multiple userspace readers want to access the timestamping data for various
purposes.

One such example is wanting to use a pps out for time synchronization, and
wanting to timestamp external events with the synchronized time base 
simultaneously.

Timestmp event consumers on the other hand, are often interested in a subset of
the available timestamp channels. linuxptp ts2phc, for example, is not happy if
more than one timestamping channel is active on the device it is reading from.

This patch-set introduces linked lists to support multiple timestamp event queue
consumers, and timestamp event channel filters through IOCTLs.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
Suggested-by: Richard Cochran <richardcochran@gmail.com>
---
v3:
  - add this patchset overview file
  - fix use of safe and non safe linked lists for loops
  - introduce new posix_clock private_data and ida object ids for better
    dicrimination of timestamp consumers
  - safer resource release procedures
  - 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-1-reibax@gmail.com/
  - fix ptp_poll() return value
  - Style changes to comform to checkpatch strict suggestions
  - more coherent ptp_read error exit routines
  - 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-2-reibax@gmail.com/

[PATCH net-next v3 1/3] ptp: Replace timestamp event queue with linked list

[Xabier Marquiegui]

Introduce linked lists to access the timestamp event queue.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
Suggested-by: Richard Cochran <richardcochran@gmail.com>
---
v3:
  - fix use of safe and non safe linked lists for loops
v2: https://lore.kernel.org/netdev/20230912220217.2008895-1-reibax@gmail.com/
  - Style changes to comform to checkpatch strict suggestions
v1: https://lore.kernel.org/netdev/20230906104754.1324412-2-reibax@gmail.com/

 drivers/ptp/ptp_chardev.c | 16 ++++++++++++++--
 drivers/ptp/ptp_clock.c   | 30 ++++++++++++++++++++++++++++--
 drivers/ptp/ptp_private.h |  4 +++-
 drivers/ptp/ptp_sysfs.c   |  6 +++++-
 4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 362bf756e6b7..197edf1179f1 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -435,10 +435,16 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 __poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct timestamp_event_queue *queue;
 
 	poll_wait(fp, &ptp->tsev_wq, wait);
 
-	return queue_cnt(&ptp->tsevq) ? EPOLLIN : 0;
+	/* Extract only the first element in the queue list
+	 * TODO: Identify the relevant queue
+	 */
+	queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
+
+	return queue_cnt(queue) ? EPOLLIN : 0;
 }
 
 #define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
@@ -447,12 +453,18 @@ ssize_t ptp_read(struct posix_clock *pc,
 		 uint rdflags, char __user *buf, size_t cnt)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
-	struct timestamp_event_queue *queue = &ptp->tsevq;
+	struct timestamp_event_queue *queue;
 	struct ptp_extts_event *event;
 	unsigned long flags;
 	size_t qcnt, i;
 	int result;
 
+	/* Extract only the first element in the queue list
+	 * TODO: Identify the relevant queue
+	 */
+	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
+				 qlist);
+
 	if (cnt % sizeof(struct ptp_extts_event) != 0)
 		return -EINVAL;
 
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 80f74e38c2da..8996ff500392 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -166,6 +166,18 @@ static struct posix_clock_operations ptp_clock_ops = {
 	.read		= ptp_read,
 };
 
+static void ptp_clean_queue_list(struct ptp_clock *ptp)
+{
+	struct timestamp_event_queue *element;
+	struct list_head *pos, *next;
+
+	list_for_each_safe(pos, next, &ptp->tsevqs) {
+		element = list_entry(pos, struct timestamp_event_queue, qlist);
+		list_del(pos);
+		kfree(element);
+	}
+}
+
 static void ptp_clock_release(struct device *dev)
 {
 	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
@@ -175,6 +187,7 @@ static void ptp_clock_release(struct device *dev)
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
+	ptp_clean_queue_list(ptp);
 	ida_free(&ptp_clocks_map, ptp->index);
 	kfree(ptp);
 }
@@ -206,6 +219,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 				     struct device *parent)
 {
 	struct ptp_clock *ptp;
+	struct timestamp_event_queue *queue = NULL;
 	int err = 0, index, major = MAJOR(ptp_devt);
 	size_t size;
 
@@ -228,7 +242,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	ptp->info = info;
 	ptp->devid = MKDEV(major, index);
 	ptp->index = index;
-	spin_lock_init(&ptp->tsevq.lock);
+	INIT_LIST_HEAD(&ptp->tsevqs);
+	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
+	if (!queue)
+		goto no_memory_queue;
+	spin_lock_init(&queue->lock);
+	list_add_tail(&queue->qlist, &ptp->tsevqs);
+	/* TODO - Transform or delete this mutex */
 	mutex_init(&ptp->tsevq_mux);
 	mutex_init(&ptp->pincfg_mux);
 	mutex_init(&ptp->n_vclocks_mux);
@@ -333,6 +353,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
+	ptp_clean_queue_list(ptp);
+no_memory_queue:
 	ida_free(&ptp_clocks_map, index);
 no_slot:
 	kfree(ptp);
@@ -375,6 +397,7 @@ EXPORT_SYMBOL(ptp_clock_unregister);
 
 void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
 {
+	struct timestamp_event_queue *tsevq;
 	struct pps_event_time evt;
 
 	switch (event->type) {
@@ -383,7 +406,10 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
 		break;
 
 	case PTP_CLOCK_EXTTS:
-		enqueue_external_timestamp(&ptp->tsevq, event);
+		/* Enqueue timestamp on all other queues */
+		list_for_each_entry(tsevq, &ptp->tsevqs, qlist) {
+			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 75f58fc468a7..314c21c39f6a 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -15,6 +15,7 @@
 #include <linux/ptp_clock.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/time.h>
+#include <linux/list.h>
 
 #define PTP_MAX_TIMESTAMPS 128
 #define PTP_BUF_TIMESTAMPS 30
@@ -25,6 +26,7 @@ struct timestamp_event_queue {
 	int head;
 	int tail;
 	spinlock_t lock;
+	struct list_head qlist;
 };
 
 struct ptp_clock {
@@ -35,7 +37,7 @@ struct ptp_clock {
 	int index; /* index into clocks.map */
 	struct pps_device *pps_source;
 	long dialed_frequency; /* remembers the frequency adjustment */
-	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
+	struct list_head tsevqs; /* timestamp fifo list */
 	struct mutex tsevq_mux; /* one process at a time reading the fifo */
 	struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
 	wait_queue_head_t tsev_wq;
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 6e4d5456a885..2675f383cd0a 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -75,12 +75,16 @@ static ssize_t extts_fifo_show(struct device *dev,
 			       struct device_attribute *attr, char *page)
 {
 	struct ptp_clock *ptp = dev_get_drvdata(dev);
-	struct timestamp_event_queue *queue = &ptp->tsevq;
+	struct timestamp_event_queue *queue;
 	struct ptp_extts_event event;
 	unsigned long flags;
 	size_t qcnt;
 	int cnt = 0;
 
+	/* The sysfs fifo will always draw from the fist queue */
+	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
+				 qlist);
+
 	memset(&event, 0, sizeof(event));
 
 	if (mutex_lock_interruptible(&ptp->tsevq_mux))
-- 
2.34.1

Re: [PATCH net-next v3 1/3] ptp: Replace timestamp event queue with linked list

[Richard Cochran]

On Thu, Sep 28, 2023 at 03:35:42PM +0200, Xabier Marquiegui wrote:

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 362bf756e6b7..197edf1179f1 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -435,10 +435,16 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  __poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
>  {
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> +	struct timestamp_event_queue *queue;
>  
>  	poll_wait(fp, &ptp->tsev_wq, wait);
>  
> -	return queue_cnt(&ptp->tsevq) ? EPOLLIN : 0;
> +	/* Extract only the first element in the queue list
> +	 * TODO: Identify the relevant queue

Don't need todos, we see what is happening.

> @@ -228,7 +242,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	ptp->info = info;
>  	ptp->devid = MKDEV(major, index);
>  	ptp->index = index;
> -	spin_lock_init(&ptp->tsevq.lock);
> +	INIT_LIST_HEAD(&ptp->tsevqs);
> +	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> +	if (!queue)
> +		goto no_memory_queue;
> +	spin_lock_init(&queue->lock);
> +	list_add_tail(&queue->qlist, &ptp->tsevqs);
> +	/* TODO - Transform or delete this mutex */

Again, no todos please, the patch that removes this mutex will/must
clearly justify the change.

>  	mutex_init(&ptp->tsevq_mux);
>  	mutex_init(&ptp->pincfg_mux);
>  	mutex_init(&ptp->n_vclocks_mux);

Thanks,
Richard

[PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Xabier Marquiegui]

Use linked lists to create one event queue per open file. This enables
simultaneous readers for timestamp event queues.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
Suggested-by: Richard Cochran <richardcochran@gmail.com>
---
v3:
  - fix use of safe and non safe linked lists for loops
  - introduce new posix_clock private_data and ida object ids for better
    dicrimination of timestamp consumers
  - safer resource release procedures
v2: https://lore.kernel.org/netdev/20230912220217.2008895-2-reibax@gmail.com/
  - fix ptp_poll() return value
  - Style changes to comform to checkpatch strict suggestions
  - more coherent ptp_read error exit routines
v1: https://lore.kernel.org/netdev/20230906104754.1324412-3-reibax@gmail.com/

 drivers/ptp/ptp_chardev.c   | 134 ++++++++++++++++++++++++++++--------
 drivers/ptp/ptp_clock.c     |  61 +++++++++++++---
 drivers/ptp/ptp_private.h   |  38 +++++++---
 drivers/ptp/ptp_sysfs.c     |  10 ++-
 include/linux/posix-clock.h |  24 ++++---
 kernel/time/posix-clock.c   |  43 +++++++++---
 6 files changed, 239 insertions(+), 71 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 197edf1179f1..65e7acaa40a9 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -101,14 +101,74 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 	return 0;
 }
 
-int ptp_open(struct posix_clock *pc, fmode_t fmode)
+int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode)
 {
+	struct ptp_clock *ptp =
+		container_of(pcuser->clk, struct ptp_clock, clock);
+	struct ida *ida = ptp_get_tsevq_ida(ptp);
+	struct timestamp_event_queue *queue;
+
+	if (!ida)
+		return -EINVAL;
+	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
+	if (!queue)
+		return -EINVAL;
+	queue->close_req = false;
+	queue->reader_pid = task_pid_nr(current);
+	spin_lock_init(&queue->lock);
+	queue->ida = ida;
+	queue->oid = ida_alloc(ida, GFP_KERNEL);
+	if (queue->oid < 0) {
+		kfree(queue);
+		return queue->oid;
+	}
+	list_add_tail(&queue->qlist, &ptp->tsevqs);
+	pcuser->private_clkdata = queue;
+
 	return 0;
 }
 
-long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
+int ptp_release(struct posix_clock_user *pcuser)
+{
+	struct ptp_clock *ptp =
+		container_of(pcuser->clk, struct ptp_clock, clock);
+	struct timestamp_event_queue *queue = pcuser->private_clkdata;
+
+	if (queue) {
+		queue->close_req = true;
+		list_move_tail(&queue->qlist, &ptp->closed_tsevqs);
+		pcuser->private_clkdata = NULL;
+	}
+	return 0;
+}
+
+void ptp_flush_users(struct posix_clock *pc)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct timestamp_event_queue *tsevq, *tsevq_nxt;
+	unsigned long flags;
+
+	if (mutex_lock_interruptible(&ptp->close_mux))
+		return;
+
+	list_for_each_entry_safe(tsevq, tsevq_nxt, &ptp->closed_tsevqs, qlist) {
+		spin_lock_irqsave(&tsevq->lock, flags);
+		if (tsevq->ida)
+			ida_free(tsevq->ida, (unsigned int)tsevq->oid);
+		tsevq->ida = NULL;
+		list_del(&tsevq->qlist);
+		spin_unlock_irqrestore(&tsevq->lock, flags);
+		kfree(tsevq);
+	}
+
+	mutex_unlock(&ptp->close_mux);
+}
+
+long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
+	       unsigned long arg)
+{
+	struct ptp_clock *ptp =
+		container_of(pcuser->clk, struct ptp_clock, clock);
 	struct ptp_sys_offset_extended *extoff = NULL;
 	struct ptp_sys_offset_precise precise_offset;
 	struct system_device_crosststamp xtstamp;
@@ -432,65 +492,75 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	return err;
 }
 
-__poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
+__poll_t ptp_poll(struct posix_clock_user *pcuser, struct file *fp,
+		  poll_table *wait)
 {
-	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct ptp_clock *ptp =
+		container_of(pcuser->clk, struct ptp_clock, clock);
 	struct timestamp_event_queue *queue;
 
-	poll_wait(fp, &ptp->tsev_wq, wait);
+	queue = pcuser->private_clkdata;
+	if (!queue)
+		return EPOLLERR;
 
-	/* Extract only the first element in the queue list
-	 * TODO: Identify the relevant queue
-	 */
-	queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
+	poll_wait(fp, &ptp->tsev_wq, wait);
 
 	return queue_cnt(queue) ? EPOLLIN : 0;
 }
 
 #define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
 
-ssize_t ptp_read(struct posix_clock *pc,
-		 uint rdflags, char __user *buf, size_t cnt)
+ssize_t ptp_read(struct posix_clock_user *pcuser, uint rdflags,
+		 char __user *buf, size_t cnt)
 {
-	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct ptp_clock *ptp =
+		container_of(pcuser->clk, struct ptp_clock, clock);
 	struct timestamp_event_queue *queue;
 	struct ptp_extts_event *event;
 	unsigned long flags;
 	size_t qcnt, i;
 	int result;
 
-	/* Extract only the first element in the queue list
-	 * TODO: Identify the relevant queue
-	 */
-	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
-				 qlist);
+	queue = pcuser->private_clkdata;
+	if (!queue) {
+		result = -EINVAL;
+		goto exit;
+	}
 
-	if (cnt % sizeof(struct ptp_extts_event) != 0)
-		return -EINVAL;
+	if (queue->close_req) {
+		result = -EPIPE;
+		goto exit;
+	}
+
+	if (!queue->ida) {
+		result = -ENODEV;
+		goto exit;
+	}
+
+	if (cnt % sizeof(struct ptp_extts_event) != 0) {
+		result = -EINVAL;
+		goto exit;
+	}
 
 	if (cnt > EXTTS_BUFSIZE)
 		cnt = EXTTS_BUFSIZE;
 
 	cnt = cnt / sizeof(struct ptp_extts_event);
 
-	if (mutex_lock_interruptible(&ptp->tsevq_mux))
-		return -ERESTARTSYS;
-
 	if (wait_event_interruptible(ptp->tsev_wq,
 				     ptp->defunct || queue_cnt(queue))) {
-		mutex_unlock(&ptp->tsevq_mux);
 		return -ERESTARTSYS;
 	}
 
 	if (ptp->defunct) {
-		mutex_unlock(&ptp->tsevq_mux);
-		return -ENODEV;
+		result = -ENODEV;
+		goto exit;
 	}
 
 	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
 	if (!event) {
-		mutex_unlock(&ptp->tsevq_mux);
-		return -ENOMEM;
+		result = -ENOMEM;
+		goto exit;
 	}
 
 	spin_lock_irqsave(&queue->lock, flags);
@@ -509,12 +579,16 @@ ssize_t ptp_read(struct posix_clock *pc,
 
 	cnt = cnt * sizeof(struct ptp_extts_event);
 
-	mutex_unlock(&ptp->tsevq_mux);
-
 	result = cnt;
-	if (copy_to_user(buf, event, cnt))
+	if (copy_to_user(buf, event, cnt)) {
 		result = -EFAULT;
+		goto free_event;
+	}
 
+free_event:
 	kfree(event);
+exit:
+	if (result < 0)
+		ptp_release(pcuser);
 	return result;
 }
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 8996ff500392..9e271ad66933 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <linux/delay.h>
 #include <uapi/linux/sched/types.h>
 
 #include "ptp_private.h"
@@ -162,20 +163,51 @@ static struct posix_clock_operations ptp_clock_ops = {
 	.clock_settime	= ptp_clock_settime,
 	.ioctl		= ptp_ioctl,
 	.open		= ptp_open,
+	.release	= ptp_release,
+	.flush_users	= ptp_flush_users,
 	.poll		= ptp_poll,
 	.read		= ptp_read,
 };
 
 static void ptp_clean_queue_list(struct ptp_clock *ptp)
 {
-	struct timestamp_event_queue *element;
-	struct list_head *pos, *next;
+	struct timestamp_event_queue *tsevq;
+	struct ptp_clock_event event;
+	unsigned long flags;
+	int cnt;
+
+	memset(&event, 0, sizeof(event));
+	/* Request close of all open files */
+	list_for_each_entry(tsevq, &ptp->tsevqs, qlist) {
+		tsevq->close_req = true;
+		/* Make sure the queue is fed so that the reader is notified */
+		enqueue_external_timestamp(tsevq, &event);
+	}
+	wake_up_interruptible(&ptp->tsev_wq);
 
-	list_for_each_safe(pos, next, &ptp->tsevqs) {
-		element = list_entry(pos, struct timestamp_event_queue, qlist);
-		list_del(pos);
-		kfree(element);
+	/* Wait for all to close */
+	cnt = list_count_nodes(&ptp->tsevqs);
+	while (cnt > 1) {
+		msleep(20);
+		cnt = list_count_nodes(&ptp->tsevqs);
 	}
+	cnt = list_count_nodes(&ptp->closed_tsevqs);
+	while (cnt > 1) {
+		msleep(20);
+		cnt = list_count_nodes(&ptp->closed_tsevqs);
+	}
+
+	/* Delete first entry */
+	tsevq = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
+				 qlist);
+
+	spin_lock_irqsave(&tsevq->lock, flags);
+	ida_destroy(tsevq->ida);
+	kfree(tsevq->ida);
+	tsevq->ida = NULL;
+	list_del(&tsevq->qlist);
+	spin_unlock_irqrestore(&tsevq->lock, flags);
+	kfree(tsevq);
 }
 
 static void ptp_clock_release(struct device *dev)
@@ -184,11 +216,11 @@ static void ptp_clock_release(struct device *dev)
 
 	ptp_cleanup_pin_groups(ptp);
 	kfree(ptp->vclock_index);
-	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
 	ptp_clean_queue_list(ptp);
 	ida_free(&ptp_clocks_map, ptp->index);
+	mutex_destroy(&ptp->close_mux);
 	kfree(ptp);
 }
 
@@ -243,15 +275,23 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	ptp->devid = MKDEV(major, index);
 	ptp->index = index;
 	INIT_LIST_HEAD(&ptp->tsevqs);
+	INIT_LIST_HEAD(&ptp->closed_tsevqs);
 	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
 	if (!queue)
 		goto no_memory_queue;
+	queue->close_req = false;
+	queue->ida = kzalloc(sizeof(*queue->ida), GFP_KERNEL);
+	if (!queue->ida)
+		goto no_memory_queue;
+	ida_init(queue->ida);
 	spin_lock_init(&queue->lock);
 	list_add_tail(&queue->qlist, &ptp->tsevqs);
-	/* TODO - Transform or delete this mutex */
-	mutex_init(&ptp->tsevq_mux);
+	queue->oid = ida_alloc(queue->ida, GFP_KERNEL);
+	if (queue->oid < 0)
+		goto ida_err;
 	mutex_init(&ptp->pincfg_mux);
 	mutex_init(&ptp->n_vclocks_mux);
+	mutex_init(&ptp->close_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
 	if (ptp->info->getcycles64 || ptp->info->getcyclesx64) {
@@ -350,9 +390,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (ptp->kworker)
 		kthread_destroy_worker(ptp->kworker);
 kworker_err:
-	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
+	mutex_destroy(&ptp->close_mux);
+ida_err:
 	ptp_clean_queue_list(ptp);
 no_memory_queue:
 	ida_free(&ptp_clocks_map, index);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 314c21c39f6a..529d3d421ba0 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -16,6 +16,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/time.h>
 #include <linux/list.h>
+#include <linux/idr.h>
 
 #define PTP_MAX_TIMESTAMPS 128
 #define PTP_BUF_TIMESTAMPS 30
@@ -26,7 +27,12 @@ struct timestamp_event_queue {
 	int head;
 	int tail;
 	spinlock_t lock;
+	struct posix_clock_user *pcreader;
 	struct list_head qlist;
+	pid_t reader_pid;
+	struct ida *ida;
+	int oid;
+	bool close_req;
 };
 
 struct ptp_clock {
@@ -38,7 +44,8 @@ struct ptp_clock {
 	struct pps_device *pps_source;
 	long dialed_frequency; /* remembers the frequency adjustment */
 	struct list_head tsevqs; /* timestamp fifo list */
-	struct mutex tsevq_mux; /* one process at a time reading the fifo */
+	struct list_head closed_tsevqs; /* close pending timestamp fifo lists */
+	struct mutex close_mux; /* protect user clean procecdures */
 	struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
 	wait_queue_head_t tsev_wq;
 	int defunct; /* tells readers to go away when clock is being removed */
@@ -109,6 +116,17 @@ static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
 	return ptp_vclock_in_use(ptp);
 }
 
+/* Find the tsevq object id handler */
+static inline struct ida *ptp_get_tsevq_ida(struct ptp_clock *ptp)
+{
+	struct timestamp_event_queue *queue;
+
+	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
+				 qlist);
+
+	return queue->ida;
+}
+
 extern struct class *ptp_class;
 
 /*
@@ -119,16 +137,20 @@ extern struct class *ptp_class;
 int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		    enum ptp_pin_function func, unsigned int chan);
 
-long ptp_ioctl(struct posix_clock *pc,
-	       unsigned int cmd, unsigned long arg);
+long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
+	       unsigned long arg);
+
+int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode);
+
+int ptp_release(struct posix_clock_user *pcuser);
 
-int ptp_open(struct posix_clock *pc, fmode_t fmode);
+void ptp_flush_users(struct posix_clock *pc);
 
-ssize_t ptp_read(struct posix_clock *pc,
-		 uint flags, char __user *buf, size_t cnt);
+ssize_t ptp_read(struct posix_clock_user *pcuser, uint flags, char __user *buf,
+		 size_t cnt);
 
-__poll_t ptp_poll(struct posix_clock *pc,
-	      struct file *fp, poll_table *wait);
+__poll_t ptp_poll(struct posix_clock_user *pcuser, struct file *fp,
+		  poll_table *wait);
 
 /*
  * see ptp_sysfs.c
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 2675f383cd0a..c02aaba729e0 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -81,15 +81,14 @@ static ssize_t extts_fifo_show(struct device *dev,
 	size_t qcnt;
 	int cnt = 0;
 
+	cnt = list_count_nodes(&ptp->tsevqs);
+	if (cnt <= 0)
+		goto out;
+
 	/* The sysfs fifo will always draw from the fist queue */
 	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
 				 qlist);
 
-	memset(&event, 0, sizeof(event));
-
-	if (mutex_lock_interruptible(&ptp->tsevq_mux))
-		return -ERESTARTSYS;
-
 	spin_lock_irqsave(&queue->lock, flags);
 	qcnt = queue_cnt(queue);
 	if (qcnt) {
@@ -104,7 +103,6 @@ static ssize_t extts_fifo_show(struct device *dev,
 	cnt = snprintf(page, PAGE_SIZE, "%u %lld %u\n",
 		       event.index, event.t.sec, event.t.nsec);
 out:
-	mutex_unlock(&ptp->tsevq_mux);
 	return cnt;
 }
 static DEVICE_ATTR(fifo, 0444, extts_fifo_show, NULL);
diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index 468328b1e1dd..8f844ac28aa8 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -14,6 +14,7 @@
 #include <linux/rwsem.h>
 
 struct posix_clock;
+struct posix_clock_user;
 
 /**
  * struct posix_clock_operations - functional interface to the clock
@@ -50,18 +51,20 @@ struct posix_clock_operations {
 	/*
 	 * Optional character device methods:
 	 */
-	long    (*ioctl)   (struct posix_clock *pc,
-			    unsigned int cmd, unsigned long arg);
+	long (*ioctl)(struct posix_clock_user *pcuser, unsigned int cmd,
+		      unsigned long arg);
 
-	int     (*open)    (struct posix_clock *pc, fmode_t f_mode);
+	int (*open)(struct posix_clock_user *pcuser, fmode_t f_mode);
 
-	__poll_t (*poll)   (struct posix_clock *pc,
-			    struct file *file, poll_table *wait);
+	__poll_t (*poll)(struct posix_clock_user *pcuser, struct file *file,
+			 poll_table *wait);
 
-	int     (*release) (struct posix_clock *pc);
+	int (*release)(struct posix_clock_user *pcuser);
 
-	ssize_t (*read)    (struct posix_clock *pc,
-			    uint flags, char __user *buf, size_t cnt);
+	void (*flush_users)(struct posix_clock *pc);
+
+	ssize_t (*read)(struct posix_clock_user *pcuser, uint flags,
+			char __user *buf, size_t cnt);
 };
 
 /**
@@ -90,6 +93,11 @@ struct posix_clock {
 	bool zombie;
 };
 
+struct posix_clock_user {
+	struct posix_clock *clk;
+	void *private_clkdata;
+};
+
 /**
  * posix_clock_register() - register a new clock
  * @clk:   Pointer to the clock. Caller must provide 'ops' field
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 77c0c2370b6d..8ae76492f7ea 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -19,7 +19,12 @@
  */
 static struct posix_clock *get_posix_clock(struct file *fp)
 {
-	struct posix_clock *clk = fp->private_data;
+	struct posix_clock_user *pcuser = fp->private_data;
+	struct posix_clock *clk;
+
+	if (!pcuser)
+		return NULL;
+	clk = pcuser->clk;
 
 	down_read(&clk->rwsem);
 
@@ -39,6 +44,7 @@ static void put_posix_clock(struct posix_clock *clk)
 static ssize_t posix_clock_read(struct file *fp, char __user *buf,
 				size_t count, loff_t *ppos)
 {
+	struct posix_clock_user *pcuser = fp->private_data;
 	struct posix_clock *clk = get_posix_clock(fp);
 	int err = -EINVAL;
 
@@ -46,7 +52,7 @@ static ssize_t posix_clock_read(struct file *fp, char __user *buf,
 		return -ENODEV;
 
 	if (clk->ops.read)
-		err = clk->ops.read(clk, fp->f_flags, buf, count);
+		err = clk->ops.read(pcuser, fp->f_flags, buf, count);
 
 	put_posix_clock(clk);
 
@@ -55,6 +61,7 @@ static ssize_t posix_clock_read(struct file *fp, char __user *buf,
 
 static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
 {
+	struct posix_clock_user *pcuser = fp->private_data;
 	struct posix_clock *clk = get_posix_clock(fp);
 	__poll_t result = 0;
 
@@ -62,7 +69,7 @@ static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
 		return EPOLLERR;
 
 	if (clk->ops.poll)
-		result = clk->ops.poll(clk, fp, wait);
+		result = clk->ops.poll(pcuser, fp, wait);
 
 	put_posix_clock(clk);
 
@@ -72,6 +79,7 @@ static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
 static long posix_clock_ioctl(struct file *fp,
 			      unsigned int cmd, unsigned long arg)
 {
+	struct posix_clock_user *pcuser = fp->private_data;
 	struct posix_clock *clk = get_posix_clock(fp);
 	int err = -ENOTTY;
 
@@ -79,7 +87,7 @@ static long posix_clock_ioctl(struct file *fp,
 		return -ENODEV;
 
 	if (clk->ops.ioctl)
-		err = clk->ops.ioctl(clk, cmd, arg);
+		err = clk->ops.ioctl(pcuser, cmd, arg);
 
 	put_posix_clock(clk);
 
@@ -90,6 +98,7 @@ static long posix_clock_ioctl(struct file *fp,
 static long posix_clock_compat_ioctl(struct file *fp,
 				     unsigned int cmd, unsigned long arg)
 {
+	struct posix_clock_user *pcuser = fp->private_data;
 	struct posix_clock *clk = get_posix_clock(fp);
 	int err = -ENOTTY;
 
@@ -97,7 +106,7 @@ static long posix_clock_compat_ioctl(struct file *fp,
 		return -ENODEV;
 
 	if (clk->ops.ioctl)
-		err = clk->ops.ioctl(clk, cmd, arg);
+		err = clk->ops.ioctl(pcuser, cmd, arg);
 
 	put_posix_clock(clk);
 
@@ -110,6 +119,7 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
 	int err;
 	struct posix_clock *clk =
 		container_of(inode->i_cdev, struct posix_clock, cdev);
+	struct posix_clock_user *pcuser;
 
 	down_read(&clk->rwsem);
 
@@ -117,14 +127,20 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
 		err = -ENODEV;
 		goto out;
 	}
+	pcuser = kzalloc(sizeof(*pcuser), GFP_KERNEL);
+	if (!pcuser) {
+		err = -ENOMEM;
+		goto out;
+	}
+	pcuser->clk = clk;
+	fp->private_data = pcuser;
 	if (clk->ops.open)
-		err = clk->ops.open(clk, fp->f_mode);
+		err = clk->ops.open(pcuser, fp->f_mode);
 	else
 		err = 0;
 
 	if (!err) {
 		get_device(clk->dev);
-		fp->private_data = clk;
 	}
 out:
 	up_read(&clk->rwsem);
@@ -133,14 +149,23 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
 
 static int posix_clock_release(struct inode *inode, struct file *fp)
 {
-	struct posix_clock *clk = fp->private_data;
+	struct posix_clock_user *pcuser = fp->private_data;
+	struct posix_clock *clk;
 	int err = 0;
 
+	if (!pcuser)
+		return -ENODEV;
+	clk = pcuser->clk;
+
 	if (clk->ops.release)
-		err = clk->ops.release(clk);
+		err = clk->ops.release(pcuser);
 
 	put_device(clk->dev);
 
+	if (clk->ops.flush_users)
+		clk->ops.flush_users(clk);
+
+	kfree(pcuser);
 	fp->private_data = NULL;
 
 	return err;
-- 
2.34.1

Re: [PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Vinicius Costa Gomes]

Xabier Marquiegui <reibax@gmail.com> writes:

> Use linked lists to create one event queue per open file. This enables
> simultaneous readers for timestamp event queues.
>
> Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
> Suggested-by: Richard Cochran <richardcochran@gmail.com>
> ---
> v3:
>   - fix use of safe and non safe linked lists for loops
>   - introduce new posix_clock private_data and ida object ids for better
>     dicrimination of timestamp consumers
>   - safer resource release procedures
> v2: https://lore.kernel.org/netdev/20230912220217.2008895-2-reibax@gmail.com/
>   - fix ptp_poll() return value
>   - Style changes to comform to checkpatch strict suggestions
>   - more coherent ptp_read error exit routines
> v1: https://lore.kernel.org/netdev/20230906104754.1324412-3-reibax@gmail.com/
>
>  drivers/ptp/ptp_chardev.c   | 134 ++++++++++++++++++++++++++++--------
>  drivers/ptp/ptp_clock.c     |  61 +++++++++++++---
>  drivers/ptp/ptp_private.h   |  38 +++++++---
>  drivers/ptp/ptp_sysfs.c     |  10 ++-
>  include/linux/posix-clock.h |  24 ++++---
>  kernel/time/posix-clock.c   |  43 +++++++++---
>  6 files changed, 239 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 197edf1179f1..65e7acaa40a9 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -101,14 +101,74 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
>  	return 0;
>  }
>  
> -int ptp_open(struct posix_clock *pc, fmode_t fmode)
> +int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode)
>  {
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
> +	struct ida *ida = ptp_get_tsevq_ida(ptp);
> +	struct timestamp_event_queue *queue;
> +
> +	if (!ida)
> +		return -EINVAL;
> +	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> +	if (!queue)
> +		return -EINVAL;
> +	queue->close_req = false;
> +	queue->reader_pid = task_pid_nr(current);
> +	spin_lock_init(&queue->lock);
> +	queue->ida = ida;
> +	queue->oid = ida_alloc(ida, GFP_KERNEL);
> +	if (queue->oid < 0) {
> +		kfree(queue);
> +		return queue->oid;

This looks wrong, accessing queue after free'ing it.

> +	}
> +	list_add_tail(&queue->qlist, &ptp->tsevqs);
> +	pcuser->private_clkdata = queue;
> +
>  	return 0;
>  }
>  
> -long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> +int ptp_release(struct posix_clock_user *pcuser)
> +{
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
> +	struct timestamp_event_queue *queue = pcuser->private_clkdata;
> +
> +	if (queue) {
> +		queue->close_req = true;
> +		list_move_tail(&queue->qlist, &ptp->closed_tsevqs);
> +		pcuser->private_clkdata = NULL;
> +	}
> +	return 0;
> +}
> +
> +void ptp_flush_users(struct posix_clock *pc)
>  {
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> +	struct timestamp_event_queue *tsevq, *tsevq_nxt;
> +	unsigned long flags;
> +
> +	if (mutex_lock_interruptible(&ptp->close_mux))
> +		return;
> +
> +	list_for_each_entry_safe(tsevq, tsevq_nxt, &ptp->closed_tsevqs, qlist) {
> +		spin_lock_irqsave(&tsevq->lock, flags);
> +		if (tsevq->ida)
> +			ida_free(tsevq->ida, (unsigned int)tsevq->oid);
> +		tsevq->ida = NULL;
> +		list_del(&tsevq->qlist);
> +		spin_unlock_irqrestore(&tsevq->lock, flags);
> +		kfree(tsevq);
> +	}
> +
> +	mutex_unlock(&ptp->close_mux);
> +}
> +
> +long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
> +	       unsigned long arg)
> +{
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
>  	struct ptp_sys_offset_extended *extoff = NULL;
>  	struct ptp_sys_offset_precise precise_offset;
>  	struct system_device_crosststamp xtstamp;
> @@ -432,65 +492,75 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	return err;
>  }
>  
> -__poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
> +__poll_t ptp_poll(struct posix_clock_user *pcuser, struct file *fp,
> +		  poll_table *wait)
>  {
> -	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
>  	struct timestamp_event_queue *queue;
>  
> -	poll_wait(fp, &ptp->tsev_wq, wait);
> +	queue = pcuser->private_clkdata;
> +	if (!queue)
> +		return EPOLLERR;
>  
> -	/* Extract only the first element in the queue list
> -	 * TODO: Identify the relevant queue
> -	 */
> -	queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
> +	poll_wait(fp, &ptp->tsev_wq, wait);
>  
>  	return queue_cnt(queue) ? EPOLLIN : 0;
>  }
>  
>  #define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
>  
> -ssize_t ptp_read(struct posix_clock *pc,
> -		 uint rdflags, char __user *buf, size_t cnt)
> +ssize_t ptp_read(struct posix_clock_user *pcuser, uint rdflags,
> +		 char __user *buf, size_t cnt)
>  {
> -	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
>  	struct timestamp_event_queue *queue;
>  	struct ptp_extts_event *event;
>  	unsigned long flags;
>  	size_t qcnt, i;
>  	int result;
>  
> -	/* Extract only the first element in the queue list
> -	 * TODO: Identify the relevant queue
> -	 */
> -	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
> -				 qlist);
> +	queue = pcuser->private_clkdata;
> +	if (!queue) {
> +		result = -EINVAL;
> +		goto exit;
> +	}
>  
> -	if (cnt % sizeof(struct ptp_extts_event) != 0)
> -		return -EINVAL;
> +	if (queue->close_req) {
> +		result = -EPIPE;
> +		goto exit;
> +	}
> +
> +	if (!queue->ida) {
> +		result = -ENODEV;
> +		goto exit;
> +	}
> +
> +	if (cnt % sizeof(struct ptp_extts_event) != 0) {
> +		result = -EINVAL;
> +		goto exit;
> +	}
>  
>  	if (cnt > EXTTS_BUFSIZE)
>  		cnt = EXTTS_BUFSIZE;
>  
>  	cnt = cnt / sizeof(struct ptp_extts_event);
>  
> -	if (mutex_lock_interruptible(&ptp->tsevq_mux))
> -		return -ERESTARTSYS;
> -
>  	if (wait_event_interruptible(ptp->tsev_wq,
>  				     ptp->defunct || queue_cnt(queue))) {
> -		mutex_unlock(&ptp->tsevq_mux);
>  		return -ERESTARTSYS;
>  	}
>  
>  	if (ptp->defunct) {
> -		mutex_unlock(&ptp->tsevq_mux);
> -		return -ENODEV;
> +		result = -ENODEV;
> +		goto exit;
>  	}
>  
>  	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
>  	if (!event) {
> -		mutex_unlock(&ptp->tsevq_mux);
> -		return -ENOMEM;
> +		result = -ENOMEM;
> +		goto exit;
>  	}
>  
>  	spin_lock_irqsave(&queue->lock, flags);
> @@ -509,12 +579,16 @@ ssize_t ptp_read(struct posix_clock *pc,
>  
>  	cnt = cnt * sizeof(struct ptp_extts_event);
>  
> -	mutex_unlock(&ptp->tsevq_mux);
> -
>  	result = cnt;
> -	if (copy_to_user(buf, event, cnt))
> +	if (copy_to_user(buf, event, cnt)) {
>  		result = -EFAULT;
> +		goto free_event;
> +	}
>  
> +free_event:
>  	kfree(event);
> +exit:
> +	if (result < 0)
> +		ptp_release(pcuser);
>  	return result;
>  }
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 8996ff500392..9e271ad66933 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
> +#include <linux/delay.h>
>  #include <uapi/linux/sched/types.h>
>  
>  #include "ptp_private.h"
> @@ -162,20 +163,51 @@ static struct posix_clock_operations ptp_clock_ops = {
>  	.clock_settime	= ptp_clock_settime,
>  	.ioctl		= ptp_ioctl,
>  	.open		= ptp_open,
> +	.release	= ptp_release,
> +	.flush_users	= ptp_flush_users,
>  	.poll		= ptp_poll,
>  	.read		= ptp_read,
>  };
>  
>  static void ptp_clean_queue_list(struct ptp_clock *ptp)
>  {
> -	struct timestamp_event_queue *element;
> -	struct list_head *pos, *next;
> +	struct timestamp_event_queue *tsevq;
> +	struct ptp_clock_event event;
> +	unsigned long flags;
> +	int cnt;
> +
> +	memset(&event, 0, sizeof(event));
> +	/* Request close of all open files */
> +	list_for_each_entry(tsevq, &ptp->tsevqs, qlist) {
> +		tsevq->close_req = true;
> +		/* Make sure the queue is fed so that the reader is notified */
> +		enqueue_external_timestamp(tsevq, &event);
> +	}
> +	wake_up_interruptible(&ptp->tsev_wq);
>  
> -	list_for_each_safe(pos, next, &ptp->tsevqs) {
> -		element = list_entry(pos, struct timestamp_event_queue, qlist);
> -		list_del(pos);
> -		kfree(element);
> +	/* Wait for all to close */
> +	cnt = list_count_nodes(&ptp->tsevqs);
> +	while (cnt > 1) {
> +		msleep(20);
> +		cnt = list_count_nodes(&ptp->tsevqs);
>  	}
> +	cnt = list_count_nodes(&ptp->closed_tsevqs);
> +	while (cnt > 1) {
> +		msleep(20);
> +		cnt = list_count_nodes(&ptp->closed_tsevqs);
> +	}
> +
> +	/* Delete first entry */
> +	tsevq = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
> +				 qlist);
> +
> +	spin_lock_irqsave(&tsevq->lock, flags);
> +	ida_destroy(tsevq->ida);
> +	kfree(tsevq->ida);
> +	tsevq->ida = NULL;
> +	list_del(&tsevq->qlist);
> +	spin_unlock_irqrestore(&tsevq->lock, flags);
> +	kfree(tsevq);
>  }
>  
>  static void ptp_clock_release(struct device *dev)
> @@ -184,11 +216,11 @@ static void ptp_clock_release(struct device *dev)
>  
>  	ptp_cleanup_pin_groups(ptp);
>  	kfree(ptp->vclock_index);
> -	mutex_destroy(&ptp->tsevq_mux);
>  	mutex_destroy(&ptp->pincfg_mux);
>  	mutex_destroy(&ptp->n_vclocks_mux);
>  	ptp_clean_queue_list(ptp);
>  	ida_free(&ptp_clocks_map, ptp->index);
> +	mutex_destroy(&ptp->close_mux);
>  	kfree(ptp);
>  }
>  
> @@ -243,15 +275,23 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	ptp->devid = MKDEV(major, index);
>  	ptp->index = index;
>  	INIT_LIST_HEAD(&ptp->tsevqs);
> +	INIT_LIST_HEAD(&ptp->closed_tsevqs);
>  	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
>  	if (!queue)
>  		goto no_memory_queue;
> +	queue->close_req = false;
> +	queue->ida = kzalloc(sizeof(*queue->ida), GFP_KERNEL);
> +	if (!queue->ida)
> +		goto no_memory_queue;
> +	ida_init(queue->ida);
>  	spin_lock_init(&queue->lock);
>  	list_add_tail(&queue->qlist, &ptp->tsevqs);
> -	/* TODO - Transform or delete this mutex */
> -	mutex_init(&ptp->tsevq_mux);
> +	queue->oid = ida_alloc(queue->ida, GFP_KERNEL);
> +	if (queue->oid < 0)
> +		goto ida_err;
>  	mutex_init(&ptp->pincfg_mux);
>  	mutex_init(&ptp->n_vclocks_mux);
> +	mutex_init(&ptp->close_mux);
>  	init_waitqueue_head(&ptp->tsev_wq);
>  
>  	if (ptp->info->getcycles64 || ptp->info->getcyclesx64) {
> @@ -350,9 +390,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	if (ptp->kworker)
>  		kthread_destroy_worker(ptp->kworker);
>  kworker_err:
> -	mutex_destroy(&ptp->tsevq_mux);
>  	mutex_destroy(&ptp->pincfg_mux);
>  	mutex_destroy(&ptp->n_vclocks_mux);
> +	mutex_destroy(&ptp->close_mux);
> +ida_err:
>  	ptp_clean_queue_list(ptp);
>  no_memory_queue:
>  	ida_free(&ptp_clocks_map, index);
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 314c21c39f6a..529d3d421ba0 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -16,6 +16,7 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/time.h>
>  #include <linux/list.h>
> +#include <linux/idr.h>
>  
>  #define PTP_MAX_TIMESTAMPS 128
>  #define PTP_BUF_TIMESTAMPS 30
> @@ -26,7 +27,12 @@ struct timestamp_event_queue {
>  	int head;
>  	int tail;
>  	spinlock_t lock;
> +	struct posix_clock_user *pcreader;
>  	struct list_head qlist;
> +	pid_t reader_pid;
> +	struct ida *ida;
> +	int oid;
> +	bool close_req;
>  };
>  
>  struct ptp_clock {
> @@ -38,7 +44,8 @@ struct ptp_clock {
>  	struct pps_device *pps_source;
>  	long dialed_frequency; /* remembers the frequency adjustment */
>  	struct list_head tsevqs; /* timestamp fifo list */
> -	struct mutex tsevq_mux; /* one process at a time reading the fifo */
> +	struct list_head closed_tsevqs; /* close pending timestamp fifo lists */
> +	struct mutex close_mux; /* protect user clean procecdures */
>  	struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
>  	wait_queue_head_t tsev_wq;
>  	int defunct; /* tells readers to go away when clock is being removed */
> @@ -109,6 +116,17 @@ static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
>  	return ptp_vclock_in_use(ptp);
>  }
>  
> +/* Find the tsevq object id handler */
> +static inline struct ida *ptp_get_tsevq_ida(struct ptp_clock *ptp)
> +{
> +	struct timestamp_event_queue *queue;
> +
> +	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
> +				 qlist);
> +
> +	return queue->ida;
> +}
> +
>  extern struct class *ptp_class;
>  
>  /*
> @@ -119,16 +137,20 @@ extern struct class *ptp_class;
>  int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
>  		    enum ptp_pin_function func, unsigned int chan);
>  
> -long ptp_ioctl(struct posix_clock *pc,
> -	       unsigned int cmd, unsigned long arg);
> +long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
> +	       unsigned long arg);
> +
> +int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode);
> +
> +int ptp_release(struct posix_clock_user *pcuser);
>  
> -int ptp_open(struct posix_clock *pc, fmode_t fmode);
> +void ptp_flush_users(struct posix_clock *pc);
>  
> -ssize_t ptp_read(struct posix_clock *pc,
> -		 uint flags, char __user *buf, size_t cnt);
> +ssize_t ptp_read(struct posix_clock_user *pcuser, uint flags, char __user *buf,
> +		 size_t cnt);
>  
> -__poll_t ptp_poll(struct posix_clock *pc,
> -	      struct file *fp, poll_table *wait);
> +__poll_t ptp_poll(struct posix_clock_user *pcuser, struct file *fp,
> +		  poll_table *wait);
>  
>  /*
>   * see ptp_sysfs.c
> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index 2675f383cd0a..c02aaba729e0 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -81,15 +81,14 @@ static ssize_t extts_fifo_show(struct device *dev,
>  	size_t qcnt;
>  	int cnt = 0;
>  
> +	cnt = list_count_nodes(&ptp->tsevqs);
> +	if (cnt <= 0)
> +		goto out;
> +
>  	/* The sysfs fifo will always draw from the fist queue */
>  	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
>  				 qlist);
>  
> -	memset(&event, 0, sizeof(event));
> -
> -	if (mutex_lock_interruptible(&ptp->tsevq_mux))
> -		return -ERESTARTSYS;
> -
>  	spin_lock_irqsave(&queue->lock, flags);
>  	qcnt = queue_cnt(queue);
>  	if (qcnt) {
> @@ -104,7 +103,6 @@ static ssize_t extts_fifo_show(struct device *dev,
>  	cnt = snprintf(page, PAGE_SIZE, "%u %lld %u\n",
>  		       event.index, event.t.sec, event.t.nsec);
>  out:
> -	mutex_unlock(&ptp->tsevq_mux);
>  	return cnt;
>  }
>  static DEVICE_ATTR(fifo, 0444, extts_fifo_show, NULL);
> diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
> index 468328b1e1dd..8f844ac28aa8 100644
> --- a/include/linux/posix-clock.h
> +++ b/include/linux/posix-clock.h
> @@ -14,6 +14,7 @@
>  #include <linux/rwsem.h>
>  
>  struct posix_clock;
> +struct posix_clock_user;
>  
>  /**
>   * struct posix_clock_operations - functional interface to the clock
> @@ -50,18 +51,20 @@ struct posix_clock_operations {
>  	/*
>  	 * Optional character device methods:
>  	 */
> -	long    (*ioctl)   (struct posix_clock *pc,
> -			    unsigned int cmd, unsigned long arg);
> +	long (*ioctl)(struct posix_clock_user *pcuser, unsigned int cmd,
> +		      unsigned long arg);
>  
> -	int     (*open)    (struct posix_clock *pc, fmode_t f_mode);
> +	int (*open)(struct posix_clock_user *pcuser, fmode_t f_mode);
>  
> -	__poll_t (*poll)   (struct posix_clock *pc,
> -			    struct file *file, poll_table *wait);
> +	__poll_t (*poll)(struct posix_clock_user *pcuser, struct file *file,
> +			 poll_table *wait);
>  
> -	int     (*release) (struct posix_clock *pc);
> +	int (*release)(struct posix_clock_user *pcuser);
>  
> -	ssize_t (*read)    (struct posix_clock *pc,
> -			    uint flags, char __user *buf, size_t cnt);
> +	void (*flush_users)(struct posix_clock *pc);
> +
> +	ssize_t (*read)(struct posix_clock_user *pcuser, uint flags,
> +			char __user *buf, size_t cnt);
>  };
>  
>  /**
> @@ -90,6 +93,11 @@ struct posix_clock {
>  	bool zombie;
>  };
>  
> +struct posix_clock_user {
> +	struct posix_clock *clk;
> +	void *private_clkdata;
> +};
> +
>  /**
>   * posix_clock_register() - register a new clock
>   * @clk:   Pointer to the clock. Caller must provide 'ops' field
> diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
> index 77c0c2370b6d..8ae76492f7ea 100644
> --- a/kernel/time/posix-clock.c
> +++ b/kernel/time/posix-clock.c
> @@ -19,7 +19,12 @@
>   */
>  static struct posix_clock *get_posix_clock(struct file *fp)
>  {
> -	struct posix_clock *clk = fp->private_data;
> +	struct posix_clock_user *pcuser = fp->private_data;
> +	struct posix_clock *clk;
> +
> +	if (!pcuser)
> +		return NULL;
> +	clk = pcuser->clk;
>  
>  	down_read(&clk->rwsem);
>  
> @@ -39,6 +44,7 @@ static void put_posix_clock(struct posix_clock *clk)
>  static ssize_t posix_clock_read(struct file *fp, char __user *buf,
>  				size_t count, loff_t *ppos)
>  {
> +	struct posix_clock_user *pcuser = fp->private_data;
>  	struct posix_clock *clk = get_posix_clock(fp);
>  	int err = -EINVAL;
>  
> @@ -46,7 +52,7 @@ static ssize_t posix_clock_read(struct file *fp, char __user *buf,
>  		return -ENODEV;
>  
>  	if (clk->ops.read)
> -		err = clk->ops.read(clk, fp->f_flags, buf, count);
> +		err = clk->ops.read(pcuser, fp->f_flags, buf, count);
>  
>  	put_posix_clock(clk);
>  
> @@ -55,6 +61,7 @@ static ssize_t posix_clock_read(struct file *fp, char __user *buf,
>  
>  static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
>  {
> +	struct posix_clock_user *pcuser = fp->private_data;
>  	struct posix_clock *clk = get_posix_clock(fp);
>  	__poll_t result = 0;
>  
> @@ -62,7 +69,7 @@ static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
>  		return EPOLLERR;
>  
>  	if (clk->ops.poll)
> -		result = clk->ops.poll(clk, fp, wait);
> +		result = clk->ops.poll(pcuser, fp, wait);
>  
>  	put_posix_clock(clk);
>  
> @@ -72,6 +79,7 @@ static __poll_t posix_clock_poll(struct file *fp, poll_table *wait)
>  static long posix_clock_ioctl(struct file *fp,
>  			      unsigned int cmd, unsigned long arg)
>  {
> +	struct posix_clock_user *pcuser = fp->private_data;
>  	struct posix_clock *clk = get_posix_clock(fp);
>  	int err = -ENOTTY;
>  
> @@ -79,7 +87,7 @@ static long posix_clock_ioctl(struct file *fp,
>  		return -ENODEV;
>  
>  	if (clk->ops.ioctl)
> -		err = clk->ops.ioctl(clk, cmd, arg);
> +		err = clk->ops.ioctl(pcuser, cmd, arg);
>  
>  	put_posix_clock(clk);
>  
> @@ -90,6 +98,7 @@ static long posix_clock_ioctl(struct file *fp,
>  static long posix_clock_compat_ioctl(struct file *fp,
>  				     unsigned int cmd, unsigned long arg)
>  {
> +	struct posix_clock_user *pcuser = fp->private_data;
>  	struct posix_clock *clk = get_posix_clock(fp);
>  	int err = -ENOTTY;
>  
> @@ -97,7 +106,7 @@ static long posix_clock_compat_ioctl(struct file *fp,
>  		return -ENODEV;
>  
>  	if (clk->ops.ioctl)
> -		err = clk->ops.ioctl(clk, cmd, arg);
> +		err = clk->ops.ioctl(pcuser, cmd, arg);
>  
>  	put_posix_clock(clk);
>  
> @@ -110,6 +119,7 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
>  	int err;
>  	struct posix_clock *clk =
>  		container_of(inode->i_cdev, struct posix_clock, cdev);
> +	struct posix_clock_user *pcuser;
>  
>  	down_read(&clk->rwsem);
>  
> @@ -117,14 +127,20 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
>  		err = -ENODEV;
>  		goto out;
>  	}
> +	pcuser = kzalloc(sizeof(*pcuser), GFP_KERNEL);
> +	if (!pcuser) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	pcuser->clk = clk;
> +	fp->private_data = pcuser;
>  	if (clk->ops.open)
> -		err = clk->ops.open(clk, fp->f_mode);
> +		err = clk->ops.open(pcuser, fp->f_mode);
>  	else
>  		err = 0;
>  
>  	if (!err) {
>  		get_device(clk->dev);
> -		fp->private_data = clk;
>  	}
>  out:
>  	up_read(&clk->rwsem);
> @@ -133,14 +149,23 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
>  
>  static int posix_clock_release(struct inode *inode, struct file *fp)
>  {
> -	struct posix_clock *clk = fp->private_data;
> +	struct posix_clock_user *pcuser = fp->private_data;
> +	struct posix_clock *clk;
>  	int err = 0;
>  
> +	if (!pcuser)
> +		return -ENODEV;
> +	clk = pcuser->clk;
> +
>  	if (clk->ops.release)
> -		err = clk->ops.release(clk);
> +		err = clk->ops.release(pcuser);
>  
>  	put_device(clk->dev);
>  
> +	if (clk->ops.flush_users)
> +		clk->ops.flush_users(clk);
> +
> +	kfree(pcuser);
>  	fp->private_data = NULL;
>  
>  	return err;
> -- 
> 2.34.1
>

-- 
Vinicius

Re: [PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Richard Cochran]

On Thu, Sep 28, 2023 at 03:35:43PM +0200, Xabier Marquiegui wrote:

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 197edf1179f1..65e7acaa40a9 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -101,14 +101,74 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
>  	return 0;
>  }
>  
> -int ptp_open(struct posix_clock *pc, fmode_t fmode)
> +int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode)
>  {
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
> +	struct ida *ida = ptp_get_tsevq_ida(ptp);

No need for IDA or ...

> +	struct timestamp_event_queue *queue;
> +
> +	if (!ida)
> +		return -EINVAL;
> +	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> +	if (!queue)
> +		return -EINVAL;
> +	queue->close_req = false;
> +	queue->reader_pid = task_pid_nr(current);

... PIDs.  Just allocate the queue and set:

> +	pcuser->private_clkdata = queue;

> +void ptp_flush_users(struct posix_clock *pc)

I can't see any need for "flush users".

open/release are per-user anyhow.

> @@ -184,11 +216,11 @@ static void ptp_clock_release(struct device *dev)
>  
>  	ptp_cleanup_pin_groups(ptp);
>  	kfree(ptp->vclock_index);
> -	mutex_destroy(&ptp->tsevq_mux);
>  	mutex_destroy(&ptp->pincfg_mux);
>  	mutex_destroy(&ptp->n_vclocks_mux);
>  	ptp_clean_queue_list(ptp);

You don't need this ^^^

Keep it simple:

- allocate queue on open()
- free the queue on release()

Hm?

>  	ida_free(&ptp_clocks_map, ptp->index);
> +	mutex_destroy(&ptp->close_mux);
>  	kfree(ptp);
>  }
>  

Thanks,
Richard

Re: [PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Richard Cochran]

On Thu, Sep 28, 2023 at 03:35:43PM +0200, Xabier Marquiegui wrote:

>  include/linux/posix-clock.h |  24 ++++---
>  kernel/time/posix-clock.c   |  43 +++++++++---

This change deserves its own patch.  Please put that first in the
series.  And be sure to CC tglx and John Stultz.

> diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
> index 468328b1e1dd..8f844ac28aa8 100644
> --- a/include/linux/posix-clock.h
> +++ b/include/linux/posix-clock.h
> @@ -14,6 +14,7 @@
>  #include <linux/rwsem.h>
>  
>  struct posix_clock;
> +struct posix_clock_user;

No need for a forward declaration, since the struct is below.
Just put it here.

> @@ -90,6 +93,11 @@ struct posix_clock {
>  	bool zombie;
>  };
>  
> +struct posix_clock_user {

The idea is fine, but how about calling this "posix_clock_context"
instead?

> +	struct posix_clock *clk;
> +	void *private_clkdata;
> +};

Thanks,
Richard

Re: [PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Richard Cochran]

On Thu, Sep 28, 2023 at 03:35:43PM +0200, Xabier Marquiegui wrote:

> @@ -19,7 +19,12 @@
>   */
>  static struct posix_clock *get_posix_clock(struct file *fp)
>  {
> -	struct posix_clock *clk = fp->private_data;
> +	struct posix_clock_user *pcuser = fp->private_data;
> +	struct posix_clock *clk;
> +
> +	if (!pcuser)
> +		return NULL;

You added a test for (fp->private_data != NULL) that wasn't there
before.  Was there an issue with the existing code?

If so, please fix that as the first patch in your series.
If not, please remove the pointless test from your change.

Thanks,
Richard

Re: [PATCH net-next v3 2/3] ptp: support multiple timestamp event readers

[Simon Horman]

On Thu, Sep 28, 2023 at 03:35:43PM +0200, Xabier Marquiegui wrote:
> Use linked lists to create one event queue per open file. This enables
> simultaneous readers for timestamp event queues.
> 
> Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
> Suggested-by: Richard Cochran <richardcochran@gmail.com>

Hi Xabier,

some minor feedback from Smatch via myself follows.

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 197edf1179f1..65e7acaa40a9 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -101,14 +101,74 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
>  	return 0;
>  }
>  
> -int ptp_open(struct posix_clock *pc, fmode_t fmode)
> +int ptp_open(struct posix_clock_user *pcuser, fmode_t fmode)
>  {
> +	struct ptp_clock *ptp =
> +		container_of(pcuser->clk, struct ptp_clock, clock);
> +	struct ida *ida = ptp_get_tsevq_ida(ptp);
> +	struct timestamp_event_queue *queue;
> +
> +	if (!ida)
> +		return -EINVAL;
> +	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> +	if (!queue)
> +		return -EINVAL;
> +	queue->close_req = false;
> +	queue->reader_pid = task_pid_nr(current);
> +	spin_lock_init(&queue->lock);
> +	queue->ida = ida;
> +	queue->oid = ida_alloc(ida, GFP_KERNEL);
> +	if (queue->oid < 0) {
> +		kfree(queue);

queue is freed on the line above but dereferenced on the line below.

As flagged by Smatch.

> +		return queue->oid;
> +	}
> +	list_add_tail(&queue->qlist, &ptp->tsevqs);
> +	pcuser->private_clkdata = queue;
> +
>  	return 0;
>  }

...

> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c

...

> @@ -243,15 +275,23 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	ptp->devid = MKDEV(major, index);
>  	ptp->index = index;
>  	INIT_LIST_HEAD(&ptp->tsevqs);
> +	INIT_LIST_HEAD(&ptp->closed_tsevqs);
>  	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
>  	if (!queue)
>  		goto no_memory_queue;
> +	queue->close_req = false;
> +	queue->ida = kzalloc(sizeof(*queue->ida), GFP_KERNEL);
> +	if (!queue->ida)
> +		goto no_memory_queue;

It's not clear to me that queue isn't leaked here.

As flagged by Smatch.

> +	ida_init(queue->ida);
>  	spin_lock_init(&queue->lock);
>  	list_add_tail(&queue->qlist, &ptp->tsevqs);
> -	/* TODO - Transform or delete this mutex */
> -	mutex_init(&ptp->tsevq_mux);
> +	queue->oid = ida_alloc(queue->ida, GFP_KERNEL);
> +	if (queue->oid < 0)
> +		goto ida_err;
>  	mutex_init(&ptp->pincfg_mux);
>  	mutex_init(&ptp->n_vclocks_mux);
> +	mutex_init(&ptp->close_mux);
>  	init_waitqueue_head(&ptp->tsev_wq);
>  
>  	if (ptp->info->getcycles64 || ptp->info->getcyclesx64) {
> @@ -350,9 +390,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	if (ptp->kworker)
>  		kthread_destroy_worker(ptp->kworker);
>  kworker_err:
> -	mutex_destroy(&ptp->tsevq_mux);
>  	mutex_destroy(&ptp->pincfg_mux);
>  	mutex_destroy(&ptp->n_vclocks_mux);
> +	mutex_destroy(&ptp->close_mux);
> +ida_err:
>  	ptp_clean_queue_list(ptp);
>  no_memory_queue:
>  	ida_free(&ptp_clocks_map, index);

...

[PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Xabier Marquiegui]

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)
 
 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

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Vinicius Costa Gomes]

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

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Xabier Marquiegui]

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.

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Vinicius Costa Gomes]

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,
-- 
Vinicius

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Richard Cochran]

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

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Simon Horman]

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;

...

Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Richard Cochran]

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

Re: [PATCH net-next v3 0/3] ptp: Support for multiple filtered timestamp event queue readers

[Vinicius Costa Gomes]

Xabier Marquiegui <reibax@gmail.com> writes:

> On systems with multiple timestamp event channels, there can be scenarios where
> multiple userspace readers want to access the timestamping data for various
> purposes.
>
> One such example is wanting to use a pps out for time synchronization, and
> wanting to timestamp external events with the synchronized time base 
> simultaneously.
>
> Timestmp event consumers on the other hand, are often interested in a subset of
> the available timestamp channels. linuxptp ts2phc, for example, is not happy if
> more than one timestamping channel is active on the device it is reading from.
>
> This patch-set introduces linked lists to support multiple timestamp event queue
> consumers, and timestamp event channel filters through IOCTLs.
>
> Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
> Suggested-by: Richard Cochran <richardcochran@gmail.com>

On the series organization side, my suggestion about the order of things:

1. Any preparation work;
2. Introduce the new UAPI: the ioctls (this needs to be front and center, as it has
   to be maintained for a long time);
3. Changes to the "core" (posix_clock and friends);
4. "glueing" everything together in the driver;
5. Tests;

(it is possible that that the "preparation" can be moved to the
"glueing" part)


Cheers,
-- 
Vinicius

Re: [PATCH net-next v3 0/3] ptp: Support for multiple filtered timestamp event queue readers

[Richard Cochran]

On Thu, Sep 28, 2023 at 03:35:41PM +0200, Xabier Marquiegui wrote:
> On systems with multiple timestamp event channels, there can be scenarios where
> multiple userspace readers want to access the timestamping data for various
> purposes.

This series is shaping up nicely.  Can you please include the diffstat
in the cover letter next time?

(git format-patch should do that for you)

Thanks,
Richard