Re: [RFC] simple_char: New infrastructure to simplify chardev management

[Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

> Thoughts?  I want to use this for the u2f driver, which will either be
> a chardev driver in its own right or use a simple new iso7816 class.
> 
> Ideally we could convert a bunch of drivers to use this, at least
> where there are no legacy minor number considerations.

I'd really like to see a few consumer posted with it, to see how the
conversion works.

> -			   topology.o container.o
> +			   topology.o container.o simple_char.o

And the code should probably go into fs/char_dev.c, and have simple
char_ names to that people use it naturally.  A bit of documentation
of all the char interfaces and why you'd want to use this one would
also be useful for driver writers.

> +static int simple_char_open(struct inode *inode, struct file *filep)
> +{
> +	struct simple_char_major *major =
> +		container_of(inode->i_cdev, struct simple_char_major,
> +			     cdev);
> +	void *private;
> +	const struct simple_char_ops *ops;
> +	int ret = 0;
> +
> +	mutex_lock(&major->lock);
> +
> +	{
> +		/*
> +		 * This is a separate block to make the locking entirely
> +		 * clear.  The only thing keeping minor alive is major->lock.
> +		 * We need to be completely done with the simple_char_minor
> +		 * by the time we release the lock.
> +		 */
> +		struct simple_char_minor *minor;
> +		minor = idr_find(&major->idr, iminor(inode));
> +		if (!minor || !minor->ops->reference(minor->private)) {
> +			mutex_unlock(&major->lock);
> +			return -ENODEV;
> +		}
> +		private = minor->private;
> +		ops = minor->ops;
> +	}
> +
> +	mutex_unlock(&major->lock);
> +
> +	replace_fops(filep, ops->fops);
> +	filep->private_data = private;

So we're back to replace_fops here.  I would much prefer if this
would the regions interface so that we don't have to rely on a full
major allocation and replacing live file operations.

> +EXPORT_SYMBOL(simple_char_major_create);

new exported function without any documentation

> +
> +void simple_char_major_free(struct simple_char_major *major)
> +{
> +	BUG_ON(!idr_is_empty(&major->idr));
> +
> +	cdev_del(&major->cdev);
> +	unregister_chrdev_region(MKDEV(major->majornum, 0), MAX_MINORS);
> +	idr_destroy(&major->idr);
> +	kfree(major);
> +}

non-static, non-exported function?

> +/**
> + * simple_char_minor_create() - create a chardev minor
> + * @major:	Major to use or NULL for a fully dynamic chardev.
> + * @ops:	simple_char_ops to associate with the minor.
> + * @private:	opaque pointer for @ops's use.
> + *
> + * simple_char_minor_create() creates a minor chardev.  For new code,
> + * @major should be NULL; this will create a minor chardev with fully
> + * dynamic major and minor numbers and without a useful name in
> + * /proc/devices.  (All recent user code should be using sysfs
> + * exclusively to map between devices and device numbers.)  For legacy
> + * code, @major can come from simple_char_major_create().

Sounds like you should not pass @major for the main interface then,
and instead have a low-level interface that takes the major pointer.