netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
       [not found] ` <Y+3m/PpzkBN9kxJY@localhost>
@ 2023-02-16 17:54   ` Matt Corallo
  2023-02-16 22:54     ` Richard Cochran
  0 siblings, 1 reply; 26+ messages in thread
From: Matt Corallo @ 2023-02-16 17:54 UTC (permalink / raw)
  To: chrony-dev, Miroslav Lichvar; +Cc: Richard Cochran, netdev



On 2/16/23 12:19 AM, Miroslav Lichvar wrote:
> On Wed, Feb 15, 2023 at 10:27:15PM -0800, Matt Corallo wrote:
>> My naive solution from skimming the code would be to shove
>> formerly-discarded samples into a global limited queue and check for
>> available timestamps in `phc_poll`. However, I have no idea if the time
>> difference between when the sample was taken by the hardware and when the
>> `HCL_CookTime` call is done would impact accuracy (or maybe the opposite,
>> since we'd then be cooking time with the hardware clock right after taking
>> the HCL sample rather than when the PHC timestamp happens), or if such a
>> patch would simply be rejected as a dirty, dirty hack rather than unifying
>> the PHC read sockets across the devices into one socket (via some global
>> tracking the device -> socket mapping?) and passing the samples out
>> appropriately. Let me know what makes the most sense here.
> 
> My first thought is that this should be addressed in the kernel, so
> even different processes having open the PHC device can receive all
> extts samples. If it turns out it's too difficult to do for the
> character device (I'm not very familiar with that subsystem), maybe it
> could be done at least in sysfs (/sys/class/ptp/ptp*/fifo or a new
> file showing the last event like the PPS assert and clear).

I mean my first thought seeing an ioctl on a socket that gives an explicit channel and then receives 
crap from other channels on the same socket was "wtf" so I went and read the kernel to figure out 
why first to see if its a driver bug. I can't seem to find *any* documentation for how these ioctls 
are supposed to work, but it seems the "request" here is kinda misnomer, its really a "configure 
hardware" request, and is unrelated to future reads on the socket, or really the specific socket at all.

As for duplicating the output across sockets, ptp_chardev.c's `ptp_read` is pretty trivial - just 
pop the next sample off the queue and return it. Tweaking that to copy the sample into every reader 
is probably above my paygrade (and has a whole host of leak risk I'd probably screw up). 
`extts_fifo_show` appears to be functionally identical.

I've CC'd the MAINTAINERs for ptp to see what they think about this, though it won't let chrony 
support this without a kernel upgrade - not sure if that's an issue for chrony or not.

Matt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-16 17:54   ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Matt Corallo
@ 2023-02-16 22:54     ` Richard Cochran
  2023-02-17  0:58       ` Matt Corallo
  2023-02-20 10:08       ` Miroslav Lichvar
  0 siblings, 2 replies; 26+ messages in thread
From: Richard Cochran @ 2023-02-16 22:54 UTC (permalink / raw)
  To: Matt Corallo; +Cc: chrony-dev, Miroslav Lichvar, netdev

