Hi Sylwester,
Thanks for the patch.
On Fri, May 10, 2013 at 12:00:37PM +0200, Sylwester Nawrocki wrote:
> This function allows to remove all media entity's links to other
> entities, leaving no references to a media entity's links array
> at its remote entities.
>
> Currently when a driver of some entity is removed it will free its
> media entities links[] array, leaving dangling pointers at other
> entities that are part of same media graph. This is troublesome when
> drivers of a media device entities are in separate kernel modules,
> removing only some modules will leave others in incorrect state.
>
> This function is intended to be used when an entity is being
> unregistered from a media device.
>
> With an assumption that media links should be created only after
> they are registered to a media device and with the graph mutex held.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> [locking error in the initial patch version]
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
> Changes since the initial version:
> - fixed erroneous double mutex lock in media_entity_links_remove()
> function.
>
> drivers/media/media-entity.c | 51 ++++++++++++++++++++++++++++++++++++++++++
> include/media/media-entity.h | 3 +++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e1cd132..bd85dc3 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -429,6 +429,57 @@ media_entity_create_link(struct media_entity *source, u16 source_pad,
> }
> EXPORT_SYMBOL_GPL(media_entity_create_link);
>
> +void __media_entity_remove_links(struct media_entity *entity)
> +{
> + int i, r;
u16? r can be defined inside the loop.
> + for (i = 0; i < entity->num_links; i++) {
> + struct media_link *link = &entity->links[i];
> + struct media_entity *remote;
> + int num_links;
num_links is u16 in struct media_entity. I'd use the same type.
> + if (link->source->entity == entity)
> + remote = link->sink->entity;
> + else
> + remote = link->source->entity;
> +
> + num_links = remote->num_links;
> +
> + for (r = 0; r < num_links; r++) {
Is caching remote->num_links needed, or do I miss something?
> + struct media_link *rlink = &remote->links[r];
> +
> + if (rlink != link->reverse)
> + continue;
> +
> + if (link->source->entity == entity)
> + remote->num_backlinks--;
> +
> + remote->num_links--;
> +
> + if (remote->num_links < 1)
How about: if (!remote->num_links) ?
> + break;
> +
> + /* Insert last entry in place of the dropped link. */
> + remote->links[r--] = remote->links[remote->num_links];
> + }
> + }
> +
> + entity->num_links = 0;
> + entity->num_backlinks = 0;
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_remove_links);
> +
> +void media_entity_remove_links(struct media_entity *entity)
> +{
> + if (WARN_ON_ONCE(entity->parent == NULL))
> + return;
> +
> + mutex_lock(&entity->parent->graph_mutex);
> + __media_entity_remove_links(entity);
> + mutex_unlock(&entity->parent->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_entity_remove_links);
> +
> static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
> {
> int ret;
--
Cheers,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
