[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1157011516.29250.248.camel@localhost.localdomain>
Date: Thu, 31 Aug 2006 10:05:16 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Andrew Morton <akpm@...l.org>
Cc: Greg KH <greg@...ah.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC] Simple userspace interface for PCI drivers
On Thu, 2006-08-31 at 00:40 -0700, Andrew Morton wrote:
> On Tue, 29 Aug 2006 23:23:38 -0700
> Greg KH <greg@...ah.com> wrote:
>
> > +static ssize_t store_sig_pid(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + iio_dummy_signal.pid = simple_strtol(buf, NULL, 10);
> > + if (iio_dummy_signal.pid == 0) {
> > + if (iio_dummy_signal.it_process) {
> > + put_task_struct(iio_dummy_signal.it_process);
> > + iio_dummy_signal.it_process = NULL;
> > + }
> > +
> > + iio_dummy_signal.pid = 0;
> > + return count;
> > + }
> > +
> > + if (iio_dummy_signal.pid == 1)
> > + goto out;
> > +
> > + iio_dummy_signal.it_process = find_task_by_pid(iio_dummy_signal.pid);
> > + if (iio_dummy_signal.it_process) {
> > + get_task_struct(iio_dummy_signal.it_process);
> > + iio_dummy_signal.it_sigev_notify = SIGEV_SIGNAL;
> > + iio_dummy_signal.it_sigev_signo = SIGALRM;
> > + iio_dummy_signal.it_sigev_value.sival_int = 0;
> > +
> > + return count;
> > + }
> > +out:
> > + iio_dummy_signal.pid = 0;
> > + return -EINVAL;
> > +}
>
> This is racy: find_task_by_pid() needs tasklist_lock or rcu_read_lock().
>
> It doesn't work as a module due to missing __put_task_struct.
>
>
> It is also rather nasty. Why go shoving some random pid into a sysfs file,
> then hang onto a ref on a task_struct for some process which exitted last
> week? It would be cleaner and more idiomatic to require that the
> controlling process hold an fd open against the instance so that resources
> can be managed correctly. Maybe use SIGIO too, so the driver doesn't need
> to know about pids and task_structs and things. Which all maps better onto
> ioctls than sysfs (ties self to stake)
>
> <looks>
>
> iio_dev.c seems to already be doing this. Why does iio_dummy.c exist?
Uurg. That's an old dummy/test driver which never got updated.
tglx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists