[PATCHv2 0/9] Various core and virtual driver fixes

[PATCHv2 0/9] Various core and virtual driver fixes

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Various fixes for bugs that I found while working on the
regression test-media script.

The CONFIG_DEBUG_KOBJECT_RELEASE=y option in particular found
a bunch of bugs where things were not released in the right
order.

Note that the first two patches are not bug fixes, but they
help debugging kobjects. Without this the object name is just
(null), which makes it hard to figure out what the object is.

Changes since v1:

- Dropped 'v4l2-device: v4l2_device_release_subdev_node can't
  reference sd' and replaced it with 'v4l2-subdev: add release()
  internal op'.
- Added 'v4l2-subdev: handle module refcounting here'.
- Added 'vimc: use new release op'.

With these changes I can run the test-media script without errors
(except for a known vimc 'do not call blocking ops when !TASK_RUNNING;'
error, waiting for a patch from Helen that fixes this).

Regards,

	Hans

Hans Verkuil (9):
  cec: fill in cec chardev kobject to ease debugging
  media-devnode: fill in media chardev kobject to ease debugging
  vivid: use vzalloc for dev->bitmap_out
  media-entity: set ent_enum->bmap to NULL after freeing it
  vim2m: replace devm_kzalloc by kzalloc
  v4l2-subdev: add release() internal op
  v4l2-subdev: handle module refcounting here
  vimc: free vimc_cap_device when the last user disappears
  vimc: use new release op

 drivers/media/cec/cec-core.c                 |  1 +
 drivers/media/media-devnode.c                |  1 +
 drivers/media/media-entity.c                 | 29 +---------------
 drivers/media/platform/vim2m.c               | 35 ++++++++++++--------
 drivers/media/platform/vimc/vimc-capture.c   | 13 ++++++--
 drivers/media/platform/vimc/vimc-common.c    |  2 ++
 drivers/media/platform/vimc/vimc-common.h    |  2 ++
 drivers/media/platform/vimc/vimc-debayer.c   | 15 +++++++--
 drivers/media/platform/vimc/vimc-scaler.c    | 15 +++++++--
 drivers/media/platform/vimc/vimc-sensor.c    | 19 ++++++++---
 drivers/media/platform/vivid/vivid-vid-out.c | 14 +++++---
 drivers/media/v4l2-core/v4l2-device.c        | 19 ++++++++---
 drivers/media/v4l2-core/v4l2-subdev.c        | 22 +++++-------
 include/media/media-entity.h                 | 24 --------------
 include/media/v4l2-subdev.h                  |  4 +++
 15 files changed, 116 insertions(+), 99 deletions(-)

-- 
2.20.1

[PATCHv2 1/9] cec: fill in cec chardev kobject to ease debugging

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The cec chardev kobject has no name, which made it hard to
debug when kobject debugging is turned on.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/cec/cec-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index cc875dabd765..f5d1578e256a 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -126,6 +126,7 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &cec_devnode_fops);
 	devnode->cdev.owner = owner;
+	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
 	if (ret) {
-- 
2.20.1

[PATCHv2 2/9] media-devnode: fill in media chardev kobject to ease debugging

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The media chardev kobject has no name, which made it hard to
debug when kobject debugging is turned on.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-devnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 6b87a721dc49..61dc05fcc55c 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -251,6 +251,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	/* Part 2: Initialize the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
+	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
 	/* Part 3: Add the media and char device */
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
-- 
2.20.1

[PATCHv2 3/9] vivid: use vzalloc for dev->bitmap_out

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

When vivid is unloaded it used vfree to free dev->bitmap_out,
but it was actually allocated using kmalloc. Use vzalloc
instead, conform what vivid-vid-cap.c does.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/vivid/vivid-vid-out.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
index e61b91b414f9..9350ca65dd91 100644
--- a/drivers/media/platform/vivid/vivid-vid-out.c
+++ b/drivers/media/platform/vivid/vivid-vid-out.c
@@ -798,7 +798,7 @@ int vivid_vid_out_s_selection(struct file *file, void *fh, struct v4l2_selection
 		s->r.height *= factor;
 		if (dev->bitmap_out && (compose->width != s->r.width ||
 					compose->height != s->r.height)) {
-			kfree(dev->bitmap_out);
+			vfree(dev->bitmap_out);
 			dev->bitmap_out = NULL;
 		}
 		*compose = s->r;
@@ -941,15 +941,19 @@ int vidioc_s_fmt_vid_out_overlay(struct file *file, void *priv,
 		return ret;
 
 	if (win->bitmap) {
-		new_bitmap = memdup_user(win->bitmap, bitmap_size);
+		new_bitmap = vzalloc(bitmap_size);
 
-		if (IS_ERR(new_bitmap))
-			return PTR_ERR(new_bitmap);
+		if (!new_bitmap)
+			return -ENOMEM;
+		if (copy_from_user(new_bitmap, win->bitmap, bitmap_size)) {
+			vfree(new_bitmap);
+			return -EFAULT;
+		}
 	}
 
 	dev->overlay_out_top = win->w.top;
 	dev->overlay_out_left = win->w.left;
-	kfree(dev->bitmap_out);
+	vfree(dev->bitmap_out);
 	dev->bitmap_out = new_bitmap;
 	dev->clipcount_out = win->clipcount;
 	if (dev->clipcount_out)
-- 
2.20.1

[PATCHv2 4/9] media-entity: set ent_enum->bmap to NULL after freeing it

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Ensure that this pointer is set to NULL after it is freed.
The vimc driver has a static media_entity and after
unbinding and rebinding the vimc device the media code will
try to free this pointer again since it wasn't set to NULL.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-entity.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 0b1cb3559140..7b2a2cc95530 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
 void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
 {
 	kfree(ent_enum->bmap);
+	ent_enum->bmap = NULL;
 }
 EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
 
-- 
2.20.1

[PATCHv2 5/9] vim2m: replace devm_kzalloc by kzalloc

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

It is not possible to use devm_kzalloc since that memory is
freed immediately when the device instance is unbound.

Various objects like the video device may still be in use
since someone has the device node open, and when that is closed
it expects the memory to be around.

So use kzalloc and release it at the appropriate time.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/vim2m.c | 35 +++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 34dcaca45d8b..dd47821fc661 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -1262,6 +1262,15 @@ static int vim2m_release(struct file *file)
 	return 0;
 }
 
+static void vim2m_device_release(struct video_device *vdev)
+{
+	struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd);
+
+	v4l2_device_unregister(&dev->v4l2_dev);
+	v4l2_m2m_release(dev->m2m_dev);
+	kfree(dev);
+}
+
 static const struct v4l2_file_operations vim2m_fops = {
 	.owner		= THIS_MODULE,
 	.open		= vim2m_open,
@@ -1277,7 +1286,7 @@ static const struct video_device vim2m_videodev = {
 	.fops		= &vim2m_fops,
 	.ioctl_ops	= &vim2m_ioctl_ops,
 	.minor		= -1,
-	.release	= video_device_release_empty,
+	.release	= vim2m_device_release,
 	.device_caps	= V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING,
 };
 
@@ -1298,13 +1307,13 @@ static int vim2m_probe(struct platform_device *pdev)
 	struct video_device *vfd;
 	int ret;
 
-	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
-		return ret;
+		goto error_free;
 
 	atomic_set(&dev->num_inst, 0);
 	mutex_init(&dev->dev_mutex);
@@ -1317,7 +1326,7 @@ static int vim2m_probe(struct platform_device *pdev)
 	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
-		goto unreg_v4l2;
+		goto error_v4l2;
 	}
 
 	video_set_drvdata(vfd, dev);
@@ -1330,7 +1339,7 @@ static int vim2m_probe(struct platform_device *pdev)
 	if (IS_ERR(dev->m2m_dev)) {
 		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem device\n");
 		ret = PTR_ERR(dev->m2m_dev);
-		goto unreg_dev;
+		goto error_dev;
 	}
 
 #ifdef CONFIG_MEDIA_CONTROLLER
@@ -1346,27 +1355,29 @@ static int vim2m_probe(struct platform_device *pdev)
 						 MEDIA_ENT_F_PROC_VIDEO_SCALER);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem media controller\n");
