Re: [PATCH v2 18/29] media: mc: Postpone graph object removal until free

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

Hi Laurent,

Thanks for the comments. Apologies for missing this earlier.

On Wed, Feb 07, 2024 at 04:18:20PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Dec 20, 2023 at 12:37:02PM +0200, Sakari Ailus wrote:
> > The media device itself will be unregistered based on it being unbound and
> > driver's remove callback being called. The graph objects themselves may
> > still be in use; rely on the media device release callback to release
> > them.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/mc/mc-device.c | 53 +++++++++++++++++-------------------
> >  1 file changed, 25 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index bbc233e726d2..10426c2796b6 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -702,8 +702,33 @@ EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> >  
> >  static void __media_device_release(struct media_device *mdev)
> >  {
> > +	struct media_entity *entity;
> > +	struct media_entity *next;
> > +	struct media_interface *intf, *tmp_intf;
> > +	struct media_entity_notify *notify, *nextp;
> > +
> >  	dev_dbg(mdev->dev, "Media device released\n");
> 
> No need for locking ? I suppose we can't reach this point if someone
> else has a reference to the media device. A comment to mention it would
> be nice.

There's indeed no point in locking a mutex in memory that's about to get
released. In fact, the mutex is about to get destroyed first. Other release
callback don't have comments, although the purpose of
__media_device_release wasn't that of an ordinary release callback. Still,
bygones are bygones.

> 
> >  
> > +	/* Remove all entities from the media device */
> > +	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> > +		__media_device_unregister_entity(entity);
> 
> Should the __media_device_unregister_entity() function be renamed to
> __media_device_remove_entity() (in a separate patch) ? Same for
> __media_device_unregister_entity_notify().

"unregister" pairs with "register" so at least for now I'd prefer to keep
the naming as-is. I'm fine discussing the naming after this set though.

> 
> > +
> > +	/* Remove all entity_notify callbacks from the media device */
> > +	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
> > +		__media_device_unregister_entity_notify(mdev, notify);
> > +
> > +	/* Remove all interfaces from the media device */
> > +	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> > +				 graph_obj.list) {
> > +		/*
> > +		 * Unlink the interface, but don't free it here; the
> > +		 * module which created it is responsible for freeing
> > +		 * it
> > +		 */
> > +		__media_remove_intf_links(intf);
> > +		media_gobj_destroy(&intf->graph_obj);
> > +	}
> > +
> >  	ida_destroy(&mdev->entity_internal_idx);
> >  	mdev->entity_internal_idx_max = 0;
> >  	media_graph_walk_cleanup(&mdev->pm_count_walk);
> > @@ -787,42 +812,14 @@ EXPORT_SYMBOL_GPL(__media_device_register);
> >  
> >  void media_device_unregister(struct media_device *mdev)
> >  {
> > -	struct media_entity *entity;
> > -	struct media_entity *next;
> > -	struct media_interface *intf, *tmp_intf;
> > -	struct media_entity_notify *notify, *nextp;
> > -
> >  	if (mdev == NULL)
> >  		return;
> >  
> >  	mutex_lock(&mdev->graph_mutex);
> > -
> > -	/* Check if mdev was ever registered at all */
> >  	if (!media_devnode_is_registered(&mdev->devnode)) {
> >  		mutex_unlock(&mdev->graph_mutex);
> 
> Unless I'm mistaken we don't need to lock the graph mutext to test this,
> so I think you can drop locking completely here.

There may be IOCTL calls in progress while unregister takes place. The test
seems to be fine outside the lock but the section below still needs the
lock.

I'll change this for v4.

> 
> >  		return;
> >  	}
> > -
> > -	/* Remove all entities from the media device */
> > -	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> > -		__media_device_unregister_entity(entity);
> > -
> > -	/* Remove all entity_notify callbacks from the media device */
> > -	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
> > -		__media_device_unregister_entity_notify(mdev, notify);
> > -
> > -	/* Remove all interfaces from the media device */
> > -	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> > -				 graph_obj.list) {
> > -		/*
> > -		 * Unlink the interface, but don't free it here; the
> > -		 * module which created it is responsible for freeing
> > -		 * it
> > -		 */
> > -		__media_remove_intf_links(intf);
> > -		media_gobj_destroy(&intf->graph_obj);
> > -	}
> > -
> >  	mutex_unlock(&mdev->graph_mutex);
> >  
> >  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> 

-- 
Kind regards,

Sakari Ailus