diff mbox series

[v2,10/29] media: mc: Delete character device early

Message ID 20231220103713.113386-11-sakari.ailus@linux.intel.com
State Superseded
Headers show
Series Media device lifetime management | expand

Commit Message

Sakari Ailus Dec. 20, 2023, 10:36 a.m. UTC 
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>
---
 drivers/media/mc/mc-devnode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

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

Thank you for the patch.

On Wed, Dec 20, 2023 at 12:36:54PM +0200, Sakari Ailus wrote:
> 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>
> ---
>  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 7e22938dfd81..8bc7450ac144 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);

Should this be moved to media_devnode_unregister() too ? It can be done
in a separate patch.

>  
> @@ -270,6 +267,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);

I initially wondered if this could raise with the cdev access in
media_open(), as the media_devnode_lock is released just before calling
cdev_dev(), but my understanding is that the dev/open race is properly
handled in the cdev layer.

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

I wonder if a similar change in v4l2-dev.c would be beneficial.

>  	device_unregister(&devnode->dev);
>  }
>
diff mbox series

Patch

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 7e22938dfd81..8bc7450ac144 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);
 
@@ -270,6 +267,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);
 }