-		goto unreg_m2m;
+		goto error_m2m;
 	}
 
 	ret = media_device_register(&dev->mdev);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to register mem2mem media device\n");
-		goto unreg_m2m_mc;
+		goto error_m2m_mc;
 	}
 #endif
 	return 0;
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-unreg_m2m_mc:
+error_m2m_mc:
 	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
-unreg_m2m:
+error_m2m:
 	v4l2_m2m_release(dev->m2m_dev);
 #endif
-unreg_dev:
+error_dev:
 	video_unregister_device(&dev->vfd);
-unreg_v4l2:
+error_v4l2:
 	v4l2_device_unregister(&dev->v4l2_dev);
+error_free:
+	kfree(dev);
 
 	return ret;
 }
@@ -1382,9 +1393,7 @@ static int vim2m_remove(struct platform_device *pdev)
 	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
 	media_device_cleanup(&dev->mdev);
 #endif
-	v4l2_m2m_release(dev->m2m_dev);
 	video_unregister_device(&dev->vfd);
-	v4l2_device_unregister(&dev->v4l2_dev);
 
 	return 0;
 }
-- 
2.20.1

[PATCHv2 6/9] v4l2-subdev: add release() internal op

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

If the subdevice created a device node, then the v4l2_subdev cannot
be freed until the last user of the device node closes it.

This means that we need a release() callback in v4l2_subdev_internal_ops
that is called from the video_device release function so the subdevice
driver can postpone freeing memory until the that callback is called.

If no video device node was created then the release callback can
be called immediately when the subdev is unregistered.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++-----
 include/media/v4l2-subdev.h           |  3 +++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index e0ddb9a52bd1..7cca0de1b730 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 }
 EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
 
+static void v4l2_subdev_release(struct v4l2_subdev *sd)
+{
+	struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
+
+	if (sd->internal_ops && sd->internal_ops->release)
+		sd->internal_ops->release(sd);
+	module_put(owner);
+}
+
 static void v4l2_device_release_subdev_node(struct video_device *vdev)
 {
-	struct v4l2_subdev *sd = video_get_drvdata(vdev);
-	sd->devnode = NULL;
+	v4l2_subdev_release(video_get_drvdata(vdev));
 	kfree(vdev);
 }
 
@@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 		media_device_unregister_entity(&sd->entity);
 	}
 #endif
