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

  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(-)
  

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);
>  }
>
  

Hi Laurent,

On Wed, Feb 07, 2024 at 12:08:10PM +0200, Laurent Pinchart wrote:
> 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.

Good question. Yes, I think that seems reasonable. The minor isn't needed
after unregistering the device.

> 
> >  
> > @@ -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>

Thanks!

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

Quite possibly. Let's see...