Hello Javier,
(Resending as I've replied by mistake to the version of the patch you had sent
to the media workshop list only)
Thank you for the patch.
On Monday 12 October 2015 13:44:11 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>
> The media_entity_cleanup() function only cleans up the entity links list
> but this operation is already made in media_device_unregister_entity().
>
> In most cases this should be harmless (besides having duplicated code)
> since the links list would be empty so the iteration would not happen
> but the links list is initialized in media_device_register_entity() so
> if a driver fails to register an entity with a media device and clean up
> the entity in the error path, a NULL deference pointer error will happen.
>
> So don't try to empty the links list in media_entity_cleanup() since
> is either done already or haven't been initialized yet.
Does this mean that it's an invalid usage of the API to create links before
registering entities ? If so it should be clearly documented somewhere, such
as in the kerneldoc of the media_create_pad_link() function.
And yes, that means that all exported API functions need kerneldoc. Sorry for
being a killjoy
On a related note, we need to solve the userspace API race caused by
registering the MC devnode before all entities and links are created an
registered. It's not a new issue so I won't call for fixing it as part of this
patch series, but we'll need to fix that with the dynamic graph update
implementation at the latest. It will likely require reworking the
initialization and registration sequences.
> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> ---
> drivers/media/media-entity.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 2c984fb7d497..eaeda2589ce5 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -268,13 +268,6 @@ EXPORT_SYMBOL_GPL(media_entity_init);
> void
> media_entity_cleanup(struct media_entity *entity)
> {
> - struct media_link *link, *tmp;
> -
> - list_for_each_entry_safe(link, tmp, &entity->links, list) {
> - media_gobj_remove(&link->graph_obj);
> - list_del(&link->list);
> - kfree(link);
> - }
> }
> EXPORT_SYMBOL_GPL(media_entity_cleanup);
As media_entity_cleanup is now empty I'd turn it into a static inline. We need
to keep the function in case cleanup ends up being needed later, but there's
no reason not to optimize the call away for now.
--
Regards,
Laurent Pinchart
--
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
