Re: [PATCH v2 15/29] media: mc: Unassign minor only if it has been assigned

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Wed, Feb 07, 2024 at 12:58:15PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Dec 20, 2023 at 12:36:59PM +0200, Sakari Ailus wrote:
> > Assign the media device minor to -1 if it has not been previously
> > assigned. There's no dependence to this yes but it will be required by
> > patch "media: mc: Implement best effort media device removal safety sans
> > refcount" soon.
> 
> After a quick look at that patch, I don't see the dependency I'm afraid.
> Could you please explain it better ?
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/mc/mc-devnode.c | 9 ++++++++-
> >  include/media/media-devnode.h | 2 +-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> > index 717408791a7c..5057c48f8870 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd)
> >  {
> >  	struct media_devnode *devnode = to_media_devnode(cd);
> >  
> > -	media_devnode_free_minor(devnode->minor);
> > +	if (devnode->minor != -1)
> > +		media_devnode_free_minor(devnode->minor);
> 
> Should the test be moved to media_devnode_free_minor() ?

The intention here is to free the minor number once and only once.

Moving the test to media_devnode_free_minor() is not an option as the
devnode may be released already by that time. See the patch the commit
message refers to.

> 
> >  
> >  	/* Release media_devnode and perform other cleanups as needed. */
> >  	if (devnode->release)
> > @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = {
> >  void media_devnode_init(struct media_devnode *devnode)
> >  {
> >  	device_initialize(&devnode->dev);
> > +	devnode->minor = -1;
> >  }
> >  
> >  int __must_check media_devnode_register(struct media_devnode *devnode,
> > @@ -220,6 +222,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);
> > @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
> >  cdev_add_error:
> >  	media_devnode_free_minor(devnode->minor);
> >  
> > +	devnode->minor = -1;
> > +
> >  	return ret;
> >  }
> >  
> > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> > index 6d46c658be21..d050f54f252a 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -60,7 +60,7 @@ struct media_file_operations {
> >   * @dev:	pointer to struct &device containing the media controller device
> >   * @cdev:	struct cdev pointer character device
> >   * @parent:	parent device
> > - * @minor:	device node minor number
> > + * @minor:	device node minor number, -1 if unassigned
> >   * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
> >   * @release:	release callback called at the end of ``media_devnode_release()``
> >   *		routine at media-device.c.
> 

-- 
Regards,

Sakari Ailus