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.
>
> >
> > 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.
>
> > 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).
>
> >
> > /* 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
