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@xxxxxxxxxxxx/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@xxxxxxxxxxxx> > > > + */ > > > + > > > +#ifndef LINUX_PPS_GEN_KERNEL_H > > > +#define LINUX_PPS_GEN_KERNEL_H > > > + > > > +#include <linux/pps_gen.h> > > > +#include <linux/cdev.h> > > > +#include <linux/device.h> > > > + > > > +/* > > > + * Global defines > > > + */ > > > + > > > +struct pps_gen_device; > > > + > > > +/* The specific PPS source info */ > > > +struct pps_gen_source_info { > > > + char name[PPS_GEN_MAX_NAME_LEN]; /* symbolic name */ > > > + bool use_system_clock; > > > + > > > + int (*get_time)(struct pps_gen_device *pps_gen, > > > + struct timespec64 *time); > > > + int (*enable)(struct pps_gen_device *pps_gen, bool enable); > > > + > > > + struct module *owner; > > > + struct device *parent; /* for device_create */ > > > +}; > > > + > > > +/* The main struct */ > > > +struct pps_gen_device { > > > + struct pps_gen_source_info info; /* PSS generator info */ > > > + bool enabled; /* PSS generator status */ > > > + > > > + unsigned int id; /* PPS generator unique ID */ > > > + struct device *dev; > > > > Why not be a real device? What is this a pointer to? > > This 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
