[RFC 00/16] Make use of kref in media device, grab references as needed

[RFC 00/16] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Hi folks,

I've been working on this for some time now but only got the full patchset
working some moments ago. The patchset properly, I believe, fixes the
issue of removing a device whilst streaming.

Media device is refcounted and its memory is only released once the last
reference is gone: unregistering is simply unregistering, it no longer
should release memory which could be further accessed.

A video node or a sub-device node also gets a reference to the media
device, i.e. the release function of the video device node will release
its reference to the media device. The same goes for file handles to the
media device.

As a side effect of refcounting the media device, it is allocate together
with the media devnode. The driver may also rely its own resources to the
media device. Alternatively there's also a priv field to hold drivers
private pointer (for container_of() is an option in this case).

I've tested this by manually unbinding the omap3isp platform device while
streaming. Driver changes are required for this to work; by not using
dynamic allocation (i.e. media_device_alloc()) the old behaviour is still
supported. This is still unlikely to be a grave problem as there are not
that many device drivers that support physically removable devices. We've
had this problem for other devices for many years without paying much
notice --- that doesn't mean I don't think at least drivers for removable
devices shouldn't be changed as part of the set later on, I'd just like to
get review comments on the approach first.

The three patches that originally partially resolved some of these issues
are reverted in the beginning of the set. I'm still posting this as an RFC
mainly since the testing is somewhat limited so far.

-- 
Kind regards,
Sakari

[RFC 01/16] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Sakari Ailus]

This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
ioctl/syscall and unregister race"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  | 15 +++++++--------
 drivers/media/media-devnode.c |  8 +-------
 include/media/media-devnode.h | 16 ++--------------
 3 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1795abe..33a9952 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -732,7 +732,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (ret < 0) {
 		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
-		media_devnode_unregister_prepare(devnode);
 		media_devnode_unregister(devnode);
 		return ret;
 	}
@@ -789,9 +788,6 @@ void media_device_unregister(struct media_device *mdev)
 		return;
 	}
 
-	/* Clear the devnode register bit to avoid races with media dev open */
-	media_devnode_unregister_prepare(mdev->devnode);
-
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
 		__media_device_unregister_entity(entity);
@@ -812,10 +808,13 @@ void media_device_unregister(struct media_device *mdev)
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 
-	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-	media_devnode_unregister(mdev->devnode);
-	/* devnode free is handled in media_devnode_*() */
-	mdev->devnode = NULL;
+	/* Check if mdev devnode was registered */
+	if (media_devnode_is_registered(mdev->devnode)) {
+		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
+		media_devnode_unregister(mdev->devnode);
+		/* devnode free is handled in media_devnode_*() */
+		mdev->devnode = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..5b605ff 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -287,7 +287,7 @@ cdev_add_error:
 	return ret;
 }
 
-void media_devnode_unregister_prepare(struct media_devnode *devnode)
+void media_devnode_unregister(struct media_devnode *devnode)
 {
 	/* Check if devnode was ever registered at all */
 	if (!media_devnode_is_registered(devnode))
@@ -295,12 +295,6 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	mutex_unlock(&media_devnode_lock);
-}
-
-void media_devnode_unregister(struct media_devnode *devnode)
-{
-	mutex_lock(&media_devnode_lock);
 	/* Delete the cdev on this minor as well */
 	cdev_del(&devnode->cdev);
 	mutex_unlock(&media_devnode_lock);
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 37d4948..d5037a9 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -127,26 +127,14 @@ int __must_check media_devnode_register(struct media_device *mdev,
 					struct module *owner);
 
 /**
- * media_devnode_unregister_prepare - clear the media device node register bit
- * @devnode: the device node to prepare for unregister
- *
- * This clears the passed device register bit. Future open calls will be met
- * with errors. Should be called before media_devnode_unregister() to avoid
- * races with unregister and device file open calls.
- *
- * This function can safely be called if the device node has never been
- * registered or has already been unregistered.
- */
-void media_devnode_unregister_prepare(struct media_devnode *devnode);
-
-/**
  * media_devnode_unregister - unregister a media device node
  * @devnode: the device node to unregister
  *
  * This unregisters the passed device. Future open calls will be met with
  * errors.
  *
- * Should be called after media_devnode_unregister_prepare()
+ * This function can safely be called if the device node has never been
+ * registered or has already been unregistered.
  */
 void media_devnode_unregister(struct media_devnode *devnode);
 
-- 
2.1.4

[RFC 02/16] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind"

[Sakari Ailus]

This reverts commit 5b28dde51d0c ("[media] media: fix use-after-free in
cdev_put() when app exits after driver unbind"). The commit was part of an
original patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  |  6 ++----
 drivers/media/media-devnode.c | 48 +++++++++++++++++--------------------------
 2 files changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 33a9952..e61fa66 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -723,16 +723,16 @@ int __must_check __media_device_register(struct media_device *mdev,
 
 	ret = media_devnode_register(mdev, devnode, owner);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
+		kfree(devnode);
 		return ret;
 	}
 
 	ret = device_create_file(&devnode->dev, &dev_attr_model);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
 		media_devnode_unregister(devnode);
+		kfree(devnode);
 		return ret;
 	}
 
@@ -812,8 +812,6 @@ void media_device_unregister(struct media_device *mdev)
 	if (media_devnode_is_registered(mdev->devnode)) {
 		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
 		media_devnode_unregister(mdev->devnode);
-		/* devnode free is handled in media_devnode_*() */
-		mdev->devnode = NULL;
 	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 5b605ff..ecdc02d 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -63,8 +63,13 @@ static void media_devnode_release(struct device *cd)
 	struct media_devnode *devnode = to_media_devnode(cd);
 
 	mutex_lock(&media_devnode_lock);
+
+	/* Delete the cdev on this minor as well */
+	cdev_del(&devnode->cdev);
+
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
+
 	mutex_unlock(&media_devnode_lock);
 
 	/* Release media_devnode and perform other cleanups as needed. */
@@ -72,7 +77,6 @@ static void media_devnode_release(struct device *cd)
 		devnode->release(devnode);
 
 	kfree(devnode);
-	pr_debug("%s: Media Devnode Deallocated\n", __func__);
 }
 
 static struct bus_type media_bus_type = {
@@ -201,8 +205,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	put_device(&devnode->dev);
-
-	pr_debug("%s: Media Release\n", __func__);
 	return 0;
 }
 
@@ -233,7 +235,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	if (minor == MEDIA_NUM_DEVICES) {
 		mutex_unlock(&media_devnode_lock);
 		pr_err("could not get a free minor\n");
-		kfree(devnode);
 		return -ENFILE;
 	}
 
@@ -243,31 +244,27 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */
-	devnode->dev.bus = &media_bus_type;
-	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
-	devnode->dev.release = media_devnode_release;
-	if (devnode->parent)
-		devnode->dev.parent = devnode->parent;
-	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	device_initialize(&devnode->dev);
-
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 
 	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
-		goto cdev_add_error;
+		goto error;
 	}
 
-	/* Part 3: Add the media device */
-	ret = device_add(&devnode->dev);
+	/* Part 3: Register the media device */
+	devnode->dev.bus = &media_bus_type;
+	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	devnode->dev.release = media_devnode_release;
+	if (devnode->parent)
+		devnode->dev.parent = devnode->parent;
+	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+	ret = device_register(&devnode->dev);
 	if (ret < 0) {
-		pr_err("%s: device_add failed\n", __func__);
-		goto device_add_error;
+		pr_err("%s: device_register failed\n", __func__);
+		goto error;
 	}
 
 	/* Part 4: Activate this minor. The char device can now be used. */
@@ -275,15 +272,12 @@ int __must_check media_devnode_register(struct media_device *mdev,
 
 	return 0;
 
-device_add_error:
-	cdev_del(&devnode->cdev);
-cdev_add_error:
+error:
 	mutex_lock(&media_devnode_lock);
+	cdev_del(&devnode->cdev);
 	clear_bit(devnode->minor, media_devnode_nums);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
 
-	put_device(&devnode->dev);
 	return ret;
 }
 
@@ -295,12 +289,8 @@ void media_devnode_unregister(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
 	mutex_unlock(&media_devnode_lock);
-	device_del(&devnode->dev);
-	devnode->media_dev = NULL;
-	put_device(&devnode->dev);
+	device_unregister(&devnode->dev);
 }
 
 /*
-- 
2.1.4

[RFC 03/16] Revert "[media] media-device: dynamically allocate struct media_devnode"

[Sakari Ailus]

This reverts commit a087ce704b80 ("[media] media-device: dynamically
allocate struct media_devnode"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c           | 44 +++++++++++-----------------------
 drivers/media/media-devnode.c          |  7 +-----
 drivers/media/usb/au0828/au0828-core.c |  4 ++--
 drivers/media/usb/uvc/uvc_driver.c     |  2 +-
 include/media/media-device.h           |  5 +++-
 include/media/media-devnode.h          | 15 ++----------
 6 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e61fa66..a1cd50f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	long ret;
 
 	mutex_lock(&dev->graph_mutex);
@@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	long ret;
 
 	switch (cmd) {
@@ -531,8 +531,7 @@ static const struct media_file_operations media_device_fops = {
 static ssize_t show_model(struct device *cd,
 			  struct device_attribute *attr, char *buf)
 {
-	struct media_devnode *devnode = to_media_devnode(cd);
-	struct media_device *mdev = devnode->media_dev;
+	struct media_device *mdev = to_media_device(to_media_devnode(cd));
 
 	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -705,34 +704,23 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
-	struct media_devnode *devnode;
 	int ret;
 
-	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
-	if (!devnode)
-		return -ENOMEM;
-
 	/* Register the device node. */
-	mdev->devnode = devnode;
-	devnode->fops = &media_device_fops;
-	devnode->parent = mdev->dev;
-	devnode->release = media_device_release;
+	mdev->devnode.fops = &media_device_fops;
+	mdev->devnode.parent = mdev->dev;
+	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	ret = media_devnode_register(mdev, devnode, owner);
-	if (ret < 0) {
-		mdev->devnode = NULL;
-		kfree(devnode);
+	ret = media_devnode_register(&mdev->devnode, owner);
+	if (ret < 0)
 		return ret;
-	}
 
-	ret = device_create_file(&devnode->dev, &dev_attr_model);
+	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
-		mdev->devnode = NULL;
-		media_devnode_unregister(devnode);
-		kfree(devnode);
+		media_devnode_unregister(&mdev->devnode);
 		return ret;
 	}
 
