[RFC 1/2] media: Use a better owner for the media device

[RFC 1/2] media: Use a better owner for the media device

[Sakari Ailus]

mdev->fops->owner is actually the owner of the very same module which
implements media_device_register(), so it can't be unloaded anyway. Instead,
use THIS_MODULE through a macro as does video_register_device().

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index d5a7a13..51217f0 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -372,7 +372,8 @@ static void media_device_release(struct media_devnode *mdev)
  * - dev must point to the parent device
  * - model must be filled with the device model name
  */
-int __must_check media_device_register(struct media_device *mdev)
+int __must_check __media_device_register(struct media_device *mdev,
+					 struct module *owner)
 {
 	int ret;
 
@@ -388,7 +389,7 @@ int __must_check media_device_register(struct media_device *mdev)
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
 	mdev->devnode.release = media_device_release;
-	ret = media_devnode_register(&mdev->devnode);
+	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
 		return ret;
 
@@ -400,7 +401,7 @@ int __must_check media_device_register(struct media_device *mdev)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(media_device_register);
+EXPORT_SYMBOL_GPL(__media_device_register);
 
 /**
  * media_device_unregister - unregister a media device
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index fb0f046..7acd19c 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -232,7 +232,8 @@ static const struct file_operations media_devnode_fops = {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_devnode *mdev)
+int __must_check media_devnode_register(struct media_devnode *mdev,
+					struct module *owner)
 {
 	int minor;
 	int ret;
@@ -253,7 +254,7 @@ int __must_check media_devnode_register(struct media_devnode *mdev)
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&mdev->cdev, &media_devnode_fops);
-	mdev->cdev.owner = mdev->fops->owner;
+	mdev->cdev.owner = owner;
 
 	ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1);
 	if (ret < 0) {
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 12155a9..6e6db78 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -87,7 +87,9 @@ struct media_device {
 /* media_devnode to media_device */
 #define to_media_device(node) container_of(node, struct media_device, devnode)
 
-int __must_check media_device_register(struct media_device *mdev);
+int __must_check __media_device_register(struct media_device *mdev,
+					 struct module *owner);
+#define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
 void media_device_unregister(struct media_device *mdev);
 
 int __must_check media_device_register_entity(struct media_device *mdev,
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 3446af2..0dc7060 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -82,7 +82,8 @@ struct media_devnode {
 /* dev to media_devnode */
 #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
 
-int __must_check media_devnode_register(struct media_devnode *mdev);
+int __must_check media_devnode_register(struct media_devnode *mdev,
+					struct module *owner);
 void media_devnode_unregister(struct media_devnode *mdev);
 
 static inline struct media_devnode *media_devnode_data(struct file *filp)
-- 
1.8.3.2

[RFC 2/2] media: v4l: Only get module if it's different than the driver for v4l2_dev

[Sakari Ailus]

When the sub-device is registered, increment the use count of the sub-device
owner only if it's different from the owner of the driver for the media
device. This avoids increasing the use count by the module itself and thus
making it possible to unload it when it's not in use.

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

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 02d1b63..9f6d1ec 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -158,7 +158,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	/* Warn if we apparently re-register a subdev */
 	WARN_ON(sd->v4l2_dev != NULL);
 
-	if (!try_module_get(sd->owner))
+	if (sd->owner != v4l2_dev->dev->driver->owner &&
+	    !try_module_get(sd->owner))
 		return -ENODEV;
 
 	sd->v4l2_dev = v4l2_dev;
@@ -192,7 +193,8 @@ error_unregister:
 	if (sd->internal_ops && sd->internal_ops->unregistered)
 		sd->internal_ops->unregistered(sd);
 error_module:
-	module_put(sd->owner);
+	if (sd->owner != v4l2_dev->dev->driver->owner)
+		module_put(sd->owner);
 	sd->v4l2_dev = NULL;
 	return err;
 }
@@ -280,6 +282,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	}
 #endif
 	video_unregister_device(sd->devnode);
-	module_put(sd->owner);
+	if (sd->owner != v4l2_dev->dev->driver->owner)
+		module_put(sd->owner);
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
-- 
1.8.3.2

[RFC v1.1 2/2] media: v4l: Only get module if it's different than the driver for v4l2_dev

[Sakari Ailus]

When the sub-device is registered, increment the use count of the sub-device
owner only if it's different from the owner of the driver for the media
device. This avoids increasing the use count by the module itself and thus
making it possible to unload it when it's not in use.

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

- Check that v4l2_dev->dev and v4l2_dev->dev->driver are non-NULL before
  using them.
- Store the information on the same owner into struct v4l2_subdev. This avoids
  issues related to unregistering subdevs through v4l2_device_unregister().

 drivers/media/v4l2-core/v4l2-device.c | 18 +++++++++++++++---
 include/media/v4l2-subdev.h           |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 02d1b63..015f92a 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -158,7 +158,17 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	/* Warn if we apparently re-register a subdev */
 	WARN_ON(sd->v4l2_dev != NULL);
 
-	if (!try_module_get(sd->owner))
+	/*
+	 * The reason to acquire the module here is to avoid unloading
+	 * a module of sub-device which is registered to a media
+	 * device. To make it possible to unload modules for media
+	 * devices that also register sub-devices, do not
+	 * try_module_get() such sub-device owners.
+	 */
+	sd->owner_v4l2_dev = v4l2_dev->dev && v4l2_dev->dev->driver &&
+		sd->owner == v4l2_dev->dev->driver->owner;
+
+	if (!sd->owner_v4l2_dev && !try_module_get(sd->owner))
 		return -ENODEV;
 
 	sd->v4l2_dev = v4l2_dev;
@@ -192,7 +202,8 @@ error_unregister:
 	if (sd->internal_ops && sd->internal_ops->unregistered)
 		sd->internal_ops->unregistered(sd);
 error_module:
-	module_put(sd->owner);
+	if (!sd->owner_v4l2_dev)
+		module_put(sd->owner);
 	sd->v4l2_dev = NULL;
 	return err;
 }