-	video_unregister_device(sd->devnode);
-	if (!sd->owner_v4l2_dev)
-		module_put(sd->owner);
+	if (sd->devnode)
+		video_unregister_device(sd->devnode);
+	else
+		v4l2_subdev_release(sd);
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 349e1c18cf48..2f2d1c8947e6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -757,6 +757,8 @@ struct v4l2_subdev_ops {
  *
  * @close: called when the subdev device node is closed.
  *
+ * @release: called when the subdev device node is released.
+ *
  * .. note::
  *	Never call this from drivers, only the v4l2 framework can call
  *	these ops.
@@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops {
 	void (*unregistered)(struct v4l2_subdev *sd);
 	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
 	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
+	void (*release)(struct v4l2_subdev *sd);
 };
 
 #define V4L2_SUBDEV_NAME_SIZE 32
-- 
2.20.1

[PATCHv2 7/9] v4l2-subdev: handle module refcounting here

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The module ownership refcounting was done in media_entity_get/put,
but that was very confusing and it did not work either in case an
application had a v4l-subdevX device open and the module was
unbound. When the v4l-subdevX device was closed the media_entity_put
was never called and the module refcount was left one too high, making
it impossible to unload it.

Since v4l2-subdev.c was the only place where media_entity_get/put was
called, just move the functionality to v4l2-subdev.c and drop those
confusing entity functions.

Store the module in subdev_fh so module_put no longer depends on
the media_entity struct.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-entity.c          | 28 ---------------------------
 drivers/media/v4l2-core/v4l2-subdev.c | 22 +++++++++------------
 include/media/media-entity.h          | 24 -----------------------
 include/media/v4l2-subdev.h           |  1 +
 4 files changed, 10 insertions(+), 65 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 7b2a2cc95530..257f20d2fb8a 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -17,7 +17,6 @@
  */
 
 #include <linux/bitmap.h>
-#include <linux/module.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <media/media-entity.h>
@@ -588,33 +587,6 @@ void media_pipeline_stop(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_pipeline_stop);
 
-/* -----------------------------------------------------------------------------
- * Module use count
- */
-
-struct media_entity *media_entity_get(struct media_entity *entity)
-{
-	if (entity == NULL)
-		return NULL;
-
-	if (entity->graph_obj.mdev->dev &&
-	    !try_module_get(entity->graph_obj.mdev->dev->driver->owner))
-		return NULL;
-
-	return entity;
-}
-EXPORT_SYMBOL_GPL(media_entity_get);
-
-void media_entity_put(struct media_entity *entity)
-{
-	if (entity == NULL)
-		return;
-
-	if (entity->graph_obj.mdev->dev)
-		module_put(entity->graph_obj.mdev->dev->driver->owner);
-}
-EXPORT_SYMBOL_GPL(media_entity_put);
-
 /* -----------------------------------------------------------------------------
  * Links management
  */
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f5f0d71ec745..d75815ab0d7b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -18,6 +18,7 @@
 
 #include <linux/ioctl.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/videodev2.h>
@@ -54,9 +55,6 @@ static int subdev_open(struct file *file)
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
 	struct v4l2_subdev_fh *subdev_fh;
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_entity *entity = NULL;
-#endif
 	int ret;
 
 	subdev_fh = kzalloc(sizeof(*subdev_fh), GFP_KERNEL);
@@ -73,12 +71,15 @@ static int subdev_open(struct file *file)
 	v4l2_fh_add(&subdev_fh->vfh);
 	file->private_data = &subdev_fh->vfh;
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (sd->v4l2_dev->mdev) {
-		entity = media_entity_get(&sd->entity);
-		if (!entity) {
+	if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
+		struct module *owner;
+
+		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
+		if (!try_module_get(owner)) {
 			ret = -EBUSY;
 			goto err;
 		}
+		subdev_fh->owner = owner;
 	}
 #endif
 
@@ -91,9 +92,7 @@ static int subdev_open(struct file *file)
 	return 0;
 
 err:
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	media_entity_put(entity);
-#endif
+	module_put(subdev_fh->owner);
 	v4l2_fh_del(&subdev_fh->vfh);
 	v4l2_fh_exit(&subdev_fh->vfh);
 	subdev_fh_free(subdev_fh);
@@ -111,10 +110,7 @@ static int subdev_close(struct file *file)
 
 	if (sd->internal_ops && sd->internal_ops->close)
 		sd->internal_ops->close(sd, subdev_fh);
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (sd->v4l2_dev->mdev)
-		media_entity_put(&sd->entity);
-#endif
+	module_put(subdev_fh->owner);
 	v4l2_fh_del(vfh);
 	v4l2_fh_exit(vfh);
 	subdev_fh_free(subdev_fh);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index e5f6960d92f6..3cbad42e3693 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -864,19 +864,6 @@ struct media_link *media_entity_find_link(struct media_pad *source,
  */
 struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
 
-/**
- * media_entity_get - Get a reference to the parent module
- *
- * @entity: The entity
- *
- * Get a reference to the parent media device module.
- *
- * The function will return immediately if @entity is %NULL.
- *
- * Return: returns a pointer to the entity on success or %NULL on failure.
- */
-struct media_entity *media_entity_get(struct media_entity *entity);
-
 /**
  * media_entity_get_fwnode_pad - Get pad number from fwnode
  *
@@ -916,17 +903,6 @@ __must_check int media_graph_walk_init(
  */
 void media_graph_walk_cleanup(struct media_graph *graph);
 
-/**
- * media_entity_put - Release the reference to the parent module
- *
- * @entity: The entity
- *
- * Release the reference count acquired by media_entity_get().
- *
- * The function will return immediately if @entity is %NULL.
- */
-void media_entity_put(struct media_entity *entity);
-
 /**
  * media_graph_walk_start - Start walking the media graph at a
  *	given entity
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 2f2d1c8947e6..33ba56f3dc1f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -905,6 +905,7 @@ struct v4l2_subdev {
  */
 struct v4l2_subdev_fh {
 	struct v4l2_fh vfh;
+	struct module *owner;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_subdev_pad_config *pad;
 #endif
-- 
2.20.1

[PATCHv2 8/9] vimc: free vimc_cap_device when the last user disappears

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Don't free vimc_cap_device immediately, instead do this
in the video_device release function which is called when the
last user closes the video device. Only then is it safe to
free the memory.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/vimc/vimc-capture.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index ea869631a3f6..3d433361d297 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -338,6 +338,15 @@ static const struct media_entity_operations vimc_cap_mops = {
 	.link_validate		= vimc_link_validate,
 };
 
+static void vimc_cap_release(struct video_device *vdev)
+{
+	struct vimc_cap_device *vcap = container_of(vdev, struct vimc_cap_device,
+						    vdev);
+
+	vimc_pads_cleanup(vcap->ved.pads);
+	kfree(vcap);
+}
+
 static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 				 void *master_data)
 {
@@ -348,8 +357,6 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 	vb2_queue_release(&vcap->queue);
 	media_entity_cleanup(ved->ent);
 	video_unregister_device(&vcap->vdev);
-	vimc_pads_cleanup(vcap->ved.pads);
-	kfree(vcap);
 }
 
 static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
@@ -467,7 +474,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	vdev = &vcap->vdev;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
 	vdev->entity.ops = &vimc_cap_mops;
-	vdev->release = video_device_release_empty;
+	vdev->release = vimc_cap_release;
 	vdev->fops = &vimc_cap_fops;
 	vdev->ioctl_ops = &vimc_cap_ioctl_ops;
 	vdev->lock = &vcap->lock;
-- 
2.20.1

[PATCHv2 9/9] vimc: use new release op

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Use the new v4l2_subdev_internal_ops release op to free the
subdev memory only once the last user closed the file handle.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vimc/vimc-common.c  |  2 ++
 drivers/media/platform/vimc/vimc-common.h  |  2 ++
 drivers/media/platform/vimc/vimc-debayer.c | 15 +++++++++++++--
 drivers/media/platform/vimc/vimc-scaler.c  | 15 +++++++++++++--
 drivers/media/platform/vimc/vimc-sensor.c  | 19 +++++++++++++++----
 5 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index c1a74bb2df58..0adbfd8fd26d 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -380,6 +380,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 			 u32 function,
 			 u16 num_pads,
 			 const unsigned long *pads_flag,
+			 const struct v4l2_subdev_internal_ops *sd_int_ops,
 			 const struct v4l2_subdev_ops *sd_ops)
 {
 	int ret;
@@ -394,6 +395,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 
 	/* Initialize the subdev */
 	v4l2_subdev_init(sd, sd_ops);
+	sd->internal_ops = sd_int_ops;
 	sd->entity.function = function;
 	sd->entity.ops = &vimc_ent_sd_mops;
 	sd->owner = THIS_MODULE;
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 84539430b5e7..07987eab988f 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -187,6 +187,7 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
  * @function:	media entity function defined by MEDIA_ENT_F_* macros
  * @num_pads:	number of pads to initialize
  * @pads_flag:	flags to use in each pad
+ * @sd_int_ops:	pointer to &struct v4l2_subdev_internal_ops.
  * @sd_ops:	pointer to &struct v4l2_subdev_ops.
  *
  * Helper function initialize and register the struct vimc_ent_device and struct
@@ -199,6 +200,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 			 u32 function,
 			 u16 num_pads,
 			 const unsigned long *pads_flag,
+			 const struct v4l2_subdev_internal_ops *sd_int_ops,
 			 const struct v4l2_subdev_ops *sd_ops);
 
 /**
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 7d77c63b99d2..eaed4233ad1b 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -489,6 +489,18 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
 
 }
 
+static void vimc_deb_release(struct v4l2_subdev *sd)
+{
+	struct vimc_deb_device *vdeb =
+				container_of(sd, struct vimc_deb_device, sd);
+
+	kfree(vdeb);
+}
+
+static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
+	.release = vimc_deb_release,
+};
+
 static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
 				 void *master_data)
 {
@@ -497,7 +509,6 @@ static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
 						    ved);
 
 	vimc_ent_sd_unregister(ved, &vdeb->sd);
-	kfree(vdeb);
 }
 
 static int vimc_deb_comp_bind(struct device *comp, struct device *master,
@@ -519,7 +530,7 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
 				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
-				   &vimc_deb_ops);
+				   &vimc_deb_int_ops, &vimc_deb_ops);
 	if (ret) {
 		kfree(vdeb);
 		return ret;
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 39b2a73dfcc1..2028afa4ef7a 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -348,6 +348,18 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
 	return vsca->src_frame;
 };
 
+static void vimc_sca_release(struct v4l2_subdev *sd)
+{
+	struct vimc_sca_device *vsca =
+				container_of(sd, struct vimc_sca_device, sd);
+
+	kfree(vsca);
+}
+
+static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
+	.release = vimc_sca_release,
+};
+
 static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
 				 void *master_data)
 {
@@ -356,7 +368,6 @@ static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
 						    ved);
 
 	vimc_ent_sd_unregister(ved, &vsca->sd);
-	kfree(vsca);
 }
 
 
@@ -379,7 +390,7 @@ static int vimc_sca_comp_bind(struct device *comp, struct device *master,
 				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
-				   &vimc_sca_ops);
+				   &vimc_sca_int_ops, &vimc_sca_ops);
 	if (ret) {
 		kfree(vsca);
 		return ret;
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 59195f262623..d7891d3bbeaa 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -301,6 +301,20 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
 	.s_ctrl = vimc_sen_s_ctrl,
 };
 
+static void vimc_sen_release(struct v4l2_subdev *sd)
+{
+	struct vimc_sen_device *vsen =
+				container_of(sd, struct vimc_sen_device, sd);
+
+	v4l2_ctrl_handler_free(&vsen->hdl);
+	tpg_free(&vsen->tpg);
+	kfree(vsen);
+}
+
+static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
+	.release = vimc_sen_release,
+};
+
 static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
 				 void *master_data)
 {
@@ -309,9 +323,6 @@ static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
 				container_of(ved, struct vimc_sen_device, ved);
 
 	vimc_ent_sd_unregister(ved, &vsen->sd);
-	v4l2_ctrl_handler_free(&vsen->hdl);
-	tpg_free(&vsen->tpg);
-	kfree(vsen);
 }
 
 /* Image Processing Controls */
@@ -371,7 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 				   pdata->entity_name,
 				   MEDIA_ENT_F_CAM_SENSOR, 1,
 				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
-				   &vimc_sen_ops);
+				   &vimc_sen_int_ops, &vimc_sen_ops);
 	if (ret)
 		goto err_free_hdl;
 
