[RFC/PATCH v7 01/12] media: Media device node support

[RFC/PATCH v7 01/12] media: Media device node support

[Laurent Pinchart]

The media_devnode structure provides support for registering and
unregistering character devices using a dynamic major number. Reference
counting is handled internally, making device drivers easier to write
without having to solve the open/disconnect race condition issue over
and over again.

The code is based on video/v4l2-dev.c.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/Kconfig         |   13 ++
 drivers/media/Makefile        |   10 +-
 drivers/media/media-devnode.c |  321 +++++++++++++++++++++++++++++++++++++++++
 include/media/media-devnode.h |   97 +++++++++++++
 4 files changed, 439 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/media-devnode.c
 create mode 100644 include/media/media-devnode.h

diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index a28541b..6b946e6 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -14,6 +14,19 @@ if MEDIA_SUPPORT
 comment "Multimedia core support"
 
 #
+# Media controller
+#
+
+config MEDIA_CONTROLLER
+	bool "Media Controller API (EXPERIMENTAL)"
+	depends on EXPERIMENTAL
+	---help---
+	  Enable the media controller API used to query media devices internal
+	  topology and configure it dynamically.
+
+	  This API is mostly used by camera interfaces in embedded platforms.
+
+#
 # V4L core and enabled API's
 #
 
diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 499b081..3a08991 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -2,7 +2,13 @@
 # Makefile for the kernel multimedia device drivers.
 #
 
+media-objs	:= media-devnode.o
+
+ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
+  obj-$(CONFIG_MEDIA_SUPPORT) += media.o
+endif
+
 obj-y += common/ IR/ video/
 
