| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <875y4fos43.fsf@intel.com>
Date: Tue, 12 Sep 2023 17:05:00 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Xabier Marquiegui <reibax@...il.com>, netdev@...r.kernel.org
Cc: richardcochran@...il.com, horms@...nel.org,
chrony-dev@...ony.tuxfamily.org, mlichvar@...hat.com, reibax@...il.com,
ntp-lists@...tcorallo.com, shuah@...nel.org, davem@...emloft.net,
rrameshbabu@...dia.com, alex.maftei@....com
Subject: Re: [PATCH net-next v2 1/3] ptp: Replace timestamp event queue with
linked list
Xabier Marquiegui <reibax@...il.com> writes:
> 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@...il.com>
> Suggested-by: Richard Cochran <richardcochran@...il.com>
> ---
> v2:
> - Style changes to comform to checkpatch strict suggestions
> v1: https://lore.kernel.org/netdev/20230906104754.1324412-2-reibax@gmail.com/
>
> drivers/ptp/ptp_chardev.c | 16 ++++++++++++++--
> drivers/ptp/ptp_clock.c | 30 ++++++++++++++++++++++++++++--
> drivers/ptp/ptp_private.h | 4 +++-
> drivers/ptp/ptp_sysfs.c | 6 +++++-
> 4 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 362bf756e6b7..197edf1179f1 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -435,10 +435,16 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> __poll_t ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
> {
> struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> + struct timestamp_event_queue *queue;
>
> poll_wait(fp, &ptp->tsev_wq, wait);
>
> - return queue_cnt(&ptp->tsevq) ? EPOLLIN : 0;
> + /* Extract only the first element in the queue list
> + * TODO: Identify the relevant queue
> + */
> + queue = list_entry(&ptp->tsevqs, struct timestamp_event_queue, qlist);
> +
> + return queue_cnt(queue) ? EPOLLIN : 0;
> }
>
> #define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
> @@ -447,12 +453,18 @@ ssize_t ptp_read(struct posix_clock *pc,
> uint rdflags, char __user *buf, size_t cnt)
> {
> struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> - struct timestamp_event_queue *queue = &ptp->tsevq;
> + struct timestamp_event_queue *queue;
> struct ptp_extts_event *event;
> unsigned long flags;
> size_t qcnt, i;
> int result;
>
> + /* Extract only the first element in the queue list
> + * TODO: Identify the relevant queue
> + */
> + queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
> + qlist);
> +
> if (cnt % sizeof(struct ptp_extts_event) != 0)
> return -EINVAL;
>
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 80f74e38c2da..7ac04a282ec5 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -166,6 +166,18 @@ static struct posix_clock_operations ptp_clock_ops = {
> .read = ptp_read,
> };
>
> +static void ptp_clean_queue_list(struct ptp_clock *ptp)
> +{
> + struct timestamp_event_queue *element;
> + struct list_head *pos;
> +
> + list_for_each(pos, &ptp->tsevqs) {
Here it should be list_for_each_safe().
> + element = list_entry(pos, struct timestamp_event_queue, qlist);
> + list_del(pos);
> + kfree(element);
> + }
> +}
> +
> static void ptp_clock_release(struct device *dev)
> {
> struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
> @@ -175,6 +187,7 @@ static void ptp_clock_release(struct device *dev)
> mutex_destroy(&ptp->tsevq_mux);
> mutex_destroy(&ptp->pincfg_mux);
> mutex_destroy(&ptp->n_vclocks_mux);
> + ptp_clean_queue_list(ptp);
> ida_free(&ptp_clocks_map, ptp->index);
> kfree(ptp);
> }
> @@ -206,6 +219,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> struct device *parent)
> {
> struct ptp_clock *ptp;
> + struct timestamp_event_queue *queue = NULL;
> int err = 0, index, major = MAJOR(ptp_devt);
> size_t size;
>
> @@ -228,7 +242,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> ptp->info = info;
> ptp->devid = MKDEV(major, index);
> ptp->index = index;
> - spin_lock_init(&ptp->tsevq.lock);
> + INIT_LIST_HEAD(&ptp->tsevqs);
> + queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> + if (!queue)
> + goto no_memory_queue;
> + spin_lock_init(&queue->lock);
> + list_add_tail(&queue->qlist, &ptp->tsevqs);
> + /* TODO - Transform or delete this mutex */
> mutex_init(&ptp->tsevq_mux);
> mutex_init(&ptp->pincfg_mux);
> mutex_init(&ptp->n_vclocks_mux);
> @@ -333,6 +353,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> mutex_destroy(&ptp->tsevq_mux);
> mutex_destroy(&ptp->pincfg_mux);
> mutex_destroy(&ptp->n_vclocks_mux);
> + ptp_clean_queue_list(ptp);
> +no_memory_queue:
> ida_free(&ptp_clocks_map, index);
> no_slot:
> kfree(ptp);
> @@ -375,6 +397,7 @@ EXPORT_SYMBOL(ptp_clock_unregister);
>
> void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
> {
> + struct timestamp_event_queue *tsevq, *tsevq_alt;
> struct pps_event_time evt;
>
> switch (event->type) {
> @@ -383,7 +406,10 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
> break;
>
> case PTP_CLOCK_EXTTS:
> - enqueue_external_timestamp(&ptp->tsevq, event);
> + /* Enqueue timestamp on all other queues */
> + list_for_each_entry_safe(tsevq, tsevq_alt, &ptp->tsevqs, qlist) {
The _safe() version is for when the "pos" entry can be removed inside
the loop. No need to use it here.
> + enqueue_external_timestamp(tsevq, event);
> + }
> wake_up_interruptible(&ptp->tsev_wq);
> break;
>
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 75f58fc468a7..314c21c39f6a 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -15,6 +15,7 @@
> #include <linux/ptp_clock.h>
> #include <linux/ptp_clock_kernel.h>
> #include <linux/time.h>
> +#include <linux/list.h>
>
> #define PTP_MAX_TIMESTAMPS 128
> #define PTP_BUF_TIMESTAMPS 30
> @@ -25,6 +26,7 @@ struct timestamp_event_queue {
> int head;
> int tail;
> spinlock_t lock;
> + struct list_head qlist;
> };
>
> struct ptp_clock {
> @@ -35,7 +37,7 @@ struct ptp_clock {
> int index; /* index into clocks.map */
> struct pps_device *pps_source;
> long dialed_frequency; /* remembers the frequency adjustment */
> - struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
> + struct list_head tsevqs; /* timestamp fifo list */
> struct mutex tsevq_mux; /* one process at a time reading the fifo */
> struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
> wait_queue_head_t tsev_wq;
> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index 6e4d5456a885..2675f383cd0a 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -75,12 +75,16 @@ static ssize_t extts_fifo_show(struct device *dev,
> struct device_attribute *attr, char *page)
> {
> struct ptp_clock *ptp = dev_get_drvdata(dev);
> - struct timestamp_event_queue *queue = &ptp->tsevq;
> + struct timestamp_event_queue *queue;
> struct ptp_extts_event event;
> unsigned long flags;
> size_t qcnt;
> int cnt = 0;
>
> + /* The sysfs fifo will always draw from the fist queue */
> + queue = list_first_entry(&ptp->tsevqs, struct timestamp_event_queue,
> + qlist);
> +
> memset(&event, 0, sizeof(event));
>
> if (mutex_lock_interruptible(&ptp->tsevq_mux))
> --
> 2.34.1
>
>
--
Vinicius
Powered by blists - more mailing lists