On Thu, Feb 16, 2023 at 09:54:56AM -0800, Matt Corallo wrote:
> 
> 
> On 2/16/23 12:19 AM, Miroslav Lichvar wrote:
> > My first thought is that this should be addressed in the kernel, so
> > even different processes having open the PHC device can receive all
> > extts samples. If it turns out it's too difficult to do for the
> > character device (I'm not very familiar with that subsystem), maybe it
> > could be done at least in sysfs (/sys/class/ptp/ptp*/fifo or a new
> > file showing the last event like the PPS assert and clear).

The PPS thing has a race, and so I'd rather not copy that!
 
> As for duplicating the output across sockets, ptp_chardev.c's `ptp_read` is
> pretty trivial - just pop the next sample off the queue and return it.
> Tweaking that to copy the sample into every reader is probably above my
> paygrade (and has a whole host of leak risk I'd probably screw up).
> `extts_fifo_show` appears to be functionally identical.

Each extts in the fifo is delivered only once.  If there are multiple
readers, each reader will receive only some of the data.  This is
similar to how a pipe behaves.

HTH,
Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-16 22:54     ` Richard Cochran
@ 2023-02-17  0:58       ` Matt Corallo
  2023-02-20 10:08       ` Miroslav Lichvar
  1 sibling, 0 replies; 26+ messages in thread
From: Matt Corallo @ 2023-02-17  0:58 UTC (permalink / raw)
  To: Richard Cochran; +Cc: chrony-dev, Miroslav Lichvar, netdev



On 2/16/23 2:54 PM, Richard Cochran wrote:
> On Thu, Feb 16, 2023 at 09:54:56AM -0800, Matt Corallo wrote:
>> As for duplicating the output across sockets, ptp_chardev.c's `ptp_read` is
>> pretty trivial - just pop the next sample off the queue and return it.
>> Tweaking that to copy the sample into every reader is probably above my
>> paygrade (and has a whole host of leak risk I'd probably screw up).
>> `extts_fifo_show` appears to be functionally identical.
> 
> Each extts in the fifo is delivered only once.  If there are multiple
> readers, each reader will receive only some of the data.  This is
> similar to how a pipe behaves.

Right, sorry if the context wasn't clear, I only realized part of the message was removed in the 
first reply after sending. The question from Miroslav was, basically, "would kernel accept something 
to only get notified of extts pulses on a given channel, and, if so, how would we go about doing that".

The "we get pulses from all extts channels on the same socket" thing is a bit annoying to munge into 
chrony - it has the concept of "refclocks" which are a single clock, in this case a single pps pulse 
generator. If you have two of them on the same PTP clock but coming in on different pins/channels it 
doesn't have a way to express that outside of two refclocks. While we could take pulses from both 
refclocks on one socket and shove them into some queue and have the refclocks pick that data up its 
a bunch of complexity on the client side and not super clean in the current codebase.

Thanks,
Matt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-16 22:54     ` Richard Cochran
  2023-02-17  0:58       ` Matt Corallo
@ 2023-02-20 10:08       ` Miroslav Lichvar
  2023-02-20 15:24         ` Richard Cochran
  1 sibling, 1 reply; 26+ messages in thread
From: Miroslav Lichvar @ 2023-02-20 10:08 UTC (permalink / raw)
  To: Richard Cochran; +Cc: Matt Corallo, chrony-dev, netdev

On Thu, Feb 16, 2023 at 02:54:29PM -0800, Richard Cochran wrote:
> Each extts in the fifo is delivered only once.  If there are multiple
> readers, each reader will receive only some of the data.  This is
> similar to how a pipe behaves.

Does it need to be that way? It seems strange for the kernel to
support enabling PPS on multiple channels at the same time, but not
allow multiple applications to receive all samples from their channel.

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-20 10:08       ` Miroslav Lichvar
@ 2023-02-20 15:24         ` Richard Cochran
  2023-02-23 20:56           ` Matt Corallo
  0 siblings, 1 reply; 26+ messages in thread
From: Richard Cochran @ 2023-02-20 15:24 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: Matt Corallo, chrony-dev, netdev

On Mon, Feb 20, 2023 at 11:08:23AM +0100, Miroslav Lichvar wrote:
> Does it need to be that way? It seems strange for the kernel to
> support enabling PPS on multiple channels at the same time, but not
> allow multiple applications to receive all samples from their channel.

It does not need to be that way, but nobody ever wanted multiple
readers before.

Implementing this would make the kernel side much more complex, as the
code would need per-reader tracking of the buffered time stamps, or
per-reader fifo buffers, etc.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-20 15:24         ` Richard Cochran
@ 2023-02-23 20:56           ` Matt Corallo
  2023-02-24  0:19             ` Richard Cochran
  0 siblings, 1 reply; 26+ messages in thread
From: Matt Corallo @ 2023-02-23 20:56 UTC (permalink / raw)
  To: Richard Cochran, Miroslav Lichvar; +Cc: chrony-dev, netdev



On 2/20/23 7:24 AM, Richard Cochran wrote:
> On Mon, Feb 20, 2023 at 11:08:23AM +0100, Miroslav Lichvar wrote:
>> Does it need to be that way? It seems strange for the kernel to
>> support enabling PPS on multiple channels at the same time, but not
>> allow multiple applications to receive all samples from their channel.
> 
> It does not need to be that way, but nobody ever wanted multiple
> readers before.
> 
> Implementing this would make the kernel side much more complex, as the
> code would need per-reader tracking of the buffered time stamps, or
> per-reader fifo buffers, etc.

There's two separate questions here - multiple readers receiving the same data, and multiple readers 
receiving data exclusively about one channel.

I'd imagine the second is (much?) easier to implement, whereas the first is a bunch of complexity.

At least personally I'm okay with the second, rather than the first, and that fixes the issue for 
chrony, though it doesn't allow one to, say, get raw samples in one program while having another 
handle them.

Matt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-23 20:56           ` Matt Corallo
@ 2023-02-24  0:19             ` Richard Cochran
  2023-02-24  1:18               ` Matt Corallo
  0 siblings, 1 reply; 26+ messages in thread
From: Richard Cochran @ 2023-02-24  0:19 UTC (permalink / raw)
  To: Matt Corallo; +Cc: Miroslav Lichvar, chrony-dev, netdev

On Thu, Feb 23, 2023 at 12:56:34PM -0800, Matt Corallo wrote:

> There's two separate questions here - multiple readers receiving the same
> data, and multiple readers receiving data exclusively about one channel.
> 
> I'd imagine the second is (much?) easier to implement, whereas the first is a bunch of complexity.

This second idea would require a new API, so that user could select a
particular channel.

First idea would only change kernel behavior without changing the API.

Thanks,
Richard



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-24  0:19             ` Richard Cochran
@ 2023-02-24  1:18               ` Matt Corallo
  2023-02-24  5:07                 ` Richard Cochran
  0 siblings, 1 reply; 26+ messages in thread
From: Matt Corallo @ 2023-02-24  1:18 UTC (permalink / raw)
  To: Richard Cochran; +Cc: Miroslav Lichvar, chrony-dev, netdev



On 2/23/23 4:19 PM, Richard Cochran wrote:
> On Thu, Feb 23, 2023 at 12:56:34PM -0800, Matt Corallo wrote:
> 
>> There's two separate questions here - multiple readers receiving the same
>> data, and multiple readers receiving data exclusively about one channel.
>>
>> I'd imagine the second is (much?) easier to implement, whereas the first is a bunch of complexity.
> 
> This second idea would require a new API, so that user could select a
> particular channel.
> 
> First idea would only change kernel behavior without changing the API.

Fair point. I figured a new IOCTL to filter was a lighter lift, even if a bunch of boilerplate to 
define it.

I'm happy to take a crack at something to get the ball rolling, though not this week. I'm sure I 
could copy+paste to make a new IOCTL work, but adding relevant queue limiting means I have to go 
read much more kernel code to figure out which datastructures already exist there :).

It sounds like I should go replace the extts queue with a circular buffer, have every reader socket 
store an index in the buffer, and new sockets read only futures pulses? I assume a new pulse already 
wakes all select()ers on the sockets so nothing would need to change there. Is there some existing 
code somewhere I should crib off of or just run and see where I get?

Thanks,
Matt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-24  1:18               ` Matt Corallo
@ 2023-02-24  5:07                 ` Richard Cochran
  2023-08-29 11:47                   ` Xabier Marquiegui
  0 siblings, 1 reply; 26+ messages in thread
From: Richard Cochran @ 2023-02-24  5:07 UTC (permalink / raw)
  To: Matt Corallo; +Cc: Miroslav Lichvar, chrony-dev, netdev

On Thu, Feb 23, 2023 at 05:18:06PM -0800, Matt Corallo wrote:

> It sounds like I should go replace the extts queue with a circular buffer,
> have every reader socket store an index in the buffer, and new sockets read
> only futures pulses?

Single circular buffer with multiple heads will be complex.

It might be simpler to allocate one queue per reader.

If there are few readers, cost of allocation and en-queuing won't matter.

Thanks,
Richard


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-02-24  5:07                 ` Richard Cochran
@ 2023-08-29 11:47                   ` Xabier Marquiegui
  2023-08-29 11:47                     ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
  0 siblings, 1 reply; 26+ messages in thread
From: Xabier Marquiegui @ 2023-08-29 11:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

I am interested in moving forward this conversation. I have found myself
in a situation where I need to use multiple external timestamp channels
with different clients. In my specific application, I use channel 0 with
ts2phc to synchronize a PPS clock reference, and I want to use channel 1
to timestamp external events. Since ts2phc is consuming from the fifo, 
if I add another consumer everything starts to go wrong.

I would like to share my idea on how to solve this. Like Richard
mentions, I just create a separate work queue per channel, and add a
sysfs interface to dynamically be able to move queues from the
shared/multiplexed queue to individual queues.

This is the first time I try to contribute something using this mailing
list, so please forgive me if I'm doing something wrong.

Let me know what you thing about my code (the patch should be on the
next message). Thanks.

Xabier.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-29 11:47                   ` Xabier Marquiegui
@ 2023-08-29 11:47                     ` Xabier Marquiegui
  2023-08-29 14:07                       ` Richard Cochran
  0 siblings, 1 reply; 26+ messages in thread
From: Xabier Marquiegui @ 2023-08-29 11:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

Add the posibility to demultiplex the timestamp channels for
external timestamp event channels.

In some applications it can be necessary to have different
consumers for different timestamp channels. For example,
synchronize to an external pps source with linuxptp ts2phc
while timestmping external events with another application.

This change proposes the dynamic creation of one char-device
per timestamp channel only if the user requests the demuxing
of timestamp channels. It allows for on-the-fly demuxing of
specific channels.

The operation can be controlled via sysfs. See file
Documentation/ABI/testing/sysfs-ptp for more details.
---
 Documentation/ABI/testing/sysfs-ptp |  16 +++
 MAINTAINERS                         |   5 +
 drivers/ptp/Makefile                |   2 +-
 drivers/ptp/ptp_chardev.c           |   2 -
 drivers/ptp/ptp_clock.c             |  22 ++-
 drivers/ptp/ptp_demuxtschan.c       | 211 ++++++++++++++++++++++++++++
 drivers/ptp/ptp_private.h           |  25 ++++
 drivers/ptp/ptp_sysfs.c             | 113 +++++++++++++++
 8 files changed, 392 insertions(+), 4 deletions(-)
 create mode 100644 drivers/ptp/ptp_demuxtschan.c

diff --git a/Documentation/ABI/testing/sysfs-ptp b/Documentation/ABI/testing/sysfs-ptp
index 9c317ac7c47a..9d9875e7c56a 100644
--- a/Documentation/ABI/testing/sysfs-ptp
+++ b/Documentation/ABI/testing/sysfs-ptp
@@ -81,6 +81,22 @@ Description:
 		switches the physical clock back to normal, adjustable
 		operation.
 
+What:		/sys/class/ptp/ptp<N>/dmtsc_en_flags
+Date:		August 2023
+Contact:	Xabier Marquiegui <reibax@gmail.com>
+Description:
+		This read/write file controls the de-multiplexing of 
+		external timestamp channel fifos. In write more, you
+		can de-multiplex by enabling a channel or re-multiplex
+		it by disabling it. Write mode can be done in channel mode
+		for single channel control "c 0 1" or mask mode for 
+		multi-channel "m 0x3 1". Syntax is:
+		<mode (m/c)> <value> <enable (0/1)>
+		In read mode you get the enable mask for all channels in 
+		hex.
+		See function dmtsc_en_flags_store on 
+		drivers/ptp/ptp_sysfs.c for more details.
+
 What:		/sys/class/ptp/ptp<N>/pins
 Date:		March 2014
 Contact:	Richard Cochran <richardcochran@gmail.com>
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..8d9a24039d67 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17171,6 +17171,11 @@ S:	Maintained
 F:	drivers/ptp/ptp_vclock.c
 F:	net/ethtool/phc_vclocks.c
 
+PTP DEMUXED TS CHANEL FIFOS
+M: Xabier Marquiegui <reibax@gmail.com>
+S: Maintained
+F: drivers/ptp/ptp_demuxtschan.c
+
 PTRACE SUPPORT
 M:	Oleg Nesterov <oleg@redhat.com>
 S:	Maintained
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 553f18bf3c83..6dbae0b9aa7c 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -3,7 +3,7 @@
 # Makefile for PTP 1588 clock support.
 #
 
-ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o ptp_vclock.o
+ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o ptp_vclock.o ptp_demuxtschan.o
 ptp_kvm-$(CONFIG_X86)			:= ptp_kvm_x86.o ptp_kvm_common.o
 ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC)	:= ptp_kvm_arm.o ptp_kvm_common.o
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 362bf756e6b7..0c900042c389 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -441,8 +441,6 @@ __poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
 	return queue_cnt(&ptp->tsevq) ? 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)
 {
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 80f74e38c2da..383508c269a2 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -172,6 +172,8 @@ static void ptp_clock_release(struct device *dev)
 
 	ptp_cleanup_pin_groups(ptp);
 	kfree(ptp->vclock_index);
+	mutex_destroy(&ptp->dmtsc_devs.dmtsc_devs_mux);
+	mutex_destroy(&ptp->dmtsc_sysfs_mux);
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
@@ -232,7 +234,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->tsevq_mux);
 	mutex_init(&ptp->pincfg_mux);
 	mutex_init(&ptp->n_vclocks_mux);
+	mutex_init(&ptp->dmtsc_sysfs_mux);
+	mutex_init(&ptp->dmtsc_devs.dmtsc_devs_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
+	ptp->dmtsc_en_flags = 0x0;
+	ptp->dmtscevq = NULL;
+	ptp->dmtsc_devs.readers = 0;
+	ptp->dmtsc_devs.clean_request = false;
 
 	if (ptp->info->getcycles64 || ptp->info->getcyclesx64) {
 		ptp->has_cycles = true;
@@ -330,6 +338,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (ptp->kworker)
 		kthread_destroy_worker(ptp->kworker);
 kworker_err:
+	mutex_destroy(&ptp->dmtsc_devs.dmtsc_devs_mux);
+	mutex_destroy(&ptp->dmtsc_sysfs_mux);
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
@@ -351,6 +361,8 @@ static int unregister_vclock(struct device *dev, void *data)
 
 int ptp_clock_unregister(struct ptp_clock *ptp)
 {
+	ptp_dmtsc_dev_uregister(ptp);
+
 	if (ptp_vclock_in_use(ptp)) {
 		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
@@ -383,7 +395,15 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
 		break;
 
 	case PTP_CLOCK_EXTTS:
-		enqueue_external_timestamp(&ptp->tsevq, event);
+		/* If event index demuxed queue mask is enabled send to dedicated fifo */
+		if (ptp->dmtsc_en_flags & (0x1 << event->index)) {
+			enqueue_external_timestamp(&ptp->dmtscevq[event->index], event);
+		}
+		else
+		{
+			enqueue_external_timestamp(&ptp->tsevq, event);
+		}
+
 		wake_up_interruptible(&ptp->tsev_wq);
 		break;
 
diff --git a/drivers/ptp/ptp_demuxtschan.c b/drivers/ptp/ptp_demuxtschan.c
new file mode 100644
index 000000000000..516a1dabe8dd
--- /dev/null
+++ b/drivers/ptp/ptp_demuxtschan.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PTP exploded timestamp event queue driver
+ *
+ * Copyright 2023 Aingura IIoT
+ */
+
+#include <linux/slab.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include "ptp_private.h"
+
+static int ptp_dmtsc_open(struct inode *inode, struct file *file)
+{
+    struct ptp_dmtsc_cdev_info *cdev = container_of(inode->i_cdev,
+                struct ptp_dmtsc_cdev_info, dmtsc_cdev);
+
+    file->private_data = cdev;
+
+    if (mutex_lock_interruptible(&cdev->pclock->dmtsc_devs.dmtsc_devs_mux))
+		return -ERESTARTSYS;
+    cdev->pclock->dmtsc_devs.readers++;
+    mutex_unlock(&cdev->pclock->dmtsc_devs.dmtsc_devs_mux);
+
+	return stream_open(inode, file);
+}
+
+int ptp_dmtsc_release (struct inode *inode, struct file *file)
+{
+    struct ptp_dmtsc_cdev_info *cdev = file->private_data;
+
+    if (mutex_lock_interruptible(&cdev->pclock->dmtsc_devs.dmtsc_devs_mux))
+		return -ERESTARTSYS;
+    cdev->pclock->dmtsc_devs.readers--;
+
+    if ((cdev->pclock->dmtsc_devs.readers == 0) &&
+        cdev->pclock->dmtsc_devs.clean_request) {
+            mutex_unlock(&cdev->pclock->dmtsc_devs.dmtsc_devs_mux);
+            ptp_dmtsc_dev_uregister(cdev->pclock);
+        }
+    mutex_unlock(&cdev->pclock->dmtsc_devs.dmtsc_devs_mux);
+    return 0;
+}
+
+ssize_t ptp_dmtsc_read(struct file *file, char __user *buf,
+		      size_t cnt, loff_t *offset)
+{
+    struct ptp_dmtsc_cdev_info *cdev = file->private_data;
+	struct timestamp_event_queue *queue = &cdev->pclock->dmtscevq[cdev->minor];
+    struct mutex *dmtsceq_mux = &cdev->pclock->dmtsc_devs.cdev_info[cdev->minor].dmtsceq_mux;
+	struct ptp_extts_event *event;
+	unsigned long flags;
+	size_t qcnt, i;
+	int result;
+
+	if (cnt % sizeof(struct ptp_extts_event) != 0)
+		return -EINVAL;
+
+	if (cnt > EXTTS_BUFSIZE)
+		cnt = EXTTS_BUFSIZE;
+
+	cnt = cnt / sizeof(struct ptp_extts_event);
+
+	if (mutex_lock_interruptible(dmtsceq_mux))
+		return -ERESTARTSYS;
+
+	if (wait_event_interruptible(cdev->pclock->tsev_wq,
+				     cdev->pclock->defunct || queue_cnt(queue))) {
+		mutex_unlock(dmtsceq_mux);
+		return -ERESTARTSYS;
+	}
+
+	if (cdev->pclock->defunct) {
+		mutex_unlock(dmtsceq_mux);
+		return -ENODEV;
+	}
+
+	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
+	if (!event) {
+		mutex_unlock(dmtsceq_mux);
+		return -ENOMEM;
+	}
+
+	spin_lock_irqsave(&queue->lock, flags);
+
+	qcnt = queue_cnt(queue);
+
+	if (cnt > qcnt)
+		cnt = qcnt;
+
+	for (i = 0; i < cnt; i++) {
+		event[i] = queue->buf[queue->head];
+		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
+	}
+
+	spin_unlock_irqrestore(&queue->lock, flags);
+
+	cnt = cnt * sizeof(struct ptp_extts_event);
+
+	mutex_unlock(dmtsceq_mux);
+
+	result = cnt;
+	if (copy_to_user(buf, event, cnt))
+		result = -EFAULT;
+
+	kfree(event);
+	return result;
+}
+
+struct file_operations fops = {
+	.owner = THIS_MODULE,
+	.open = ptp_dmtsc_open,
+	.read = ptp_dmtsc_read,
+    .release = ptp_dmtsc_release
+};
+
+void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
+{
+    int idx, major;
+    dev_t device;
+
+    major = MAJOR(ptp->dmtsc_devs.devid);
+    for (idx = 0; idx < ptp->info->n_ext_ts ; idx++) {
+        if (ptp->dmtsc_devs.cdev_info[idx].minor >= 0) {
+            device = MKDEV(major, idx);
+            device_destroy(ptp->dmtsc_devs.dmtsc_class, device);
+            cdev_del(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev);
+            ptp->dmtsc_devs.cdev_info[idx].minor = -1;
+        }
+    }
+    class_destroy(ptp->dmtsc_devs.dmtsc_class);
+    unregister_chrdev_region(ptp->dmtsc_devs.devid, ptp->info->n_ext_ts);
+    mutex_destroy(&ptp->dmtsc_devs.cdev_info[idx].dmtsceq_mux);
+}
+
+int ptp_dmtsc_dev_register(struct ptp_clock *ptp)
+{
+    int err, idx, major;
+    dev_t device;
+    struct device *dev;
+
+    // Create fifos for all channels. The mask will control which of them get fed
+    ptp->dmtscevq = kcalloc(ptp->info->n_ext_ts, sizeof(*ptp->dmtscevq), GFP_KERNEL);
+    if (!ptp->dmtscevq) {
+        err = -EFAULT;
+        goto err;
+    }
+    ptp->dmtsc_devs.cdev_info = kcalloc(ptp->info->n_ext_ts,
+        sizeof(*ptp->dmtsc_devs.cdev_info), GFP_KERNEL);
+    if (!ptp->dmtsc_devs.cdev_info) {
+        err = -ENODEV;
+        goto fifo_clean;
+    }
+    for (idx = 0; idx < ptp->info->n_ext_ts ; idx++) {
+        ptp->dmtsc_devs.cdev_info[idx].minor = -1;
+    }
+    // Create devices for all channels. The mask will control which of them get fed
+    err = alloc_chrdev_region(&ptp->dmtsc_devs.devid, 0, ptp->info->n_ext_ts, "ptptsevqch");
+    if (!err) {
+        major = MAJOR(ptp->dmtsc_devs.devid);
+        ptp->dmtsc_devs.dmtsc_class = class_create(THIS_MODULE, "ptptsevqch_class");
+        for (idx = 0; idx < ptp->info->n_ext_ts ; idx++) {
+            mutex_init(&ptp->dmtsc_devs.cdev_info[idx].dmtsceq_mux);
+            device = MKDEV(major, idx);
+            ptp->dmtsc_devs.cdev_info[idx].pclock = ptp;
+            cdev_init(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev, &fops);
+            err = cdev_add(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev, device, 1);
+            if (err) {
+                goto cdev_clean;
+            } else {
+                ptp->dmtsc_devs.cdev_info[idx].minor = idx;
+                dev = device_create(ptp->dmtsc_devs.dmtsc_class, &ptp->dev, device, NULL, "ptp%dch%d", ptp->index, idx);
+                if (IS_ERR(dev)) {
+                    err = PTR_ERR(dev);
+                    goto cdev_clean;
+                }
+            }
+        }
+    } else {
+        goto dev_clean;
+    }
+    return 0;
+
+cdev_clean:
+    ptp_dmtsc_cdev_clean(ptp);
+dev_clean:
+    kfree(ptp->dmtsc_devs.cdev_info);
+    ptp->dmtsc_devs.cdev_info = NULL;
+fifo_clean:
+    kfree(ptp->dmtscevq);
+    ptp->dmtscevq = NULL;
+err:
+    return err;
+}
+
+void ptp_dmtsc_dev_uregister(struct ptp_clock *ptp)
+{
+    if (mutex_lock_interruptible(&ptp->dmtsc_devs.dmtsc_devs_mux))
+		return;
+    if (ptp->dmtsc_devs.readers > 0) {
+        ptp->dmtsc_devs.clean_request = true;
+        mutex_unlock(&ptp->dmtsc_devs.dmtsc_devs_mux);
+        return;
+    }
+    mutex_unlock(&ptp->dmtsc_devs.dmtsc_devs_mux);
+    ptp_dmtsc_cdev_clean(ptp);
+    kfree(ptp->dmtsc_devs.cdev_info);
+    ptp->dmtsc_devs.cdev_info = NULL;
+    kfree(ptp->dmtscevq);
+    ptp->dmtscevq = NULL;
+}
\ No newline at end of file
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 75f58fc468a7..9fc7b6ec6517 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -20,6 +20,8 @@
 #define PTP_BUF_TIMESTAMPS 30
 #define PTP_DEFAULT_MAX_VCLOCKS 20
 
+#define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
+
 struct timestamp_event_queue {
 	struct ptp_extts_event buf[PTP_MAX_TIMESTAMPS];
 	int head;
@@ -27,6 +29,22 @@ struct timestamp_event_queue {
 	spinlock_t lock;
 };
 
+struct ptp_dmtsc_cdev_info {
+	struct cdev dmtsc_cdev; /* Demuxed event device chardev */
+	int minor; /* Demuxed event queue chardev device minor */
+	struct ptp_clock *pclock; /* Direct access to parent clock device */
+	struct mutex dmtsceq_mux; /* Protect access to demuxed event queue */
+};
+
+struct ptp_dmtsc_dev_info {
+	dev_t devid;
+	int readers; /* Amount of users with chardev open */
+	bool clean_request; /* Signal userspace open chardev preventing safe device removal */
+	struct mutex dmtsc_devs_mux; /* Protect access to device management */
+	struct class *dmtsc_class;
+	struct ptp_dmtsc_cdev_info *cdev_info;
+};
+
 struct ptp_clock {
 	struct posix_clock clock;
 	struct device dev;
@@ -36,6 +54,10 @@ struct ptp_clock {
 	struct pps_device *pps_source;
 	long dialed_frequency; /* remembers the frequency adjustment */
 	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
+	u32 dmtsc_en_flags; /* Demultiplexed timestamp channels enable flags */
+	struct mutex dmtsc_sysfs_mux; /* Demultiplexed timestamp channels sysfs mutex */
+	struct timestamp_event_queue *dmtscevq; /* Demultiplexed timestamp channel fifos */
+	struct ptp_dmtsc_dev_info dmtsc_devs; /* Demultiplexed timestamp channel access character devices */
 	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;
@@ -139,4 +161,7 @@ void ptp_cleanup_pin_groups(struct ptp_clock *ptp);
 
 struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock);
 void ptp_vclock_unregister(struct ptp_vclock *vclock);
+
+int ptp_dmtsc_dev_register(struct ptp_clock *ptp);
+void ptp_dmtsc_dev_uregister(struct ptp_clock *ptp);
 #endif
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 6e4d5456a885..fc85aa1d3d23 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -316,6 +316,117 @@ static ssize_t max_vclocks_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(max_vclocks);
 
+static ssize_t dmtsc_en_flags_show(struct device *dev,
+			      struct device_attribute *attr, char *page)
+{
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+	ssize_t size;
+
+	size = snprintf(page, PAGE_SIZE - 1, "0x%X\n", ptp->dmtsc_en_flags);
+
+	return size;
+}
+
+static ssize_t dmtsc_en_flags_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+	int err = -EINVAL;
+	char mode, val[7];
+	int enable, channel, cnt;
+	u32 req_mask, new_mask;
+
+	/* Read mode, value and enable
+	    - mode: "m" - Channel mask mode
+		        "c" - Single channel mode
+		- value: channel mask in hex or channel number in decimal
+			lsb = channel 0
+		- enable: '1' - enable
+				  '0' - disable
+	*/
+	cnt = sscanf(buf, "%c %s %d", &mode, val, &enable);
+	if (cnt != 3)
+		return err;
+
+	if (mutex_lock_interruptible(&ptp->dmtsc_sysfs_mux))
+		return -ERESTARTSYS;
+
+	switch (mode) {
+		case 'm':
+			if (kstrtou32(val, 0, &req_mask))
+			{
+				dev_info(dev, "dmtscevq invalid arguments");
+				goto out;
+			}
+			break;
+		case 'c':
+			if (kstrtoint(val, 0, &channel)) {
+				dev_info(dev, "dmtscevq invalid channel number");
+				goto out;
+			}
+			if ((channel < 0) || (channel > 31))
+			{
+				dev_info(dev, "dmtscevq channel number out of range");
+				goto out;
+			}
+			req_mask = (0x1 << channel);
+			break;
+		default:
+			goto out;
+			break;
+	}
+
+	switch (enable) {
+		case 0:
+			new_mask = ptp->dmtsc_en_flags & ~req_mask;
+			break;
+		case 1:
+			new_mask = ptp->dmtsc_en_flags | req_mask;
+			break;
+		default:
+			dev_info(dev, "dmtscevq invalid enable value");
+			break;
+	}
+
+	if (new_mask == 0x0) {
+		// All queues disabled. Remove all character devices.
+		if (ptp->dmtscevq != NULL) {
+			ptp_dmtsc_dev_uregister(ptp);
+		} else {
+			if (ptp->dmtsc_en_flags != 0x0) {
+				dev_info(dev, "dmtscevq. Unexpected error: TSEVQ exploded buffers presumed unitialized. Skipping.");
+			}
+		}
+	} else {
+		// At least une queue enabled. Create all character devices.
+		// The mask will feed the selected character device and keep others inactive.
+		ptp->dmtsc_devs.readers = 0;
+		ptp->dmtsc_devs.clean_request = false;
+		if (ptp->dmtscevq == NULL) {
+			err = ptp_dmtsc_dev_register(ptp);
+			if (err != 0) {
+				dev_info(dev, "dmtscevq. Error while trying to register exploded queues");
+				goto out;
+			}
+		} else {
+			if (ptp->dmtsc_en_flags == 0x0) {
+				dev_info(dev, "dmtscevq. Unexpected error: TSEVQ exploded buffers already initialized, skipping initialization.");
+			}
+		}
+
+	}
+
+	ptp->dmtsc_en_flags = new_mask;
+
+	mutex_unlock(&ptp->dmtsc_sysfs_mux);
+	return count;
+out:
+	mutex_unlock(&ptp->dmtsc_sysfs_mux);
+	return err;
+}
+static DEVICE_ATTR_RW(dmtsc_en_flags);
+
 static struct attribute *ptp_attrs[] = {
 	&dev_attr_clock_name.attr,
 
@@ -333,6 +444,8 @@ static struct attribute *ptp_attrs[] = {
 	&dev_attr_pps_enable.attr,
 	&dev_attr_n_vclocks.attr,
 	&dev_attr_max_vclocks.attr,
+
+	&dev_attr_dmtsc_en_flags.attr,
 	NULL
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-29 11:47                     ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
@ 2023-08-29 14:07                       ` Richard Cochran
  2023-08-29 14:15                         ` Richard Cochran
  2023-08-30 21:41                         ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Xabier Marquiegui
  0 siblings, 2 replies; 26+ messages in thread
From: Richard Cochran @ 2023-08-29 14:07 UTC (permalink / raw)
  To: Xabier Marquiegui; +Cc: chrony-dev, mlichvar, netdev, ntp-lists

On Tue, Aug 29, 2023 at 01:47:52PM +0200, Xabier Marquiegui wrote:
> Add the posibility to demultiplex the timestamp channels for
> external timestamp event channels.
> 
> In some applications it can be necessary to have different
> consumers for different timestamp channels. For example,
> synchronize to an external pps source with linuxptp ts2phc
> while timestmping external events with another application.
> 
> This change proposes the dynamic creation of one char-device
> per timestamp channel only if the user requests the demuxing
> of timestamp channels. It allows for on-the-fly demuxing of
> specific channels.

No need to make complex configuration to enable this.  Just make one
queue per open character device, and one for sysfs.

> The operation can be controlled via sysfs. See file
> Documentation/ABI/testing/sysfs-ptp for more details.

No need for new sysfs hooks.

> ---
>  Documentation/ABI/testing/sysfs-ptp |  16 +++
>  MAINTAINERS                         |   5 +
>  drivers/ptp/Makefile                |   2 +-
>  drivers/ptp/ptp_chardev.c           |   2 -
>  drivers/ptp/ptp_clock.c             |  22 ++-
>  drivers/ptp/ptp_demuxtschan.c       | 211 ++++++++++++++++++++++++++++

No need to add a second char dev implementation.
Just change the existing one to have a per-file queue.

General comment: Lots of coding style violations here.
See CodingStyle and use scripts/checkpatch.pl

Thanks,
Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-29 14:07                       ` Richard Cochran
@ 2023-08-29 14:15                         ` Richard Cochran
  2023-08-30 21:41                         ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Xabier Marquiegui
  1 sibling, 0 replies; 26+ messages in thread
From: Richard Cochran @ 2023-08-29 14:15 UTC (permalink / raw)
  To: Xabier Marquiegui; +Cc: chrony-dev, mlichvar, netdev, ntp-lists

On Tue, Aug 29, 2023 at 07:07:57AM -0700, Richard Cochran wrote:

> > The operation can be controlled via sysfs. See file
> > Documentation/ABI/testing/sysfs-ptp for more details.
> 
> No need for new sysfs hooks.

Just add a ioctl that adds a filter to an open file.
The filter can be a simple bit mask of say, 1024 bits.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [chrony-dev] Support for Multiple PPS Inputs on single PHC
  2023-08-29 14:07                       ` Richard Cochran
  2023-08-29 14:15                         ` Richard Cochran
@ 2023-08-30 21:41                         ` Xabier Marquiegui
  2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
  1 sibling, 1 reply; 26+ messages in thread
From: Xabier Marquiegui @ 2023-08-30 21:41 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax


Thank you very much for your prompt and kind answer. I appreciate you
taking the time to help me learn and improve my contributions.

I have now tweaked my proposal. You are absolutely right that the whole
thing is simpler and more intuitive without the sysfs control structure.

I have also removed redundant code. I hope it's better now. Let me know
what you think.

Thank you very much,
Xabier.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-30 21:41                         ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Xabier Marquiegui
@ 2023-08-30 21:41                           ` Xabier Marquiegui
  2023-08-30 22:01                             ` Richard Cochran
                                               ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Xabier Marquiegui @ 2023-08-30 21:41 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

Add the posibility to demultiplex the timestamp channels for
external timestamp event channels.

In some applications it can be necessary to have different
consumers for different timestamp channels. For example,
synchronize to an external pps source with linuxptp ts2phc
while timestmping external events with another application.

This commit maintains the original multiplexed queue and adds an
individual queue per external timestamp channel. All enabled channels
will be directed to the multiplexed queue by default. On file open, a
specific channel will be redirected to the dedicated char device, and on
close it will automatically go back to the multiplexed queue.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
---
 drivers/ptp/ptp_chardev.c | 167 +++++++++++++++++++++++++++++++++++---
 drivers/ptp/ptp_clock.c   |  43 +++++++---
 drivers/ptp/ptp_private.h |  22 +++++
 3 files changed, 210 insertions(+), 22 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 362bf756e6b7..c31cfc5b0907 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -10,11 +10,14 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
-
+#include <linux/cdev.h>
+#include <linux/fs.h>
 #include <linux/nospec.h>
 
 #include "ptp_private.h"
 
+#define DMTSC_NOT -1
+
 static int ptp_disable_pinfunc(struct ptp_clock_info *ops,
 			       enum ptp_pin_function func, unsigned int chan)
 {
@@ -443,16 +446,24 @@ __poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
 
 #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_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
+		       int dmtsc)
 {
-	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
-	struct timestamp_event_queue *queue = &ptp->tsevq;
+	struct timestamp_event_queue *queue;
+	struct mutex *tsevq_mux;
 	struct ptp_extts_event *event;
 	unsigned long flags;
 	size_t qcnt, i;
 	int result;
 
+	if (dmtsc < 0) {
+		queue = &ptp->tsevq;
+		tsevq_mux = &ptp->tsevq_mux;
+	} else {
+		queue = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq;
+		tsevq_mux = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq_mux;
+	}
+
 	if (cnt % sizeof(struct ptp_extts_event) != 0)
 		return -EINVAL;
 
@@ -461,23 +472,23 @@ ssize_t ptp_read(struct posix_clock *pc,
 
 	cnt = cnt / sizeof(struct ptp_extts_event);
 
-	if (mutex_lock_interruptible(&ptp->tsevq_mux))
+	if (mutex_lock_interruptible(tsevq_mux))
 		return -ERESTARTSYS;
 
 	if (wait_event_interruptible(ptp->tsev_wq,
 				     ptp->defunct || queue_cnt(queue))) {
-		mutex_unlock(&ptp->tsevq_mux);
+		mutex_unlock(tsevq_mux);
 		return -ERESTARTSYS;
 	}
 
 	if (ptp->defunct) {
-		mutex_unlock(&ptp->tsevq_mux);
+		mutex_unlock(tsevq_mux);
 		return -ENODEV;
 	}
 
 	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
 	if (!event) {
-		mutex_unlock(&ptp->tsevq_mux);
+		mutex_unlock(tsevq_mux);
 		return -ENOMEM;
 	}
 
@@ -497,7 +508,7 @@ ssize_t ptp_read(struct posix_clock *pc,
 
 	cnt = cnt * sizeof(struct ptp_extts_event);
 
-	mutex_unlock(&ptp->tsevq_mux);
+	mutex_unlock(tsevq_mux);
 
 	result = cnt;
 	if (copy_to_user(buf, event, cnt))
@@ -506,3 +517,139 @@ ssize_t ptp_read(struct posix_clock *pc,
 	kfree(event);
 	return result;
 }
+
+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);
+
+	return ptp_queue_read(ptp, buf, cnt, DMTSC_NOT);
+}
+
+static int ptp_dmtsc_open(struct inode *inode, struct file *file)
+{
+	struct ptp_dmtsc_cdev_info *cdev = container_of(
+		inode->i_cdev, struct ptp_dmtsc_cdev_info, dmtsc_cdev);
+
+	file->private_data = cdev;
+
+	if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
+		return -ERESTARTSYS;
+	cdev->pclock->dmtsc_en_flags |= (0x1 << (cdev->minor));
+	mutex_unlock(&cdev->pclock->dmtsc_en_mux);
+
+	return stream_open(inode, file);
+}
+
+int ptp_dmtsc_release(struct inode *inode, struct file *file)
+{
+	struct ptp_dmtsc_cdev_info *cdev = file->private_data;
+
+	if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
+		return -ERESTARTSYS;
+	cdev->pclock->dmtsc_en_flags &= ~(0x1 << (cdev->minor));
+	mutex_unlock(&cdev->pclock->dmtsc_en_mux);
+
+	return 0;
+}
+
+ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
+		       loff_t *offset)
+{
+	struct ptp_dmtsc_cdev_info *cdev = file->private_data;
+
+	return ptp_queue_read(cdev->pclock, buf, cnt, cdev->minor);
+}
+
+static const struct file_operations fops = {
+						.owner = THIS_MODULE,
+						.open = ptp_dmtsc_open,
+						.read = ptp_dmtsc_read,
+						.release = ptp_dmtsc_release
+						};
+
+void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
+{
+	int idx, major;
+	dev_t device;
+
+	major = MAJOR(ptp->dmtsc_devs.devid);
+	for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
+		if (ptp->dmtsc_devs.cdev_info[idx].minor >= 0) {
+			device = MKDEV(major, idx);
+			device_destroy(ptp->dmtsc_devs.dmtsc_class, device);
+			cdev_del(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev);
+			ptp->dmtsc_devs.cdev_info[idx].minor = -1;
+		}
+	}
+	class_destroy(ptp->dmtsc_devs.dmtsc_class);
+	unregister_chrdev_region(ptp->dmtsc_devs.devid, ptp->info->n_ext_ts);
+	mutex_destroy(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
+}
+
+int ptp_dmtsc_dev_register(struct ptp_clock *ptp)
+{
+	int err, idx, major;
+	dev_t device;
+	struct device *dev;
+
+	/* Allocate memory for demuxed device management */
+	ptp->dmtsc_devs.cdev_info = kcalloc(ptp->info->n_ext_ts,
+					    sizeof(*ptp->dmtsc_devs.cdev_info),
+					    GFP_KERNEL);
+	if (!ptp->dmtsc_devs.cdev_info) {
+		err = -ENODEV;
+		goto err;
+	}
+	for (idx = 0; idx < ptp->info->n_ext_ts; idx++)
+		ptp->dmtsc_devs.cdev_info[idx].minor = -1;
+	/* Create devices for all channels. The mask will control which of them get fed */
+	err = alloc_chrdev_region(&ptp->dmtsc_devs.devid, 0,
+				  ptp->info->n_ext_ts, "ptptsevqch");
+	if (!err) {
+		major = MAJOR(ptp->dmtsc_devs.devid);
+		ptp->dmtsc_devs.dmtsc_class =
+			class_create(THIS_MODULE, "ptptsevqch_class");
+		for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
+			mutex_init(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
+			device = MKDEV(major, idx);
+			ptp->dmtsc_devs.cdev_info[idx].pclock = ptp;
+			cdev_init(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev,
+				  &fops);
+			err = cdev_add(
+				&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev,
+				device, 1);
+			if (err) {
+				goto cdev_clean;
+			} else {
+				ptp->dmtsc_devs.cdev_info[idx].minor = idx;
+				dev = device_create(ptp->dmtsc_devs.dmtsc_class,
+						    &ptp->dev, device, NULL,
+						    "ptp%dch%d", ptp->index,
+						    idx);
+				if (IS_ERR(dev)) {
+					err = PTR_ERR(dev);
+					goto cdev_clean;
+				}
+			}
+		}
+	} else {
+		goto dev_clean;
+	}
+	return 0;
+
+cdev_clean:
+	ptp_dmtsc_cdev_clean(ptp);
+dev_clean:
+	kfree(ptp->dmtsc_devs.cdev_info);
+	ptp->dmtsc_devs.cdev_info = NULL;
+err:
+	return err;
+}
+
+void ptp_dmtsc_dev_uregister(struct ptp_clock *ptp)
+{
+	ptp_dmtsc_cdev_clean(ptp);
+	kfree(ptp->dmtsc_devs.cdev_info);
+	ptp->dmtsc_devs.cdev_info = NULL;
+}
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 80f74e38c2da..0a42c27c3514 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -172,6 +172,7 @@ static void ptp_clock_release(struct device *dev)
 
 	ptp_cleanup_pin_groups(ptp);
 	kfree(ptp->vclock_index);
+	mutex_destroy(&ptp->dmtsc_en_mux);
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
@@ -232,7 +233,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->tsevq_mux);
 	mutex_init(&ptp->pincfg_mux);
 	mutex_init(&ptp->n_vclocks_mux);
+	mutex_init(&ptp->dmtsc_en_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
+	ptp->dmtsc_en_flags = 0x0;
 
 	if (ptp->info->getcycles64 || ptp->info->getcyclesx64) {
 		ptp->has_cycles = true;
@@ -307,21 +310,27 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 
 	/* Create a posix clock and link it to the device. */
 	err = posix_clock_register(&ptp->clock, &ptp->dev);
-	if (err) {
-		if (ptp->pps_source)
-			pps_unregister_source(ptp->pps_source);
+	if (err)
+		goto reg_err;
 
-		if (ptp->kworker)
-			kthread_destroy_worker(ptp->kworker);
+	/* Create chardevs for demuxed external timestamp channels */
+	if (ptp_dmtsc_dev_register(ptp))
+		goto reg_err;
 
-		put_device(&ptp->dev);
+	return ptp;
 
-		pr_err("failed to create posix clock\n");
-		return ERR_PTR(err);
-	}
+reg_err:
+	ptp_dmtsc_dev_uregister(ptp);
+	if (ptp->pps_source)
+		pps_unregister_source(ptp->pps_source);
 
-	return ptp;
+	if (ptp->kworker)
+		kthread_destroy_worker(ptp->kworker);
+
+	put_device(&ptp->dev);
 
+	pr_err("failed to create posix clock\n");
+	return ERR_PTR(err);
 no_pps:
 	ptp_cleanup_pin_groups(ptp);
 no_pin_groups:
@@ -330,6 +339,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (ptp->kworker)
 		kthread_destroy_worker(ptp->kworker);
 kworker_err:
+	mutex_destroy(&ptp->dmtsc_en_mux);
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	mutex_destroy(&ptp->n_vclocks_mux);
@@ -367,6 +377,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
 
+	ptp_dmtsc_dev_uregister(ptp);
+
 	posix_clock_unregister(&ptp->clock);
 
 	return 0;
@@ -378,12 +390,19 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
 	struct pps_event_time evt;
 
 	switch (event->type) {
-
 	case PTP_CLOCK_ALARM:
 		break;
 
 	case PTP_CLOCK_EXTTS:
-		enqueue_external_timestamp(&ptp->tsevq, event);
+		/* If event index demuxed queue mask is enabled send to dedicated fifo */
+		if (ptp->dmtsc_en_flags & (0x1 << event->index)) {
+			enqueue_external_timestamp(
+				&ptp->dmtsc_devs.cdev_info[event->index].tsevq,
+				event);
+		} else {
+			enqueue_external_timestamp(&ptp->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..c473ef75d8d7 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -27,6 +27,20 @@ struct timestamp_event_queue {
 	spinlock_t lock;
 };
 
+struct ptp_dmtsc_cdev_info {
+	struct cdev dmtsc_cdev; /* Demuxed event device chardev */
+	int minor; /* Demuxed event queue chardev device minor */
+	struct ptp_clock *pclock; /* Direct access to parent clock device */
+	struct mutex tsevq_mux; /* Protect access to device management */
+	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
+};
+
+struct ptp_dmtsc_dev_info {
+	dev_t devid;
+	struct class *dmtsc_class;
+	struct ptp_dmtsc_cdev_info *cdev_info;
+};
+
 struct ptp_clock {
 	struct posix_clock clock;
 	struct device dev;
@@ -36,6 +50,11 @@ struct ptp_clock {
 	struct pps_device *pps_source;
 	long dialed_frequency; /* remembers the frequency adjustment */
 	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
+	u32 dmtsc_en_flags; /* Demultiplexed timestamp channels enable flags */
+	struct mutex
+		dmtsc_en_mux; /* Demultiplexed timestamp channels sysfs mutex */
+	struct ptp_dmtsc_dev_info
+		dmtsc_devs; /* Demultiplexed timestamp channel access character devices */
 	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;
@@ -139,4 +158,7 @@ void ptp_cleanup_pin_groups(struct ptp_clock *ptp);
 
 struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock);
 void ptp_vclock_unregister(struct ptp_vclock *vclock);
+
+int ptp_dmtsc_dev_register(struct ptp_clock *ptp);
+void ptp_dmtsc_dev_uregister(struct ptp_clock *ptp);
 #endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
@ 2023-08-30 22:01                             ` Richard Cochran
  2023-09-06 10:47                               ` Xabier Marquiegui
  2023-08-31  0:29                             ` [PATCH] ptp: Demultiplexed timestamp channels kernel test robot
                                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 26+ messages in thread
From: Richard Cochran @ 2023-08-30 22:01 UTC (permalink / raw)
  To: Xabier Marquiegui; +Cc: chrony-dev, mlichvar, netdev, ntp-lists

On Wed, Aug 30, 2023 at 11:41:01PM +0200, Xabier Marquiegui wrote:

> +ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
> +		       loff_t *offset)
> +{
> +	struct ptp_dmtsc_cdev_info *cdev = file->private_data;
> +
> +	return ptp_queue_read(cdev->pclock, buf, cnt, cdev->minor);
> +}
> +
> +static const struct file_operations fops = {
> +						.owner = THIS_MODULE,
> +						.open = ptp_dmtsc_open,
> +						.read = ptp_dmtsc_read,
> +						.release = ptp_dmtsc_release
> +						};

I'll say it again... There is no need to add a new char dev!

You will need a patch series of at least three patches, step by step:

1. Delete ptp_clock.tsevq
   Replace it will a linked list of `struct timestamp_event_queue`
   Populate the list with ONE queue in ptp_clock_register()

   This will be your first patch.
   It will have the same functionality as before.

2. Allocate a new `struct timestamp_event_queue` on posix_clock_operations.open()
   Add it into the list.
   Remove it again on posix_clock_operations.release()

   This will introduce new functionality, as all events will go to all
   consumers.

3. Introduce a new ioctl that filters events based on user preference.

Hm?

Thanks,
Richard


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
  2023-08-30 22:01                             ` Richard Cochran
@ 2023-08-31  0:29                             ` kernel test robot
  2023-08-31 13:28                             ` kernel test robot
  2023-08-31 16:20                             ` kernel test robot
  3 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2023-08-31  0:29 UTC (permalink / raw)
  To: Xabier Marquiegui, richardcochran
  Cc: oe-kbuild-all, chrony-dev, mlichvar, netdev, ntp-lists, reibax

Hi Xabier,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.5 next-20230830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xabier-Marquiegui/ptp-Demultiplexed-timestamp-channels/20230831-054428
base:   net/main
patch link:    https://lore.kernel.org/r/20230830214101.509086-2-reibax%40gmail.com
patch subject: [PATCH] ptp: Demultiplexed timestamp channels
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230831/202308310809.wcvhamyw-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308310809.wcvhamyw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308310809.wcvhamyw-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/ptp/ptp_chardev.c:449:9: warning: no previous prototype for 'ptp_queue_read' [-Wmissing-prototypes]
     449 | ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
         |         ^~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:544:5: warning: no previous prototype for 'ptp_dmtsc_release' [-Wmissing-prototypes]
     544 | int ptp_dmtsc_release(struct inode *inode, struct file *file)
         |     ^~~~~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:556:9: warning: no previous prototype for 'ptp_dmtsc_read' [-Wmissing-prototypes]
     556 | ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
         |         ^~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:571:6: warning: no previous prototype for 'ptp_dmtsc_cdev_clean' [-Wmissing-prototypes]
     571 | void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
         |      ^~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/linkage.h:7,
                    from include/linux/preempt.h:10,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:17,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:7,
                    from include/linux/mm_types_task.h:13,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/ptp/ptp_chardev.c:7:
   drivers/ptp/ptp_chardev.c: In function 'ptp_dmtsc_dev_register':
   include/linux/export.h:31:21: error: passing argument 1 of 'class_create' from incompatible pointer type [-Werror=incompatible-pointer-types]
      31 | #define THIS_MODULE ((struct module *)0)
         |                     ^~~~~~~~~~~~~~~~~~~~
         |                     |
         |                     struct module *
   drivers/ptp/ptp_chardev.c:612:38: note: in expansion of macro 'THIS_MODULE'
     612 |                         class_create(THIS_MODULE, "ptptsevqch_class");
         |                                      ^~~~~~~~~~~
   In file included from include/linux/device.h:31,
                    from include/linux/cdev.h:8,
                    from include/linux/posix-clock.h:10,
                    from drivers/ptp/ptp_chardev.c:8:
   include/linux/device/class.h:230:54: note: expected 'const char *' but argument is of type 'struct module *'
     230 | struct class * __must_check class_create(const char *name);
         |                                          ~~~~~~~~~~~~^~~~
   drivers/ptp/ptp_chardev.c:612:25: error: too many arguments to function 'class_create'
     612 |                         class_create(THIS_MODULE, "ptptsevqch_class");
         |                         ^~~~~~~~~~~~
   include/linux/device/class.h:230:29: note: declared here
     230 | struct class * __must_check class_create(const char *name);
         |                             ^~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/ptp_queue_read +449 drivers/ptp/ptp_chardev.c

   448	
 > 449	ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
   450			       int dmtsc)
   451	{
   452		struct timestamp_event_queue *queue;
   453		struct mutex *tsevq_mux;
   454		struct ptp_extts_event *event;
   455		unsigned long flags;
   456		size_t qcnt, i;
   457		int result;
   458	
   459		if (dmtsc < 0) {
   460			queue = &ptp->tsevq;
   461			tsevq_mux = &ptp->tsevq_mux;
   462		} else {
   463			queue = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq;
   464			tsevq_mux = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq_mux;
   465		}
   466	
   467		if (cnt % sizeof(struct ptp_extts_event) != 0)
   468			return -EINVAL;
   469	
   470		if (cnt > EXTTS_BUFSIZE)
   471			cnt = EXTTS_BUFSIZE;
   472	
   473		cnt = cnt / sizeof(struct ptp_extts_event);
   474	
   475		if (mutex_lock_interruptible(tsevq_mux))
   476			return -ERESTARTSYS;
   477	
   478		if (wait_event_interruptible(ptp->tsev_wq,
   479					     ptp->defunct || queue_cnt(queue))) {
   480			mutex_unlock(tsevq_mux);
   481			return -ERESTARTSYS;
   482		}
   483	
   484		if (ptp->defunct) {
   485			mutex_unlock(tsevq_mux);
   486			return -ENODEV;
   487		}
   488	
   489		event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
   490		if (!event) {
   491			mutex_unlock(tsevq_mux);
   492			return -ENOMEM;
   493		}
   494	
   495		spin_lock_irqsave(&queue->lock, flags);
   496	
   497		qcnt = queue_cnt(queue);
   498	
   499		if (cnt > qcnt)
   500			cnt = qcnt;
   501	
   502		for (i = 0; i < cnt; i++) {
   503			event[i] = queue->buf[queue->head];
   504			queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
   505		}
   506	
   507		spin_unlock_irqrestore(&queue->lock, flags);
   508	
   509		cnt = cnt * sizeof(struct ptp_extts_event);
   510	
   511		mutex_unlock(tsevq_mux);
   512	
   513		result = cnt;
   514		if (copy_to_user(buf, event, cnt))
   515			result = -EFAULT;
   516	
   517		kfree(event);
   518		return result;
   519	}
   520	
   521	ssize_t ptp_read(struct posix_clock *pc, uint rdflags, char __user *buf,
   522			 size_t cnt)
   523	{
   524		struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
   525	
   526		return ptp_queue_read(ptp, buf, cnt, DMTSC_NOT);
   527	}
   528	
   529	static int ptp_dmtsc_open(struct inode *inode, struct file *file)
   530	{
   531		struct ptp_dmtsc_cdev_info *cdev = container_of(
   532			inode->i_cdev, struct ptp_dmtsc_cdev_info, dmtsc_cdev);
   533	
   534		file->private_data = cdev;
   535	
   536		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   537			return -ERESTARTSYS;
   538		cdev->pclock->dmtsc_en_flags |= (0x1 << (cdev->minor));
   539		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   540	
   541		return stream_open(inode, file);
   542	}
   543	
 > 544	int ptp_dmtsc_release(struct inode *inode, struct file *file)
   545	{
   546		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   547	
   548		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   549			return -ERESTARTSYS;
   550		cdev->pclock->dmtsc_en_flags &= ~(0x1 << (cdev->minor));
   551		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   552	
   553		return 0;
   554	}
   555	
 > 556	ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
   557			       loff_t *offset)
   558	{
   559		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   560	
   561		return ptp_queue_read(cdev->pclock, buf, cnt, cdev->minor);
   562	}
   563	
   564	static const struct file_operations fops = {
   565							.owner = THIS_MODULE,
   566							.open = ptp_dmtsc_open,
   567							.read = ptp_dmtsc_read,
   568							.release = ptp_dmtsc_release
   569							};
   570	
 > 571	void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
   572	{
   573		int idx, major;
   574		dev_t device;
   575	
   576		major = MAJOR(ptp->dmtsc_devs.devid);
   577		for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
   578			if (ptp->dmtsc_devs.cdev_info[idx].minor >= 0) {
   579				device = MKDEV(major, idx);
   580				device_destroy(ptp->dmtsc_devs.dmtsc_class, device);
   581				cdev_del(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev);
   582				ptp->dmtsc_devs.cdev_info[idx].minor = -1;
   583			}
   584		}
   585		class_destroy(ptp->dmtsc_devs.dmtsc_class);
   586		unregister_chrdev_region(ptp->dmtsc_devs.devid, ptp->info->n_ext_ts);
   587		mutex_destroy(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
   588	}
   589	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
  2023-08-30 22:01                             ` Richard Cochran
  2023-08-31  0:29                             ` [PATCH] ptp: Demultiplexed timestamp channels kernel test robot
@ 2023-08-31 13:28                             ` kernel test robot
  2023-08-31 16:20                             ` kernel test robot
  3 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2023-08-31 13:28 UTC (permalink / raw)
  To: Xabier Marquiegui, richardcochran
  Cc: llvm, oe-kbuild-all, chrony-dev, mlichvar, netdev, ntp-lists, reibax

Hi Xabier,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]
[also build test ERROR on net-next/main linus/master horms-ipvs/master v6.5 next-20230831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xabier-Marquiegui/ptp-Demultiplexed-timestamp-channels/20230831-054428
base:   net/main
patch link:    https://lore.kernel.org/r/20230830214101.509086-2-reibax%40gmail.com
patch subject: [PATCH] ptp: Demultiplexed timestamp channels
config: powerpc64-randconfig-r026-20230831 (https://download.01.org/0day-ci/archive/20230831/202308312155.cA1uQaGm-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308312155.cA1uQaGm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308312155.cA1uQaGm-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/ptp/ptp_chardev.c:449:9: warning: no previous prototype for function 'ptp_queue_read' [-Wmissing-prototypes]
     449 | ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
         |         ^
   drivers/ptp/ptp_chardev.c:449:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     449 | ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
         | ^
         | static 
>> drivers/ptp/ptp_chardev.c:544:5: warning: no previous prototype for function 'ptp_dmtsc_release' [-Wmissing-prototypes]
     544 | int ptp_dmtsc_release(struct inode *inode, struct file *file)
         |     ^
   drivers/ptp/ptp_chardev.c:544:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     544 | int ptp_dmtsc_release(struct inode *inode, struct file *file)
         | ^
         | static 
>> drivers/ptp/ptp_chardev.c:556:9: warning: no previous prototype for function 'ptp_dmtsc_read' [-Wmissing-prototypes]
     556 | ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
         |         ^
   drivers/ptp/ptp_chardev.c:556:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     556 | ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
         | ^
         | static 
>> drivers/ptp/ptp_chardev.c:571:6: warning: no previous prototype for function 'ptp_dmtsc_cdev_clean' [-Wmissing-prototypes]
     571 | void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
         |      ^
   drivers/ptp/ptp_chardev.c:571:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     571 | void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
         | ^
         | static 
>> drivers/ptp/ptp_chardev.c:612:30: error: too many arguments to function call, expected single argument 'name', have 2 arguments
     612 |                         class_create(THIS_MODULE, "ptptsevqch_class");
         |                         ~~~~~~~~~~~~              ^~~~~~~~~~~~~~~~~~
   include/linux/device/class.h:230:29: note: 'class_create' declared here
     230 | struct class * __must_check class_create(const char *name);
         |                             ^
   4 warnings and 1 error generated.


vim +/name +612 drivers/ptp/ptp_chardev.c

   448	
 > 449	ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
   450			       int dmtsc)
   451	{
   452		struct timestamp_event_queue *queue;
   453		struct mutex *tsevq_mux;
   454		struct ptp_extts_event *event;
   455		unsigned long flags;
   456		size_t qcnt, i;
   457		int result;
   458	
   459		if (dmtsc < 0) {
   460			queue = &ptp->tsevq;
   461			tsevq_mux = &ptp->tsevq_mux;
   462		} else {
   463			queue = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq;
   464			tsevq_mux = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq_mux;
   465		}
   466	
   467		if (cnt % sizeof(struct ptp_extts_event) != 0)
   468			return -EINVAL;
   469	
   470		if (cnt > EXTTS_BUFSIZE)
   471			cnt = EXTTS_BUFSIZE;
   472	
   473		cnt = cnt / sizeof(struct ptp_extts_event);
   474	
   475		if (mutex_lock_interruptible(tsevq_mux))
   476			return -ERESTARTSYS;
   477	
   478		if (wait_event_interruptible(ptp->tsev_wq,
   479					     ptp->defunct || queue_cnt(queue))) {
   480			mutex_unlock(tsevq_mux);
   481			return -ERESTARTSYS;
   482		}
   483	
   484		if (ptp->defunct) {
   485			mutex_unlock(tsevq_mux);
   486			return -ENODEV;
   487		}
   488	
   489		event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
   490		if (!event) {
   491			mutex_unlock(tsevq_mux);
   492			return -ENOMEM;
   493		}
   494	
   495		spin_lock_irqsave(&queue->lock, flags);
   496	
   497		qcnt = queue_cnt(queue);
   498	
   499		if (cnt > qcnt)
   500			cnt = qcnt;
   501	
   502		for (i = 0; i < cnt; i++) {
   503			event[i] = queue->buf[queue->head];
   504			queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
   505		}
   506	
   507		spin_unlock_irqrestore(&queue->lock, flags);
   508	
   509		cnt = cnt * sizeof(struct ptp_extts_event);
   510	
   511		mutex_unlock(tsevq_mux);
   512	
   513		result = cnt;
   514		if (copy_to_user(buf, event, cnt))
   515			result = -EFAULT;
   516	
   517		kfree(event);
   518		return result;
   519	}
   520	
   521	ssize_t ptp_read(struct posix_clock *pc, uint rdflags, char __user *buf,
   522			 size_t cnt)
   523	{
   524		struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
   525	
   526		return ptp_queue_read(ptp, buf, cnt, DMTSC_NOT);
   527	}
   528	
   529	static int ptp_dmtsc_open(struct inode *inode, struct file *file)
   530	{
   531		struct ptp_dmtsc_cdev_info *cdev = container_of(
   532			inode->i_cdev, struct ptp_dmtsc_cdev_info, dmtsc_cdev);
   533	
   534		file->private_data = cdev;
   535	
   536		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   537			return -ERESTARTSYS;
   538		cdev->pclock->dmtsc_en_flags |= (0x1 << (cdev->minor));
   539		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   540	
   541		return stream_open(inode, file);
   542	}
   543	
 > 544	int ptp_dmtsc_release(struct inode *inode, struct file *file)
   545	{
   546		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   547	
   548		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   549			return -ERESTARTSYS;
   550		cdev->pclock->dmtsc_en_flags &= ~(0x1 << (cdev->minor));
   551		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   552	
   553		return 0;
   554	}
   555	
 > 556	ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
   557			       loff_t *offset)
   558	{
   559		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   560	
   561		return ptp_queue_read(cdev->pclock, buf, cnt, cdev->minor);
   562	}
   563	
   564	static const struct file_operations fops = {
   565							.owner = THIS_MODULE,
   566							.open = ptp_dmtsc_open,
   567							.read = ptp_dmtsc_read,
   568							.release = ptp_dmtsc_release
   569							};
   570	
 > 571	void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
   572	{
   573		int idx, major;
   574		dev_t device;
   575	
   576		major = MAJOR(ptp->dmtsc_devs.devid);
   577		for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
   578			if (ptp->dmtsc_devs.cdev_info[idx].minor >= 0) {
   579				device = MKDEV(major, idx);
   580				device_destroy(ptp->dmtsc_devs.dmtsc_class, device);
   581				cdev_del(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev);
   582				ptp->dmtsc_devs.cdev_info[idx].minor = -1;
   583			}
   584		}
   585		class_destroy(ptp->dmtsc_devs.dmtsc_class);
   586		unregister_chrdev_region(ptp->dmtsc_devs.devid, ptp->info->n_ext_ts);
   587		mutex_destroy(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
   588	}
   589	
   590	int ptp_dmtsc_dev_register(struct ptp_clock *ptp)
   591	{
   592		int err, idx, major;
   593		dev_t device;
   594		struct device *dev;
   595	
   596		/* Allocate memory for demuxed device management */
   597		ptp->dmtsc_devs.cdev_info = kcalloc(ptp->info->n_ext_ts,
   598						    sizeof(*ptp->dmtsc_devs.cdev_info),
   599						    GFP_KERNEL);
   600		if (!ptp->dmtsc_devs.cdev_info) {
   601			err = -ENODEV;
   602			goto err;
   603		}
   604		for (idx = 0; idx < ptp->info->n_ext_ts; idx++)
   605			ptp->dmtsc_devs.cdev_info[idx].minor = -1;
   606		/* Create devices for all channels. The mask will control which of them get fed */
   607		err = alloc_chrdev_region(&ptp->dmtsc_devs.devid, 0,
   608					  ptp->info->n_ext_ts, "ptptsevqch");
   609		if (!err) {
   610			major = MAJOR(ptp->dmtsc_devs.devid);
   611			ptp->dmtsc_devs.dmtsc_class =
 > 612				class_create(THIS_MODULE, "ptptsevqch_class");
   613			for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
   614				mutex_init(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
   615				device = MKDEV(major, idx);
   616				ptp->dmtsc_devs.cdev_info[idx].pclock = ptp;
   617				cdev_init(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev,
   618					  &fops);
   619				err = cdev_add(
   620					&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev,
   621					device, 1);
   622				if (err) {
   623					goto cdev_clean;
   624				} else {
   625					ptp->dmtsc_devs.cdev_info[idx].minor = idx;
   626					dev = device_create(ptp->dmtsc_devs.dmtsc_class,
   627							    &ptp->dev, device, NULL,
   628							    "ptp%dch%d", ptp->index,
   629							    idx);
   630					if (IS_ERR(dev)) {
   631						err = PTR_ERR(dev);
   632						goto cdev_clean;
   633					}
   634				}
   635			}
   636		} else {
   637			goto dev_clean;
   638		}
   639		return 0;
   640	
   641	cdev_clean:
   642		ptp_dmtsc_cdev_clean(ptp);
   643	dev_clean:
   644		kfree(ptp->dmtsc_devs.cdev_info);
   645		ptp->dmtsc_devs.cdev_info = NULL;
   646	err:
   647		return err;
   648	}
   649	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] ptp: Demultiplexed timestamp channels
  2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
                                               ` (2 preceding siblings ...)
  2023-08-31 13:28                             ` kernel test robot
@ 2023-08-31 16:20                             ` kernel test robot
  3 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2023-08-31 16:20 UTC (permalink / raw)
  To: Xabier Marquiegui, richardcochran
  Cc: oe-kbuild-all, chrony-dev, mlichvar, netdev, ntp-lists, reibax

Hi Xabier,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.5 next-20230831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xabier-Marquiegui/ptp-Demultiplexed-timestamp-channels/20230831-054428
base:   net/main
patch link:    https://lore.kernel.org/r/20230830214101.509086-2-reibax%40gmail.com
patch subject: [PATCH] ptp: Demultiplexed timestamp channels
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20230901/202309010030.U9g5VVWf-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230901/202309010030.U9g5VVWf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309010030.U9g5VVWf-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/ptp/ptp_chardev.c:449:9: warning: no previous declaration for 'ptp_queue_read' [-Wmissing-declarations]
    ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
            ^~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:544:5: warning: no previous declaration for 'ptp_dmtsc_release' [-Wmissing-declarations]
    int ptp_dmtsc_release(struct inode *inode, struct file *file)
        ^~~~~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:556:9: warning: no previous declaration for 'ptp_dmtsc_read' [-Wmissing-declarations]
    ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
            ^~~~~~~~~~~~~~
>> drivers/ptp/ptp_chardev.c:571:6: warning: no previous declaration for 'ptp_dmtsc_cdev_clean' [-Wmissing-declarations]
    void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
         ^~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/linkage.h:7:0,
                    from include/linux/kernel.h:17,
                    from include/linux/cpumask.h:10,
                    from include/linux/mm_types_task.h:14,
                    from include/linux/mm_types.h:5,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/ptp/ptp_chardev.c:7:
   drivers/ptp/ptp_chardev.c: In function 'ptp_dmtsc_dev_register':
   include/linux/export.h:31:21: error: passing argument 1 of 'class_create' from incompatible pointer type [-Werror=incompatible-pointer-types]
    #define THIS_MODULE ((struct module *)0)
                        ^
   drivers/ptp/ptp_chardev.c:612:17: note: in expansion of macro 'THIS_MODULE'
       class_create(THIS_MODULE, "ptptsevqch_class");
                    ^~~~~~~~~~~
   In file included from include/linux/device.h:31:0,
                    from include/linux/cdev.h:8,
                    from include/linux/posix-clock.h:10,
                    from drivers/ptp/ptp_chardev.c:8:
   include/linux/device/class.h:230:29: note: expected 'const char *' but argument is of type 'struct module *'
    struct class * __must_check class_create(const char *name);
                                ^~~~~~~~~~~~
   drivers/ptp/ptp_chardev.c:612:4: error: too many arguments to function 'class_create'
       class_create(THIS_MODULE, "ptptsevqch_class");
       ^~~~~~~~~~~~
   In file included from include/linux/device.h:31:0,
                    from include/linux/cdev.h:8,
                    from include/linux/posix-clock.h:10,
                    from drivers/ptp/ptp_chardev.c:8:
   include/linux/device/class.h:230:29: note: declared here
    struct class * __must_check class_create(const char *name);
                                ^~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/ptp_queue_read +449 drivers/ptp/ptp_chardev.c

   448	
 > 449	ssize_t ptp_queue_read(struct ptp_clock *ptp, char __user *buf, size_t cnt,
   450			       int dmtsc)
   451	{
   452		struct timestamp_event_queue *queue;
   453		struct mutex *tsevq_mux;
   454		struct ptp_extts_event *event;
   455		unsigned long flags;
   456		size_t qcnt, i;
   457		int result;
   458	
   459		if (dmtsc < 0) {
   460			queue = &ptp->tsevq;
   461			tsevq_mux = &ptp->tsevq_mux;
   462		} else {
   463			queue = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq;
   464			tsevq_mux = &ptp->dmtsc_devs.cdev_info[dmtsc].tsevq_mux;
   465		}
   466	
   467		if (cnt % sizeof(struct ptp_extts_event) != 0)
   468			return -EINVAL;
   469	
   470		if (cnt > EXTTS_BUFSIZE)
   471			cnt = EXTTS_BUFSIZE;
   472	
   473		cnt = cnt / sizeof(struct ptp_extts_event);
   474	
   475		if (mutex_lock_interruptible(tsevq_mux))
   476			return -ERESTARTSYS;
   477	
   478		if (wait_event_interruptible(ptp->tsev_wq,
   479					     ptp->defunct || queue_cnt(queue))) {
   480			mutex_unlock(tsevq_mux);
   481			return -ERESTARTSYS;
   482		}
   483	
   484		if (ptp->defunct) {
   485			mutex_unlock(tsevq_mux);
   486			return -ENODEV;
   487		}
   488	
   489		event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
   490		if (!event) {
   491			mutex_unlock(tsevq_mux);
   492			return -ENOMEM;
   493		}
   494	
   495		spin_lock_irqsave(&queue->lock, flags);
   496	
   497		qcnt = queue_cnt(queue);
   498	
   499		if (cnt > qcnt)
   500			cnt = qcnt;
   501	
   502		for (i = 0; i < cnt; i++) {
   503			event[i] = queue->buf[queue->head];
   504			queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
   505		}
   506	
   507		spin_unlock_irqrestore(&queue->lock, flags);
   508	
   509		cnt = cnt * sizeof(struct ptp_extts_event);
   510	
   511		mutex_unlock(tsevq_mux);
   512	
   513		result = cnt;
   514		if (copy_to_user(buf, event, cnt))
   515			result = -EFAULT;
   516	
   517		kfree(event);
   518		return result;
   519	}
   520	
   521	ssize_t ptp_read(struct posix_clock *pc, uint rdflags, char __user *buf,
   522			 size_t cnt)
   523	{
   524		struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
   525	
   526		return ptp_queue_read(ptp, buf, cnt, DMTSC_NOT);
   527	}
   528	
   529	static int ptp_dmtsc_open(struct inode *inode, struct file *file)
   530	{
   531		struct ptp_dmtsc_cdev_info *cdev = container_of(
   532			inode->i_cdev, struct ptp_dmtsc_cdev_info, dmtsc_cdev);
   533	
   534		file->private_data = cdev;
   535	
   536		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   537			return -ERESTARTSYS;
   538		cdev->pclock->dmtsc_en_flags |= (0x1 << (cdev->minor));
   539		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   540	
   541		return stream_open(inode, file);
   542	}
   543	
 > 544	int ptp_dmtsc_release(struct inode *inode, struct file *file)
   545	{
   546		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   547	
   548		if (mutex_lock_interruptible(&cdev->pclock->dmtsc_en_mux))
   549			return -ERESTARTSYS;
   550		cdev->pclock->dmtsc_en_flags &= ~(0x1 << (cdev->minor));
   551		mutex_unlock(&cdev->pclock->dmtsc_en_mux);
   552	
   553		return 0;
   554	}
   555	
 > 556	ssize_t ptp_dmtsc_read(struct file *file, char __user *buf, size_t cnt,
   557			       loff_t *offset)
   558	{
   559		struct ptp_dmtsc_cdev_info *cdev = file->private_data;
   560	
   561		return ptp_queue_read(cdev->pclock, buf, cnt, cdev->minor);
   562	}
   563	
   564	static const struct file_operations fops = {
   565							.owner = THIS_MODULE,
   566							.open = ptp_dmtsc_open,
   567							.read = ptp_dmtsc_read,
   568							.release = ptp_dmtsc_release
   569							};
   570	
 > 571	void ptp_dmtsc_cdev_clean(struct ptp_clock *ptp)
   572	{
   573		int idx, major;
   574		dev_t device;
   575	
   576		major = MAJOR(ptp->dmtsc_devs.devid);
   577		for (idx = 0; idx < ptp->info->n_ext_ts; idx++) {
   578			if (ptp->dmtsc_devs.cdev_info[idx].minor >= 0) {
   579				device = MKDEV(major, idx);
   580				device_destroy(ptp->dmtsc_devs.dmtsc_class, device);
   581				cdev_del(&ptp->dmtsc_devs.cdev_info[idx].dmtsc_cdev);
   582				ptp->dmtsc_devs.cdev_info[idx].minor = -1;
   583			}
   584		}
   585		class_destroy(ptp->dmtsc_devs.dmtsc_class);
   586		unregister_chrdev_region(ptp->dmtsc_devs.devid, ptp->info->n_ext_ts);
   587		mutex_destroy(&ptp->dmtsc_devs.cdev_info[idx].tsevq_mux);
   588	}
   589	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 26+ messages in thread

* (no subject)
  2023-08-30 22:01                             ` Richard Cochran
@ 2023-09-06 10:47                               ` Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 1/3] ptp: Replace timestamp event queue with linked list Xabier Marquiegui
                                                   ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Xabier Marquiegui @ 2023-09-06 10:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax


Thank you for your patience Richard, and thank you for guiding me. Let's
see if I got closer to what we want this time.

Let me know what you thing about these patches, please.

Thanks,
Xabier


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/3] ptp: Replace timestamp event queue with linked list
  2023-09-06 10:47                               ` Xabier Marquiegui
@ 2023-09-06 10:47                                 ` Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 2/3] ptp: support multiple timestamp event readers Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 3/3] ptp: support event queue reader channel masks Xabier Marquiegui
  2 siblings, 0 replies; 26+ messages in thread
From: Xabier Marquiegui @ 2023-09-06 10:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

This is the first of a set of patches to introduce linked lists to the
timestamp event queue. The final goal is to be able to have multiple
readers for the timestamp queue.

On this one we maintain the original feature set, and we just introduce
the linked lists to the data structure.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
---
 drivers/ptp/ptp_chardev.c | 18 ++++++++++++++++--
 drivers/ptp/ptp_clock.c   | 30 ++++++++++++++++++++++++++++--
 drivers/ptp/ptp_private.h |  4 +++-
 drivers/ptp/ptp_sysfs.c   |  6 +++++-
 4 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 362bf756e6b7..1ea11f864abb 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -435,10 +435,17 @@ 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 +454,19 @@ 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..dd48b9f41535 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 list_head *pos;
+	struct timestamp_event_queue *element;
+
+	list_for_each(pos, &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(struct timestamp_event_queue), GFP_KERNEL);
+	if (queue == NULL)
+		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);
@@ -376,6 +398,7 @@ EXPORT_SYMBOL(ptp_clock_unregister);
 void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
 {
 	struct pps_event_time evt;
+	struct timestamp_event_queue *tsevq, *tsevq_alt;
 
 	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_safe(tsevq, tsevq_alt, &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..014293255677 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; /* Link to other queues */
 };
 
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/3] ptp: support multiple timestamp event readers
  2023-09-06 10:47                               ` Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 1/3] ptp: Replace timestamp event queue with linked list Xabier Marquiegui
@ 2023-09-06 10:47                                 ` Xabier Marquiegui
  2023-09-06 18:13                                   ` Simon Horman
  2023-09-06 22:13                                   ` kernel test robot
  2023-09-06 10:47                                 ` [PATCH 3/3] ptp: support event queue reader channel masks Xabier Marquiegui
  2 siblings, 2 replies; 26+ messages in thread
From: Xabier Marquiegui @ 2023-09-06 10:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

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>
---
 drivers/ptp/ptp_chardev.c | 95 ++++++++++++++++++++++++++++++---------
 drivers/ptp/ptp_clock.c   |  6 +--
 drivers/ptp/ptp_private.h |  4 +-
 drivers/ptp/ptp_sysfs.c   |  4 --
 4 files changed, 80 insertions(+), 29 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 1ea11f864abb..c65dc6fefaa6 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -103,9 +103,39 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 
 int ptp_open(struct posix_clock *pc, fmode_t fmode)
 {
+	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct timestamp_event_queue *queue;
+
+	queue = kzalloc(sizeof(struct timestamp_event_queue), GFP_KERNEL);
+	if (queue == NULL)
+		return -EINVAL;
+	queue->reader_pid = task_pid_nr(current);
+	list_add_tail(&queue->qlist, &ptp->tsevqs);
+
 	return 0;
 }
 
+int ptp_release(struct posix_clock *pc)
+{
+	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct list_head *pos, *n;
+	struct timestamp_event_queue *element;
+	int found = -1;
+	pid_t reader_pid = task_pid_nr(current);
+
+	list_for_each_safe(pos, n, &ptp->tsevqs) {
+		element = list_entry(pos, struct timestamp_event_queue, qlist);
+		if (element->reader_pid == reader_pid) {
+			list_del(pos);
+			kfree(element);
+			found = 0;
+			return found;
+		}
+	}
+
+	return found;
+}
+
 long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
@@ -436,14 +466,25 @@ __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;
+	struct list_head *pos, *n;
+	bool found = false;
+	pid_t reader_pid = task_pid_nr(current);
 
 	poll_wait(fp, &ptp->tsev_wq, wait);
 
 	/*
-	 * Extract only the first element in the queue list
-	 * TODO: Identify the relevant queue
+	 * Extract only the desired element in the queue list
 	 */
-	queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
+	list_for_each_safe(pos, n, &ptp->tsevqs) {
+		queue = list_entry(pos, struct timestamp_event_queue, qlist);
+		if (queue->reader_pid == reader_pid) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;
 
 	return queue_cnt(queue) ? EPOLLIN : 0;
 }
@@ -459,40 +500,50 @@ ssize_t ptp_read(struct posix_clock *pc,
 	unsigned long flags;
 	size_t qcnt, i;
 	int result;
+	struct list_head *pos, *n;
+	bool found = false;
+	pid_t reader_pid = task_pid_nr(current);
 
 	/*
-	 * Extract only the first element in the queue list
-	 * TODO: Identify the relevant queue
+	 * Extract only the desired element in the queue list
 	 */
-	queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
-				 qlist);
+	list_for_each_safe(pos, n, &ptp->tsevqs) {
+		queue = list_entry(pos, struct timestamp_event_queue, qlist);
+		if (queue->reader_pid == reader_pid) {
+			found = true;
+			break;
+		}
+	}
 
-	if (cnt % sizeof(struct ptp_extts_event) != 0)
-		return -EINVAL;
+	if (!found) {
+		result = -EINVAL;
+		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);
@@ -511,12 +562,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(pc);
 	return result;
 }
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index dd48b9f41535..dc2f045cacbd 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -162,6 +162,7 @@ static struct posix_clock_operations ptp_clock_ops = {
 	.clock_settime	= ptp_clock_settime,
 	.ioctl		= ptp_ioctl,
 	.open		= ptp_open,
+	.release	= ptp_release,
 	.poll		= ptp_poll,
 	.read		= ptp_read,
 };
@@ -184,7 +185,6 @@ 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);
@@ -246,10 +246,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	queue = kzalloc(sizeof(struct timestamp_event_queue), GFP_KERNEL);
 	if (queue == NULL)
 		goto no_memory_queue;
+	queue->reader_pid = 0;
 	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);
 	init_waitqueue_head(&ptp->tsev_wq);
@@ -350,7 +349,6 @@ 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);
 	ptp_clean_queue_list(ptp);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 014293255677..56b0c9df188d 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -27,6 +27,7 @@ struct timestamp_event_queue {
 	int tail;
 	spinlock_t lock;
 	struct list_head qlist; /* Link to other queues */
+	pid_t reader_pid;
 };
 
 struct ptp_clock {
@@ -38,7 +39,6 @@ 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 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 */
@@ -124,6 +124,8 @@ long ptp_ioctl(struct posix_clock *pc,
 
 int ptp_open(struct posix_clock *pc, fmode_t fmode);
 
+int ptp_release(struct posix_clock *pc);
+
 ssize_t ptp_read(struct posix_clock *pc,
 		 uint flags, char __user *buf, size_t cnt);
 
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 2675f383cd0a..512b0164ef18 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -87,9 +87,6 @@ static ssize_t extts_fifo_show(struct device *dev,
 
 	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 +101,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);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/3] ptp: support event queue reader channel masks
  2023-09-06 10:47                               ` Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 1/3] ptp: Replace timestamp event queue with linked list Xabier Marquiegui
  2023-09-06 10:47                                 ` [PATCH 2/3] ptp: support multiple timestamp event readers Xabier Marquiegui
@ 2023-09-06 10:47                                 ` Xabier Marquiegui
  2023-09-06 18:18                                   ` kernel test robot
  2 siblings, 1 reply; 26+ messages in thread
From: Xabier Marquiegui @ 2023-09-06 10:47 UTC (permalink / raw)
  To: richardcochran; +Cc: chrony-dev, mlichvar, netdev, ntp-lists, reibax

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.

Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
---
 drivers/ptp/ptp_chardev.c             | 17 +++++++++++++++++
 drivers/ptp/ptp_clock.c               |  4 +++-
 drivers/ptp/ptp_private.h             |  1 +
 include/uapi/linux/ptp_clock.h        |  7 +++++++
 tools/testing/selftests/ptp/testptp.c | 26 +++++++++++++++++++++++++-
 5 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index c65dc6fefaa6..72697189ac59 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -109,6 +109,7 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
 	queue = kzalloc(sizeof(struct timestamp_event_queue), GFP_KERNEL);
 	if (queue == NULL)
 		return -EINVAL;
+	queue->mask = 0xFFFFFFFF;
 	queue->reader_pid = task_pid_nr(current);
 	list_add_tail(&queue->qlist, &ptp->tsevqs);
 
@@ -139,9 +140,11 @@ int ptp_release(struct posix_clock *pc)
 long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct timestamp_event_queue *tsevq, *tsevq_alt;
 	struct ptp_sys_offset_extended *extoff = NULL;
 	struct ptp_sys_offset_precise precise_offset;
 	struct system_device_crosststamp xtstamp;
+	struct ptp_tsfilter_request tsfilter_req;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
 	struct ptp_system_timestamp sts;
@@ -451,6 +454,20 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		mutex_unlock(&ptp->pincfg_mux);
 		break;
 
+	case PTP_FILTERTS_REQUEST:
+		if (copy_from_user(&tsfilter_req, (void __user *)arg,
+				   sizeof(tsfilter_req))) {
+			err = -EFAULT;
+			break;
+		}
+		list_for_each_entry_safe(tsevq, tsevq_alt, &ptp->tsevqs, qlist) {
+			if (tsevq->reader_pid == tsfilter_req.reader_pid) {
+				tsevq->mask = tsfilter_req.mask;
+				break;
+			}
+		}
+		break;
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index dc2f045cacbd..360bd5f9d759 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -247,6 +247,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (queue == NULL)
 		goto no_memory_queue;
 	queue->reader_pid = 0;
+	queue->mask = 0xFFFFFFFF;
 	spin_lock_init(&queue->lock);
 	list_add_tail(&queue->qlist, &ptp->tsevqs);
 	mutex_init(&ptp->pincfg_mux);
@@ -406,7 +407,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_safe(tsevq, tsevq_alt, &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 56b0c9df188d..07f9e6b64e99 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -28,6 +28,7 @@ struct timestamp_event_queue {
 	spinlock_t lock;
 	struct list_head qlist; /* Link to other queues */
 	pid_t reader_pid;
+	int mask;
 };
 
 struct ptp_clock {
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 05cc35fc94ac..372d0a1dc6f7 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -105,6 +105,11 @@ struct ptp_extts_request {
 	unsigned int rsv[2]; /* Reserved for future use. */
 };
 
+struct ptp_tsfilter_request {
+	pid_t reader_pid; /* PID of process reading the timestamp event queue */
+	unsigned int mask; /* Channel mask. LSB = channel 0 */
+};
+
 struct ptp_perout_request {
 	union {
 		/*
@@ -224,6 +229,8 @@ 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_REQUEST \
+	_IOW(PTP_CLK_MAGIC, 19, struct ptp_tsfilter_request)
 
 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..8c4d40c16cdf 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -121,6 +121,7 @@ static void usage(char *progname)
 		" -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 pid,msk apply ts channel mask to queue open by pid\n"
 		" -g         get the ptp clock time\n"
 		" -h         prints this message\n"
 		" -i val     index for event/trigger\n"
@@ -162,6 +163,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_request tsfilter_req;
 
 	char *progname;
 	unsigned int i;
@@ -194,9 +196,14 @@ int main(int argc, char *argv[])
 	int64_t pulsewidth = -1;
 	int64_t perout = -1;
 
+	tsfilter_req.reader_pid = 0;
+	tsfilter_req.mask = 0xFFFFFFFF;
+
 	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 +217,14 @@ int main(int argc, char *argv[])
 		case 'f':
 			adjfreq = atoi(optarg);
 			break;
+		case 'F':
+			cnt = sscanf(optarg, "%d,%X", &tsfilter_req.reader_pid,
+				     &tsfilter_req.mask);
+			if (cnt != 2) {
+				usage(progname);
+				return -1;
+			}
+			break;
 		case 'g':
 			gettime = 1;
 			break;
@@ -604,6 +619,15 @@ int main(int argc, char *argv[])
 		free(xts);
 	}
 
+	if (tsfilter_req.reader_pid != 0) {
+		if (ioctl(fd, PTP_FILTERTS_REQUEST, &tsfilter_req)) {
+			perror("PTP_FILTERTS_REQUEST");
+		} else {
+			printf("Timestamp event queue mask 0x%X applied to reader with PID: %d\n",
+			       (int)tsfilter_req.mask, tsfilter_req.reader_pid);
+		}
+	}
+
 	close(fd);
 	return 0;
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] ptp: support multiple timestamp event readers
  2023-09-06 10:47                                 ` [PATCH 2/3] ptp: support multiple timestamp event readers Xabier Marquiegui
@ 2023-09-06 18:13                                   ` Simon Horman
  2023-09-06 22:13                                   ` kernel test robot
  1 sibling, 0 replies; 26+ messages in thread
From: Simon Horman @ 2023-09-06 18:13 UTC (permalink / raw)
  To: Xabier Marquiegui; +Cc: richardcochran, chrony-dev, mlichvar, netdev, ntp-lists

On Wed, Sep 06, 2023 at 12:47:53PM +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>

Hi Xabier,

some minor feedback from my side

...

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 1ea11f864abb..c65dc6fefaa6 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -103,9 +103,39 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
>  
>  int ptp_open(struct posix_clock *pc, fmode_t fmode)
>  {
> +	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> +	struct timestamp_event_queue *queue;
> +
> +	queue = kzalloc(sizeof(struct timestamp_event_queue), GFP_KERNEL);
> +	if (queue == NULL)

nit: this could be: if (!queue)

> +		return -EINVAL;
> +	queue->reader_pid = task_pid_nr(current);
> +	list_add_tail(&queue->qlist, &ptp->tsevqs);
> +
>  	return 0;
>  }

...

> @@ -436,14 +466,25 @@ __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;
> +	struct list_head *pos, *n;
> +	bool found = false;
> +	pid_t reader_pid = task_pid_nr(current);
>  
>  	poll_wait(fp, &ptp->tsev_wq, wait);
>  
>  	/*
> -	 * Extract only the first element in the queue list
> -	 * TODO: Identify the relevant queue
> +	 * Extract only the desired element in the queue list
>  	 */
> -	queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
> +	list_for_each_safe(pos, n, &ptp->tsevqs) {
> +		queue = list_entry(pos, struct timestamp_event_queue, qlist);
> +		if (queue->reader_pid == reader_pid) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		return -EINVAL;

Sparse complains that the return type for this function is __poll_t,
but here an int is returned. Perhaps returning EPOLLERR is appropriate here?

>  
>  	return queue_cnt(queue) ? EPOLLIN : 0;
>  }

...

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] ptp: support event queue reader channel masks
  2023-09-06 10:47                                 ` [PATCH 3/3] ptp: support event queue reader channel masks Xabier Marquiegui
@ 2023-09-06 18:18                                   ` kernel test robot
  0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2023-09-06 18:18 UTC (permalink / raw)
  To: Xabier Marquiegui, richardcochran
  Cc: oe-kbuild-all, chrony-dev, mlichvar, netdev, ntp-lists, reibax