-- 
2.20.1

Re: [PATCHv2 4/9] media-entity: set ent_enum->bmap to NULL after freeing it

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Ensure that this pointer is set to NULL after it is freed.
> The vimc driver has a static media_entity and after
> unbinding and rebinding the vimc device the media code will
> try to free this pointer again since it wasn't set to NULL.

I still think the problem lies in the vimc driver. Reusing static
structures is really asking for trouble. I'm however not opposed to
merging this patch, as you mentioned the problem will be fixed in vimc
too. I still wonder, though, if merging this couldn't make it easier for
drivers to do the wrong thing.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-entity.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 0b1cb3559140..7b2a2cc95530 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
>  void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
>  {
>  	kfree(ent_enum->bmap);
> +	ent_enum->bmap = NULL;
>  }
>  EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 6/9] v4l2-subdev: add release() internal op

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> If the subdevice created a device node, then the v4l2_subdev cannot
> be freed until the last user of the device node closes it.
> 
> This means that we need a release() callback in v4l2_subdev_internal_ops
> that is called from the video_device release function so the subdevice
> driver can postpone freeing memory until the that callback is called.
> 
> If no video device node was created then the release callback can
> be called immediately when the subdev is unregistered.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++-----
>  include/media/v4l2-subdev.h           |  3 +++
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index e0ddb9a52bd1..7cca0de1b730 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>  
> +static void v4l2_subdev_release(struct v4l2_subdev *sd)
> +{
> +	struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
> +
> +	if (sd->internal_ops && sd->internal_ops->release)
> +		sd->internal_ops->release(sd);
> +	module_put(owner);
> +}
> +
>  static void v4l2_device_release_subdev_node(struct video_device *vdev)
>  {
> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
> -	sd->devnode = NULL;
> +	v4l2_subdev_release(video_get_drvdata(vdev));
>  	kfree(vdev);
>  }
>  
> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  		media_device_unregister_entity(&sd->entity);
>  	}
>  #endif
> -	video_unregister_device(sd->devnode);
> -	if (!sd->owner_v4l2_dev)
> -		module_put(sd->owner);
> +	if (sd->devnode)
> +		video_unregister_device(sd->devnode);
> +	else
> +		v4l2_subdev_release(sd);
>  }

Don't we also need to handle the error path in
v4l2_device_register_subdev_nodes() ?

>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 349e1c18cf48..2f2d1c8947e6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops {
>   *
>   * @close: called when the subdev device node is closed.
>   *
> + * @release: called when the subdev device node is released.
> + *

I think this should be expanded a bit. First of all, we should mention
what happens when the subdev doesn't have a device node, and then we
should also explain what drivers are supposed to do in this operation.

At what point do you think we should add a WARN_ON(!sd->internal_ops ||
!sd->internal_ops->release) ?

I expect we'll need to refcount the subdev structure, with the
video_device only having one of the multiple references to the subdev,
but that can be implemented later. Overall this moves us in the right
direction, thank you for your work.

>   * .. note::
>   *	Never call this from drivers, only the v4l2 framework can call
>   *	these ops.
> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops {
>  	void (*unregistered)(struct v4l2_subdev *sd);
>  	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>  	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> +	void (*release)(struct v4l2_subdev *sd);
>  };
>  
>  #define V4L2_SUBDEV_NAME_SIZE 32

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 7/9] v4l2-subdev: handle module refcounting here

[Laurent Pinchart]

Hi Hans,

(CC'ing Sakari)

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:45AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The module ownership refcounting was done in media_entity_get/put,
> but that was very confusing and it did not work either in case an
> application had a v4l-subdevX device open and the module was
> unbound. When the v4l-subdevX device was closed the media_entity_put
> was never called and the module refcount was left one too high, making
> it impossible to unload it.
> 
> Since v4l2-subdev.c was the only place where media_entity_get/put was
> called, just move the functionality to v4l2-subdev.c and drop those
> confusing entity functions.

I wonder if we will later need to refcount media entities, but we can
reintroduce a different version of those two functions then, it doesn't
prevent their removal now.

Sakari, when working on lifetime management of objects in the media and
V4L2 core, did you come across a need to refcount entities ?

> Store the module in subdev_fh so module_put no longer depends on
> the media_entity struct.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-entity.c          | 28 ---------------------------
>  drivers/media/v4l2-core/v4l2-subdev.c | 22 +++++++++------------
>  include/media/media-entity.h          | 24 -----------------------
>  include/media/v4l2-subdev.h           |  1 +
>  4 files changed, 10 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 7b2a2cc95530..257f20d2fb8a 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -17,7 +17,6 @@
>   */
>  
>  #include <linux/bitmap.h>
> -#include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
> @@ -588,33 +587,6 @@ void media_pipeline_stop(struct media_entity *entity)
>  }
>  EXPORT_SYMBOL_GPL(media_pipeline_stop);
>  
> -/* -----------------------------------------------------------------------------
> - * Module use count
> - */
> -
> -struct media_entity *media_entity_get(struct media_entity *entity)
> -{
> -	if (entity == NULL)
> -		return NULL;
> -
> -	if (entity->graph_obj.mdev->dev &&
> -	    !try_module_get(entity->graph_obj.mdev->dev->driver->owner))
> -		return NULL;
> -
> -	return entity;
> -}
> -EXPORT_SYMBOL_GPL(media_entity_get);
> -
> -void media_entity_put(struct media_entity *entity)
> -{
> -	if (entity == NULL)
> -		return;
> -
> -	if (entity->graph_obj.mdev->dev)
> -		module_put(entity->graph_obj.mdev->dev->driver->owner);
> -}
> -EXPORT_SYMBOL_GPL(media_entity_put);
> -
>  /* -----------------------------------------------------------------------------
>   * Links management
>   */
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f5f0d71ec745..d75815ab0d7b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/ioctl.h>
>  #include <linux/mm.h>
> +#include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
> @@ -54,9 +55,6 @@ static int subdev_open(struct file *file)
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>  	struct v4l2_subdev_fh *subdev_fh;
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	struct media_entity *entity = NULL;
> -#endif
>  	int ret;
>  
>  	subdev_fh = kzalloc(sizeof(*subdev_fh), GFP_KERNEL);
> @@ -73,12 +71,15 @@ static int subdev_open(struct file *file)
>  	v4l2_fh_add(&subdev_fh->vfh);
>  	file->private_data = &subdev_fh->vfh;
>  #if defined(CONFIG_MEDIA_CONTROLLER)