@@ -783,7 +771,7 @@ void media_device_unregister(struct media_device *mdev)
 	mutex_lock(&mdev->graph_mutex);
 
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(mdev->devnode)) {
+	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
@@ -806,13 +794,9 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	dev_dbg(mdev->dev, "Media device unregistered\n");
-
-	/* Check if mdev devnode was registered */
-	if (media_devnode_is_registered(mdev->devnode)) {
-		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-		media_devnode_unregister(mdev->devnode);
-	}
+	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	dev_dbg(mdev->dev, "Media device unregistering\n");
+	media_devnode_unregister(&mdev->devnode);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index ecdc02d..7481c96 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,7 +44,6 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
-#include <media/media-device.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -75,8 +74,6 @@ static void media_devnode_release(struct device *cd)
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
-
-	kfree(devnode);
 }
 
 static struct bus_type media_bus_type = {
@@ -222,8 +219,7 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -242,7 +238,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	mutex_unlock(&media_devnode_lock);
 
 	devnode->minor = minor;
-	devnode->media_dev = mdev;
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bf53553..321ea5c 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	struct media_device *mdev = dev->media_dev;
 	struct media_entity_notify *notify, *nextp;
 
-	if (!mdev || !media_devnode_is_registered(mdev->devnode))
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
 		return;
 
 	/* Remove au0828 entity_notify callbacks */
@@ -482,7 +482,7 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 	if (!dev->media_dev)
 		return 0;
 
-	if (!media_devnode_is_registered(dev->media_dev->devnode)) {
+	if (!media_devnode_is_registered(&dev->media_dev->devnode)) {
 
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 302e284..451e84e9 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1674,7 +1674,7 @@ 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))
+	if (media_devnode_is_registered(&dev->mdev.devnode))
 		media_device_unregister(&dev->mdev);
 	media_device_cleanup(&dev->mdev);
 #endif
diff --git a/include/media/media-device.h b/include/media/media-device.h
index f743ae2..a9b33c4 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -347,7 +347,7 @@ struct media_entity_notify {
 struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
-	struct media_devnode *devnode;
+	struct media_devnode devnode;
 
 	char model[32];
 	char driver_name[32];
@@ -393,6 +393,9 @@ struct usb_device;
 #define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
 #define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
 
+/* media_devnode to media_device */
+#define to_media_device(node) container_of(node, struct media_device, devnode)
+
 /**
  * media_entity_enum_init - Initialise an entity enumeration
  *
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d5037a9..a0f6823 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -33,8 +33,6 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
-struct media_device;
-
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
@@ -84,8 +82,6 @@ struct media_file_operations {
  * before registering the node.
  */
 struct media_devnode {
-	struct media_device *media_dev;
-
 	/* device ops */
 	const struct media_file_operations *fops;
 
@@ -108,8 +104,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
- * @mdev: struct media_device we want to register a device node
- * @devnode: media device node structure we want to register
+ * @devnode: struct media_devnode we want to register a device node
  * @owner: should be filled with %THIS_MODULE
  *
  * The registration code assigns minor numbers and registers the new device node
@@ -122,8 +117,7 @@ struct media_devnode {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
 
 /**
@@ -153,14 +147,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
  *	false otherwise.
  *
  * @devnode: pointer to struct &media_devnode.
- *
- * Note: If mdev is NULL, it also returns false.
  */
 static inline int media_devnode_is_registered(struct media_devnode *devnode)
 {
-	if (!devnode)
-		return false;
-
 	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 }
 
-- 
2.1.4

[RFC 04/16] media: Remove useless curly braces and parentheses

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a1cd50f..8bdc316 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -596,9 +596,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 			       &entity->pads[i].graph_obj);
 
 	/* invoke entity_notify callbacks */
-	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
-		(notify)->notify(entity, notify->notify_data);
-	}
+	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
+		notify->notify(entity, notify->notify_data);
 
 	if (mdev->entity_internal_idx_max
 	    >= mdev->pm_count_walk.ent_enum.idx_max) {
-- 
2.1.4

[RFC 05/16] media: devnode: Refcount the media devnode

[Sakari Ailus]

Add reference count to the media devnode. The media_devnode is intended to
be embedded in another struct such as media_device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  |   7 +--
 drivers/media/media-devnode.c | 114 +++++++++++++++++++++++++++++++++---------
 include/media/media-devnode.h |  47 +++++++++++++----
 3 files changed, 133 insertions(+), 35 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8bdc316..a11b3be 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = {
 static ssize_t show_model(struct device *cd,
 			  struct device_attribute *attr, char *buf)
 {
-	struct media_device *mdev = to_media_device(to_media_devnode(cd));
+	struct media_device *mdev = to_media_device(
+		to_media_devnode_cdev(cd)->devnode);
 
 	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -717,7 +718,7 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (ret < 0)
 		return ret;
 
-	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
+	ret = device_create_file(&mdev->devnode.mdc->dev, &dev_attr_model);
 	if (ret < 0) {
 		media_devnode_unregister(&mdev->devnode);
 		return ret;
@@ -793,7 +794,7 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	device_remove_file(&mdev->devnode.mdc->dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
 }
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 7481c96..566ef08 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -57,25 +57,54 @@ static DEFINE_MUTEX(media_devnode_lock);
 static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
 
 /* Called when the last user of the media device exits. */
-static void media_devnode_release(struct device *cd)
+static void media_devnode_device_release(struct device *cd)
 {
-	struct media_devnode *devnode = to_media_devnode(cd);
+	struct media_devnode_cdev *mdc = to_media_devnode_cdev(cd);
 
-	mutex_lock(&media_devnode_lock);
+	dev_dbg(cd, "release media devnode device\n");
 
 	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
+	cdev_del(&mdc->cdev);
 
 	/* Mark device node number as free */
-	clear_bit(devnode->minor, media_devnode_nums);
-
+	mutex_lock(&media_devnode_lock);
+	clear_bit(MINOR(mdc->cdev.dev), media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
+}
+
+static void media_devnode_release(struct kref *kref)
+{
+	struct media_devnode *devnode =
+		container_of(kref, struct media_devnode, kref);
+
+	dev_dbg(&devnode->mdc->dev, "release media devnode\n");
+
+	put_device(&devnode->mdc->dev);
 
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
 }
 
+void media_devnode_init(struct media_devnode *devnode)
+{
+	kref_init(&devnode->kref);
+	devnode->use_kref = true;
+}
+EXPORT_SYMBOL_GPL(media_devnode_init);
+
+void media_devnode_get(struct media_devnode *devnode)
+{
+	kref_get(&devnode->kref);
+}
+EXPORT_SYMBOL_GPL(media_devnode_get);
+
+void media_devnode_put(struct media_devnode *devnode)
+{
+	kref_put(&devnode->kref, media_devnode_release);
+}
+EXPORT_SYMBOL_GPL(media_devnode_put);
+
 static struct bus_type media_bus_type = {
 	.name = MEDIA_NAME,
 };
@@ -164,7 +193,8 @@ static int media_open(struct inode *inode, struct file *filp)
 	 * a crash.
 	 */
 	mutex_lock(&media_devnode_lock);
-	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
+	devnode = container_of(inode->i_cdev,
+			       struct media_devnode_cdev, cdev)->devnode;
 	/* return ENXIO if the media device has been removed
 	   already or if it is not registered anymore. */
 	if (!media_devnode_is_registered(devnode)) {
@@ -172,7 +202,10 @@ static int media_open(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	}
 	/* and increase the device refcount */
-	get_device(&devnode->dev);
+	if (devnode->use_kref)
+		media_devnode_get(devnode);
+	else
+		get_device(&devnode->mdc->dev);
 	mutex_unlock(&media_devnode_lock);
 
 	filp->private_data = devnode;
@@ -180,7 +213,10 @@ static int media_open(struct inode *inode, struct file *filp)
 	if (devnode->fops->open) {
 		ret = devnode->fops->open(filp);
 		if (ret) {
-			put_device(&devnode->dev);
+			if (devnode->use_kref)
+				media_devnode_put(devnode);
+			else
+				put_device(&devnode->mdc->dev);
 			filp->private_data = NULL;
 			return ret;
 		}
@@ -201,7 +237,11 @@ static int media_release(struct inode *inode, struct file *filp)
 
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
-	put_device(&devnode->dev);
+	if (devnode->use_kref)
+		media_devnode_put(devnode);
+	else
+		put_device(&devnode->mdc->dev);
+
 	return 0;
 }
 
@@ -222,14 +262,20 @@ static const struct file_operations media_devnode_fops = {
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
+	struct media_devnode_cdev *mdc;
 	int minor;
 	int ret;
 
+	mdc = kzalloc(sizeof(*mdc), GFP_KERNEL);
+	if (!mdc)
+		return -ENOMEM;
+
 	/* Part 1: Find a free minor number */
 	mutex_lock(&media_devnode_lock);
 	minor = find_next_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES, 0);
 	if (minor == MEDIA_NUM_DEVICES) {
 		mutex_unlock(&media_devnode_lock);
+		kfree(mdc);
 		pr_err("could not get a free minor\n");
 		return -ENFILE;
 	}
@@ -240,23 +286,26 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	devnode->minor = minor;
 
 	/* Part 2: Initialize and register the character device */
-	cdev_init(&devnode->cdev, &media_devnode_fops);
-	devnode->cdev.owner = owner;
+	cdev_init(&mdc->cdev, &media_devnode_fops);
+	mdc->cdev.owner = owner;
+	mdc->devnode = devnode;
+	devnode->mdc = mdc;
 
-	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
+	ret = cdev_add(&mdc->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor),
+		       1);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
 		goto error;
 	}
 
 	/* Part 3: Register the media device */
-	devnode->dev.bus = &media_bus_type;
-	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
-	devnode->dev.release = media_devnode_release;
+	devnode->mdc->dev.bus = &media_bus_type;
+	devnode->mdc->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	devnode->mdc->dev.release = media_devnode_device_release;
 	if (devnode->parent)
-		devnode->dev.parent = devnode->parent;
-	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	ret = device_register(&devnode->dev);
+		devnode->mdc->dev.parent = devnode->parent;
+	dev_set_name(&devnode->mdc->dev, "media%d", devnode->minor);
+	ret = device_register(&devnode->mdc->dev);
 	if (ret < 0) {
 		pr_err("%s: device_register failed\n", __func__);
 		goto error;
@@ -265,14 +314,19 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	/* Part 4: Activate this minor. The char device can now be used. */
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 
+	/* For us, reference released in media_devnode_release(). */
+	get_device(&devnode->mdc->dev);
+
 	return 0;
 
 error:
 	mutex_lock(&media_devnode_lock);
-	cdev_del(&devnode->cdev);
+	cdev_del(&mdc->cdev);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
+	kfree(mdc);
+
 	return ret;
 }
 
@@ -282,16 +336,30 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	if (!media_devnode_is_registered(devnode))
 		return;
 
+	dev_dbg(&devnode->mdc->dev, "unregister media devnode\n");
+
+	device_unregister(&devnode->mdc->dev);
+
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
-	device_unregister(&devnode->dev);
+
+	if (!devnode->use_kref) {
+		media_devnode_device_release(&devnode->mdc->dev);
+
+		if (devnode->release)
+			devnode->release(devnode);
+
+		return;
+	}
+
+	media_devnode_put(devnode);
 }
 
 /*
  *	Initialise media for linux
  */
-static int __init media_devnode_init(void)
+static int __init __media_devnode_init(void)
 {
 	int ret;
 
@@ -319,7 +387,7 @@ static void __exit media_devnode_exit(void)
 	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
 }
 
-subsys_initcall(media_devnode_init);
+subsys_initcall(__media_devnode_init);
 module_exit(media_devnode_exit)
 
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index a0f6823..4e9d066 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -28,10 +28,11 @@
 #ifndef _MEDIA_DEVNODE_H
 #define _MEDIA_DEVNODE_H
 
-#include <linux/poll.h>
-#include <linux/fs.h>
-#include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/kref.h>
+#include <linux/poll.h>
 
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
@@ -65,15 +66,24 @@ struct media_file_operations {
 	int (*release) (struct file *);
 };
 
+struct media_devnode_cdev {
+	struct device dev;		/* media device */
+	struct cdev cdev;		/* character device */
+	struct media_devnode *devnode;	/* pointer to media_devnode */
+};
+
+#define to_media_devnode_cdev(cd) \
+	container_of(cd, struct media_devnode_cdev, dev)
+
 /**
  * struct media_devnode - Media device node
  * @media_dev:	pointer to struct &media_device
  * @fops:	pointer to struct &media_file_operations with media device ops
- * @dev:	pointer to struct &device containing the media controller device
- * @cdev:	struct cdev pointer character device
  * @parent:	parent device
  * @minor:	device node minor number
  * @flags:	flags, combination of the MEDIA_FLAG_* constants
+ * @kref:	kref to this object
+ * @use_kref:	tell whether we're using kref or not (TEMPORARY)
  * @release:	release callback called at the end of media_devnode_release()
  *
  * This structure represents a media-related device node.
@@ -86,20 +96,39 @@ struct media_devnode {
 	const struct media_file_operations *fops;
 
 	/* sysfs */
-	struct device dev;		/* media device */
-	struct cdev cdev;		/* character device */
 	struct device *parent;		/* device parent */
 
 	/* device info */
 	int minor;
 	unsigned long flags;		/* Use bitops to access flags */
+	struct media_devnode_cdev *mdc;
+	struct kref kref;
+	bool use_kref;
 
 	/* callbacks */
 	void (*release)(struct media_devnode *devnode);
 };
 
-/* dev to media_devnode */
-#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
+/**
+ * media_devnode_init - initialise a media device node (with kref)
+ *
+ * devnode: the device node to be initialised
+ */
+void media_devnode_init(struct media_devnode *devnode);
+
+/**
+ * media_devnode_get - get a reference to a media device node
+ *
+ * @devnode: the device node to get a reference to
+ */
+void media_devnode_get(struct media_devnode *devnode);
+
+/**
+ * media_devnode_put - put a reference to a media device node
+ *
+ * @devnode: the device node to put a reference from
+ */
+void media_devnode_put(struct media_devnode *devnode);
 
 /**
  * media_devnode_register - register a media device node
-- 
2.1.4

Re: [RFC 05/16] media: devnode: Refcount the media devnode

[Hans Verkuil]

On 07/15/2016 12:35 AM, Sakari Ailus wrote:
> Add reference count to the media devnode. The media_devnode is intended to
> be embedded in another struct such as media_device.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c  |   7 +--
>  drivers/media/media-devnode.c | 114 +++++++++++++++++++++++++++++++++---------
>  include/media/media-devnode.h |  47 +++++++++++++----
>  3 files changed, 133 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 8bdc316..a11b3be 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = {
>  static ssize_t show_model(struct device *cd,
>  			  struct device_attribute *attr, char *buf)
>  {
> -	struct media_device *mdev = to_media_device(to_media_devnode(cd));
> +	struct media_device *mdev = to_media_device(
> +		to_media_devnode_cdev(cd)->devnode);
>  
>  	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
>  }
> @@ -717,7 +718,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> +	ret = device_create_file(&mdev->devnode.mdc->dev, &dev_attr_model);
>  	if (ret < 0) {
>  		media_devnode_unregister(&mdev->devnode);
>  		return ret;
> @@ -793,7 +794,7 @@ void media_device_unregister(struct media_device *mdev)
>  
>  	mutex_unlock(&mdev->graph_mutex);
>  
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> +	device_remove_file(&mdev->devnode.mdc->dev, &dev_attr_model);
>  	dev_dbg(mdev->dev, "Media device unregistering\n");
>  	media_devnode_unregister(&mdev->devnode);
>  }
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 7481c96..566ef08 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -57,25 +57,54 @@ static DEFINE_MUTEX(media_devnode_lock);
>  static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
>  
>  /* Called when the last user of the media device exits. */
> -static void media_devnode_release(struct device *cd)
> +static void media_devnode_device_release(struct device *cd)
>  {
> -	struct media_devnode *devnode = to_media_devnode(cd);
> +	struct media_devnode_cdev *mdc = to_media_devnode_cdev(cd);
>  
> -	mutex_lock(&media_devnode_lock);
> +	dev_dbg(cd, "release media devnode device\n");
>  
>  	/* Delete the cdev on this minor as well */
> -	cdev_del(&devnode->cdev);
> +	cdev_del(&mdc->cdev);
>  
>  	/* Mark device node number as free */
> -	clear_bit(devnode->minor, media_devnode_nums);
> -
> +	mutex_lock(&media_devnode_lock);
> +	clear_bit(MINOR(mdc->cdev.dev), media_devnode_nums);
>  	mutex_unlock(&media_devnode_lock);
> +}
> +
> +static void media_devnode_release(struct kref *kref)
> +{
> +	struct media_devnode *devnode =
> +		container_of(kref, struct media_devnode, kref);
> +
> +	dev_dbg(&devnode->mdc->dev, "release media devnode\n");
> +
> +	put_device(&devnode->mdc->dev);
>  
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
>  }
>  
> +void media_devnode_init(struct media_devnode *devnode)
> +{
> +	kref_init(&devnode->kref);
> +	devnode->use_kref = true;
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_init);
> +
> +void media_devnode_get(struct media_devnode *devnode)
> +{
> +	kref_get(&devnode->kref);
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_get);
> +
> +void media_devnode_put(struct media_devnode *devnode)
> +{
> +	kref_put(&devnode->kref, media_devnode_release);
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_put);
> +
>  static struct bus_type media_bus_type = {
>  	.name = MEDIA_NAME,
>  };
> @@ -164,7 +193,8 @@ static int media_open(struct inode *inode, struct file *filp)
>  	 * a crash.
>  	 */
>  	mutex_lock(&media_devnode_lock);
> -	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
> +	devnode = container_of(inode->i_cdev,
> +			       struct media_devnode_cdev, cdev)->devnode;
>  	/* return ENXIO if the media device has been removed
>  	   already or if it is not registered anymore. */
>  	if (!media_devnode_is_registered(devnode)) {
> @@ -172,7 +202,10 @@ static int media_open(struct inode *inode, struct file *filp)
>  		return -ENXIO;
>  	}
>  	/* and increase the device refcount */
> -	get_device(&devnode->dev);
> +	if (devnode->use_kref)
> +		media_devnode_get(devnode);
> +	else
> +		get_device(&devnode->mdc->dev);
>  	mutex_unlock(&media_devnode_lock);
>  
>  	filp->private_data = devnode;
> @@ -180,7 +213,10 @@ static int media_open(struct inode *inode, struct file *filp)
>  	if (devnode->fops->open) {
>  		ret = devnode->fops->open(filp);
>  		if (ret) {
> -			put_device(&devnode->dev);
> +			if (devnode->use_kref)
> +				media_devnode_put(devnode);
> +			else
> +				put_device(&devnode->mdc->dev);
>  			filp->private_data = NULL;
>  			return ret;
>  		}
> @@ -201,7 +237,11 @@ static int media_release(struct inode *inode, struct file *filp)
>  
>  	/* decrease the refcount unconditionally since the release()
>  	   return value is ignored. */
> -	put_device(&devnode->dev);
> +	if (devnode->use_kref)
> +		media_devnode_put(devnode);
> +	else
> +		put_device(&devnode->mdc->dev);
> +
>  	return 0;
>  }
>  
> @@ -222,14 +262,20 @@ static const struct file_operations media_devnode_fops = {
>  int __must_check media_devnode_register(struct media_devnode *devnode,
>  					struct module *owner)
>  {
> +	struct media_devnode_cdev *mdc;
>  	int minor;
>  	int ret;
>  
> +	mdc = kzalloc(sizeof(*mdc), GFP_KERNEL);

Why is this no longer embedded in struct media_devnode? I see no reason for that
change.

> +	if (!mdc)
> +		return -ENOMEM;
> +
>  	/* Part 1: Find a free minor number */
>  	mutex_lock(&media_devnode_lock);
>  	minor = find_next_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES, 0);
>  	if (minor == MEDIA_NUM_DEVICES) {
>  		mutex_unlock(&media_devnode_lock);
> +		kfree(mdc);
>  		pr_err("could not get a free minor\n");
>  		return -ENFILE;
>  	}
> @@ -240,23 +286,26 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>  	devnode->minor = minor;
>  
>  	/* Part 2: Initialize and register the character device */
> -	cdev_init(&devnode->cdev, &media_devnode_fops);
> -	devnode->cdev.owner = owner;
> +	cdev_init(&mdc->cdev, &media_devnode_fops);
> +	mdc->cdev.owner = owner;
> +	mdc->devnode = devnode;
> +	devnode->mdc = mdc;
>  
> -	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
> +	ret = cdev_add(&mdc->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor),
> +		       1);
>  	if (ret < 0) {
>  		pr_err("%s: cdev_add failed\n", __func__);
>  		goto error;
>  	}
>  
>  	/* Part 3: Register the media device */
> -	devnode->dev.bus = &media_bus_type;
> -	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> -	devnode->dev.release = media_devnode_release;
> +	devnode->mdc->dev.bus = &media_bus_type;
> +	devnode->mdc->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> +	devnode->mdc->dev.release = media_devnode_device_release;
>  	if (devnode->parent)
> -		devnode->dev.parent = devnode->parent;
> -	dev_set_name(&devnode->dev, "media%d", devnode->minor);
> -	ret = device_register(&devnode->dev);
> +		devnode->mdc->dev.parent = devnode->parent;
> +	dev_set_name(&devnode->mdc->dev, "media%d", devnode->minor);
> +	ret = device_register(&devnode->mdc->dev);
>  	if (ret < 0) {
>  		pr_err("%s: device_register failed\n", __func__);
>  		goto error;
> @@ -265,14 +314,19 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>  	/* Part 4: Activate this minor. The char device can now be used. */
>  	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  
> +	/* For us, reference released in media_devnode_release(). */
> +	get_device(&devnode->mdc->dev);
> +

I would really like to see this synced with what drivers/staging/media/cec/cec-core.c is
doing. That code is similar to what other subsystems are doing and seems to be according
to best practices. What we do in v4l2-dev.c is certainly NOT according to best practices.

One of the things I'd like to do is to fix v4l2-dev.c as well, but that will take time.

Ideally all the code that registers device nodes should all use the same method. That makes
it easier to review and to fix issues in the future.

>  	return 0;
>  
>  error:
>  	mutex_lock(&media_devnode_lock);
> -	cdev_del(&devnode->cdev);
> +	cdev_del(&mdc->cdev);
>  	clear_bit(devnode->minor, media_devnode_nums);
>  	mutex_unlock(&media_devnode_lock);
>  
> +	kfree(mdc);
> +
>  	return ret;
>  }
>  
> @@ -282,16 +336,30 @@ void media_devnode_unregister(struct media_devnode *devnode)
>  	if (!media_devnode_is_registered(devnode))
>  		return;
>  
> +	dev_dbg(&devnode->mdc->dev, "unregister media devnode\n");
> +
> +	device_unregister(&devnode->mdc->dev);
> +
>  	mutex_lock(&media_devnode_lock);
>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  	mutex_unlock(&media_devnode_lock);
> -	device_unregister(&devnode->dev);
> +
> +	if (!devnode->use_kref) {
> +		media_devnode_device_release(&devnode->mdc->dev);
> +
> +		if (devnode->release)
> +			devnode->release(devnode);
> +
> +		return;
> +	}
> +
> +	media_devnode_put(devnode);
>  }
>  
>  /*
>   *	Initialise media for linux
>   */
> -static int __init media_devnode_init(void)
> +static int __init __media_devnode_init(void)
>  {
>  	int ret;
>  
> @@ -319,7 +387,7 @@ static void __exit media_devnode_exit(void)
>  	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
>  }
>  
> -subsys_initcall(media_devnode_init);
> +subsys_initcall(__media_devnode_init);
>  module_exit(media_devnode_exit)
>  
>  MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index a0f6823..4e9d066 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -28,10 +28,11 @@
>  #ifndef _MEDIA_DEVNODE_H
>  #define _MEDIA_DEVNODE_H
>  
> -#include <linux/poll.h>
> -#include <linux/fs.h>
> -#include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/kref.h>
> +#include <linux/poll.h>
>  
>  /*
>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
> @@ -65,15 +66,24 @@ struct media_file_operations {
>  	int (*release) (struct file *);
>  };
>  
> +struct media_devnode_cdev {
> +	struct device dev;		/* media device */
> +	struct cdev cdev;		/* character device */
> +	struct media_devnode *devnode;	/* pointer to media_devnode */
> +};
> +
> +#define to_media_devnode_cdev(cd) \
> +	container_of(cd, struct media_devnode_cdev, dev)
> +
>  /**
>   * struct media_devnode - Media device node
>   * @media_dev:	pointer to struct &media_device
>   * @fops:	pointer to struct &media_file_operations with media device ops
> - * @dev:	pointer to struct &device containing the media controller device
> - * @cdev:	struct cdev pointer character device
>   * @parent:	parent device
>   * @minor:	device node minor number
>   * @flags:	flags, combination of the MEDIA_FLAG_* constants
> + * @kref:	kref to this object
> + * @use_kref:	tell whether we're using kref or not (TEMPORARY)

You say this is temporary, but it is never removed. I don't see why you need
a kref at all, usually you just use put/get_device(&devnode->dev).

>   * @release:	release callback called at the end of media_devnode_release()
>   *
>   * This structure represents a media-related device node.
> @@ -86,20 +96,39 @@ struct media_devnode {
>  	const struct media_file_operations *fops;
>  
>  	/* sysfs */
> -	struct device dev;		/* media device */
> -	struct cdev cdev;		/* character device */
>  	struct device *parent;		/* device parent */
>  
>  	/* device info */
>  	int minor;
>  	unsigned long flags;		/* Use bitops to access flags */
> +	struct media_devnode_cdev *mdc;
> +	struct kref kref;
> +	bool use_kref;
>  
>  	/* callbacks */
>  	void (*release)(struct media_devnode *devnode);
>  };
>  
> -/* dev to media_devnode */
> -#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
> +/**
> + * media_devnode_init - initialise a media device node (with kref)
> + *
> + * devnode: the device node to be initialised
> + */
> +void media_devnode_init(struct media_devnode *devnode);
> +
> +/**
> + * media_devnode_get - get a reference to a media device node
> + *
> + * @devnode: the device node to get a reference to
> + */
> +void media_devnode_get(struct media_devnode *devnode);
> +
> +/**
> + * media_devnode_put - put a reference to a media device node
> + *
> + * @devnode: the device node to put a reference from
> + */
> +void media_devnode_put(struct media_devnode *devnode);
>  
>  /**
>   * media_devnode_register - register a media device node
> 

Regards,

	Hans

Re: [RFC 05/16] media: devnode: Refcount the media devnode

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> 
> 
> On 07/15/2016 12:35 AM, Sakari Ailus wrote:
>> Add reference count to the media devnode. The media_devnode is intended to
>> be embedded in another struct such as media_device.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/media/media-device.c  |   7 +--
>>  drivers/media/media-devnode.c | 114 +++++++++++++++++++++++++++++++++---------
>>  include/media/media-devnode.h |  47 +++++++++++++----
>>  3 files changed, 133 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 8bdc316..a11b3be 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops = {
>>  static ssize_t show_model(struct device *cd,
>>  			  struct device_attribute *attr, char *buf)
>>  {
>> -	struct media_device *mdev = to_media_device(to_media_devnode(cd));
>> +	struct media_device *mdev = to_media_device(
>> +		to_media_devnode_cdev(cd)->devnode);
>>  
>>  	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
>>  }
>> @@ -717,7 +718,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>> +	ret = device_create_file(&mdev->devnode.mdc->dev, &dev_attr_model);
>>  	if (ret < 0) {
>>  		media_devnode_unregister(&mdev->devnode);
>>  		return ret;
>> @@ -793,7 +794,7 @@ void media_device_unregister(struct media_device *mdev)
>>  
>>  	mutex_unlock(&mdev->graph_mutex);
>>  
>> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>> +	device_remove_file(&mdev->devnode.mdc->dev, &dev_attr_model);
>>  	dev_dbg(mdev->dev, "Media device unregistering\n");
>>  	media_devnode_unregister(&mdev->devnode);
>>  }
>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>> index 7481c96..566ef08 100644
>> --- a/drivers/media/media-devnode.c
>> +++ b/drivers/media/media-devnode.c
>> @@ -57,25 +57,54 @@ static DEFINE_MUTEX(media_devnode_lock);
>>  static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
>>  
>>  /* Called when the last user of the media device exits. */
>> -static void media_devnode_release(struct device *cd)
>> +static void media_devnode_device_release(struct device *cd)
>>  {
>> -	struct media_devnode *devnode = to_media_devnode(cd);
>> +	struct media_devnode_cdev *mdc = to_media_devnode_cdev(cd);
>>  
>> -	mutex_lock(&media_devnode_lock);
>> +	dev_dbg(cd, "release media devnode device\n");
>>  
>>  	/* Delete the cdev on this minor as well */
>> -	cdev_del(&devnode->cdev);
>> +	cdev_del(&mdc->cdev);
>>  
>>  	/* Mark device node number as free */
>> -	clear_bit(devnode->minor, media_devnode_nums);
>> -
>> +	mutex_lock(&media_devnode_lock);
>> +	clear_bit(MINOR(mdc->cdev.dev), media_devnode_nums);
>>  	mutex_unlock(&media_devnode_lock);
>> +}
>> +
>> +static void media_devnode_release(struct kref *kref)
>> +{
>> +	struct media_devnode *devnode =
>> +		container_of(kref, struct media_devnode, kref);
>> +
>> +	dev_dbg(&devnode->mdc->dev, "release media devnode\n");
>> +
>> +	put_device(&devnode->mdc->dev);
>>  
>>  	/* Release media_devnode and perform other cleanups as needed. */
>>  	if (devnode->release)
>>  		devnode->release(devnode);
>>  }
>>  
>> +void media_devnode_init(struct media_devnode *devnode)
>> +{
>> +	kref_init(&devnode->kref);
>> +	devnode->use_kref = true;
>> +}
>> +EXPORT_SYMBOL_GPL(media_devnode_init);
>> +
>> +void media_devnode_get(struct media_devnode *devnode)
>> +{
>> +	kref_get(&devnode->kref);
>> +}
>> +EXPORT_SYMBOL_GPL(media_devnode_get);
>> +
>> +void media_devnode_put(struct media_devnode *devnode)
>> +{
>> +	kref_put(&devnode->kref, media_devnode_release);
>> +}
>> +EXPORT_SYMBOL_GPL(media_devnode_put);
>> +
>>  static struct bus_type media_bus_type = {
>>  	.name = MEDIA_NAME,
>>  };
>> @@ -164,7 +193,8 @@ static int media_open(struct inode *inode, struct file *filp)
>>  	 * a crash.
>>  	 */
>>  	mutex_lock(&media_devnode_lock);
>> -	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
>> +	devnode = container_of(inode->i_cdev,
>> +			       struct media_devnode_cdev, cdev)->devnode;
>>  	/* return ENXIO if the media device has been removed
>>  	   already or if it is not registered anymore. */
>>  	if (!media_devnode_is_registered(devnode)) {
>> @@ -172,7 +202,10 @@ static int media_open(struct inode *inode, struct file *filp)
>>  		return -ENXIO;
>>  	}
>>  	/* and increase the device refcount */
>> -	get_device(&devnode->dev);
>> +	if (devnode->use_kref)
>> +		media_devnode_get(devnode);
>> +	else
>> +		get_device(&devnode->mdc->dev);
>>  	mutex_unlock(&media_devnode_lock);
>>  
>>  	filp->private_data = devnode;
>> @@ -180,7 +213,10 @@ static int media_open(struct inode *inode, struct file *filp)
>>  	if (devnode->fops->open) {
>>  		ret = devnode->fops->open(filp);
>>  		if (ret) {
>> -			put_device(&devnode->dev);
>> +			if (devnode->use_kref)
>> +				media_devnode_put(devnode);
>> +			else
>> +				put_device(&devnode->mdc->dev);
>>  			filp->private_data = NULL;
>>  			return ret;
>>  		}
>> @@ -201,7 +237,11 @@ static int media_release(struct inode *inode, struct file *filp)
>>  
>>  	/* decrease the refcount unconditionally since the release()
>>  	   return value is ignored. */
>> -	put_device(&devnode->dev);
>> +	if (devnode->use_kref)
>> +		media_devnode_put(devnode);
>> +	else
>> +		put_device(&devnode->mdc->dev);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -222,14 +262,20 @@ static const struct file_operations media_devnode_fops = {
>>  int __must_check media_devnode_register(struct media_devnode *devnode,
>>  					struct module *owner)
>>  {
>> +	struct media_devnode_cdev *mdc;
>>  	int minor;
>>  	int ret;
>>  
>> +	mdc = kzalloc(sizeof(*mdc), GFP_KERNEL);
> 
> Why is this no longer embedded in struct media_devnode? I see no reason for that
> change.

The lifetime of the cdev can be different from the media_devnode.

> 
>> +	if (!mdc)
>> +		return -ENOMEM;
>> +
>>  	/* Part 1: Find a free minor number */
>>  	mutex_lock(&media_devnode_lock);
>>  	minor = find_next_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES, 0);
>>  	if (minor == MEDIA_NUM_DEVICES) {
>>  		mutex_unlock(&media_devnode_lock);
>> +		kfree(mdc);
>>  		pr_err("could not get a free minor\n");
>>  		return -ENFILE;
>>  	}
>> @@ -240,23 +286,26 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>>  	devnode->minor = minor;
>>  
>>  	/* Part 2: Initialize and register the character device */
>> -	cdev_init(&devnode->cdev, &media_devnode_fops);
>> -	devnode->cdev.owner = owner;
>> +	cdev_init(&mdc->cdev, &media_devnode_fops);
>> +	mdc->cdev.owner = owner;
>> +	mdc->devnode = devnode;
>> +	devnode->mdc = mdc;
>>  
>> -	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
>> +	ret = cdev_add(&mdc->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor),
>> +		       1);
>>  	if (ret < 0) {
>>  		pr_err("%s: cdev_add failed\n", __func__);
>>  		goto error;
>>  	}
>>  
>>  	/* Part 3: Register the media device */
>> -	devnode->dev.bus = &media_bus_type;
>> -	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
>> -	devnode->dev.release = media_devnode_release;
>> +	devnode->mdc->dev.bus = &media_bus_type;
>> +	devnode->mdc->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
>> +	devnode->mdc->dev.release = media_devnode_device_release;
>>  	if (devnode->parent)
>> -		devnode->dev.parent = devnode->parent;
>> -	dev_set_name(&devnode->dev, "media%d", devnode->minor);
>> -	ret = device_register(&devnode->dev);
>> +		devnode->mdc->dev.parent = devnode->parent;
>> +	dev_set_name(&devnode->mdc->dev, "media%d", devnode->minor);
>> +	ret = device_register(&devnode->mdc->dev);
>>  	if (ret < 0) {
>>  		pr_err("%s: device_register failed\n", __func__);
>>  		goto error;
>> @@ -265,14 +314,19 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>>  	/* Part 4: Activate this minor. The char device can now be used. */
>>  	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>>  
>> +	/* For us, reference released in media_devnode_release(). */
>> +	get_device(&devnode->mdc->dev);
>> +
> 
> I would really like to see this synced with what drivers/staging/media/cec/cec-core.c is
> doing. That code is similar to what other subsystems are doing and seems to be according
> to best practices. What we do in v4l2-dev.c is certainly NOT according to best practices.
> 
> One of the things I'd like to do is to fix v4l2-dev.c as well, but that will take time.
> 
> Ideally all the code that registers device nodes should all use the same method. That makes
> it easier to review and to fix issues in the future.

Good point. Let me check this and update the set accordingly.

> 
>>  	return 0;
>>  
>>  error:
>>  	mutex_lock(&media_devnode_lock);
>> -	cdev_del(&devnode->cdev);
>> +	cdev_del(&mdc->cdev);
>>  	clear_bit(devnode->minor, media_devnode_nums);
>>  	mutex_unlock(&media_devnode_lock);
>>  
>> +	kfree(mdc);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -282,16 +336,30 @@ void media_devnode_unregister(struct media_devnode *devnode)
>>  	if (!media_devnode_is_registered(devnode))
>>  		return;
>>  
>> +	dev_dbg(&devnode->mdc->dev, "unregister media devnode\n");
>> +
>> +	device_unregister(&devnode->mdc->dev);
>> +
>>  	mutex_lock(&media_devnode_lock);
>>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>>  	mutex_unlock(&media_devnode_lock);
>> -	device_unregister(&devnode->dev);
>> +
>> +	if (!devnode->use_kref) {
>> +		media_devnode_device_release(&devnode->mdc->dev);
>> +
>> +		if (devnode->release)
>> +			devnode->release(devnode);
>> +
>> +		return;
>> +	}
>> +
>> +	media_devnode_put(devnode);
>>  }
>>  
>>  /*
>>   *	Initialise media for linux
>>   */
>> -static int __init media_devnode_init(void)
>> +static int __init __media_devnode_init(void)
>>  {
>>  	int ret;
>>  
>> @@ -319,7 +387,7 @@ static void __exit media_devnode_exit(void)
>>  	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
>>  }
>>  
>> -subsys_initcall(media_devnode_init);
>> +subsys_initcall(__media_devnode_init);
>>  module_exit(media_devnode_exit)
>>  
>>  MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>> index a0f6823..4e9d066 100644
>> --- a/include/media/media-devnode.h
>> +++ b/include/media/media-devnode.h
>> @@ -28,10 +28,11 @@
>>  #ifndef _MEDIA_DEVNODE_H
>>  #define _MEDIA_DEVNODE_H
>>  
>> -#include <linux/poll.h>
>> -#include <linux/fs.h>
>> -#include <linux/device.h>
>>  #include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/kref.h>
>> +#include <linux/poll.h>
>>  
>>  /*
>>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>> @@ -65,15 +66,24 @@ struct media_file_operations {
>>  	int (*release) (struct file *);
>>  };
>>  
>> +struct media_devnode_cdev {
>> +	struct device dev;		/* media device */
>> +	struct cdev cdev;		/* character device */
>> +	struct media_devnode *devnode;	/* pointer to media_devnode */
>> +};
>> +
>> +#define to_media_devnode_cdev(cd) \
>> +	container_of(cd, struct media_devnode_cdev, dev)
>> +
>>  /**
>>   * struct media_devnode - Media device node
>>   * @media_dev:	pointer to struct &media_device
>>   * @fops:	pointer to struct &media_file_operations with media device ops
>> - * @dev:	pointer to struct &device containing the media controller device
>> - * @cdev:	struct cdev pointer character device
>>   * @parent:	parent device
>>   * @minor:	device node minor number
>>   * @flags:	flags, combination of the MEDIA_FLAG_* constants
>> + * @kref:	kref to this object
>> + * @use_kref:	tell whether we're using kref or not (TEMPORARY)
> 
> You say this is temporary, but it is never removed. I don't see why you need
> a kref at all, usually you just use put/get_device(&devnode->dev).

Well, that's perhaps worded badly. It's required as long as there are
drivers that haven't been converted yet. The set currently converts only
omap3isp driver. I think it'd be good to review the framework changes
before making more changes to the drivers.

Converting all the drivers so that they would actually be safe to remove
whilst in use would require a lot of driver changes, requiring e.g. the
removal of devm_() usage. I don't think that should be done by this
patchset.

> 
>>   * @release:	release callback called at the end of media_devnode_release()
>>   *
>>   * This structure represents a media-related device node.
>> @@ -86,20 +96,39 @@ struct media_devnode {
>>  	const struct media_file_operations *fops;
>>  
>>  	/* sysfs */
>> -	struct device dev;		/* media device */
>> -	struct cdev cdev;		/* character device */
>>  	struct device *parent;		/* device parent */
>>  
>>  	/* device info */
>>  	int minor;
>>  	unsigned long flags;		/* Use bitops to access flags */
>> +	struct media_devnode_cdev *mdc;
>> +	struct kref kref;
>> +	bool use_kref;
>>  
>>  	/* callbacks */
>>  	void (*release)(struct media_devnode *devnode);
>>  };
>>  
>> -/* dev to media_devnode */
>> -#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
>> +/**
>> + * media_devnode_init - initialise a media device node (with kref)
>> + *
>> + * devnode: the device node to be initialised
>> + */
>> +void media_devnode_init(struct media_devnode *devnode);
>> +
>> +/**
>> + * media_devnode_get - get a reference to a media device node
>> + *
>> + * @devnode: the device node to get a reference to
>> + */
>> +void media_devnode_get(struct media_devnode *devnode);
>> +
>> +/**
>> + * media_devnode_put - put a reference to a media device node
>> + *
>> + * @devnode: the device node to put a reference from
>> + */
>> +void media_devnode_put(struct media_devnode *devnode);
>>  
>>  /**
>>   * media_devnode_register - register a media device node
>>
> 
> Regards,
> 
> 	Hans
> 


-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[RFC 06/16] media: Dynamically allocate the media device

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

Allow allocating the media device dynamically. As the struct media_device
embeds struct media_devnode, the lifetime of that object is that same than
that of the media_device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 28 ++++++++++++++++++++++++++--
 include/media/media-device.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a11b3be..1bab2c6 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -543,9 +543,18 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
 {
-	dev_dbg(mdev->parent, "Media device released\n");
+	struct media_device *mdev = to_media_device(devnode);
+
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
+	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
+	mutex_destroy(&mdev->graph_mutex);
+	dev_dbg(devnode->parent, "Media device released\n");
+
+	if (devnode->use_kref)
+		kfree(mdev);
 }
 
 /**
@@ -692,6 +701,21 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
+struct media_device *media_device_alloc(void)
+{
+	struct media_device *mdev;
+
+	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return NULL;
+
+	media_devnode_init(&mdev->devnode);
+	media_device_init(mdev);
+
+	return mdev;
+}
+EXPORT_SYMBOL_GPL(media_device_alloc);
+
 void media_device_cleanup(struct media_device *mdev)
 {
 	ida_destroy(&mdev->entity_internal_idx);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index a9b33c4..cb5722b 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -428,6 +428,42 @@ static inline __must_check int media_entity_enum_init(
 void media_device_init(struct media_device *mdev);
 
 /**
+ * media_device_alloc() - Allocate and initialise a media device
+ *
+ * Allocate and initialise a media device. Returns a media device.
+ * The media device is refcounted, and this function returns a media
+ * device the refcount of which is one (1).
+ *
+ * References are taken and given using media_device_get() and
+ * media_device_put().
+ */
+struct media_device *media_device_alloc(void);
+
+/**
+ * media_device_get() - Get a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_get(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: get media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		media_devnode_get(&(mdev)->devnode);			\
+	} while (0)
+
+/**
+ * media_device_put() - Put a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_put(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: put media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		media_devnode_put(&(mdev)->devnode);			\
+	} while (0)
+
+/**
  * media_device_cleanup() - Cleanups a media device element
  *
  * @mdev:	pointer to struct &media_device
@@ -654,6 +690,8 @@ void __media_device_usb_init(struct media_device *mdev,
 			     const char *driver_name);
 
 #else
+#define media_device_get(mdev) do { } while (0)
+#define media_device_put(mdev) do { } while (0)
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
-- 
2.1.4

[RFC 07/16] media-device: struct media_device requires struct device

[Sakari Ailus]

The media device always has a device around. Require one as an argument
for media_device_alloc().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 13 +++++++++++--
 include/media/media-device.h |  4 +++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1bab2c6..1eadf02 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -701,15 +701,23 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
-struct media_device *media_device_alloc(void)
+struct media_device *media_device_alloc(struct device *dev)
 {
 	struct media_device *mdev;
 
+	dev = get_device(dev);
+	if (!dev)
+		return NULL;
+
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
-	if (!mdev)
+	if (!mdev) {
+		put_device(dev);
 		return NULL;
+	}
 
 	media_devnode_init(&mdev->devnode);
+
+	mdev->dev = dev;
 	media_device_init(mdev);
 
 	return mdev;
@@ -722,6 +730,7 @@ void media_device_cleanup(struct media_device *mdev)
 	mdev->entity_internal_idx_max = 0;
 	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
+	put_device(mdev->dev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index cb5722b..5bf97e5 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -430,6 +430,8 @@ void media_device_init(struct media_device *mdev);
 /**
  * media_device_alloc() - Allocate and initialise a media device
  *
+ * @dev:	The associated struct device pointer
+ *
  * Allocate and initialise a media device. Returns a media device.
  * The media device is refcounted, and this function returns a media
  * device the refcount of which is one (1).
@@ -437,7 +439,7 @@ void media_device_init(struct media_device *mdev);
  * References are taken and given using media_device_get() and
  * media_device_put().
  */
