[PATCH v4 00/26] Media device lifetime management

[PATCH v4 00/26] Media device lifetime management

[Sakari Ailus]

Hi folks,

This is a refresh of my 2016 RFC patchset to start addressing object
lifetime issues in Media controller. It further allows continuing work to
address lifetime management of media entities.

The underlying problem is described in detail in v4 of the previous RFC:
<URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
In brief, there is currently no connection between releasing media device
(and related) memory and IOCTL calls, meaning that there is a time window
during which released kernel memory can be accessed, and that access can be
triggered from the user space. The only reason why this is not a grave
security issue is that it is not triggerable by the user alone but requires
unbinding a device. That is still not an excuse for not fixing it.

This set differs from the earlier RFC to address the issue in the
following respects:

- Make changes for ipu3-cio2 driver, too.

- Continue to provide best effort attempt to keep the window between device
  removal and user space being able to access released memory as small as
  possible. This means the problem won't become worse for drivers for which
  Media device lifetime management has not been implemented.

The latter is achieved by adding a new object, Media devnode compat
reference, which is allocated, refcounted and eventually released by the
Media controller framework itself, and where the information on registration
and open filehandles is maintained. This is only done if the driver does not
manage the lifetime of the media device itself, i.e. its release operation
is NULL.

Due to this, Media device file handles will also be introduced by this
patchset. I thought the first user of this would be Media device events but
it seems we already need them here.

Some patches are temporarily reverted in order to make reworks easier,
then applied later on. Others are not re-applied: this is a change of
direction, not development over those patches. It would be possible to
squash the reverts into others on the expense of readability, so the
reverts are maintained for that reason.

I've tested this on ipu3-cio2 with and without the refcounting patch (media:
ipu3-cio2: Release the cio2 device context by media device callback),
including failures in a few parts of the driver initialisation process in
the MC framework. I've also tested the vimc driver.

Questions and comments are welcome.

since v3:

- Rework commit message of patch re-converting to cdev_device_add(). It's
  no longer the same patch.

- Convert drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
  and drivers/media/test-drivers/visl/visl-core.c to changed
  media_devnode_is_registered() argument.

- Properly check for NULL Media device on a V4L2 device in
  video_register_media_controller().

- Don't acquire Media device graph mutex in media_device_unregister() for
  checking whether the media device is registered.

- Remove extra call to v4l2_async_nf_cleanup() in
  drivers/media/platform/ti/omap3isp/isp.c.

since v2:

- Switch to spin_{,un}lock_irq() in fw_list_lock use.

- Clarify documentatino regarding unregistering and releasing the media
  device.

- Release minor number at unregister time (vs. release time). This
  effectively caused a few patches to be dropped and one more to be added
  (mc: Clear minor number reservation at unregistration time).

- Added a comment in ipu3-cio2 driver to clarify destroying mutexes in
  cio2_queue_exit().

- In ipu3-cio2 driver:

	- Clean up the notifier in probe.

	- Unregister the sub-devices at driver unbind time.

	- Remove queue initialisation error handling. The queues are
	  released in cio2_queues_exit() later in any case.

- Clean up V4L2 device teardown as suggested by Hans.

- Rewrap added text in video_register_media_controller().

- Make media_device_{get,put}() functions (they were macros) for better
  type checking.

- Improve kerneldoc for media_device_cleanup().

- Document release function in media_file_operations.

- Correct kerneldoc documentation for struct media_devnode.

- Fix media_devnode_is_registered() usage in sound/usb/media.c that was
  missed in v2.

- Drop old git tags from revert² patches.

- Drop revert and re-applification of "media: uvcvideo: Refactor teardown
  of uvc on USB disconnect", instead take this account into other patches.

- Drop patch "ipu3-cio2: Call v4l2_device_unregister() earlier".

- Remove patch "ipu3-cio2: Request IRQ earlier" from this set, it'll be
  merged separately.

since v1:

- Align subject prefixes with current media tree practices.

- Make release changes to the vimc driver (last patch of the set). This
  was actually easy as vimc already centralised resource release to struct
  v4l2_device, so it was just moved to the media device.

- Move cdev field to struct media_devnode_compat_ref and add dev field to
  the struct, these are needed during device release. This now includes
  also the character device which is accessed by __fput(). I've now tested
  ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct
  media_devnode_compat_ref becomes redundant and is removed. Both devices
  are registered in case of best effort memory safety support and used for
  refcounting.

- Drop omap3isp driver patch moving away from devm_request_irq().

- Add a patch to warn of drivers not releasing media device safely (i.e.
  relying on the best effort memory safety mechanism without refcounting).

- Add a patch to document how the best effort memory release safety helper
  works.

- Add a note on releasing driver's context with the media device, not the
  V4L2 device, in MC documentation.

- Check media device is registered before accessing its fops in
  media_read(), media_write(), media_ioctl and media_compat_ioctl().

- Document best effort media device lifetime management (new patch).

- Use media_devnode_free_minor() in unallocating device node minor number
  in media_devnode_register().

- Continue to rely on devm_register_irq() in ipu3-cio2 driver but register
  the IRQ later on (compared to v1).

- Drop the patch to move away from devm_request_irq() in omap3isp.

- Fix putting references to media device and V4L2 device in 
  v4l2_device_release().

- Add missing media_device_get() (in v1) for M2M devices in
  video_register_media_controller().

- Unconditionally set the media devnode release function in
  media_device_init(). There's no harm doing so and the caller of
  media_device_init() may set the ops after calling the function.


Laurent Pinchart (1):
  media: mc: Add per-file-handle data support

Sakari Ailus (25):
  Revert "[media] media: fix media devnode ioctl/syscall and unregister
    race"
  Revert "media: utilize new cdev_device_add helper function"
  Revert "[media] media: fix use-after-free in cdev_put() when app exits
    after driver unbind"
  media: mc, cec: Make use of cdev_device_add() again
  Revert "[media] media-device: dynamically allocate struct
    media_devnode"
  media: mc: Drop nop release callback
  media: mc: Drop media_dev description from struct media_devnode
  media: mc: Do not call cdev_device_del() if cdev_device_add() fails
  media: mc: Delete character device early
  media: mc: Clear minor number reservation at unregistration time
  media: mc: Split initialising and adding media devnode
  media: mc: Shuffle functions around
  media: mc: Initialise media devnode in media_device_init()
  media: mc: Refcount the media device
  media: v4l: Acquire a reference to the media device for every video
    device
  media: mc: Postpone graph object removal until free
  media: omap3isp: Release the isp device struct by media device
    callback
  media: ipu3-cio2: Release the cio2 device context by media device
    callback
  media: vimc: Release resources on media device release
  media: Documentation: Document how Media device resources are released
  media: mc: Maintain a list of open file handles in a media device
  media: mc: Implement best effort media device removal safety sans
    refcount
  media: mc: Warn about drivers not releasing media device safely
  media: mc: Enforce one-time registration
  media: Documentation: Document media device memory safety helper

 Documentation/driver-api/media/mc-core.rst    |  18 +-
 drivers/media/cec/core/cec-core.c             |   2 +-
 drivers/media/mc/mc-device.c                  | 258 +++++++++++-------
 drivers/media/mc/mc-devnode.c                 | 208 +++++++++-----
 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  72 +++--
 .../vcodec/decoder/mtk_vcodec_dec_drv.c       |   2 +-
 drivers/media/platform/ti/omap3isp/isp.c      |  25 +-
 drivers/media/test-drivers/vimc/vimc-core.c   |  15 +-
 drivers/media/test-drivers/visl/visl-core.c   |   2 +-
 drivers/media/usb/au0828/au0828-core.c        |   4 +-
 drivers/media/usb/uvc/uvc_driver.c            |   2 +-
 drivers/media/v4l2-core/v4l2-dev.c            |  67 +++--
 drivers/staging/media/sunxi/cedrus/cedrus.c   |   2 +-
 include/media/media-device.h                  |  53 +++-
 include/media/media-devnode.h                 | 136 ++++++---
 include/media/media-fh.h                      |  32 +++
 sound/usb/media.c                             |   8 +-
 17 files changed, 623 insertions(+), 283 deletions(-)
 create mode 100644 include/media/media-fh.h

-- 
2.39.2

[PATCH v4 01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Sakari Ailus]

This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
ioctl/syscall and unregister race"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c  | 15 +++++++--------
 drivers/media/mc/mc-devnode.c |  8 +-------
 include/media/media-devnode.h | 16 ++--------------
 3 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c0dd4ae57227..6c569ecd4b3d 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -741,7 +741,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (ret < 0) {
 		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
-		media_devnode_unregister_prepare(devnode);
 		media_devnode_unregister(devnode);
 		return ret;
 	}
@@ -797,9 +796,6 @@ void media_device_unregister(struct media_device *mdev)
 		return;
 	}
 
-	/* Clear the devnode register bit to avoid races with media dev open */
-	media_devnode_unregister_prepare(mdev->devnode);
-
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
 		__media_device_unregister_entity(entity);
@@ -824,10 +820,13 @@ void media_device_unregister(struct media_device *mdev)
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 
-	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-	media_devnode_unregister(mdev->devnode);
-	/* devnode free is handled in media_devnode_*() */
-	mdev->devnode = NULL;
+	/* Check if mdev devnode was registered */
+	if (media_devnode_is_registered(mdev->devnode)) {
+		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
+		media_devnode_unregister(mdev->devnode);
+		/* devnode free is handled in media_devnode_*() */
+		mdev->devnode = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 318e267e798e..22905c1d86e8 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -265,7 +265,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	return ret;
 }
 
-void media_devnode_unregister_prepare(struct media_devnode *devnode)
+void media_devnode_unregister(struct media_devnode *devnode)
 {
 	/* Check if devnode was ever registered at all */
 	if (!media_devnode_is_registered(devnode))
@@ -273,12 +273,6 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	mutex_unlock(&media_devnode_lock);
-}
-
-void media_devnode_unregister(struct media_devnode *devnode)
-{
-	mutex_lock(&media_devnode_lock);
 	/* Delete the cdev on this minor as well */
 	cdev_device_del(&devnode->cdev, &devnode->dev);
 	devnode->media_dev = NULL;
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d27c1c646c28..46f0d3ae44d1 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -115,19 +115,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 					struct media_devnode *devnode,
 					struct module *owner);
 
-/**
- * media_devnode_unregister_prepare - clear the media device node register bit
- * @devnode: the device node to prepare for unregister
- *
- * This clears the passed device register bit. Future open calls will be met
- * with errors. Should be called before media_devnode_unregister() to avoid
- * races with unregister and device file open calls.
- *
- * This function can safely be called if the device node has never been
- * registered or has already been unregistered.
- */
-void media_devnode_unregister_prepare(struct media_devnode *devnode);
-
 /**
  * media_devnode_unregister - unregister a media device node
  * @devnode: the device node to unregister
@@ -135,7 +122,8 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode);
  * This unregisters the passed device. Future open calls will be met with
  * errors.
  *
- * Should be called after media_devnode_unregister_prepare()
+ * This function can safely be called if the device node has never been
+ * registered or has already been unregistered.
  */
 void media_devnode_unregister(struct media_devnode *devnode);
 
-- 
2.39.2

[PATCH v4 02/26] Revert "media: utilize new cdev_device_add helper function"

[Sakari Ailus]

This reverts commit 857313e51006ff51524579bcd8808b70f9a80812. This patch is
temporarily reverted for internal rework.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/core/cec-core.c | 16 ++++++++++++----
 drivers/media/mc/mc-devnode.c     | 22 ++++++++++++++++------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c
index 6f940df0230c..3bacfd0ecd83 100644
--- a/drivers/media/cec/core/cec-core.c
+++ b/drivers/media/cec/core/cec-core.c
@@ -137,19 +137,26 @@ 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.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	devnode->registered = true;
-	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
-	if (ret) {
+	ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
+	if (ret < 0) {
+		pr_err("%s: cdev_add failed\n", __func__);
 		devnode->registered = false;
-		pr_err("%s: cdev_device_add failed\n", __func__);
 		goto clr_bit;
 	}
 
+	ret = device_add(&devnode->dev);
+	if (ret)
+		goto cdev_del;
+
 	return 0;
 
+cdev_del:
+	cdev_del(&devnode->cdev);
 clr_bit:
 	mutex_lock(&cec_devnode_lock);
 	clear_bit(devnode->minor, cec_devnode_nums);
@@ -195,7 +202,8 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	cec_adap_enable(adap);
 	mutex_unlock(&adap->lock);
 
-	cdev_device_del(&devnode->cdev, &devnode->dev);
+	device_del(&devnode->dev);
+	cdev_del(&devnode->cdev);
 	put_device(&devnode->dev);
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 22905c1d86e8..d36bc9891f3f 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -239,22 +239,31 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
 	device_initialize(&devnode->dev);
 
-	/* Part 2: Initialize the character device */
+	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
+	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
-	/* Part 3: Add the media and char device */
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
+	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t),
+					     devnode->minor), 1);
 	if (ret < 0) {
-		clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-		pr_err("%s: cdev_device_add failed\n", __func__);
+		pr_err("%s: cdev_add failed\n", __func__);
 		goto cdev_add_error;
 	}
 
+	/* Part 3: Add the media device */
+	ret = device_add(&devnode->dev);
+	if (ret < 0) {
+		pr_err("%s: device_add failed\n", __func__);
+		goto device_add_error;
+	}
+
 	return 0;
 
+device_add_error:
+	cdev_del(&devnode->cdev);
 cdev_add_error:
 	mutex_lock(&media_devnode_lock);
 	clear_bit(devnode->minor, media_devnode_nums);
@@ -274,9 +283,10 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	/* Delete the cdev on this minor as well */
-	cdev_device_del(&devnode->cdev, &devnode->dev);
+	cdev_del(&devnode->cdev);
 	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
+	device_del(&devnode->dev);
 
 	put_device(&devnode->dev);
 }
-- 
2.39.2

[PATCH v4 03/26] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind"

[Sakari Ailus]

This reverts commit 5b28dde51d0c ("[media] media: fix use-after-free in
cdev_put() when app exits after driver unbind"). The commit was part of an
original patchset to avoid crashes when an unregistering device is in use.

This revert is performed to roll back to a state which is more suitable
for the objective: making media device refcountable.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c  |  6 ++---
 drivers/media/mc/mc-devnode.c | 47 ++++++++++++++---------------------
 2 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 6c569ecd4b3d..4772a7f55112 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -732,16 +732,16 @@ int __must_check __media_device_register(struct media_device *mdev,
 
 	ret = media_devnode_register(mdev, devnode, owner);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
+		kfree(devnode);
 		return ret;
 	}
 
 	ret = device_create_file(&devnode->dev, &dev_attr_model);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
 		media_devnode_unregister(devnode);
+		kfree(devnode);
 		return ret;
 	}
 
@@ -824,8 +824,6 @@ void media_device_unregister(struct media_device *mdev)
 	if (media_devnode_is_registered(mdev->devnode)) {
 		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
 		media_devnode_unregister(mdev->devnode);
-		/* devnode free is handled in media_devnode_*() */
-		mdev->devnode = NULL;
 	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index d36bc9891f3f..bc223a427020 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -51,8 +51,13 @@ static void media_devnode_release(struct device *cd)
 	struct media_devnode *devnode = to_media_devnode(cd);
 
 	mutex_lock(&media_devnode_lock);
+
+	/* Delete the cdev on this minor as well */
+	cdev_del(&devnode->cdev);
+
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
+
 	mutex_unlock(&media_devnode_lock);
 
 	/* Release media_devnode and perform other cleanups as needed. */
@@ -60,7 +65,6 @@ static void media_devnode_release(struct device *cd)
 		devnode->release(devnode);
 
 	kfree(devnode);
-	pr_debug("%s: Media Devnode Deallocated\n", __func__);
 }
 
 static const struct bus_type media_bus_type = {
@@ -189,7 +193,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	put_device(&devnode->dev);
-
 	return 0;
 }
 
@@ -220,7 +223,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	if (minor == MEDIA_NUM_DEVICES) {
 		mutex_unlock(&media_devnode_lock);
 		pr_err("could not get a free minor\n");
-		kfree(devnode);
 		return -ENFILE;
 	}
 
@@ -230,19 +232,9 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */
-	devnode->dev.bus = &media_bus_type;
-	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
-	devnode->dev.release = media_devnode_release;
-	if (devnode->parent)
-		devnode->dev.parent = devnode->parent;
-	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	device_initialize(&devnode->dev);
-
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
@@ -250,27 +242,30 @@ int __must_check media_devnode_register(struct media_device *mdev,
 					     devnode->minor), 1);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
-		goto cdev_add_error;
+		goto error;
 	}
 
-	/* Part 3: Add the media device */
-	ret = device_add(&devnode->dev);
+	/* Part 3: Register the media device */
+	devnode->dev.bus = &media_bus_type;
+	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	devnode->dev.release = media_devnode_release;
+	if (devnode->parent)
+		devnode->dev.parent = devnode->parent;
+	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+	ret = device_register(&devnode->dev);
 	if (ret < 0) {
-		pr_err("%s: device_add failed\n", __func__);
-		goto device_add_error;
+		pr_err("%s: device_register failed\n", __func__);
+		goto error;
 	}
 
 	return 0;
 
-device_add_error:
-	cdev_del(&devnode->cdev);
-cdev_add_error:
+error:
 	mutex_lock(&media_devnode_lock);
+	cdev_del(&devnode->cdev);
 	clear_bit(devnode->minor, media_devnode_nums);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
 
-	put_device(&devnode->dev);
 	return ret;
 }
 
@@ -282,13 +277,9 @@ void media_devnode_unregister(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
-	device_del(&devnode->dev);
 
-	put_device(&devnode->dev);
+	device_unregister(&devnode->dev);
 }
 
 /*
-- 
2.39.2

[PATCH v4 04/26] media: mc, cec: Make use of cdev_device_add() again

[Sakari Ailus]

Bring back cdev_device_add() to Media controller and CEC frameworks after
briefly being removed for reverting other, unrelated patches.

The original commit is 857313e51006ff51524579bcd8808b70f9a80812.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/cec/core/cec-core.c | 16 ++++------------
 drivers/media/mc/mc-devnode.c     | 25 ++++++++++---------------
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c
index 3bacfd0ecd83..764ec73b0bb0 100644
--- a/drivers/media/cec/core/cec-core.c
+++ b/drivers/media/cec/core/cec-core.c
@@ -137,26 +137,19 @@ 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.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	devnode->registered = true;
-	ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
-	if (ret < 0) {
-		pr_err("%s: cdev_add failed\n", __func__);
+	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
+	if (ret) {
+		pr_err("%s: cdev_device_add failed\n", __func__);
 		devnode->registered = false;
 		goto clr_bit;
 	}
 
-	ret = device_add(&devnode->dev);
-	if (ret)
-		goto cdev_del;
-
 	return 0;
 
-cdev_del:
-	cdev_del(&devnode->cdev);
 clr_bit:
 	mutex_lock(&cec_devnode_lock);
 	clear_bit(devnode->minor, cec_devnode_nums);
@@ -202,8 +195,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	cec_adap_enable(adap);
 	mutex_unlock(&adap->lock);
 
-	device_del(&devnode->dev);
-	cdev_del(&devnode->cdev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 	put_device(&devnode->dev);
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index bc223a427020..64bbff0f90cd 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -232,37 +232,32 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 2: Initialize and register the character device */
+	/* Part 2: Initialize the media and character devices */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
-	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t),
-					     devnode->minor), 1);
-	if (ret < 0) {
-		pr_err("%s: cdev_add failed\n", __func__);
-		goto error;
-	}
-
-	/* Part 3: Register the media device */
 	devnode->dev.bus = &media_bus_type;
 	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
 	devnode->dev.release = media_devnode_release;
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	ret = device_register(&devnode->dev);
+	device_initialize(&devnode->dev);
+
+	/* Part 3: Add the media and character devices */
+	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
+	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
 	if (ret < 0) {
-		pr_err("%s: device_register failed\n", __func__);
-		goto error;
+		pr_err("%s: cdev_device_add failed\n", __func__);
+		goto cdev_add_error;
 	}
 
 	return 0;
 
-error:
+cdev_add_error:
 	mutex_lock(&media_devnode_lock);