On a side note, do you think it's time to select MEDIA_CONTROLLER
unconditionally ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -	if (sd->v4l2_dev->mdev) {
> -		entity = media_entity_get(&sd->entity);
> -		if (!entity) {
> +	if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
> +		struct module *owner;
> +
> +		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
> +		if (!try_module_get(owner)) {
>  			ret = -EBUSY;
>  			goto err;
>  		}
> +		subdev_fh->owner = owner;
>  	}
>  #endif
>  
> @@ -91,9 +92,7 @@ static int subdev_open(struct file *file)
>  	return 0;
>  
>  err:
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	media_entity_put(entity);
> -#endif
> +	module_put(subdev_fh->owner);
>  	v4l2_fh_del(&subdev_fh->vfh);
>  	v4l2_fh_exit(&subdev_fh->vfh);
>  	subdev_fh_free(subdev_fh);
> @@ -111,10 +110,7 @@ static int subdev_close(struct file *file)
>  
>  	if (sd->internal_ops && sd->internal_ops->close)
>  		sd->internal_ops->close(sd, subdev_fh);
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (sd->v4l2_dev->mdev)
> -		media_entity_put(&sd->entity);
> -#endif
> +	module_put(subdev_fh->owner);
>  	v4l2_fh_del(vfh);
>  	v4l2_fh_exit(vfh);
>  	subdev_fh_free(subdev_fh);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e5f6960d92f6..3cbad42e3693 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -864,19 +864,6 @@ struct media_link *media_entity_find_link(struct media_pad *source,
>   */
>  struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
>  
> -/**
> - * media_entity_get - Get a reference to the parent module
> - *
> - * @entity: The entity
> - *
> - * Get a reference to the parent media device module.
> - *
> - * The function will return immediately if @entity is %NULL.
> - *
> - * Return: returns a pointer to the entity on success or %NULL on failure.
> - */
> -struct media_entity *media_entity_get(struct media_entity *entity);
> -
>  /**
>   * media_entity_get_fwnode_pad - Get pad number from fwnode
>   *
> @@ -916,17 +903,6 @@ __must_check int media_graph_walk_init(
>   */
>  void media_graph_walk_cleanup(struct media_graph *graph);
>  
> -/**
> - * media_entity_put - Release the reference to the parent module
> - *
> - * @entity: The entity
> - *
> - * Release the reference count acquired by media_entity_get().
> - *
> - * The function will return immediately if @entity is %NULL.
> - */
> -void media_entity_put(struct media_entity *entity);
> -
>  /**
>   * media_graph_walk_start - Start walking the media graph at a
>   *	given entity
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 2f2d1c8947e6..33ba56f3dc1f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -905,6 +905,7 @@ struct v4l2_subdev {
>   */
>  struct v4l2_subdev_fh {
>  	struct v4l2_fh vfh;
> +	struct module *owner;
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  	struct v4l2_subdev_pad_config *pad;
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 9/9] vimc: use new release op

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:47AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Use the new v4l2_subdev_internal_ops release op to free the
> subdev memory only once the last user closed the file handle.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/vimc/vimc-common.c  |  2 ++
>  drivers/media/platform/vimc/vimc-common.h  |  2 ++
>  drivers/media/platform/vimc/vimc-debayer.c | 15 +++++++++++++--
>  drivers/media/platform/vimc/vimc-scaler.c  | 15 +++++++++++++--
>  drivers/media/platform/vimc/vimc-sensor.c  | 19 +++++++++++++++----
>  5 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index c1a74bb2df58..0adbfd8fd26d 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -380,6 +380,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  			 u32 function,
>  			 u16 num_pads,
>  			 const unsigned long *pads_flag,
> +			 const struct v4l2_subdev_internal_ops *sd_int_ops,
>  			 const struct v4l2_subdev_ops *sd_ops)
>  {
>  	int ret;
> @@ -394,6 +395,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  
>  	/* Initialize the subdev */
>  	v4l2_subdev_init(sd, sd_ops);
> +	sd->internal_ops = sd_int_ops;
>  	sd->entity.function = function;
>  	sd->entity.ops = &vimc_ent_sd_mops;
>  	sd->owner = THIS_MODULE;
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 84539430b5e7..07987eab988f 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -187,6 +187,7 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
>   * @function:	media entity function defined by MEDIA_ENT_F_* macros
>   * @num_pads:	number of pads to initialize
>   * @pads_flag:	flags to use in each pad
> + * @sd_int_ops:	pointer to &struct v4l2_subdev_internal_ops.

Nitpicking, most of the lines here don't have a period at the end.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>   * @sd_ops:	pointer to &struct v4l2_subdev_ops.
>   *
>   * Helper function initialize and register the struct vimc_ent_device and struct
> @@ -199,6 +200,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  			 u32 function,
>  			 u16 num_pads,
>  			 const unsigned long *pads_flag,
> +			 const struct v4l2_subdev_internal_ops *sd_int_ops,
>  			 const struct v4l2_subdev_ops *sd_ops);
>  
>  /**
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 7d77c63b99d2..eaed4233ad1b 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -489,6 +489,18 @@ static void *vimc_deb_process_frame(struct vimc_ent_device *ved,
>  
>  }
>  
> +static void vimc_deb_release(struct v4l2_subdev *sd)
> +{
> +	struct vimc_deb_device *vdeb =
> +				container_of(sd, struct vimc_deb_device, sd);
> +
> +	kfree(vdeb);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
> +	.release = vimc_deb_release,
> +};
> +
>  static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
>  				 void *master_data)
>  {
> @@ -497,7 +509,6 @@ static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
>  						    ved);
>  
>  	vimc_ent_sd_unregister(ved, &vdeb->sd);
> -	kfree(vdeb);
>  }
>  
>  static int vimc_deb_comp_bind(struct device *comp, struct device *master,
> @@ -519,7 +530,7 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
>  				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
>  				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>  				   MEDIA_PAD_FL_SOURCE},
> -				   &vimc_deb_ops);
> +				   &vimc_deb_int_ops, &vimc_deb_ops);
>  	if (ret) {
>  		kfree(vdeb);
>  		return ret;
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 39b2a73dfcc1..2028afa4ef7a 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -348,6 +348,18 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>  	return vsca->src_frame;
>  };
>  
> +static void vimc_sca_release(struct v4l2_subdev *sd)
> +{
> +	struct vimc_sca_device *vsca =
> +				container_of(sd, struct vimc_sca_device, sd);
> +
> +	kfree(vsca);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
> +	.release = vimc_sca_release,
> +};
> +
>  static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
>  				 void *master_data)
>  {
> @@ -356,7 +368,6 @@ static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
>  						    ved);
>  
>  	vimc_ent_sd_unregister(ved, &vsca->sd);
> -	kfree(vsca);
>  }
>  
>  
> @@ -379,7 +390,7 @@ static int vimc_sca_comp_bind(struct device *comp, struct device *master,
>  				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
>  				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>  				   MEDIA_PAD_FL_SOURCE},
> -				   &vimc_sca_ops);
> +				   &vimc_sca_int_ops, &vimc_sca_ops);
>  	if (ret) {
>  		kfree(vsca);
>  		return ret;
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 59195f262623..d7891d3bbeaa 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -301,6 +301,20 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>  	.s_ctrl = vimc_sen_s_ctrl,
>  };
>  
> +static void vimc_sen_release(struct v4l2_subdev *sd)
> +{
> +	struct vimc_sen_device *vsen =
> +				container_of(sd, struct vimc_sen_device, sd);
> +
> +	v4l2_ctrl_handler_free(&vsen->hdl);
> +	tpg_free(&vsen->tpg);
> +	kfree(vsen);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
> +	.release = vimc_sen_release,
> +};
> +
>  static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>  				 void *master_data)
>  {
> @@ -309,9 +323,6 @@ static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>  				container_of(ved, struct vimc_sen_device, ved);
>  
>  	vimc_ent_sd_unregister(ved, &vsen->sd);
> -	v4l2_ctrl_handler_free(&vsen->hdl);
> -	tpg_free(&vsen->tpg);
> -	kfree(vsen);
>  }
>  
>  /* Image Processing Controls */
> @@ -371,7 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  				   pdata->entity_name,
>  				   MEDIA_ENT_F_CAM_SENSOR, 1,
>  				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
> -				   &vimc_sen_ops);
> +				   &vimc_sen_int_ops, &vimc_sen_ops);
>  	if (ret)
>  		goto err_free_hdl;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 6/9] v4l2-subdev: add release() internal op