-struct media_device *media_device_alloc(void);
+struct media_device *media_device_alloc(struct device *dev);
 
 /**
  * media_device_get() - Get a reference to a media device
-- 
2.1.4

[RFC 08/16] media: Provide a way to the driver to set a private pointer

[Sakari Ailus]

Now that the media device can be allocated dynamically, drivers have no
longer a way to conveniently obtain the driver private data structure.
Provide one again in the form of a private pointer passed to the
media_device_alloc() function.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c |  3 ++-
 include/media/media-device.h | 15 ++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1eadf02..eae57c6 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -701,7 +701,7 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
-struct media_device *media_device_alloc(struct device *dev)
+struct media_device *media_device_alloc(struct device *dev, void *priv)
 {
 	struct media_device *mdev;
 
@@ -719,6 +719,7 @@ struct media_device *media_device_alloc(struct device *dev)
 
 	mdev->dev = dev;
 	media_device_init(mdev);
+	mdev->priv = priv;
 
 	return mdev;
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 5bf97e5..8582e23 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -283,6 +283,7 @@ struct media_entity_notify {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @priv:	A pointer to driver private data
  * @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 +349,7 @@ struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	void *priv;
 
 	char model[32];
 	char driver_name[32];
@@ -431,6 +433,7 @@ void media_device_init(struct media_device *mdev);
  * media_device_alloc() - Allocate and initialise a media device
  *
  * @dev:	The associated struct device pointer
+ * @priv:	pointer to a driver private data structure
  *
  * Allocate and initialise a media device. Returns a media device.
  * The media device is refcounted, and this function returns a media
@@ -439,7 +442,7 @@ void media_device_init(struct media_device *mdev);
  * References are taken and given using media_device_get() and
  * media_device_put().
  */
