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

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

Em 
 escreveu:

> On 16/12/16 11:57, Mauro Carvalho Chehab wrote:
> > 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;  
> 
> 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@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);  
> 
> 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