On 16/12/16 13:00, Mauro Carvalho Chehab wrote:
Em
escreveu:
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.
Ah! So, you're basically proposing to have a separate struct for
V4L2 devnode as well, right?
Makes sense.
No need for that, that's already struct video_device.
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.
Yes, such change will require first to be sure that drivers would be
doing the right thing.
So devm_ resources are released right after remove() exits, not when the last reference
goes to 0. In other words, devm_ typically can't be used for these complex scenarios,
certainly not for memory. See discussion with Laurent on irc.
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().
OK.
@@ -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?
Yes, that's right (vdev->v4l2_dev->mdev->dev).
Laurent helped me realize that this won't work either: mdev->dev is typically a
platform/pci/usb device, and that won't go away when you rmmod the driver.
So while taking a refcount on that device doesn't hurt, we also need to take a refcount
on a kref inside the mdev. Just like v4l2_device this struct has an unfortunate name.
It's not a device, but a root structure for media devices.
We really need a whiteboard for this :-(
Regards,
Hans
/* 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
Thanks,
Mauro
--
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
--
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