-struct media_device *media_device_alloc(struct device *dev);
+struct media_device *media_device_alloc(struct device *dev, void *priv);
 
 /**
  * media_device_get() - Get a reference to a media device
@@ -466,6 +469,16 @@ struct media_device *media_device_alloc(struct device *dev);
 	} while (0)
 
 /**
+ * media_device_priv() - Obtain the driver private pointer
+ *
+ * Returns a pointer passed to the media_device_alloc() function.
+ */
+static inline void *media_device_priv(struct media_device *mdev)
+{
+	return mdev->priv;
+}
+
+/**
  * media_device_cleanup() - Cleanups a media device element
  *
  * @mdev:	pointer to struct &media_device
-- 
2.1.4

[RFC 09/16] media: Add release callback for media device

[Sakari Ailus]

The release callback may be used by the driver to signal the release of
the media device. This makes it possible to embed a driver private struct
to the same memory allocation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 11 ++++++++++-
 include/media/media-device.h |  8 +++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index eae57c6..7538572 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -553,6 +553,9 @@ static void media_device_release(struct media_devnode *devnode)
 	mutex_destroy(&mdev->graph_mutex);
 	dev_dbg(devnode->parent, "Media device released\n");
 
+	if (mdev->release)
+		mdev->release(mdev);
+
 	if (devnode->use_kref)
 		kfree(mdev);
 }
@@ -701,10 +704,16 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
-struct media_device *media_device_alloc(struct device *dev, void *priv)
+struct media_device *media_device_alloc(struct device *dev, void *priv,
+					size_t size)
 {
 	struct media_device *mdev;
 
+	if (!size)
+		size = sizeof(*mdev);
+	else if (WARN_ON(size < sizeof(*mdev)))
+		return NULL;
+
 	dev = get_device(dev);
 	if (!dev)
 		return NULL;
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 8582e23..34671e1 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -383,6 +383,7 @@ struct media_device {
 
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
+	void (*release)(struct media_device *mdev);
 };
 
 /* We don't need to include pci.h or usb.h here */
