| 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: <541eb5c6-5546-4170-9e8b-d421d55822a1@enneenne.com>
Date: Wed, 9 Oct 2024 10:48:14 +0200
From: Rodolfo Giometti <giometti@...eenne.com>
To: Greg KH <greg@...ah.com>
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>, corbet@....net,
Hall Christopher S <christopher.s.hall@...el.com>,
Mohan Subramanian <subramanian.mohan@...el.com>, tglx@...utronix.de,
andriy.shevchenko@...ux.intel.com, Dong Eddie <eddie.dong@...el.com>,
N Pandith <pandith.n@...el.com>,
T R Thejesh Reddy <thejesh.reddy.t.r@...el.com>,
Zage David <david.zage@...el.com>,
Chinnadurai Srinivasan <srinivasan.chinnadurai@...el.com>
Subject: Re: [RFC 1/3] drivers pps: add PPS generators support
On 08/10/24 17:42, Greg KH wrote:
> 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@...eenne.com>
>
> 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.
Done.
>>
>> 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@...eenne.com>
>
> Why so many spaces after "2024"?
Fixed.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> Why is this needed, drivers should only ever use dev_*() calls, never
> pr_*() calls.
Fixed.
>> +
>> +#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 :)
Fixed.
> Also, why is it needed at all?
I need a reference within module_exit() to the device created into module_init().
>> +
>> +/*
>> + * 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.
Fixed.
>> +
>> + 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...
Fixed.
>> +
>> + return 0;
>> +}
>> +
>> +module_init(pps_gen_dummy_init);
>> +module_exit(pps_gen_dummy_exit);
>> +
>> +MODULE_AUTHOR("Rodolfo Giometti <giometti@...eenne.com>");
>> +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@...eenne.com>
>
> Again spaces.
Fixed.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> Again, not needed.
Fixed.
>> +
>> +#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?
PPS sources have related cdevs, so by analogy PPS generators also have them.
>> +#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?
PPS sources have ioctl()s, so by analogy... :)
>> +
>> +#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...
As far as I know we must use a mutex...
>> + 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.
Fixed.
>> + kobject_put(&pps_gen->dev->kobj);
>
> Messing with a kobject reference directly from a device feels wrong and
> should never be done.
I followed the suggestions in this patch whose look sane to me:
https://lore.kernel.org/lkml/fc5fe55c-422d-4e63-a5bd-8b6b2d3e6c62@enneenne.com/T/
> Please use the proper apis.
Which API are you talking about? Can you please provide some advice?
>
>
>> + 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?
Fixed.
>
>> + .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.
I see, but we can also use pr_debug()! :P
>
>> + 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()?
Honestly I prefer pr_debug() because this message is not device related, but it
is geneated by the PPS subsystem.
>> +
>> + 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?
Fixed.
>> +
>> + /* 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...
Fixed.
>> +
>> + 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()?
All PPS symbols are defined as EXPORT_SYMBOL()...
>> +
>> +/* 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?
Again, see the above patch.
>> +}
>> +
>> +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 __?
Ditto.
>> + 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@...eenne.com>");
>> +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@...eenne.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> again...
Fixed.
>> +
>> +#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...
Fixed.
>> +
>> + 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?
This can be useful in order to distinguish between different PPS generators in
the system.
> 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.
I'm not sure to understand what you mean... The "name" attribute is just a label
which the userspace my (or my not) use to know which generator to enable or not.
>> +}
>> +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@...eenne.com>
>> + */
>> +
>> +#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 is a pointer to the device created within the pps_gen_register_cdev().
>> +};
>
> This structure can be private, right?
Yes. Just the PPS subsystem uses it.
>> +
>> +/*
>> + * Global variables
>> + */
>> +
>> +extern const struct attribute_group *pps_gen_groups[];
>
> Why is this global?
It is used in drivers/pps/generators/pps_gen.c and referenced in
drivers/pps/generators/sysfs.c.
>> +
>> +/*
>> + * 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+"?
Fixed.
>> +/*
>> + * PPS generator API header
>> + *
>> + * Copyright (C) 2024 Rodolfo Giometti <giometti@...eenne.com>
>> + *
>> + * 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.
Fixed.
>> + */
>> +
>> +
>> +#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?
Fixed.
>> +#define PPS_GEN_MAX_NAME_LEN 32
>
> Why is this exported to userspace?
Fixed.
>> +
>> +#include <linux/ioctl.h>
>> +
>> +#define PPS_GEN_SETENABLE _IOW('g', 0xa1, unsigned int *)
>
> Documentation for this new ioctl?
Where should I add it? Can you please provide some advice?
Thanks a lot for your suggestions! I'm gping to provide a first release for this
patchset shortly.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@...eenne.com
Linux Device Driver giometti@...ux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
Powered by blists - more mailing lists