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

[Laurent Pinchart]

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 
> ---
>  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(>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(>ctrl_handler, sd->ctrl_handler, NULL);
> + if (ret < 0)
> + return ret;
> +
> + ret = v4l2_subdev_call(sd, video, g_tvnorms, >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(>vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(>vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(>vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(>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(>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(>v4l2_dev, >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(>notifier);
> 
> + /* Checks internaly if handlers have been init or not */
> + v4l2_ctrl_handler_free(>ctrl_handler);
> +
> + rvin_v4l2_remove(vin);
> +
>   rvin_dma_remove(vin);
> 
>   return 0;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> 

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

[Niklas Söderlund]

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
user-space to open the video device if the subdevice have not yet been
bound.

Signed-off-by: Niklas Söderlund 
---
 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.
+*/
+   ret = v4l2_ctrl_handler_init(>ctrl_handler, 16);
+   if (ret < 0)
+   return ret;
+
+   ret = v4l2_ctrl_add_handler(>ctrl_handler, sd->ctrl_handler, NULL);
+   if (ret < 0)
+   return ret;
+
+   ret = v4l2_subdev_call(sd, video, g_tvnorms, >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(>vdev, VIDIOC_G_STD);
+   v4l2_disable_ioctl(>vdev, VIDIOC_S_STD);
+   v4l2_disable_ioctl(>vdev, VIDIOC_QUERYSTD);
+   v4l2_disable_ioctl(>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(>ctrl_handler);
vin->digital.subdev = NULL;
 }
 
@@ -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;
+
ret = v4l2_async_notifier_register(>v4l2_dev, >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(>notifier);
 
+   /* Checks internaly if handlers have been init or not */
+   v4l2_ctrl_handler_free(>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(>lock);
 
+   if (!vin->digital.subdev) {
+   ret = -ENODEV;
+   goto unlock;
+   }
+
file->private_data = vin;
 
ret = v4l2_fh_open(file);
@@ -844,9 +849,6 @@ void rvin_v4l2_remove(struct rvin_dev *vin)
v4l2_info(>v4l2_dev, "Removing %s\n",