| 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: <2024100917-daybed-suffering-7367@gregkh>
Date: Wed, 9 Oct 2024 11:19:18 +0200
From: Greg KH <greg@...ah.com>
To: Rodolfo Giometti <giometti@...eenne.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 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@...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().
Why isn't it a real cdev instead?
thanks,
greg k-h
Powered by blists - more mailing lists