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

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Fri, 16 Dec 2016 11:11:25 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> > Would it make sense to enforce that dependency. Can we tie /dev/media usecount
> > to /dev/video etc. usecount? In other words:
> >
> > /dev/video is opened, then open /dev/media.  
> 
> When a device node is registered it should increase the refcount on the media_device
> (as I proposed, that would be mdev->dev). When a device node is unregistered and the
> last user disappeared, then it can decrease the media_device refcount.
> 
> So as long as anyone is using a device node, the media_device will stick around as
> well.
> 
> No need to take refcounts on open/close.

That makes sense. You're meaning something like the enclosed (untested)
patch?

> One note: as I mentioned, the video_device does not set the cdev parent correctly,
> so that bug needs to be fixed first for this to work.

Actually, __video_register_device() seems to be setting the parent
properly:

	if (vdev->dev_parent == NULL)
		vdev->dev_parent = vdev->v4l2_dev->dev;

Thanks,
Mauro

[PATCH] Be sure that the media_device won't be freed too early

This code snippet is untested.

Signed-off-by: Mauro Carvalho chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8756275e9fc4..5fdeab382069 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -706,7 +706,7 @@ int __must_check __media_device_register(struct media_device *mdev,
 	struct media_devnode *devnode;
 	int ret;
 
-	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
+	devnode = devm_kzalloc(mdev->dev, sizeof(*devnode), GFP_KERNEL);
 	if (!devnode)
 		return -ENOMEM;
 
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561ab2615..14a3c56dbcac 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -196,6 +196,7 @@ static void v4l2_device_release(struct device *cd)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	if (v4l2_dev->mdev) {
 		/* Remove interfaces and interface links */
+		put_device(v4l2_dev->mdev->dev);
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
 			media_device_unregister_entity(&vdev->entity);
@@ -810,6 +811,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 			return -ENOMEM;
 		}
 	}
+	get_device(vdev->v4l2_dev->dev);
 
 	/* FIXME: how to create the other interface links? */
 
@@ -1015,6 +1017,11 @@ void video_unregister_device(struct video_device *vdev)
 	if (!vdev || !video_is_registered(vdev))
 		return;
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (vdev->v4l2_dev->dev)
+		put_device(vdev->v4l2_dev->dev);
+#endif
+
 	mutex_lock(&videodev_lock);
 	/* This must be in a critical section to prevent a race with v4l2_open.
 	 * Once this bit has been cleared video_get may never be called again.
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 62bbed76dbbc..53f42090c762 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -188,6 +188,7 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 		err = media_device_register_entity(v4l2_dev->mdev, entity);
 		if (err < 0)
 			goto error_module;
+		get_device(v4l2_dev->mdev->dev);
 	}
 #endif
 
@@ -205,6 +206,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 
 error_unregister:
 #if defined(CONFIG_MEDIA_CONTROLLER)
+	if (v4l2_dev->mdev)
+		put_device(v4l2_dev->mdev->dev);
 	media_device_unregister_entity(entity);
 #endif
 error_module:
@@ -310,6 +313,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 		 * links are removed by the function below, in the right order
 		 */
 		media_device_unregister_entity(&sd->entity);
+		put_device(v4l2_dev->mdev->dev);
 	}
 #endif
 	video_unregister_device(sd->devnode);