On 16/12/16 11:57, Mauro Carvalho Chehab wrote:
Em Fri, 16 Dec 2016 11:11:25 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> 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;
No, I mean this code (from cec-core.c):
/* Part 2: Initialize and register the character device */
cdev_init(&devnode->cdev, &cec_devnode_fops);
devnode->cdev.kobj.parent = &devnode->dev.kobj;
devnode->cdev.owner = owner;
ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
if (ret < 0) {
pr_err("%s: cdev_add failed\n", __func__);
goto clr_bit;
}
ret = device_add(&devnode->dev);
if (ret)
goto cdev_del;
which sets cdev.kobj.parent. And that would indeed be vdev->dev_parent.
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@xxxxxxxxxxxxxxxx>
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);
I'm not sure about this change. I *think* this would work, but *only* if all
the refcounting is 100% correct, and we know it isn't at the moment. So I
think this should be postponed until we are confident everything is correct.
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);
I think this is the wrong order: put_device should go after media_device_unregister_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);
You mean v4l2_dev->mdev->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);
Ditto.
+#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);
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