@@ -280,6 +291,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	}
 #endif
 	video_unregister_device(sd->devnode);
-	module_put(sd->owner);
+	if (!sd->owner_v4l2_dev)
+		module_put(sd->owner);
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d67210a..6d03b54 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -579,6 +579,7 @@ struct v4l2_subdev {
 #endif
 	struct list_head list;
 	struct module *owner;
+	bool owner_v4l2_dev;
 	u32 flags;
 	struct v4l2_device *v4l2_dev;
 	const struct v4l2_subdev_ops *ops;
-- 
1.8.3.2

Re: [RFC v1.1 2/2] media: v4l: Only get module if it's different than the driver for v4l2_dev

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 17 December 2013 15:49:24 Sakari Ailus wrote:
> When the sub-device is registered, increment the use count of the sub-device
> owner only if it's different from the owner of the driver for the media
> device. This avoids increasing the use count by the module itself and thus
> making it possible to unload it when it's not in use.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This looks good to me, but I wonder whether a more generic solution won't be 
needed, to solve the multiple circular reference issues we (will) have with 
subdevices and clocks. My gut feeling is that such a generic solution will 
also cater for the needs of the problem you're trying to solve here.

This being said, there's no reason to delay this patch until a more generic 
solution is available, so

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

> ---
> Changes since v1:
> 
> - Check that v4l2_dev->dev and v4l2_dev->dev->driver are non-NULL before
>   using them.
> - Store the information on the same owner into struct v4l2_subdev. This
> avoids issues related to unregistering subdevs through
> v4l2_device_unregister().
> 
>  drivers/media/v4l2-core/v4l2-device.c | 18 +++++++++++++++---
>  include/media/v4l2-subdev.h           |  1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c
> b/drivers/media/v4l2-core/v4l2-device.c index 02d1b63..015f92a 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -158,7 +158,17 @@ int v4l2_device_register_subdev(struct v4l2_device
> *v4l2_dev, /* Warn if we apparently re-register a subdev */
>  	WARN_ON(sd->v4l2_dev != NULL);
> 
> -	if (!try_module_get(sd->owner))
> +	/*
> +	 * The reason to acquire the module here is to avoid unloading
> +	 * a module of sub-device which is registered to a media
> +	 * device. To make it possible to unload modules for media
> +	 * devices that also register sub-devices, do not
> +	 * try_module_get() such sub-device owners.
> +	 */
> +	sd->owner_v4l2_dev = v4l2_dev->dev && v4l2_dev->dev->driver &&
> +		sd->owner == v4l2_dev->dev->driver->owner;
> +
> +	if (!sd->owner_v4l2_dev && !try_module_get(sd->owner))
>  		return -ENODEV;
> 
>  	sd->v4l2_dev = v4l2_dev;
> @@ -192,7 +202,8 @@ error_unregister:
>  	if (sd->internal_ops && sd->internal_ops->unregistered)
>  		sd->internal_ops->unregistered(sd);
>  error_module:
> -	module_put(sd->owner);
> +	if (!sd->owner_v4l2_dev)
> +		module_put(sd->owner);
>  	sd->v4l2_dev = NULL;
>  	return err;
>  }
> @@ -280,6 +291,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev
> *sd) }
>  #endif
>  	video_unregister_device(sd->devnode);
> -	module_put(sd->owner);
> +	if (!sd->owner_v4l2_dev)
> +		module_put(sd->owner);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d67210a..6d03b54 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -579,6 +579,7 @@ struct v4l2_subdev {
>  #endif
>  	struct list_head list;
>  	struct module *owner;
> +	bool owner_v4l2_dev;
>  	u32 flags;
>  	struct v4l2_device *v4l2_dev;
>  	const struct v4l2_subdev_ops *ops;
-- 
Regards,

Laurent Pinchart

Re: [RFC v1.1 2/2] media: v4l: Only get module if it's different than the driver for v4l2_dev

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Tuesday 17 December 2013 15:49:24 Sakari Ailus wrote:
>> When the sub-device is registered, increment the use count of the sub-device
>> owner only if it's different from the owner of the driver for the media
>> device. This avoids increasing the use count by the module itself and thus
>> making it possible to unload it when it's not in use.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> This looks good to me, but I wonder whether a more generic solution won't be
> needed, to solve the multiple circular reference issues we (will) have with
> subdevices and clocks. My gut feeling is that such a generic solution will
> also cater for the needs of the problem you're trying to solve here.

I can't immediately think of solving this in a generic fashion. There 
are dependencies to API behaviour for instance. For clocks this could be 
resolved by changing how clk_get() is used by sensor drivers, or 
changing the clock framework to allow unregistering clocks even if they 
have been obtained by the users but not enabled. Considering the current 
implementation of clk_unregister(), the need for (some) changes is 
apparent. (I could miss some changes elsewhere as I just checked 
linux-media.)

The above would still resolve this for clocks alone.

> This being said, there's no reason to delay this patch until a more generic
> solution is available, so
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/2] media: Use a better owner for the media device

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday 13 December 2013 14:03:35 Sakari Ailus wrote:
> mdev->fops->owner is actually the owner of the very same module which
> implements media_device_register(), so it can't be unloaded anyway. Instead,
> use THIS_MODULE through a macro as does video_register_device().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This looks good to me.

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