-obj-$(CONFIG_VIDEO_DEV) += radio/
-obj-$(CONFIG_DVB_CORE)  += dvb/
+obj-$(CONFIG_VIDEO_DEV)		+= radio/
+obj-$(CONFIG_DVB_CORE)		+= dvb/
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
new file mode 100644
index 0000000..7804b70
--- /dev/null
+++ b/drivers/media/media-devnode.c
@@ -0,0 +1,321 @@
+/*
+ * Media device node
+ *
+ * Copyright (C) 2010 Nokia Corporation
+ *
+ * Based on drivers/media/video/v4l2_dev.c code authored by
+ *	Mauro Carvalho Chehab <mchehab@infradead.org> (version 2)
+ *	Alan Cox, <alan@lxorguk.ukuu.org.uk> (version 1)
+ *
+ * Contacts: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *	     Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * --
+ *
+ * Generic media device node infrastructure to register and unregister
+ * character devices using a dynamic major number and proper reference
+ * counting.
+ */
+
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/kmod.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+#include <linux/smp_lock.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <asm/system.h>
+
+#include <media/media-devnode.h>
+
+#define MEDIA_NUM_DEVICES	256
+#define MEDIA_NAME		"media"
+
+static dev_t media_dev_t;
+
+/*
+ *	Active devices
+ */
+static DEFINE_MUTEX(media_devnode_lock);
+static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
+
+/* Called when the last user of the media device exits. */
+static void media_devnode_release(struct device *cd)
+{
+	struct media_devnode *mdev = to_media_devnode(cd);
+
+	mutex_lock(&media_devnode_lock);
+
+	/* Delete the cdev on this minor as well */
+	cdev_del(&mdev->cdev);
+
+	/* Mark device node number as free */
+	clear_bit(mdev->minor, media_devnode_nums);
+
+	mutex_unlock(&media_devnode_lock);
+
+	/* Release media_devnode and perform other cleanups as needed. */
+	if (mdev->release)
+		mdev->release(mdev);
+}
+
+static struct bus_type media_bus_type = {
+	.name = MEDIA_NAME,
+};
+
+static ssize_t media_read(struct file *filp, char __user *buf,
+		size_t sz, loff_t *off)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	if (!mdev->fops->read)
+		return -EINVAL;
+	if (!media_devnode_is_registered(mdev))
+		return -EIO;
+	return mdev->fops->read(filp, buf, sz, off);
+}
+
+static ssize_t media_write(struct file *filp, const char __user *buf,
+		size_t sz, loff_t *off)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	if (!mdev->fops->write)
+		return -EINVAL;
+	if (!media_devnode_is_registered(mdev))
+		return -EIO;
+	return mdev->fops->write(filp, buf, sz, off);
+}
+
+static unsigned int media_poll(struct file *filp,
+			       struct poll_table_struct *poll)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	if (!media_devnode_is_registered(mdev))
+		return POLLERR | POLLHUP;
+	if (!mdev->fops->poll)
+		return DEFAULT_POLLMASK;
+	return mdev->fops->poll(filp, poll);
+}
+
+static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	if (!mdev->fops->ioctl)
+		return -ENOTTY;
+
+	if (!media_devnode_is_registered(mdev))
+		return -EIO;
+
+	return mdev->fops->ioctl(filp, cmd, arg);
+}
+
+/* Override for the open function */
+static int media_open(struct inode *inode, struct file *filp)
+{
+	struct media_devnode *mdev;
+	int ret;
+
+	/* Check if the media device is available. This needs to be done with
+	 * the media_devnode_lock held to prevent an open/unregister race:
+	 * without the lock, the device could be unregistered and freed between
+	 * the media_devnode_is_registered() and get_device() calls, leading to
+	 * a crash.
+	 */
+	mutex_lock(&media_devnode_lock);
+	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
+	/* return ENXIO if the media device has been removed
+	   already or if it is not registered anymore. */
+	if (!media_devnode_is_registered(mdev)) {
+		mutex_unlock(&media_devnode_lock);
+		return -ENXIO;
+	}
+	/* and increase the device refcount */
+	get_device(&mdev->dev);
+	mutex_unlock(&media_devnode_lock);
+
+	filp->private_data = mdev;
+
+	if (mdev->fops->open) {
+		ret = mdev->fops->open(filp);
+		if (ret) {
+			put_device(&mdev->dev);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/* Override for the release function */
+static int media_release(struct inode *inode, struct file *filp)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+	int ret = 0;
+
+	if (mdev->fops->release)
+		mdev->fops->release(filp);
+
+	/* decrease the refcount unconditionally since the release()
+	   return value is ignored. */
+	put_device(&mdev->dev);
+	filp->private_data = NULL;
+	return ret;
+}
+
+static const struct file_operations media_devnode_fops = {
+	.owner = THIS_MODULE,
+	.read = media_read,
+	.write = media_write,
+	.open = media_open,
+	.unlocked_ioctl = media_ioctl,
+	.release = media_release,
+	.poll = media_poll,
+	.llseek = no_llseek,
+};
+
+/**
+ * media_devnode_register - register a media device node
+ * @mdev: media device node structure we want to register
+ *
+ * The registration code assigns minor numbers and registers the new device node
+ * with the kernel. An error is returned if no free minor number can be found,
+ * or if the registration of the device node fails.
+ *
+ * Zero is returned on success.
+ *
+ * Note that if the media_devnode_register call fails, the release() callback of
+ * the media_devnode structure is *not* called, so the caller is responsible for
+ * freeing any data.
+ */
+int __must_check media_devnode_register(struct media_devnode *mdev)
+{
+	int minor;
+	int ret;
+
+	/* Part 1: Find a free minor number */
+	mutex_lock(&media_devnode_lock);
+	minor = find_next_zero_bit(media_devnode_nums, 0, MEDIA_NUM_DEVICES);
+	if (minor == MEDIA_NUM_DEVICES) {
+		mutex_unlock(&media_devnode_lock);
+		printk(KERN_ERR "could not get a free minor\n");
+		return -ENFILE;
+	}
+
+	set_bit(mdev->minor, media_devnode_nums);
+	mutex_unlock(&media_devnode_lock);
+
+	mdev->minor = minor;
+
+	/* Part 2: Initialize and register the character device */
+	cdev_init(&mdev->cdev, &media_devnode_fops);
+	mdev->cdev.owner = mdev->fops->owner;
+
+	ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1);
+	if (ret < 0) {
+		printk(KERN_ERR "%s: cdev_add failed\n", __func__);
+		goto error;
+	}
+
+	/* Part 3: Register the media device */
+	mdev->dev.bus = &media_bus_type;
+	mdev->dev.devt = MKDEV(MAJOR(media_dev_t), mdev->minor);
+	mdev->dev.release = media_devnode_release;
+	if (mdev->parent)
+		mdev->dev.parent = mdev->parent;
+	dev_set_name(&mdev->dev, "media%d", mdev->minor);
+	ret = device_register(&mdev->dev);
+	if (ret < 0) {
+		printk(KERN_ERR "%s: device_register failed\n", __func__);
+		goto error;
+	}
+
+	/* Part 4: Activate this minor. The char device can now be used. */
+	set_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+
+	return 0;
+
+error:
+	cdev_del(&mdev->cdev);
+	clear_bit(mdev->minor, media_devnode_nums);
+	return ret;
+}
+
+/**
+ * media_devnode_unregister - unregister a media device node
+ * @mdev: the device node to unregister
+ *
+ * This unregisters the passed device. Future open calls will be met with
+ * errors.
+ *
+ * This function can safely be called if the device node has never been
+ * registered or has already been unregistered.
+ */
+void media_devnode_unregister(struct media_devnode *mdev)
+{
+	/* Check if mdev was ever registered at all */
+	if (!media_devnode_is_registered(mdev))
+		return;
+
+	mutex_lock(&media_devnode_lock);
+	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+	mutex_unlock(&media_devnode_lock);
+	device_unregister(&mdev->dev);
+}
+
+/*
+ *	Initialise media for linux
+ */
+static int __init media_devnode_init(void)
+{
+	int ret;
+
+	printk(KERN_INFO "Linux media interface: v0.10\n");
+	ret = alloc_chrdev_region(&media_dev_t, 0, MEDIA_NUM_DEVICES,
+				  MEDIA_NAME);
+	if (ret < 0) {
+		printk(KERN_WARNING "media: unable to allocate major\n");
+		return ret;
+	}
+
+	ret = bus_register(&media_bus_type);
+	if (ret < 0) {
+		unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
+		printk(KERN_WARNING "media: bus_register failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void __exit media_devnode_exit(void)
+{
+	bus_unregister(&media_bus_type);
+	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
+}
+
+module_init(media_devnode_init)
+module_exit(media_devnode_exit)
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Device node registration for media drivers");
+MODULE_LICENSE("GPL");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
new file mode 100644
index 0000000..01cd034
--- /dev/null
+++ b/include/media/media-devnode.h
@@ -0,0 +1,97 @@
+/*
+ * Media device node
+ *
+ * Copyright (C) 2010 Nokia Corporation
+ *
+ * Contacts: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *	     Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * --
+ *
+ * Common functions for media-related drivers to register and unregister media
+ * device nodes.
+ */
+
+#ifndef _MEDIA_DEVNODE_H
+#define _MEDIA_DEVNODE_H
+
+#include <linux/poll.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
+
+/*
+ * Flag to mark the media_devnode struct as registered. Drivers must not touch
+ * this flag directly, it will be set and cleared by media_devnode_register and
+ * media_devnode_unregister.
+ */
+#define MEDIA_FLAG_REGISTERED	0
+
+struct media_file_operations {
+	struct module *owner;
+	ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
+	ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
+	unsigned int (*poll) (struct file *, struct poll_table_struct *);
+	long (*ioctl) (struct file *, unsigned int, unsigned long);
+	int (*open) (struct file *);
+	int (*release) (struct file *);
+};
+
+/**
+ * struct media_devnode - Media device node
+ * @parent:	parent device
+ * @minor:	device node minor number
+ * @flags:	flags, combination of the MEDIA_FLAG_* constants
+ *
+ * This structure represents a media-related device node.
+ *
+ * The @parent is a physical device. It must be set by core or device drivers
+ * before registering the node.
+ */
+struct media_devnode {
+	/* device ops */
+	const struct media_file_operations *fops;
+
+	/* sysfs */
+	struct device dev;		/* media device */
+	struct cdev cdev;		/* character device */
+	struct device *parent;		/* device parent */
+
+	/* device info */
+	int minor;
+	unsigned long flags;		/* Use bitops to access flags */
+
+	/* callbacks */
+	void (*release)(struct media_devnode *mdev);
+};
+
+/* dev to media_devnode */
+#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
+
+int __must_check media_devnode_register(struct media_devnode *mdev);
+void media_devnode_unregister(struct media_devnode *mdev);
+
+static inline struct media_devnode *media_devnode_data(struct file *filp)
+{
+	return filp->private_data;
+}
+
+static inline int media_devnode_is_registered(struct media_devnode *mdev)
+{
+	return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+}
+
+#endif /* _MEDIA_DEVNODE_H */
-- 
1.7.2.2

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Greg KH]

On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> The media_devnode structure provides support for registering and
> unregistering character devices using a dynamic major number. Reference
> counting is handled internally, making device drivers easier to write
> without having to solve the open/disconnect race condition issue over
> and over again.

What race condition are you referring to?

> +config MEDIA_CONTROLLER
> +	bool "Media Controller API (EXPERIMENTAL)"
> +	depends on EXPERIMENTAL
> +	---help---
> +	  Enable the media controller API used to query media devices internal
> +	  topology and configure it dynamically.
> +
> +	  This API is mostly used by camera interfaces in embedded platforms.

That's nice, but why should I enable this?  Or will drivers enable it
automatically?


> +#define MEDIA_NUM_DEVICES	256

Why this limit?

> +#define MEDIA_NAME		"media"

Are you sure this is a good name for a camera?

> +static dev_t media_dev_t;

Only one major number?  Is it always dynamic?

> +
> +/*
> + *	Active devices
> + */
> +static DEFINE_MUTEX(media_devnode_lock);
> +static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
> +
> +/* Called when the last user of the media device exits. */
> +static void media_devnode_release(struct device *cd)
> +{
> +	struct media_devnode *mdev = to_media_devnode(cd);
> +
> +	mutex_lock(&media_devnode_lock);
> +
> +	/* Delete the cdev on this minor as well */
> +	cdev_del(&mdev->cdev);
> +
> +	/* Mark device node number as free */
> +	clear_bit(mdev->minor, media_devnode_nums);
> +
> +	mutex_unlock(&media_devnode_lock);
> +
> +	/* Release media_devnode and perform other cleanups as needed. */
> +	if (mdev->release)
> +		mdev->release(mdev);
> +}

You forgot to free the device structure here as well, right?

> +static ssize_t media_read(struct file *filp, char __user *buf,
> +		size_t sz, loff_t *off)
> +{
> +	struct media_devnode *mdev = media_devnode_data(filp);
> +
> +	if (!mdev->fops->read)
> +		return -EINVAL;
> +	if (!media_devnode_is_registered(mdev))
> +		return -EIO;

How could this happen?  And are you sure -EIO is correct?

> +	return mdev->fops->read(filp, buf, sz, off);
> +}
> +
> +static ssize_t media_write(struct file *filp, const char __user *buf,
> +		size_t sz, loff_t *off)
> +{
> +	struct media_devnode *mdev = media_devnode_data(filp);
> +
> +	if (!mdev->fops->write)
> +		return -EINVAL;
> +	if (!media_devnode_is_registered(mdev))
> +		return -EIO;

Same as above, and same comment in other places (poll, ioctl.)

> +/* Override for the open function */
> +static int media_open(struct inode *inode, struct file *filp)
> +{
> +	struct media_devnode *mdev;
> +	int ret;
> +
> +	/* Check if the media device is available. This needs to be done with
> +	 * the media_devnode_lock held to prevent an open/unregister race:
> +	 * without the lock, the device could be unregistered and freed between
> +	 * the media_devnode_is_registered() and get_device() calls, leading to
> +	 * a crash.
> +	 */
> +	mutex_lock(&media_devnode_lock);
> +	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);

By virtue of having the reference to the module held by the vfs, this
shouldn't ever go away, even if the lock is not held.

> +	/* return ENXIO if the media device has been removed
> +	   already or if it is not registered anymore. */
> +	if (!media_devnode_is_registered(mdev)) {
> +		mutex_unlock(&media_devnode_lock);
> +		return -ENXIO;
> +	}

So you can unregister a device at any time, even if the device is open,
or about to be opened?  Then that's fine, but you can put the lock after
the container_of(), right?

> +	/* and increase the device refcount */
> +	get_device(&mdev->dev);

How is that holding anything into memory?  Don't you want to keep the
module that the fops pointer in the device in memory, not necessarily
the device itself?

> +	mutex_unlock(&media_devnode_lock);
> +
> +	filp->private_data = mdev;
> +
> +	if (mdev->fops->open) {
> +		ret = mdev->fops->open(filp);
> +		if (ret) {
> +			put_device(&mdev->dev);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

No reference counting for the fops?  Why not?

Anyway, it looks like what you really want is an "easier" way to handle
a cdev and a struct device that will export the proper information to
userspace, right?

Why not do this generically, fixing up the cdev interface (which really
needs it) and not tie it to media devices at all, making it possible for
_everyone_ to use this type of infrastructure?

That seems like the better thing to do here.

thanks,

greg k-h

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Greg KH]

On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> +/*
> + * Flag to mark the media_devnode struct as registered. Drivers must not touch
> + * this flag directly, it will be set and cleared by media_devnode_register and
> + * media_devnode_unregister.
> + */
> +#define MEDIA_FLAG_REGISTERED	0

It's a define, not a flag, or anything that any driver could touch.

And if you don't want anyone to touch the thing, then make it private
and unable to be touched by anyone else.  Otherwise it will be
touched...

thanks,

greg k-h

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Laurent Pinchart]

Hi Greg,

Thank you for the review.

On Thursday 23 December 2010 04:32:53 Greg KH wrote:
> On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> > The media_devnode structure provides support for registering and
> > unregistering character devices using a dynamic major number. Reference
> > counting is handled internally, making device drivers easier to write
> > without having to solve the open/disconnect race condition issue over
> > and over again.
> 
> What race condition are you referring to?

In a nutshell, the race between device disconnection, which results in the 
device instance being delete (if not in use of course), and open() calls from 
userspace. The problem has been solved in V4L a couple of releases ago after 
suffering from this race for a too long time. As V4L devices (and now media 
devices) need to create both a struct device and a struct cdev instance, 
careful locking is needed.

> > +config MEDIA_CONTROLLER
> > +	bool "Media Controller API (EXPERIMENTAL)"
> > +	depends on EXPERIMENTAL
> > +	---help---
> > +	  Enable the media controller API used to query media devices internal
> > +	  topology and configure it dynamically.
> > +
> > +	  This API is mostly used by camera interfaces in embedded platforms.
> 
> That's nice, but why should I enable this?  Or will drivers enable it
> automatically?

Drivers depending on the media controller API will enable this, yes. The 
option will probably removed later when the API won't be deemed as 
experimental anymore.

> > +#define MEDIA_NUM_DEVICES	256
> 
> Why this limit?

Because I'm using a bitmap to store the used minor numbers, and I thus need a 
limit. I could get rid of it of it by using a linked list, but that will not 
be efficient (you could argue that the list will hold a few entries only most 
of the time, but in that case a limit of 256 minors wouldn't be a problem 
:-)).

> > +#define MEDIA_NAME		"media"
> 
> Are you sure this is a good name for a camera?

It's not just camera. Media devices are... well, media devices. Basically 
anything that can handle audio and/or video streams. The media controller API 
can be used by plain audio devices.

> > +static dev_t media_dev_t;
> 
> Only one major number?  Is it always dynamic?

Yes, one major and (for now) 256 minors. Is there a problem with it being 
dynamic ?

> > +
> > +/*
> > + *	Active devices
> > + */
> > +static DEFINE_MUTEX(media_devnode_lock);
> > +static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
> > +
> > +/* Called when the last user of the media device exits. */
> > +static void media_devnode_release(struct device *cd)
> > +{
> > +	struct media_devnode *mdev = to_media_devnode(cd);
> > +
> > +	mutex_lock(&media_devnode_lock);
> > +
> > +	/* Delete the cdev on this minor as well */
> > +	cdev_del(&mdev->cdev);
> > +
> > +	/* Mark device node number as free */
> > +	clear_bit(mdev->minor, media_devnode_nums);
> > +
> > +	mutex_unlock(&media_devnode_lock);
> > +
> > +	/* Release media_devnode and perform other cleanups as needed. */
> > +	if (mdev->release)
> > +		mdev->release(mdev);
> > +}
> 
> You forgot to free the device structure here as well, right?

That will be done by the release callback. The media_devnode structure is 
embedded in the media_device structure, which will be embedded in driver-
specific structures.

> > +static ssize_t media_read(struct file *filp, char __user *buf,
> > +		size_t sz, loff_t *off)
> > +{
> > +	struct media_devnode *mdev = media_devnode_data(filp);
> > +
> > +	if (!mdev->fops->read)
> > +		return -EINVAL;
> > +	if (!media_devnode_is_registered(mdev))
> > +		return -EIO;
> 
> How could this happen?

This can happen when a USB device is disconnected for instance.

> And are you sure -EIO is correct?

-ENXIO is probably better (I always confuse that with -ENODEV).

> > +	return mdev->fops->read(filp, buf, sz, off);
> > +}
> > +
> > +static ssize_t media_write(struct file *filp, const char __user *buf,
> > +		size_t sz, loff_t *off)
> > +{
> > +	struct media_devnode *mdev = media_devnode_data(filp);
> > +
> > +	if (!mdev->fops->write)
> > +		return -EINVAL;
> > +	if (!media_devnode_is_registered(mdev))
> > +		return -EIO;
> 
> Same as above, and same comment in other places (poll, ioctl.)

OK.

> > +/* Override for the open function */
> > +static int media_open(struct inode *inode, struct file *filp)
> > +{
> > +	struct media_devnode *mdev;
> > +	int ret;
> > +
> > +	/* Check if the media device is available. This needs to be done with
> > +	 * the media_devnode_lock held to prevent an open/unregister race:
> > +	 * without the lock, the device could be unregistered and freed between
> > +	 * the media_devnode_is_registered() and get_device() calls, leading to
> > +	 * a crash.
> > +	 */
> > +	mutex_lock(&media_devnode_lock);
> > +	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
> 
> By virtue of having the reference to the module held by the vfs, this
> shouldn't ever go away, even if the lock is not held.

inode->i_cdev is set to NULL by cdev_default_release() which can be called 
from media_devnode_unregister(). I could move to container_of outside the 
lock, but in that case I would have to check for mdev == NULL || 
!mdev_devnode_is_registered(mdev) (or move the NULL check inside 
mdev_devnode_is_registered). Is that what you would like ?

> > +	/* return ENXIO if the media device has been removed
> > +	   already or if it is not registered anymore. */
> > +	if (!media_devnode_is_registered(mdev)) {
> > +		mutex_unlock(&media_devnode_lock);
> > +		return -ENXIO;
> > +	}
> 
> So you can unregister a device at any time, even if the device is open,
> or about to be opened?

That's correct. That way drivers don't need to care about unregister/open 
races, media_devnode will handle it for them.

> Then that's fine, but you can put the lock after the container_of(), right?

If I add a NULL check (as explained above), yes.

> > +	/* and increase the device refcount */
> > +	get_device(&mdev->dev);
> 
> How is that holding anything into memory?

That will prevent the device instance from being freed until the device is 
closed, thereby holding both the device instance and the cdev instance in 
memory.

> Don't you want to keep the module that the fops pointer in the device in
> memory, not necessarily the device itself?

The cdev owner pointer is set to the fops owner. Unless I'm mistaken it will 
keep the module in memory. I need to keep the device in memory (or rather the 
media_devnode structure that embeds it) to handle file operations on a device 
that gets unregistered after it has been opened.

> > +	mutex_unlock(&media_devnode_lock);
> > +
> > +	filp->private_data = mdev;
> > +
> > +	if (mdev->fops->open) {
> > +		ret = mdev->fops->open(filp);
> > +		if (ret) {
> > +			put_device(&mdev->dev);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> No reference counting for the fops?  Why not?

Because cdev already increments the owner refcount on open().

> Anyway, it looks like what you really want is an "easier" way to handle
> a cdev and a struct device that will export the proper information to
> userspace, right?
> 
> Why not do this generically, fixing up the cdev interface (which really
> needs it) and not tie it to media devices at all, making it possible for
> _everyone_ to use this type of infrastructure?
> 
> That seems like the better thing to do here.

Sounds like a good idea. You're a better cdev expert than me, so could you 
give me a few pointers ? Do you want me to create a new object that will hold 
a struct cdev and a struct device together, or to embed the device structure 
into the existing cdev structure ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Greg KH]

On Fri, Dec 24, 2010 at 12:59:38PM +0100, Laurent Pinchart wrote:
> Hi Greg,
> 
> Thank you for the review.
> 
> On Thursday 23 December 2010 04:32:53 Greg KH wrote:
> > On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> > > The media_devnode structure provides support for registering and
> > > unregistering character devices using a dynamic major number. Reference
> > > counting is handled internally, making device drivers easier to write
> > > without having to solve the open/disconnect race condition issue over
> > > and over again.
> > 
> > What race condition are you referring to?
> 
> In a nutshell, the race between device disconnection, which results in the 
> device instance being delete (if not in use of course), and open() calls from 
> userspace. The problem has been solved in V4L a couple of releases ago after 
> suffering from this race for a too long time. As V4L devices (and now media 
> devices) need to create both a struct device and a struct cdev instance, 
> careful locking is needed.

Ok, that's not really nice, but I guess it is needed.

> > > +config MEDIA_CONTROLLER
> > > +	bool "Media Controller API (EXPERIMENTAL)"
> > > +	depends on EXPERIMENTAL
> > > +	---help---
> > > +	  Enable the media controller API used to query media devices internal
> > > +	  topology and configure it dynamically.
> > > +
> > > +	  This API is mostly used by camera interfaces in embedded platforms.
> > 
> > That's nice, but why should I enable this?  Or will drivers enable it
> > automatically?
> 
> Drivers depending on the media controller API will enable this, yes. The 
> option will probably removed later when the API won't be deemed as 
> experimental anymore.
> 
> > > +#define MEDIA_NUM_DEVICES	256
> > 
> > Why this limit?
> 
> Because I'm using a bitmap to store the used minor numbers, and I thus need a 
> limit. I could get rid of it of it by using a linked list, but that will not 
> be efficient (you could argue that the list will hold a few entries only most 
> of the time, but in that case a limit of 256 minors wouldn't be a problem 
> :-)).

As it's only needed to be looked up at open() time, why not just make it
dynamic?

> > > +#define MEDIA_NAME		"media"
> > 
> > Are you sure this is a good name for a camera?
> 
> It's not just camera. Media devices are... well, media devices. Basically 
> anything that can handle audio and/or video streams. The media controller API 
> can be used by plain audio devices.

Ok.

> > > +static dev_t media_dev_t;
> > 
> > Only one major number?  Is it always dynamic?
> 
> Yes, one major and (for now) 256 minors. Is there a problem with it being 
> dynamic ?

No, just curious.

> > > +
> > > +/*
> > > + *	Active devices
> > > + */
> > > +static DEFINE_MUTEX(media_devnode_lock);
> > > +static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
> > > +
> > > +/* Called when the last user of the media device exits. */
> > > +static void media_devnode_release(struct device *cd)
> > > +{
> > > +	struct media_devnode *mdev = to_media_devnode(cd);
> > > +
> > > +	mutex_lock(&media_devnode_lock);
> > > +
> > > +	/* Delete the cdev on this minor as well */
> > > +	cdev_del(&mdev->cdev);
> > > +
> > > +	/* Mark device node number as free */
> > > +	clear_bit(mdev->minor, media_devnode_nums);
> > > +
> > > +	mutex_unlock(&media_devnode_lock);
> > > +
> > > +	/* Release media_devnode and perform other cleanups as needed. */
> > > +	if (mdev->release)
> > > +		mdev->release(mdev);
> > > +}
> > 
> > You forgot to free the device structure here as well, right?
> 
> That will be done by the release callback. The media_devnode structure is 
> embedded in the media_device structure, which will be embedded in driver-
> specific structures.

Ok.

> > > +static ssize_t media_read(struct file *filp, char __user *buf,
> > > +		size_t sz, loff_t *off)
> > > +{
> > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > +
> > > +	if (!mdev->fops->read)
> > > +		return -EINVAL;
> > > +	if (!media_devnode_is_registered(mdev))
> > > +		return -EIO;
> > 
> > How could this happen?
> 
> This can happen when a USB device is disconnected for instance.

But what's to keep that from happening on the next line as well?  That
doesn't seem like a check you can ever be sure about, so I wouldn't do
it at all.

> > And are you sure -EIO is correct?
> 
> -ENXIO is probably better (I always confuse that with -ENODEV).
> 
> > > +	return mdev->fops->read(filp, buf, sz, off);
> > > +}
> > > +
> > > +static ssize_t media_write(struct file *filp, const char __user *buf,
> > > +		size_t sz, loff_t *off)
> > > +{
> > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > +
> > > +	if (!mdev->fops->write)
> > > +		return -EINVAL;
> > > +	if (!media_devnode_is_registered(mdev))
> > > +		return -EIO;
> > 
> > Same as above, and same comment in other places (poll, ioctl.)
> 
> OK.
> 
> > > +/* Override for the open function */
> > > +static int media_open(struct inode *inode, struct file *filp)
> > > +{
> > > +	struct media_devnode *mdev;
> > > +	int ret;
> > > +
> > > +	/* Check if the media device is available. This needs to be done with
> > > +	 * the media_devnode_lock held to prevent an open/unregister race:
> > > +	 * without the lock, the device could be unregistered and freed between
> > > +	 * the media_devnode_is_registered() and get_device() calls, leading to
> > > +	 * a crash.
> > > +	 */
> > > +	mutex_lock(&media_devnode_lock);
> > > +	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
> > 
> > By virtue of having the reference to the module held by the vfs, this
> > shouldn't ever go away, even if the lock is not held.
> 
> inode->i_cdev is set to NULL by cdev_default_release() which can be called 
> from media_devnode_unregister(). I could move to container_of outside the 
> lock, but in that case I would have to check for mdev == NULL || 
> !mdev_devnode_is_registered(mdev) (or move the NULL check inside 
> mdev_devnode_is_registered). Is that what you would like ?

As container_of _ALWAYS_ returns a valid pointer, you can't check it for
NULL.  I don't know, it just doesn't seem correct here, but if you are
sure it's working properly, I'll not push the issue.

> > > +	/* return ENXIO if the media device has been removed
> > > +	   already or if it is not registered anymore. */
> > > +	if (!media_devnode_is_registered(mdev)) {
> > > +		mutex_unlock(&media_devnode_lock);
> > > +		return -ENXIO;
> > > +	}
> > 
> > So you can unregister a device at any time, even if the device is open,
> > or about to be opened?
> 
> That's correct. That way drivers don't need to care about unregister/open 
> races, media_devnode will handle it for them.

Ok.

> > Then that's fine, but you can put the lock after the container_of(), right?
> 
> If I add a NULL check (as explained above), yes.

Again, you can't check for NULL as the result of container_of() that
does not work (hint, container_of() is just pointer math, without ever
looking at the original pointer value.)

> > > +	/* and increase the device refcount */
> > > +	get_device(&mdev->dev);
> > 
> > How is that holding anything into memory?
> 
> That will prevent the device instance from being freed until the device is 
> closed, thereby holding both the device instance and the cdev instance in 
> memory.

Tricky :)

> > Don't you want to keep the module that the fops pointer in the device in
> > memory, not necessarily the device itself?
> 
> The cdev owner pointer is set to the fops owner. Unless I'm mistaken it will 
> keep the module in memory. I need to keep the device in memory (or rather the 
> media_devnode structure that embeds it) to handle file operations on a device 
> that gets unregistered after it has been opened.

Ok.

> > > +	mutex_unlock(&media_devnode_lock);
> > > +
> > > +	filp->private_data = mdev;
> > > +
> > > +	if (mdev->fops->open) {
> > > +		ret = mdev->fops->open(filp);
> > > +		if (ret) {
> > > +			put_device(&mdev->dev);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > No reference counting for the fops?  Why not?
> 
> Because cdev already increments the owner refcount on open().

Ok.

> > Anyway, it looks like what you really want is an "easier" way to handle
> > a cdev and a struct device that will export the proper information to
> > userspace, right?
> > 
> > Why not do this generically, fixing up the cdev interface (which really
> > needs it) and not tie it to media devices at all, making it possible for
> > _everyone_ to use this type of infrastructure?
> > 
> > That seems like the better thing to do here.
> 
> Sounds like a good idea. You're a better cdev expert than me, so could you 
> give me a few pointers ? Do you want me to create a new object that will hold 
> a struct cdev and a struct device together, or to embed the device structure 
> into the existing cdev structure ?

I don't really know, all I know is that cdev is a difficult thing to
handle at times, but not everyone who uses it needs a struct device.
But some people do (as this code shows), so I guess it needs to be a
whole new structure/interface that binds the two together like you just
did.  I think that would be good for a lot more places other than just
the media subsystem, so it should go into the core kernel instead.

thanks,

greg k-h

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Hans Verkuil]

On Thursday, January 06, 2011 23:19:12 Greg KH wrote:

<snip>

> > > > +static ssize_t media_read(struct file *filp, char __user *buf,
> > > > +		size_t sz, loff_t *off)
> > > > +{
> > > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > > +
> > > > +	if (!mdev->fops->read)
> > > > +		return -EINVAL;
> > > > +	if (!media_devnode_is_registered(mdev))
> > > > +		return -EIO;
> > > 
> > > How could this happen?
> > 
> > This can happen when a USB device is disconnected for instance.
> 
> But what's to keep that from happening on the next line as well?

Nothing.

> That
> doesn't seem like a check you can ever be sure about, so I wouldn't do
> it at all.

Actually, there is a reason why this was done for v4l (and now the media
API): typically, once a USB disconnect happens V4L drivers will call
video_unregister_device() which calls device_unregister. Afterwards the
device node should reject any new file operations except for release().

Obviously, this check can be done in the driver as well, but doing this
check in the V4L core has the advantage of 1) consistent return codes and
2) drivers no longer have to check.

Of course, since the disconnect can happen at any time drivers still need
to be able to handle errors from the USB subsystem due to disconnects, but
that is something they always have to do.

> 
> > > And are you sure -EIO is correct?
> > 
> > -ENXIO is probably better (I always confuse that with -ENODEV).

I wondered why V4L uses -EIO and I think it is related to the V4L2 specification
of the read() function:

EIO
I/O error. This indicates some hardware problem or a failure to communicate with
a remote device (USB camera etc.).

Well, I guess a disconnect can be seen as a failure to communicate :-)

I think that ENODEV is much better. After all, there is no device
anymore after a disconnect.

Is there some standard for this?

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Greg KH]

On Fri, Jan 07, 2011 at 12:27:11AM +0100, Hans Verkuil wrote:
> On Thursday, January 06, 2011 23:19:12 Greg KH wrote:
> 
> <snip>
> 
> > > > > +static ssize_t media_read(struct file *filp, char __user *buf,
> > > > > +		size_t sz, loff_t *off)
> > > > > +{
> > > > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > > > +
> > > > > +	if (!mdev->fops->read)
> > > > > +		return -EINVAL;
> > > > > +	if (!media_devnode_is_registered(mdev))
> > > > > +		return -EIO;
> > > > 
> > > > How could this happen?
> > > 
> > > This can happen when a USB device is disconnected for instance.
> > 
> > But what's to keep that from happening on the next line as well?
> 
> Nothing.
> 
> > That
> > doesn't seem like a check you can ever be sure about, so I wouldn't do
> > it at all.
> 
> Actually, there is a reason why this was done for v4l (and now the media
> API): typically, once a USB disconnect happens V4L drivers will call
> video_unregister_device() which calls device_unregister. Afterwards the
> device node should reject any new file operations except for release().
> 
> Obviously, this check can be done in the driver as well, but doing this
> check in the V4L core has the advantage of 1) consistent return codes and
> 2) drivers no longer have to check.
> 
> Of course, since the disconnect can happen at any time drivers still need
> to be able to handle errors from the USB subsystem due to disconnects, but
> that is something they always have to do.
> 
> > 
> > > > And are you sure -EIO is correct?
> > > 
> > > -ENXIO is probably better (I always confuse that with -ENODEV).
> 
> I wondered why V4L uses -EIO and I think it is related to the V4L2 specification
> of the read() function:
> 
> EIO
> I/O error. This indicates some hardware problem or a failure to communicate with
> a remote device (USB camera etc.).
> 
> Well, I guess a disconnect can be seen as a failure to communicate :-)
> 
> I think that ENODEV is much better. After all, there is no device
> anymore after a disconnect.
> 
> Is there some standard for this?

I've always thought that -ENODEV was good for when the device
disappeared.

thanks,

greg k-h

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Andy Walls]

Why, yes, there is a standard:

http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html

A somewhat verbose description of the errnos is in section 2.3.

-Andy

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>On Thursday, January 06, 2011 23:19:12 Greg KH wrote:
>
><snip>
>
>> > > > +static ssize_t media_read(struct file *filp, char __user *buf,
>> > > > +		size_t sz, loff_t *off)
>> > > > +{
>> > > > +	struct media_devnode *mdev = media_devnode_data(filp);
>> > > > +
>> > > > +	if (!mdev->fops->read)
>> > > > +		return -EINVAL;
>> > > > +	if (!media_devnode_is_registered(mdev))
>> > > > +		return -EIO;
>> > > 
>> > > How could this happen?
>> > 
>> > This can happen when a USB device is disconnected for instance.
>> 
>> But what's to keep that from happening on the next line as well?
>
>Nothing.
>
>> That
>> doesn't seem like a check you can ever be sure about, so I wouldn't do
>> it at all.
>
>Actually, there is a reason why this was done for v4l (and now the media
>API): typically, once a USB disconnect happens V4L drivers will call
>video_unregister_device() which calls device_unregister. Afterwards the
>device node should reject any new file operations except for release().
>
>Obviously, this check can be done in the driver as well, but doing this
>check in the V4L core has the advantage of 1) consistent return codes and
>2) drivers no longer have to check.
>
>Of course, since the disconnect can happen at any time drivers still need
>to be able to handle errors from the USB subsystem due to disconnects, but
>that is something they always have to do.
>
>> 
>> > > And are you sure -EIO is correct?
>> > 
>> > -ENXIO is probably better (I always confuse that with -ENODEV).
>
>I wondered why V4L uses -EIO and I think it is related to the V4L2 specification
>of the read() function:
>
>EIO
>I/O error. This indicates some hardware problem or a failure to communicate with
>a remote device (USB camera etc.).
>
>Well, I guess a disconnect can be seen as a failure to communicate :-)
>
>I think that ENODEV is much better. After all, there is no device
>anymore after a disconnect.
>
>Is there some standard for this?
>
>Regards,
>
>	Hans
>
>-- 
>Hans Verkuil - video4linux developer - sponsored by Cisco
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Jan 06, 2011 at 07:24:27PM -0500, Andy Walls wrote:
> Why, yes, there is a standard:
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html
> 
> A somewhat verbose description of the errnos is in section 2.3.

