| Message ID | 20231220103713.113386-19-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Media device lifetime management | expand |
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. > > + /* 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(). > + > + /* 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. > 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);
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); >
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"); + /* 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); + } + 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); 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);