[Hans Verkuil]

On 3/5/19 8:46 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> If the subdevice created a device node, then the v4l2_subdev cannot
>> be freed until the last user of the device node closes it.
>>
>> This means that we need a release() callback in v4l2_subdev_internal_ops
>> that is called from the video_device release function so the subdevice
>> driver can postpone freeing memory until the that callback is called.
>>
>> If no video device node was created then the release callback can
>> be called immediately when the subdev is unregistered.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++-----
>>  include/media/v4l2-subdev.h           |  3 +++
>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>> index e0ddb9a52bd1..7cca0de1b730 100644
>> --- a/drivers/media/v4l2-core/v4l2-device.c
>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>>  
>> +static void v4l2_subdev_release(struct v4l2_subdev *sd)
>> +{
>> +	struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
>> +
>> +	if (sd->internal_ops && sd->internal_ops->release)
>> +		sd->internal_ops->release(sd);
>> +	module_put(owner);
>> +}
>> +
>>  static void v4l2_device_release_subdev_node(struct video_device *vdev)
>>  {
>> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
>> -	sd->devnode = NULL;
>> +	v4l2_subdev_release(video_get_drvdata(vdev));
>>  	kfree(vdev);
>>  }
>>  
>> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>>  		media_device_unregister_entity(&sd->entity);
>>  	}
>>  #endif
>> -	video_unregister_device(sd->devnode);
>> -	if (!sd->owner_v4l2_dev)
>> -		module_put(sd->owner);
>> +	if (sd->devnode)
>> +		video_unregister_device(sd->devnode);
>> +	else
>> +		v4l2_subdev_release(sd);
>>  }
> 
> Don't we also need to handle the error path in
> v4l2_device_register_subdev_nodes() ?

No, in the error path of v4l2_device_register_subdev_nodes() the registered
video devices are simply unregistered, and so they will do the clean up
via v4l2_device_release_subdev_node(). Nothing changes there.

> 
>>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 349e1c18cf48..2f2d1c8947e6 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops {
>>   *
>>   * @close: called when the subdev device node is closed.
>>   *
>> + * @release: called when the subdev device node is released.
>> + *
> 
> I think this should be expanded a bit. First of all, we should mention
> what happens when the subdev doesn't have a device node, and then we
> should also explain what drivers are supposed to do in this operation.

I'll extend this a bit.

> At what point do you think we should add a WARN_ON(!sd->internal_ops ||
> !sd->internal_ops->release) ?

I wondered about that myself. I think the first step is to make the
regression test for the virtual drivers work flawlessly (i.e. no more
compliance errors/warnings/kernel oopses/warnings), then I want to extend
the regression test so you can run it for other drivers than the virtual
drivers.

After that I plan on asking the maintainers of the drivers that use the MC
to run the test and see if they hit this issue (they likely will). Once
enough of those drivers have been fixed we can add this warning.

That was the long answer, the short answer is: hopefully by the end of this year :-)

> I expect we'll need to refcount the subdev structure, with the
> video_device only having one of the multiple references to the subdev,
> but that can be implemented later. Overall this moves us in the right
> direction, thank you for your work.

Yeah, I need to do more testing for this. One test that isn't in the regression
test is unbinding subdevs, but not the main bridge device. If refcounting is
really necessary, then such a test should uncover this. It's getting quite
difficult to understand all the dependencies.

> 
>>   * .. note::
>>   *	Never call this from drivers, only the v4l2 framework can call
>>   *	these ops.
>> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops {
>>  	void (*unregistered)(struct v4l2_subdev *sd);
>>  	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>>  	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>> +	void (*release)(struct v4l2_subdev *sd);
>>  };
>>  
>>  #define V4L2_SUBDEV_NAME_SIZE 32
> 

Regards,

	Hans

Re: [PATCHv2 7/9] v4l2-subdev: handle module refcounting here

[Hans Verkuil]

On 3/5/19 8:52 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> (CC'ing Sakari)
> 
> Thank you for the patch.
> 
> On Tue, Mar 05, 2019 at 10:58:45AM +0100, hverkuil-cisco@xs4all.nl wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> The module ownership refcounting was done in media_entity_get/put,
>> but that was very confusing and it did not work either in case an
>> application had a v4l-subdevX device open and the module was
>> unbound. When the v4l-subdevX device was closed the media_entity_put
>> was never called and the module refcount was left one too high, making
>> it impossible to unload it.
>>
>> Since v4l2-subdev.c was the only place where media_entity_get/put was
>> called, just move the functionality to v4l2-subdev.c and drop those
>> confusing entity functions.
> 
> I wonder if we will later need to refcount media entities, but we can
> reintroduce a different version of those two functions then, it doesn't
> prevent their removal now.
> 
> Sakari, when working on lifetime management of objects in the media and
> V4L2 core, did you come across a need to refcount entities ?
> 
>> Store the module in subdev_fh so module_put no longer depends on
>> the media_entity struct.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/media-entity.c          | 28 ---------------------------
>>  drivers/media/v4l2-core/v4l2-subdev.c | 22 +++++++++------------
>>  include/media/media-entity.h          | 24 -----------------------
>>  include/media/v4l2-subdev.h           |  1 +
>>  4 files changed, 10 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index 7b2a2cc95530..257f20d2fb8a 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -17,7 +17,6 @@
>>   */
>>  
>>  #include <linux/bitmap.h>
>> -#include <linux/module.h>
>>  #include <linux/property.h>
>>  #include <linux/slab.h>
>>  #include <media/media-entity.h>
>> @@ -588,33 +587,6 @@ void media_pipeline_stop(struct media_entity *entity)
>>  }
>>  EXPORT_SYMBOL_GPL(media_pipeline_stop);
>>  
>> -/* -----------------------------------------------------------------------------
>> - * Module use count
>> - */
>> -
>> -struct media_entity *media_entity_get(struct media_entity *entity)
>> -{
>> -	if (entity == NULL)
>> -		return NULL;
>> -
>> -	if (entity->graph_obj.mdev->dev &&
>> -	    !try_module_get(entity->graph_obj.mdev->dev->driver->owner))
>> -		return NULL;
>> -
>> -	return entity;
>> -}
>> -EXPORT_SYMBOL_GPL(media_entity_get);
>> -
>> -void media_entity_put(struct media_entity *entity)
>> -{
>> -	if (entity == NULL)
>> -		return;
>> -
>> -	if (entity->graph_obj.mdev->dev)
>> -		module_put(entity->graph_obj.mdev->dev->driver->owner);
>> -}
>> -EXPORT_SYMBOL_GPL(media_entity_put);
>> -
>>  /* -----------------------------------------------------------------------------
>>   * Links management
>>   */
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index f5f0d71ec745..d75815ab0d7b 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -18,6 +18,7 @@
>>  
>>  #include <linux/ioctl.h>
>>  #include <linux/mm.h>
>> +#include <linux/module.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>  #include <linux/videodev2.h>
>> @@ -54,9 +55,6 @@ static int subdev_open(struct file *file)
>>  	struct video_device *vdev = video_devdata(file);
>>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>>  	struct v4l2_subdev_fh *subdev_fh;
>> -#if defined(CONFIG_MEDIA_CONTROLLER)
>> -	struct media_entity *entity = NULL;
>> -#endif
>>  	int ret;
>>  
>>  	subdev_fh = kzalloc(sizeof(*subdev_fh), GFP_KERNEL);
>> @@ -73,12 +71,15 @@ static int subdev_open(struct file *file)
>>  	v4l2_fh_add(&subdev_fh->vfh);
>>  	file->private_data = &subdev_fh->vfh;
>>  #if defined(CONFIG_MEDIA_CONTROLLER)
> 
> On a side note, do you think it's time to select MEDIA_CONTROLLER
> unconditionally ?

