Hello Laurent,
On 11/23/2015 10:41 AM, Laurent Pinchart wrote:
Hello Javier,
Thank you for the patch.
Thanks a lot for your review and feedback.
On Monday 12 October 2015 13:44:11 Mauro Carvalho Chehab wrote:
From: Javier Martinez Canillas javier@osg.samsung.com
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
Yes, with the changes in the MC core, the links can't be created before registering the entities with the media device (because the mdev object IDs are needed when creating the links).
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 :-)
Agreed, it was decided to leave the documentation out for now until the MC next gen changes settle to avoid having to re-work it on every spin.
But for sure this new behavior will be clearly documented once the changes are agreed upon.
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.
Yes, that user-space API race has been already fixed by the series:
[PATCH v4 0/2] [media] Fix race between graph enumeration and entities registration
https://lkml.org/lkml/2015/9/15/371
but as you said, is not part of the series and can be picked as a follow up (although I added this series as a dependency just to avoid merge conflicts).
Signed-off-by: Javier Martinez Canillas javier@osg.samsung.com
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.
Agreed.
Best regards,