Ah, so perhaps -ENXIO is the correct thing to return with here.

thanks,

greg k-h

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Andy Walls]

Yeah well, lame-o default email client on my Android phone.

-Andy

Greg KH <gregkh@suse.de> wrote:

>
>
>A: No.
>Q: Should I include quotations after my reply?
>
>http://daringfireball.net/2007/07/on_top
>
>On Thu, Jan 06, 2011 at 07:24:27PM -0500, Andy Walls wrote:
>> Why, yes, there is a standard:
>> 
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html
>> 
>> A somewhat verbose description of the errnos is in section 2.3.
>
>Ah, so perhaps -ENXIO is the correct thing to return with here.
>
>thanks,
>
>greg k-h
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Laurent Pinchart]

Hi Greg,

On Thursday 06 January 2011 23:19:12 Greg KH wrote:
> On Fri, Dec 24, 2010 at 12:59:38PM +0100, Laurent Pinchart wrote:
> > On Thursday 23 December 2010 04:32:53 Greg KH wrote:
> > > On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> > > > +config MEDIA_CONTROLLER
> > > > +	bool "Media Controller API (EXPERIMENTAL)"
> > > > +	depends on EXPERIMENTAL
> > > > +	---help---
> > > > +	  Enable the media controller API used to query media devices
> > > > internal +	  topology and configure it dynamically.
> > > > +
> > > > +	  This API is mostly used by camera interfaces in embedded
> > > > platforms.
> > > 
> > > That's nice, but why should I enable this?  Or will drivers enable it
> > > automatically?
> > 
> > Drivers depending on the media controller API will enable this, yes. The
> > option will probably removed later when the API won't be deemed as
> > experimental anymore.
> > 
> > > > +#define MEDIA_NUM_DEVICES	256
> > > 
> > > Why this limit?
> > 
> > Because I'm using a bitmap to store the used minor numbers, and I thus
> > need a limit. I could get rid of it of it by using a linked list, but
> > that will not be efficient (you could argue that the list will hold a
> > few entries only most of the time, but in that case a limit of 256
> > minors wouldn't be a problem
> > 
> > :-)).
> 
> As it's only needed to be looked up at open() time, why not just make it
> dynamic?

