[RFC PATCH 0/4] Media Device Allocator API

[RFC PATCH 0/4] Media Device Allocator API

[Shuah Khan]

There are known problems with media device life time management. When media
device is released while an media ioctl is in progress, ioctls fail with
use-after-free errors and kernel hangs in some cases.

Media Device can be in any the following states:

- Allocated
- Registered (could be tied to more than one driver)
- Unregistered, not in use (media device file is not open)
- Unregistered, in use (media device file is not open)
- Released

When media device belongs to  more than one driver, registrations should be
refcounted to avoid unregistering when one of the drivers does unregister.
A refcount field in the struct media_device covers this case. Unregister on
a Media Allocator media device is a kref_put() call. The media device should
be unregistered only when the last unregister occurs.


When a media device is in use when it is unregistered, it should not be
released until the application exits when it detects the unregistered
status. Media device that is in use when it is unregistered is moved to
to_delete_list. When the last unregister occurs, media device is unregistered
and becomes an unregistered, still allocated device. Unregister marks the
device to be deleted.

When media device belongs to more than one driver, as both drivers could be
unbound/bound, driver should not end up getting stale media device that is
on its way out. Moving the unregistered media device to to_delete_list helps
this case as well.

I am sending this RFC series out for review. I tested to verify media ioctls
don't fail when media device is unregistered and it gets released when the
last reference goes. I still need to do more testing. I didb't fully test
if the media device gets deleted in all cases. So please consider this as
work in progress. I decided to send this out to get early review to see if
this solution is viable.

Shuah Khan (4):
  media: Add Media Device Allocator API
  media: Add Media Device Allocator API documentation
  media: Add refcount to keep track of media device registrations
  drivers: change au0828, uvcvideo, snd-usb-audio to use Media Device
    Allocator

 drivers/media/Makefile                 |   3 +-
 drivers/media/media-dev-allocator.c    | 153 +++++++++++++++++++++++++++++++++
 drivers/media/media-device.c           |  53 ++++++++++++
 drivers/media/media-devnode.c          |   3 +
 drivers/media/usb/au0828/au0828-core.c |   7 +-
 drivers/media/usb/au0828/au0828.h      |   1 +
 drivers/media/usb/uvc/uvc_driver.c     |  32 ++++---
 drivers/media/usb/uvc/uvcvideo.h       |   3 +-
 include/media/media-dev-allocator.h    | 113 ++++++++++++++++++++++++
 include/media/media-device.h           |  32 +++++++
 sound/usb/media.c                      |  10 ++-
 sound/usb/media.h                      |   1 +
 12 files changed, 390 insertions(+), 21 deletions(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h

-- 
2.5.0

[RFC PATCH 1/4] media: Add Media Device Allocator API

[Shuah Khan]

Add Media Device Allocator API to manage Media Device life time problems.
There are known problems with media device life time management. When media
device is released while an media ioctl is in progress, ioctls fail with
use-after-free errors and kernel hangs in some cases.

Media Allocator API provides interfaces to allocate a refcounted media
device from system wide global list and maintains the state until the
last user of the media device releases it.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/Makefile              |   3 +-
 drivers/media/media-dev-allocator.c | 153 ++++++++++++++++++++++++++++++++++++
 include/media/media-dev-allocator.h |  81 +++++++++++++++++++
 3 files changed, 236 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index e608bbc..b08f091 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -2,7 +2,8 @@
 # Makefile for the kernel multimedia device drivers.
 #
 
-media-objs	:= media-device.o media-devnode.o media-entity.o
+media-objs	:= media-device.o media-devnode.o media-entity.o \
+		   media-dev-allocator.o
 
 #
 # I2C drivers should come before other drivers, otherwise they'll fail
diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
new file mode 100644
index 0000000..51edc49
--- /dev/null
+++ b/drivers/media/media-dev-allocator.c
@@ -0,0 +1,153 @@
+/*
+ * media-devkref.c - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+/*
+ * This file adds Media Controller Device Instance with
+ * Kref support. A system wide global media device list
+ * is managed and each  media device is refcounted. The
+ * last put on the media device releases the media device
+ * instance.
+*/
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+
+#include <linux/slab.h>
+#include <linux/kref.h>
+#include <media/media-device.h>
+
+static LIST_HEAD(media_device_list);
+static LIST_HEAD(media_device_to_delete_list);
+static DEFINE_MUTEX(media_device_lock);
+
+struct media_device_instance {
+	struct media_device mdev;
+	struct list_head list;
+	struct list_head to_delete_list;
+	struct device *dev;
+	struct kref refcount;
+	bool to_delete; /* should be set when devnode is deleted */
+};
+
+static struct media_device *__media_device_get(struct device *dev,
+					       bool alloc, bool kref)
+{
+	struct media_device_instance *mdi;
+
+	mutex_lock(&media_device_lock);
+
+	list_for_each_entry(mdi, &media_device_list, list) {
+		if (mdi->dev == dev) {
+			if (kref) {
+				kref_get(&mdi->refcount);
+				pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+			}
+			goto done;
+		}
+	}
+
+	if (!alloc) {
+		mdi = NULL;
+		goto done;
+	}
+
+	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
+	if (!mdi)
+		goto done;
+
+	mdi->dev = dev;
+	kref_init(&mdi->refcount);
+	list_add_tail(&mdi->list, &media_device_list);
+	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+
+done:
+	mutex_unlock(&media_device_lock);
+
+	return mdi ? &mdi->mdev : NULL;
+}
+
+struct media_device *media_device_get(struct device *dev)
+{
+	pr_info("%s\n", __func__);
+	return __media_device_get(dev, true, true);
+}
+EXPORT_SYMBOL_GPL(media_device_get);
+
+/* Don't increment kref - this is a search and find */
+struct media_device *media_device_find(struct device *dev)
+{
+	pr_info("%s\n", __func__);
+	return __media_device_get(dev, false, false);
+}
+EXPORT_SYMBOL_GPL(media_device_find);
+
+/* don't allocate - increment kref if one is found */
+struct media_device *media_device_get_ref(struct device *dev)
+{
+	pr_info("%s\n", __func__);
+	return __media_device_get(dev, false, true);
+}
+EXPORT_SYMBOL_GPL(media_device_get_ref);
+
+static void media_device_instance_release(struct kref *kref)
+{
+	struct media_device_instance *mdi =
+		container_of(kref, struct media_device_instance, refcount);
+
+	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+
+	list_del(&mdi->list);
+	kfree(mdi);
+}
+
+void media_device_put(struct device *dev)
+{
+	struct media_device_instance *mdi;
+
+	mutex_lock(&media_device_lock);
+	/* search first in the media_device_list */
+	list_for_each_entry(mdi, &media_device_list, list) {
+		if (mdi->dev == dev) {
+			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+			kref_put(&mdi->refcount, media_device_instance_release);
+			goto done;
+		}
+	}
+	/* search in the media_device_to_delete_list */
+	list_for_each_entry(mdi, &media_device_to_delete_list, to_delete_list) {
+		if (mdi->dev == dev) {
+			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+			kref_put(&mdi->refcount, media_device_instance_release);
+			goto done;
+		}
+	}
+done:
+	mutex_unlock(&media_device_lock);
+}
+EXPORT_SYMBOL_GPL(media_device_put);
+
+void media_device_set_to_delete_state(struct device *dev)
+{
+	struct media_device_instance *mdi;
+
+	mutex_lock(&media_device_lock);
+	list_for_each_entry(mdi, &media_device_list, list) {
+		if (mdi->dev == dev) {
+			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
+			mdi->to_delete = true;
+			list_move(&mdi->list, &media_device_to_delete_list);
+			goto done;
+		}
+	}
+done:
+	mutex_unlock(&media_device_lock);
+}
+EXPORT_SYMBOL_GPL(media_device_set_to_delete_state);
+
+#endif /* CONFIG_MEDIA_CONTROLLER */
diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
new file mode 100644
index 0000000..2932c90
--- /dev/null
+++ b/include/media/media-dev-allocator.h
@@ -0,0 +1,81 @@
+/*
+ * media-devkref.c - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+/*
+ * This file adds Media Controller Device Instance with Kref support.
+ * A system wide global media device list is managed and each media
+ * device is refcounted. The last put on the media device releases
+ * the media device instance.
+*/
+
+#ifndef _MEDIA_DEV_ALLOCTOR_H
+#define _MEDIA_DEV_ALLOCTOR_H
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+/**
+ * media_device_get() - Allocate and return global media device
+ *
+ * @mdev
+ *
+ * This interface should be called to allocate media device. A new media
+ * device instance is created and added to the system wide media device
+ * instance list. If media device instance exists, media_device_get()
+ * increments the reference count and returns the media device. When
+ * more than one driver control the media device, the first driver to
+ * probe will allocate and the second driver when it calls media_device_get()
+ * it will get a reference.
+ *
+ */
+struct media_device *media_device_get(struct device *dev);
+/**
+ * media_device_get_ref() - Get reference to an allocated and registered
+ *			    media device.
+ *
+ * @mdev
+ *
+ * This interface should be called to get a reference to an allocated media
+ * device. media_open() ioctl should call this to hold a reference to ensure
+ * the media device will not be released until the media_release() does a put
+ * on it.
+ */
+struct media_device *media_device_get_ref(struct device *dev);
+/**
+ * media_device_find() - Find an allocated and registered media device.
+ *
+ * @mdev
+ *
+ * This interface should be called to find a media device. This will not
+ * incremnet the reference count.
+ */
+struct media_device *media_device_find(struct device *dev);
+/**
+ * media_device_put() - Release refcounted media device. Calls kref_put()
+ *
+ * @mdev
+ *
+ * This interface should be called to decrement refcount.
+ */
+void media_device_put(struct device *dev);
+/**
+ * media_device_set_to_delete_state() - Set the state to be deleted.
+ *
+ * @mdev
+ *
+ * This interface is used to not release the media device under from
+ * an active ioctl if unregister happens.
+ */
+void media_device_set_to_delete_state(struct device *dev);
+#else
+struct media_device *media_device_get(struct device *dev) { return NULL; }
+struct media_device *media_device_find(struct device *dev) { return NULL; }
+void media_device_put(struct media_device *mdev) { }
+void media_device_set_to_delete_state(struct device *dev) { }
+#endif /* CONFIG_MEDIA_CONTROLLER */
+#endif
-- 
2.5.0

[RFC PATCH 2/4] media: Add Media Device Allocator API documentation

[Shuah Khan]

Add Media Device Allocator API documentation.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 include/media/media-dev-allocator.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
index 2932c90..174840c 100644
--- a/include/media/media-dev-allocator.h
+++ b/include/media/media-dev-allocator.h
@@ -20,6 +20,38 @@
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 /**
+ * DOC: Media Controller Device Allocator API
+ * There are known problems with media device life time management. When media
+ * device is released while an media ioctl is in progress, ioctls fail with
+ * use-after-free errors and kernel hangs in some cases.
+ * 
+ * Media Device can be in any the following states:
+ * 
+ * - Allocated
+ * - Registered (could be tied to more than one driver)
+ * - Unregistered, not in use (media device file is not open)
+ * - Unregistered, in use (media device file is not open)
+ * - Released
+ * 
+ * When media device belongs to  more than one driver, registrations should be
+ * refcounted to avoid unregistering when one of the drivers does unregister.
+ * A refcount field in the struct media_device covers this case. Unregister on
+ * a Media Allocator media device is a kref_put() call. The media device should
+ * be unregistered only when the last unregister occurs.
+ * 
+ * When a media device is in use when it is unregistered, it should not be
+ * released until the application exits when it detects the unregistered
+ * status. Media device that is in use when it is unregistered is moved to
+ * to_delete_list. When the last unregister occurs, media device is unregistered
+ * and becomes an unregistered, still allocated device. Unregister marks the
+ * device to be deleted.
+ * 
+ * When media device belongs to more than one driver, as both drivers could be
+ * unbound/bound, driver should not end up getting stale media device that is
+ * on its way out. Moving the unregistered media device to to_delete_list helps
+ * this case as well.
+ */
+/**
  * media_device_get() - Allocate and return global media device
  *
  * @mdev
-- 
2.5.0

[RFC PATCH 3/4] media: Add refcount to keep track of media device registrations

[Shuah Khan]

Add refcount to keep track of media device registrations to avoid release
of media device when one of the drivers does unregister when media device
belongs to more than one driver. Also add a new interfaces to unregister
a media device allocated using Media Device Allocator and a hold register
refcount. Change media_open() to get media device reference and put the
reference in media_release().

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/media-device.c  | 53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/media/media-devnode.c |  3 +++
 include/media/media-device.h  | 32 ++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 93aff4e..3359235 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -36,6 +36,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-dev-allocator.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
@@ -702,6 +703,7 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->entity_notify);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
+	kref_init(&mdev->refcount);
 
 	dev_dbg(mdev->dev, "Media device initialized\n");
 }
@@ -730,6 +732,13 @@ printk("%s: mdev %p\n", __func__, mdev);
 	/* Check if mdev was ever registered at all */
 	mutex_lock(&mdev->graph_mutex);
 
+	/* if media device is already registered, bump the register refcount */
+	if (media_devnode_is_registered(&mdev->devnode)) {
+		kref_get(&mdev->refcount);
+		mutex_unlock(&mdev->graph_mutex);
+		return 0;
+	}
+
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
@@ -756,6 +765,22 @@ err:
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
+void media_device_register_ref(struct media_device *mdev)
+{
+	if (!mdev)
+		return;
+
+	pr_info("%s: mdev %p\n", __func__, mdev);
+	mutex_lock(&mdev->graph_mutex);
+
+	/* Check if mdev is registered - bump registered refcount */
+	if (media_devnode_is_registered(&mdev->devnode))
+		kref_get(&mdev->refcount);
+
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_register_ref);
+
 int __must_check media_device_register_entity_notify(struct media_device *mdev,
 					struct media_entity_notify *nptr)
 {
@@ -829,6 +854,34 @@ printk("%s: mdev=%p\n", __func__, mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
+static void __media_device_unregister_kref(struct kref *kref)
+{
+	struct media_device *mdev;
+
+	mdev = container_of(kref, struct media_device, refcount);
+	__media_device_unregister(mdev);
+}
+
+void media_device_unregister_put(struct media_device *mdev)
+{
+	int ret;
+
+	if (mdev == NULL)
+		return;
+
+	pr_info("%s: mdev=%p\n", __func__, mdev);
+	ret = kref_put_mutex(&mdev->refcount, __media_device_unregister_kref,
+			     &mdev->graph_mutex);
+	if (ret) {
+		/* __media_device_unregister() ran */
+		__media_device_cleanup(mdev);
+		mutex_unlock(&mdev->graph_mutex);
+		mutex_destroy(&mdev->graph_mutex);
+		media_device_set_to_delete_state(mdev->dev);
+	}
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_put);
+
 static void media_device_release_devres(struct device *dev, void *res)
 {
 }
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 29409f4..d1d1263 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,6 +44,7 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
+#include <media/media-dev-allocator.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -186,6 +187,7 @@ static int media_open(struct inode *inode, struct file *filp)
 		}
 	}
 
+	media_device_get_ref(mdev->parent);
 	return 0;
 }
 