-	cdev_del(&devnode->cdev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
-- 
2.39.2

[PATCH v4 05/26] Revert "[media] media-device: dynamically allocate struct media_devnode"

[Sakari Ailus]

This reverts commit a087ce704b80 ("[media] media-device: dynamically
allocate struct media_devnode"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

This revert is performed to roll back to a state which is more suitable
for the objective: making media device refcountable.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c                  | 44 ++++++-------------
 drivers/media/mc/mc-devnode.c                 |  7 +--
 .../vcodec/decoder/mtk_vcodec_dec_drv.c       |  2 +-
 drivers/media/test-drivers/visl/visl-core.c   |  2 +-
 drivers/media/usb/au0828/au0828-core.c        |  4 +-
 drivers/media/usb/uvc/uvc_driver.c            |  2 +-
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  2 +-
 include/media/media-device.h                  |  5 ++-
 include/media/media-devnode.h                 | 15 +------
 sound/usb/media.c                             |  8 ++--
 10 files changed, 31 insertions(+), 60 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 4772a7f55112..d4553a3705f5 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -435,7 +435,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long __arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	const struct media_ioctl_info *info;
 	void __user *arg = (void __user *)__arg;
 	char __karg[256], *karg = __karg;
@@ -519,7 +519,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	long ret;
 
 	switch (cmd) {
@@ -555,8 +555,7 @@ static const struct media_file_operations media_device_fops = {
 static ssize_t model_show(struct device *cd,
 			  struct device_attribute *attr, char *buf)
 {
-	struct media_devnode *devnode = to_media_devnode(cd);
-	struct media_device *mdev = devnode->media_dev;
+	struct media_device *mdev = to_media_device(to_media_devnode(cd));
 
 	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -714,34 +713,23 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
-	struct media_devnode *devnode;
 	int ret;
 
-	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
-	if (!devnode)
-		return -ENOMEM;
-
 	/* Register the device node. */
-	mdev->devnode = devnode;
-	devnode->fops = &media_device_fops;
-	devnode->parent = mdev->dev;
-	devnode->release = media_device_release;
+	mdev->devnode.fops = &media_device_fops;
+	mdev->devnode.parent = mdev->dev;
+	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	ret = media_devnode_register(mdev, devnode, owner);
-	if (ret < 0) {
-		mdev->devnode = NULL;
-		kfree(devnode);
+	ret = media_devnode_register(&mdev->devnode, owner);
+	if (ret < 0)
 		return ret;
-	}
 
-	ret = device_create_file(&devnode->dev, &dev_attr_model);
+	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
-		mdev->devnode = NULL;
-		media_devnode_unregister(devnode);
-		kfree(devnode);
+		media_devnode_unregister(&mdev->devnode);
 		return ret;
 	}
 
@@ -791,7 +779,7 @@ void media_device_unregister(struct media_device *mdev)
 	mutex_lock(&mdev->graph_mutex);
 
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(mdev->devnode)) {
+	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
@@ -818,13 +806,9 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	dev_dbg(mdev->dev, "Media device unregistered\n");
-
-	/* Check if mdev devnode was registered */
-	if (media_devnode_is_registered(mdev->devnode)) {
-		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-		media_devnode_unregister(mdev->devnode);
-	}
+	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	dev_dbg(mdev->dev, "Media device unregistering\n");
+	media_devnode_unregister(&mdev->devnode);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 64bbff0f90cd..2e33c2007f08 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -32,7 +32,6 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
-#include <media/media-device.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -63,8 +62,6 @@ static void media_devnode_release(struct device *cd)
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
-
-	kfree(devnode);
 }
 
 static const struct bus_type media_bus_type = {
@@ -210,8 +207,7 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -230,7 +226,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	mutex_unlock(&media_devnode_lock);
 
 	devnode->minor = minor;
-	devnode->media_dev = mdev;
 
 	/* Part 2: Initialize the media and character devices */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
index 2073781ccadb..0fd681b4571d 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
@@ -570,7 +570,7 @@ static void mtk_vcodec_dec_remove(struct platform_device *pdev)
 
 	destroy_workqueue(dev->decode_workqueue);
 
-	if (media_devnode_is_registered(dev->mdev_dec.devnode)) {
+	if (media_devnode_is_registered(&dev->mdev_dec.devnode)) {
 		media_device_unregister(&dev->mdev_dec);
 		v4l2_m2m_unregister_media_controller(dev->m2m_dev_dec);
 		media_device_cleanup(&dev->mdev_dec);
diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
index c46464bcaf2e..151d4a09559c 100644
--- a/drivers/media/test-drivers/visl/visl-core.c
+++ b/drivers/media/test-drivers/visl/visl-core.c
@@ -513,7 +513,7 @@ static void visl_remove(struct platform_device *pdev)
 	v4l2_info(&dev->v4l2_dev, "Removing " VISL_NAME);
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(dev->mdev.devnode)) {
+	if (media_devnode_is_registered(&dev->mdev.devnode)) {
 		media_device_unregister(&dev->mdev);
 		v4l2_m2m_unregister_media_controller(dev->m2m_dev);
 	}
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 1e246b47766d..4101e12cda5a 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -128,7 +128,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	struct media_device *mdev = dev->media_dev;
 	struct media_entity_notify *notify, *nextp;
 
-	if (!mdev || !media_devnode_is_registered(mdev->devnode))
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
 		return;
 
 	/* Remove au0828 entity_notify callbacks */
@@ -566,7 +566,7 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 	if (!dev->media_dev)
 		return 0;
 
-	if (!media_devnode_is_registered(dev->media_dev->devnode)) {
+	if (!media_devnode_is_registered(&dev->media_dev->devnode)) {
 
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 8fe24c98087e..436c1d361906 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1923,7 +1923,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(dev->mdev.devnode))
+	if (media_devnode_is_registered(&dev->mdev.devnode))
 		media_device_unregister(&dev->mdev);
 #endif
 }
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index f52df6836045..f8f678835a4c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -548,7 +548,7 @@ static void cedrus_remove(struct platform_device *pdev)
 	struct cedrus_dev *dev = platform_get_drvdata(pdev);
 
 	cancel_delayed_work_sync(&dev->watchdog_work);
-	if (media_devnode_is_registered(dev->mdev.devnode)) {
+	if (media_devnode_is_registered(&dev->mdev.devnode)) {
 		media_device_unregister(&dev->mdev);
 		v4l2_m2m_unregister_media_controller(dev->m2m_dev);
 		media_device_cleanup(&dev->mdev);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 53d2a16a70b0..c791f3d5ad77 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -145,7 +145,7 @@ struct media_device_ops {
 struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
-	struct media_devnode *devnode;
+	struct media_devnode devnode;
 
 	char model[32];
 	char driver_name[32];
@@ -191,6 +191,9 @@ struct usb_device;
 #define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
 #define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
 
+/* media_devnode to media_device */
+#define to_media_device(node) container_of(node, struct media_device, devnode)
+
 /**
  * media_device_init() - Initializes a media device element
  *
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 46f0d3ae44d1..1117d1dfd6bf 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -21,8 +21,6 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
-struct media_device;
-
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
@@ -73,8 +71,6 @@ struct media_file_operations {
  * before registering the node.
  */
 struct media_devnode {
-	struct media_device *media_dev;
-
 	/* device ops */
 	const struct media_file_operations *fops;
 
@@ -97,8 +93,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
- * @mdev: struct media_device we want to register a device node
- * @devnode: media device node structure we want to register
+ * @devnode: struct media_devnode we want to register a device node
  * @owner: should be filled with %THIS_MODULE
  *
  * The registration code assigns minor numbers and registers the new device node
@@ -111,8 +106,7 @@ struct media_devnode {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
 
 /**
@@ -142,14 +136,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
  *	false otherwise.
  *
  * @devnode: pointer to struct &media_devnode.
- *
- * Note: If mdev is NULL, it also returns false.
  */
 static inline int media_devnode_is_registered(struct media_devnode *devnode)
 {
-	if (!devnode)
-		return false;
-
 	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 }
 
diff --git a/sound/usb/media.c b/sound/usb/media.c
index d48db6f3ae65..f97e937378ee 100644
--- a/sound/usb/media.c
+++ b/sound/usb/media.c
@@ -122,7 +122,7 @@ void snd_media_stream_delete(struct snd_usb_substream *subs)
 		struct media_device *mdev;
 
 		mdev = mctl->media_dev;
-		if (mdev && media_devnode_is_registered(mdev->devnode)) {
+		if (mdev && media_devnode_is_registered(&mdev->devnode)) {
 			media_devnode_remove(mctl->intf_devnode);
 			media_device_unregister_entity(&mctl->media_entity);
 			media_entity_cleanup(&mctl->media_entity);
@@ -239,14 +239,14 @@ static void snd_media_mixer_delete(struct snd_usb_audio *chip)
 		if (!mixer->media_mixer_ctl)
 			continue;
 
-		if (media_devnode_is_registered(mdev->devnode)) {
+		if (media_devnode_is_registered(&mdev->devnode)) {
 			media_device_unregister_entity(&mctl->media_entity);
 			media_entity_cleanup(&mctl->media_entity);
 		}
 		kfree(mctl);
 		mixer->media_mixer_ctl = NULL;
 	}
-	if (media_devnode_is_registered(mdev->devnode))
+	if (media_devnode_is_registered(&mdev->devnode))
 		media_devnode_remove(chip->ctl_intf_media_devnode);
 	chip->ctl_intf_media_devnode = NULL;
 }
@@ -284,7 +284,7 @@ int snd_media_device_create(struct snd_usb_audio *chip,
 			"Couldn't create media mixer entities. Error: %d\n",
 			ret);
 
-	if (!media_devnode_is_registered(mdev->devnode)) {
+	if (!media_devnode_is_registered(&mdev->devnode)) {
 		/* don't register if snd_media_mixer_init() failed */
 		if (ret)
 			goto create_fail;
-- 
2.39.2

[PATCH v4 06/26] media: mc: Drop nop release callback

[Sakari Ailus]

The release callback is only used to print a debug message. Drop it. (It
will be re-introduced later in a different form.)

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/mc/mc-device.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index d4553a3705f5..c0ea08a8fc31 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -566,11 +566,6 @@ static DEVICE_ATTR_RO(model);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *devnode)
-{
-	dev_dbg(devnode->parent, "Media device released\n");
-}
-
 static void __media_device_unregister_entity(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
@@ -718,7 +713,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
-	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
-- 
2.39.2

[PATCH v4 07/26] media: mc: Drop media_dev description from struct media_devnode

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-devnode.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 1117d1dfd6bf..024dfb98a6fd 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -55,7 +55,6 @@ struct media_file_operations {
 
 /**
  * struct media_devnode - Media device node
- * @media_dev:	pointer to struct &media_device
  * @fops:	pointer to struct &media_file_operations with media device ops
  * @dev:	pointer to struct &device containing the media controller device
  * @cdev:	struct cdev pointer character device
-- 
2.39.2

[PATCH v4 08/26] media: mc: Do not call cdev_device_del() if cdev_device_add() fails

[Sakari Ailus]

cdev_device_del() is the right function to remove a device when
cdev_device_add() succeeds. If it does not, however, put_device() needs to
be used instead. Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-devnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 2e33c2007f08..707593d127a7 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -252,9 +252,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 
 cdev_add_error:
 	mutex_lock(&media_devnode_lock);
-	cdev_device_del(&devnode->cdev, &devnode->dev);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
+	put_device(&devnode->dev);
 
 	return ret;
 }
-- 
2.39.2

[PATCH v4 09/26] media: mc: Delete character device early

[Sakari Ailus]

The parent of the character device related to the media devnode is the
media devnode. Thus the character device needs to be released before the
media devnode's release function. Move it to unregistering of the media
devnode, which mirrors adding the character device in conjunction with
registering the media devnode.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/mc/mc-devnode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 707593d127a7..38c472498f9e 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -51,9 +51,6 @@ static void media_devnode_release(struct device *cd)
 
 	mutex_lock(&media_devnode_lock);
 
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
-
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
 
@@ -269,6 +266,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 
+	cdev_del(&devnode->cdev);
 	device_unregister(&devnode->dev);
 }
 
-- 
2.39.2

[PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Sakari Ailus]

Clear the media device's minor number reservation at unregister time as
there's no need to keep it reserved for longer. This makes it possible to
reserve the same minor right after unregistration.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 38c472498f9e..f7ecabe469a4 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -49,13 +49,6 @@ static void media_devnode_release(struct device *cd)
 {
 	struct media_devnode *devnode = to_media_devnode(cd);
 
-	mutex_lock(&media_devnode_lock);
-
-	/* Mark device node number as free */
-	clear_bit(devnode->minor, media_devnode_nums);
-
-	mutex_unlock(&media_devnode_lock);
-
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
@@ -268,6 +261,10 @@ void media_devnode_unregister(struct media_devnode *devnode)
 
 	cdev_del(&devnode->cdev);
 	device_unregister(&devnode->dev);
+
+	mutex_lock(&media_devnode_lock);
+	clear_bit(devnode->minor, media_devnode_nums);
+	mutex_unlock(&media_devnode_lock);
 }
 
 /*
-- 
2.39.2

[PATCH v4 11/26] media: mc: Split initialising and adding media devnode

[Sakari Ailus]

Registering a device node of an entity belonging to a media device will
require a reference to the struct device. Taking that reference is only
possible once the device has been initialised, which took place only when
it was registered. Split this in two, and initialise the device when the
media device is allocated.

Don't propagate the effects of these changes to drivers yet, we want to
expose media_device refcounting with media_device_get() and
media_device_put() functions first.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c  | 18 +++++++++++++-----
 drivers/media/mc/mc-devnode.c | 18 ++++++++++--------
 include/media/media-devnode.h | 19 ++++++++++++++-----
 3 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c0ea08a8fc31..dd4d589a6701 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -717,19 +717,26 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
+	media_devnode_init(&mdev->devnode);
+
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		return ret;
+		goto err_put;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0) {
-		media_devnode_unregister(&mdev->devnode);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_unregister;
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+err_unregister:
+	media_devnode_unregister(&mdev->devnode);
+err_put:
+	put_device(&mdev->devnode.dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -803,6 +810,7 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
+	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index f7ecabe469a4..214b9b142d90 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -197,6 +197,11 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
+void media_devnode_init(struct media_devnode *devnode)
+{
+	device_initialize(&devnode->dev);
+}
+
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
@@ -228,7 +233,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	device_initialize(&devnode->dev);
 
 	/* Part 3: Add the media and character devices */
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
@@ -244,7 +248,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	mutex_lock(&media_devnode_lock);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
-	put_device(&devnode->dev);
 
 	return ret;
 }
@@ -259,8 +262,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 
-	cdev_del(&devnode->cdev);
-	device_unregister(&devnode->dev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(devnode->minor, media_devnode_nums);
@@ -270,7 +272,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
 /*
  *	Initialise media for linux
  */
-static int __init media_devnode_init(void)
+static int __init media_devnode_module_init(void)
 {
 	int ret;
 
@@ -292,14 +294,14 @@ static int __init media_devnode_init(void)
 	return 0;
 }
 
-static void __exit media_devnode_exit(void)
+static void __exit media_devnode_module_exit(void)
 {
 	bus_unregister(&media_bus_type);
 	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
 }
 
-subsys_initcall(media_devnode_init);
-module_exit(media_devnode_exit)
+subsys_initcall(media_devnode_module_init);
+module_exit(media_devnode_module_exit)
 
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
 MODULE_DESCRIPTION("Device node registration for media drivers");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 024dfb98a6fd..113c317e6a0e 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -89,6 +89,17 @@ struct media_devnode {
 /* dev to media_devnode */
 #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
 
+/**
+ * media_devnode_init - initialise a media devnode
+ *
+ * @devnode: struct media_devnode we want to initialise
+ *
+ * Initialise a media devnode. Note that after initialising the media
+ * devnode is refcounted. Releasing references to it may be done using
+ * put_device().
+ */
+void media_devnode_init(struct media_devnode *devnode);
+
 /**
  * media_devnode_register - register a media device node
  *
@@ -99,11 +110,9 @@ struct media_devnode {
  * with the kernel. An error is returned if no free minor number can be found,
  * or if the registration of the device node fails.
  *
- * Zero is returned on success.
- *
- * Note that if the media_devnode_register call fails, the release() callback of
- * the media_devnode structure is *not* called, so the caller is responsible for
- * freeing any data.
+ * Zero is returned on success. Note that in case
+ * media_devnode_register() fails, the caller is responsible for
+ * releasing the reference to the device using put_device().
  */
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
-- 
2.39.2

[PATCH v4 12/26] media: mc: Shuffle functions around

[Sakari Ailus]

As the call paths of the functions in question will change, move them
around in anticipation of that. No other changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/mc/mc-device.c | 54 ++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index dd4d589a6701..f1f3addf7932 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -673,6 +673,33 @@ void media_device_unregister_entity(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+void media_device_register_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	list_add_tail(&nptr->list, &mdev->entity_notify);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
+
+/*
+ * Note: Should be called with mdev->lock held.
+ */
+static void __media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	list_del(&nptr->list);
+}
+
+void media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	__media_device_unregister_entity_notify(mdev, nptr);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+
 void media_device_init(struct media_device *mdev)
 {
 	INIT_LIST_HEAD(&mdev->entities);
@@ -740,33 +767,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
-void media_device_register_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	list_add_tail(&nptr->list, &mdev->entity_notify);
-	mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
-
-/*
- * Note: Should be called with mdev->lock held.
- */
-static void __media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	list_del(&nptr->list);
-}
-
-void media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	__media_device_unregister_entity_notify(mdev, nptr);
-	mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
-
 void media_device_unregister(struct media_device *mdev)
 {
 	struct media_entity *entity;
-- 
2.39.2

[PATCH v4 13/26] media: mc: Initialise media devnode in media_device_init()

[Sakari Ailus]

Call media_devnode_init() from media_device_init(). This has the effect of
creating a struct device for the media_devnode before it is registered,
making it possible to obtain a reference to it for e.g. video devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/mc/mc-device.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index f1f3addf7932..f1d89d940fe1 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -711,9 +711,10 @@ void media_device_init(struct media_device *mdev)
 	mutex_init(&mdev->req_queue_mutex);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
-
 	atomic_set(&mdev->request_id, 0);
 
+	media_devnode_init(&mdev->devnode);
+
 	if (!*mdev->bus_info)
 		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
 				   mdev->dev);
@@ -729,6 +730,7 @@ void media_device_cleanup(struct media_device *mdev)
 	media_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
 	mutex_destroy(&mdev->req_queue_mutex);
+	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
@@ -744,26 +746,19 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	media_devnode_init(&mdev->devnode);
-
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		goto err_put;
+		return ret;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0)
-		goto err_unregister;
+	if (ret < 0) {
+		media_devnode_unregister(&mdev->devnode);
+		return ret;
+	}
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
-
-err_unregister:
-	media_devnode_unregister(&mdev->devnode);
-err_put:
-	put_device(&mdev->devnode.dev);
-
-	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -810,7 +805,6 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
-	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
-- 
2.39.2

[PATCH v4 14/26] media: mc: Refcount the media device

[Sakari Ailus]

As the struct media_device embeds struct media_devnode, the lifetime of
that object must be that same than that of the media_device.

References are obtained by media_device_get() and released by
media_device_put(). In order to use refcounting, the driver must set the
release callback before calling media_device_init() on the media device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c  | 35 +++++++++++++++++++++++++++++------
 drivers/media/mc/mc-devnode.c |  2 +-
 include/media/media-device.h  | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index f1d89d940fe1..bbc233e726d2 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -700,6 +700,31 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
+static void __media_device_release(struct media_device *mdev)
+{
+	dev_dbg(mdev->dev, "Media device released\n");
+
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
+	media_graph_walk_cleanup(&mdev->pm_count_walk);
+	mutex_destroy(&mdev->graph_mutex);
+	mutex_destroy(&mdev->req_queue_mutex);
+}
+
+static void media_device_release(struct media_devnode *devnode)
+{
+	struct media_device *mdev = to_media_device(devnode);
+
+	if (mdev->ops && mdev->ops->release) {
+		/*
+		 * If release op isn't set, __media_device_release() is called
+		 * via media_device_cleanup().
+		 */
+		__media_device_release(mdev);
+		mdev->ops->release(mdev);
+	}
+}
+
 void media_device_init(struct media_device *mdev)
 {
 	INIT_LIST_HEAD(&mdev->entities);
@@ -713,6 +738,7 @@ void media_device_init(struct media_device *mdev)
 	ida_init(&mdev->entity_internal_idx);
 	atomic_set(&mdev->request_id, 0);
 
+	mdev->devnode.release = media_device_release;
 	media_devnode_init(&mdev->devnode);
 
 	if (!*mdev->bus_info)
@@ -725,12 +751,9 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
-	ida_destroy(&mdev->entity_internal_idx);
-	mdev->entity_internal_idx_max = 0;
-	media_graph_walk_cleanup(&mdev->pm_count_walk);
-	mutex_destroy(&mdev->graph_mutex);
-	mutex_destroy(&mdev->req_queue_mutex);
-	put_device(&mdev->devnode.dev);
+	WARN_ON(mdev->ops && mdev->ops->release);
+	__media_device_release(mdev);
+	media_device_put(mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 214b9b142d90..f27d5d288a38 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -200,6 +200,7 @@ static const struct file_operations media_devnode_fops = {
 void media_devnode_init(struct media_devnode *devnode)
 {
 	device_initialize(&devnode->dev);
+	devnode->dev.release = media_devnode_release;
 }
 
 int __must_check media_devnode_register(struct media_devnode *devnode,
@@ -229,7 +230,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 
 	devnode->dev.bus = &media_bus_type;
 	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
-	devnode->dev.release = media_devnode_release;
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index c791f3d5ad77..f1afbfc4dca2 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -62,6 +62,7 @@ struct media_entity_notify {
  *	       request (and thus the buffer) must be available to the driver.
  *	       And once a buffer is queued, then the driver can complete
  *	       or delete objects from the request before req_queue exits.
+ * @release: Release the resources of the media device.
  */
 struct media_device_ops {
 	int (*link_notify)(struct media_link *link, u32 flags,
@@ -70,6 +71,7 @@ struct media_device_ops {
 	void (*req_free)(struct media_request *req);
 	int (*req_validate)(struct media_request *req);
 	void (*req_queue)(struct media_request *req);
+	void (*release)(struct media_device *mdev);
 };
 
 /**
@@ -219,6 +221,32 @@ struct usb_device;
  */
 void media_device_init(struct media_device *mdev);
 
+/**
+ * media_device_get() - atomically increment the reference count for the media
+ * device
+ *
+ * Returns: the media device
+ *
+ * @mdev: media device
+ */
+static inline struct media_device *media_device_get(struct media_device *mdev)
+{
+	get_device(&mdev->devnode.dev);
+
+	return mdev;
+}
+
+/**
+ * media_device_put() - atomically decrement the reference count for the media
+ * device
+ *
+ * @mdev: media device
+ */
+static inline void media_device_put(struct media_device *mdev)
+{
+	put_device(&mdev->devnode.dev);
+}
+
 /**
  * media_device_cleanup() - Cleanups a media device element
  *
@@ -435,6 +463,13 @@ void __media_device_usb_init(struct media_device *mdev,
 static inline void media_device_init(struct media_device *mdev)
 {
 }
+static inline struct media_device *media_device_get(struct media_device *mdev)
+{
+	return NULL;
+}
+static inline void media_device_put(struct media_device *mdev)
+{
+}
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
-- 
2.39.2

[PATCH v4 15/26] media: v4l: Acquire a reference to the media device for every video device

[Sakari Ailus]

The video device depends on the existence of its media device --- if there
is one. Acquire a reference to it.

Note that when the media device release callback is used, then the V4L2
device release callback is ignored and a warning is issued if both are
set.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 53 ++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index be2ba7ca5de2..4bf4398fd2fe 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd)
 {
 	struct video_device *vdev = to_video_device(cd);
 	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+	bool v4l2_dev_call_release = v4l2_dev->release;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device *mdev = v4l2_dev->mdev;
+	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
+#endif
 
 	mutex_lock(&videodev_lock);
 	if (WARN_ON(video_devices[vdev->minor] != vdev)) {
@@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev && vdev->vfl_dir != VFL_DIR_M2M) {
 		/* Remove interfaces and interface links */
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -207,23 +212,28 @@ static void v4l2_device_release(struct device *cd)
 	}
 #endif
 
-	/* Do not call v4l2_device_put if there is no release callback set.
-	 * Drivers that have no v4l2_device release callback might free the
-	 * v4l2_dev instance in the video_device release callback below, so we
-	 * must perform this check here.
-	 *
-	 * TODO: In the long run all drivers that use v4l2_device should use the
-	 * v4l2_device release callback. This check will then be unnecessary.
-	 */
-	if (v4l2_dev->release == NULL)
-		v4l2_dev = NULL;
-
 	/* Release video_device and perform other
 	   cleanups as needed. */
 	vdev->release(vdev);
 
-	/* Decrease v4l2_device refcount */
-	if (v4l2_dev)
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev)
+		media_device_put(mdev);
+
+	/*
+	 * Generally both struct media_device and struct v4l2_device are
+	 * embedded in the same driver's context struct so having a release
+	 * callback in both is a bug.
+	 */
+	if (WARN_ON(v4l2_dev_call_release && mdev_has_release))
+		v4l2_dev_call_release = false;
+#endif
+
+	/*
+	 * Decrease v4l2_device refcount, but only if the media device doesn't
+	 * have a release callback.
+	 */
+	if (v4l2_dev_call_release)
 		v4l2_device_put(v4l2_dev);
 }
 
@@ -795,11 +805,17 @@ static int video_register_media_controller(struct video_device *vdev)
 	u32 intf_type;
 	int ret;
 
-	/* Memory-to-memory devices are more complex and use
-	 * their own function to register its mc entities.
+	if (!vdev->v4l2_dev->mdev)
+		return 0;
+
+	/*
+	 * Memory-to-memory devices are more complex and use their own function
+	 * to register its mc entities.
 	 */
-	if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M)
+	if (vdev->vfl_dir == VFL_DIR_M2M) {
+		media_device_get(vdev->v4l2_dev->mdev);
 		return 0;
+	}
 
 	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
 	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
@@ -878,6 +894,7 @@ static int video_register_media_controller(struct video_device *vdev)
 
 	/* FIXME: how to create the other interface links? */
 
+	media_device_get(vdev->v4l2_dev->mdev);
 #endif
 	return 0;
 }
-- 
2.39.2

[PATCH v4 16/26] media: mc: Postpone graph object removal until free

[Sakari Ailus]

The media device itself will be unregistered based on it being unbound and
driver's remove callback being called. The graph objects themselves may
still be in use; rely on the media device release callback to release
them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/mc/mc-device.c | 59 ++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index bbc233e726d2..f1a88edb7573 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -702,8 +702,33 @@ EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
 static void __media_device_release(struct media_device *mdev)
 {
+	struct media_entity *entity;
+	struct media_entity *next;
+	struct media_interface *intf, *tmp_intf;
+	struct media_entity_notify *notify, *nextp;
+
 	dev_dbg(mdev->dev, "Media device released\n");
 
+	/* Remove all entities from the media device */
+	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
+		__media_device_unregister_entity(entity);
+
+	/* Remove all entity_notify callbacks from the media device */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
+		__media_device_unregister_entity_notify(mdev, notify);
+
+	/* Remove all interfaces from the media device */
+	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+				 graph_obj.list) {
+		/*
+		 * Unlink the interface, but don't free it here; the
+		 * module which created it is responsible for freeing
+		 * it
+		 */
+		__media_remove_intf_links(intf);
+		media_gobj_destroy(&intf->graph_obj);
+	}
+
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
 	media_graph_walk_cleanup(&mdev->pm_count_walk);
@@ -787,43 +812,11 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 
 void media_device_unregister(struct media_device *mdev)
 {
-	struct media_entity *entity;
-	struct media_entity *next;
-	struct media_interface *intf, *tmp_intf;
-	struct media_entity_notify *notify, *nextp;
-
 	if (mdev == NULL)
 		return;
 
-	mutex_lock(&mdev->graph_mutex);
-
-	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(&mdev->devnode)) {
-		mutex_unlock(&mdev->graph_mutex);
+	if (!media_devnode_is_registered(&mdev->devnode))
 		return;
-	}
-
-	/* Remove all entities from the media device */
-	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		__media_device_unregister_entity(entity);
-
-	/* Remove all entity_notify callbacks from the media device */
-	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
-		__media_device_unregister_entity_notify(mdev, notify);
-
-	/* Remove all interfaces from the media device */
-	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
-				 graph_obj.list) {
-		/*
-		 * Unlink the interface, but don't free it here; the
-		 * module which created it is responsible for freeing
-		 * it
-		 */
-		__media_remove_intf_links(intf);
-		media_gobj_destroy(&intf->graph_obj);
-	}
-
-	mutex_unlock(&mdev->graph_mutex);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
-- 
2.39.2

[PATCH v4 17/26] media: omap3isp: Release the isp device struct by media device callback

[Sakari Ailus]

Use the media device release callback to release the isp device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the isp driver is being
unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/ti/omap3isp/isp.c | 25 ++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c
index 1cda23244c7b..b63b750452b1 100644
--- a/drivers/media/platform/ti/omap3isp/isp.c
+++ b/drivers/media/platform/ti/omap3isp/isp.c
@@ -649,8 +649,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
 	return IRQ_HANDLED;
 }
 
+static void isp_release(struct media_device *mdev);
+
 static const struct media_device_ops isp_media_ops = {
 	.link_notify = v4l2_pipeline_link_notify,
+	.release = isp_release,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1607,7 +1610,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
 	v4l2_device_unregister(&isp->v4l2_dev);
-	media_device_cleanup(&isp->media_dev);
 }
 
 static int isp_link_entity(
@@ -1955,6 +1957,19 @@ static void isp_detach_iommu(struct isp_device *isp)
 #endif
 }
 
+static void isp_release(struct media_device *mdev)
+{
+	struct isp_device *isp =
+		container_of(mdev, struct isp_device, media_dev);
+
+	isp_cleanup_modules(isp);
+
+	media_entity_enum_cleanup(&isp->crashed);
+	v4l2_async_nf_cleanup(&isp->notifier);
+
+	kfree(isp);
+}
+
 static int isp_attach_iommu(struct isp_device *isp)
 {
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
@@ -2002,18 +2017,16 @@ static void isp_remove(struct platform_device *pdev)
 	struct isp_device *isp = platform_get_drvdata(pdev);
 
 	v4l2_async_nf_unregister(&isp->notifier);
-	v4l2_async_nf_cleanup(&isp->notifier);
 	isp_unregister_entities(isp);
-	isp_cleanup_modules(isp);
+
 	isp_xclk_cleanup(isp);
 
 	__omap3isp_get(isp, false);
 	isp_detach_iommu(isp);
 	__omap3isp_put(isp, false);
 
-	media_entity_enum_cleanup(&isp->crashed);
-
-	kfree(isp);
+	/* May release isp immediately */
+	media_device_put(&isp->media_dev);
 }
 
 enum isp_of_phy {
-- 
2.39.2

[PATCH v4 18/26] media: ipu3-cio2: Release the cio2 device context by media device callback

[Sakari Ailus]

Use the media device release callback to release the cio2 device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the ipu3-cio2 driver is
being unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 72 ++++++++++++++++--------
 1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 81ec8630453b..2f1202b2bc77 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -239,9 +239,15 @@ static int cio2_fbpt_init(struct cio2_device *cio2, struct cio2_queue *q)
 	return 0;
 }
 
-static void cio2_fbpt_exit(struct cio2_queue *q, struct device *dev)
+static int cio2_fbpt_exit(struct cio2_queue *q, struct device *dev)
 {
+	if (!q->fbpt)
+		return -ENOENT;
+
 	dma_free_coherent(dev, CIO2_FBPT_SIZE, q->fbpt, q->fbpt_bus_addr);
+	q->fbpt = NULL;
+
+	return 0;
 }
 
 /**************** CSI2 hardware setup ****************/
@@ -1631,13 +1637,16 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 
 static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q)
 {
-	vb2_video_unregister_device(&q->vdev);
 	media_entity_cleanup(&q->vdev.entity);
-	v4l2_device_unregister_subdev(&q->subdev);
 	media_entity_cleanup(&q->subdev.entity);
-	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
-	mutex_destroy(&q->subdev_lock);
-	mutex_destroy(&q->lock);
+	/*
+	 * Note that not all mutexes may have been initialised, destroy only
+	 * those that have.
+	 */
+	if (!cio2_fbpt_exit(q, &cio2->pci_dev->dev)) {
+		mutex_destroy(&q->subdev_lock);
+		mutex_destroy(&q->lock);
+	}
 }
 
 static int cio2_queues_init(struct cio2_device *cio2)
@@ -1647,16 +1656,10 @@ static int cio2_queues_init(struct cio2_device *cio2)
 	for (i = 0; i < CIO2_QUEUES; i++) {
 		r = cio2_queue_init(cio2, &cio2->queue[i]);
 		if (r)
-			break;
+			return r;
 	}
 
-	if (i == CIO2_QUEUES)
-		return 0;
-
-	for (i--; i >= 0; i--)
-		cio2_queue_exit(cio2, &cio2->queue[i]);
-
-	return r;
+	return 0;
 }
 
 static void cio2_queues_exit(struct cio2_device *cio2)
@@ -1667,6 +1670,22 @@ static void cio2_queues_exit(struct cio2_device *cio2)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
 }
 
+static void cio2_media_release(struct media_device *mdev)
+{
+	struct cio2_device *cio2 =
+		container_of(mdev, struct cio2_device, media_dev);
+
+	cio2_queues_exit(cio2);
+	cio2_fbpt_exit_dummy(cio2);
+	mutex_destroy(&cio2->lock);
+
+	kfree(cio2);
+}
+
+static const struct media_device_ops cio2_mdev_ops = {
+	.release = cio2_media_release,
+};
+
 /**************** PCI interface ****************/
 
 static int cio2_pci_probe(struct pci_dev *pci_dev,
@@ -1685,7 +1704,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	if (r)
 		return r;
 
-	cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
+	cio2 = kzalloc(sizeof(*cio2), GFP_KERNEL);
 	if (!cio2)
 		return -ENOMEM;
 	cio2->pci_dev = pci_dev;
@@ -1730,6 +1749,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	mutex_init(&cio2->lock);
 
 	cio2->media_dev.dev = dev;
+	cio2->media_dev.ops = &cio2_mdev_ops;
 	strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME,
 		sizeof(cio2->media_dev.model));
 	cio2->media_dev.hw_revision = 0;
@@ -1737,7 +1757,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	media_device_init(&cio2->media_dev);
 	r = media_device_register(&cio2->media_dev);
 	if (r < 0)
-		goto fail_mutex_destroy;
+		goto fail_media_device_put;
 
 	cio2->v4l2_dev.mdev = &cio2->media_dev;
 	r = v4l2_device_register(dev, &cio2->v4l2_dev);
@@ -1772,34 +1792,36 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 fail_clean_notifier:
 	v4l2_async_nf_unregister(&cio2->notifier);
 	v4l2_async_nf_cleanup(&cio2->notifier);
-	cio2_queues_exit(cio2);
+
 fail_v4l2_device_unregister:
 	v4l2_device_unregister(&cio2->v4l2_dev);
+
 fail_media_device_unregister:
 	media_device_unregister(&cio2->media_dev);
-	media_device_cleanup(&cio2->media_dev);
-fail_mutex_destroy:
-	mutex_destroy(&cio2->lock);
-	cio2_fbpt_exit_dummy(cio2);
 
+fail_media_device_put:
+	media_device_put(&cio2->media_dev);
 	return r;
 }
 
 static void cio2_pci_remove(struct pci_dev *pci_dev)
 {
 	struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
+	unsigned int i;
 
 	media_device_unregister(&cio2->media_dev);
 	v4l2_async_nf_unregister(&cio2->notifier);
 	v4l2_async_nf_cleanup(&cio2->notifier);
-	cio2_queues_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
+	for (i = 0; i < CIO2_QUEUES; i++) {
+		vb2_video_unregister_device(&cio2->queue[i].vdev);
+		v4l2_device_unregister_subdev(&cio2->queue[i].subdev);
+	}
 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_cleanup(&cio2->media_dev);
-	mutex_destroy(&cio2->lock);
 
 	pm_runtime_forbid(&pci_dev->dev);
 	pm_runtime_get_noresume(&pci_dev->dev);
+
+	media_device_put(&cio2->media_dev);
 }
 
 static int __maybe_unused cio2_runtime_suspend(struct device *dev)
-- 
2.39.2

[PATCH v4 19/26] media: vimc: Release resources on media device release

[Sakari Ailus]

Release all the resources when the media device is released, moving away
from the struct v4l2_device used for that purpose. This is done to
exemplify the use of the media device's release callback.

Switch to container_of_const(), too, while we're changing the code anyway.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
index af127476e920..3e59f8c256c7 100644
--- a/drivers/media/test-drivers/vimc/vimc-core.c
+++ b/drivers/media/test-drivers/vimc/vimc-core.c
@@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
 	return 0;
 }
 
-static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+static void vimc_mdev_release(struct media_device *mdev)
 {
 	struct vimc_device *vimc =
-		container_of(v4l2_dev, struct vimc_device, v4l2_dev);
+		container_of_const(mdev, struct vimc_device, mdev);
 
 	vimc_release_subdevs(vimc);
-	media_device_cleanup(&vimc->mdev);
 	kfree(vimc->ent_devs);
 	kfree(vimc);
 }
@@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc)
 	return ret;
 }
 
+static const struct media_device_ops vimc_mdev_ops = {
+	.release = vimc_mdev_release,
+};
+
 static int vimc_probe(struct platform_device *pdev)
 {
 	const struct font_desc *font = find_font("VGA8x16");
@@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev)
 	snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info),
 		 "platform:%s", VIMC_PDEV_NAME);
 	vimc->mdev.dev = &pdev->dev;
+	vimc->mdev.ops = &vimc_mdev_ops;
 	media_device_init(&vimc->mdev);
 
 	ret = vimc_register_devices(vimc);
 	if (ret) {
-		media_device_cleanup(&vimc->mdev);
-		kfree(vimc);
+		media_device_put(&vimc->mdev);
 		return ret;
 	}
 	/*
@@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev)
 	 * if the registration fails, we release directly from probe
 	 */
 
-	vimc->v4l2_dev.release = vimc_v4l2_dev_release;
 	platform_set_drvdata(pdev, vimc);
 	return 0;
 }
@@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev)
 	media_device_unregister(&vimc->mdev);
 	v4l2_device_unregister(&vimc->v4l2_dev);
 	v4l2_device_put(&vimc->v4l2_dev);
+	media_device_put(&vimc->mdev);
 }
 
 static void vimc_dev_release(struct device *dev)
-- 
2.39.2

[PATCH v4 20/26] media: Documentation: Document how Media device resources are released

[Sakari Ailus]

Document that after unregistering, Media device memory resources are
released by the release() callback rather than by calling
media_device_cleanup().

Also add that driver memory resources should be bound to the Media device,
not V4L2 device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++--
 include/media/media-device.h               |  6 ++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst
index 2456950ce8ff..f9108f14d1ed 100644
--- a/Documentation/driver-api/media/mc-core.rst
+++ b/Documentation/driver-api/media/mc-core.rst
@@ -46,13 +46,27 @@ Drivers initialise media device instances by calling
 :c:func:`media_device_init()`. After initialising a media device instance, it is
 registered by calling :c:func:`__media_device_register()` via the macro
 ``media_device_register()`` and unregistered by calling
-:c:func:`media_device_unregister()`. An initialised media device must be
-eventually cleaned up by calling :c:func:`media_device_cleanup()`.
+:c:func:`media_device_unregister()`. The resources of a newly unregistered media
+device will be released by the ``release()`` callback of :c:type:`media_device`
+ops, which will be called when the last user of the media device has released it
+calling :c:func:`media_device_put()`.
+
+The ``release()`` callback is the way all the resources of the media device are
+released once :c:func:`media_device_init()` has been called. This is also
+relevant during device driver's probe function as the ``release()`` callback
+will also have to be able to safely release the resources related to a partially
+initialised media device.
 
 Note that it is not allowed to unregister a media device instance that was not
 previously registered, or clean up a media device instance that was not
 previously initialised.
 
+Media device and driver's per-device context
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Drivers should use the struct media_device_ops ``release()`` callback to release
+their own resources and not e.g. that of the struct v4l2_device.
+
 Entities
 ^^^^^^^^
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index f1afbfc4dca2..fe4625f3f62b 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -252,8 +252,10 @@ static inline void media_device_put(struct media_device *mdev)
  *
  * @mdev:	pointer to struct &media_device
  *
- * This function that will destroy the graph_mutex that is
- * initialized in media_device_init().
+ * This function that will destroy the graph_mutex that is initialized in
+ * media_device_init(). Note that *only* drivers that do not manage releasing
+ * the memory of th media device itself call this function. This function is
+ * thus effectively DEPRECATED.
  */
 void media_device_cleanup(struct media_device *mdev);
 
-- 
2.39.2

[PATCH v4 21/26] media: mc: Add per-file-handle data support

[Sakari Ailus]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The media devnode core associates devnodes with files by storing the
devnode pointer in the file structure private_data field. In order to
allow tracking of per-file-handle data introduce a new media devnode
file handle structure that stores the devnode pointer, and store a
pointer to that structure in the file private_data field.

Users of the media devnode code (the only existing user being
media_device) are responsible for managing their own subclass of the
media_devnode_fh structure.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Prepare struct media_device_fh to be used for maintaining file handle list
to avoid shuffling things here and there right after.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c  | 14 +++++++++++++-
 drivers/media/mc/mc-devnode.c | 20 +++++++++-----------
 include/media/media-device.h  |  7 +++++++
 include/media/media-devnode.h | 18 +++++++++++++++++-
 include/media/media-fh.h      | 32 ++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 13 deletions(-)
 create mode 100644 include/media/media-fh.h

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index f1a88edb7573..a9505ab4412d 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -22,6 +22,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-fh.h>
 #include <media/media-request.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
@@ -35,7 +36,6 @@
 #define MEDIA_ENT_SUBTYPE_MASK			0x0000ffff
 #define MEDIA_ENT_T_DEVNODE_UNKNOWN		(MEDIA_ENT_F_OLD_BASE | \
 						 MEDIA_ENT_SUBTYPE_MASK)
-
 /* -----------------------------------------------------------------------------
  * Userspace API
  */
@@ -47,11 +47,23 @@ static inline void __user *media_get_uptr(__u64 arg)
 
 static int media_device_open(struct file *filp)
 {
+	struct media_device_fh *fh;
+
+	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+	if (!fh)
+		return -ENOMEM;
+
+	filp->private_data = &fh->fh;
+
 	return 0;
 }
 
 static int media_device_close(struct file *filp)
 {
+	struct media_device_fh *fh = media_device_fh(filp);
+
+	kfree(fh);
+
 	return 0;
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index f27d5d288a38..26491daaba96 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -133,6 +133,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 static int media_open(struct inode *inode, struct file *filp)
 {
 	struct media_devnode *devnode;
+	struct media_devnode_fh *fh;
 	int ret;
 
 	/* Check if the media device is available. This needs to be done with
@@ -153,17 +154,15 @@ static int media_open(struct inode *inode, struct file *filp)
 	get_device(&devnode->dev);
 	mutex_unlock(&media_devnode_lock);
 
-	filp->private_data = devnode;
-
-	if (devnode->fops->open) {
-		ret = devnode->fops->open(filp);
-		if (ret) {
-			put_device(&devnode->dev);
-			filp->private_data = NULL;
-			return ret;
-		}
+	ret = devnode->fops->open(filp);
+	if (ret) {
+		put_device(&devnode->dev);
+		return ret;
 	}
 
+	fh = filp->private_data;
+	fh->devnode = devnode;
+
 	return 0;
 }
 
@@ -172,8 +171,7 @@ static int media_release(struct inode *inode, struct file *filp)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (devnode->fops->release)
-		devnode->fops->release(filp);
+	devnode->fops->release(filp);
 
 	filp->private_data = NULL;
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index fe4625f3f62b..f9f7c37e7d57 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -110,6 +110,10 @@ struct media_device_ops {
  *		     other operations that stop or start streaming.
  * @request_id: Used to generate unique request IDs
  *
+ * @fh_list:	List of file handles in the media device
+ *		(struct media_device_fh.mdev_list).
+ * @fh_list_lock: Serialise access to fh_list list.
+ *
  * This structure represents an abstract high-level media device. It allows easy
  * access to entities and provides basic media device-level support. The
  * structure can be allocated directly or embedded in a larger structure.
@@ -182,6 +186,9 @@ struct media_device {
 
 	struct mutex req_queue_mutex;
 	atomic_t request_id;
+
+	struct list_head fh_list;
+	spinlock_t fh_list_lock;
 };
 
 /* We don't need to include usb.h here */
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 113c317e6a0e..e4e8552598eb 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -53,6 +53,20 @@ struct media_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * struct media_devnode_fh - Media device node file handle
+ * @devnode:	pointer to the media device node
+ *
+ * This structure serves as a base for per-file-handle data storage. Media
+ * device node users embed media_devnode_fh in their custom file handle data
+ * structures and store the media_devnode_fh in the file private_data in order
+ * to let the media device node core locate the media_devnode corresponding to a
+ * file handle.
+ */
+struct media_devnode_fh {
+	struct media_devnode *devnode;
+};
+
 /**
  * struct media_devnode - Media device node
  * @fops:	pointer to struct &media_file_operations with media device ops
@@ -136,7 +150,9 @@ void media_devnode_unregister(struct media_devnode *devnode);
  */
 static inline struct media_devnode *media_devnode_data(struct file *filp)
 {
-	return filp->private_data;
+	struct media_devnode_fh *fh = filp->private_data;
+
+	return fh->devnode;
 }
 
 /**
diff --git a/include/media/media-fh.h b/include/media/media-fh.h
new file mode 100644
index 000000000000..6f00744b81d6
--- /dev/null
+++ b/include/media/media-fh.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Media device file handle
+ *
+ * Copyright (C) 2019--2023 Intel Corporation
+ */
+
+#ifndef MEDIA_FH_H
+#define MEDIA_FH_H
+
+#include <linux/list.h>
+#include <linux/file.h>
+
+#include <media/media-devnode.h>
+
+/**
+ * struct media_device_fh - File handle specific information on MC
+ *
+ * @fh: The media device file handle
+ * @mdev_list: This file handle in media device's list of file handles
+ */
+struct media_device_fh {
+	struct media_devnode_fh fh;
+	struct list_head mdev_list;
+};
+
+static inline struct media_device_fh *media_device_fh(struct file *filp)
+{
+	return container_of(filp->private_data, struct media_device_fh, fh);
+}
+
+#endif /* MEDIA_FH_H */
-- 
2.39.2

[PATCH v4 22/26] media: mc: Maintain a list of open file handles in a media device

[Sakari Ailus]

The list of file handles is needed to deliver media events as well as for
other purposes in the future.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 19 ++++++++++++++++++-
 drivers/media/mc/mc-devnode.c |  2 +-
 include/media/media-devnode.h |  4 +++-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index a9505ab4412d..46d1b0c9d8be 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -45,8 +45,9 @@ static inline void __user *media_get_uptr(__u64 arg)
 	return (void __user *)(uintptr_t)arg;
 }
 
-static int media_device_open(struct file *filp)
+static int media_device_open(struct media_devnode *devnode, struct file *filp)
 {
+	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh;
 
 	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
@@ -55,13 +56,23 @@ static int media_device_open(struct file *filp)
 
 	filp->private_data = &fh->fh;
 
+	spin_lock_irq(&mdev->fh_list_lock);
+	list_add(&fh->mdev_list, &mdev->fh_list);
+	spin_unlock_irq(&mdev->fh_list_lock);
+
 	return 0;
 }
 
 static int media_device_close(struct file *filp)
 {
+	struct media_devnode *devnode = media_devnode_data(filp);
+	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh = media_device_fh(filp);
 
+	spin_lock_irq(&mdev->fh_list_lock);
+	list_del(&fh->mdev_list);
+	spin_unlock_irq(&mdev->fh_list_lock);
+
 	kfree(fh);
 
 	return 0;
@@ -769,11 +780,13 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->pads);
 	INIT_LIST_HEAD(&mdev->links);
 	INIT_LIST_HEAD(&mdev->entity_notify);
+	INIT_LIST_HEAD(&mdev->fh_list);
 
 	mutex_init(&mdev->req_queue_mutex);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 	atomic_set(&mdev->request_id, 0);
+	spin_lock_init(&mdev->fh_list_lock);
 
 	mdev->devnode.release = media_device_release;
 	media_devnode_init(&mdev->devnode);
@@ -830,6 +843,10 @@ void media_device_unregister(struct media_device *mdev)
 	if (!media_devnode_is_registered(&mdev->devnode))
 		return;
 
+	spin_lock_irq(&mdev->fh_list_lock);
+	list_del_init(&mdev->fh_list);
+	spin_unlock_irq(&mdev->fh_list_lock);
+
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 26491daaba96..617156963911 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -154,7 +154,7 @@ static int media_open(struct inode *inode, struct file *filp)
 	get_device(&devnode->dev);
 	mutex_unlock(&media_devnode_lock);
 
-	ret = devnode->fops->open(filp);
+	ret = devnode->fops->open(devnode, filp);
 	if (ret) {
 		put_device(&devnode->dev);
 		return ret;
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index e4e8552598eb..898fa67ca090 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -21,6 +21,8 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
+struct media_devnode;
+
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
@@ -49,7 +51,7 @@ struct media_file_operations {
 	__poll_t (*poll) (struct file *, struct poll_table_struct *);
 	long (*ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
-	int (*open) (struct file *);
+	int (*open) (struct media_devnode *, struct file *);
 	int (*release) (struct file *);
 };
 
-- 
2.39.2

[PATCH v4 23/26] media: mc: Implement best effort media device removal safety sans refcount

[Sakari Ailus]

Add a new helper data structures media_devnode_compat_ref and
media_devnode_cdev. The latter is used to prevent user space from calling
IOCTLs or other system calls to the media device that has been already
unregistered and the former to help with obtaining the container struct
(either media_devnode_compat_ref or media_devnode) in media_open().

The media device's memory may of course still be released during the call
but there is only so much that can be done to this without the driver
managing the lifetime of the resources it needs somehow.

This patch should be reverted once all drivers have been converted to manage
their resources' lifetime.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c       |  49 +++++++++---
 drivers/media/mc/mc-devnode.c      | 118 ++++++++++++++++++++++-------
 drivers/media/v4l2-core/v4l2-dev.c |  26 +++++--
 include/media/media-device.h       |   8 +-
 include/media/media-devnode.h      |  65 ++++++++++++++--
 5 files changed, 210 insertions(+), 56 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 46d1b0c9d8be..8cdd0d46e865 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -54,6 +54,8 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
 	if (!fh)
 		return -ENOMEM;
 
+	fh->fh.ref = devnode->ref;
+
 	filp->private_data = &fh->fh;
 
 	spin_lock_irq(&mdev->fh_list_lock);
@@ -65,13 +67,16 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
 
 static int media_device_close(struct file *filp)
 {
-	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh = media_device_fh(filp);
 
-	spin_lock_irq(&mdev->fh_list_lock);
-	list_del(&fh->mdev_list);
-	spin_unlock_irq(&mdev->fh_list_lock);
+	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
+		struct media_devnode *devnode = media_devnode_data(filp);
+		struct media_device *mdev = to_media_device(devnode);
+
+		spin_lock_irq(&mdev->fh_list_lock);
+		list_del(&fh->mdev_list);
+		spin_unlock_irq(&mdev->fh_list_lock);
+	}
 
 	kfree(fh);
 
@@ -810,28 +815,45 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
+	struct media_devnode_compat_ref *ref = NULL;
 	int ret;
 
+	if (!mdev->ops || !mdev->ops->release) {
+		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
+		if (!ref)
+			return -ENOMEM;
+	}
+
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
+	mdev->devnode.ref = ref;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		return ret;
+		goto out_put_ref;
+
+	ret = device_create_file(media_devnode_dev(&mdev->devnode),
+				 &dev_attr_model);
+	if (ret < 0)
+		goto out_devnode_unregister;
 
-	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0) {
-		media_devnode_unregister(&mdev->devnode);
-		return ret;
-	}
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+out_devnode_unregister:
+	media_devnode_unregister(&mdev->devnode);
+
+out_put_ref:
+	if (ref)
+		put_device(&ref->dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -847,9 +869,12 @@ void media_device_unregister(struct media_device *mdev)
 	list_del_init(&mdev->fh_list);
 	spin_unlock_irq(&mdev->fh_list_lock);
 
-	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	device_remove_file(media_devnode_dev(&mdev->devnode), &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
+
+	if (mdev->devnode.ref)
+		put_device(&mdev->devnode.ref->dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 617156963911..8cb4e0eec17f 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -49,24 +49,52 @@ static void media_devnode_release(struct device *cd)
 {
 	struct media_devnode *devnode = to_media_devnode(cd);
 
+	/* If the devnode has a ref, it is simply released by the user. */
+	if (devnode->ref)
+		return;
+
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
 }
 
+static void media_devnode_ref_release(struct device *cd)
+{
+	struct media_devnode_compat_ref *ref =
+		container_of_const(cd, struct media_devnode_compat_ref, dev);
+
+	kfree(ref);
+}
+
+struct media_devnode *to_media_devnode(struct device *dev)
+{
+	if (dev->release == media_devnode_release)
+		return container_of(dev, struct media_devnode, dev);
+
+	return container_of(dev, struct media_devnode_compat_ref, dev)->devnode;
+}
+
 static const struct bus_type media_bus_type = {
 	.name = MEDIA_NAME,
 };
 
+static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
+{
+	if (fh->ref)
+		return atomic_read(&fh->ref->registered);
+
+	return media_devnode_is_registered(fh->devnode);
+}
+
 static ssize_t media_read(struct file *filp, char __user *buf,
 		size_t sz, loff_t *off)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
+	if (!media_devnode_is_registered_compat(filp->private_data))
+		return -EIO;
 	if (!devnode->fops->read)
 		return -EINVAL;
-	if (!media_devnode_is_registered(devnode))
-		return -EIO;
 	return devnode->fops->read(filp, buf, sz, off);
 }
 
@@ -75,10 +103,10 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
+	if (!media_devnode_is_registered_compat(filp->private_data))
+		return -EIO;
 	if (!devnode->fops->write)
 		return -EINVAL;
-	if (!media_devnode_is_registered(devnode))
-		return -EIO;
 	return devnode->fops->write(filp, buf, sz, off);
 }
 
@@ -87,7 +115,7 @@ static __poll_t media_poll(struct file *filp,
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (!media_devnode_is_registered(devnode))
+	if (!media_devnode_is_registered_compat(filp->private_data))
 		return EPOLLERR | EPOLLHUP;
 	if (!devnode->fops->poll)
 		return DEFAULT_POLLMASK;
@@ -99,14 +127,9 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
 	      long (*ioctl_func)(struct file *filp, unsigned int cmd,
 				 unsigned long arg))
 {
-	struct media_devnode *devnode = media_devnode_data(filp);
-
 	if (!ioctl_func)
 		return -ENOTTY;
 
-	if (!media_devnode_is_registered(devnode))
-		return -EIO;
-
 	return ioctl_func(filp, cmd, arg);
 }
 
@@ -114,6 +137,9 @@ static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
+	if (!media_devnode_is_registered_compat(filp->private_data))
+		return -EIO;
+
 	return __media_ioctl(filp, cmd, arg, devnode->fops->ioctl);
 }
 
@@ -124,6 +150,9 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
+	if (!media_devnode_is_registered_compat(filp->private_data))
+		return -EIO;
+
 	return __media_ioctl(filp, cmd, arg, devnode->fops->compat_ioctl);
 }
 
@@ -132,6 +161,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 /* Override for the open function */
 static int media_open(struct inode *inode, struct file *filp)
 {
+	struct media_devnode_cdev *mcdev;
 	struct media_devnode *devnode;
 	struct media_devnode_fh *fh;
 	int ret;
@@ -143,7 +173,12 @@ static int media_open(struct inode *inode, struct file *filp)
 	 * a crash.
 	 */
 	mutex_lock(&media_devnode_lock);
-	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
+	mcdev = container_of(inode->i_cdev, struct media_devnode_cdev, cdev);
+	if (mcdev->is_compat_ref)
+		devnode = container_of(mcdev, struct media_devnode_compat_ref,
+				       mcdev)->devnode;
+	else
+		devnode = container_of(mcdev, struct media_devnode, mcdev);
 	/* return ENXIO if the media device has been removed
 	   already or if it is not registered anymore. */
 	if (!media_devnode_is_registered(devnode)) {
@@ -151,12 +186,12 @@ static int media_open(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	}
 	/* and increase the device refcount */
-	get_device(&devnode->dev);
+	get_device(media_devnode_dev(devnode));
 	mutex_unlock(&media_devnode_lock);
 
 	ret = devnode->fops->open(devnode, filp);
 	if (ret) {
-		put_device(&devnode->dev);
+		put_device(media_devnode_dev(devnode));
 		return ret;
 	}
 
@@ -169,15 +204,21 @@ static int media_open(struct inode *inode, struct file *filp)
 /* Override for the release function */
 static int media_release(struct inode *inode, struct file *filp)
 {
-	struct media_devnode *devnode = media_devnode_data(filp);
-
-	devnode->fops->release(filp);
+	struct media_devnode_fh *fh = filp->private_data;
+	struct device *dev;
+
+	if (!fh->ref) {
+		dev = &fh->devnode->dev;
+		fh->devnode->fops->release(filp);
+	} else {
+		dev = &fh->ref->dev;
+		fh->ref->release(filp);
+	}
 
 	filp->private_data = NULL;
 
-	/* decrease the refcount unconditionally since the release()
-	   return value is ignored. */
-	put_device(&devnode->dev);
+	put_device(dev);
+
 	return 0;
 }
 
@@ -204,6 +245,9 @@ void media_devnode_init(struct media_devnode *devnode)
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
+	struct media_devnode_compat_ref *ref = devnode->ref;
+	struct cdev *cdev;
+	struct device *dev;
 	int minor;
 	int ret;
 
@@ -222,19 +266,31 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	devnode->minor = minor;
 
 	/* Part 2: Initialize the media and character devices */
-	cdev_init(&devnode->cdev, &media_devnode_fops);
-	devnode->cdev.owner = owner;
-	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
-
-	devnode->dev.bus = &media_bus_type;
-	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	cdev = ref ? &ref->mcdev.cdev : &devnode->mcdev.cdev;
+	cdev_init(cdev, &media_devnode_fops);
+	cdev->owner = owner;
+	kobject_set_name(&cdev->kobj, "media%d", devnode->minor);
+
+	if (!ref) {
+		dev = &devnode->dev;
+	} else {
+		ref->mcdev.is_compat_ref = true;
+		device_initialize(&ref->dev);
+		atomic_set(&ref->registered, 1);
+		ref->devnode = devnode;
+		ref->release = devnode->fops->release;
+		dev = &ref->dev;
+		dev->release = media_devnode_ref_release;
+	}
+	dev->bus = &media_bus_type;
+	dev->devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
 	if (devnode->parent)
-		devnode->dev.parent = devnode->parent;
-	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+		dev->parent = devnode->parent;
+	dev_set_name(dev, "media%d", devnode->minor);
 
 	/* Part 3: Add the media and character devices */
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
+	ret = cdev_device_add(cdev, dev);
 	if (ret < 0) {
 		pr_err("%s: cdev_device_add failed\n", __func__);
 		goto cdev_add_error;
@@ -256,11 +312,15 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	if (!media_devnode_is_registered(devnode))
 		return;
 
+	if (devnode->ref)
+		atomic_set(&devnode->ref->registered, 0);
+
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 
-	cdev_device_del(&devnode->cdev, &devnode->dev);
+	cdev_device_del(devnode->ref ? &devnode->ref->mcdev.cdev :
+			&devnode->mcdev.cdev, media_devnode_dev(devnode));
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(devnode->minor, media_devnode_nums);
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 4bf4398fd2fe..2b19a845c3a4 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -179,7 +179,7 @@ static void v4l2_device_release(struct device *cd)
 	bool v4l2_dev_call_release = v4l2_dev->release;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev = v4l2_dev->mdev;
-	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
+	bool mdev_has_ref = mdev && mdev->devnode.ref;
 #endif
 
 	mutex_lock(&videodev_lock);
@@ -212,12 +212,24 @@ static void v4l2_device_release(struct device *cd)
 	}
 #endif
 
-	/* Release video_device and perform other
-	   cleanups as needed. */
+	/*
+	 * Put struct media_devnode_compat_ref here as indicated by
+	 * mdev_has_ref. mdev may be released by vdev->release() below.
+	 */
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev && mdev_has_ref)
+		media_device_put(mdev);
+#endif
+
+	/* Release video_device and perform other cleanups as needed. */
 	vdev->release(vdev);
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (mdev)
+	/*
+	 * Put a reference to struct media_device acquired in
+	 * video_register_media_controller().
+	 */
+	if (mdev && !mdev_has_ref)
 		media_device_put(mdev);
 
 	/*
@@ -225,13 +237,15 @@ static void v4l2_device_release(struct device *cd)
 	 * embedded in the same driver's context struct so having a release
 	 * callback in both is a bug.
 	 */
-	if (WARN_ON(v4l2_dev_call_release && mdev_has_release))
+	if (WARN_ON(v4l2_dev_call_release && !mdev_has_ref))
 		v4l2_dev_call_release = false;
 #endif
 
 	/*
 	 * Decrease v4l2_device refcount, but only if the media device doesn't
-	 * have a release callback.
+	 * have a release callback. Otherwise one could expect accessing
+	 * released memory --- driver's context struct refcounted already via
+	 * struct media_device.
 	 */
 	if (v4l2_dev_call_release)
 		v4l2_device_put(v4l2_dev);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index f9f7c37e7d57..30f9b78d1ce7 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -259,10 +259,10 @@ static inline void media_device_put(struct media_device *mdev)
  *
  * @mdev:	pointer to struct &media_device
  *
- * This function that will destroy the graph_mutex that is initialized in
- * media_device_init(). Note that *only* drivers that do not manage releasing
- * the memory of th media device itself call this function. This function is
- * thus effectively DEPRECATED.
+ * This function will destroy the graph_mutex that is initialized in
+ * media_device_init(). Note that only drivers that do not have a proper release
+ * callback of the struct media_device call this function. This function is thus
+ * effectively DEPRECATED.
  */
 void media_device_cleanup(struct media_device *mdev);
 
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 898fa67ca090..5dee1acbc3f7 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -41,8 +41,7 @@ struct media_devnode;
  * @compat_ioctl: pointer to the function that will handle 32 bits userspace
  *	calls to the ioctl() syscall on a Kernel compiled with 64 bits.
  * @open: pointer to the function that implements open() syscall
- * @release: pointer to the function that will release the resources allocated
- *	by the @open function.
+ * @release: pointer to the function that implements release() syscall
  */
 struct media_file_operations {
 	struct module *owner;
@@ -55,9 +54,54 @@ struct media_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * struct media_devnode_cdev - Workaround for drivers not managing media device
+ *			       lifetime - character device
+ *
+ * Store the characeter device and whether this is a compatibility reference or
+ * not. struct media_devnode_cdev is embedded in either struct
+ * media_devnode_compat_ref or struct media_devnode.
+ *
+ * @cdev: the chracter device
+ * @is_compat_ref: Is this a compatibility reference or not?
+ */
+struct media_devnode_cdev {
+	struct cdev cdev;
+	bool is_compat_ref;
+};
+
+/**
+ * struct media_devnode_compat_ref - Workaround for drivers not managing media
+ *				     device lifetime - reference
+ *
+ * The purpose if this struct is to support drivers that do not manage the
+ * lifetime of their respective media devices to avoid the worst effects of
+ * this, namely an IOCTL call on an open file handle to a device that has been
+ * unbound causing a kernel oops systematically. This is not a fix. The proper,
+ * reliable way to handle this is to manage the resources used by the
+ * driver. This struct and its use can be removed once all drivers have been
+ * converted.
+ *
+ * @dev: struct device that remains in place as long as any reference remains
+ * @mcdev: the related character device
+ * @devnode: a pointer back to the devnode
+ * @release:	pointer to the function that will release the resources
+ *		allocated by the @open function.
+ * @registered: is this device registered?
+ */
+struct media_devnode_compat_ref {
+	struct device dev;
+	struct media_devnode_cdev mcdev;
+	struct media_devnode *devnode;
+	int (*release)(struct file *);
+	atomic_t registered;
+};
+
 /**
  * struct media_devnode_fh - Media device node file handle
  * @devnode:	pointer to the media device node
+ * @ref:	media device compat ref, if the driver does not manage media
+ *		device lifetime
  *
  * This structure serves as a base for per-file-handle data storage. Media
  * device node users embed media_devnode_fh in their custom file handle data
@@ -67,18 +111,21 @@ struct media_file_operations {
  */
 struct media_devnode_fh {
 	struct media_devnode *devnode;
+	struct media_devnode_compat_ref *ref;
 };
 
 /**
  * struct media_devnode - Media device node
  * @fops:	pointer to struct &media_file_operations with media device ops
  * @dev:	pointer to struct &device containing the media controller device
- * @cdev:	struct cdev pointer character device
+ * @mcdev:	related to the character device
  * @parent:	parent device
  * @minor:	device node minor number
  * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
  * @release:	release callback called at the end of ``media_devnode_release()``
  *		routine at media-device.c.
+ * @ref:	reference for providing best effort memory safety in device
+ *		removal
  *
  * This structure represents a media-related device node.
  *
@@ -91,7 +138,7 @@ struct media_devnode {
 
 	/* sysfs */
 	struct device dev;		/* media device */
-	struct cdev cdev;		/* character device */
+	struct media_devnode_cdev mcdev; /* character device + compat status */
 	struct device *parent;		/* device parent */
 
 	/* device info */
@@ -100,10 +147,18 @@ struct media_devnode {
 
 	/* callbacks */
 	void (*release)(struct media_devnode *devnode);
+
+	/* compat reference */
+	struct media_devnode_compat_ref *ref;
 };
 
+static inline struct device *media_devnode_dev(struct media_devnode *devnode)
+{
+	return devnode->ref ? &devnode->ref->dev : &devnode->dev;
+}
+
 /* dev to media_devnode */
-#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
+struct media_devnode *to_media_devnode(struct device *dev);
 
 /**
  * media_devnode_init - initialise a media devnode
-- 
2.39.2

[PATCH v4 24/26] media: mc: Warn about drivers not releasing media device safely

[Sakari Ailus]

The media device and associated resources may be released only when its
memory is no longer used. Warn about drivers not doing this, but instead
releasing the resources at driver unbind time.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 8cdd0d46e865..51836faa6d1a 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -822,6 +822,9 @@ int __must_check __media_device_register(struct media_device *mdev,
 		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
 		if (!ref)
 			return -ENOMEM;
+
+		dev_warn(mdev->dev,
+			 "Set mdev release op to safely release resources!\n");
 	}
 
 	/* Register the device node. */
-- 
2.39.2

[PATCH v4 25/26] media: mc: Enforce one-time registration

[Sakari Ailus]

A media devnode may be registered only once. Enforce this by setting the
minor to -1 in init.

Registration initialises the character device and sets up the device name.
These should take place only once during the lifetime of the media device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 8cb4e0eec17f..758971f310c3 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -240,6 +240,7 @@ void media_devnode_init(struct media_devnode *devnode)
 {
 	device_initialize(&devnode->dev);
 	devnode->dev.release = media_devnode_release;
+	devnode->minor = -1;
 }
 
 int __must_check media_devnode_register(struct media_devnode *devnode,
@@ -251,6 +252,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	int minor;
 	int ret;
 
+	if (devnode->minor != -1)
+		return -EINVAL;
+
 	/* Part 1: Find a free minor number */
 	mutex_lock(&media_devnode_lock);
 	minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES);
-- 
2.39.2

[PATCH v4 26/26] media: Documentation: Document media device memory safety helper

[Sakari Ailus]

Document how the best effort memory safety helper for accessing media
device works, and that drivers should be converted to refcount the media
device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 758971f310c3..4bc0c03844e5 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -243,6 +243,32 @@ void media_devnode_init(struct media_devnode *devnode)
 	devnode->minor = -1;
 }
 
+/*
+ * Best effort media device lifetime management for old drivers
+ *
+ * Drivers that do not manage the lifetime of the media device are provided with
+ * a best effort lifetime management support. This means that as the driver does
+ * not release the media device once all users are gone but when the device is
+ * unbound, there are bound to be (brief) moments when released memory may get
+ * accessed. All drivers should be converted to release their memory at a safe
+ * time, i.e. provide a release callback in struct media_file_operations to do
+ * so. This is especially important for drivers for devices that are
+ * unpluggable, e.g. USB devices.
+ *
+ * A second struct device is used to manage the lifetime of a helper object,
+ * struct media_devnode_compat_ref. For a media device, one is initialised in
+ * media_devnode_register and put in media_devnode_unregister. This object is
+ * also used as the device of the media character device so file handles to the
+ * media device have a reference to this object. When the media device is
+ * released, any file handle retains a reference to this helper that also
+ * contains the media device's registration status. If a media device is
+ * released and a user space process attempts to access the file handle, an
+ * error is returned.
+ *
+ * The struct device in struct media_devnode is put at media_device_cleanup and
+ * uses an empty release callback, reflecting the expectation the driver will
+ * release the memory of the media device at unbind time.
+ */
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
-- 
2.39.2

Re: [PATCH v4 07/26] media: mc: Drop media_dev description from struct media_devnode

[Hans Verkuil]

This needs a proper commit log.

Specifically it should mention that due to the revert in patch 05/26
the media_dev field was deleted, but that that revert didn't remove it
from the kerneldoc comments.

I would also suggest that you move this patch to 06/26 so that it
comes right after the 05/26 revert, since the two really belong together.

With that, you can add my:

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

Regards,

	Hans

On 10/06/2024 12:05, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/media-devnode.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index 1117d1dfd6bf..024dfb98a6fd 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -55,7 +55,6 @@ struct media_file_operations {
>  
>  /**
>   * struct media_devnode - Media device node
> - * @media_dev:	pointer to struct &media_device
>   * @fops:	pointer to struct &media_file_operations with media device ops
>   * @dev:	pointer to struct &device containing the media controller device
>   * @cdev:	struct cdev pointer character device

Re: [PATCH v4 08/26] media: mc: Do not call cdev_device_del() if cdev_device_add() fails

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> cdev_device_del() is the right function to remove a device when
> cdev_device_add() succeeds. If it does not, however, put_device() needs to
> be used instead. Fix this.

Hmm, this too is due to a revert patch (03/26) removing something that needs
to be reinstated.

Wouldn't it be better to fold this into 04/26, with a comment in the commit
log of that commit?

The problem with this commit log is that it suggests that it fixes a bug,
when really it just corrects something introduced by a revert.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/mc/mc-devnode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 2e33c2007f08..707593d127a7 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -252,9 +252,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>  
>  cdev_add_error:
>  	mutex_lock(&media_devnode_lock);
> -	cdev_device_del(&devnode->cdev, &devnode->dev);
>  	clear_bit(devnode->minor, media_devnode_nums);
>  	mutex_unlock(&media_devnode_lock);
> +	put_device(&devnode->dev);
>  
>  	return ret;
>  }

Re: [PATCH v4 15/26] media: v4l: Acquire a reference to the media device for every video device

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> The video device depends on the existence of its media device --- if there
> is one. Acquire a reference to it.
> 
> Note that when the media device release callback is used, then the V4L2
> device release callback is ignored and a warning is issued if both are
> set.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 53 ++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index be2ba7ca5de2..4bf4398fd2fe 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd)
>  {
>  	struct video_device *vdev = to_video_device(cd);
>  	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> +	bool v4l2_dev_call_release = v4l2_dev->release;
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct media_device *mdev = v4l2_dev->mdev;
> +	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
> +#endif
>  
>  	mutex_lock(&videodev_lock);
>  	if (WARN_ON(video_devices[vdev->minor] != vdev)) {
> @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd)
>  
>  	mutex_unlock(&videodev_lock);
>  
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	if (mdev && vdev->vfl_dir != VFL_DIR_M2M) {
>  		/* Remove interfaces and interface links */
>  		media_devnode_remove(vdev->intf_devnode);
>  		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
> @@ -207,23 +212,28 @@ static void v4l2_device_release(struct device *cd)
>  	}
>  #endif
>  
> -	/* Do not call v4l2_device_put if there is no release callback set.
> -	 * Drivers that have no v4l2_device release callback might free the
> -	 * v4l2_dev instance in the video_device release callback below, so we
> -	 * must perform this check here.
> -	 *
> -	 * TODO: In the long run all drivers that use v4l2_device should use the
> -	 * v4l2_device release callback. This check will then be unnecessary.
> -	 */
> -	if (v4l2_dev->release == NULL)
> -		v4l2_dev = NULL;
> -
>  	/* Release video_device and perform other
>  	   cleanups as needed. */
>  	vdev->release(vdev);
>  
> -	/* Decrease v4l2_device refcount */
> -	if (v4l2_dev)
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	if (mdev)
> +		media_device_put(mdev);
> +
> +	/*
> +	 * Generally both struct media_device and struct v4l2_device are
> +	 * embedded in the same driver's context struct so having a release
> +	 * callback in both is a bug.
> +	 */
> +	if (WARN_ON(v4l2_dev_call_release && mdev_has_release))
> +		v4l2_dev_call_release = false;
> +#endif
> +
> +	/*
> +	 * Decrease v4l2_device refcount, but only if the media device doesn't
> +	 * have a release callback.
> +	 */
> +	if (v4l2_dev_call_release)
>  		v4l2_device_put(v4l2_dev);
>  }
>  
> @@ -795,11 +805,17 @@ static int video_register_media_controller(struct video_device *vdev)
>  	u32 intf_type;
>  	int ret;
>  
> -	/* Memory-to-memory devices are more complex and use
> -	 * their own function to register its mc entities.
> +	if (!vdev->v4l2_dev->mdev)
> +		return 0;
> +
> +	/*
> +	 * Memory-to-memory devices are more complex and use their own function
> +	 * to register its mc entities.
>  	 */
> -	if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M)
> +	if (vdev->vfl_dir == VFL_DIR_M2M) {
> +		media_device_get(vdev->v4l2_dev->mdev);
>  		return 0;
> +	}
>  
>  	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
>  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
> @@ -878,6 +894,7 @@ static int video_register_media_controller(struct video_device *vdev)
>  
>  	/* FIXME: how to create the other interface links? */
>  
> +	media_device_get(vdev->v4l2_dev->mdev);
>  #endif
>  	return 0;
>  }

