From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
Kieran Bingham <kieran.bingham@ideasonboard.com>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
tomoharu.fukawa.eb@renesas.com, linux-media@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v6 02/25] rcar-vin: register the video device at probe time
Date: Wed, 23 Aug 2017 11:43:09 +0300 [thread overview]
Message-ID: <6104719.MxKyBo7YOL@avalon> (raw)
In-Reply-To: <20170822232640.26147-3-niklas.soderlund+renesas@ragnatech.se>
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
next prev parent reply other threads:[~2017-08-23 8:42 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-22 23:26 [PATCH v6 00/25] rcar-vin: Add Gen3 with media controller Niklas Söderlund
2017-08-22 23:26 ` [PATCH v6 01/25] rcar-vin: add Gen3 devicetree bindings documentation Niklas Söderlund
2017-08-23 8:14 ` Laurent Pinchart
2017-08-22 23:26 ` [PATCH v6 02/25] rcar-vin: register the video device at probe time Niklas Söderlund
2017-08-23 8:43 ` Laurent Pinchart [this message]
2017-08-22 23:26 ` [PATCH v6 03/25] rcar-vin: move chip information to own struct Niklas Söderlund
2017-08-23 15:49 ` Laurent Pinchart
2017-09-25 9:44 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 04/25] rcar-vin: move max width and height information to chip information Niklas Söderlund
2017-09-25 9:45 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 05/25] rcar-vin: change name of video device Niklas Söderlund
2017-09-25 9:46 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 06/25] rcar-vin: move functions regarding scaling Niklas Söderlund
2017-09-25 9:46 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 07/25] rcar-vin: all Gen2 boards can scale simplify logic Niklas Söderlund
2017-09-25 9:48 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 08/25] rcar-vin: do not reset crop and compose when setting format Niklas Söderlund
2017-09-25 9:49 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 09/25] rcar-vin: do not allow changing scaling and composing while streaming Niklas Söderlund
2017-09-25 9:50 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 10/25] rcar-vin: read subdevice format for crop only when needed Niklas Söderlund
2017-09-25 9:54 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 11/25] rcar-vin: fix handling of single field frames (top, bottom and alternate fields) Niklas Söderlund
2017-09-25 9:58 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 12/25] rcar-vin: move media bus configuration to struct rvin_info Niklas Söderlund
2017-09-25 10:00 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 13/25] rcar-vin: enable Gen3 hardware configuration Niklas Söderlund
2017-09-25 10:01 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 14/25] rcar-vin: add functions to manipulate Gen3 CHSEL value Niklas Söderlund
2017-09-25 10:04 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 15/25] rcar-vin: add flag to switch to media controller mode Niklas Söderlund
2017-09-25 10:04 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 16/25] rcar-vin: break out format alignment and checking Niklas Söderlund
2017-09-25 10:08 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 17/25] rcar-vin: use different v4l2 operations in media controller mode Niklas Söderlund
2017-09-25 10:11 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 18/25] rcar-vin: prepare for media controller mode initialization Niklas Söderlund
2017-09-25 10:11 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 19/25] rcar-vin: add group allocator functions Niklas Söderlund
2017-09-25 10:22 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 20/25] rcar-vin: add chsel information to rvin_info Niklas Söderlund
2017-09-25 10:26 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 21/25] rcar-vin: parse Gen3 OF and setup media graph Niklas Söderlund
2017-09-25 10:36 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 22/25] rcar-vin: add link notify for Gen3 Niklas Söderlund
2017-09-25 10:43 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 23/25] rcar-vin: extend {start,stop}_streaming to work with media controller Niklas Söderlund
2017-09-25 10:46 ` Hans Verkuil
2017-08-22 23:26 ` [PATCH v6 24/25] rcar-vin: enable support for r8a7795 Niklas Söderlund
2017-09-25 10:47 ` Hans Verkuil
2017-10-03 8:30 ` Geert Uytterhoeven
2017-08-22 23:26 ` [PATCH v6 25/25] rcar-vin: enable support for r8a7796 Niklas Söderlund
2017-09-25 10:47 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6104719.MxKyBo7YOL@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil@xs4all.nl \
--cc=kieran.bingham@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
--cc=tomoharu.fukawa.eb@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).