Hi Xabier,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]
[also build test ERROR on net-next/main linus/master next-20230906]
[cannot apply to shuah-kselftest/next shuah-kselftest/fixes horms-ipvs/master v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xabier-Marquiegui/ptp-support-multiple-timestamp-event-readers/20230906-194848
base:   net/main
patch link:    https://lore.kernel.org/r/20230906104754.1324412-4-reibax%40gmail.com
patch subject: [PATCH 3/3] ptp: support event queue reader channel masks
config: i386-randconfig-005-20230906 (https://download.01.org/0day-ci/archive/20230907/202309070203.1o2AVeeS-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309070203.1o2AVeeS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309070203.1o2AVeeS-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <command-line>:32:
>> ./usr/include/linux/ptp_clock.h:109:2: error: unknown type name 'pid_t'
     109 |  pid_t reader_pid; /* PID of process reading the timestamp event queue */
         |  ^~~~~

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] ptp: support multiple timestamp event readers
  2023-09-06 10:47                                 ` [PATCH 2/3] ptp: support multiple timestamp event readers Xabier Marquiegui
  2023-09-06 18:13                                   ` Simon Horman
@ 2023-09-06 22:13                                   ` kernel test robot
  1 sibling, 0 replies; 26+ messages in thread
From: kernel test robot @ 2023-09-06 22:13 UTC (permalink / raw)
  To: Xabier Marquiegui, richardcochran
  Cc: oe-kbuild-all, chrony-dev, mlichvar, netdev, ntp-lists, reibax

Hi Xabier,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.5 next-20230906]
[cannot apply to shuah-kselftest/next shuah-kselftest/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xabier-Marquiegui/ptp-support-multiple-timestamp-event-readers/20230906-194848
base:   net/main
patch link:    https://lore.kernel.org/r/20230906104754.1324412-3-reibax%40gmail.com
patch subject: [PATCH 2/3] ptp: support multiple timestamp event readers
config: i386-randconfig-061-20230906 (https://download.01.org/0day-ci/archive/20230907/202309070650.eysYMNnu-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309070650.eysYMNnu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309070650.eysYMNnu-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/ptp/ptp_chardev.c:487:24: sparse: sparse: incorrect type in return expression (different base types) @@     expected restricted __poll_t @@     got int @@
   drivers/ptp/ptp_chardev.c:487:24: sparse:     expected restricted __poll_t
   drivers/ptp/ptp_chardev.c:487:24: sparse:     got int

vim +487 drivers/ptp/ptp_chardev.c

   464	
   465	__poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
   466	{
   467		struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
   468		struct timestamp_event_queue *queue;
   469		struct list_head *pos, *n;
   470		bool found = false;
   471		pid_t reader_pid = task_pid_nr(current);
   472	
   473		poll_wait(fp, &ptp->tsev_wq, wait);
   474	
   475		/*
   476		 * Extract only the desired element in the queue list
   477		 */
   478		list_for_each_safe(pos, n, &ptp->tsevqs) {
   479			queue = list_entry(pos, struct timestamp_event_queue, qlist);
   480			if (queue->reader_pid == reader_pid) {
   481				found = true;
   482				break;
   483			}
   484		}
   485	
   486		if (!found)
 > 487			return -EINVAL;
   488	
   489		return queue_cnt(queue) ? EPOLLIN : 0;
   490	}
   491	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2023-09-06 22:14 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <72ac9741-27f5-36a5-f64c-7d81008eebbc@bluematt.me>
     [not found] ` <Y+3m/PpzkBN9kxJY@localhost>
2023-02-16 17:54   ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Matt Corallo
2023-02-16 22:54     ` Richard Cochran
2023-02-17  0:58       ` Matt Corallo
2023-02-20 10:08       ` Miroslav Lichvar
2023-02-20 15:24         ` Richard Cochran
2023-02-23 20:56           ` Matt Corallo
2023-02-24  0:19             ` Richard Cochran
2023-02-24  1:18               ` Matt Corallo
2023-02-24  5:07                 ` Richard Cochran
2023-08-29 11:47                   ` Xabier Marquiegui
2023-08-29 11:47                     ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
2023-08-29 14:07                       ` Richard Cochran
2023-08-29 14:15                         ` Richard Cochran
2023-08-30 21:41                         ` [chrony-dev] Support for Multiple PPS Inputs on single PHC Xabier Marquiegui
2023-08-30 21:41                           ` [PATCH] ptp: Demultiplexed timestamp channels Xabier Marquiegui
2023-08-30 22:01                             ` Richard Cochran
2023-09-06 10:47                               ` Xabier Marquiegui
2023-09-06 10:47                                 ` [PATCH 1/3] ptp: Replace timestamp event queue with linked list Xabier Marquiegui
2023-09-06 10:47                                 ` [PATCH 2/3] ptp: support multiple timestamp event readers Xabier Marquiegui
2023-09-06 18:13                                   ` Simon Horman
2023-09-06 22:13                                   ` kernel test robot
2023-09-06 10:47                                 ` [PATCH 3/3] ptp: support event queue reader channel masks Xabier Marquiegui
2023-09-06 18:18                                   ` kernel test robot
2023-08-31  0:29                             ` [PATCH] ptp: Demultiplexed timestamp channels kernel test robot
2023-08-31 13:28                             ` kernel test robot
2023-08-31 16:20                             ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).