Re: [RFC] Simple userspace interface for PCI drivers

[Andrew Morton]

On Tue, 29 Aug 2006 23:23:38 -0700
Greg KH <greg@xxxxxxxxx> 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/