@@ -434,15 +435,20 @@ void media_device_init(struct media_device *mdev);
  *
  * @dev:	The associated struct device pointer
  * @priv:	pointer to a driver private data structure
+ * @size:	size of a driver structure containing the media device
  *
  * Allocate and initialise a media device. Returns a media device.
  * The media device is refcounted, and this function returns a media
  * device the refcount of which is one (1).
  *
+ * The size parameter can be zero if the media_device is not embedded
+ * in another struct.
+ *
  * References are taken and given using media_device_get() and
  * media_device_put().
  */
-struct media_device *media_device_alloc(struct device *dev, void *priv);
+struct media_device *media_device_alloc(struct device *dev, void *priv,
+					size_t size);
 
 /**
  * media_device_get() - Get a reference to a media device
-- 
2.1.4

[RFC 10/16] media: Document media device allocation API

[Sakari Ailus]

Document the addition of the media_device_alloc() function to allocate a
media device. Also, document how reference counting and releasing a media
device works.

Deprecate API elements which are no longer needed with dynamically
allocated media devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-device.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/media/media-device.h b/include/media/media-device.h
index 34671e1..5243737 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -61,16 +61,28 @@
  *
  * * Media device:
  *
- * A media device is represented by a struct &media_device instance, defined in
- * include/media/media-device.h. Allocation of the structure is handled by the
- * media device driver, usually by embedding the &media_device instance in a
- * larger driver-specific structure.
+ * A media device is represented by a struct &media_device instance,
+ * defined in include/media/media-device.h. The memory for a media
+ * device is allocated using media_device_alloc() function.
+ * media_device_alloc() may also be used to allocate extra memory for
+ * driver's purpose. media_device_priv() may be used to obtain a
+ * driver's private pointer related to a media device. The two are
+ * intended as alternatives, whichever serves the purpose better.
  *
  * Drivers register media device instances by calling
  *	__media_device_register() via the macro media_device_register()
  * and unregistered by calling
  *	media_device_unregister().
  *
+ * The media device structure itself is not reference counted, but it
+ * relies on the kref which is part of struct media_devnode which it
+ * embeds. Acquiring a reference to a media device requires calling
+ * media_device_get() on the media device, likewise releasing a
+ * reference is done using media_device_put(). Once the last reference
+ * is gone, the media device is released iff it was allocated using
+ * media_device_alloc(). The media device's release() callback is
+ * called once the last reference has been released.
+ *
  * * Entities, pads and links:
  *
  * - Entities
@@ -419,6 +431,9 @@ static inline __must_check int media_entity_enum_init(
  *
  * @mdev:	pointer to struct &media_device
  *
+ * DEPRECATED --- use media_device_alloc() and rely on reference
+ * counts and the release callback instead.
+ *
  * This function initializes the media device prior to its registration.
  * The media device initialization and registration is split in two functions
  * to avoid race conditions and make the media device available to user-space
@@ -489,6 +504,9 @@ static inline void *media_device_priv(struct media_device *mdev)
  *
  * @mdev:	pointer to struct &media_device
  *
+ * DEPRECATED --- use media_device_alloc() and rely on reference
+ * counts and the release callback instead.
+ *
  * This function that will destroy the graph_mutex that is
  * initialized in media_device_init().
  */
