Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH <greg@kroah.com>]

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