How so ? With include/linux/idr.h ?

> > > > +/* Override for the open function */
> > > > +static int media_open(struct inode *inode, struct file *filp)
> > > > +{
> > > > +	struct media_devnode *mdev;
> > > > +	int ret;
> > > > +
> > > > +	/* Check if the media device is available. This needs to be done
> > > > with +	 * the media_devnode_lock held to prevent an open/unregister
> > > > race: +	 * without the lock, the device could be unregistered and
> > > > freed between +	 * the media_devnode_is_registered() and
> > > > get_device() calls, leading to +	 * a crash.
> > > > +	 */
> > > > +	mutex_lock(&media_devnode_lock);
> > > > +	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
> > > 
> > > By virtue of having the reference to the module held by the vfs, this
> > > shouldn't ever go away, even if the lock is not held.
> > 
> > inode->i_cdev is set to NULL by cdev_default_release() which can be
> > called from media_devnode_unregister(). I could move to container_of
> > outside the lock, but in that case I would have to check for mdev ==
> > NULL || !mdev_devnode_is_registered(mdev) (or move the NULL check inside
> > mdev_devnode_is_registered). Is that what you would like ?
> 
> As container_of _ALWAYS_ returns a valid pointer, you can't check it for
> NULL. I don't know, it just doesn't seem correct here, but if you are sure
> it's working properly, I'll not push the issue.

I haven't found any issue with it. I'm not sure why it would be incorrect to 
be honest. Am I missing something ?

> > > Then that's fine, but you can put the lock after the container_of(),
> > > right?
> > 
> > If I add a NULL check (as explained above), yes.
> 
> Again, you can't check for NULL as the result of container_of() that
> does not work (hint, container_of() is just pointer math, without ever
> looking at the original pointer value.)

Yes, my bad, I meant inode->i_cdev == NULL || 
!mdev_devnode_is_registered(mdev). If I moved the container_of outside of the 
locked section I would need to add an extra check inside, and I don't think 
the resulting locked section will get any smaller.

> > > > +	/* and increase the device refcount */
> > > > +	get_device(&mdev->dev);
> > > 
> > > How is that holding anything into memory?
> > 
> > That will prevent the device instance from being freed until the device
> > is closed, thereby holding both the device instance and the cdev
> > instance in memory.
> 
> Tricky :)