Re: [PATCH v4 12/26] media: mc: Shuffle functions around

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> As the call paths of the functions in question will change, move them
> around in anticipation of that. No other changes.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

That should be:

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

Regards,

	Hans

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/mc/mc-device.c | 54 ++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index dd4d589a6701..f1f3addf7932 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -673,6 +673,33 @@ void media_device_unregister_entity(struct media_entity *entity)
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>  
> +void media_device_register_entity_notify(struct media_device *mdev,
> +					struct media_entity_notify *nptr)
> +{
> +	mutex_lock(&mdev->graph_mutex);
> +	list_add_tail(&nptr->list, &mdev->entity_notify);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
> +
> +/*
> + * Note: Should be called with mdev->lock held.
> + */
> +static void __media_device_unregister_entity_notify(struct media_device *mdev,
> +					struct media_entity_notify *nptr)
> +{
> +	list_del(&nptr->list);
> +}
> +
> +void media_device_unregister_entity_notify(struct media_device *mdev,
> +					struct media_entity_notify *nptr)
> +{
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_device_unregister_entity_notify(mdev, nptr);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> +
>  void media_device_init(struct media_device *mdev)
>  {
>  	INIT_LIST_HEAD(&mdev->entities);
> @@ -740,33 +767,6 @@ int __must_check __media_device_register(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> -void media_device_register_entity_notify(struct media_device *mdev,
> -					struct media_entity_notify *nptr)
> -{
> -	mutex_lock(&mdev->graph_mutex);
> -	list_add_tail(&nptr->list, &mdev->entity_notify);
> -	mutex_unlock(&mdev->graph_mutex);
> -}
> -EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
> -
> -/*
> - * Note: Should be called with mdev->lock held.
> - */
> -static void __media_device_unregister_entity_notify(struct media_device *mdev,
> -					struct media_entity_notify *nptr)
> -{
> -	list_del(&nptr->list);
> -}
> -
> -void media_device_unregister_entity_notify(struct media_device *mdev,
> -					struct media_entity_notify *nptr)
> -{
> -	mutex_lock(&mdev->graph_mutex);
> -	__media_device_unregister_entity_notify(mdev, nptr);
> -	mutex_unlock(&mdev->graph_mutex);
> -}
> -EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> -
>  void media_device_unregister(struct media_device *mdev)
>  {
>  	struct media_entity *entity;

Re: [PATCH v4 16/26] media: mc: Postpone graph object removal until free

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> The media device itself will be unregistered based on it being unbound and
> driver's remove callback being called. The graph objects themselves may
> still be in use; rely on the media device release callback to release
> them.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Also please update to <hverkuil-cisco@xs4all.nl>.

Thank you!

	Hans

> ---
>  drivers/media/mc/mc-device.c | 59 ++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index bbc233e726d2..f1a88edb7573 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -702,8 +702,33 @@ EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>  
>  static void __media_device_release(struct media_device *mdev)
>  {
> +	struct media_entity *entity;
> +	struct media_entity *next;
> +	struct media_interface *intf, *tmp_intf;
> +	struct media_entity_notify *notify, *nextp;
> +
>  	dev_dbg(mdev->dev, "Media device released\n");
>  
> +	/* Remove all entities from the media device */
> +	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> +		__media_device_unregister_entity(entity);
> +
> +	/* Remove all entity_notify callbacks from the media device */
> +	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
> +		__media_device_unregister_entity_notify(mdev, notify);
> +
> +	/* Remove all interfaces from the media device */
> +	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> +				 graph_obj.list) {
> +		/*
> +		 * Unlink the interface, but don't free it here; the
> +		 * module which created it is responsible for freeing
> +		 * it
> +		 */
> +		__media_remove_intf_links(intf);
> +		media_gobj_destroy(&intf->graph_obj);
> +	}
> +
>  	ida_destroy(&mdev->entity_internal_idx);
>  	mdev->entity_internal_idx_max = 0;
>  	media_graph_walk_cleanup(&mdev->pm_count_walk);
> @@ -787,43 +812,11 @@ EXPORT_SYMBOL_GPL(__media_device_register);
>  
>  void media_device_unregister(struct media_device *mdev)
>  {
> -	struct media_entity *entity;
> -	struct media_entity *next;
> -	struct media_interface *intf, *tmp_intf;
> -	struct media_entity_notify *notify, *nextp;
> -
>  	if (mdev == NULL)
>  		return;
>  
> -	mutex_lock(&mdev->graph_mutex);
> -
> -	/* Check if mdev was ever registered at all */
> -	if (!media_devnode_is_registered(&mdev->devnode)) {
> -		mutex_unlock(&mdev->graph_mutex);
> +	if (!media_devnode_is_registered(&mdev->devnode))
>  		return;
> -	}
> -
> -	/* Remove all entities from the media device */
> -	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> -		__media_device_unregister_entity(entity);
> -
> -	/* Remove all entity_notify callbacks from the media device */
> -	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
> -		__media_device_unregister_entity_notify(mdev, notify);
> -
> -	/* Remove all interfaces from the media device */
> -	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> -				 graph_obj.list) {
> -		/*
> -		 * Unlink the interface, but don't free it here; the
> -		 * module which created it is responsible for freeing
> -		 * it
> -		 */
> -		__media_remove_intf_links(intf);
> -		media_gobj_destroy(&intf->graph_obj);
> -	}
> -
> -	mutex_unlock(&mdev->graph_mutex);
>  
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	dev_dbg(mdev->dev, "Media device unregistering\n");

Re: [PATCH v4 19/26] media: vimc: Release resources on media device release

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> Release all the resources when the media device is released, moving away
> from the struct v4l2_device used for that purpose. This is done to
> exemplify the use of the media device's release callback.
> 
> Switch to container_of_const(), too, while we're changing the code anyway.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> index af127476e920..3e59f8c256c7 100644
> --- a/drivers/media/test-drivers/vimc/vimc-core.c
> +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
>  	return 0;
>  }
>  
> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +static void vimc_mdev_release(struct media_device *mdev)
>  {
>  	struct vimc_device *vimc =
> -		container_of(v4l2_dev, struct vimc_device, v4l2_dev);
> +		container_of_const(mdev, struct vimc_device, mdev);

Please don't mix this in. It makes no sense here since vimc is never
const. Such a change doesn't belong in this series. So just leave it
at container_of and update the commit log.

With that change you can add my:

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

Regards,

	Hans

>  
>  	vimc_release_subdevs(vimc);
> -	media_device_cleanup(&vimc->mdev);
>  	kfree(vimc->ent_devs);
>  	kfree(vimc);
>  }
> @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc)
>  	return ret;
>  }
>  
> +static const struct media_device_ops vimc_mdev_ops = {
> +	.release = vimc_mdev_release,
> +};
> +
>  static int vimc_probe(struct platform_device *pdev)
>  {
>  	const struct font_desc *font = find_font("VGA8x16");
> @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev)
>  	snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info),
>  		 "platform:%s", VIMC_PDEV_NAME);
>  	vimc->mdev.dev = &pdev->dev;
> +	vimc->mdev.ops = &vimc_mdev_ops;
>  	media_device_init(&vimc->mdev);
>  
>  	ret = vimc_register_devices(vimc);
>  	if (ret) {
> -		media_device_cleanup(&vimc->mdev);
> -		kfree(vimc);
> +		media_device_put(&vimc->mdev);
>  		return ret;
>  	}
>  	/*
> @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev)
>  	 * if the registration fails, we release directly from probe
>  	 */
>  
> -	vimc->v4l2_dev.release = vimc_v4l2_dev_release;
>  	platform_set_drvdata(pdev, vimc);
>  	return 0;
>  }
> @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev)
>  	media_device_unregister(&vimc->mdev);
>  	v4l2_device_unregister(&vimc->v4l2_dev);
>  	v4l2_device_put(&vimc->v4l2_dev);
> +	media_device_put(&vimc->mdev);
>  }
>  
>  static void vimc_dev_release(struct device *dev)