-- 
2.1.4

[RFC 11/16] v4l: Acquire a reference to the media device for every video device

[Sakari Ailus]

The video device depends on the existence of its media device --- if there
is one. Acquire a reference to it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 70b559d..80cc675 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -171,6 +171,7 @@ static void v4l2_device_release(struct device *cd)
 {
 	struct video_device *vdev = to_video_device(cd);
 	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+	struct media_device *mdev = v4l2_dev->mdev;
 
 	mutex_lock(&videodev_lock);
 	if (WARN_ON(video_device[vdev->minor] != vdev)) {
@@ -194,7 +195,7 @@ static void v4l2_device_release(struct device *cd)
 	mutex_unlock(&videodev_lock);
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev) {
+	if (mdev) {
 		/* Remove interfaces and interface links */
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -220,6 +221,11 @@ static void v4l2_device_release(struct device *cd)
 	/* Decrease v4l2_device refcount */
 	if (v4l2_dev)
 		v4l2_device_put(v4l2_dev);
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (mdev)
+		media_device_put(mdev);
+#endif
 }
 
 static struct class video_class = {
@@ -808,6 +814,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 
 	/* FIXME: how to create the other interface links? */
 
+	media_device_get(vdev->v4l2_dev->mdev);
 #endif
 	return 0;
 }
-- 
2.1.4

[RFC 12/16] media: Shuffle functions around

[Sakari Ailus]

As the call paths of the functions in question will change, move them
around in anticipation of that. No other changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 90 ++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7538572..26fe37f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -543,23 +543,6 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *devnode)
-{
-	struct media_device *mdev = to_media_device(devnode);
-
-	ida_destroy(&mdev->entity_internal_idx);
-	mdev->entity_internal_idx_max = 0;
-	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
-	mutex_destroy(&mdev->graph_mutex);
-	dev_dbg(devnode->parent, "Media device released\n");
-
-	if (mdev->release)
-		mdev->release(mdev);
-
-	if (devnode->use_kref)
-		kfree(mdev);
-}
-
 /**
  * media_device_register_entity - Register an entity with a media device
  * @mdev:	The media device
@@ -680,6 +663,34 @@ void media_device_unregister_entity(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+int __must_check media_device_register_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	list_add_tail(&nptr->list, &mdev->entity_notify);
+	mutex_unlock(&mdev->graph_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
+
+/*
+ * Note: Should be called with mdev->lock held.
+ */
+static void __media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	list_del(&nptr->list);
+}
+
+void media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	__media_device_unregister_entity_notify(mdev, nptr);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+
 /**
  * media_device_init() - initialize a media device
  * @mdev:	The media device
@@ -744,6 +755,23 @@ void media_device_cleanup(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
+static void media_device_release(struct media_devnode *devnode)
+{
+	struct media_device *mdev = to_media_device(devnode);
+
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
+	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
+	mutex_destroy(&mdev->graph_mutex);
+	dev_dbg(devnode->parent, "Media device released\n");
+
+	if (mdev->release)
+		mdev->release(mdev);
+
+	if (devnode->use_kref)
+		kfree(mdev);
+}
+
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
@@ -773,34 +801,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
-int __must_check media_device_register_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	list_add_tail(&nptr->list, &mdev->entity_notify);
-	mutex_unlock(&mdev->graph_mutex);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
-
-/*
- * Note: Should be called with mdev->lock held.
- */
-static void __media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	list_del(&nptr->list);
-}
-
-void media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	__media_device_unregister_entity_notify(mdev, nptr);
-	mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
-
 void media_device_unregister(struct media_device *mdev)
 {
 	struct media_entity *entity;
-- 
2.1.4

[RFC 13/16] media-device: Postpone graph object removal until free

[Sakari Ailus]

The media device itself will be unregistered based on it being unbound and
driver's remove callback being called. The graph objects themselves may
still be in use; rely on the kref release callback to release them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 26fe37f..85fb663 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -758,6 +758,26 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 static void media_device_release(struct media_devnode *devnode)
 {
 	struct media_device *mdev = to_media_device(devnode);
+	struct media_entity *entity;
+	struct media_entity *next;
+	struct media_interface *intf, *tmp_intf;
+	struct media_entity_notify *notify, *nextp;
+
+	/* Remove all entities from the media device */
+	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
+		__media_device_unregister_entity(entity);
+
+	/* Remove all entity_notify callbacks from the media device */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
+		__media_device_unregister_entity_notify(mdev, notify);
+
+	/* Remove all interfaces from the media device */
+	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+				 graph_obj.list) {
+		__media_remove_intf_links(intf);
+		media_gobj_destroy(&intf->graph_obj);
+		kfree(intf);
+	}
 
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
@@ -803,38 +823,14 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 
 void media_device_unregister(struct media_device *mdev)
 {
-	struct media_entity *entity;
-	struct media_entity *next;
-	struct media_interface *intf, *tmp_intf;
-	struct media_entity_notify *notify, *nextp;
-
 	if (mdev == NULL)
 		return;
 
 	mutex_lock(&mdev->graph_mutex);
-
-	/* Check if mdev was ever registered at all */
 	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
-
-	/* Remove all entities from the media device */
-	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		__media_device_unregister_entity(entity);
-
-	/* Remove all entity_notify callbacks from the media device */
-	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
-		__media_device_unregister_entity_notify(mdev, notify);
-
-	/* Remove all interfaces from the media device */
-	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
-				 graph_obj.list) {
-		__media_remove_intf_links(intf);
-		media_gobj_destroy(&intf->graph_obj);
-		kfree(intf);
-	}
-
 	mutex_unlock(&mdev->graph_mutex);
 
 	device_remove_file(&mdev->devnode.mdc->dev, &dev_attr_model);
-- 
2.1.4

[RFC 14/16] omap3isp: Allocate the media device dynamically

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c      | 23 ++++++++++++-----------
 drivers/media/platform/omap3isp/isp.h      |  2 +-
 drivers/media/platform/omap3isp/ispvideo.c |  2 +-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 5d54e2c..b09d2508 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1597,8 +1597,7 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
 	v4l2_device_unregister(&isp->v4l2_dev);
-	media_device_unregister(&isp->media_dev);
-	media_device_cleanup(&isp->media_dev);
+	media_device_unregister(isp->media_dev);
 }
 
 static int isp_link_entity(
@@ -1676,14 +1675,16 @@ static int isp_register_entities(struct isp_device *isp)
 {
 	int ret;
 
-	isp->media_dev.dev = isp->dev;
-	strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
-		sizeof(isp->media_dev.model));
-	isp->media_dev.hw_revision = isp->revision;
-	isp->media_dev.link_notify = v4l2_pipeline_link_notify;
-	media_device_init(&isp->media_dev);
+	isp->media_dev = media_device_alloc(isp->dev, isp, 0);
+	if (!isp->media_dev)
+		return -ENOMEM;
+
+	strlcpy(isp->media_dev->model, "TI OMAP3 ISP",
+		sizeof(isp->media_dev->model));
+	isp->media_dev->hw_revision = isp->revision;
+	isp->media_dev->link_notify = v4l2_pipeline_link_notify;
 
-	isp->v4l2_dev.mdev = &isp->media_dev;
+	isp->v4l2_dev.mdev = isp->media_dev;
 	ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
 	if (ret < 0) {
 		dev_err(isp->dev, "%s: V4L2 device registration failed (%d)\n",
@@ -2161,7 +2162,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct isp_bus_cfg *bus;
 	int ret;
 
-	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
+	ret = media_entity_enum_init(&isp->crashed, isp->media_dev);
 	if (ret)
 		return ret;
 
@@ -2179,7 +2180,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	if (ret < 0)
 		return ret;
 
-	return media_device_register(&isp->media_dev);
+	return media_device_register(isp->media_dev);
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 7e6f663..7378279 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -176,7 +176,7 @@ struct isp_xclk {
 struct isp_device {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_async_notifier notifier;
-	struct media_device media_dev;
+	struct media_device *media_dev;
 	struct device *dev;
 	u32 revision;
 
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 7d9f359..45ef38c 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1077,7 +1077,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
 
-	ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
+	ret = media_entity_enum_init(&pipe->ent_enum, video->isp->media_dev);
 	if (ret)
 		goto err_enum_init;
 
-- 
2.1.4

[RFC 15/16] omap3isp: Release the isp device struct by media device callback

[Sakari Ailus]

Use the media device release callback to release the isp device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the isp driver is being
unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index b09d2508..a0dff24 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1671,6 +1671,8 @@ static int isp_link_entity(
 	return media_create_pad_link(entity, i, input, pad, flags);
 }
 
+static void isp_release(struct media_device *mdev);
+
 static int isp_register_entities(struct isp_device *isp)
 {
 	int ret;
@@ -1683,6 +1685,7 @@ static int isp_register_entities(struct isp_device *isp)
 		sizeof(isp->media_dev->model));
 	isp->media_dev->hw_revision = isp->revision;
 	isp->media_dev->link_notify = v4l2_pipeline_link_notify;
+	isp->media_dev->release = isp_release;
 
 	isp->v4l2_dev.mdev = isp->media_dev;
 	ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
@@ -1943,6 +1946,20 @@ static void isp_detach_iommu(struct isp_device *isp)
 	iommu_group_remove_device(isp->dev);
 }
 
+static void isp_release(struct media_device *mdev)
+{
+	struct isp_device *isp = media_device_priv(mdev);
+
+	isp_cleanup_modules(isp);
+	isp_xclk_cleanup(isp);
+
+	__omap3isp_get(isp, false);
+	isp_detach_iommu(isp);
+	__omap3isp_put(isp, false);
+
+	media_entity_enum_cleanup(&isp->crashed);
+}
+
 static int isp_attach_iommu(struct isp_device *isp)
 {
 	struct dma_iommu_mapping *mapping;
@@ -2003,14 +2020,6 @@ static int isp_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&isp->notifier);
 	isp_unregister_entities(isp);
-	isp_cleanup_modules(isp);
-	isp_xclk_cleanup(isp);
-
-	__omap3isp_get(isp, false);
-	isp_detach_iommu(isp);
-	__omap3isp_put(isp, false);
-
-	media_entity_enum_cleanup(&isp->crashed);
 
 	return 0;
 }
-- 
2.1.4

[RFC 16/16] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.

Instead, rely on the media device which will stick around until all users
are gone.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c         | 38 ++++++++++++++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c     |  3 ++-
 drivers/media/platform/omap3isp/isph3a_aewb.c | 20 +++++++++-----
 drivers/media/platform/omap3isp/isph3a_af.c   | 20 +++++++++-----
 drivers/media/platform/omap3isp/isphist.c     |  5 ++--
 drivers/media/platform/omap3isp/ispstat.c     |  2 ++
 6 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index a0dff24..b11818f 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1370,7 +1370,7 @@ static int isp_get_clocks(struct isp_device *isp)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
-		clk = devm_clk_get(isp->dev, isp_clocks[i]);
+		clk = clk_get(isp->dev, isp_clocks[i]);
 		if (IS_ERR(clk)) {
 			dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
 			return PTR_ERR(clk);
@@ -1382,6 +1382,14 @@ static int isp_get_clocks(struct isp_device *isp)
 	return 0;
 }
 
+static void isp_put_clocks(struct isp_device *isp)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
+		clk_put(isp->clock[i]);
+}
+
 /*
  * omap3isp_get - Acquire the ISP resource.
  *
@@ -1596,7 +1604,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_af);
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
-	v4l2_device_unregister(&isp->v4l2_dev);
 	media_device_unregister(isp->media_dev);
 }
 
@@ -1950,6 +1957,8 @@ static void isp_release(struct media_device *mdev)
 {
 	struct isp_device *isp = media_device_priv(mdev);
 
+	v4l2_device_unregister(&isp->v4l2_dev);
+
 	isp_cleanup_modules(isp);
 	isp_xclk_cleanup(isp);
 
@@ -1958,6 +1967,10 @@ static void isp_release(struct media_device *mdev)
 	__omap3isp_put(isp, false);
 
 	media_entity_enum_cleanup(&isp->crashed);
+
+	isp_put_clocks(isp);
+
+	kfree(isp);
 }
 
 static int isp_attach_iommu(struct isp_device *isp)
@@ -2210,7 +2223,7 @@ static int isp_probe(struct platform_device *pdev)
 	int ret;
 	int i, m;
 
-	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
+	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
 	if (!isp) {
 		dev_err(&pdev->dev, "could not allocate memory\n");
 		return -ENOMEM;
@@ -2219,21 +2232,23 @@ static int isp_probe(struct platform_device *pdev)
 	ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
 				   &isp->phy_type);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 						      "syscon");
-	if (IS_ERR(isp->syscon))
-		return PTR_ERR(isp->syscon);
+	if (IS_ERR(isp->syscon)) {
+		ret = PTR_ERR(isp->syscon);
+		goto error_release_isp;
+	}
 
 	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
 					 &isp->syscon_offset);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
 	if (ret < 0)
-		return ret;
+		goto error_release_isp;
 
 	isp->autoidle = autoidle;
 
@@ -2250,8 +2265,8 @@ static int isp_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, isp);
 
 	/* Regulators */
-	isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1");
-	isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2");
+	isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1");
+	isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2");
 
 	/* Clocks
 	 *
@@ -2383,6 +2398,9 @@ error_isp:
 	__omap3isp_put(isp, false);
 error:
 	mutex_destroy(&isp->isp_mutex);
+	isp_put_clocks(isp);
+error_release_isp:
+	kfree(isp);
 
 	return ret;
 }
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..d49ce8a 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 	 */
 	if (isp->revision == ISP_REVISION_2_0) {
-		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
+		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
@@ -1163,4 +1163,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
 
 	omap3isp_video_cleanup(&ccp2->video_in);
 	media_entity_cleanup(&ccp2->subdev.entity);
+	regulator_put(ccp2->vdds_csib);
 }
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index ccaf92f..130df8b 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 {
 	struct ispstat *aewb = &isp->isp_aewb;
 	struct omap3isp_h3a_aewb_config *aewb_cfg;
-	struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+	struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+	int ret;
 
-	aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+	aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
 	if (!aewb_cfg)
 		return -ENOMEM;
 
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	aewb->isp = isp;
 
 	/* Set recover state configuration */
-	aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
-					GFP_KERNEL);
+	aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
 	if (!aewb_recover_cfg) {
 		dev_err(aewb->isp->dev, "AEWB: cannot allocate memory for "
 					"recover configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,20 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
 		dev_err(aewb->isp->dev, "AEWB: recover configuration is "
 					"invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
 	aewb->recover_priv = aewb_recover_cfg;
 
 	return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err:
+	kfree(aewb_cfg);
+	kfree(aewb_recover_cfg);
+
+	return ret;
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 92937f7..7eecf97 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 {
 	struct ispstat *af = &isp->isp_af;
 	struct omap3isp_h3a_af_config *af_cfg;
-	struct omap3isp_h3a_af_config *af_recover_cfg;
+	struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
+	int ret;
 
-	af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
+	af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
 	if (af_cfg == NULL)
 		return -ENOMEM;
 
@@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	af->isp = isp;
 
 	/* Set recover state configuration */
-	af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
-				      GFP_KERNEL);
+	af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
 	if (!af_recover_cfg) {
 		dev_err(af->isp->dev, "AF: cannot allocate memory for recover "
 				      "configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +382,20 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	if (h3a_af_validate_params(af, af_recover_cfg)) {
 		dev_err(af->isp->dev, "AF: recover configuration is "
 				      "invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
 	af->recover_priv = af_recover_cfg;
 
 	return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err:
+	kfree(af_cfg);
+	kfree(af_recover_cfg);
+
+	return ret;
 }
 
 void omap3isp_h3a_af_cleanup(struct isp_device *isp)
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index 7138b04..976cab0 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp)
 {
 	struct ispstat *hist = &isp->isp_hist;
 	struct omap3isp_hist_config *hist_cfg;
-	int ret = -1;
+	int ret;
 
-	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
+	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
 	if (hist_cfg == NULL)
 		return -ENOMEM;
 
@@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp)
 	if (ret) {
 		if (hist->dma_ch)
 			dma_release_channel(hist->dma_ch);
+		kfree(hist_cfg);
 	}
 
 	return ret;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 1b9217d..1c1365f 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1059,4 +1059,6 @@ void omap3isp_stat_cleanup(struct ispstat *stat)
 	mutex_destroy(&stat->ioctl_lock);
 	isp_stat_bufs_free(stat);
 	kfree(stat->buf);
+	kfree(stat->priv);
+	kfree(stat->recover_priv);
 }
-- 
2.1.4

Re: [RFC 00/16] Make use of kref in media device, grab references as needed

[Mauro Carvalho Chehab]

Em Fri, 15 Jul 2016 01:34:55 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi folks,
> 
> I've been working on this for some time now but only got the full patchset
> working some moments ago. The patchset properly, I believe, fixes the
> issue of removing a device whilst streaming.
> 
> Media device is refcounted and its memory is only released once the last
> reference is gone: unregistering is simply unregistering, it no longer
> should release memory which could be further accessed.
> 
> A video node or a sub-device node also gets a reference to the media
> device, i.e. the release function of the video device node will release
> its reference to the media device. The same goes for file handles to the
> media device.
> 
> As a side effect of refcounting the media device, it is allocate together
> with the media devnode. The driver may also rely its own resources to the
> media device. Alternatively there's also a priv field to hold drivers
> private pointer (for container_of() is an option in this case).
> 
> I've tested this by manually unbinding the omap3isp platform device while
> streaming. Driver changes are required for this to work; by not using
> dynamic allocation (i.e. media_device_alloc()) the old behaviour is still
> supported. This is still unlikely to be a grave problem as there are not
> that many device drivers that support physically removable devices. We've
> had this problem for other devices for many years without paying much
> notice --- that doesn't mean I don't think at least drivers for removable
> devices shouldn't be changed as part of the set later on, I'd just like to
> get review comments on the approach first.
> 
> The three patches that originally partially resolved some of these issues
> are reverted in the beginning of the set. I'm still posting this as an RFC
> mainly since the testing is somewhat limited so far.


I didn't look inside this patch series. Won't likely have time to
look at core changes before the end of the merge window. However,
I found several structural problems on this RFC:

1) Please do incremental changes, instead of reverting patches. It is
really hard for reviewers to be sure that nothing breaks when someone
simply reverts a previous approach and add its own.

2) Each individual patch should not cause regressions to none of
the existing drivers or to the core. The revert re-introduces bugs.

3) Each patch should not break compilation. Patch 06/16, for example,
changes the structure used by the release method:

-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)

Without touching a single driver. That means compilation breakages.
This is not acceptable upstream.

It should be touching *all* drivers that use the function altogether.

4) From a very quick look at the series, without trying to compile the
series (with would very likely break), it seems that all drivers that
uses the media controller should be migrated to the new way.

