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@xxxxxxxxxxxxxxx>
> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> 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);
--
Regards,
Laurent Pinchart
