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

[Hans Verkuil]

On 11/11/17 01:38, 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

"unbound and later rebound": that's a manual operation via /sys bind/unbind?

I remain unhappy about this patch. It's a workaround for a more basic problem
IMHO.

Can you move this patch to the end of the series? That way I can decide later
whether to merge it or not without blocking the rest of the series.

Other than this patch the only blocking patch is the bindings patch which hasn't
got an Ack yet.

Regards,

Hans

> 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 probe time and don't allow
> user-space to open the video device if the subdevice is not bound yet.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 124 
> +++-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  47 ++-
>  drivers/media/platform/rcar-vin/rcar-vin.h  |   5 +-
>  3 files changed, 95 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 108d776f32651b27..856df3e407c05d97 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -46,54 +46,18 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int 
> direction)
>   return -EINVAL;
>  }
>  
> -static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
> -{
> - struct v4l2_subdev *sd = entity->subdev;
> - struct v4l2_subdev_mbus_code_enum code = {
> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> - };
> -
> - code.index = 0;
> - code.pad = entity->source_pad;
> - while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, )) {
> - code.index++;
> - switch (code.code) {
> - case MEDIA_BUS_FMT_YUYV8_1X16:
> - case MEDIA_BUS_FMT_UYVY8_2X8:
> - case MEDIA_BUS_FMT_UYVY10_2X10:
> - case MEDIA_BUS_FMT_RGB888_1X24:
> - entity->code = code.code;
> - return true;
> - default:
> - break;
> - }
> - }
> -
> - return false;
> -}
> -
>  static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
>   int ret;
>  
> - /* Verify subdevices mbus format */
> - if (!rvin_mbus_supported(vin->digital)) {
> - vin_err(vin, "Unsupported media bus format for %s\n",
> - vin->digital->subdev->name);
> - return -EINVAL;
> - }
> -
> - vin_dbg(vin, "Found media bus format for %s: %d\n",
> - vin->digital->subdev->name, vin->digital->code);
> -
>   ret = v4l2_device_register_subdev_nodes(>v4l2_dev);
>   if (ret < 0) {
>   vin_err(vin, "Failed to register subdev nodes\n");
>   return ret;
>   }
>  
> - return rvin_v4l2_probe(vin);
> + return 0;
>  }
>  
>  static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -103,8 +67,15 @@ 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);
> +
> + mutex_lock(>lock);
> +
> + vin->vdev.ctrl_handler = NULL;
> + v4l2_ctrl_handler_free(>ctrl_handler);
> +
>   vin->digital->subdev = NULL;
> +
> + mutex_unlock(>lock);
>  }
>  
>  static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -112,12 +83,14 @@ static int rvin_digital_notify_bound(struct 
> v4l2_async_notifier *notifier,
>struct v4l2_async_subdev *asd)
>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
> + struct v4l2_subdev_mbus_code_enum code = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
>   int ret;
>  
>   v4l2_set_subdev_hostdata(subdev, vin);
>  
>   /* Find source and sink pad of remote subdevice */
> -
>   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
>   if (ret < 0)
>   return ret;
> @@ -126,21 +99,82 @@ static int rvin_digital_notify_bound(struct 
> v4l2_async_notifier *notifier,
>   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
>   vin->digital->sink_pad = ret < 0 ? 0 : ret;
>  
> + /* Find compatible subdevices mbus format */
> + vin->digital->code = 0;
> + code.index = 0;
> + code.pad = vin->digital->source_pad;
> + while (!vin->digital->code &&
> +

[PATCH v7 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 probe time and don't allow
user-space to open the video device if the subdevice is not bound yet.

Signed-off-by: Niklas Söderlund 
---
 drivers/media/platform/rcar-vin/rcar-core.c | 124 +++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  47 ++-
 drivers/media/platform/rcar-vin/rcar-vin.h  |   5 +-
 3 files changed, 95 insertions(+), 81 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
b/drivers/media/platform/rcar-vin/rcar-core.c
index 108d776f32651b27..856df3e407c05d97 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -46,54 +46,18 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int 
direction)
return -EINVAL;
 }
 
-static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
-{
-   struct v4l2_subdev *sd = entity->subdev;
-   struct v4l2_subdev_mbus_code_enum code = {
-   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
-   };
-
-   code.index = 0;
-   code.pad = entity->source_pad;
-   while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, )) {
-   code.index++;
-   switch (code.code) {
-   case MEDIA_BUS_FMT_YUYV8_1X16:
-   case MEDIA_BUS_FMT_UYVY8_2X8:
-   case MEDIA_BUS_FMT_UYVY10_2X10:
-   case MEDIA_BUS_FMT_RGB888_1X24:
-   entity->code = code.code;
-   return true;
-   default:
-   break;
-   }
-   }
-
-   return false;
-}
-
 static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 {
struct rvin_dev *vin = notifier_to_vin(notifier);
int ret;
 
-   /* Verify subdevices mbus format */
-   if (!rvin_mbus_supported(vin->digital)) {
-   vin_err(vin, "Unsupported media bus format for %s\n",
-   vin->digital->subdev->name);
-   return -EINVAL;
-   }
-
-   vin_dbg(vin, "Found media bus format for %s: %d\n",
-   vin->digital->subdev->name, vin->digital->code);
-
ret = v4l2_device_register_subdev_nodes(>v4l2_dev);
if (ret < 0) {
vin_err(vin, "Failed to register subdev nodes\n");
return ret;
}
 
-   return rvin_v4l2_probe(vin);
+   return 0;
 }
 
 static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -103,8 +67,15 @@ 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);
+
+   mutex_lock(>lock);
+
+   vin->vdev.ctrl_handler = NULL;
+   v4l2_ctrl_handler_free(>ctrl_handler);
+
vin->digital->subdev = NULL;
+
+   mutex_unlock(>lock);
 }
 
 static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
@@ -112,12 +83,14 @@ static int rvin_digital_notify_bound(struct 
v4l2_async_notifier *notifier,
 struct v4l2_async_subdev *asd)
 {
struct rvin_dev *vin = notifier_to_vin(notifier);
+   struct v4l2_subdev_mbus_code_enum code = {
+   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+   };
int ret;
 
v4l2_set_subdev_hostdata(subdev, vin);
 
/* Find source and sink pad of remote subdevice */
-
ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
if (ret < 0)
return ret;
@@ -126,21 +99,82 @@ static int rvin_digital_notify_bound(struct 
v4l2_async_notifier *notifier,
ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
vin->digital->sink_pad = ret < 0 ? 0 : ret;
 
+   /* Find compatible subdevices mbus format */
+   vin->digital->code = 0;
+   code.index = 0;
+   code.pad = vin->digital->source_pad;
+   while (!vin->digital->code &&
+  !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, )) {
+   code.index++;
+   switch (code.code) {
+   case MEDIA_BUS_FMT_YUYV8_1X16:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_UYVY10_2X10:
+   case MEDIA_BUS_FMT_RGB888_1X24:
+   vin->digital->code = code.code;
+   vin_dbg(vin, "Found media bus format for %s: %d\n",
+   subdev->name, vin->digital->code);
+   break;
+   default:
+   break;
+