I wouldn't be opposed to that.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

	Hans

> 
>> -	if (sd->v4l2_dev->mdev) {
>> -		entity = media_entity_get(&sd->entity);
>> -		if (!entity) {
>> +	if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
>> +		struct module *owner;
>> +
>> +		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
>> +		if (!try_module_get(owner)) {
>>  			ret = -EBUSY;
>>  			goto err;
>>  		}
>> +		subdev_fh->owner = owner;
>>  	}
>>  #endif
>>  
>> @@ -91,9 +92,7 @@ static int subdev_open(struct file *file)
>>  	return 0;
>>  
>>  err:
>> -#if defined(CONFIG_MEDIA_CONTROLLER)
>> -	media_entity_put(entity);
>> -#endif
>> +	module_put(subdev_fh->owner);
>>  	v4l2_fh_del(&subdev_fh->vfh);
>>  	v4l2_fh_exit(&subdev_fh->vfh);
>>  	subdev_fh_free(subdev_fh);
>> @@ -111,10 +110,7 @@ static int subdev_close(struct file *file)
>>  
>>  	if (sd->internal_ops && sd->internal_ops->close)
>>  		sd->internal_ops->close(sd, subdev_fh);
>> -#if defined(CONFIG_MEDIA_CONTROLLER)
>> -	if (sd->v4l2_dev->mdev)
>> -		media_entity_put(&sd->entity);
>> -#endif
>> +	module_put(subdev_fh->owner);
>>  	v4l2_fh_del(vfh);
>>  	v4l2_fh_exit(vfh);
>>  	subdev_fh_free(subdev_fh);
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index e5f6960d92f6..3cbad42e3693 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -864,19 +864,6 @@ struct media_link *media_entity_find_link(struct media_pad *source,
>>   */
>>  struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
>>  
>> -/**
>> - * media_entity_get - Get a reference to the parent module
>> - *
>> - * @entity: The entity
>> - *
>> - * Get a reference to the parent media device module.
>> - *
>> - * The function will return immediately if @entity is %NULL.
>> - *
>> - * Return: returns a pointer to the entity on success or %NULL on failure.
>> - */
>> -struct media_entity *media_entity_get(struct media_entity *entity);
>> -
>>  /**
>>   * media_entity_get_fwnode_pad - Get pad number from fwnode
>>   *
>> @@ -916,17 +903,6 @@ __must_check int media_graph_walk_init(
>>   */
>>  void media_graph_walk_cleanup(struct media_graph *graph);
>>  
>> -/**
>> - * media_entity_put - Release the reference to the parent module
>> - *
>> - * @entity: The entity
>> - *
>> - * Release the reference count acquired by media_entity_get().
>> - *
>> - * The function will return immediately if @entity is %NULL.
>> - */
>> -void media_entity_put(struct media_entity *entity);
>> -
>>  /**
>>   * media_graph_walk_start - Start walking the media graph at a
>>   *	given entity
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 2f2d1c8947e6..33ba56f3dc1f 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -905,6 +905,7 @@ struct v4l2_subdev {
>>   */
>>  struct v4l2_subdev_fh {
>>  	struct v4l2_fh vfh;
>> +	struct module *owner;
>>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>  	struct v4l2_subdev_pad_config *pad;
>>  #endif
> 

Re: [PATCHv2 6/9] v4l2-subdev: add release() internal op

[Laurent Pinchart]

Hi Hans,

On Thu, Mar 07, 2019 at 09:54:53AM +0100, Hans Verkuil wrote:
> On 3/5/19 8:46 PM, Laurent Pinchart wrote:
> > On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote:
> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>
> >> If the subdevice created a device node, then the v4l2_subdev cannot
> >> be freed until the last user of the device node closes it.
> >>
> >> This means that we need a release() callback in v4l2_subdev_internal_ops
> >> that is called from the video_device release function so the subdevice
> >> driver can postpone freeing memory until the that callback is called.
> >>
> >> If no video device node was created then the release callback can
> >> be called immediately when the subdev is unregistered.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++-----
> >>  include/media/v4l2-subdev.h           |  3 +++
> >>  2 files changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> >> index e0ddb9a52bd1..7cca0de1b730 100644
> >> --- a/drivers/media/v4l2-core/v4l2-device.c
> >> +++ b/drivers/media/v4l2-core/v4l2-device.c
> >> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
> >>  }
> >>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
> >>  
> >> +static void v4l2_subdev_release(struct v4l2_subdev *sd)
> >> +{
> >> +	struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
> >> +
> >> +	if (sd->internal_ops && sd->internal_ops->release)
> >> +		sd->internal_ops->release(sd);
> >> +	module_put(owner);
> >> +}
> >> +
> >>  static void v4l2_device_release_subdev_node(struct video_device *vdev)
> >>  {
> >> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
> >> -	sd->devnode = NULL;
> >> +	v4l2_subdev_release(video_get_drvdata(vdev));
> >>  	kfree(vdev);
> >>  }
> >>  
> >> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> >>  		media_device_unregister_entity(&sd->entity);
> >>  	}
> >>  #endif
> >> -	video_unregister_device(sd->devnode);
> >> -	if (!sd->owner_v4l2_dev)
> >> -		module_put(sd->owner);
> >> +	if (sd->devnode)
> >> +		video_unregister_device(sd->devnode);
> >> +	else
> >> +		v4l2_subdev_release(sd);
> >>  }
> > 
> > Don't we also need to handle the error path in
> > v4l2_device_register_subdev_nodes() ?
> 
> No, in the error path of v4l2_device_register_subdev_nodes() the registered
> video devices are simply unregistered, and so they will do the clean up
> via v4l2_device_release_subdev_node(). Nothing changes there.

You're right, my bad. In that case,

> >>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 349e1c18cf48..2f2d1c8947e6 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops {
> >>   *
> >>   * @close: called when the subdev device node is closed.
> >>   *
> >> + * @release: called when the subdev device node is released.
> >> + *
> > 
> > I think this should be expanded a bit. First of all, we should mention
> > what happens when the subdev doesn't have a device node, and then we
> > should also explain what drivers are supposed to do in this operation.
> 
> I'll extend this a bit.

With the documentation expanded,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > At what point do you think we should add a WARN_ON(!sd->internal_ops ||
> > !sd->internal_ops->release) ?
> 
> I wondered about that myself. I think the first step is to make the
> regression test for the virtual drivers work flawlessly (i.e. no more
> compliance errors/warnings/kernel oopses/warnings), then I want to extend
> the regression test so you can run it for other drivers than the virtual
> drivers.
> 
> After that I plan on asking the maintainers of the drivers that use the MC
> to run the test and see if they hit this issue (they likely will). Once
> enough of those drivers have been fixed we can add this warning.
> 
> That was the long answer, the short answer is: hopefully by the end of this year :-)

:-) The plan sounds good to me.

> > I expect we'll need to refcount the subdev structure, with the
> > video_device only having one of the multiple references to the subdev,
> > but that can be implemented later. Overall this moves us in the right
> > direction, thank you for your work.
> 
> Yeah, I need to do more testing for this. One test that isn't in the regression
> test is unbinding subdevs, but not the main bridge device.

That will be another big can of worms :-S

Another issue that needs to be tackled is the race between userspace and
unbind. Once the unbind function returns access to the device isn't
allowed anymore, so we need to ensure that we block unbind until all
userspace calls return, and that we block new ones once unbind has
started (I believe we've discussed this face-to-face with Sakari a
couple of years ago). Do you plan to work on that too ? I think it's
more important than the subdev partial unbind, as it can really lead to
crashes when hot-pluggable devices are removed.