Re: [PATCH v4 22/26] media: mc: Maintain a list of open file handles in a media device

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> The list of file handles is needed to deliver media events as well as for
> other purposes in the future.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-device.c  | 19 ++++++++++++++++++-
>  drivers/media/mc/mc-devnode.c |  2 +-
>  include/media/media-devnode.h |  4 +++-
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index a9505ab4412d..46d1b0c9d8be 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -45,8 +45,9 @@ static inline void __user *media_get_uptr(__u64 arg)
>  	return (void __user *)(uintptr_t)arg;
>  }
>  
> -static int media_device_open(struct file *filp)
> +static int media_device_open(struct media_devnode *devnode, struct file *filp)
>  {
> +	struct media_device *mdev = to_media_device(devnode);
>  	struct media_device_fh *fh;
>  
>  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> @@ -55,13 +56,23 @@ static int media_device_open(struct file *filp)
>  
>  	filp->private_data = &fh->fh;
>  
> +	spin_lock_irq(&mdev->fh_list_lock);
> +	list_add(&fh->mdev_list, &mdev->fh_list);
> +	spin_unlock_irq(&mdev->fh_list_lock);
> +
>  	return 0;
>  }
>  
>  static int media_device_close(struct file *filp)
>  {
> +	struct media_devnode *devnode = media_devnode_data(filp);
> +	struct media_device *mdev = to_media_device(devnode);
>  	struct media_device_fh *fh = media_device_fh(filp);
>  
> +	spin_lock_irq(&mdev->fh_list_lock);
> +	list_del(&fh->mdev_list);
> +	spin_unlock_irq(&mdev->fh_list_lock);
> +
>  	kfree(fh);
>  
>  	return 0;
> @@ -769,11 +780,13 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->pads);
>  	INIT_LIST_HEAD(&mdev->links);
>  	INIT_LIST_HEAD(&mdev->entity_notify);
> +	INIT_LIST_HEAD(&mdev->fh_list);
>  
>  	mutex_init(&mdev->req_queue_mutex);
>  	mutex_init(&mdev->graph_mutex);
>  	ida_init(&mdev->entity_internal_idx);
>  	atomic_set(&mdev->request_id, 0);
> +	spin_lock_init(&mdev->fh_list_lock);
>  
>  	mdev->devnode.release = media_device_release;
>  	media_devnode_init(&mdev->devnode);
> @@ -830,6 +843,10 @@ void media_device_unregister(struct media_device *mdev)
>  	if (!media_devnode_is_registered(&mdev->devnode))
>  		return;
>  
> +	spin_lock_irq(&mdev->fh_list_lock);
> +	list_del_init(&mdev->fh_list);
> +	spin_unlock_irq(&mdev->fh_list_lock);