@@ -201,6 +203,7 @@ static int media_release(struct inode *inode, struct file *filp)
 	   return value is ignored. */
 	put_device(&mdev->dev);
 	filp->private_data = NULL;
+	media_device_put(mdev->parent);
 	return 0;
 }
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index e59772e..64114ae 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -284,6 +284,8 @@ struct media_entity_notify {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @refcount:	Media device register reference count. Used when more
+ *		than one driver owns the device.
  * @driver_name: Optional device driver name. If not set, calls to
  *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
  *		This is needed for USB drivers for example, as otherwise
@@ -348,6 +350,7 @@ struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	struct kref refcount;
 
 	char model[32];
 	char driver_name[32];
@@ -501,6 +504,17 @@ int __must_check __media_device_register(struct media_device *mdev,
 #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
 
 /**
+ * media_device_register_ref() - Increments media device register refcount
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * When more than one driver is associated with the media device, it is
+ * necessary to refcount the number of registrations to avoid unregister
+ * when it is still in use.
+ */
+void media_device_register_ref(struct media_device *mdev);
+
+/**
  * media_device_unregister() - Unregisters a media device element
  *
  * @mdev:	pointer to struct &media_device
@@ -512,6 +526,18 @@ int __must_check __media_device_register(struct media_device *mdev,
 void media_device_unregister(struct media_device *mdev);
 
 /**
+ * media_device_unregister_put() - Unregisters a media device element
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * Should be called to unregister media device allocated with Media Device
+ * Allocator API media_device_get() interface.
+ * It is safe to call this function on an unregistered (but initialised)
+ * media device.
+ */
+void media_device_unregister_put(struct media_device *mdev);
+
+/**
  * media_device_register_entity() - registers a media entity inside a
  *	previously registered media device.
  *
@@ -681,9 +707,15 @@ static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
 }
+static inline void media_device_register_ref(struct media_device *mdev)
+{
+}
 static inline void media_device_unregister(struct media_device *mdev)
 {
 }
+static inline void media_device_unregister_put(struct media_device *mdev)
+{
+}
 static inline int media_device_register_entity(struct media_device *mdev,
 						struct media_entity *entity)
 {
-- 
2.5.0

[RFC PATCH 4/4] drivers: change au0828, uvcvideo, snd-usb-audio to use Media Device Allocator

[Shuah Khan]

Change au0828, uvcvideo, snd-usb-audio to use Media Device Allocator and
new Media Controller API media_device_unregister_put().

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c |  7 +++++--
 drivers/media/usb/au0828/au0828.h      |  1 +
 drivers/media/usb/uvc/uvc_driver.c     | 32 ++++++++++++++++++--------------
 drivers/media/usb/uvc/uvcvideo.h       |  3 ++-
 sound/usb/media.c                      | 10 +++++++---
 sound/usb/media.h                      |  1 +
 6 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 85c13ca..b410166 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -157,7 +157,8 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	dev->media_dev->enable_source = NULL;
 	dev->media_dev->disable_source = NULL;
 
-	media_device_unregister_devres(dev->media_dev);
+	media_device_unregister_put(dev->media_dev);
+	media_device_put(dev->media_dev->dev);
 	dev->media_dev = NULL;
 #endif
 }
@@ -210,7 +211,7 @@ static int au0828_media_device_init(struct au0828_dev *dev,
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev;
 
-	mdev = media_device_get_devres(&udev->dev);
+	mdev = media_device_get(&udev->dev);
 	if (!mdev)
 		return -ENOMEM;
 
@@ -485,11 +486,13 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
 		if (ret) {
+			media_device_put(dev->media_dev->dev);
 			dev_err(&udev->dev,
 				"Media Device Register Error: %d\n", ret);
 			return ret;
 		}
 	} else {
+		media_device_register_ref(dev->media_dev);
 		/*
 		 * Call au0828_media_graph_notify() to connect
 		 * audio graph to our graph. In this case, audio
diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
index 87f3284..3edd50f 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -35,6 +35,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 #include <media/media-device.h>
+#include <media/media-dev-allocator.h>
 
 /* DVB */
 #include "demux.h"
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 451e84e9..81d95b8 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1674,9 +1674,8 @@ static void uvc_delete(struct uvc_device *dev)
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(&dev->mdev.devnode))
-		media_device_unregister(&dev->mdev);
-	media_device_cleanup(&dev->mdev);
+	if (media_devnode_is_registered(&dev->mdev->devnode))
+		media_device_unregister_put(dev->mdev);
 #endif
 
 	list_for_each_safe(p, n, &dev->chains) {
@@ -1929,17 +1928,20 @@ static int uvc_probe(struct usb_interface *intf,
 
 	/* Initialize the media device and register the V4L2 device. */
 #ifdef CONFIG_MEDIA_CONTROLLER
-	dev->mdev.dev = &intf->dev;
-	strlcpy(dev->mdev.model, dev->name, sizeof(dev->mdev.model));
+	dev->mdev = media_device_get(&intf->dev);
+	if (!dev->mdev)
+		goto media_device_get_error;
+	dev->mdev->dev = &intf->dev;
+	strlcpy(dev->mdev->model, dev->name, sizeof(dev->mdev->model));
 	if (udev->serial)
-		strlcpy(dev->mdev.serial, udev->serial,
-			sizeof(dev->mdev.serial));
-	strcpy(dev->mdev.bus_info, udev->devpath);
-	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	dev->mdev.driver_version = LINUX_VERSION_CODE;
-	media_device_init(&dev->mdev);
-
-	dev->vdev.mdev = &dev->mdev;
+		strlcpy(dev->mdev->serial, udev->serial,
+			sizeof(dev->mdev->serial));
+	strcpy(dev->mdev->bus_info, udev->devpath);
+	dev->mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
+	dev->mdev->driver_version = LINUX_VERSION_CODE;
+	media_device_init(dev->mdev);
+
+	dev->vdev.mdev = dev->mdev;
 #endif
 	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
 		goto error;
@@ -1958,7 +1960,7 @@ static int uvc_probe(struct usb_interface *intf,
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 	/* Register the media device node */
-	if (media_device_register(&dev->mdev) < 0)
+	if (media_device_register(dev->mdev) < 0)
 		goto error;
 #endif
 	/* Save our data pointer in the interface data. */
@@ -1976,6 +1978,8 @@ static int uvc_probe(struct usb_interface *intf,
 	return 0;
 
 error:
+	media_device_put(&intf->dev);
+media_device_get_error:
 	uvc_unregister_video(dev);
 	return -ENODEV;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7e4d3ee..a5ef719 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -12,6 +12,7 @@
 #include <linux/uvcvideo.h>
 #include <linux/videodev2.h>
 #include <media/media-device.h>
+#include <media/media-dev-allocator.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fh.h>
@@ -543,7 +544,7 @@ struct uvc_device {
 
 	/* Video control interface */
 #ifdef CONFIG_MEDIA_CONTROLLER
-	struct media_device mdev;
+	struct media_device *mdev;
 #endif
 	struct v4l2_device vdev;
 	__u16 uvc_version;
diff --git a/sound/usb/media.c b/sound/usb/media.c
index e35af88..bd7016d 100644
--- a/sound/usb/media.c
+++ b/sound/usb/media.c
@@ -262,7 +262,7 @@ int media_snd_device_create(struct snd_usb_audio *chip,
 	struct usb_device *usbdev = interface_to_usbdev(iface);
 	int ret;
 
-	mdev = media_device_get_devres(&usbdev->dev);
+	mdev = media_device_get(&usbdev->dev);
 	if (!mdev)
 		return -ENOMEM;
 
@@ -270,15 +270,18 @@ int media_snd_device_create(struct snd_usb_audio *chip,
 	if (!mdev->dev)
 		media_device_usb_init(mdev, usbdev, NULL);
 
+	/* register if needed, otherwise get register reference */
 	if (!media_devnode_is_registered(&mdev->devnode)) {
 		ret = media_device_register(mdev);
 		if (ret) {
+			media_device_put(mdev->dev);
 			dev_err(&usbdev->dev,
 				"Couldn't register media device. Error: %d\n",
 				ret);
 			return ret;
 		}
-	}
+	} else
+		media_device_register_ref(mdev);
 
 	/* save media device - avoid lookups */
 	chip->media_dev = mdev;
@@ -312,7 +315,8 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
 	media_snd_mixer_delete(chip);
 
 	if (mdev) {
-		media_device_unregister_devres(mdev);
+		media_device_unregister_put(mdev);
+		media_device_put(mdev->dev);
 		chip->media_dev = NULL;
 	}
 }
diff --git a/sound/usb/media.h b/sound/usb/media.h
index 1dcdcdc..42ce8eb 100644
--- a/sound/usb/media.h
+++ b/sound/usb/media.h
@@ -21,6 +21,7 @@
 
 #include <media/media-device.h>
 #include <media/media-entity.h>
+#include <media/media-dev-allocator.h>
 #include <sound/asound.h>
 
 struct media_ctl {
-- 
2.5.0

Re: [RFC PATCH 1/4] media: Add Media Device Allocator API

[Joe Perches]

On Fri, 2016-03-25 at 22:38 -0600, Shuah Khan wrote:
> Add Media Device Allocator API to manage Media Device life time problems.
> There are known problems with media device life time management. When media
> device is released while an media ioctl is in progress, ioctls fail with
> use-after-free errors and kernel hangs in some cases.

Seems reasonable, thanks.

trivial:

> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
[]
> +static struct media_device *__media_device_get(struct device *dev,
> +					       bool alloc, bool kref)
> +{
[]
> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);

All of the pr_info uses here seem like debugging
and should likely be pr_debug instead.
> +struct media_device *media_device_find(struct device *dev)
> +{
> +	pr_info("%s\n", __func__);

These seem like function tracing and maybe could/should
use ftrace instead.
+/* don't allocate - increment kref if one is found */
> +struct media_device *media_device_get_ref(struct device *dev)
> +{
> +	pr_info("%s\n", __func__);

Re: [RFC PATCH 1/4] media: Add Media Device Allocator API

[Shuah Khan]

On 03/26/2016 06:50 AM, Joe Perches wrote:
> On Fri, 2016-03-25 at 22:38 -0600, Shuah Khan wrote:
>> Add Media Device Allocator API to manage Media Device life time problems.
>> There are known problems with media device life time management. When media
>> device is released while an media ioctl is in progress, ioctls fail with
>> use-after-free errors and kernel hangs in some cases.
> 
> Seems reasonable, thanks.
> 
> trivial:
> 
>> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
> []
>> +static struct media_device *__media_device_get(struct device *dev,
>> +					       bool alloc, bool kref)
>> +{
> []
>> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> 
> All of the pr_info uses here seem like debugging
> and should likely be pr_debug instead.

Correct. These are for debug and I plan to either remove them completely
or make them pr_debug().

>> +struct media_device *media_device_find(struct device *dev)
>> +{
>> +	pr_info("%s\n", __func__);
> 
> These seem like function tracing and maybe could/should
> use ftrace instead.
> +/* don't allocate - increment kref if one is found */
>> +struct media_device *media_device_get_ref(struct device *dev)
>> +{
>> +	pr_info("%s\n", __func__);
> 

Same here. This is also debug. However, you gave me an idea, this could be a
tracevent, if I find it useful for event tracing. It might be useful to be able
to track kref holds on this.

thanks,
-- Shuah

Re: [RFC PATCH 2/4] media: Add Media Device Allocator API documentation

[Mauro Carvalho Chehab]

Em Fri, 25 Mar 2016 22:38:43 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Add Media Device Allocator API documentation.

Please merge this with the previous patch.

> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  include/media/media-dev-allocator.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> index 2932c90..174840c 100644
> --- a/include/media/media-dev-allocator.h
> +++ b/include/media/media-dev-allocator.h
> @@ -20,6 +20,38 @@
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  /**
> + * DOC: Media Controller Device Allocator API
> + * There are known problems with media device life time management. When media
> + * device is released while an media ioctl is in progress, ioctls fail with
> + * use-after-free errors and kernel hangs in some cases.
> + * 
> + * Media Device can be in any the following states:
> + * 
> + * - Allocated
> + * - Registered (could be tied to more than one driver)
> + * - Unregistered, not in use (media device file is not open)
> + * - Unregistered, in use (media device file is not open)
> + * - Released
> + * 
> + * When media device belongs to  more than one driver, registrations should be
> + * refcounted to avoid unregistering when one of the drivers does unregister.
> + * A refcount field in the struct media_device covers this case. Unregister on
> + * a Media Allocator media device is a kref_put() call. The media device should
> + * be unregistered only when the last unregister occurs.
> + * 
> + * When a media device is in use when it is unregistered, it should not be
> + * released until the application exits when it detects the unregistered
> + * status. Media device that is in use when it is unregistered is moved to
> + * to_delete_list. When the last unregister occurs, media device is unregistered
> + * and becomes an unregistered, still allocated device. Unregister marks the
> + * device to be deleted.
> + * 
> + * When media device belongs to more than one driver, as both drivers could be
> + * unbound/bound, driver should not end up getting stale media device that is
> + * on its way out. Moving the unregistered media device to to_delete_list helps
> + * this case as well.
> + */
> +/**
>   * media_device_get() - Allocate and return global media device
>   *
>   * @mdev


-- 
Thanks,
Mauro

Re: [RFC PATCH 1/4] media: Add Media Device Allocator API

[Mauro Carvalho Chehab]

Hi Shuah,

I reviewed the entire patch series, but I'm adding the comments only here,
as the other patches are coherent with this one.

Em Fri, 25 Mar 2016 22:38:42 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Add Media Device Allocator API to manage Media Device life time problems.
> There are known problems with media device life time management. When media
> device is released while an media ioctl is in progress, ioctls fail with
> use-after-free errors and kernel hangs in some cases.
> 
> Media Allocator API provides interfaces to allocate a refcounted media
> device from system wide global list and maintains the state until the
> last user of the media device releases it.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/Makefile              |   3 +-
>  drivers/media/media-dev-allocator.c | 153 ++++++++++++++++++++++++++++++++++++
>  include/media/media-dev-allocator.h |  81 +++++++++++++++++++
>  3 files changed, 236 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/media-dev-allocator.c
>  create mode 100644 include/media/media-dev-allocator.h
> 
> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> index e608bbc..b08f091 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -2,7 +2,8 @@
>  # Makefile for the kernel multimedia device drivers.
>  #
>  
> -media-objs	:= media-device.o media-devnode.o media-entity.o
> +media-objs	:= media-device.o media-devnode.o media-entity.o \
> +		   media-dev-allocator.o
>  
>  #
>  # I2C drivers should come before other drivers, otherwise they'll fail
> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
> new file mode 100644
> index 0000000..51edc49
> --- /dev/null
> +++ b/drivers/media/media-dev-allocator.c
> @@ -0,0 +1,153 @@
> +/*
> + * media-devkref.c - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds Media Controller Device Instance with
> + * Kref support. A system wide global media device list
> + * is managed and each  media device is refcounted. The
> + * last put on the media device releases the media device
> + * instance.
> +*/
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER

No need for an ifdef here. This file will be compiled only
if CONFIG_MEDIA_CONTROLLER, as you added it for media.ko
dependencies at the Makefile.

> +
> +#include <linux/slab.h>
> +#include <linux/kref.h>
> +#include <media/media-device.h>
> +
> +static LIST_HEAD(media_device_list);
> +static LIST_HEAD(media_device_to_delete_list);
> +static DEFINE_MUTEX(media_device_lock);
> +
> +struct media_device_instance {
> +	struct media_device mdev;
> +	struct list_head list;
> +	struct list_head to_delete_list;
> +	struct device *dev;
> +	struct kref refcount;

> +	bool to_delete; /* should be set when devnode is deleted */

I don't think this is needed.

> +};
> +
> +static struct media_device *__media_device_get(struct device *dev,
> +					       bool alloc, bool kref)
> +{

Please don't use "kref" for a boolean here. Makes it confusing, as reviewers
would expect "kref" to be of "struct kref" type.

Also, alloc seems to be a bad name for the other bool.

Maybe you could call them as "do_alloc" and "create_kref".

> +	struct media_device_instance *mdi;
> +
> +	mutex_lock(&media_device_lock);
> +
> +	list_for_each_entry(mdi, &media_device_list, list) {
> +		if (mdi->dev == dev) {
> +			if (kref) {
> +				kref_get(&mdi->refcount);
> +				pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +			}
> +			goto done;
> +		}
> +	}
> +
> +	if (!alloc) {
> +		mdi = NULL;
> +		goto done;
> +	}
> +
> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
> +	if (!mdi)
> +		goto done;
> +
> +	mdi->dev = dev;
> +	kref_init(&mdi->refcount);
> +	list_add_tail(&mdi->list, &media_device_list);
> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> +done:
> +	mutex_unlock(&media_device_lock);
> +
> +	return mdi ? &mdi->mdev : NULL;
> +}
> +
> +struct media_device *media_device_get(struct device *dev)
> +{
> +	pr_info("%s\n", __func__);
> +	return __media_device_get(dev, true, true);
> +}
> +EXPORT_SYMBOL_GPL(media_device_get);
> +
> +/* Don't increment kref - this is a search and find */
> +struct media_device *media_device_find(struct device *dev)
> +{
> +	pr_info("%s\n", __func__);
> +	return __media_device_get(dev, false, false);
> +}
> +EXPORT_SYMBOL_GPL(media_device_find);
> +
> +/* don't allocate - increment kref if one is found */
> +struct media_device *media_device_get_ref(struct device *dev)
> +{
> +	pr_info("%s\n", __func__);
> +	return __media_device_get(dev, false, true);
> +}
> +EXPORT_SYMBOL_GPL(media_device_get_ref);
> +
> +static void media_device_instance_release(struct kref *kref)
> +{
> +	struct media_device_instance *mdi =
> +		container_of(kref, struct media_device_instance, refcount);
> +
> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> +	list_del(&mdi->list);
> +	kfree(mdi);
> +}
> +
> +void media_device_put(struct device *dev)
> +{
> +	struct media_device_instance *mdi;
> +
> +	mutex_lock(&media_device_lock);
> +	/* search first in the media_device_list */
> +	list_for_each_entry(mdi, &media_device_list, list) {
> +		if (mdi->dev == dev) {
> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +			kref_put(&mdi->refcount, media_device_instance_release);
> +			goto done;
> +		}
> +	}
> +	/* search in the media_device_to_delete_list */
> +	list_for_each_entry(mdi, &media_device_to_delete_list, to_delete_list) {
> +		if (mdi->dev == dev) {
> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +			kref_put(&mdi->refcount, media_device_instance_release);
> +			goto done;
> +		}
> +	}
> +done:
> +	mutex_unlock(&media_device_lock);
> +}
> +EXPORT_SYMBOL_GPL(media_device_put);

You also need something to initialize the kref for places where
the struct media_device is embedded, or to convert all of them to use
dynamically-allocated media_device structs.

> +
> +void media_device_set_to_delete_state(struct device *dev)
> +{
> +	struct media_device_instance *mdi;
> +
> +	mutex_lock(&media_device_lock);
> +	list_for_each_entry(mdi, &media_device_list, list) {
> +		if (mdi->dev == dev) {
> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +			mdi->to_delete = true;
> +			list_move(&mdi->list, &media_device_to_delete_list);
> +			goto done;
> +		}
> +	}
> +done:
> +	mutex_unlock(&media_device_lock);
> +}
> +EXPORT_SYMBOL_GPL(media_device_set_to_delete_state);
> +
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> new file mode 100644
> index 0000000..2932c90
> --- /dev/null
> +++ b/include/media/media-dev-allocator.h
> @@ -0,0 +1,81 @@
> +/*
> + * media-devkref.c - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds Media Controller Device Instance with Kref support.
> + * A system wide global media device list is managed and each media
> + * device is refcounted. The last put on the media device releases
> + * the media device instance.
> +*/
> +
> +#ifndef _MEDIA_DEV_ALLOCTOR_H
> +#define _MEDIA_DEV_ALLOCTOR_H
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +/**
> + * media_device_get() - Allocate and return global media device
> + *
> + * @mdev
> + *
> + * This interface should be called to allocate media device. A new media
> + * device instance is created and added to the system wide media device
> + * instance list. If media device instance exists, media_device_get()
> + * increments the reference count and returns the media device. When
> + * more than one driver control the media device, the first driver to
> + * probe will allocate and the second driver when it calls media_device_get()
> + * it will get a reference.
> + *
> + */
> +struct media_device *media_device_get(struct device *dev);
> +/**
> + * media_device_get_ref() - Get reference to an allocated and registered
> + *			    media device.
> + *
> + * @mdev
> + *
> + * This interface should be called to get a reference to an allocated media
> + * device. media_open() ioctl should call this to hold a reference to ensure
> + * the media device will not be released until the media_release() does a put
> + * on it.
> + */
> +struct media_device *media_device_get_ref(struct device *dev);
> +/**
> + * media_device_find() - Find an allocated and registered media device.
> + *
> + * @mdev
> + *
> + * This interface should be called to find a media device. This will not
> + * incremnet the reference count.
> + */
> +struct media_device *media_device_find(struct device *dev);
> +/**
> + * media_device_put() - Release refcounted media device. Calls kref_put()
> + *
> + * @mdev
> + *
> + * This interface should be called to decrement refcount.
> + */
> +void media_device_put(struct device *dev);
> +/**
> + * media_device_set_to_delete_state() - Set the state to be deleted.
> + *
> + * @mdev
> + *
> + * This interface is used to not release the media device under from
> + * an active ioctl if unregister happens.
> + */
> +void media_device_set_to_delete_state(struct device *dev);
> +#else
> +struct media_device *media_device_get(struct device *dev) { return NULL; }
> +struct media_device *media_device_find(struct device *dev) { return NULL; }
> +void media_device_put(struct media_device *mdev) { }
> +void media_device_set_to_delete_state(struct device *dev) { }
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +#endif


-- 
Thanks,
Mauro

Re: [RFC PATCH 3/4] media: Add refcount to keep track of media device registrations

[Mauro Carvalho Chehab]

Em Fri, 25 Mar 2016 22:38:44 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Add refcount to keep track of media device registrations to avoid release
> of media device when one of the drivers does unregister when media device
> belongs to more than one driver. Also add a new interfaces to unregister
> a media device allocated using Media Device Allocator and a hold register
> refcount. Change media_open() to get media device reference and put the
> reference in media_release().
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/media-device.c  | 53 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/media/media-devnode.c |  3 +++
>  include/media/media-device.h  | 32 ++++++++++++++++++++++++++
>  3 files changed, 88 insertions(+)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 93aff4e..3359235 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -36,6 +36,7 @@
>  #include <media/media-device.h>
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
> +#include <media/media-dev-allocator.h>
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  
> @@ -702,6 +703,7 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->entity_notify);
>  	mutex_init(&mdev->graph_mutex);
>  	ida_init(&mdev->entity_internal_idx);
> +	kref_init(&mdev->refcount);
>  
>  	dev_dbg(mdev->dev, "Media device initialized\n");
>  }
> @@ -730,6 +732,13 @@ printk("%s: mdev %p\n", __func__, mdev);
>  	/* Check if mdev was ever registered at all */
>  	mutex_lock(&mdev->graph_mutex);
>  
> +	/* if media device is already registered, bump the register refcount */
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		kref_get(&mdev->refcount);
> +		mutex_unlock(&mdev->graph_mutex);
> +		return 0;
> +	}
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -756,6 +765,22 @@ err:
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> +void media_device_register_ref(struct media_device *mdev)
> +{
> +	if (!mdev)
> +		return;
> +
> +	pr_info("%s: mdev %p\n", __func__, mdev);
> +	mutex_lock(&mdev->graph_mutex);
> +
> +	/* Check if mdev is registered - bump registered refcount */
> +	if (media_devnode_is_registered(&mdev->devnode))
> +		kref_get(&mdev->refcount);
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_device_register_ref);
> +
>  int __must_check media_device_register_entity_notify(struct media_device *mdev,
>  					struct media_entity_notify *nptr)
>  {
> @@ -829,6 +854,34 @@ printk("%s: mdev=%p\n", __func__, mdev);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
> +static void __media_device_unregister_kref(struct kref *kref)
> +{
> +	struct media_device *mdev;
> +
> +	mdev = container_of(kref, struct media_device, refcount);
> +	__media_device_unregister(mdev);
> +}
> +
> +void media_device_unregister_put(struct media_device *mdev)
> +{
> +	int ret;
> +
> +	if (mdev == NULL)
> +		return;
> +
> +	pr_info("%s: mdev=%p\n", __func__, mdev);
> +	ret = kref_put_mutex(&mdev->refcount, __media_device_unregister_kref,
> +			     &mdev->graph_mutex);
> +	if (ret) {
> +		/* __media_device_unregister() ran */
> +		__media_device_cleanup(mdev);
> +		mutex_unlock(&mdev->graph_mutex);
> +		mutex_destroy(&mdev->graph_mutex);
> +		media_device_set_to_delete_state(mdev->dev);

Where you're freeing the media_dev (or its container struct)?

You need to be sure that it will be freed only here.

> +	}
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_put);
> +
>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 29409f4..d1d1263 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -44,6 +44,7 @@
>  #include <linux/uaccess.h>
>  
>  #include <media/media-devnode.h>
> +#include <media/media-dev-allocator.h>
>  
>  #define MEDIA_NUM_DEVICES	256
>  #define MEDIA_NAME		"media"
> @@ -186,6 +187,7 @@ static int media_open(struct inode *inode, struct file *filp)
>  		}
>  	}
>  
> +	media_device_get_ref(mdev->parent);
>  	return 0;
>  }
>  
> @@ -201,6 +203,7 @@ static int media_release(struct inode *inode, struct file *filp)
>  	   return value is ignored. */
>  	put_device(&mdev->dev);
>  	filp->private_data = NULL;
> +	media_device_put(mdev->parent);
>  	return 0;
>  }
>  
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index e59772e..64114ae 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -284,6 +284,8 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @refcount:	Media device register reference count. Used when more
> + *		than one driver owns the device.
>   * @driver_name: Optional device driver name. If not set, calls to
>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>   *		This is needed for USB drivers for example, as otherwise
> @@ -348,6 +350,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref refcount;

You can't simply embed a kref at media_device. The problem is that
several conditions should be met for this approach to work:

1) The structs that have struct media_device should not have a kref
their own (I guess that's the current case, but it should be documented
somewhere);

2) the struct that embeds it can only be destroyed when kref refcount
reaches zero. That actually means that the core should either allocate
the struct itself or that a release callback for those structs should
do the kfree(). It means that all drivers should be changed for it to
happen.

Also, if you're adding a kref here, you likely should not have a kref
at struct media_device_instance(), as there's no need for two kref
destroy logic.

>  
>  	char model[32];
>  	char driver_name[32];
> @@ -501,6 +504,17 @@ int __must_check __media_device_register(struct media_device *mdev,
>  #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
>  
>  /**
> + * media_device_register_ref() - Increments media device register refcount
> + *
> + * @mdev:	pointer to struct &media_device
> + *
> + * When more than one driver is associated with the media device, it is
> + * necessary to refcount the number of registrations to avoid unregister
> + * when it is still in use.
> + */
> +void media_device_register_ref(struct media_device *mdev);
> +
> +/**
>   * media_device_unregister() - Unregisters a media device element
>   *
>   * @mdev:	pointer to struct &media_device
> @@ -512,6 +526,18 @@ int __must_check __media_device_register(struct media_device *mdev,
>  void media_device_unregister(struct media_device *mdev);
>  
>  /**
> + * media_device_unregister_put() - Unregisters a media device element
> + *
> + * @mdev:	pointer to struct &media_device
> + *
> + * Should be called to unregister media device allocated with Media Device
> + * Allocator API media_device_get() interface.
> + * It is safe to call this function on an unregistered (but initialised)
> + * media device.
> + */
> +void media_device_unregister_put(struct media_device *mdev);
> +
> +/**
>   * media_device_register_entity() - registers a media entity inside a
>   *	previously registered media device.
>   *
> @@ -681,9 +707,15 @@ static inline int media_device_register(struct media_device *mdev)
>  {
>  	return 0;
>  }
> +static inline void media_device_register_ref(struct media_device *mdev)
> +{
> +}
>  static inline void media_device_unregister(struct media_device *mdev)
>  {
>  }
> +static inline void media_device_unregister_put(struct media_device *mdev)
> +{
> +}
>  static inline int media_device_register_entity(struct media_device *mdev,
>  						struct media_entity *entity)
>  {


-- 
Thanks,
Mauro

Re: [RFC PATCH 4/4] drivers: change au0828, uvcvideo, snd-usb-audio to use Media Device Allocator

[Mauro Carvalho Chehab]

Em Fri, 25 Mar 2016 22:38:45 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Change au0828, uvcvideo, snd-usb-audio to use Media Device Allocator and
> new Media Controller API media_device_unregister_put().
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/usb/au0828/au0828-core.c |  7 +++++--
>  drivers/media/usb/au0828/au0828.h      |  1 +
>  drivers/media/usb/uvc/uvc_driver.c     | 32 ++++++++++++++++++--------------
>  drivers/media/usb/uvc/uvcvideo.h       |  3 ++-
>  sound/usb/media.c                      | 10 +++++++---
>  sound/usb/media.h                      |  1 +

Why this patch is not touching the other places where struct
media_device is being used?

Also, to avoid runtime troubles, I guess it should be merged with
patch 3/4.


>  6 files changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 85c13ca..b410166 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -157,7 +157,8 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
>  	dev->media_dev->enable_source = NULL;
>  	dev->media_dev->disable_source = NULL;
>  
> -	media_device_unregister_devres(dev->media_dev);
> +	media_device_unregister_put(dev->media_dev);
> +	media_device_put(dev->media_dev->dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> @@ -210,7 +211,7 @@ static int au0828_media_device_init(struct au0828_dev *dev,
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_device *mdev;
>  
> -	mdev = media_device_get_devres(&udev->dev);
> +	mdev = media_device_get(&udev->dev);
>  	if (!mdev)
>  		return -ENOMEM;
>  
> @@ -485,11 +486,13 @@ static int au0828_media_device_register(struct au0828_dev *dev,
>  		/* register media device */
>  		ret = media_device_register(dev->media_dev);
>  		if (ret) {
> +			media_device_put(dev->media_dev->dev);
>  			dev_err(&udev->dev,
>  				"Media Device Register Error: %d\n", ret);
>  			return ret;
>  		}
>  	} else {
> +		media_device_register_ref(dev->media_dev);
>  		/*
>  		 * Call au0828_media_graph_notify() to connect
>  		 * audio graph to our graph. In this case, audio
> diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
> index 87f3284..3edd50f 100644
> --- a/drivers/media/usb/au0828/au0828.h
> +++ b/drivers/media/usb/au0828/au0828.h
> @@ -35,6 +35,7 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fh.h>
>  #include <media/media-device.h>
> +#include <media/media-dev-allocator.h>
>  
>  /* DVB */
>  #include "demux.h"
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 451e84e9..81d95b8 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1674,9 +1674,8 @@ static void uvc_delete(struct uvc_device *dev)
>  	if (dev->vdev.dev)
>  		v4l2_device_unregister(&dev->vdev);
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	if (media_devnode_is_registered(&dev->mdev.devnode))
> -		media_device_unregister(&dev->mdev);
> -	media_device_cleanup(&dev->mdev);
> +	if (media_devnode_is_registered(&dev->mdev->devnode))
> +		media_device_unregister_put(dev->mdev);
>  #endif
>  
>  	list_for_each_safe(p, n, &dev->chains) {
> @@ -1929,17 +1928,20 @@ static int uvc_probe(struct usb_interface *intf,
>  
>  	/* Initialize the media device and register the V4L2 device. */
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	dev->mdev.dev = &intf->dev;
> -	strlcpy(dev->mdev.model, dev->name, sizeof(dev->mdev.model));
> +	dev->mdev = media_device_get(&intf->dev);
> +	if (!dev->mdev)
> +		goto media_device_get_error;

> +	dev->mdev->dev = &intf->dev;

I guess you don't need it, as media_device_get() should have already
stored the struct device pointer at mdev->dev.

> +	strlcpy(dev->mdev->model, dev->name, sizeof(dev->mdev->model));
>  	if (udev->serial)
> -		strlcpy(dev->mdev.serial, udev->serial,
> -			sizeof(dev->mdev.serial));
> -	strcpy(dev->mdev.bus_info, udev->devpath);
> -	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
> -	dev->mdev.driver_version = LINUX_VERSION_CODE;
> -	media_device_init(&dev->mdev);
> -
> -	dev->vdev.mdev = &dev->mdev;
> +		strlcpy(dev->mdev->serial, udev->serial,
> +			sizeof(dev->mdev->serial));
> +	strcpy(dev->mdev->bus_info, udev->devpath);
> +	dev->mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
> +	dev->mdev->driver_version = LINUX_VERSION_CODE;
> +	media_device_init(dev->mdev);

It would be better to have a previous patch calling
media_device_usb_init() before this one. That would make easier to
see the actual differences above, and will make sure that all USB
drivers would be using the same logic to initialize the media controller
data.

> +
> +	dev->vdev.mdev = dev->mdev;
>  #endif
>  	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
>  		goto error;
> @@ -1958,7 +1960,7 @@ static int uvc_probe(struct usb_interface *intf,
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	/* Register the media device node */
> -	if (media_device_register(&dev->mdev) < 0)
> +	if (media_device_register(dev->mdev) < 0)
>  		goto error;
>  #endif
>  	/* Save our data pointer in the interface data. */
> @@ -1976,6 +1978,8 @@ static int uvc_probe(struct usb_interface *intf,
>  	return 0;
>  
>  error:
> +	media_device_put(&intf->dev);
> +media_device_get_error:
>  	uvc_unregister_video(dev);
>  	return -ENODEV;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 7e4d3ee..a5ef719 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -12,6 +12,7 @@
>  #include <linux/uvcvideo.h>
>  #include <linux/videodev2.h>
>  #include <media/media-device.h>
> +#include <media/media-dev-allocator.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fh.h>
> @@ -543,7 +544,7 @@ struct uvc_device {
>  
>  	/* Video control interface */
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	struct media_device mdev;
> +	struct media_device *mdev;
>  #endif
>  	struct v4l2_device vdev;
>  	__u16 uvc_version;
> diff --git a/sound/usb/media.c b/sound/usb/media.c
> index e35af88..bd7016d 100644
> --- a/sound/usb/media.c
> +++ b/sound/usb/media.c
> @@ -262,7 +262,7 @@ int media_snd_device_create(struct snd_usb_audio *chip,
>  	struct usb_device *usbdev = interface_to_usbdev(iface);
>  	int ret;
>  
> -	mdev = media_device_get_devres(&usbdev->dev);
> +	mdev = media_device_get(&usbdev->dev);
>  	if (!mdev)
>  		return -ENOMEM;
>  
> @@ -270,15 +270,18 @@ int media_snd_device_create(struct snd_usb_audio *chip,
>  	if (!mdev->dev)
>  		media_device_usb_init(mdev, usbdev, NULL);
>  
> +	/* register if needed, otherwise get register reference */
>  	if (!media_devnode_is_registered(&mdev->devnode)) {
>  		ret = media_device_register(mdev);
>  		if (ret) {
> +			media_device_put(mdev->dev);
>  			dev_err(&usbdev->dev,
>  				"Couldn't register media device. Error: %d\n",
>  				ret);
>  			return ret;
>  		}
> -	}
> +	} else
> +		media_device_register_ref(mdev);
>  
>  	/* save media device - avoid lookups */
>  	chip->media_dev = mdev;
> @@ -312,7 +315,8 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
>  	media_snd_mixer_delete(chip);
>  
>  	if (mdev) {
> -		media_device_unregister_devres(mdev);
> +		media_device_unregister_put(mdev);
> +		media_device_put(mdev->dev);
>  		chip->media_dev = NULL;
>  	}
>  }
> diff --git a/sound/usb/media.h b/sound/usb/media.h
> index 1dcdcdc..42ce8eb 100644
> --- a/sound/usb/media.h
> +++ b/sound/usb/media.h
> @@ -21,6 +21,7 @@
>  
>  #include <media/media-device.h>
>  #include <media/media-entity.h>
> +#include <media/media-dev-allocator.h>
>  #include <sound/asound.h>
>  
>  struct media_ctl {


-- 
Thanks,
Mauro

Re: [RFC PATCH 2/4] media: Add Media Device Allocator API documentation

[Shuah Khan]

On 03/28/2016 12:28 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 25 Mar 2016 22:38:43 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Add Media Device Allocator API documentation.
> 
> Please merge this with the previous patch.

Yes. I will merge them.

-- Shuah
> 
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  include/media/media-dev-allocator.h | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
>> index 2932c90..174840c 100644
>> --- a/include/media/media-dev-allocator.h
>> +++ b/include/media/media-dev-allocator.h
>> @@ -20,6 +20,38 @@
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  /**
>> + * DOC: Media Controller Device Allocator API
>> + * There are known problems with media device life time management. When media
>> + * device is released while an media ioctl is in progress, ioctls fail with
>> + * use-after-free errors and kernel hangs in some cases.
>> + * 
>> + * Media Device can be in any the following states:
>> + * 
>> + * - Allocated
>> + * - Registered (could be tied to more than one driver)
>> + * - Unregistered, not in use (media device file is not open)
>> + * - Unregistered, in use (media device file is not open)
>> + * - Released
>> + * 
>> + * When media device belongs to  more than one driver, registrations should be
>> + * refcounted to avoid unregistering when one of the drivers does unregister.
>> + * A refcount field in the struct media_device covers this case. Unregister on
>> + * a Media Allocator media device is a kref_put() call. The media device should
>> + * be unregistered only when the last unregister occurs.
>> + * 
>> + * When a media device is in use when it is unregistered, it should not be
>> + * released until the application exits when it detects the unregistered
>> + * status. Media device that is in use when it is unregistered is moved to
>> + * to_delete_list. When the last unregister occurs, media device is unregistered
>> + * and becomes an unregistered, still allocated device. Unregister marks the
>> + * device to be deleted.
>> + * 
>> + * When media device belongs to more than one driver, as both drivers could be
>> + * unbound/bound, driver should not end up getting stale media device that is
>> + * on its way out. Moving the unregistered media device to to_delete_list helps
>> + * this case as well.
>> + */
>> +/**
>>   * media_device_get() - Allocate and return global media device
>>   *
>>   * @mdev
> 
> 

Re: [RFC PATCH 1/4] media: Add Media Device Allocator API

[Shuah Khan]

On 03/28/2016 12:28 PM, Mauro Carvalho Chehab wrote:
> Hi Shuah,
> 
> I reviewed the entire patch series, but I'm adding the comments only here,
> as the other patches are coherent with this one.

That is fine.

> 
> Em Fri, 25 Mar 2016 22:38:42 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Add Media Device Allocator API to manage Media Device life time problems.
>> There are known problems with media device life time management. When media
>> device is released while an media ioctl is in progress, ioctls fail with
>> use-after-free errors and kernel hangs in some cases.
>>
>> Media Allocator API provides interfaces to allocate a refcounted media
>> device from system wide global list and maintains the state until the
>> last user of the media device releases it.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/Makefile              |   3 +-
>>  drivers/media/media-dev-allocator.c | 153 ++++++++++++++++++++++++++++++++++++
>>  include/media/media-dev-allocator.h |  81 +++++++++++++++++++
>>  3 files changed, 236 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/media/media-dev-allocator.c
>>  create mode 100644 include/media/media-dev-allocator.h
>>
>> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
>> index e608bbc..b08f091 100644
>> --- a/drivers/media/Makefile
>> +++ b/drivers/media/Makefile
>> @@ -2,7 +2,8 @@
>>  # Makefile for the kernel multimedia device drivers.
>>  #
>>  
>> -media-objs	:= media-device.o media-devnode.o media-entity.o
>> +media-objs	:= media-device.o media-devnode.o media-entity.o \
>> +		   media-dev-allocator.o
>>  
>>  #
>>  # I2C drivers should come before other drivers, otherwise they'll fail
>> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
>> new file mode 100644
>> index 0000000..51edc49
>> --- /dev/null
>> +++ b/drivers/media/media-dev-allocator.c
>> @@ -0,0 +1,153 @@
>> +/*
>> + * media-devkref.c - Media Controller Device Allocator API
>> + *
>> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
>> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
>> + *
>> + * This file is released under the GPLv2.
>> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> + */
>> +
>> +/*
>> + * This file adds Media Controller Device Instance with
>> + * Kref support. A system wide global media device list
>> + * is managed and each  media device is refcounted. The
>> + * last put on the media device releases the media device
>> + * instance.
>> +*/
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
> 
> No need for an ifdef here. This file will be compiled only
> if CONFIG_MEDIA_CONTROLLER, as you added it for media.ko
> dependencies at the Makefile.

Right. I will remove it.

> 
>> +
>> +#include <linux/slab.h>
>> +#include <linux/kref.h>
>> +#include <media/media-device.h>
>> +
>> +static LIST_HEAD(media_device_list);
>> +static LIST_HEAD(media_device_to_delete_list);
>> +static DEFINE_MUTEX(media_device_lock);
>> +
>> +struct media_device_instance {
>> +	struct media_device mdev;
>> +	struct list_head list;
>> +	struct list_head to_delete_list;
>> +	struct device *dev;
>> +	struct kref refcount;
> 
>> +	bool to_delete; /* should be set when devnode is deleted */
> 
> I don't think this is needed.

This is necessary to handle race condition between ioctls and media
device unregister. Consider the case where the driver does unregister
while application has the device open and ioctl is in progress. In this
case, media device should not be released until the application exits
with an error detecting media device has been unregistered. All ioctls
check for this condition.

A second issue is if the media device isn't free'd, a subsequent driver
bind finds the media device that is still in the list. So this flag is
used to move the media device instance to a second to be deleted list.
We do have to make sure the media device gets deleted from this to be
deleted list in the cases when the applications dies without releasing
the reference to the media device which triggers the put.

When more than one driver is in play (e.g: au0828 and snd_usb_audio),
it is necessary to keep track of how many drivers are associated with
the media device. So we need a second kref in to do that. I saw your
comment on RFC PATCH 3/4 about embedding kref in struct media_device.
I could add another kref to the media device instance to keep track
of registrations to trigger unregister.

> 
>> +};
>> +
>> +static struct media_device *__media_device_get(struct device *dev,
>> +					       bool alloc, bool kref)
>> +{
> 
> Please don't use "kref" for a boolean here. Makes it confusing, as reviewers
> would expect "kref" to be of "struct kref" type.
> 
> Also, alloc seems to be a bad name for the other bool.
> 
> Maybe you could call them as "do_alloc" and "create_kref".

Yes. Agreed. I changed the names.

> 
>> +	struct media_device_instance *mdi;
>> +
>> +	mutex_lock(&media_device_lock);
>> +
>> +	list_for_each_entry(mdi, &media_device_list, list) {
>> +		if (mdi->dev == dev) {
>> +			if (kref) {
>> +				kref_get(&mdi->refcount);
>> +				pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +			}
>> +			goto done;
>> +		}
>> +	}
>> +
>> +	if (!alloc) {
>> +		mdi = NULL;
>> +		goto done;
>> +	}
>> +
>> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
>> +	if (!mdi)
>> +		goto done;
>> +
>> +	mdi->dev = dev;
>> +	kref_init(&mdi->refcount);
>> +	list_add_tail(&mdi->list, &media_device_list);
>> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +
>> +done:
>> +	mutex_unlock(&media_device_lock);
>> +
>> +	return mdi ? &mdi->mdev : NULL;
>> +}
>> +
>> +struct media_device *media_device_get(struct device *dev)
>> +{
>> +	pr_info("%s\n", __func__);
>> +	return __media_device_get(dev, true, true);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_get);
>> +
>> +/* Don't increment kref - this is a search and find */
>> +struct media_device *media_device_find(struct device *dev)
>> +{
>> +	pr_info("%s\n", __func__);
>> +	return __media_device_get(dev, false, false);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_find);
>> +
>> +/* don't allocate - increment kref if one is found */
>> +struct media_device *media_device_get_ref(struct device *dev)
>> +{
>> +	pr_info("%s\n", __func__);
>> +	return __media_device_get(dev, false, true);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_get_ref);
>> +
>> +static void media_device_instance_release(struct kref *kref)
>> +{
>> +	struct media_device_instance *mdi =
>> +		container_of(kref, struct media_device_instance, refcount);
>> +
>> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +
>> +	list_del(&mdi->list);
>> +	kfree(mdi);
>> +}
>> +
>> +void media_device_put(struct device *dev)
>> +{
>> +	struct media_device_instance *mdi;
>> +
>> +	mutex_lock(&media_device_lock);
>> +	/* search first in the media_device_list */
>> +	list_for_each_entry(mdi, &media_device_list, list) {
>> +		if (mdi->dev == dev) {
>> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +			kref_put(&mdi->refcount, media_device_instance_release);
>> +			goto done;
>> +		}
>> +	}
>> +	/* search in the media_device_to_delete_list */
>> +	list_for_each_entry(mdi, &media_device_to_delete_list, to_delete_list) {
>> +		if (mdi->dev == dev) {
>> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +			kref_put(&mdi->refcount, media_device_instance_release);
>> +			goto done;
>> +		}
>> +	}
>> +done:
>> +	mutex_unlock(&media_device_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_put);
> 
> You also need something to initialize the kref for places where
> the struct media_device is embedded, or to convert all of them to use
> dynamically-allocated media_device structs.

Yes. I think we need to convert all drivers to use the media device
allocator. That is the only way to solve the ioctl problems.

> 
>> +
>> +void media_device_set_to_delete_state(struct device *dev)
>> +{
>> +	struct media_device_instance *mdi;
>> +
>> +	mutex_lock(&media_device_lock);
>> +	list_for_each_entry(mdi, &media_device_list, list) {
>> +		if (mdi->dev == dev) {
>> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
>> +			mdi->to_delete = true;
>> +			list_move(&mdi->list, &media_device_to_delete_list);
>> +			goto done;
>> +		}
>> +	}
>> +done:
>> +	mutex_unlock(&media_device_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_set_to_delete_state);
>> +
>> +#endif /* CONFIG_MEDIA_CONTROLLER */
>> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
>> new file mode 100644
>> index 0000000..2932c90
>> --- /dev/null
>> +++ b/include/media/media-dev-allocator.h
>> @@ -0,0 +1,81 @@
>> +/*
>> + * media-devkref.c - Media Controller Device Allocator API
>> + *
>> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
>> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
>> + *
>> + * This file is released under the GPLv2.
>> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> + */
>> +
>> +/*
>> + * This file adds Media Controller Device Instance with Kref support.
>> + * A system wide global media device list is managed and each media
>> + * device is refcounted. The last put on the media device releases
>> + * the media device instance.
>> +*/
>> +
>> +#ifndef _MEDIA_DEV_ALLOCTOR_H
>> +#define _MEDIA_DEV_ALLOCTOR_H
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +/**
>> + * media_device_get() - Allocate and return global media device
>> + *
>> + * @mdev
>> + *
>> + * This interface should be called to allocate media device. A new media
>> + * device instance is created and added to the system wide media device
>> + * instance list. If media device instance exists, media_device_get()
>> + * increments the reference count and returns the media device. When
>> + * more than one driver control the media device, the first driver to
>> + * probe will allocate and the second driver when it calls media_device_get()
>> + * it will get a reference.
>> + *
>> + */
>> +struct media_device *media_device_get(struct device *dev);
>> +/**
>> + * media_device_get_ref() - Get reference to an allocated and registered
>> + *			    media device.
>> + *
>> + * @mdev
>> + *
>> + * This interface should be called to get a reference to an allocated media
>> + * device. media_open() ioctl should call this to hold a reference to ensure
>> + * the media device will not be released until the media_release() does a put
>> + * on it.
>> + */
>> +struct media_device *media_device_get_ref(struct device *dev);
>> +/**
>> + * media_device_find() - Find an allocated and registered media device.
>> + *
>> + * @mdev
>> + *
>> + * This interface should be called to find a media device. This will not
>> + * incremnet the reference count.
>> + */
>> +struct media_device *media_device_find(struct device *dev);
>> +/**
>> + * media_device_put() - Release refcounted media device. Calls kref_put()
>> + *
>> + * @mdev
>> + *
>> + * This interface should be called to decrement refcount.
>> + */
>> +void media_device_put(struct device *dev);
>> +/**
>> + * media_device_set_to_delete_state() - Set the state to be deleted.
>> + *
>> + * @mdev
>> + *
>> + * This interface is used to not release the media device under from
>> + * an active ioctl if unregister happens.
>> + */
>> +void media_device_set_to_delete_state(struct device *dev);
>> +#else
>> +struct media_device *media_device_get(struct device *dev) { return NULL; }
>> +struct media_device *media_device_find(struct device *dev) { return NULL; }
>> +void media_device_put(struct media_device *mdev) { }
>> +void media_device_set_to_delete_state(struct device *dev) { }
>> +#endif /* CONFIG_MEDIA_CONTROLLER */
>> +#endif
> 
> 

thanks,
-- Shuah

Re: [RFC PATCH 3/4] media: Add refcount to keep track of media device registrations

[Shuah Khan]

On 03/28/2016 12:28 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 25 Mar 2016 22:38:44 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Add refcount to keep track of media device registrations to avoid release
>> of media device when one of the drivers does unregister when media device
>> belongs to more than one driver. Also add a new interfaces to unregister
>> a media device allocated using Media Device Allocator and a hold register
>> refcount. Change media_open() to get media device reference and put the
>> reference in media_release().
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/media-device.c  | 53 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/media/media-devnode.c |  3 +++
>>  include/media/media-device.h  | 32 ++++++++++++++++++++++++++
>>  3 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 93aff4e..3359235 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -36,6 +36,7 @@
>>  #include <media/media-device.h>
>>  #include <media/media-devnode.h>
>>  #include <media/media-entity.h>
>> +#include <media/media-dev-allocator.h>
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  
>> @@ -702,6 +703,7 @@ void media_device_init(struct media_device *mdev)
>>  	INIT_LIST_HEAD(&mdev->entity_notify);
>>  	mutex_init(&mdev->graph_mutex);
>>  	ida_init(&mdev->entity_internal_idx);
>> +	kref_init(&mdev->refcount);
>>  
>>  	dev_dbg(mdev->dev, "Media device initialized\n");
>>  }
>> @@ -730,6 +732,13 @@ printk("%s: mdev %p\n", __func__, mdev);
>>  	/* Check if mdev was ever registered at all */
>>  	mutex_lock(&mdev->graph_mutex);
>>  
>> +	/* if media device is already registered, bump the register refcount */
>> +	if (media_devnode_is_registered(&mdev->devnode)) {
>> +		kref_get(&mdev->refcount);
>> +		mutex_unlock(&mdev->graph_mutex);
>> +		return 0;
>> +	}
>> +
>>  	/* Register the device node. */
>>  	mdev->devnode.fops = &media_device_fops;
>>  	mdev->devnode.parent = mdev->dev;
>> @@ -756,6 +765,22 @@ err:
>>  }
>>  EXPORT_SYMBOL_GPL(__media_device_register);
>>  
>> +void media_device_register_ref(struct media_device *mdev)
>> +{
>> +	if (!mdev)
>> +		return;
>> +
>> +	pr_info("%s: mdev %p\n", __func__, mdev);
>> +	mutex_lock(&mdev->graph_mutex);
>> +
>> +	/* Check if mdev is registered - bump registered refcount */
>> +	if (media_devnode_is_registered(&mdev->devnode))
>> +		kref_get(&mdev->refcount);
>> +
>> +	mutex_unlock(&mdev->graph_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_register_ref);
>> +
>>  int __must_check media_device_register_entity_notify(struct media_device *mdev,
>>  					struct media_entity_notify *nptr)
>>  {
>> @@ -829,6 +854,34 @@ printk("%s: mdev=%p\n", __func__, mdev);
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_unregister);
>>  
>> +static void __media_device_unregister_kref(struct kref *kref)
>> +{
>> +	struct media_device *mdev;
>> +
>> +	mdev = container_of(kref, struct media_device, refcount);
>> +	__media_device_unregister(mdev);
>> +}
>> +
>> +void media_device_unregister_put(struct media_device *mdev)
>> +{
>> +	int ret;
>> +
>> +	if (mdev == NULL)
>> +		return;
>> +
>> +	pr_info("%s: mdev=%p\n", __func__, mdev);
>> +	ret = kref_put_mutex(&mdev->refcount, __media_device_unregister_kref,
>> +			     &mdev->graph_mutex);
>> +	if (ret) {
>> +		/* __media_device_unregister() ran */
>> +		__media_device_cleanup(mdev);
>> +		mutex_unlock(&mdev->graph_mutex);
>> +		mutex_destroy(&mdev->graph_mutex);
>> +		media_device_set_to_delete_state(mdev->dev);
> 
> Where you're freeing the media_dev (or its container struct)?
> 
> You need to be sure that it will be freed only here.

This step doesn't free the media device. It simply move it from
the main media device instance list to the to be deleted list.

This is necessary to handle race condition between ioctls and media
device unregister. Consider the case where the driver does unregister
while application has the device open and ioctl is in progress. In this
case, media device should not be released until the application exits
with an error detecting media device has been unregistered. All ioctls
check for this condition.

A second issue is if the media device isn't free'd, a subsequent driver
bind finds the media device that is still in the list. So this flag is
used to move the media device instance to a second to be deleted list.
We do have to make sure the media device gets deleted from this to be
deleted list in the cases when the applications dies without releasing
the reference to the media device which triggers the put.

> 
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_unregister_put);
>> +
>>  static void media_device_release_devres(struct device *dev, void *res)
>>  {
>>  }
>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>> index 29409f4..d1d1263 100644
>> --- a/drivers/media/media-devnode.c
>> +++ b/drivers/media/media-devnode.c
>> @@ -44,6 +44,7 @@
>>  #include <linux/uaccess.h>
>>  
>>  #include <media/media-devnode.h>
>> +#include <media/media-dev-allocator.h>
>>  
>>  #define MEDIA_NUM_DEVICES	256
>>  #define MEDIA_NAME		"media"
>> @@ -186,6 +187,7 @@ static int media_open(struct inode *inode, struct file *filp)
>>  		}
>>  	}
>>  
>> +	media_device_get_ref(mdev->parent);
>>  	return 0;
>>  }
>>  
>> @@ -201,6 +203,7 @@ static int media_release(struct inode *inode, struct file *filp)
>>  	   return value is ignored. */
>>  	put_device(&mdev->dev);
>>  	filp->private_data = NULL;
>> +	media_device_put(mdev->parent);
>>  	return 0;
>>  }
>>  
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index e59772e..64114ae 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -284,6 +284,8 @@ struct media_entity_notify {
>>   * struct media_device - Media device
>>   * @dev:	Parent device
>>   * @devnode:	Media device node
>> + * @refcount:	Media device register reference count. Used when more
>> + *		than one driver owns the device.
>>   * @driver_name: Optional device driver name. If not set, calls to
>>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>>   *		This is needed for USB drivers for example, as otherwise
>> @@ -348,6 +350,7 @@ struct media_device {
>>  	/* dev->driver_data points to this struct. */
>>  	struct device *dev;
>>  	struct media_devnode devnode;
>> +	struct kref refcount;
> 
> You can't simply embed a kref at media_device. The problem is that
> several conditions should be met for this approach to work:
> 
> 1) The structs that have struct media_device should not have a kref
> their own (I guess that's the current case, but it should be documented
> somewhere);
> 
> 2) the struct that embeds it can only be destroyed when kref refcount
> reaches zero. That actually means that the core should either allocate
> the struct itself or that a release callback for those structs should
> do the kfree(). It means that all drivers should be changed for it to
> happen.
> 
> Also, if you're adding a kref here, you likely should not have a kref
> at struct media_device_instance(), as there's no need for two kref
> destroy logic.

When more than one driver is in play (e.g: au0828 and snd_usb_audio),
it is necessary to keep track of how many drivers are associated with
the media device. So we need a second kref in to do that.

I could add another kref to the media device instance to keep track
of registrations to trigger unregister.

> 
>>  
>>  	char model[32];
>>  	char driver_name[32];
>> @@ -501,6 +504,17 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
>>  
>>  /**
>> + * media_device_register_ref() - Increments media device register refcount
>> + *
>> + * @mdev:	pointer to struct &media_device
>> + *
>> + * When more than one driver is associated with the media device, it is
>> + * necessary to refcount the number of registrations to avoid unregister
>> + * when it is still in use.
>> + */
>> +void media_device_register_ref(struct media_device *mdev);
>> +
>> +/**
>>   * media_device_unregister() - Unregisters a media device element
>>   *
>>   * @mdev:	pointer to struct &media_device
>> @@ -512,6 +526,18 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  void media_device_unregister(struct media_device *mdev);
>>  
>>  /**
>> + * media_device_unregister_put() - Unregisters a media device element
>> + *
>> + * @mdev:	pointer to struct &media_device
>> + *
>> + * Should be called to unregister media device allocated with Media Device
>> + * Allocator API media_device_get() interface.
>> + * It is safe to call this function on an unregistered (but initialised)
>> + * media device.
>> + */
>> +void media_device_unregister_put(struct media_device *mdev);
>> +
>> +/**
>>   * media_device_register_entity() - registers a media entity inside a
>>   *	previously registered media device.
>>   *
>> @@ -681,9 +707,15 @@ static inline int media_device_register(struct media_device *mdev)
>>  {
>>  	return 0;
>>  }
>> +static inline void media_device_register_ref(struct media_device *mdev)
>> +{
>> +}
>>  static inline void media_device_unregister(struct media_device *mdev)
>>  {
>>  }
>> +static inline void media_device_unregister_put(struct media_device *mdev)
>> +{
>> +}
>>  static inline int media_device_register_entity(struct media_device *mdev,
>>  						struct media_entity *entity)
>>  {
> 
> 

thanks,
-- Shuah

Re: [RFC PATCH 1/4] media: Add Media Device Allocator API

[Mauro Carvalho Chehab]

Em Mon, 28 Mar 2016 15:34:15 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 03/28/2016 12:28 PM, Mauro Carvalho Chehab wrote:
> > Hi Shuah,
> > 
> > I reviewed the entire patch series, but I'm adding the comments only here,
> > as the other patches are coherent with this one.  
> 
> That is fine.
> 
> > 
> > Em Fri, 25 Mar 2016 22:38:42 -0600
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >   
> >> Add Media Device Allocator API to manage Media Device life time problems.
> >> There are known problems with media device life time management. When media
> >> device is released while an media ioctl is in progress, ioctls fail with
> >> use-after-free errors and kernel hangs in some cases.
> >>
> >> Media Allocator API provides interfaces to allocate a refcounted media
> >> device from system wide global list and maintains the state until the
> >> last user of the media device releases it.
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >>  drivers/media/Makefile              |   3 +-
> >>  drivers/media/media-dev-allocator.c | 153 ++++++++++++++++++++++++++++++++++++
> >>  include/media/media-dev-allocator.h |  81 +++++++++++++++++++
> >>  3 files changed, 236 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/media/media-dev-allocator.c
> >>  create mode 100644 include/media/media-dev-allocator.h
> >>
> >> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> >> index e608bbc..b08f091 100644
> >> --- a/drivers/media/Makefile
> >> +++ b/drivers/media/Makefile
> >> @@ -2,7 +2,8 @@
> >>  # Makefile for the kernel multimedia device drivers.
> >>  #
> >>  
> >> -media-objs	:= media-device.o media-devnode.o media-entity.o
> >> +media-objs	:= media-device.o media-devnode.o media-entity.o \
> >> +		   media-dev-allocator.o
> >>  
> >>  #
> >>  # I2C drivers should come before other drivers, otherwise they'll fail
> >> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
> >> new file mode 100644
> >> index 0000000..51edc49
> >> --- /dev/null
> >> +++ b/drivers/media/media-dev-allocator.c
> >> @@ -0,0 +1,153 @@
> >> +/*
> >> + * media-devkref.c - Media Controller Device Allocator API
> >> + *
> >> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> >> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> >> + *
> >> + * This file is released under the GPLv2.
> >> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> + */
> >> +
> >> +/*
> >> + * This file adds Media Controller Device Instance with
> >> + * Kref support. A system wide global media device list
> >> + * is managed and each  media device is refcounted. The
> >> + * last put on the media device releases the media device
> >> + * instance.
> >> +*/
> >> +
> >> +#ifdef CONFIG_MEDIA_CONTROLLER  
> > 
> > No need for an ifdef here. This file will be compiled only
> > if CONFIG_MEDIA_CONTROLLER, as you added it for media.ko
> > dependencies at the Makefile.  
> 
> Right. I will remove it.
> 
> >   
> >> +
> >> +#include <linux/slab.h>
> >> +#include <linux/kref.h>
> >> +#include <media/media-device.h>
> >> +
> >> +static LIST_HEAD(media_device_list);
> >> +static LIST_HEAD(media_device_to_delete_list);
> >> +static DEFINE_MUTEX(media_device_lock);
> >> +
> >> +struct media_device_instance {
> >> +	struct media_device mdev;
> >> +	struct list_head list;
> >> +	struct list_head to_delete_list;
> >> +	struct device *dev;
> >> +	struct kref refcount;  
> >   
> >> +	bool to_delete; /* should be set when devnode is deleted */  
> > 
> > I don't think this is needed.  
> 
> This is necessary to handle race condition between ioctls and media
> device unregister. Consider the case where the driver does unregister
> while application has the device open and ioctl is in progress. 

That is easy to handle: just take the graph_lock at the ioctl code
and at the unregister code. This will serialize unregistering.

> In this
> case, media device should not be released until the application exits
> with an error detecting media device has been unregistered. All ioctls
> check for this condition.
> 
> A second issue is if the media device isn't free'd, a subsequent driver
> bind finds the media device that is still in the list.

Again, you don't need a newer flag, as there's already one flag at
media_devnode that it is used for such case.

For media_device, this can be safely freed if properly locked, even
if media_devnode is not freed.

A new flag here just makes things more complicated, non-standard and
may lead into errors.

> So this flag is
> used to move the media device instance to a second to be deleted list.
> We do have to make sure the media device gets deleted from this to be
> deleted list in the cases when the applications dies without releasing
> the reference to the media device which triggers the put.
> 
> When more than one driver is in play (e.g: au0828 and snd_usb_audio),
> it is necessary to keep track of how many drivers are associated with
> the media device. 

That's exactly what kref does.

> So we need a second kref in to do that. 

I'm not convinced why you need two krefs. Just one seems to be enough.

The more complex it is, the easier to get it wrong.

IMHO, we should first solve the race issues at the MC core, and then put the
media_device allocation in a linked list, and not the reverse, as otherwise
we'll need to do lots of cleanups.

So, IMHO, we should first:

- apply the media_lock patch I did (folded with the fixup);
- the patch that dynamically allocate media_devnode;
- fix the cdev stuff;
- add more patches to fix the remaining MC core races.

After that, apply that kref patch I wrote and convert the *_devres into
a linked list.

> I saw your comment on RFC PATCH 3/4 about embedding kref in struct media_device.
> I could add another kref to the media device instance to keep track
> of registrations to trigger unregister.
> 
> >   
> >> +};
> >> +
> >> +static struct media_device *__media_device_get(struct device *dev,
> >> +					       bool alloc, bool kref)
> >> +{  
> > 
> > Please don't use "kref" for a boolean here. Makes it confusing, as reviewers
> > would expect "kref" to be of "struct kref" type.
> > 
> > Also, alloc seems to be a bad name for the other bool.
> > 
> > Maybe you could call them as "do_alloc" and "create_kref".  
> 
> Yes. Agreed. I changed the names.
> 
> >   
> >> +	struct media_device_instance *mdi;
> >> +
> >> +	mutex_lock(&media_device_lock);
> >> +
> >> +	list_for_each_entry(mdi, &media_device_list, list) {
> >> +		if (mdi->dev == dev) {
> >> +			if (kref) {
> >> +				kref_get(&mdi->refcount);
> >> +				pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +			}
> >> +			goto done;
> >> +		}
> >> +	}
> >> +
> >> +	if (!alloc) {
> >> +		mdi = NULL;
> >> +		goto done;
> >> +	}
> >> +
> >> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
> >> +	if (!mdi)
> >> +		goto done;
> >> +
> >> +	mdi->dev = dev;
> >> +	kref_init(&mdi->refcount);
> >> +	list_add_tail(&mdi->list, &media_device_list);
> >> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +
> >> +done:
> >> +	mutex_unlock(&media_device_lock);
> >> +
> >> +	return mdi ? &mdi->mdev : NULL;
> >> +}
> >> +
> >> +struct media_device *media_device_get(struct device *dev)
> >> +{
> >> +	pr_info("%s\n", __func__);
> >> +	return __media_device_get(dev, true, true);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_get);
> >> +
> >> +/* Don't increment kref - this is a search and find */
> >> +struct media_device *media_device_find(struct device *dev)
> >> +{
> >> +	pr_info("%s\n", __func__);
> >> +	return __media_device_get(dev, false, false);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_find);
> >> +
> >> +/* don't allocate - increment kref if one is found */
> >> +struct media_device *media_device_get_ref(struct device *dev)
> >> +{
> >> +	pr_info("%s\n", __func__);
> >> +	return __media_device_get(dev, false, true);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_get_ref);
> >> +
> >> +static void media_device_instance_release(struct kref *kref)
> >> +{
> >> +	struct media_device_instance *mdi =
> >> +		container_of(kref, struct media_device_instance, refcount);
> >> +
> >> +	pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +
> >> +	list_del(&mdi->list);
> >> +	kfree(mdi);
> >> +}
> >> +
> >> +void media_device_put(struct device *dev)
> >> +{
> >> +	struct media_device_instance *mdi;
> >> +
> >> +	mutex_lock(&media_device_lock);
> >> +	/* search first in the media_device_list */
> >> +	list_for_each_entry(mdi, &media_device_list, list) {
> >> +		if (mdi->dev == dev) {
> >> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +			kref_put(&mdi->refcount, media_device_instance_release);
> >> +			goto done;
> >> +		}
> >> +	}
> >> +	/* search in the media_device_to_delete_list */
> >> +	list_for_each_entry(mdi, &media_device_to_delete_list, to_delete_list) {
> >> +		if (mdi->dev == dev) {
> >> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +			kref_put(&mdi->refcount, media_device_instance_release);
> >> +			goto done;
> >> +		}
> >> +	}
> >> +done:
> >> +	mutex_unlock(&media_device_lock);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_put);  
> > 
> > You also need something to initialize the kref for places where
> > the struct media_device is embedded, or to convert all of them to use
> > dynamically-allocated media_device structs.  
> 
> Yes. I think we need to convert all drivers to use the media device
> allocator. That is the only way to solve the ioctl problems.

No. Fixing ioctl problems can only solved by doing dynamic allocation of
media_devnode.

> 
> >   
> >> +
> >> +void media_device_set_to_delete_state(struct device *dev)
> >> +{
> >> +	struct media_device_instance *mdi;
> >> +
> >> +	mutex_lock(&media_device_lock);
> >> +	list_for_each_entry(mdi, &media_device_list, list) {
> >> +		if (mdi->dev == dev) {
> >> +			pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> >> +			mdi->to_delete = true;
> >> +			list_move(&mdi->list, &media_device_to_delete_list);
> >> +			goto done;
> >> +		}
> >> +	}
> >> +done:
> >> +	mutex_unlock(&media_device_lock);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_set_to_delete_state);
> >> +
> >> +#endif /* CONFIG_MEDIA_CONTROLLER */
> >> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> >> new file mode 100644
> >> index 0000000..2932c90
> >> --- /dev/null
> >> +++ b/include/media/media-dev-allocator.h
> >> @@ -0,0 +1,81 @@
> >> +/*
> >> + * media-devkref.c - Media Controller Device Allocator API
> >> + *
> >> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> >> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> >> + *
> >> + * This file is released under the GPLv2.
> >> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> + */
> >> +
> >> +/*
> >> + * This file adds Media Controller Device Instance with Kref support.
> >> + * A system wide global media device list is managed and each media
> >> + * device is refcounted. The last put on the media device releases
> >> + * the media device instance.
> >> +*/
> >> +
> >> +#ifndef _MEDIA_DEV_ALLOCTOR_H
> >> +#define _MEDIA_DEV_ALLOCTOR_H
> >> +
> >> +#ifdef CONFIG_MEDIA_CONTROLLER
> >> +/**
> >> + * media_device_get() - Allocate and return global media device
> >> + *
> >> + * @mdev
> >> + *
> >> + * This interface should be called to allocate media device. A new media
> >> + * device instance is created and added to the system wide media device
> >> + * instance list. If media device instance exists, media_device_get()
> >> + * increments the reference count and returns the media device. When
> >> + * more than one driver control the media device, the first driver to
> >> + * probe will allocate and the second driver when it calls media_device_get()
> >> + * it will get a reference.
> >> + *
> >> + */
> >> +struct media_device *media_device_get(struct device *dev);
> >> +/**
> >> + * media_device_get_ref() - Get reference to an allocated and registered
> >> + *			    media device.
> >> + *
> >> + * @mdev
> >> + *
> >> + * This interface should be called to get a reference to an allocated media
> >> + * device. media_open() ioctl should call this to hold a reference to ensure
> >> + * the media device will not be released until the media_release() does a put
> >> + * on it.
> >> + */
> >> +struct media_device *media_device_get_ref(struct device *dev);
> >> +/**
> >> + * media_device_find() - Find an allocated and registered media device.
> >> + *
> >> + * @mdev
> >> + *
> >> + * This interface should be called to find a media device. This will not
> >> + * incremnet the reference count.
> >> + */
> >> +struct media_device *media_device_find(struct device *dev);
> >> +/**
> >> + * media_device_put() - Release refcounted media device. Calls kref_put()
> >> + *
> >> + * @mdev
> >> + *
> >> + * This interface should be called to decrement refcount.
> >> + */
> >> +void media_device_put(struct device *dev);
> >> +/**
> >> + * media_device_set_to_delete_state() - Set the state to be deleted.
> >> + *
> >> + * @mdev
> >> + *
> >> + * This interface is used to not release the media device under from
> >> + * an active ioctl if unregister happens.
> >> + */
> >> +void media_device_set_to_delete_state(struct device *dev);
> >> +#else
> >> +struct media_device *media_device_get(struct device *dev) { return NULL; }
> >> +struct media_device *media_device_find(struct device *dev) { return NULL; }
> >> +void media_device_put(struct media_device *mdev) { }
> >> +void media_device_set_to_delete_state(struct device *dev) { }
> >> +#endif /* CONFIG_MEDIA_CONTROLLER */
> >> +#endif  
> > 
> >   
> 
> thanks,
> -- Shuah
> 
> --
> 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


-- 

Cheers,
Mauro

Re: [RFC PATCH 3/4] media: Add refcount to keep track of media device registrations

[Mauro Carvalho Chehab]

Em Mon, 28 Mar 2016 15:37:43 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 03/28/2016 12:28 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 25 Mar 2016 22:38:44 -0600
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >   
> >> Add refcount to keep track of media device registrations to avoid release
> >> of media device when one of the drivers does unregister when media device
> >> belongs to more than one driver. Also add a new interfaces to unregister
> >> a media device allocated using Media Device Allocator and a hold register
> >> refcount. Change media_open() to get media device reference and put the
> >> reference in media_release().
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >>  drivers/media/media-device.c  | 53 +++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/media/media-devnode.c |  3 +++
> >>  include/media/media-device.h  | 32 ++++++++++++++++++++++++++
> >>  3 files changed, 88 insertions(+)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 93aff4e..3359235 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -36,6 +36,7 @@
> >>  #include <media/media-device.h>
> >>  #include <media/media-devnode.h>
> >>  #include <media/media-entity.h>
> >> +#include <media/media-dev-allocator.h>
> >>  
> >>  #ifdef CONFIG_MEDIA_CONTROLLER
> >>  
> >> @@ -702,6 +703,7 @@ void media_device_init(struct media_device *mdev)
> >>  	INIT_LIST_HEAD(&mdev->entity_notify);
> >>  	mutex_init(&mdev->graph_mutex);
> >>  	ida_init(&mdev->entity_internal_idx);
> >> +	kref_init(&mdev->refcount);
> >>  
> >>  	dev_dbg(mdev->dev, "Media device initialized\n");
> >>  }
> >> @@ -730,6 +732,13 @@ printk("%s: mdev %p\n", __func__, mdev);
> >>  	/* Check if mdev was ever registered at all */
> >>  	mutex_lock(&mdev->graph_mutex);
> >>  
> >> +	/* if media device is already registered, bump the register refcount */
> >> +	if (media_devnode_is_registered(&mdev->devnode)) {
> >> +		kref_get(&mdev->refcount);
> >> +		mutex_unlock(&mdev->graph_mutex);
> >> +		return 0;
> >> +	}
> >> +
> >>  	/* Register the device node. */
> >>  	mdev->devnode.fops = &media_device_fops;
> >>  	mdev->devnode.parent = mdev->dev;
> >> @@ -756,6 +765,22 @@ err:
> >>  }
> >>  EXPORT_SYMBOL_GPL(__media_device_register);
> >>  
> >> +void media_device_register_ref(struct media_device *mdev)
> >> +{
> >> +	if (!mdev)
> >> +		return;
> >> +
> >> +	pr_info("%s: mdev %p\n", __func__, mdev);
> >> +	mutex_lock(&mdev->graph_mutex);
> >> +
> >> +	/* Check if mdev is registered - bump registered refcount */
> >> +	if (media_devnode_is_registered(&mdev->devnode))
> >> +		kref_get(&mdev->refcount);
> >> +
> >> +	mutex_unlock(&mdev->graph_mutex);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_register_ref);
> >> +
> >>  int __must_check media_device_register_entity_notify(struct media_device *mdev,
> >>  					struct media_entity_notify *nptr)
> >>  {
> >> @@ -829,6 +854,34 @@ printk("%s: mdev=%p\n", __func__, mdev);
> >>  }
> >>  EXPORT_SYMBOL_GPL(media_device_unregister);
> >>  
> >> +static void __media_device_unregister_kref(struct kref *kref)
> >> +{
> >> +	struct media_device *mdev;
> >> +
> >> +	mdev = container_of(kref, struct media_device, refcount);
> >> +	__media_device_unregister(mdev);
> >> +}
> >> +
> >> +void media_device_unregister_put(struct media_device *mdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (mdev == NULL)
> >> +		return;
> >> +
> >> +	pr_info("%s: mdev=%p\n", __func__, mdev);
> >> +	ret = kref_put_mutex(&mdev->refcount, __media_device_unregister_kref,
> >> +			     &mdev->graph_mutex);
> >> +	if (ret) {
> >> +		/* __media_device_unregister() ran */
> >> +		__media_device_cleanup(mdev);
> >> +		mutex_unlock(&mdev->graph_mutex);
> >> +		mutex_destroy(&mdev->graph_mutex);
> >> +		media_device_set_to_delete_state(mdev->dev);  
> > 
> > Where you're freeing the media_dev (or its container struct)?
> > 
> > You need to be sure that it will be freed only here.  
> 
> This step doesn't free the media device. It simply move it from
> the main media device instance list to the to be deleted list.
> 
> This is necessary to handle race condition between ioctls and media
> device unregister. Consider the case where the driver does unregister
> while application has the device open and ioctl is in progress.

This can never happen, provided that the data is properly locked.

And if it is not locked, there will be other race issues.

> In this
> case, media device should not be released until the application exits
> with an error detecting media device has been unregistered. All ioctls
> check for this condition.
> 
> A second issue is if the media device isn't free'd, a subsequent driver
> bind finds the media device that is still in the list. So this flag is
> used to move the media device instance to a second to be deleted list.
> We do have to make sure the media device gets deleted from this to be
> deleted list in the cases when the applications dies without releasing
> the reference to the media device which triggers the put.

See my previous e-mail.

> 
> >   
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_device_unregister_put);
> >> +
> >>  static void media_device_release_devres(struct device *dev, void *res)
> >>  {
> >>  }
> >> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> >> index 29409f4..d1d1263 100644
> >> --- a/drivers/media/media-devnode.c
> >> +++ b/drivers/media/media-devnode.c
> >> @@ -44,6 +44,7 @@
> >>  #include <linux/uaccess.h>
> >>  
> >>  #include <media/media-devnode.h>
> >> +#include <media/media-dev-allocator.h>
> >>  
> >>  #define MEDIA_NUM_DEVICES	256
> >>  #define MEDIA_NAME		"media"
> >> @@ -186,6 +187,7 @@ static int media_open(struct inode *inode, struct file *filp)
> >>  		}
> >>  	}
> >>  
> >> +	media_device_get_ref(mdev->parent);
> >>  	return 0;
> >>  }
> >>  
> >> @@ -201,6 +203,7 @@ static int media_release(struct inode *inode, struct file *filp)
> >>  	   return value is ignored. */
> >>  	put_device(&mdev->dev);
> >>  	filp->private_data = NULL;
> >> +	media_device_put(mdev->parent);
> >>  	return 0;
> >>  }
> >>  
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index e59772e..64114ae 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -284,6 +284,8 @@ struct media_entity_notify {
> >>   * struct media_device - Media device
> >>   * @dev:	Parent device
> >>   * @devnode:	Media device node
> >> + * @refcount:	Media device register reference count. Used when more
> >> + *		than one driver owns the device.
> >>   * @driver_name: Optional device driver name. If not set, calls to
> >>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
> >>   *		This is needed for USB drivers for example, as otherwise
> >> @@ -348,6 +350,7 @@ struct media_device {
> >>  	/* dev->driver_data points to this struct. */
> >>  	struct device *dev;
> >>  	struct media_devnode devnode;
> >> +	struct kref refcount;  
> > 
> > You can't simply embed a kref at media_device. The problem is that
> > several conditions should be met for this approach to work:
> > 
> > 1) The structs that have struct media_device should not have a kref
> > their own (I guess that's the current case, but it should be documented
> > somewhere);
> > 
> > 2) the struct that embeds it can only be destroyed when kref refcount
> > reaches zero. That actually means that the core should either allocate
> > the struct itself or that a release callback for those structs should
> > do the kfree(). It means that all drivers should be changed for it to
> > happen.
> > 
> > Also, if you're adding a kref here, you likely should not have a kref
> > at struct media_device_instance(), as there's no need for two kref
> > destroy logic.  
> 
> When more than one driver is in play (e.g: au0828 and snd_usb_audio),
> it is necessary to keep track of how many drivers are associated with
> the media device. So we need a second kref in to do that.

Why two krefs? It is one struct that needs to be tracked, so just one
kref should be enough. 

> 
> I could add another kref to the media device instance to keep track
> of registrations to trigger unregister.

No! don't add lots of kref. Just one is enough. Complex solutions won't make
it any better.

> >   
> >>  
> >>  	char model[32];
> >>  	char driver_name[32];
> >> @@ -501,6 +504,17 @@ int __must_check __media_device_register(struct media_device *mdev,
> >>  #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
> >>  
> >>  /**
> >> + * media_device_register_ref() - Increments media device register refcount
> >> + *
> >> + * @mdev:	pointer to struct &media_device
> >> + *
> >> + * When more than one driver is associated with the media device, it is
> >> + * necessary to refcount the number of registrations to avoid unregister
> >> + * when it is still in use.
> >> + */
> >> +void media_device_register_ref(struct media_device *mdev);
> >> +
> >> +/**
> >>   * media_device_unregister() - Unregisters a media device element
> >>   *
> >>   * @mdev:	pointer to struct &media_device
> >> @@ -512,6 +526,18 @@ int __must_check __media_device_register(struct media_device *mdev,
> >>  void media_device_unregister(struct media_device *mdev);
> >>  
> >>  /**
> >> + * media_device_unregister_put() - Unregisters a media device element
> >> + *
> >> + * @mdev:	pointer to struct &media_device
> >> + *
> >> + * Should be called to unregister media device allocated with Media Device
> >> + * Allocator API media_device_get() interface.
> >> + * It is safe to call this function on an unregistered (but initialised)
> >> + * media device.
> >> + */
> >> +void media_device_unregister_put(struct media_device *mdev);
> >> +
> >> +/**
> >>   * media_device_register_entity() - registers a media entity inside a
> >>   *	previously registered media device.
> >>   *
> >> @@ -681,9 +707,15 @@ static inline int media_device_register(struct media_device *mdev)
> >>  {
> >>  	return 0;
> >>  }
> >> +static inline void media_device_register_ref(struct media_device *mdev)
> >> +{
> >> +}
> >>  static inline void media_device_unregister(struct media_device *mdev)
> >>  {
> >>  }
> >> +static inline void media_device_unregister_put(struct media_device *mdev)
> >> +{
> >> +}
> >>  static inline int media_device_register_entity(struct media_device *mdev,
> >>  						struct media_entity *entity)
> >>  {  
> > 
> >   
> 
> thanks,
> -- Shuah
> --
> 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


-- 

Cheers,
Mauro