Tell me about it. I've spent lots of time on this issue in V4L2 a while ago. 
Fortunately refcount nightmares are getting less frequent ;-)

> > > Anyway, it looks like what you really want is an "easier" way to handle
> > > a cdev and a struct device that will export the proper information to
> > > userspace, right?
> > > 
> > > Why not do this generically, fixing up the cdev interface (which really
> > > needs it) and not tie it to media devices at all, making it possible
> > > for _everyone_ to use this type of infrastructure?
> > > 
> > > That seems like the better thing to do here.
> > 
> > Sounds like a good idea. You're a better cdev expert than me, so could
> > you give me a few pointers ? Do you want me to create a new object that
> > will hold a struct cdev and a struct device together, or to embed the
> > device structure into the existing cdev structure ?
> 
> I don't really know, all I know is that cdev is a difficult thing to
> handle at times, but not everyone who uses it needs a struct device.
> But some people do (as this code shows), so I guess it needs to be a
> whole new structure/interface that binds the two together like you just
> did.  I think that would be good for a lot more places other than just
> the media subsystem, so it should go into the core kernel instead.

There are so few direct struct cdev users in the kernel that I'm beginning to 
wonder if this is the right way to go or if I'm just using cdev in a way it 
hasn't been designed to be used. My understanding was that I needed an 
instance of struct cdev per minor, but many subsystems seems to handle cdev 
differently. Is there any detailed documentation regarding how it should be 
used ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH v7 01/12] media: Media device node support

[Raymond Yau]

2011/1/10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Hi Greg,
>
> On Thursday 06 January 2011 23:19:12 Greg KH wrote:
> > On Fri, Dec 24, 2010 at 12:59:38PM +0100, Laurent Pinchart wrote:
> > > On Thursday 23 December 2010 04:32:53 Greg KH wrote:
> > > > On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> > > > > +config MEDIA_CONTROLLER
> > > > > +       bool "Media Controller API (EXPERIMENTAL)"
> > > > > +       depends on EXPERIMENTAL
> > > > > +       ---help---
> > > > > +         Enable the media controller API used to query media
> devices
> > > > > internal +        topology and configure it dynamically.
> > > > > +
> > > > > +         This API is mostly used by camera interfaces in embedded
> > > > > platforms.
>

codecgraph and the graph function HDA-analyzer already expose the internal
topology

Do this new API allow media application to reconfigure the HDA's  audio path
of a desktop to support multi streaming playback

http://www.intel.com/support/motherboards/desktop/sb/cs-020642.htm#multistream