Huh? This doesn't make sense to me. Unregistering the media device
makes no difference to the list of open filehandles.

> +
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	dev_dbg(mdev->dev, "Media device unregistering\n");
>  	media_devnode_unregister(&mdev->devnode);
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 26491daaba96..617156963911 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -154,7 +154,7 @@ static int media_open(struct inode *inode, struct file *filp)
>  	get_device(&devnode->dev);
>  	mutex_unlock(&media_devnode_lock);
>  
> -	ret = devnode->fops->open(filp);
> +	ret = devnode->fops->open(devnode, filp);
>  	if (ret) {
>  		put_device(&devnode->dev);
>  		return ret;
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index e4e8552598eb..898fa67ca090 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -21,6 +21,8 @@
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  
> +struct media_devnode;
> +
>  /*
>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>   * this flag directly, it will be set and cleared by media_devnode_register and
> @@ -49,7 +51,7 @@ struct media_file_operations {
>  	__poll_t (*poll) (struct file *, struct poll_table_struct *);
>  	long (*ioctl) (struct file *, unsigned int, unsigned long);
>  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> -	int (*open) (struct file *);
> +	int (*open) (struct media_devnode *, struct file *);
>  	int (*release) (struct file *);
>  };
>  

Regards,

	Hans

Re: [PATCH v4 19/26] media: vimc: Release resources on media device release

[Sakari Ailus]

Hi Hans,

Thank you for the review.

On Mon, Jun 17, 2024 at 11:49:54AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > Release all the resources when the media device is released, moving away
> > from the struct v4l2_device used for that purpose. This is done to
> > exemplify the use of the media device's release callback.
> > 
> > Switch to container_of_const(), too, while we're changing the code anyway.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > index af127476e920..3e59f8c256c7 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
> >  	return 0;
> >  }
> >  
> > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> > +static void vimc_mdev_release(struct media_device *mdev)
> >  {
> >  	struct vimc_device *vimc =
> > -		container_of(v4l2_dev, struct vimc_device, v4l2_dev);
> > +		container_of_const(mdev, struct vimc_device, mdev);
> 
> Please don't mix this in. It makes no sense here since vimc is never
> const. Such a change doesn't belong in this series. So just leave it
> at container_of and update the commit log.

Ok, I can remove it.

I also posted a patch to address the matter in container_of()
documentation. Let's see.

> 
> With that change you can add my:
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thank you!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 24/26] media: mc: Warn about drivers not releasing media device safely

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> The media device and associated resources may be released only when its
> memory is no longer used. Warn about drivers not doing this, but instead
> releasing the resources at driver unbind time.

I think this should be folded in the previous patch.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index 8cdd0d46e865..51836faa6d1a 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -822,6 +822,9 @@ int __must_check __media_device_register(struct media_device *mdev,
>  		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
>  		if (!ref)
>  			return -ENOMEM;
> +
> +		dev_warn(mdev->dev,
> +			 "Set mdev release op to safely release resources!\n");

I think this needs a comment as well. Basically stating the same as the
commit log message.

Regards,

	Hans

>  	}
>  
>  	/* Register the device node. */

Re: [PATCH v4 25/26] media: mc: Enforce one-time registration

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> A media devnode may be registered only once. Enforce this by setting the
> minor to -1 in init.
> 
> Registration initialises the character device and sets up the device name.
> These should take place only once during the lifetime of the media device.

This has nothing to do with patch 23/26, right?

I would move this to before that patch.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

Regards,

	Hans

> ---
>  drivers/media/mc/mc-devnode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 8cb4e0eec17f..758971f310c3 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -240,6 +240,7 @@ void media_devnode_init(struct media_devnode *devnode)
>  {
>  	device_initialize(&devnode->dev);
>  	devnode->dev.release = media_devnode_release;
> +	devnode->minor = -1;
>  }
>  
>  int __must_check media_devnode_register(struct media_devnode *devnode,
> @@ -251,6 +252,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>  	int minor;
>  	int ret;
>  
> +	if (devnode->minor != -1)
> +		return -EINVAL;
> +
>  	/* Part 1: Find a free minor number */
>  	mutex_lock(&media_devnode_lock);
>  	minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES);

Re: [PATCH v4 07/26] media: mc: Drop media_dev description from struct media_devnode

[Sakari Ailus]

Hi Hans,

Thanks for the review.

On Mon, Jun 17, 2024 at 11:02:16AM +0200, Hans Verkuil wrote:
> This needs a proper commit log.

Oops. I thought this had been fixed already but I'll do that now.

I'll use:

The revert of patch "[media] media-device: dynamically allocate struct
media_devnode" did not remove the kerneldoc documentation of the field
media_dev in struct media_devnode. Do it now.

> 
> Specifically it should mention that due to the revert in patch 05/26
> the media_dev field was deleted, but that that revert didn't remove it
> from the kerneldoc comments.
> 
> I would also suggest that you move this patch to 06/26 so that it
> comes right after the 05/26 revert, since the two really belong together.

I agree.

> 
> With that, you can add my:
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thank you!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 23/26] media: mc: Implement best effort media device removal safety sans refcount

[Hans Verkuil]

Hi Sakari,

On 10/06/2024 12:05, Sakari Ailus wrote:
> Add a new helper data structures media_devnode_compat_ref and
> media_devnode_cdev. The latter is used to prevent user space from calling
> IOCTLs or other system calls to the media device that has been already
> unregistered and the former to help with obtaining the container struct
> (either media_devnode_compat_ref or media_devnode) in media_open().
> 
> The media device's memory may of course still be released during the call
> but there is only so much that can be done to this without the driver
> managing the lifetime of the resources it needs somehow.
> 
> This patch should be reverted once all drivers have been converted to manage
> their resources' lifetime.

I know we discussed this before, but I really have a very, very hard time
accepting this patch. It is so terribly *ugly*.

I re-read the discussion about this from the original v1 series back in early 2023.

