On Tue, Oct 08, 2024 at 03:50:31PM +0200, Rodolfo Giometti wrote:
> Sometimes one needs to be able not only to catch PPS signals but to
> produce them also. For example, running a distributed simulation,
> which requires computers' clock to be synchronized very tightly.
>
> This patch adds PPS generators class in order to have a well-defined
> interface for these devices.
>
> Signed-off-by: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Some minor comments on the design:
> ---
> drivers/pps/Makefile | 3 +-
> drivers/pps/generators/Kconfig | 19 +-
> drivers/pps/generators/Makefile | 4 +
> drivers/pps/generators/pps_gen-dummy.c | 83 ++++++++
> drivers/pps/generators/pps_gen.c | 283 +++++++++++++++++++++++++
> drivers/pps/generators/sysfs.c | 89 ++++++++
> include/linux/pps_gen_kernel.h | 57 +++++
> include/uapi/linux/pps_gen.h | 35 +++
> 8 files changed, 571 insertions(+), 2 deletions(-)
> create mode 100644 drivers/pps/generators/pps_gen-dummy.c
> create mode 100644 drivers/pps/generators/pps_gen.c
> create mode 100644 drivers/pps/generators/sysfs.c
> create mode 100644 include/linux/pps_gen_kernel.h
> create mode 100644 include/uapi/linux/pps_gen.h
>
> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> index ceaf65cc1f1d..0aea394d4e4d 100644
> --- a/drivers/pps/Makefile
> +++ b/drivers/pps/Makefile
> @@ -6,6 +6,7 @@
> pps_core-y := pps.o kapi.o sysfs.o
> pps_core-$(CONFIG_NTP_PPS) += kc.o
> obj-$(CONFIG_PPS) := pps_core.o
> -obj-y += clients/ generators/
> +obj-y += clients/
> +obj-$(CONFIG_PPS_GENERATOR) += generators/
>
> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
> index d615e640fcad..e3dfe31609ba 100644
> --- a/drivers/pps/generators/Kconfig
> +++ b/drivers/pps/generators/Kconfig
> @@ -3,7 +3,22 @@
> # PPS generators configuration
> #
>
> -comment "PPS generators support"
> +menuconfig PPS_GENERATOR
> + tristate "PPS generators support"
> + help
> + PPS generators are special hardware which are able to produce PPS
> + (Pulse Per Second) signals.
> +
> +if PPS_GENERATOR
> +
> +config PPS_GENERATOR_DUMMY
> + tristate "Dummy PPS generator (Testing generator, use for debug)"
> + help
> + If you say yes here you get support for a PPS debugging generator
> + (which actual generates no PPS signal at all).
> +
> + This driver can also be built as a module. If so, the module
> + will be called pps_gen-dummy.
Put the dummy driver in a separate patch please, it doesn't belong with
the core changes.
>
> config PPS_GENERATOR_PARPORT
> tristate "Parallel port PPS signal generator"
> @@ -12,3 +27,5 @@ config PPS_GENERATOR_PARPORT
> If you say yes here you get support for a PPS signal generator which
> utilizes STROBE pin of a parallel port to send PPS signals. It uses
> parport abstraction layer and hrtimers to precisely control the signal.
> +
> +endif # PPS_GENERATOR
> diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
> index 2589fd0f2481..dc1aa5a4688b 100644
> --- a/drivers/pps/generators/Makefile
> +++ b/drivers/pps/generators/Makefile
> @@ -3,6 +3,10 @@
> # Makefile for PPS generators.
> #
>
> +pps_gen_core-y := pps_gen.o sysfs.o
> +obj-$(CONFIG_PPS_GENERATOR) := pps_gen_core.o
> +
> +obj-$(CONFIG_PPS_GENERATOR_DUMMY) += pps_gen-dummy.o
> obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
>
> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/generators/pps_gen-dummy.c b/drivers/pps/generators/pps_gen-dummy.c
> new file mode 100644
> index 000000000000..2d257f3f9bf9
> --- /dev/null
> +++ b/drivers/pps/generators/pps_gen-dummy.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS dummy generator
> + *
> + * Copyright (C) 2024 Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Why so many spaces after "2024"?
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why is this needed, drivers should only ever use dev_*() calls, never
pr_*() calls.
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/pps_gen_kernel.h>
> +
> +/*
> + * Global variables
> + */
> +
> +static struct pps_gen_device *pps_gen;
That's by definition, static, not global :)
Also, why is it needed at all?
> +
> +/*
> + * PPS Generator methods
> + */
> +
> +static int pps_gen_dummy_get_time(struct pps_gen_device *pps_gen,
> + struct timespec64 *time)
> +{
> + struct system_time_snapshot snap;
> +
> + ktime_get_snapshot(&snap);
> + *time = ktime_to_timespec64(snap.real);
> +
> + return 0;
> +}
> +
> +static int pps_gen_dummy_enable(struct pps_gen_device *pps_gen, bool enable)
> +{
> + /* always enabled */
> + return 0;
> +}
> +
> +/*
> + * The PPS info struct
> + */
> +
> +static struct pps_gen_source_info pps_gen_dummy_info = {
> + .name = "dummy",
> + .use_system_clock = true,
> + .get_time = pps_gen_dummy_get_time,
> + .enable = pps_gen_dummy_enable,
> +};
> +
> +/*
> + * Module staff
> + */
> +
> +static void __exit pps_gen_dummy_exit(void)
> +{
> + dev_info(pps_gen->dev, "dummy PPS generator unregistered\n");
When drivers are working properly, they are quiet.
> +
> + pps_gen_unregister_source(pps_gen);
> +}
> +
> +static int __init pps_gen_dummy_init(void)
> +{
> + pps_gen = pps_gen_register_source(&pps_gen_dummy_info);
> + if (IS_ERR(pps_gen)) {
> + pr_err("cannot register PPS generator\n");
> + return PTR_ERR(pps_gen);
> + }
> +
> + dev_info(pps_gen->dev, "dummy PPS generator registered\n");
Again, quiet...
> +
> + return 0;
> +}
> +
> +module_init(pps_gen_dummy_init);
> +module_exit(pps_gen_dummy_exit);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("LinuxPPS dummy generator");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pps/generators/pps_gen.c b/drivers/pps/generators/pps_gen.c
> new file mode 100644
> index 000000000000..40b05087b4b4
> --- /dev/null
> +++ b/drivers/pps/generators/pps_gen.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS generators core file
> + *
> + * Copyright (C) 2024 Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Again spaces.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Again, not needed.
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/sched.h>
> +#include <linux/time.h>
> +#include <linux/timex.h>
> +#include <linux/uaccess.h>
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
> +#include <linux/cdev.h>
Why a cdev?
> +#include <linux/fs.h>
> +#include <linux/pps_gen_kernel.h>
> +#include <linux/slab.h>
> +
> +/*
> + * Local variables
> + */
> +
> +static int pps_gen_major;
> +static struct class *pps_gen_class;
> +
> +static DEFINE_MUTEX(pps_gen_idr_lock);
> +static DEFINE_IDR(pps_gen_idr);
> +
> +/*
> + * Char device methods
> + */
> +
> +static long pps_gen_cdev_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct pps_gen_device *pps_gen = file->private_data;
> + unsigned int __user *uiuarg = (unsigned int __user *) arg;
> + unsigned int status;
> + int ret;
> +
> + switch (cmd) {
> + case PPS_GEN_SETENABLE:
> + dev_dbg(pps_gen->dev, "PPS_GEN_SETENABLE\n");
> +
> + ret = get_user(status, uiuarg);
> + if (ret)
> + return -EFAULT;
> +
> + ret = pps_gen->info.enable(pps_gen, status);
> + if (ret)
> + return ret;
> + pps_gen->enabled = status;
> +
> + break;
> +
> + default:
> + return -ENOTTY;
> + }
> +
> + return 0;
> +}
Why is there an ioctl call? That's a totally different user/kernel api
than sysfs, why do you have 2?
> +
> +#ifdef CONFIG_COMPAT
> +static long pps_gen_cdev_compat_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> + return pps_gen_cdev_ioctl(file, cmd, arg);
> +}
> +#else
> +#define pps_gen_cdev_compat_ioctl NULL
> +#endif
> +
> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> +{
> + struct pps_gen_device *pps_gen;
> +
> + mutex_lock(&pps_gen_idr_lock);
> + pps_gen = idr_find(&pps_gen_idr, id);
> + if (pps_gen)
> + kobject_get(&pps_gen->dev->kobj);
> +
> + mutex_unlock(&pps_gen_idr_lock);
Doesn't an idr have a lock in it? I can never remember...
> + return pps_gen;
> +}
> +
> +static int pps_gen_cdev_open(struct inode *inode, struct file *file)
> +{
> + struct pps_gen_device *pps_gen = pps_gen_idr_get(iminor(inode));
> +
> + if (!pps_gen)
> + return -ENODEV;
> +
> + file->private_data = pps_gen;
> + return 0;
> +}
> +
> +static int pps_gen_cdev_release(struct inode *inode, struct file *file)
> +{
> + struct pps_gen_device *pps_gen = file->private_data;
> +
> + WARN_ON(pps_gen->id != iminor(inode));
If this can never happen, don't add this line. If it can happen, then
handle it properly. Either way, don't reboot a box because it happened.
> + kobject_put(&pps_gen->dev->kobj);
Messing with a kobject reference directly from a device feels wrong and
should never be done. Please use the proper apis.
> + return 0;
> +}
> +
> +/*
> + * Char device stuff
> + */
> +
> +static const struct file_operations pps_gen_cdev_fops = {
> + .owner = THIS_MODULE,
> + .compat_ioctl = pps_gen_cdev_compat_ioctl,
Why compat for a new ioctl? Why not write it properly to not need it?
> + .unlocked_ioctl = pps_gen_cdev_ioctl,
> + .open = pps_gen_cdev_open,
> + .release = pps_gen_cdev_release,
> +};
> +
> +static void pps_gen_device_destruct(struct device *dev)
> +{
> + struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +
> + pr_debug("deallocating pps-gen%d\n", pps_gen->id);
ftrace is your friend.
> + kfree(dev);
> + kfree(pps_gen);
> +}
> +
> +static int pps_gen_register_cdev(struct pps_gen_device *pps_gen)
> +{
> + int err;
> + dev_t devt;
> +
> + mutex_lock(&pps_gen_idr_lock);
> +
> + err = idr_alloc(&pps_gen_idr, pps_gen, 0, PPS_GEN_MAX_SOURCES,
> + GFP_KERNEL);
> + if (err < 0) {
> + if (err == -ENOSPC) {
> + pr_err("%s: too many PPS sources in the system\n",
> + pps_gen->info.name);
> + err = -EBUSY;
> + }
> + goto out_unlock;
> + }
> + pps_gen->id = err;
> +
> + devt = MKDEV(pps_gen_major, pps_gen->id);
> + pps_gen->dev = device_create(pps_gen_class, pps_gen->info.parent, devt,
> + pps_gen, "pps-gen%d", pps_gen->id);
> + if (IS_ERR(pps_gen->dev)) {
> + err = PTR_ERR(pps_gen->dev);
> + goto free_idr;
> + }
> +
> + /* Override the release function with our own */
> + pps_gen->dev->release = pps_gen_device_destruct;
> +
> + pr_debug("generator %s got cdev (%d:%d)\n",
> + pps_gen->info.name, pps_gen_major, pps_gen->id);
Why not dev_dbg()?
> +
> + kobject_get(&pps_gen->dev->kobj);
> + mutex_unlock(&pps_gen_idr_lock);
> + return 0;
> +
> +free_idr:
> + idr_remove(&pps_gen_idr, pps_gen->id);
> +out_unlock:
> + mutex_unlock(&pps_gen_idr_lock);
> + return err;
> +}
> +
> +static void pps_gen_unregister_cdev(struct pps_gen_device *pps_gen)
> +{
> + pr_debug("unregistering pps-gen%d\n", pps_gen->id);
> + device_destroy(pps_gen_class, pps_gen->dev->devt);
> +
> + /* Now we can release the ID for re-use */
> + mutex_lock(&pps_gen_idr_lock);
> + idr_remove(&pps_gen_idr, pps_gen->id);
> + kobject_put(&pps_gen->dev->kobj);
> + mutex_unlock(&pps_gen_idr_lock);
> +}
> +
> +/*
> + * Exported functions
> + */
> +
> +/* pps_gen_register_source - add a PPS generator in the system
> + * @info: the PPS generator info struct
> + *
> + * The function returns, in case of success, the PPS generaor device. Otherwise
> + * ERR_PTR(errno).
> + */
> +
> +struct pps_gen_device *pps_gen_register_source(struct pps_gen_source_info *info)
> +{
> + struct pps_gen_device *pps_gen;
> + int err;
> +
> + pps_gen = kzalloc(sizeof(struct pps_gen_device), GFP_KERNEL);
> + if (pps_gen == NULL) {
> + err = -ENOMEM;
> + goto pps_gen_register_source_exit;
> + }
> + pps_gen->info = *info;
> + pps_gen->enabled = false;
Whitespace is all messed up here, did you run checkpatch?
> +
> + /* Create the char device */
> + err = pps_gen_register_cdev(pps_gen);
> + if (err < 0) {
> + pr_err("%s: unable to create char device\n",
> + info->name);
> + goto kfree_pps_gen;
> + }
> +
> + dev_info(pps_gen->dev, "new PPS generator %s\n", info->name);
Again, quiet...
> +
> + return pps_gen;
> +
> +kfree_pps_gen:
> + kfree(pps_gen);
> +
> +pps_gen_register_source_exit:
> + pr_err("%s: unable to register generaor\n", info->name);
dev_err()?
> +
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(pps_gen_register_source);
I have to ask, why not EXPORT_SYMBOL_GPL()?
> +
> +/* pps_gen_unregister_source - remove a PPS generator from the system
> + * @pps_gen: the PPS generator
> + */
> +
> +void pps_gen_unregister_source(struct pps_gen_device *pps_gen)
> +{
> + pps_gen_unregister_cdev(pps_gen);
> +}
> +EXPORT_SYMBOL(pps_gen_unregister_source);
> +
> +/*
> + * Module stuff
> + */
> +
> +static void __exit pps_gen_exit(void)
> +{
> + class_destroy(pps_gen_class);
> + __unregister_chrdev(pps_gen_major, 0, PPS_GEN_MAX_SOURCES, "pps-gen");
Why the __ version? Are you sure?
> +}
> +
> +static int __init pps_gen_init(void)
> +{
> + pps_gen_class = class_create("pps-gen");
> + if (IS_ERR(pps_gen_class)) {
> + pr_err("failed to allocate class\n");
> + return PTR_ERR(pps_gen_class);
> + }
> + pps_gen_class->dev_groups = pps_gen_groups;
> +
> + pps_gen_major = __register_chrdev(0, 0, PPS_GEN_MAX_SOURCES, "pps-gen",
> + &pps_gen_cdev_fops);
Again, why __?
> + if (pps_gen_major < 0) {
> + pr_err("failed to allocate char device region\n");
> + goto remove_class;
> + }
> +
> + return 0;
> +
> +remove_class:
> + class_destroy(pps_gen_class);
> + return pps_gen_major;
> +}
> +
> +subsys_initcall(pps_gen_init);
> +module_exit(pps_gen_exit);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("LinuxPPS generators support");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pps/generators/sysfs.c b/drivers/pps/generators/sysfs.c
> new file mode 100644
> index 000000000000..247030d138e1
> --- /dev/null
> +++ b/drivers/pps/generators/sysfs.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS generators sysfs support
> + *
> + * Copyright (C) 2024 Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
again...
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/pps_gen_kernel.h>
> +
> +/*
> + * Attribute functions
> + */
> +
> +static ssize_t system_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
Again whitespace...
> +
> + return sysfs_emit(buf, "%d\n", pps_gen->info.use_system_clock);
> +}
> +static DEVICE_ATTR_RO(system);
> +
> +static ssize_t time_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> + struct timespec64 time;
> + int ret;
> +
> + ret = pps_gen->info.get_time(pps_gen, &time);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%llu %09lu\n", time.tv_sec, time.tv_nsec);
> +}
> +static DEVICE_ATTR_RO(time);
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> + bool status;
> + unsigned int enable;
> + int ret;
> +
> + ret = sscanf(buf, "%u", &enable);
> + if (ret != 1)
> + return -EINVAL;
> + status = !!enable;
> +
> + ret = pps_gen->info.enable(pps_gen, status);
> + if (ret)
> + return ret;
> + pps_gen->enabled = status;
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(enable);
> +
> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%s\n", pps_gen->info.name);
Why have a separate name? That shouldn't matter at all. If it does
matter, than link to the device that created it properly, don't make up
yet another name for your device.
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *pps_gen_attrs[] = {
> + &dev_attr_enable.attr,
> + &dev_attr_name.attr,
> + &dev_attr_time.attr,
> + &dev_attr_system.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group pps_gen_group = {
> + .attrs = pps_gen_attrs,
> +};
> +
> +const struct attribute_group *pps_gen_groups[] = {
> + &pps_gen_group,
> + NULL,
> +};
> diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
> new file mode 100644
> index 000000000000..5513415b53ec
> --- /dev/null
> +++ b/include/linux/pps_gen_kernel.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * PPS generator API kernel header
> + *
> + * Copyright (C) 2024 Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> + */
> +
> +#ifndef LINUX_PPS_GEN_KERNEL_H
> +#define LINUX_PPS_GEN_KERNEL_H
> +
> +#include <linux/pps_gen.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +/*
> + * Global defines
> + */
> +
> +struct pps_gen_device;
> +
> +/* The specific PPS source info */
> +struct pps_gen_source_info {
> + char name[PPS_GEN_MAX_NAME_LEN]; /* symbolic name */
> + bool use_system_clock;
> +
> + int (*get_time)(struct pps_gen_device *pps_gen,
> + struct timespec64 *time);
> + int (*enable)(struct pps_gen_device *pps_gen, bool enable);
> +
> + struct module *owner;
> + struct device *parent; /* for device_create */
> +};
> +
> +/* The main struct */
> +struct pps_gen_device {
> + struct pps_gen_source_info info; /* PSS generator info */
> + bool enabled; /* PSS generator status */
> +
> + unsigned int id; /* PPS generator unique ID */
> + struct device *dev;
Why not be a real device? What is this a pointer to?
> +};
This structure can be private, right?
> +
> +/*
> + * Global variables
> + */
> +
> +extern const struct attribute_group *pps_gen_groups[];
Why is this global?
> +
> +/*
> + * Exported functions
> + */
> +
> +extern struct pps_gen_device *pps_gen_register_source(
> + struct pps_gen_source_info *info);
> +extern void pps_gen_unregister_source(struct pps_gen_device *pps_gen);
> +
> +#endif /* LINUX_PPS_GEN_KERNEL_H */
> diff --git a/include/uapi/linux/pps_gen.h b/include/uapi/linux/pps_gen.h
> new file mode 100644
> index 000000000000..7b6f50fcab8c
> --- /dev/null
> +++ b/include/uapi/linux/pps_gen.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
I have to ask, why "GPL-2.0+"?
> +/*
> + * PPS generator API header
> + *
> + * Copyright (C) 2024 Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
License boilerplate should be removed.
> + */
> +
> +
> +#ifndef _PPS_GEN_H_
> +#define _PPS_GEN_H_
> +
> +#include <linux/types.h>
> +
> +#define PPS_GEN_MAX_SOURCES 16 /* should be enough... */
What is this for? Who is using it in userspace?
> +#define PPS_GEN_MAX_NAME_LEN 32
Why is this exported to userspace?
> +
> +#include <linux/ioctl.h>
> +
> +#define PPS_GEN_SETENABLE _IOW('g', 0xa1, unsigned int *)
Documentation for this new ioctl?
thanks,
greg k-h
