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@xxxxxxxxxxxx> > --- > 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