It means that you'll need to patch all drivers altogether as you're
changing the kAPI at the same patch you change it.

So, please rework your patch series to make it easier for everybody
to review and test it.

Thanks,
Mauro

Re: [RFC 00/16] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Hi Mauro,

Thank you for your reply.

Mauro Carvalho Chehab wrote:
> Em Fri, 15 Jul 2016 01:34:55 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
>> Hi folks,
>>
>> I've been working on this for some time now but only got the full patchset
>> working some moments ago. The patchset properly, I believe, fixes the
>> issue of removing a device whilst streaming.
>>
>> Media device is refcounted and its memory is only released once the last
>> reference is gone: unregistering is simply unregistering, it no longer
>> should release memory which could be further accessed.
>>
>> A video node or a sub-device node also gets a reference to the media
>> device, i.e. the release function of the video device node will release
>> its reference to the media device. The same goes for file handles to the
>> media device.
>>
>> As a side effect of refcounting the media device, it is allocate together
>> with the media devnode. The driver may also rely its own resources to the
>> media device. Alternatively there's also a priv field to hold drivers
>> private pointer (for container_of() is an option in this case).
>>
>> I've tested this by manually unbinding the omap3isp platform device while
>> streaming. Driver changes are required for this to work; by not using
>> dynamic allocation (i.e. media_device_alloc()) the old behaviour is still
>> supported. This is still unlikely to be a grave problem as there are not
>> that many device drivers that support physically removable devices. We've
>> had this problem for other devices for many years without paying much
>> notice --- that doesn't mean I don't think at least drivers for removable
>> devices shouldn't be changed as part of the set later on, I'd just like to
>> get review comments on the approach first.
>>
>> The three patches that originally partially resolved some of these issues
>> are reverted in the beginning of the set. I'm still posting this as an RFC
>> mainly since the testing is somewhat limited so far.
> 
> 
> I didn't look inside this patch series. Won't likely have time to
> look at core changes before the end of the merge window. However,
> I found several structural problems on this RFC:
> 
> 1) Please do incremental changes, instead of reverting patches. It is
> really hard for reviewers to be sure that nothing breaks when someone
> simply reverts a previous approach and add its own.