> ---
>  drivers/media/media-device.c  | 7 ++++---
>  drivers/media/media-devnode.c | 5 +++--
>  include/media/media-device.h  | 4 +++-
>  include/media/media-devnode.h | 3 ++-
>  4 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index d5a7a13..51217f0 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -372,7 +372,8 @@ static void media_device_release(struct media_devnode
> *mdev) * - dev must point to the parent device
>   * - model must be filled with the device model name
>   */
> -int __must_check media_device_register(struct media_device *mdev)
> +int __must_check __media_device_register(struct media_device *mdev,
> +					 struct module *owner)
>  {
>  	int ret;
> 
> @@ -388,7 +389,7 @@ int __must_check media_device_register(struct
> media_device *mdev) mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
>  	mdev->devnode.release = media_device_release;
> -	ret = media_devnode_register(&mdev->devnode);
> +	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -400,7 +401,7 @@ int __must_check media_device_register(struct
> media_device *mdev)
> 
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(media_device_register);
> +EXPORT_SYMBOL_GPL(__media_device_register);
> 
>  /**
>   * media_device_unregister - unregister a media device
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index fb0f046..7acd19c 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -232,7 +232,8 @@ static const struct file_operations media_devnode_fops =
> { * the media_devnode structure is *not* called, so the caller is
> responsible for * freeing any data.
>   */
> -int __must_check media_devnode_register(struct media_devnode *mdev)
> +int __must_check media_devnode_register(struct media_devnode *mdev,
> +					struct module *owner)
>  {
>  	int minor;
>  	int ret;
> @@ -253,7 +254,7 @@ int __must_check media_devnode_register(struct
> media_devnode *mdev)
> 
>  	/* Part 2: Initialize and register the character device */
>  	cdev_init(&mdev->cdev, &media_devnode_fops);
> -	mdev->cdev.owner = mdev->fops->owner;
> +	mdev->cdev.owner = owner;
> 
>  	ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1);
>  	if (ret < 0) {
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 12155a9..6e6db78 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -87,7 +87,9 @@ struct media_device {
>  /* media_devnode to media_device */
>  #define to_media_device(node) container_of(node, struct media_device,
> devnode)
> 
> -int __must_check media_device_register(struct media_device *mdev);
> +int __must_check __media_device_register(struct media_device *mdev,
> +					 struct module *owner);
> +#define media_device_register(mdev) __media_device_register(mdev,
> THIS_MODULE) void media_device_unregister(struct media_device *mdev);
> 
>  int __must_check media_device_register_entity(struct media_device *mdev,
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index 3446af2..0dc7060 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -82,7 +82,8 @@ struct media_devnode {
>  /* dev to media_devnode */
>  #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
> 
> -int __must_check media_devnode_register(struct media_devnode *mdev);
> +int __must_check media_devnode_register(struct media_devnode *mdev,
> +					struct module *owner);
>  void media_devnode_unregister(struct media_devnode *mdev);
> 
>  static inline struct media_devnode *media_devnode_data(struct file *filp)
-- 
Regards,

Laurent Pinchart