> If refcounting is really necessary, then such a test should uncover
> this. It's getting quite difficult to understand all the dependencies.
> 
> >>   * .. note::
> >>   *	Never call this from drivers, only the v4l2 framework can call
> >>   *	these ops.
> >> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops {
> >>  	void (*unregistered)(struct v4l2_subdev *sd);
> >>  	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> >>  	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> >> +	void (*release)(struct v4l2_subdev *sd);
> >>  };
> >>  
> >>  #define V4L2_SUBDEV_NAME_SIZE 32

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 4/9] media-entity: set ent_enum->bmap to NULL after freeing it

[Hans Verkuil]

On 3/5/19 8:39 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> Ensure that this pointer is set to NULL after it is freed.
>> The vimc driver has a static media_entity and after
>> unbinding and rebinding the vimc device the media code will
>> try to free this pointer again since it wasn't set to NULL.
> 
> I still think the problem lies in the vimc driver. Reusing static
> structures is really asking for trouble. I'm however not opposed to
> merging this patch, as you mentioned the problem will be fixed in vimc
> too. I still wonder, though, if merging this couldn't make it easier for
> drivers to do the wrong thing.

I'm keeping this patch :-)

I don't think that what vimc is doing is wrong in principle, just very unusual.

Also I think it makes the mc framework more robust by properly zeroing this
pointer.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/media-entity.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index 0b1cb3559140..7b2a2cc95530 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
>>  void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
>>  {
>>  	kfree(ent_enum->bmap);
>> +	ent_enum->bmap = NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
>>  
> 

Re: [PATCHv2 7/9] v4l2-subdev: handle module refcounting here

[Sakari Ailus]

Hi Laurent, Hans,

On Tue, Mar 05, 2019 at 09:52:14PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> (CC'ing Sakari)
> 
> Thank you for the patch.
> 
> On Tue, Mar 05, 2019 at 10:58:45AM +0100, hverkuil-cisco@xs4all.nl wrote:
> > From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > 
> > The module ownership refcounting was done in media_entity_get/put,
> > but that was very confusing and it did not work either in case an
> > application had a v4l-subdevX device open and the module was
> > unbound. When the v4l-subdevX device was closed the media_entity_put
> > was never called and the module refcount was left one too high, making
> > it impossible to unload it.

Hmm. This is not really a proper fix for the problem although it lets you
unload the module in the case things got horribly wrong. Instead the media
device may not be allowed to disappear while it is being accessed
elsewhere.

I think that the merit of this patch is cleaning the code, not really
fixing a bug. But keeping the owner you can't access later because you've
released the memory isn't neat either.

How about adding a comment next to the owner field, e.g. "TODO: address
refcounting properly"?

> > 
> > Since v4l2-subdev.c was the only place where media_entity_get/put was
> > called, just move the functionality to v4l2-subdev.c and drop those
> > confusing entity functions.
> 
> I wonder if we will later need to refcount media entities, but we can
> reintroduce a different version of those two functions then, it doesn't
> prevent their removal now.

I agree.

> 
> Sakari, when working on lifetime management of objects in the media and
> V4L2 core, did you come across a need to refcount entities ?

We will need to do that to allow removing entities safely. The current
patchset only deals with the media device and it's stille pending on DVB
framework issues.

-- 
Regards,

Sakari Ailus

Re: [PATCHv2 4/9] media-entity: set ent_enum->bmap to NULL after freeing it

[Laurent Pinchart]

Hi Hans,

On Thu, Mar 07, 2019 at 10:23:03AM +0100, Hans Verkuil wrote:
> On 3/5/19 8:39 PM, Laurent Pinchart wrote:
> > On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote:
> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>
> >> Ensure that this pointer is set to NULL after it is freed.
> >> The vimc driver has a static media_entity and after
> >> unbinding and rebinding the vimc device the media code will
> >> try to free this pointer again since it wasn't set to NULL.
> > 
> > I still think the problem lies in the vimc driver. Reusing static
> > structures is really asking for trouble. I'm however not opposed to
> > merging this patch, as you mentioned the problem will be fixed in vimc
> > too. I still wonder, though, if merging this couldn't make it easier for
> > drivers to do the wrong thing.
> 
> I'm keeping this patch :-)
> 
> I don't think that what vimc is doing is wrong in principle, just very unusual.

I disagree here. We've developed the media controller (and V4L2) core
code with many assumptions that structures are zeroed on allocation. For
the structures that are meant to be registered once only, the code
assumes, explicitly and implicitly, that some of the fields are zeroed.
Removing that assumption for the odd case of vimc will require you to
chase bugs for a long time. You've caught a few of the easier ones here,
I'm sure other will linger for a much longer time before they get fixed.
In the vimc case, the best option is to zero the structure manually if
you don't want to allocate it dynamically (and I think it should be
allocated dynamically).

For the record, I ran into a similar problem before when trying to
unregister and re-register a struct device. I reported what I considered
to be a bug, and Greg very clearly told me it was plain wrong. You will
run into similar issues due to the platform_device embedded in struct
vimc_device. Let's just allocate it dynamically.

> Also I think it makes the mc framework more robust by properly zeroing this
> pointer.

This patch is not wrong per-se, and I'm not opposed to it, but we should
fix issues in drivers, which would render this patch unneeded.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 4/9] media-entity: set ent_enum->bmap to NULL after freeing it

[Hans Verkuil]

On 3/8/19 12:26 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thu, Mar 07, 2019 at 10:23:03AM +0100, Hans Verkuil wrote:
>> On 3/5/19 8:39 PM, Laurent Pinchart wrote:
>>> On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote:
>>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>
>>>> Ensure that this pointer is set to NULL after it is freed.
>>>> The vimc driver has a static media_entity and after
>>>> unbinding and rebinding the vimc device the media code will
>>>> try to free this pointer again since it wasn't set to NULL.
>>>
>>> I still think the problem lies in the vimc driver. Reusing static
>>> structures is really asking for trouble. I'm however not opposed to
>>> merging this patch, as you mentioned the problem will be fixed in vimc
>>> too. I still wonder, though, if merging this couldn't make it easier for
>>> drivers to do the wrong thing.
>>
>> I'm keeping this patch :-)
>>
>> I don't think that what vimc is doing is wrong in principle, just very unusual.
> 
> I disagree here. We've developed the media controller (and V4L2) core
> code with many assumptions that structures are zeroed on allocation. For
> the structures that are meant to be registered once only, the code
> assumes, explicitly and implicitly, that some of the fields are zeroed.
> Removing that assumption for the odd case of vimc will require you to
> chase bugs for a long time. You've caught a few of the easier ones here,
> I'm sure other will linger for a much longer time before they get fixed.
> In the vimc case, the best option is to zero the structure manually if
> you don't want to allocate it dynamically (and I think it should be
> allocated dynamically).
> 
> For the record, I ran into a similar problem before when trying to
> unregister and re-register a struct device. I reported what I considered
> to be a bug, and Greg very clearly told me it was plain wrong. You will
> run into similar issues due to the platform_device embedded in struct
> vimc_device. Let's just allocate it dynamically.
> 
>> Also I think it makes the mc framework more robust by properly zeroing this
>> pointer.
> 
> This patch is not wrong per-se, and I'm not opposed to it, but we should
> fix issues in drivers, which would render this patch unneeded.
> 

I posted a v4 where I drop this patch and instead zero the global
media_device struct in the vimc probe() function.

Regards,

	Hans