I believe people are more familiar with the state of the code with the
reverts than without them. The first two reverted patches I don't really
have a problem with, but they depend on the third reverted patch which
is more problematic and they'll no longer be needed afterwards. To
refresh our memory:

<URL:http://www.spinics.net/lists/linux-media/msg100355.html>
<URL:http://www.spinics.net/lists/linux-media/msg100927.html>
<URL:http://www.spinics.net/lists/linux-media/msg100952.html>

> 
> 2) Each individual patch should not cause regressions to none of
> the existing drivers or to the core. The revert re-introduces bugs.

We've had the problem for five years without even realising it. What's
merged now is a workaround that avoids *some* of the underlying problems.

With the current media-master, the system still crashes if the device is
removed during video streaming. With this set (and appropriate driver
changes) it does not. Driver changes alone are not enough to fix this
either.

> 
> 3) Each patch should not break compilation. Patch 06/16, for example,
> changes the structure used by the release method:
> 
> -static void media_device_release(struct media_devnode *mdev)
> +static void media_device_release(struct media_devnode *devnode)
> 
> Without touching a single driver. That means compilation breakages.
> This is not acceptable upstream.
> 
> It should be touching *all* drivers that use the function altogether.

This change you're referring to in patch 06/16 changes the name of the
argument of a function to devnode. This change was missing from patch
"[media] media-devnode: fix namespace mess".

What comes to media_device_alloc() and media_device_get()/put(), their
use is optional. Driver changes are needed at least for drivers that can
be removed physically from the system. Once all drivers are converted,
we can remove the old API.

> 
> 4) From a very quick look at the series, without trying to compile the
> series (with would very likely break), it seems that all drivers that
> uses the media controller should be migrated to the new way.
> 
> It means that you'll need to patch all drivers altogether as you're
> changing the kAPI at the same patch you change it.

I want to first get the review comments on the API itself and then move
the removable drivers to use it. Individual drivers may still have
issues with removing devices while they're in use.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 00/16] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Sakari Ailus wrote:
> I believe people are more familiar with the state of the code with the
> reverts than without them. The first two reverted patches I don't really
> have a problem with, but they depend on the third reverted patch which
> is more problematic and they'll no longer be needed afterwards. To
> refresh our memory:

"media: fix media devnode ioctl/syscall and unregister race" is actually
a part of the workaround as well:

<URL:http://www.spinics.net/lists/linux-media/msg101295.html>
<URL:http://www.spinics.net/lists/linux-media/msg101327.html>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 00/16] Make use of kref in media device, grab references as needed

[Mauro Carvalho Chehab]

Em Tue, 19 Jul 2016 10:27:47 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thank you for your reply.
> 
> Mauro Carvalho Chehab wrote:
> > Em Fri, 15 Jul 2016 01:34:55 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> >> Hi folks,
> >>
> >> I've been working on this for some time now but only got the full patchset
> >> working some moments ago. The patchset properly, I believe, fixes the
> >> issue of removing a device whilst streaming.
> >>
> >> Media device is refcounted and its memory is only released once the last
> >> reference is gone: unregistering is simply unregistering, it no longer
> >> should release memory which could be further accessed.
> >>
> >> A video node or a sub-device node also gets a reference to the media
> >> device, i.e. the release function of the video device node will release
> >> its reference to the media device. The same goes for file handles to the
> >> media device.
> >>
> >> As a side effect of refcounting the media device, it is allocate together
> >> with the media devnode. The driver may also rely its own resources to the
> >> media device. Alternatively there's also a priv field to hold drivers
> >> private pointer (for container_of() is an option in this case).
> >>
> >> I've tested this by manually unbinding the omap3isp platform device while
> >> streaming. Driver changes are required for this to work; by not using
> >> dynamic allocation (i.e. media_device_alloc()) the old behaviour is still
> >> supported. This is still unlikely to be a grave problem as there are not
> >> that many device drivers that support physically removable devices. We've
> >> had this problem for other devices for many years without paying much
> >> notice --- that doesn't mean I don't think at least drivers for removable
> >> devices shouldn't be changed as part of the set later on, I'd just like to
> >> get review comments on the approach first.
> >>
> >> The three patches that originally partially resolved some of these issues
> >> are reverted in the beginning of the set. I'm still posting this as an RFC
> >> mainly since the testing is somewhat limited so far.  
> > 
> > 
> > I didn't look inside this patch series. Won't likely have time to
> > look at core changes before the end of the merge window. However,
> > I found several structural problems on this RFC:
> > 
> > 1) Please do incremental changes, instead of reverting patches. It is
> > really hard for reviewers to be sure that nothing breaks when someone
> > simply reverts a previous approach and add its own.  
> 
> I believe people are more familiar with the state of the code with the
> reverts than without them. 

What people are more familiar depends on what each person knows and
when he lastly looked at the code. Kernel's policy is to not build patches
based on what you think the others know, but, instead, we develop stuff
incrementally.

So, if you want such patch series to be reviewed and merged, you should
apply incremental changes, not destroy everything before applying
your stuff, because your work were based on a past version.

Btw, one of the things I'm missing on this series is what's the problem
you're still facing with the upstream version, as you didn't add any
descripton and OOPSes that you're noticing upstream.

Anyone reviewing this series would need to be able to reproduce such
problem, and add to their existing test case scenarios, to be sure
that the solution won't cause regressions to their own scenarios
and will solve for yours.

> The first two reverted patches I don't really
> have a problem with, but they depend on the third reverted patch which
> is more problematic and they'll no longer be needed afterwards. To
> refresh our memory:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg100355.html>
> <URL:http://www.spinics.net/lists/linux-media/msg100927.html>
> <URL:http://www.spinics.net/lists/linux-media/msg100952.html>
> 
> > 
> > 2) Each individual patch should not cause regressions to none of
> > the existing drivers or to the core. The revert re-introduces bugs.  
> 
> We've had the problem for five years without even realising it. What's
> merged now is a workaround that avoids *some* of the underlying problems.

I don't doubt that there are still underlying problems. Making
a race-free solution for the drivers bind/unbind scenario is not
trivial. Yet, as I said before, we need not only the patches but
the scenarios you're testing, for us to be able to reproduce your
problem and verify that your solution solves it, without causing
regressions to other test scenarios.

> With the current media-master, the system still crashes if the device is
> removed during video streaming. With this set (and appropriate driver
> changes) it does not. Driver changes alone are not enough to fix this
> either.

The best way to test such scenario would be, IMHO, to add patches for
some USB driver, like uvcvideo or au0828, as, there you can actually
physically remove the hardware while streaming.

One problem with OMAP3 driver is that the OMAP3 CPU is a single
core one. So, you won't see there any CPU concurrency issues.

> > 
> > 3) Each patch should not break compilation. Patch 06/16, for example,
> > changes the structure used by the release method:
> > 
> > -static void media_device_release(struct media_devnode *mdev)
> > +static void media_device_release(struct media_devnode *devnode)
> > 
> > Without touching a single driver. That means compilation breakages.
> > This is not acceptable upstream.
> > 
> > It should be touching *all* drivers that use the function altogether.  
> 
> This change you're referring to in patch 06/16 changes the name of the
> argument of a function to devnode. This change was missing from patch
> "[media] media-devnode: fix namespace mess".

What I'm saying is that, every time you change the arguments of a
function, *all* drivers using such function should be changed at the
same time, as otherwise, the Kernel won't build anymore after such
patch. I used patch 6/16 only as an example.

If this patch is, instead, a fixup for the "fix namespace mess"
patch, just submit it outside this RFC patch series.

> What comes to media_device_alloc() and media_device_get()/put(), their
> use is optional. Driver changes are needed at least for drivers that can
> be removed physically from the system. Once all drivers are converted,
> we can remove the old API.
> 
> > 
> > 4) From a very quick look at the series, without trying to compile the
> > series (with would very likely break), it seems that all drivers that
> > uses the media controller should be migrated to the new way.
> > 
> > It means that you'll need to patch all drivers altogether as you're
> > changing the kAPI at the same patch you change it.  
> 
> I want to first get the review comments on the API itself and then move
> the removable drivers to use it. Individual drivers may still have
> issues with removing devices while they're in use.

I can only review and test this patch series after:

1) knowing the test scenarios you're using and the OOPS you're
   getting;

2) having the changes applied to an USB driver, as my multi-core
   test machines only support USB devices.

-- 
Thanks,
Mauro