diff mbox series

[v2,11/29] media: mc: Split initialising and adding media devnode

Message ID 20231220103713.113386-12-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers show
Series Media device lifetime management | expand

Commit Message

Sakari Ailus Dec. 20, 2023, 10:36 a.m. UTC 
As 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 distribute the effects of these changes yet. Add media_device_get()
and media_device_put() 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 | 17 ++++++++++-------
 include/media/media-devnode.h | 19 ++++++++++++++-----
 3 files changed, 37 insertions(+), 17 deletions(-)

Comments

Laurent Pinchart Feb. 7, 2024, 10:46 a.m. UTC | #1 
Hi Sakari,

Thank you for the patch.

On Wed, Dec 20, 2023 at 12:36:55PM +0200, Sakari Ailus wrote:
> As registering a device node of an entity belonging to a media device

Did you mean s/As registering/Registering/ ?

> will require a reference to the struct device. Taking that reference is
> only possible once the device has been initialised, which took place only

s/took/takes/

> when it was registered. Split this in two, and initialise the device when

s/was/is/

> the media device is allocated.
> 
> Don't distribute the effects of these changes yet. Add media_device_get()
> and media_device_put() first.

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.


I'm not sure that's exactly what you meant though.

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

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

> ---
>  drivers/media/mc/mc-device.c  | 18 +++++++++++++-----
>  drivers/media/mc/mc-devnode.c | 17 ++++++++++-------
>  include/media/media-devnode.h | 19 ++++++++++++++-----
>  3 files changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index c0ea08a8fc31..ebf037cd5f4a 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 out_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 out_unregister;
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> +
> +out_unregister:

I would name the labels err_unregister and err_put.

> +	media_devnode_unregister(&mdev->devnode);
> +out_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 8bc7450ac144..7b17419050fb 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -204,6 +204,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)
>  {
> @@ -235,7 +240,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 */
>  	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
> @@ -267,14 +271,13 @@ 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);
>  }
>  
>  /*
>   *	Initialise media for linux
>   */
> -static int __init media_devnode_init(void)
> +static int __init media_devnode_module_init(void)
>  {
>  	int ret;
>  
> @@ -296,14 +299,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 1117d1dfd6bf..6d46c658be21 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -90,6 +90,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
>   *
> @@ -100,11 +111,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);
Sakari Ailus March 5, 2024, 8:59 a.m. UTC | #2 
Hi Laurent,

Thanks for the review.

On Wed, Feb 07, 2024 at 12:46:13PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Dec 20, 2023 at 12:36:55PM +0200, Sakari Ailus wrote:
> > As registering a device node of an entity belonging to a media device
> 
> Did you mean s/As registering/Registering/ ?

That works better in the current context. I'll use that.

> 
> > will require a reference to the struct device. Taking that reference is
> > only possible once the device has been initialised, which took place only
> 
> s/took/takes/
> 
> > when it was registered. Split this in two, and initialise the device when
> 
> s/was/is/

This describes the behaviour before the patch. I think it makes sense to
use imperfect here.

> 
> > the media device is allocated.
> > 
> > Don't distribute the effects of these changes yet. Add media_device_get()
> > and media_device_put() first.
> 
> 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.
> 
> 
> I'm not sure that's exactly what you meant though.

Yes, that's roughly what I meant, I can use the above text.

> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> 
> > ---
> >  drivers/media/mc/mc-device.c  | 18 +++++++++++++-----
> >  drivers/media/mc/mc-devnode.c | 17 ++++++++++-------
> >  include/media/media-devnode.h | 19 ++++++++++++++-----
> >  3 files changed, 37 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index c0ea08a8fc31..ebf037cd5f4a 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 out_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 out_unregister;
> >  
> >  	dev_dbg(mdev->dev, "Media device registered\n");
> >  
> >  	return 0;
> > +
> > +out_unregister:
> 
> I would name the labels err_unregister and err_put.

Sounds good.

> 
> > +	media_devnode_unregister(&mdev->devnode);
> > +out_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 8bc7450ac144..7b17419050fb 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -204,6 +204,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)
> >  {
> > @@ -235,7 +240,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 */
> >  	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
> > @@ -267,14 +271,13 @@ 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);
> >  }
> >  
> >  /*
> >   *	Initialise media for linux
> >   */
> > -static int __init media_devnode_init(void)
> > +static int __init media_devnode_module_init(void)
> >  {
> >  	int ret;
> >  
> > @@ -296,14 +299,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 1117d1dfd6bf..6d46c658be21 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -90,6 +90,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
> >   *
> > @@ -100,11 +111,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);
>
diff mbox series

Patch

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c0ea08a8fc31..ebf037cd5f4a 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 out_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 out_unregister;
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+out_unregister:
+	media_devnode_unregister(&mdev->devnode);
+out_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 8bc7450ac144..7b17419050fb 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -204,6 +204,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)
 {
@@ -235,7 +240,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 */
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
@@ -267,14 +271,13 @@  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);
 }
 
 /*
  *	Initialise media for linux
  */
-static int __init media_devnode_init(void)
+static int __init media_devnode_module_init(void)
 {
 	int ret;
 
@@ -296,14 +299,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 1117d1dfd6bf..6d46c658be21 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -90,6 +90,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
  *
@@ -100,11 +111,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);