Re: [PATCH v6 02/25] rcar-vin: register the video device at probe time

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

Thank you for the patch.

On Wednesday, 23 August 2017 02:26:17 EEST Niklas Söderlund wrote:
> The driver registers the video device from the async complete callback
> and unregistered in the async unbind callback. This creates problems if
> if the subdevice is bound, unbound and later rebound. The second time
> video_register_device() is called it fails:
> 
>    kobject (eb3be918): tried to init an initialized object, something is
> seriously wrong.
> 
> To prevent this register the video device at prob time and don't allow

s/prob/probe/

> user-space to open the video device if the subdevice have not yet been
> bound.

s/have not yet been bound/is not bound yet/

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 42 ++++++++++++++++++++++++--
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++----------------------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>  3 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 77dff047c41c803e..aefbe8e3ccddb3e4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -74,6 +74,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity
> *entity) static int rvin_digital_notify_complete(struct v4l2_async_notifier
> *notifier) {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
>  	int ret;
> 
>  	/* Verify subdevices mbus format */
> @@ -92,7 +93,35 @@ static int rvin_digital_notify_complete(struct
> v4l2_async_notifier *notifier) return ret;
>  	}
> 
> -	return rvin_v4l2_probe(vin);
> +	/* Add the controls */
> +	/*
> +	 * Currently the subdev with the largest number of controls (13) is
> +	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> +	 * that this is a hint only: too large and you waste some memory, too
> +	 * small and there is a (very) small performance hit when looking up
> +	 * controls in the internal hash.

No need to copy the help text from the v4l2_ctrl_handler_init() documentation 
:-)

> +	 */
> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> +	if (ret < 0)
> +		return ret;

This is racy. You set vdev->ctrl_handler at probe time, but only initialize 
the control handler later. I think it would be better to leave vdev-
>ctrl_handler to NULL and only set it here after initializing the handler. You 
also need proper locking.

> +	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> +		return ret;
> +
> +	if (vin->vdev.tvnorms == 0) {
> +		/* Disable the STD API if there are no tvnorms defined */
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> +		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> +	}
> +
> +	return rvin_reset_format(vin);
>  }
> 
>  static void rvin_digital_notify_unbind(struct v4l2_async_notifier
> *notifier, @@ -102,7 +131,7 @@ static void
> rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier, struct
> rvin_dev *vin = notifier_to_vin(notifier);
> 
>  	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> -	rvin_v4l2_remove(vin);
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>  	vin->digital.subdev = NULL;

You need locking here too. Nothing prevents an open file handle from issuing a 
control ioctl after the handler is freed by the subdev unbind.

>  }
> 
> @@ -231,6 +260,10 @@ static int rvin_digital_graph_init(struct rvin_dev
> *vin) vin->notifier.unbind = rvin_digital_notify_unbind;
>  	vin->notifier.complete = rvin_digital_notify_complete;
> 
> +	ret = rvin_v4l2_probe(vin);
> +	if (ret)
> +		return ret;

Registering the V4L2 devnodes in the rvin_digital_graph_init() function sounds 
weird. Maybe you should rename the function ? And while at it, I'd rename 
rvin_v4l2_probe() to rvin_v4l2_register().

>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
> @@ -314,6 +347,11 @@ static int rcar_vin_remove(struct platform_device
> *pdev)
> 
>  	v4l2_async_notifier_unregister(&vin->notifier);
> 
> +	/* Checks internaly if handlers have been init or not */
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
> +	rvin_v4l2_remove(vin);
> +
>  	rvin_dma_remove(vin);
> 
>  	return 0;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> dd37ea8116804df3..81ff59c3b4744075 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -103,7 +103,7 @@ static void rvin_reset_crop_compose(struct rvin_dev
> *vin) vin->compose.height = vin->format.height;
>  }
> 
> -static int rvin_reset_format(struct rvin_dev *vin)
> +int rvin_reset_format(struct rvin_dev *vin)
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -781,6 +781,11 @@ static int rvin_open(struct file *file)
> 
>  	mutex_lock(&vin->lock);
> 
> +	if (!vin->digital.subdev) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}

This is racy, vin->digital.subdev is set in the bound notifier, while you 
initialize the control handler in the complete notifier. I would perform 
initializations in the bound notifier instead of the complete notifier. Please 
make sure you use proper locking.

>  	file->private_data = vin;
> 
>  	ret = v4l2_fh_open(file);
> @@ -844,9 +849,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
>  	v4l2_info(&vin->v4l2_dev, "Removing %s\n",
>  		  video_device_node_name(&vin->vdev));
> 
> -	/* Checks internaly if handlers have been init or not */
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
>  	/* Checks internaly if vdev have been init or not */
>  	video_unregister_device(&vin->vdev);
>  }
> @@ -869,41 +871,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
>  int rvin_v4l2_probe(struct rvin_dev *vin)
>  {
>  	struct video_device *vdev = &vin->vdev;
> -	struct v4l2_subdev *sd = vin_to_source(vin);
>  	int ret;
> 
> -	v4l2_set_subdev_hostdata(sd, vin);
> -
>  	vin->v4l2_dev.notify = rvin_notify;
> 
> -	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> -
> -	if (vin->vdev.tvnorms == 0) {
> -		/* Disable the STD API if there are no tvnorms defined */
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> -		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> -	}
> -
> -	/* Add the controls */
> -	/*
> -	 * Currently the subdev with the largest number of controls (13) is
> -	 * ov6550. So let's pick 16 as a hint for the control handler. Note
> -	 * that this is a hint only: too large and you waste some memory, too
> -	 * small and there is a (very) small performance hit when looking up
> -	 * controls in the internal hash.
> -	 */
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> -	if (ret < 0)
> -		return ret;
> -
>  	/* video node */
>  	vdev->fops = &rvin_fops;
>  	vdev->v4l2_dev = &vin->v4l2_dev;
> @@ -917,7 +888,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>  		V4L2_CAP_READWRITE;
> 
>  	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
> -	rvin_reset_format(vin);
> 
>  	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
>  	if (ret) {


-- 
Regards,

Laurent Pinchart