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@enneenne.com>
---
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.
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@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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;
+
+/*
+ * 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");
+
+ 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");
+
+ return 0;
+}
+
+module_init(pps_gen_dummy_init);
+module_exit(pps_gen_dummy_exit);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.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@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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>
+#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;
+}
+
+#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);
+ 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));
+ kobject_put(&pps_gen->dev->kobj);
+ return 0;
+}
+
+/*
+ * Char device stuff
+ */
+
+static const struct file_operations pps_gen_cdev_fops = {
+ .owner = THIS_MODULE,
+ .compat_ioctl = pps_gen_cdev_compat_ioctl,
+ .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);
+ 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);
+
+ 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;
+
+ /* 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);
+
+ return pps_gen;
+
+kfree_pps_gen:
+ kfree(pps_gen);
+
+pps_gen_register_source_exit:
+ pr_err("%s: unable to register generaor\n", info->name);
+
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL(pps_gen_register_source);
+
+/* 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");
+}
+
+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);
+ 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@enneenne.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@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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);
+
+ 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);
+}
+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@enneenne.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;
+};
+
+/*
+ * Global variables
+ */
+
+extern const struct attribute_group *pps_gen_groups[];
+
+/*
+ * 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 */
+/*
+ * PPS generator API header
+ *
+ * Copyright (C) 2024 Rodolfo Giometti <giometti@enneenne.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.
+ */
+
+
+#ifndef _PPS_GEN_H_
+#define _PPS_GEN_H_
+
+#include <linux/types.h>
+
+#define PPS_GEN_MAX_SOURCES 16 /* should be enough... */
+#define PPS_GEN_MAX_NAME_LEN 32
+
+#include <linux/ioctl.h>
+
+#define PPS_GEN_SETENABLE _IOW('g', 0xa1, unsigned int *)
+
+#endif /* _PPS_GEN_H_ */
--
2.34.1Hi Rudolfo,
> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com>
> Sent: Tuesday, October 08, 2024 6:51 AM
> To: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@enneenne.com>
> ---
> 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
This looks pretty good to me. I would like to see an alarm callback. We are able
to detect a missed event and rather than stopping inexplicably or writing to the
system log, it would be better to be able to notify an application directly.
Off the top of my head, something like:
void pps_gen_alarm(pps_gen_device *pps_gen) {
pps_gen->alarm = 1;
sysfs_notify(pps_gen->dev->kobj, NULL, "alarm");
}
The device is reset by disabling/enabling and this resets the alarm flag.
Could we add something like this?
Thanks,
Chris
On 09/10/24 04:49, Hall, Christopher S wrote:
> Hi Rudolfo,
>
>> -----Original Message-----
>> From: Rodolfo Giometti <giometti@enneenne.com>
>> Sent: Tuesday, October 08, 2024 6:51 AM
>> To: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
>
>> 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@enneenne.com>
>> ---
>> 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
>
> This looks pretty good to me. I would like to see an alarm callback. We are able
> to detect a missed event and rather than stopping inexplicably or writing to the
> system log, it would be better to be able to notify an application directly.
>
> Off the top of my head, something like:
>
> void pps_gen_alarm(pps_gen_device *pps_gen) {
> pps_gen->alarm = 1;
> sysfs_notify(pps_gen->dev->kobj, NULL, "alarm");
> }
>
> The device is reset by disabling/enabling and this resets the alarm flag.
>
> Could we add something like this?
Yes, of course. But it's better to provide a proper patch against my patchset V1
(I'm going to anticipate them to you shortly), since I'm not sure to understand
what you need.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
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@enneenne.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.
>
> 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@enneenne.com>
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@enneenne.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@enneenne.com>
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@enneenne.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@enneenne.com>
> + */
> +
> +#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@enneenne.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 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@enneenne.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.
> + */
> +
> +
> +#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
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@enneenne.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@enneenne.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@enneenne.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@enneenne.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@enneenne.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@enneenne.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@enneenne.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@enneenne.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@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > +#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...
If you do, someone will come along and remove it, please see:
https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
as an example (with links that show it is not needed).
thanks,
greg k-h
On 10/10/24 09:15, Greg KH wrote:
> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>> +#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...
>
> If you do, someone will come along and remove it, please see:
> https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> as an example (with links that show it is not needed).
Here is an example about ida API, but I'm using idr API.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
> On 10/10/24 09:15, Greg KH wrote:
> > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > > > +#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...
> >
> > If you do, someone will come along and remove it, please see:
> > https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> > as an example (with links that show it is not needed).
>
> Here is an example about ida API, but I'm using idr API.
Why not use ida then? :)
On 10/10/24 09:54, Greg KH wrote:
> On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
>> On 10/10/24 09:15, Greg KH wrote:
>>> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>>>> +#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...
>>>
>>> If you do, someone will come along and remove it, please see:
>>> https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
>>> as an example (with links that show it is not needed).
>>
>> Here is an example about ida API, but I'm using idr API.
>
> Why not use ida then? :)
Because we need an ID associated with a pointer.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
> On 10/10/24 09:54, Greg KH wrote:
> > On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
> > > On 10/10/24 09:15, Greg KH wrote:
> > > > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > > > > > +#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...
> > > >
> > > > If you do, someone will come along and remove it, please see:
> > > > https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> > > > as an example (with links that show it is not needed).
> > >
> > > Here is an example about ida API, but I'm using idr API.
> >
> > Why not use ida then? :)
>
> Because we need an ID associated with a pointer.
Ah, ok, but why? Why is the "id" here the mapping to the pointer? Why
not use the structures you already have to store this in (i.e. the
normal driver model stuff?)
All you should need an idr/ida for is to pick a unique "number" for
naming your class device. Everything after that should already be there
for you to get access to the structures you need to get access to.
thanks,
greg k-h
On 10/10/24 12:04, Greg KH wrote:
> On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
>> On 10/10/24 09:54, Greg KH wrote:
>>> On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
>>>> On 10/10/24 09:15, Greg KH wrote:
>>>>> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>>>>>> +#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...
>>>>>
>>>>> If you do, someone will come along and remove it, please see:
>>>>> https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
>>>>> as an example (with links that show it is not needed).
>>>>
>>>> Here is an example about ida API, but I'm using idr API.
>>>
>>> Why not use ida then? :)
>>
>> Because we need an ID associated with a pointer.
>
> Ah, ok, but why? Why is the "id" here the mapping to the pointer? Why
> not use the structures you already have to store this in (i.e. the
> normal driver model stuff?)
>
> All you should need an idr/ida for is to pick a unique "number" for
> naming your class device. Everything after that should already be there
> for you to get access to the structures you need to get access to.
OK, let me review the code and then I'm going to propose a new patchset.
Thanks for your suggestions. :)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > + 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/
That patch is wrong.
> > Please use the proper apis.
>
> Which API are you talking about? Can you please provide some advice?
get_device()
You are working on devices, NOT a raw kobject, no driver should EVER be
calling into a kobject function or a sysfs function, there should be
driver core functions for everything you need to do.
>
> >
> >
> > > + 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.
But you have a device, please use it! Otherwise it's impossible to
track back what is going on to what device in the system.
> > > +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.
Again, rely on the backing device structure for this (i.e. the symlink
in sysfs), you do not need to duplicate existing infrastructure.
> > 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.
Again, it's tied to the device in the system, don't list that same thing
again.
>
> > > +}
> > > +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@enneenne.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().
Why isn't it a real cdev instead?
thanks,
greg k-h
On 09/10/24 11:19, Greg KH wrote: > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote: >>>> + 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/ > > That patch is wrong. :( >>> Please use the proper apis. >> >> Which API are you talking about? Can you please provide some advice? > > get_device() > > You are working on devices, NOT a raw kobject, no driver should EVER be > calling into a kobject function or a sysfs function, there should be > driver core functions for everything you need to do. OK, I'm going to provide a new RFC taking in account what you suggest. Thanks, Rodolfo -- GNU/Linux Solutions e-mail: giometti@enneenne.com Linux Device Driver giometti@linux.it Embedded Systems phone: +39 349 2432127 UNIX programming
© 2016 - 2024 Red Hat, Inc.