As I understand it, the main reason for this is that this patch series
embeds the media_devnode into media_device (patch 05/26, Revert "[media] media-device:
dynamically allocate struct media_devnode").

While imperfect, that original patch solved a crash when an unregistered device
is still in use.

So you reverted that, fixed more issues and created a new way of keeping track
of who uses the media device and allow drivers to switch to cleaning up using
the media device release callback. All very nice.

But by no longer dynamically allocating media_devnode, now all drivers that are
not yet converted to the new method (i.e. pretty much all of them) will crash.

So you add this horrible hack by allocating a media_devnode_compat_ref containing
a struct cdev, so effectively reproducing the original situation where struct
media_devnode was allocated dynamically.

In other words, depending on the driver, the media code uses either one cdev
(embedded in media_device) or another (dynamically allocated).

It's crazy.

Why not keep media_devnode dynamically allocated? Does it block the new
media_device release mechanism somehow?

Once all drivers are converted to the new mechanism, then media_devnode can
be embedded directly, but until that time, just keep it as-is. I.e. you just
postpone that last step.

It also means that you do not need to keep track of open file handles, since
it is only used at the moment for this specific hack. I understand that it
is very likely needed in the future for media events, but let's add it then,
and not mix it up in this lifetime management series.

Most of this series looks really nice, but I truly believe that you need
to keep media_devnode dynamically allocated.

I tried hard to convince myself that this patch is OK, but it just can't
accept it.



> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-device.c       |  49 +++++++++---
>  drivers/media/mc/mc-devnode.c      | 118 ++++++++++++++++++++++-------
>  drivers/media/v4l2-core/v4l2-dev.c |  26 +++++--
>  include/media/media-device.h       |   8 +-
>  include/media/media-devnode.h      |  65 ++++++++++++++--
>  5 files changed, 210 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index 46d1b0c9d8be..8cdd0d46e865 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -54,6 +54,8 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
>  	if (!fh)
>  		return -ENOMEM;
>  
> +	fh->fh.ref = devnode->ref;
> +
>  	filp->private_data = &fh->fh;
>  
>  	spin_lock_irq(&mdev->fh_list_lock);
> @@ -65,13 +67,16 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
>  
>  static int media_device_close(struct file *filp)
>  {
> -	struct media_devnode *devnode = media_devnode_data(filp);
> -	struct media_device *mdev = to_media_device(devnode);
>  	struct media_device_fh *fh = media_device_fh(filp);
>  
> -	spin_lock_irq(&mdev->fh_list_lock);
> -	list_del(&fh->mdev_list);
> -	spin_unlock_irq(&mdev->fh_list_lock);
> +	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
> +		struct media_devnode *devnode = media_devnode_data(filp);
> +		struct media_device *mdev = to_media_device(devnode);
> +
> +		spin_lock_irq(&mdev->fh_list_lock);
> +		list_del(&fh->mdev_list);
> +		spin_unlock_irq(&mdev->fh_list_lock);
> +	}
>  
>  	kfree(fh);
>  
> @@ -810,28 +815,45 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
>  int __must_check __media_device_register(struct media_device *mdev,
>  					 struct module *owner)
>  {
> +	struct media_devnode_compat_ref *ref = NULL;
>  	int ret;
>  
> +	if (!mdev->ops || !mdev->ops->release) {
> +		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
> +		if (!ref)
> +			return -ENOMEM;
> +	}
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> +	mdev->devnode.ref = ref;
>  
>  	/* Set version 0 to indicate user-space that the graph is static */
>  	mdev->topology_version = 0;
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto out_put_ref;
> +
> +	ret = device_create_file(media_devnode_dev(&mdev->devnode),
> +				 &dev_attr_model);
> +	if (ret < 0)
> +		goto out_devnode_unregister;
>  
> -	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> -	if (ret < 0) {
> -		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> -	}
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> +
> +out_devnode_unregister:
> +	media_devnode_unregister(&mdev->devnode);
> +
> +out_put_ref:
> +	if (ref)
> +		put_device(&ref->dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> @@ -847,9 +869,12 @@ void media_device_unregister(struct media_device *mdev)
>  	list_del_init(&mdev->fh_list);
>  	spin_unlock_irq(&mdev->fh_list_lock);
>  
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> +	device_remove_file(media_devnode_dev(&mdev->devnode), &dev_attr_model);
>  	dev_dbg(mdev->dev, "Media device unregistering\n");
>  	media_devnode_unregister(&mdev->devnode);
> +
> +	if (mdev->devnode.ref)
> +		put_device(&mdev->devnode.ref->dev);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 617156963911..8cb4e0eec17f 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -49,24 +49,52 @@ static void media_devnode_release(struct device *cd)
>  {
>  	struct media_devnode *devnode = to_media_devnode(cd);
>  
> +	/* If the devnode has a ref, it is simply released by the user. */

"the user"? You mean "the refcount's release function"?

> +	if (devnode->ref)
> +		return;
> +
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
>  }
>  
> +static void media_devnode_ref_release(struct device *cd)
> +{
> +	struct media_devnode_compat_ref *ref =
> +		container_of_const(cd, struct media_devnode_compat_ref, dev);
> +
> +	kfree(ref);
> +}
> +
> +struct media_devnode *to_media_devnode(struct device *dev)
> +{
> +	if (dev->release == media_devnode_release)
> +		return container_of(dev, struct media_devnode, dev);
> +
> +	return container_of(dev, struct media_devnode_compat_ref, dev)->devnode;
> +}
> +
>  static const struct bus_type media_bus_type = {
>  	.name = MEDIA_NAME,
>  };
>  
> +static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
> +{
> +	if (fh->ref)
> +		return atomic_read(&fh->ref->registered);
> +
> +	return media_devnode_is_registered(fh->devnode);
> +}
> +
>  static ssize_t media_read(struct file *filp, char __user *buf,
>  		size_t sz, loff_t *off)
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> +	if (!media_devnode_is_registered_compat(filp->private_data))
> +		return -EIO;
>  	if (!devnode->fops->read)
>  		return -EINVAL;
> -	if (!media_devnode_is_registered(devnode))
> -		return -EIO;
>  	return devnode->fops->read(filp, buf, sz, off);
>  }
>  
> @@ -75,10 +103,10 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> +	if (!media_devnode_is_registered_compat(filp->private_data))
> +		return -EIO;
>  	if (!devnode->fops->write)
>  		return -EINVAL;
> -	if (!media_devnode_is_registered(devnode))
> -		return -EIO;
>  	return devnode->fops->write(filp, buf, sz, off);
>  }
>  
> @@ -87,7 +115,7 @@ static __poll_t media_poll(struct file *filp,
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> -	if (!media_devnode_is_registered(devnode))
> +	if (!media_devnode_is_registered_compat(filp->private_data))
>  		return EPOLLERR | EPOLLHUP;
>  	if (!devnode->fops->poll)
>  		return DEFAULT_POLLMASK;
> @@ -99,14 +127,9 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
>  	      long (*ioctl_func)(struct file *filp, unsigned int cmd,
>  				 unsigned long arg))
>  {
> -	struct media_devnode *devnode = media_devnode_data(filp);
> -
>  	if (!ioctl_func)
>  		return -ENOTTY;
>  
> -	if (!media_devnode_is_registered(devnode))
> -		return -EIO;
> -
>  	return ioctl_func(filp, cmd, arg);
>  }
>  
> @@ -114,6 +137,9 @@ static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> +	if (!media_devnode_is_registered_compat(filp->private_data))
> +		return -EIO;
> +
>  	return __media_ioctl(filp, cmd, arg, devnode->fops->ioctl);
>  }
>  
> @@ -124,6 +150,9 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> +	if (!media_devnode_is_registered_compat(filp->private_data))
> +		return -EIO;
> +
>  	return __media_ioctl(filp, cmd, arg, devnode->fops->compat_ioctl);
>  }
>  
> @@ -132,6 +161,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
>  /* Override for the open function */
>  static int media_open(struct inode *inode, struct file *filp)
>  {
> +	struct media_devnode_cdev *mcdev;
>  	struct media_devnode *devnode;
>  	struct media_devnode_fh *fh;
>  	int ret;
> @@ -143,7 +173,12 @@ static int media_open(struct inode *inode, struct file *filp)
>  	 * a crash.
>  	 */
>  	mutex_lock(&media_devnode_lock);
> -	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
> +	mcdev = container_of(inode->i_cdev, struct media_devnode_cdev, cdev);
> +	if (mcdev->is_compat_ref)
> +		devnode = container_of(mcdev, struct media_devnode_compat_ref,
> +				       mcdev)->devnode;
> +	else
> +		devnode = container_of(mcdev, struct media_devnode, mcdev);
>  	/* return ENXIO if the media device has been removed
>  	   already or if it is not registered anymore. */
>  	if (!media_devnode_is_registered(devnode)) {
> @@ -151,12 +186,12 @@ static int media_open(struct inode *inode, struct file *filp)
>  		return -ENXIO;
>  	}
>  	/* and increase the device refcount */
> -	get_device(&devnode->dev);
> +	get_device(media_devnode_dev(devnode));
>  	mutex_unlock(&media_devnode_lock);
>  
>  	ret = devnode->fops->open(devnode, filp);
>  	if (ret) {
> -		put_device(&devnode->dev);
> +		put_device(media_devnode_dev(devnode));
>  		return ret;
>  	}
>  
> @@ -169,15 +204,21 @@ static int media_open(struct inode *inode, struct file *filp)
>  /* Override for the release function */
>  static int media_release(struct inode *inode, struct file *filp)
>  {
> -	struct media_devnode *devnode = media_devnode_data(filp);
> -
> -	devnode->fops->release(filp);
> +	struct media_devnode_fh *fh = filp->private_data;
> +	struct device *dev;
> +
> +	if (!fh->ref) {
> +		dev = &fh->devnode->dev;
> +		fh->devnode->fops->release(filp);
> +	} else {
> +		dev = &fh->ref->dev;
> +		fh->ref->release(filp);
> +	}
>  
>  	filp->private_data = NULL;
>  
> -	/* decrease the refcount unconditionally since the release()
> -	   return value is ignored. */
> -	put_device(&devnode->dev);
> +	put_device(dev);
> +
>  	return 0;
>  }
>  
> @@ -204,6 +245,9 @@ void media_devnode_init(struct media_devnode *devnode)
>  int __must_check media_devnode_register(struct media_devnode *devnode,
>  					struct module *owner)
>  {
> +	struct media_devnode_compat_ref *ref = devnode->ref;
> +	struct cdev *cdev;
> +	struct device *dev;
>  	int minor;
>  	int ret;
>  
> @@ -222,19 +266,31 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>  	devnode->minor = minor;
>  
>  	/* Part 2: Initialize the media and character devices */
> -	cdev_init(&devnode->cdev, &media_devnode_fops);
> -	devnode->cdev.owner = owner;
> -	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
> -
> -	devnode->dev.bus = &media_bus_type;
> -	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> +	cdev = ref ? &ref->mcdev.cdev : &devnode->mcdev.cdev;
> +	cdev_init(cdev, &media_devnode_fops);
> +	cdev->owner = owner;
> +	kobject_set_name(&cdev->kobj, "media%d", devnode->minor);
> +
> +	if (!ref) {
> +		dev = &devnode->dev;
> +	} else {
> +		ref->mcdev.is_compat_ref = true;
> +		device_initialize(&ref->dev);
> +		atomic_set(&ref->registered, 1);
> +		ref->devnode = devnode;
> +		ref->release = devnode->fops->release;
> +		dev = &ref->dev;
> +		dev->release = media_devnode_ref_release;
> +	}
> +	dev->bus = &media_bus_type;
> +	dev->devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
>  	if (devnode->parent)
> -		devnode->dev.parent = devnode->parent;
> -	dev_set_name(&devnode->dev, "media%d", devnode->minor);
> +		dev->parent = devnode->parent;
> +	dev_set_name(dev, "media%d", devnode->minor);
>  
>  	/* Part 3: Add the media and character devices */
>  	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
> -	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
> +	ret = cdev_device_add(cdev, dev);
>  	if (ret < 0) {
>  		pr_err("%s: cdev_device_add failed\n", __func__);
>  		goto cdev_add_error;
> @@ -256,11 +312,15 @@ void media_devnode_unregister(struct media_devnode *devnode)
>  	if (!media_devnode_is_registered(devnode))
>  		return;
>  
> +	if (devnode->ref)
> +		atomic_set(&devnode->ref->registered, 0);
> +
>  	mutex_lock(&media_devnode_lock);
>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  	mutex_unlock(&media_devnode_lock);
>  
> -	cdev_device_del(&devnode->cdev, &devnode->dev);
> +	cdev_device_del(devnode->ref ? &devnode->ref->mcdev.cdev :
> +			&devnode->mcdev.cdev, media_devnode_dev(devnode));
>  
>  	mutex_lock(&media_devnode_lock);
>  	clear_bit(devnode->minor, media_devnode_nums);
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 4bf4398fd2fe..2b19a845c3a4 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -179,7 +179,7 @@ static void v4l2_device_release(struct device *cd)
>  	bool v4l2_dev_call_release = v4l2_dev->release;
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_device *mdev = v4l2_dev->mdev;
> -	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
> +	bool mdev_has_ref = mdev && mdev->devnode.ref;
>  #endif
>  
>  	mutex_lock(&videodev_lock);
> @@ -212,12 +212,24 @@ static void v4l2_device_release(struct device *cd)
>  	}
>  #endif
>  
> -	/* Release video_device and perform other
> -	   cleanups as needed. */
> +	/*
> +	 * Put struct media_devnode_compat_ref here as indicated by
> +	 * mdev_has_ref. mdev may be released by vdev->release() below.
> +	 */
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	if (mdev && mdev_has_ref)
> +		media_device_put(mdev);
> +#endif
> +
> +	/* Release video_device and perform other cleanups as needed. */
>  	vdev->release(vdev);
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	if (mdev)
> +	/*
> +	 * Put a reference to struct media_device acquired in
> +	 * video_register_media_controller().
> +	 */
> +	if (mdev && !mdev_has_ref)
>  		media_device_put(mdev);
>  
>  	/*
> @@ -225,13 +237,15 @@ static void v4l2_device_release(struct device *cd)
>  	 * embedded in the same driver's context struct so having a release
>  	 * callback in both is a bug.
>  	 */
> -	if (WARN_ON(v4l2_dev_call_release && mdev_has_release))
> +	if (WARN_ON(v4l2_dev_call_release && !mdev_has_ref))
>  		v4l2_dev_call_release = false;
>  #endif
>  
>  	/*
>  	 * Decrease v4l2_device refcount, but only if the media device doesn't
> -	 * have a release callback.
> +	 * have a release callback. Otherwise one could expect accessing
> +	 * released memory --- driver's context struct refcounted already via
> +	 * struct media_device.
>  	 */
>  	if (v4l2_dev_call_release)
>  		v4l2_device_put(v4l2_dev);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index f9f7c37e7d57..30f9b78d1ce7 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -259,10 +259,10 @@ static inline void media_device_put(struct media_device *mdev)
>   *
>   * @mdev:	pointer to struct &media_device
>   *
> - * This function that will destroy the graph_mutex that is initialized in
> - * media_device_init(). Note that *only* drivers that do not manage releasing
> - * the memory of th media device itself call this function. This function is
> - * thus effectively DEPRECATED.
> + * This function will destroy the graph_mutex that is initialized in
> + * media_device_init(). Note that only drivers that do not have a proper release
> + * callback of the struct media_device call this function. This function is thus
> + * effectively DEPRECATED.
>   */
>  void media_device_cleanup(struct media_device *mdev);
>  
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index 898fa67ca090..5dee1acbc3f7 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -41,8 +41,7 @@ struct media_devnode;
>   * @compat_ioctl: pointer to the function that will handle 32 bits userspace
>   *	calls to the ioctl() syscall on a Kernel compiled with 64 bits.
>   * @open: pointer to the function that implements open() syscall
> - * @release: pointer to the function that will release the resources allocated
> - *	by the @open function.
> + * @release: pointer to the function that implements release() syscall
>   */
>  struct media_file_operations {
>  	struct module *owner;
> @@ -55,9 +54,54 @@ struct media_file_operations {
>  	int (*release) (struct file *);
>  };
>  
> +/**
> + * struct media_devnode_cdev - Workaround for drivers not managing media device
> + *			       lifetime - character device
> + *
> + * Store the characeter device and whether this is a compatibility reference or
> + * not. struct media_devnode_cdev is embedded in either struct
> + * media_devnode_compat_ref or struct media_devnode.
> + *
> + * @cdev: the chracter device
> + * @is_compat_ref: Is this a compatibility reference or not?
> + */
> +struct media_devnode_cdev {
> +	struct cdev cdev;
> +	bool is_compat_ref;
> +};
> +
> +/**
> + * struct media_devnode_compat_ref - Workaround for drivers not managing media
> + *				     device lifetime - reference
> + *
> + * The purpose if this struct is to support drivers that do not manage the
> + * lifetime of their respective media devices to avoid the worst effects of
> + * this, namely an IOCTL call on an open file handle to a device that has been
> + * unbound causing a kernel oops systematically. This is not a fix. The proper,
> + * reliable way to handle this is to manage the resources used by the
> + * driver. This struct and its use can be removed once all drivers have been
> + * converted.
> + *
> + * @dev: struct device that remains in place as long as any reference remains
> + * @mcdev: the related character device
> + * @devnode: a pointer back to the devnode
> + * @release:	pointer to the function that will release the resources
> + *		allocated by the @open function.
> + * @registered: is this device registered?
> + */
> +struct media_devnode_compat_ref {
> +	struct device dev;
> +	struct media_devnode_cdev mcdev;
> +	struct media_devnode *devnode;
> +	int (*release)(struct file *);
> +	atomic_t registered;
> +};
> +
>  /**
>   * struct media_devnode_fh - Media device node file handle
>   * @devnode:	pointer to the media device node
> + * @ref:	media device compat ref, if the driver does not manage media
> + *		device lifetime
>   *
>   * This structure serves as a base for per-file-handle data storage. Media
>   * device node users embed media_devnode_fh in their custom file handle data
> @@ -67,18 +111,21 @@ struct media_file_operations {
>   */
>  struct media_devnode_fh {
>  	struct media_devnode *devnode;
> +	struct media_devnode_compat_ref *ref;
>  };
>  
>  /**
>   * struct media_devnode - Media device node
>   * @fops:	pointer to struct &media_file_operations with media device ops
>   * @dev:	pointer to struct &device containing the media controller device
> - * @cdev:	struct cdev pointer character device
> + * @mcdev:	related to the character device
>   * @parent:	parent device
>   * @minor:	device node minor number
>   * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
>   * @release:	release callback called at the end of ``media_devnode_release()``
>   *		routine at media-device.c.
> + * @ref:	reference for providing best effort memory safety in device
> + *		removal
>   *
>   * This structure represents a media-related device node.
>   *
> @@ -91,7 +138,7 @@ struct media_devnode {
>  
>  	/* sysfs */
>  	struct device dev;		/* media device */
> -	struct cdev cdev;		/* character device */
> +	struct media_devnode_cdev mcdev; /* character device + compat status */
>  	struct device *parent;		/* device parent */
>  
>  	/* device info */
> @@ -100,10 +147,18 @@ struct media_devnode {
>  
>  	/* callbacks */
>  	void (*release)(struct media_devnode *devnode);
> +
> +	/* compat reference */
> +	struct media_devnode_compat_ref *ref;
>  };
>  
> +static inline struct device *media_devnode_dev(struct media_devnode *devnode)
> +{
> +	return devnode->ref ? &devnode->ref->dev : &devnode->dev;
> +}
> +
>  /* dev to media_devnode */
> -#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
> +struct media_devnode *to_media_devnode(struct device *dev);
>  
>  /**
>   * media_devnode_init - initialise a media devnode

Regards,

	Hans

Re: [PATCH v4 00/26] Media device lifetime management

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> Hi folks,
> 
> This is a refresh of my 2016 RFC patchset to start addressing object
> lifetime issues in Media controller. It further allows continuing work to
> address lifetime management of media entities.
> 
> The underlying problem is described in detail in v4 of the previous RFC:
> <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
> In brief, there is currently no connection between releasing media device
> (and related) memory and IOCTL calls, meaning that there is a time window
> during which released kernel memory can be accessed, and that access can be
> triggered from the user space. The only reason why this is not a grave
> security issue is that it is not triggerable by the user alone but requires
> unbinding a device. That is still not an excuse for not fixing it.
> 
> This set differs from the earlier RFC to address the issue in the
> following respects:
> 
> - Make changes for ipu3-cio2 driver, too.
> 
> - Continue to provide best effort attempt to keep the window between device
>   removal and user space being able to access released memory as small as
>   possible. This means the problem won't become worse for drivers for which
>   Media device lifetime management has not been implemented.
> 
> The latter is achieved by adding a new object, Media devnode compat
> reference, which is allocated, refcounted and eventually released by the
> Media controller framework itself, and where the information on registration
> and open filehandles is maintained. This is only done if the driver does not
> manage the lifetime of the media device itself, i.e. its release operation
> is NULL.
> 
> Due to this, Media device file handles will also be introduced by this
> patchset. I thought the first user of this would be Media device events but
> it seems we already need them here.
> 
> Some patches are temporarily reverted in order to make reworks easier,
> then applied later on. Others are not re-applied: this is a change of
> direction, not development over those patches. It would be possible to
> squash the reverts into others on the expense of readability, so the
> reverts are maintained for that reason.
> 
> I've tested this on ipu3-cio2 with and without the refcounting patch (media:
> ipu3-cio2: Release the cio2 device context by media device callback),
> including failures in a few parts of the driver initialisation process in
> the MC framework. I've also tested the vimc driver.

You need to convert at least one m2m test driver (vicodec and ideally also
vim2m). M2M device have their own media controller setup, so it is good to
have at least one converted.

Regards,

	Hans

> 
> Questions and comments are welcome.
> 
> since v3:
> 
> - Rework commit message of patch re-converting to cdev_device_add(). It's
>   no longer the same patch.
> 
> - Convert drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>   and drivers/media/test-drivers/visl/visl-core.c to changed
>   media_devnode_is_registered() argument.
> 
> - Properly check for NULL Media device on a V4L2 device in
>   video_register_media_controller().
> 
> - Don't acquire Media device graph mutex in media_device_unregister() for
>   checking whether the media device is registered.
> 
> - Remove extra call to v4l2_async_nf_cleanup() in
>   drivers/media/platform/ti/omap3isp/isp.c.
> 
> since v2:
> 
> - Switch to spin_{,un}lock_irq() in fw_list_lock use.
> 
> - Clarify documentatino regarding unregistering and releasing the media
>   device.
> 
> - Release minor number at unregister time (vs. release time). This
>   effectively caused a few patches to be dropped and one more to be added
>   (mc: Clear minor number reservation at unregistration time).
> 
> - Added a comment in ipu3-cio2 driver to clarify destroying mutexes in
>   cio2_queue_exit().
> 
> - In ipu3-cio2 driver:
> 
> 	- Clean up the notifier in probe.
> 
> 	- Unregister the sub-devices at driver unbind time.
> 
> 	- Remove queue initialisation error handling. The queues are
> 	  released in cio2_queues_exit() later in any case.
> 
> - Clean up V4L2 device teardown as suggested by Hans.
> 
> - Rewrap added text in video_register_media_controller().
> 
> - Make media_device_{get,put}() functions (they were macros) for better
>   type checking.
> 
> - Improve kerneldoc for media_device_cleanup().
> 
> - Document release function in media_file_operations.
> 
> - Correct kerneldoc documentation for struct media_devnode.
> 
> - Fix media_devnode_is_registered() usage in sound/usb/media.c that was
>   missed in v2.
> 
> - Drop old git tags from revert² patches.
> 
> - Drop revert and re-applification of "media: uvcvideo: Refactor teardown
>   of uvc on USB disconnect", instead take this account into other patches.
> 
> - Drop patch "ipu3-cio2: Call v4l2_device_unregister() earlier".
> 
> - Remove patch "ipu3-cio2: Request IRQ earlier" from this set, it'll be
>   merged separately.
> 
> since v1:
> 
> - Align subject prefixes with current media tree practices.
> 
> - Make release changes to the vimc driver (last patch of the set). This
>   was actually easy as vimc already centralised resource release to struct
>   v4l2_device, so it was just moved to the media device.
> 
> - Move cdev field to struct media_devnode_compat_ref and add dev field to
>   the struct, these are needed during device release. This now includes
>   also the character device which is accessed by __fput(). I've now tested
>   ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct
>   media_devnode_compat_ref becomes redundant and is removed. Both devices
>   are registered in case of best effort memory safety support and used for
>   refcounting.
> 
> - Drop omap3isp driver patch moving away from devm_request_irq().
> 
> - Add a patch to warn of drivers not releasing media device safely (i.e.
>   relying on the best effort memory safety mechanism without refcounting).
> 
> - Add a patch to document how the best effort memory release safety helper
>   works.
> 
> - Add a note on releasing driver's context with the media device, not the
>   V4L2 device, in MC documentation.
> 
> - Check media device is registered before accessing its fops in
>   media_read(), media_write(), media_ioctl and media_compat_ioctl().
> 
> - Document best effort media device lifetime management (new patch).
> 
> - Use media_devnode_free_minor() in unallocating device node minor number
>   in media_devnode_register().
> 
> - Continue to rely on devm_register_irq() in ipu3-cio2 driver but register
>   the IRQ later on (compared to v1).
> 
> - Drop the patch to move away from devm_request_irq() in omap3isp.
> 
> - Fix putting references to media device and V4L2 device in 
>   v4l2_device_release().
> 
> - Add missing media_device_get() (in v1) for M2M devices in
>   video_register_media_controller().
> 
> - Unconditionally set the media devnode release function in
>   media_device_init(). There's no harm doing so and the caller of
>   media_device_init() may set the ops after calling the function.
> 
> 
> Laurent Pinchart (1):
>   media: mc: Add per-file-handle data support
> 
> Sakari Ailus (25):
>   Revert "[media] media: fix media devnode ioctl/syscall and unregister
>     race"
>   Revert "media: utilize new cdev_device_add helper function"
>   Revert "[media] media: fix use-after-free in cdev_put() when app exits
>     after driver unbind"
>   media: mc, cec: Make use of cdev_device_add() again
>   Revert "[media] media-device: dynamically allocate struct
>     media_devnode"
>   media: mc: Drop nop release callback
>   media: mc: Drop media_dev description from struct media_devnode
>   media: mc: Do not call cdev_device_del() if cdev_device_add() fails
>   media: mc: Delete character device early
>   media: mc: Clear minor number reservation at unregistration time
>   media: mc: Split initialising and adding media devnode
>   media: mc: Shuffle functions around
>   media: mc: Initialise media devnode in media_device_init()
>   media: mc: Refcount the media device
>   media: v4l: Acquire a reference to the media device for every video
>     device
>   media: mc: Postpone graph object removal until free
>   media: omap3isp: Release the isp device struct by media device
>     callback
>   media: ipu3-cio2: Release the cio2 device context by media device
>     callback
>   media: vimc: Release resources on media device release
>   media: Documentation: Document how Media device resources are released
>   media: mc: Maintain a list of open file handles in a media device
>   media: mc: Implement best effort media device removal safety sans
>     refcount
>   media: mc: Warn about drivers not releasing media device safely
>   media: mc: Enforce one-time registration
>   media: Documentation: Document media device memory safety helper
> 
>  Documentation/driver-api/media/mc-core.rst    |  18 +-
>  drivers/media/cec/core/cec-core.c             |   2 +-
>  drivers/media/mc/mc-device.c                  | 258 +++++++++++-------
>  drivers/media/mc/mc-devnode.c                 | 208 +++++++++-----
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  72 +++--
>  .../vcodec/decoder/mtk_vcodec_dec_drv.c       |   2 +-
>  drivers/media/platform/ti/omap3isp/isp.c      |  25 +-
>  drivers/media/test-drivers/vimc/vimc-core.c   |  15 +-
>  drivers/media/test-drivers/visl/visl-core.c   |   2 +-
>  drivers/media/usb/au0828/au0828-core.c        |   4 +-
>  drivers/media/usb/uvc/uvc_driver.c            |   2 +-
>  drivers/media/v4l2-core/v4l2-dev.c            |  67 +++--
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   2 +-
>  include/media/media-device.h                  |  53 +++-
>  include/media/media-devnode.h                 | 136 ++++++---
>  include/media/media-fh.h                      |  32 +++
>  sound/usb/media.c                             |   8 +-
>  17 files changed, 623 insertions(+), 283 deletions(-)
>  create mode 100644 include/media/media-fh.h
> 

Re: [PATCH v4 08/26] media: mc: Do not call cdev_device_del() if cdev_device_add() fails

[Sakari Ailus]

Hi Hans,
On Mon, Jun 17, 2024 at 11:13:57AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > cdev_device_del() is the right function to remove a device when
> > cdev_device_add() succeeds. If it does not, however, put_device() needs to
> > be used instead. Fix this.
> 
> Hmm, this too is due to a revert patch (03/26) removing something that needs
> to be reinstated.
> 
> Wouldn't it be better to fold this into 04/26, with a comment in the commit
> log of that commit?

I agree, I'll fold it there.

> 
> The problem with this commit log is that it suggests that it fixes a bug,
> when really it just corrects something introduced by a revert.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 22/26] media: mc: Maintain a list of open file handles in a media device

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 11:57:20AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > The list of file handles is needed to deliver media events as well as for
> > other purposes in the future.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/mc/mc-device.c  | 19 ++++++++++++++++++-
> >  drivers/media/mc/mc-devnode.c |  2 +-
> >  include/media/media-devnode.h |  4 +++-
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index a9505ab4412d..46d1b0c9d8be 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -45,8 +45,9 @@ static inline void __user *media_get_uptr(__u64 arg)
> >  	return (void __user *)(uintptr_t)arg;
> >  }
> >  
> > -static int media_device_open(struct file *filp)
> > +static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >  {
> > +	struct media_device *mdev = to_media_device(devnode);
> >  	struct media_device_fh *fh;
> >  
> >  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> > @@ -55,13 +56,23 @@ static int media_device_open(struct file *filp)
> >  
> >  	filp->private_data = &fh->fh;
> >  
> > +	spin_lock_irq(&mdev->fh_list_lock);
> > +	list_add(&fh->mdev_list, &mdev->fh_list);
> > +	spin_unlock_irq(&mdev->fh_list_lock);
> > +
> >  	return 0;
> >  }
> >  
> >  static int media_device_close(struct file *filp)
> >  {
> > +	struct media_devnode *devnode = media_devnode_data(filp);
> > +	struct media_device *mdev = to_media_device(devnode);
> >  	struct media_device_fh *fh = media_device_fh(filp);
> >  
> > +	spin_lock_irq(&mdev->fh_list_lock);
> > +	list_del(&fh->mdev_list);
> > +	spin_unlock_irq(&mdev->fh_list_lock);
> > +
> >  	kfree(fh);
> >  
> >  	return 0;
> > @@ -769,11 +780,13 @@ void media_device_init(struct media_device *mdev)
> >  	INIT_LIST_HEAD(&mdev->pads);
> >  	INIT_LIST_HEAD(&mdev->links);
> >  	INIT_LIST_HEAD(&mdev->entity_notify);
> > +	INIT_LIST_HEAD(&mdev->fh_list);
> >  
> >  	mutex_init(&mdev->req_queue_mutex);
> >  	mutex_init(&mdev->graph_mutex);
> >  	ida_init(&mdev->entity_internal_idx);
> >  	atomic_set(&mdev->request_id, 0);
> > +	spin_lock_init(&mdev->fh_list_lock);
> >  
> >  	mdev->devnode.release = media_device_release;
> >  	media_devnode_init(&mdev->devnode);
> > @@ -830,6 +843,10 @@ void media_device_unregister(struct media_device *mdev)
> >  	if (!media_devnode_is_registered(&mdev->devnode))
> >  		return;
> >  
> > +	spin_lock_irq(&mdev->fh_list_lock);
> > +	list_del_init(&mdev->fh_list);
> > +	spin_unlock_irq(&mdev->fh_list_lock);
> 
> Huh? This doesn't make sense to me. Unregistering the media device
> makes no difference to the list of open filehandles.

Right, I agree with that.

Presumably the list will be empty at release time. I think I'll drop this
and add a sanity check for the list.

> 
> > +
> >  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> >  	dev_dbg(mdev->dev, "Media device unregistering\n");
> >  	media_devnode_unregister(&mdev->devnode);
> > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> > index 26491daaba96..617156963911 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -154,7 +154,7 @@ static int media_open(struct inode *inode, struct file *filp)
> >  	get_device(&devnode->dev);
> >  	mutex_unlock(&media_devnode_lock);
> >  
> > -	ret = devnode->fops->open(filp);
> > +	ret = devnode->fops->open(devnode, filp);
> >  	if (ret) {
> >  		put_device(&devnode->dev);
> >  		return ret;
> > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> > index e4e8552598eb..898fa67ca090 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -21,6 +21,8 @@
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> >  
> > +struct media_devnode;
> > +
> >  /*
> >   * Flag to mark the media_devnode struct as registered. Drivers must not touch
> >   * this flag directly, it will be set and cleared by media_devnode_register and
> > @@ -49,7 +51,7 @@ struct media_file_operations {
> >  	__poll_t (*poll) (struct file *, struct poll_table_struct *);
> >  	long (*ioctl) (struct file *, unsigned int, unsigned long);
> >  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> > -	int (*open) (struct file *);
> > +	int (*open) (struct media_devnode *, struct file *);
> >  	int (*release) (struct file *);
> >  };
> >  
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 24/26] media: mc: Warn about drivers not releasing media device safely

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 12:40:31PM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > The media device and associated resources may be released only when its
> > memory is no longer used. Warn about drivers not doing this, but instead
> > releasing the resources at driver unbind time.
> 
> I think this should be folded in the previous patch.
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/mc/mc-device.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index 8cdd0d46e865..51836faa6d1a 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -822,6 +822,9 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
> >  		if (!ref)
> >  			return -ENOMEM;
> > +
> > +		dev_warn(mdev->dev,
> > +			 "Set mdev release op to safely release resources!\n");
> 
> I think this needs a comment as well. Basically stating the same as the
> commit log message.

I'll address these for v5.

-- 
Sakari Ailus

Re: [PATCH v4 12/26] media: mc: Shuffle functions around

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 11:41:10AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > As the call paths of the functions in question will change, move them
> > around in anticipation of that. No other changes.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> That should be:
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

To be addressed for v5, the same for 16th patch.

-- 
Sakari Ailus

Re: [PATCH v4 23/26] media: mc: Implement best effort media device removal safety sans refcount

[Sakari Ailus]

Hi Hans,

Thank you for the review.

On Mon, Jun 17, 2024 at 01:54:09PM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > Add a new helper data structures media_devnode_compat_ref and
> > media_devnode_cdev. The latter is used to prevent user space from calling
> > IOCTLs or other system calls to the media device that has been already
> > unregistered and the former to help with obtaining the container struct
> > (either media_devnode_compat_ref or media_devnode) in media_open().
> > 
> > The media device's memory may of course still be released during the call
> > but there is only so much that can be done to this without the driver
> > managing the lifetime of the resources it needs somehow.
> > 
> > This patch should be reverted once all drivers have been converted to manage
> > their resources' lifetime.
> 
> I know we discussed this before, but I really have a very, very hard time
> accepting this patch. It is so terribly *ugly*.

I don't pretend otherwise.

> 
> I re-read the discussion about this from the original v1 series back in early 2023.
> 
> As I understand it, the main reason for this is that this patch series
> embeds the media_devnode into media_device (patch 05/26, Revert "[media] media-device:
> dynamically allocate struct media_devnode").
> 
> While imperfect, that original patch solved a crash when an unregistered device
> is still in use.

That's not entirely true. The original patch reduced the likelihood of a
crash (as well as security issues) but did not fix them. The only reason
this never was a grave security issue was that the condition wasn't
user-triggerable.

Additionally, long-lived IOCTL calls such as dequeueing media events won't
be possible without proper media device lifetime management this is more
than just "a nice cleanup".

> 
> So you reverted that, fixed more issues and created a new way of keeping track
> of who uses the media device and allow drivers to switch to cleaning up using
> the media device release callback. All very nice.
> 
> But by no longer dynamically allocating media_devnode, now all drivers that are
> not yet converted to the new method (i.e. pretty much all of them) will crash.
> 
> So you add this horrible hack by allocating a media_devnode_compat_ref containing
> a struct cdev, so effectively reproducing the original situation where struct
> media_devnode was allocated dynamically.
> 
> In other words, depending on the driver, the media code uses either one cdev
> (embedded in media_device) or another (dynamically allocated).
> 
> It's crazy.
> 
> Why not keep media_devnode dynamically allocated? Does it block the new
> media_device release mechanism somehow?
> 
> Once all drivers are converted to the new mechanism, then media_devnode can
> be embedded directly, but until that time, just keep it as-is. I.e. you just
> postpone that last step.
> 
> It also means that you do not need to keep track of open file handles, since
> it is only used at the moment for this specific hack. I understand that it
> is very likely needed in the future for media events, but let's add it then,
> and not mix it up in this lifetime management series.
> 
> Most of this series looks really nice, but I truly believe that you need
> to keep media_devnode dynamically allocated.
> 
> I tried hard to convince myself that this patch is OK, but it just can't
> accept it.

Admittedly I didn't think it'd become this complicated but it did, given
that both need to work without issues.

I'd like to remind that the two should be unified, hence the state before
this patch is a better starting point for that work than where the series
started off. Do note that it would be possible now to merge struct
media_device and struct media_devnode without touching all drivers.

Even if I'd re-separate struct media_device and struct media_devnode
allocations, the convoluted code paths would remain. I'm not sure how much
simplification that would achieve.

I'd rather merge struct media_devnode and struct media_device in the same
set. That's actually useful work that takes us forward while leaving the
workaround in place for drivers that need it.

> 
> 
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/mc/mc-device.c       |  49 +++++++++---
> >  drivers/media/mc/mc-devnode.c      | 118 ++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/v4l2-dev.c |  26 +++++--
> >  include/media/media-device.h       |   8 +-
> >  include/media/media-devnode.h      |  65 ++++++++++++++--
> >  5 files changed, 210 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index 46d1b0c9d8be..8cdd0d46e865 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -54,6 +54,8 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >  	if (!fh)
> >  		return -ENOMEM;
> >  
> > +	fh->fh.ref = devnode->ref;
> > +
> >  	filp->private_data = &fh->fh;
> >  
> >  	spin_lock_irq(&mdev->fh_list_lock);
> > @@ -65,13 +67,16 @@ static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >  
> >  static int media_device_close(struct file *filp)
> >  {
> > -	struct media_devnode *devnode = media_devnode_data(filp);
> > -	struct media_device *mdev = to_media_device(devnode);
> >  	struct media_device_fh *fh = media_device_fh(filp);
> >  
> > -	spin_lock_irq(&mdev->fh_list_lock);
> > -	list_del(&fh->mdev_list);
> > -	spin_unlock_irq(&mdev->fh_list_lock);
> > +	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
> > +		struct media_devnode *devnode = media_devnode_data(filp);
> > +		struct media_device *mdev = to_media_device(devnode);
> > +
> > +		spin_lock_irq(&mdev->fh_list_lock);
> > +		list_del(&fh->mdev_list);
> > +		spin_unlock_irq(&mdev->fh_list_lock);
> > +	}
> >  
> >  	kfree(fh);
> >  
> > @@ -810,28 +815,45 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
> >  int __must_check __media_device_register(struct media_device *mdev,
> >  					 struct module *owner)
> >  {
> > +	struct media_devnode_compat_ref *ref = NULL;
> >  	int ret;
> >  
> > +	if (!mdev->ops || !mdev->ops->release) {
> > +		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
> > +		if (!ref)
> > +			return -ENOMEM;
> > +	}
> > +
> >  	/* Register the device node. */
> >  	mdev->devnode.fops = &media_device_fops;
> >  	mdev->devnode.parent = mdev->dev;
> > +	mdev->devnode.ref = ref;
> >  
> >  	/* Set version 0 to indicate user-space that the graph is static */
> >  	mdev->topology_version = 0;
> >  
> >  	ret = media_devnode_register(&mdev->devnode, owner);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto out_put_ref;
> > +
> > +	ret = device_create_file(media_devnode_dev(&mdev->devnode),
> > +				 &dev_attr_model);
> > +	if (ret < 0)
> > +		goto out_devnode_unregister;
> >  
> > -	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> > -	if (ret < 0) {
> > -		media_devnode_unregister(&mdev->devnode);
> > -		return ret;
> > -	}
> >  
> >  	dev_dbg(mdev->dev, "Media device registered\n");
> >  
> >  	return 0;
> > +
> > +out_devnode_unregister:
> > +	media_devnode_unregister(&mdev->devnode);
> > +
> > +out_put_ref:
> > +	if (ref)
> > +		put_device(&ref->dev);
> > +
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(__media_device_register);
> >  
> > @@ -847,9 +869,12 @@ void media_device_unregister(struct media_device *mdev)
> >  	list_del_init(&mdev->fh_list);
> >  	spin_unlock_irq(&mdev->fh_list_lock);
> >  
> > -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> > +	device_remove_file(media_devnode_dev(&mdev->devnode), &dev_attr_model);
> >  	dev_dbg(mdev->dev, "Media device unregistering\n");
> >  	media_devnode_unregister(&mdev->devnode);
> > +
> > +	if (mdev->devnode.ref)
> > +		put_device(&mdev->devnode.ref->dev);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> >  
> > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> > index 617156963911..8cb4e0eec17f 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -49,24 +49,52 @@ static void media_devnode_release(struct device *cd)
> >  {
> >  	struct media_devnode *devnode = to_media_devnode(cd);
> >  
> > +	/* If the devnode has a ref, it is simply released by the user. */
> 
> "the user"? You mean "the refcount's release function"?

The driver that registered the media device. Ref only exists for drivers
that don't properly release the memory in the release callback (this one).

> 
> > +	if (devnode->ref)
> > +		return;
> > +
> >  	/* Release media_devnode and perform other cleanups as needed. */
> >  	if (devnode->release)
> >  		devnode->release(devnode);
> >  }
> >  
> > +static void media_devnode_ref_release(struct device *cd)
> > +{
> > +	struct media_devnode_compat_ref *ref =
> > +		container_of_const(cd, struct media_devnode_compat_ref, dev);
> > +
> > +	kfree(ref);
> > +}
> > +
> > +struct media_devnode *to_media_devnode(struct device *dev)
> > +{
> > +	if (dev->release == media_devnode_release)
> > +		return container_of(dev, struct media_devnode, dev);
> > +
> > +	return container_of(dev, struct media_devnode_compat_ref, dev)->devnode;
> > +}
> > +
> >  static const struct bus_type media_bus_type = {
> >  	.name = MEDIA_NAME,
> >  };
> >  
> > +static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
> > +{
> > +	if (fh->ref)
> > +		return atomic_read(&fh->ref->registered);
> > +
> > +	return media_devnode_is_registered(fh->devnode);
> > +}
> > +
> >  static ssize_t media_read(struct file *filp, char __user *buf,
> >  		size_t sz, loff_t *off)
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  
> > +	if (!media_devnode_is_registered_compat(filp->private_data))
> > +		return -EIO;
> >  	if (!devnode->fops->read)
> >  		return -EINVAL;
> > -	if (!media_devnode_is_registered(devnode))
> > -		return -EIO;
> >  	return devnode->fops->read(filp, buf, sz, off);
> >  }
> >  
> > @@ -75,10 +103,10 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  
> > +	if (!media_devnode_is_registered_compat(filp->private_data))
> > +		return -EIO;
> >  	if (!devnode->fops->write)
> >  		return -EINVAL;
> > -	if (!media_devnode_is_registered(devnode))
> > -		return -EIO;
> >  	return devnode->fops->write(filp, buf, sz, off);
> >  }
> >  
> > @@ -87,7 +115,7 @@ static __poll_t media_poll(struct file *filp,
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  
> > -	if (!media_devnode_is_registered(devnode))
> > +	if (!media_devnode_is_registered_compat(filp->private_data))
> >  		return EPOLLERR | EPOLLHUP;
> >  	if (!devnode->fops->poll)
> >  		return DEFAULT_POLLMASK;
> > @@ -99,14 +127,9 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
> >  	      long (*ioctl_func)(struct file *filp, unsigned int cmd,
> >  				 unsigned long arg))
> >  {
> > -	struct media_devnode *devnode = media_devnode_data(filp);
> > -
> >  	if (!ioctl_func)
> >  		return -ENOTTY;
> >  
> > -	if (!media_devnode_is_registered(devnode))
> > -		return -EIO;
> > -
> >  	return ioctl_func(filp, cmd, arg);
> >  }
> >  
> > @@ -114,6 +137,9 @@ static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  
> > +	if (!media_devnode_is_registered_compat(filp->private_data))
> > +		return -EIO;
> > +
> >  	return __media_ioctl(filp, cmd, arg, devnode->fops->ioctl);
> >  }
> >  
> > @@ -124,6 +150,9 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  
> > +	if (!media_devnode_is_registered_compat(filp->private_data))
> > +		return -EIO;
> > +
> >  	return __media_ioctl(filp, cmd, arg, devnode->fops->compat_ioctl);
> >  }
> >  
> > @@ -132,6 +161,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
> >  /* Override for the open function */
> >  static int media_open(struct inode *inode, struct file *filp)
> >  {
> > +	struct media_devnode_cdev *mcdev;
> >  	struct media_devnode *devnode;
> >  	struct media_devnode_fh *fh;
> >  	int ret;
> > @@ -143,7 +173,12 @@ static int media_open(struct inode *inode, struct file *filp)
> >  	 * a crash.
> >  	 */
> >  	mutex_lock(&media_devnode_lock);
> > -	devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
> > +	mcdev = container_of(inode->i_cdev, struct media_devnode_cdev, cdev);
> > +	if (mcdev->is_compat_ref)
> > +		devnode = container_of(mcdev, struct media_devnode_compat_ref,
> > +				       mcdev)->devnode;
> > +	else
> > +		devnode = container_of(mcdev, struct media_devnode, mcdev);
> >  	/* return ENXIO if the media device has been removed
> >  	   already or if it is not registered anymore. */
> >  	if (!media_devnode_is_registered(devnode)) {
> > @@ -151,12 +186,12 @@ static int media_open(struct inode *inode, struct file *filp)
> >  		return -ENXIO;
> >  	}
> >  	/* and increase the device refcount */
> > -	get_device(&devnode->dev);
> > +	get_device(media_devnode_dev(devnode));
> >  	mutex_unlock(&media_devnode_lock);
> >  
> >  	ret = devnode->fops->open(devnode, filp);
> >  	if (ret) {
> > -		put_device(&devnode->dev);
> > +		put_device(media_devnode_dev(devnode));
> >  		return ret;
> >  	}
> >  
> > @@ -169,15 +204,21 @@ static int media_open(struct inode *inode, struct file *filp)
> >  /* Override for the release function */
> >  static int media_release(struct inode *inode, struct file *filp)
> >  {
> > -	struct media_devnode *devnode = media_devnode_data(filp);
> > -
> > -	devnode->fops->release(filp);
> > +	struct media_devnode_fh *fh = filp->private_data;
> > +	struct device *dev;
> > +
> > +	if (!fh->ref) {
> > +		dev = &fh->devnode->dev;
> > +		fh->devnode->fops->release(filp);
> > +	} else {
> > +		dev = &fh->ref->dev;
> > +		fh->ref->release(filp);
> > +	}
> >  
> >  	filp->private_data = NULL;
> >  
> > -	/* decrease the refcount unconditionally since the release()
> > -	   return value is ignored. */
> > -	put_device(&devnode->dev);
> > +	put_device(dev);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -204,6 +245,9 @@ void media_devnode_init(struct media_devnode *devnode)
> >  int __must_check media_devnode_register(struct media_devnode *devnode,
> >  					struct module *owner)
> >  {
> > +	struct media_devnode_compat_ref *ref = devnode->ref;
> > +	struct cdev *cdev;
> > +	struct device *dev;
> >  	int minor;
> >  	int ret;
> >  
> > @@ -222,19 +266,31 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
> >  	devnode->minor = minor;
> >  
> >  	/* Part 2: Initialize the media and character devices */
> > -	cdev_init(&devnode->cdev, &media_devnode_fops);
> > -	devnode->cdev.owner = owner;
> > -	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
> > -
> > -	devnode->dev.bus = &media_bus_type;
> > -	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> > +	cdev = ref ? &ref->mcdev.cdev : &devnode->mcdev.cdev;
> > +	cdev_init(cdev, &media_devnode_fops);
> > +	cdev->owner = owner;
> > +	kobject_set_name(&cdev->kobj, "media%d", devnode->minor);
> > +
> > +	if (!ref) {
> > +		dev = &devnode->dev;
> > +	} else {
> > +		ref->mcdev.is_compat_ref = true;
> > +		device_initialize(&ref->dev);
> > +		atomic_set(&ref->registered, 1);
> > +		ref->devnode = devnode;
> > +		ref->release = devnode->fops->release;
> > +		dev = &ref->dev;
> > +		dev->release = media_devnode_ref_release;
> > +	}
> > +	dev->bus = &media_bus_type;
> > +	dev->devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> >  	if (devnode->parent)
> > -		devnode->dev.parent = devnode->parent;
> > -	dev_set_name(&devnode->dev, "media%d", devnode->minor);
> > +		dev->parent = devnode->parent;
> > +	dev_set_name(dev, "media%d", devnode->minor);
> >  
> >  	/* Part 3: Add the media and character devices */
> >  	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
> > -	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
> > +	ret = cdev_device_add(cdev, dev);
> >  	if (ret < 0) {
> >  		pr_err("%s: cdev_device_add failed\n", __func__);
> >  		goto cdev_add_error;
> > @@ -256,11 +312,15 @@ void media_devnode_unregister(struct media_devnode *devnode)
> >  	if (!media_devnode_is_registered(devnode))
> >  		return;
> >  
> > +	if (devnode->ref)
> > +		atomic_set(&devnode->ref->registered, 0);
> > +
> >  	mutex_lock(&media_devnode_lock);
> >  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
> >  	mutex_unlock(&media_devnode_lock);
> >  
> > -	cdev_device_del(&devnode->cdev, &devnode->dev);
> > +	cdev_device_del(devnode->ref ? &devnode->ref->mcdev.cdev :
> > +			&devnode->mcdev.cdev, media_devnode_dev(devnode));
> >  
> >  	mutex_lock(&media_devnode_lock);
> >  	clear_bit(devnode->minor, media_devnode_nums);
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 4bf4398fd2fe..2b19a845c3a4 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -179,7 +179,7 @@ static void v4l2_device_release(struct device *cd)
> >  	bool v4l2_dev_call_release = v4l2_dev->release;
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >  	struct media_device *mdev = v4l2_dev->mdev;
> > -	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
> > +	bool mdev_has_ref = mdev && mdev->devnode.ref;
> >  #endif
> >  
> >  	mutex_lock(&videodev_lock);
> > @@ -212,12 +212,24 @@ static void v4l2_device_release(struct device *cd)
> >  	}
> >  #endif
> >  
> > -	/* Release video_device and perform other
> > -	   cleanups as needed. */
> > +	/*
> > +	 * Put struct media_devnode_compat_ref here as indicated by
> > +	 * mdev_has_ref. mdev may be released by vdev->release() below.
> > +	 */
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	if (mdev && mdev_has_ref)
> > +		media_device_put(mdev);
> > +#endif
> > +
> > +	/* Release video_device and perform other cleanups as needed. */
> >  	vdev->release(vdev);
> >  
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> > -	if (mdev)
> > +	/*
> > +	 * Put a reference to struct media_device acquired in
> > +	 * video_register_media_controller().
> > +	 */
> > +	if (mdev && !mdev_has_ref)
> >  		media_device_put(mdev);
> >  
> >  	/*
> > @@ -225,13 +237,15 @@ static void v4l2_device_release(struct device *cd)
> >  	 * embedded in the same driver's context struct so having a release
> >  	 * callback in both is a bug.
> >  	 */
> > -	if (WARN_ON(v4l2_dev_call_release && mdev_has_release))
> > +	if (WARN_ON(v4l2_dev_call_release && !mdev_has_ref))
> >  		v4l2_dev_call_release = false;
> >  #endif
> >  
> >  	/*
> >  	 * Decrease v4l2_device refcount, but only if the media device doesn't
> > -	 * have a release callback.
> > +	 * have a release callback. Otherwise one could expect accessing
> > +	 * released memory --- driver's context struct refcounted already via
> > +	 * struct media_device.
> >  	 */
> >  	if (v4l2_dev_call_release)
> >  		v4l2_device_put(v4l2_dev);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index f9f7c37e7d57..30f9b78d1ce7 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -259,10 +259,10 @@ static inline void media_device_put(struct media_device *mdev)
> >   *
> >   * @mdev:	pointer to struct &media_device
> >   *
> > - * This function that will destroy the graph_mutex that is initialized in
> > - * media_device_init(). Note that *only* drivers that do not manage releasing
> > - * the memory of th media device itself call this function. This function is
> > - * thus effectively DEPRECATED.
> > + * This function will destroy the graph_mutex that is initialized in
> > + * media_device_init(). Note that only drivers that do not have a proper release
> > + * callback of the struct media_device call this function. This function is thus
> > + * effectively DEPRECATED.
> >   */
> >  void media_device_cleanup(struct media_device *mdev);
> >  
> > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> > index 898fa67ca090..5dee1acbc3f7 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -41,8 +41,7 @@ struct media_devnode;
> >   * @compat_ioctl: pointer to the function that will handle 32 bits userspace
> >   *	calls to the ioctl() syscall on a Kernel compiled with 64 bits.
> >   * @open: pointer to the function that implements open() syscall
> > - * @release: pointer to the function that will release the resources allocated
> > - *	by the @open function.
> > + * @release: pointer to the function that implements release() syscall
> >   */
> >  struct media_file_operations {
> >  	struct module *owner;
> > @@ -55,9 +54,54 @@ struct media_file_operations {
> >  	int (*release) (struct file *);
> >  };
> >  
> > +/**
> > + * struct media_devnode_cdev - Workaround for drivers not managing media device
> > + *			       lifetime - character device
> > + *
> > + * Store the characeter device and whether this is a compatibility reference or
> > + * not. struct media_devnode_cdev is embedded in either struct
> > + * media_devnode_compat_ref or struct media_devnode.
> > + *
> > + * @cdev: the chracter device
> > + * @is_compat_ref: Is this a compatibility reference or not?
> > + */
> > +struct media_devnode_cdev {
> > +	struct cdev cdev;
> > +	bool is_compat_ref;
> > +};
> > +
> > +/**
> > + * struct media_devnode_compat_ref - Workaround for drivers not managing media
> > + *				     device lifetime - reference
> > + *
> > + * The purpose if this struct is to support drivers that do not manage the
> > + * lifetime of their respective media devices to avoid the worst effects of
> > + * this, namely an IOCTL call on an open file handle to a device that has been
> > + * unbound causing a kernel oops systematically. This is not a fix. The proper,
> > + * reliable way to handle this is to manage the resources used by the
> > + * driver. This struct and its use can be removed once all drivers have been
> > + * converted.
> > + *
> > + * @dev: struct device that remains in place as long as any reference remains
> > + * @mcdev: the related character device
> > + * @devnode: a pointer back to the devnode
> > + * @release:	pointer to the function that will release the resources
> > + *		allocated by the @open function.
> > + * @registered: is this device registered?
> > + */
> > +struct media_devnode_compat_ref {
> > +	struct device dev;
> > +	struct media_devnode_cdev mcdev;
> > +	struct media_devnode *devnode;
> > +	int (*release)(struct file *);
> > +	atomic_t registered;
> > +};
> > +
> >  /**
> >   * struct media_devnode_fh - Media device node file handle
> >   * @devnode:	pointer to the media device node
> > + * @ref:	media device compat ref, if the driver does not manage media
> > + *		device lifetime
> >   *
> >   * This structure serves as a base for per-file-handle data storage. Media
> >   * device node users embed media_devnode_fh in their custom file handle data
> > @@ -67,18 +111,21 @@ struct media_file_operations {
> >   */
> >  struct media_devnode_fh {
> >  	struct media_devnode *devnode;
> > +	struct media_devnode_compat_ref *ref;
> >  };
> >  
> >  /**
> >   * struct media_devnode - Media device node
> >   * @fops:	pointer to struct &media_file_operations with media device ops
> >   * @dev:	pointer to struct &device containing the media controller device
> > - * @cdev:	struct cdev pointer character device
> > + * @mcdev:	related to the character device
> >   * @parent:	parent device
> >   * @minor:	device node minor number
> >   * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
> >   * @release:	release callback called at the end of ``media_devnode_release()``
> >   *		routine at media-device.c.
> > + * @ref:	reference for providing best effort memory safety in device
> > + *		removal
> >   *
> >   * This structure represents a media-related device node.
> >   *
> > @@ -91,7 +138,7 @@ struct media_devnode {
> >  
> >  	/* sysfs */
> >  	struct device dev;		/* media device */
> > -	struct cdev cdev;		/* character device */
> > +	struct media_devnode_cdev mcdev; /* character device + compat status */
> >  	struct device *parent;		/* device parent */
> >  
> >  	/* device info */
> > @@ -100,10 +147,18 @@ struct media_devnode {
> >  
> >  	/* callbacks */
> >  	void (*release)(struct media_devnode *devnode);
> > +
> > +	/* compat reference */
> > +	struct media_devnode_compat_ref *ref;
> >  };
> >  
> > +static inline struct device *media_devnode_dev(struct media_devnode *devnode)
> > +{
> > +	return devnode->ref ? &devnode->ref->dev : &devnode->dev;
> > +}
> > +
> >  /* dev to media_devnode */
> > -#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
> > +struct media_devnode *to_media_devnode(struct device *dev);
> >  
> >  /**
> >   * media_devnode_init - initialise a media devnode
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 22/26] media: mc: Maintain a list of open file handles in a media device

[Hans Verkuil]

On 17/06/2024 19:46, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jun 17, 2024 at 11:57:20AM +0200, Hans Verkuil wrote:
>> On 10/06/2024 12:05, Sakari Ailus wrote:
>>> The list of file handles is needed to deliver media events as well as for
>>> other purposes in the future.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>  drivers/media/mc/mc-device.c  | 19 ++++++++++++++++++-
>>>  drivers/media/mc/mc-devnode.c |  2 +-
>>>  include/media/media-devnode.h |  4 +++-
>>>  3 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>> index a9505ab4412d..46d1b0c9d8be 100644
>>> --- a/drivers/media/mc/mc-device.c
>>> +++ b/drivers/media/mc/mc-device.c
>>> @@ -45,8 +45,9 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>  	return (void __user *)(uintptr_t)arg;
>>>  }
>>>  
>>> -static int media_device_open(struct file *filp)
>>> +static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>  {
>>> +	struct media_device *mdev = to_media_device(devnode);
>>>  	struct media_device_fh *fh;
>>>  
>>>  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
>>> @@ -55,13 +56,23 @@ static int media_device_open(struct file *filp)
>>>  
>>>  	filp->private_data = &fh->fh;
>>>  
>>> +	spin_lock_irq(&mdev->fh_list_lock);
>>> +	list_add(&fh->mdev_list, &mdev->fh_list);
>>> +	spin_unlock_irq(&mdev->fh_list_lock);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>>  static int media_device_close(struct file *filp)
>>>  {
>>> +	struct media_devnode *devnode = media_devnode_data(filp);
>>> +	struct media_device *mdev = to_media_device(devnode);
>>>  	struct media_device_fh *fh = media_device_fh(filp);
>>>  
>>> +	spin_lock_irq(&mdev->fh_list_lock);
>>> +	list_del(&fh->mdev_list);
>>> +	spin_unlock_irq(&mdev->fh_list_lock);
>>> +
>>>  	kfree(fh);
>>>  
>>>  	return 0;
>>> @@ -769,11 +780,13 @@ void media_device_init(struct media_device *mdev)
>>>  	INIT_LIST_HEAD(&mdev->pads);
>>>  	INIT_LIST_HEAD(&mdev->links);
>>>  	INIT_LIST_HEAD(&mdev->entity_notify);
>>> +	INIT_LIST_HEAD(&mdev->fh_list);
>>>  
>>>  	mutex_init(&mdev->req_queue_mutex);
>>>  	mutex_init(&mdev->graph_mutex);
>>>  	ida_init(&mdev->entity_internal_idx);
>>>  	atomic_set(&mdev->request_id, 0);
>>> +	spin_lock_init(&mdev->fh_list_lock);
>>>  
>>>  	mdev->devnode.release = media_device_release;
>>>  	media_devnode_init(&mdev->devnode);
>>> @@ -830,6 +843,10 @@ void media_device_unregister(struct media_device *mdev)
>>>  	if (!media_devnode_is_registered(&mdev->devnode))
>>>  		return;
>>>  
>>> +	spin_lock_irq(&mdev->fh_list_lock);
>>> +	list_del_init(&mdev->fh_list);
>>> +	spin_unlock_irq(&mdev->fh_list_lock);
>>
>> Huh? This doesn't make sense to me. Unregistering the media device
>> makes no difference to the list of open filehandles.
> 
> Right, I agree with that.
> 
> Presumably the list will be empty at release time. I think I'll drop this
> and add a sanity check for the list.

Why would it be empty? You can have multiple fhs open when media_device_unregister
is called. But that's fine, eventually all fhs will be closed.

Regards,

	Hans

> 
>>
>>> +
>>>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>>  	dev_dbg(mdev->dev, "Media device unregistering\n");
>>>  	media_devnode_unregister(&mdev->devnode);
>>> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
>>> index 26491daaba96..617156963911 100644
>>> --- a/drivers/media/mc/mc-devnode.c
>>> +++ b/drivers/media/mc/mc-devnode.c
>>> @@ -154,7 +154,7 @@ static int media_open(struct inode *inode, struct file *filp)
>>>  	get_device(&devnode->dev);
>>>  	mutex_unlock(&media_devnode_lock);
>>>  
>>> -	ret = devnode->fops->open(filp);
>>> +	ret = devnode->fops->open(devnode, filp);
>>>  	if (ret) {
>>>  		put_device(&devnode->dev);
>>>  		return ret;
>>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>>> index e4e8552598eb..898fa67ca090 100644
>>> --- a/include/media/media-devnode.h
>>> +++ b/include/media/media-devnode.h
>>> @@ -21,6 +21,8 @@
>>>  #include <linux/device.h>
>>>  #include <linux/cdev.h>
>>>  
>>> +struct media_devnode;
>>> +
>>>  /*
>>>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>>>   * this flag directly, it will be set and cleared by media_devnode_register and
>>> @@ -49,7 +51,7 @@ struct media_file_operations {
>>>  	__poll_t (*poll) (struct file *, struct poll_table_struct *);
>>>  	long (*ioctl) (struct file *, unsigned int, unsigned long);
>>>  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
>>> -	int (*open) (struct file *);
>>> +	int (*open) (struct media_devnode *, struct file *);
>>>  	int (*release) (struct file *);
>>>  };
>>>  
>>
> 

Re: [PATCH v4 22/26] media: mc: Maintain a list of open file handles in a media device

[Sakari Ailus]

Hi Hans,

On Tue, Jun 18, 2024 at 07:35:45AM +0200, Hans Verkuil wrote:
> On 17/06/2024 19:46, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Jun 17, 2024 at 11:57:20AM +0200, Hans Verkuil wrote:
> >> On 10/06/2024 12:05, Sakari Ailus wrote:
> >>> @@ -830,6 +843,10 @@ void media_device_unregister(struct media_device *mdev)
> >>>  	if (!media_devnode_is_registered(&mdev->devnode))
> >>>  		return;
> >>>  
> >>> +	spin_lock_irq(&mdev->fh_list_lock);
> >>> +	list_del_init(&mdev->fh_list);
> >>> +	spin_unlock_irq(&mdev->fh_list_lock);
> >>
> >> Huh? This doesn't make sense to me. Unregistering the media device
> >> makes no difference to the list of open filehandles.
> > 
> > Right, I agree with that.
> > 
> > Presumably the list will be empty at release time. I think I'll drop this
> > and add a sanity check for the list.
> 
> Why would it be empty? You can have multiple fhs open when
> media_device_unregister is called. But that's fine, eventually all fhs
> will be closed.

By "release" I meant device's (memory) release callback, i.e. no file
handles to the device would remain open by that time.

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 25/26] media: mc: Enforce one-time registration

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 12:42:02PM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > A media devnode may be registered only once. Enforce this by setting the
> > minor to -1 in init.
> > 
> > Registration initialises the character device and sets up the device name.
> > These should take place only once during the lifetime of the media device.
> 
> This has nothing to do with patch 23/26, right?
> 
> I would move this to before that patch.

Sounds good.

> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

-- 
Sakari Ailus

Re: [PATCH v4 00/26] Media device lifetime management

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 01:55:53PM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > Hi folks,
> > 
> > This is a refresh of my 2016 RFC patchset to start addressing object
> > lifetime issues in Media controller. It further allows continuing work to
> > address lifetime management of media entities.
> > 
> > The underlying problem is described in detail in v4 of the previous RFC:
> > <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
> > In brief, there is currently no connection between releasing media device
> > (and related) memory and IOCTL calls, meaning that there is a time window
> > during which released kernel memory can be accessed, and that access can be
> > triggered from the user space. The only reason why this is not a grave
> > security issue is that it is not triggerable by the user alone but requires
> > unbinding a device. That is still not an excuse for not fixing it.
> > 
> > This set differs from the earlier RFC to address the issue in the
> > following respects:
> > 
> > - Make changes for ipu3-cio2 driver, too.
> > 
> > - Continue to provide best effort attempt to keep the window between device
> >   removal and user space being able to access released memory as small as
> >   possible. This means the problem won't become worse for drivers for which
> >   Media device lifetime management has not been implemented.
> > 
> > The latter is achieved by adding a new object, Media devnode compat
> > reference, which is allocated, refcounted and eventually released by the
> > Media controller framework itself, and where the information on registration
> > and open filehandles is maintained. This is only done if the driver does not
> > manage the lifetime of the media device itself, i.e. its release operation
> > is NULL.
> > 
> > Due to this, Media device file handles will also be introduced by this
> > patchset. I thought the first user of this would be Media device events but
> > it seems we already need them here.
> > 
> > Some patches are temporarily reverted in order to make reworks easier,
> > then applied later on. Others are not re-applied: this is a change of
> > direction, not development over those patches. It would be possible to
> > squash the reverts into others on the expense of readability, so the
> > reverts are maintained for that reason.
> > 
> > I've tested this on ipu3-cio2 with and without the refcounting patch (media:
> > ipu3-cio2: Release the cio2 device context by media device callback),
> > including failures in a few parts of the driver initialisation process in
> > the MC framework. I've also tested the vimc driver.
> 
> You need to convert at least one m2m test driver (vicodec and ideally also
> vim2m). M2M device have their own media controller setup, so it is good to
> have at least one converted.

My earlier objection to convert vim2m to managed media device lifetime was
that vim2m was obviously intended to be compiled without MC but it seems
since 016baa59bf9f6 CONFIG_MEDIA_CONTROLLER is selected for the driver.
Thus the related #ifdefs can be removed, too.

I'll add two patches for this, one that can be merged independently for
removing the #ifdefs.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 23/26] media: mc: Implement best effort media device removal safety sans refcount

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 08:28:07PM +0000, Sakari Ailus wrote:
> > It also means that you do not need to keep track of open file handles, since
> > it is only used at the moment for this specific hack. I understand that it
> > is very likely needed in the future for media events, but let's add it then,
> > and not mix it up in this lifetime management series.

Beyond Media device events, Media device file handles will soon be needed
for proper handling of buffers related to requests in the core. I don't
think we should postpone adding media device file handles.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> Clear the media device's minor number reservation at unregister time as
> there's no need to keep it reserved for longer. This makes it possible to
> reserve the same minor right after unregistration.

Have you tested this?

I'm not certain whether this won't cause kobject errors. If an app has the
media device open when the device is unbound, then the old device node still
is around until the application closes the fh. I'm not sure if you can create
a new device node with the same minor while an old device node with the same
minor is still around.

V4L2 and CEC definitely both keep the old minor until the last user has gone.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-devnode.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 38c472498f9e..f7ecabe469a4 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -49,13 +49,6 @@ static void media_devnode_release(struct device *cd)
>  {
>  	struct media_devnode *devnode = to_media_devnode(cd);
>  
> -	mutex_lock(&media_devnode_lock);
> -
> -	/* Mark device node number as free */
> -	clear_bit(devnode->minor, media_devnode_nums);
> -
> -	mutex_unlock(&media_devnode_lock);
> -
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
> @@ -268,6 +261,10 @@ void media_devnode_unregister(struct media_devnode *devnode)
>  
>  	cdev_del(&devnode->cdev);
>  	device_unregister(&devnode->dev);
> +
> +	mutex_lock(&media_devnode_lock);
> +	clear_bit(devnode->minor, media_devnode_nums);
> +	mutex_unlock(&media_devnode_lock);
>  }
>  
>  /*

Re: [PATCH v4 01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> ioctl/syscall and unregister race"). The commit was part of an original
> patchset to avoid crashes when an unregistering device is in use.

Reverting all these old commits and then adding back some of the code that
was removed is IMHO a bad idea.

I took patches 1-8 and just folded them all together, and the end result
was *much* easier to review. And the resulting patch can be cleaned up a
bit as there are some unnecessary changes included (e.g. a cec change).

With all the reverts and then reinstating code I also have little confidence
in the quality of the code if you have to do a git bisect later and you
end up in the middle of patches 1-8: it is far better to just do a single
patch. Effectively it is embedding devnode in media_device, so just do
that.

Regards,

	Hans

Re: [PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Sakari Ailus]

Hi Hans,

On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > Clear the media device's minor number reservation at unregister time as
> > there's no need to keep it reserved for longer. This makes it possible to
> > reserve the same minor right after unregistration.
> 
> Have you tested this?

By unbinding and re-binding a device while file handles to the old Media
device one are still around.

> 
> I'm not certain whether this won't cause kobject errors. If an app has the
> media device open when the device is unbound, then the old device node still
> is around until the application closes the fh. I'm not sure if you can create
> a new device node with the same minor while an old device node with the same
> minor is still around.

I added the patch based on Laurent's comments
<URL:https://lore.kernel.org/linux-media/20240207100810.GG23702@pendragon.ideasonboard.com/>.

> 
> V4L2 and CEC definitely both keep the old minor until the last user has gone.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Sakari Ailus]

Hi Hans,

On Thu, Jun 27, 2024 at 08:53:22AM +0200, Hans Verkuil wrote:
> On 10/06/2024 12:05, Sakari Ailus wrote:
> > This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> > ioctl/syscall and unregister race"). The commit was part of an original
> > patchset to avoid crashes when an unregistering device is in use.
> 
> Reverting all these old commits and then adding back some of the code that
> was removed is IMHO a bad idea.
> 
> I took patches 1-8 and just folded them all together, and the end result
> was *much* easier to review. And the resulting patch can be cleaned up a
> bit as there are some unnecessary changes included (e.g. a cec change).
> 
> With all the reverts and then reinstating code I also have little confidence
> in the quality of the code if you have to do a git bisect later and you
> end up in the middle of patches 1-8: it is far better to just do a single
> patch. Effectively it is embedding devnode in media_device, so just do
> that.

The reverts shouldn't really need a review as we're just reverting to an
earlier state of affairs in MC codebase. As you probably noticed, over the
first 8 patches there is little more than reverts while the rest are fixes
(as well as some preparation for what follows). So from review point of
view I do prefer the current state of the first 8 patches.

I'd like to have Laurent's opinion on this, too.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Sakari Ailus]

On Thu, Jun 27, 2024 at 06:58:03AM +0000, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote:
> > On 10/06/2024 12:05, Sakari Ailus wrote:
> > > Clear the media device's minor number reservation at unregister time as
> > > there's no need to keep it reserved for longer. This makes it possible to
> > > reserve the same minor right after unregistration.
> > 
> > Have you tested this?
> 
> By unbinding and re-binding a device while file handles to the old Media
> device one are still around.

Also in MC we don't have an array indexed by minor numbers of Media
devices, as we have for V4L2 device nodes.

How is it for CEC?

-- 
Sakari Ailus

Re: [PATCH v4 01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Hans Verkuil]

On 27/06/2024 09:04, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Jun 27, 2024 at 08:53:22AM +0200, Hans Verkuil wrote:
>> On 10/06/2024 12:05, Sakari Ailus wrote:
>>> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
>>> ioctl/syscall and unregister race"). The commit was part of an original
>>> patchset to avoid crashes when an unregistering device is in use.
>>
>> Reverting all these old commits and then adding back some of the code that
>> was removed is IMHO a bad idea.
>>
>> I took patches 1-8 and just folded them all together, and the end result
>> was *much* easier to review. And the resulting patch can be cleaned up a
>> bit as there are some unnecessary changes included (e.g. a cec change).
>>
>> With all the reverts and then reinstating code I also have little confidence
>> in the quality of the code if you have to do a git bisect later and you
>> end up in the middle of patches 1-8: it is far better to just do a single
>> patch. Effectively it is embedding devnode in media_device, so just do
>> that.
> 
> The reverts shouldn't really need a review as we're just reverting to an
> earlier state of affairs in MC codebase. As you probably noticed, over the
> first 8 patches there is little more than reverts while the rest are fixes
> (as well as some preparation for what follows). So from review point of
> view I do prefer the current state of the first 8 patches.
> 
> I'd like to have Laurent's opinion on this, too.
> 

Just try folding patches 1-8 together. The end result is far, far easier
to understand.

In addition, those reverts also change things like cec, which I really
don't like.

And if there are fixes, should those be done first?

Basically what I want to see in this patch series is:

- first fix any existing bugs in the existing code: those can
  also go in right now, no need to wait for the whole series to
  be approved. E.g. patch 10/26 is a good example of that.
- then (TBD) have a single patch that embeds the devnode,
- then add the new lifetime features.

Right now it is an unholy mix of bug fixes, reverts, reinstatements,
and new features, and it is hard to see what is what.

Regards,

	Hans

Re: [PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Hans Verkuil]

On 27/06/2024 09:10, Sakari Ailus wrote:
> On Thu, Jun 27, 2024 at 06:58:03AM +0000, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote:
>>> On 10/06/2024 12:05, Sakari Ailus wrote:
>>>> Clear the media device's minor number reservation at unregister time as
>>>> there's no need to keep it reserved for longer. This makes it possible to
>>>> reserve the same minor right after unregistration.
>>>
>>> Have you tested this?
>>
>> By unbinding and re-binding a device while file handles to the old Media
>> device one are still around.
> 
> Also in MC we don't have an array indexed by minor numbers of Media
> devices, as we have for V4L2 device nodes.
> 
> How is it for CEC?
> 

It doesn't have such an array either. I think that's V4L2 specific, and I wonder
if it is really needed. That code is very old, so it wouldn't surprise me if it
can be removed.

Regards,

	Hans

Re: [PATCH v4 10/26] media: mc: Clear minor number reservation at unregistration time

[Hans Verkuil]

On 10/06/2024 12:05, Sakari Ailus wrote:
> Clear the media device's minor number reservation at unregister time as
> there's no need to keep it reserved for longer. This makes it possible to
> reserve the same minor right after unregistration.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hverkuil+cisco@kernel.org>

Regards,

	Hans

> ---
>  drivers/media/mc/mc-devnode.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 38c472498f9e..f7ecabe469a4 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -49,13 +49,6 @@ static void media_devnode_release(struct device *cd)
>  {
>  	struct media_devnode *devnode = to_media_devnode(cd);
>  
> -	mutex_lock(&media_devnode_lock);
> -
> -	/* Mark device node number as free */
> -	clear_bit(devnode->minor, media_devnode_nums);
> -
> -	mutex_unlock(&media_devnode_lock);
> -
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
> @@ -268,6 +261,10 @@ void media_devnode_unregister(struct media_devnode *devnode)
>  
>  	cdev_del(&devnode->cdev);
>  	device_unregister(&devnode->dev);
> +
> +	mutex_lock(&media_devnode_lock);
> +	clear_bit(devnode->minor, media_devnode_nums);
> +	mutex_unlock(&media_devnode_lock);
>  }
>  
>  /*