Em Mon, 23 Nov 2015 13:26:30 -0200 Mauro Carvalho Chehab mchehab@osg.samsung.com escreveu:
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 0d85c6c28004..3e649cacfc07 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -25,6 +25,7 @@ #include <linux/ioctl.h> #include <linux/media.h> #include <linux/types.h> +#include <linux/slab.h>
Could you please keep headers sorted alphabetically ?
Ok, I'll reorder on a later patch.
Done. Will send the patch.
@@ -150,22 +151,21 @@ static long __media_device_enum_links(struct media_device *mdev, }
if (links->links) {
struct media_link_desc __user *ulink;
unsigned int l;
struct media_link *ent_link;
struct media_link_desc __user *ulink = links->links;
Might be slightly nitpicking, but I think variables would be more coherent if they were called
struct media_link_desc __user *ulink = links->links; struct media_link *link;
Nomenclatures tend to generate endless discussions ;)
IMO, calling it as "link" here is confusing, as it is not clear if this is a Kernel or an Userspace "link"...
...
for (l = 0, ulink = links->links; l < entity->num_links; l++) {
list_for_each_entry(ent_link, &entity->links, list) { struct media_link_desc link;
and
struct media_link_desc klink;
and calling it as "klink" is confusing to me ;) as this is the struct defined at the userspace API, and not the struct defined at the Kernelspace ABI.
Perhaps we could call those media_link_desc as "klink_desc" and "ulink_desc", instead.
Done. Will send the patch.
here.
/* Ignore backlinks. */
if (entity->links[l].source->entity != entity)
if (ent_link->source->entity != entity) continue;
memset(&link, 0, sizeof(link));
media_device_kpad_to_upad(entity->links[l].source,
media_device_kpad_to_upad(ent_link->source, &link.source);
media_device_kpad_to_upad(entity->links[l].sink,
media_device_kpad_to_upad(ent_link->sink, &link.sink);
link.flags = entity->links[l].flags;
link.flags = ent_link->flags; if (copy_to_user(ulink, &link, sizeof(*ulink))) return -EFAULT; ulink++;
@@ -437,6 +437,7 @@ int __must_check media_device_register_entity(struct media_device *mdev, /* Warn if we apparently re-register an entity */ WARN_ON(entity->graph_obj.mdev != NULL); entity->graph_obj.mdev = mdev;
- INIT_LIST_HEAD(&entity->links);
I'd move this to media_entity_init(). I've spent time wondering how the code could work without crashing during testing as the list wasn't initialized in media_entity_init().
I wrote this code lots of months ago... I guess there was a reason for this to be here, and not there, but I can't remember why.
I'll give it a try.
I remembered the reason. The problem here is that, despite media_entity_init() name, what this function actually does is to create the PAD objects. If an entity doesn't have any PAD, this function doesn't need to be called. There is at least one place where this function is not called.
What I propose here is to rename this function to media_entity_register_pads() in order to better reflect what the function actually does.
I'm sending such patch.
Speaking of testing, have you checked for memory leaks with kmemleak ? Given the extent of the rework I think this should really be tested.
No, I didn't. I'm not sure if au0828 currently passes on kmemleak or not. What I did is I checked that all created graph objects were removed, via enabling the dynamic_prinks at the graph object init/remove functions.
This was addressed during the final rebase.
spin_lock(&mdev->lock); /* Initialize media_gobj embedded at the entity */ @@ -465,13 +466,17 @@ void media_device_unregister_entity(struct media_entity *entity) { int i; struct media_device *mdev = entity->graph_obj.mdev;
struct media_link *link, *tmp;
if (mdev == NULL) return;
spin_lock(&mdev->lock);
- for (i = 0; i < entity->num_links; i++)
media_gobj_remove(&entity->links[i].graph_obj);
- list_for_each_entry_safe(link, tmp, &entity->links, list) {
media_gobj_remove(&link->graph_obj);
list_del(&link->list);
kfree(link);
Shouldn't you remove the backlinks too ? How about calling __media_entity_remove_link() to centralize the link removal code ?
Yes. I'll use __media_entity_remove_link() here.
This was addressed during the final rebase.
- } for (i = 0; i < entity->num_pads; i++) media_gobj_remove(&entity->pads[i].graph_obj); media_gobj_remove(&entity->graph_obj);
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index 0926f08be981..d5efa0e2c88c 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -221,21 +221,13 @@ int media_entity_init(struct media_entity *entity, u16 num_pads, struct media_pad *pads) {
struct media_link *links;
unsigned int max_links = num_pads; unsigned int i;
links = kzalloc(max_links * sizeof(links[0]), GFP_KERNEL);
if (links == NULL)
return -ENOMEM;
Now that the function doesn't allocate links anymore you should fix its kerneldoc that still mentions preallocation.
OK.
Fixed. I moved the media-framework.txt to kerneldoc and fixed it to reflect the current status of the media controller.
I'll send the patches for it too.
entity->group_id = 0;
entity->max_links = max_links; entity->num_links = 0; entity->num_backlinks = 0; entity->num_pads = num_pads; entity->pads = pads;
entity->links = links;
for (i = 0; i < num_pads; i++) { pads[i].entity = entity;
@@ -249,7 +241,13 @@ EXPORT_SYMBOL_GPL(media_entity_init); void media_entity_cleanup(struct media_entity *entity) {
- kfree(entity->links);
- 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);
@@ -275,7 +273,7 @@ static void stack_push(struct media_entity_graph *graph, return; } graph->top++;
- graph->stack[graph->top].link = 0;
- graph->stack[graph->top].link = (&entity->links)->next;
Anything wrong with entity->links.next ?
No, but I use a regex to find all the occurrences of the previous struct ;)
I'll change it.
I asked Javier to do this change.
graph->stack[graph->top].entity = entity; }
@@ -317,6 +315,7 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph, } EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
No need for an extra blank line.
OK.
Done on a separate patch.
/**
- media_entity_graph_walk_next - Get the next entity in the graph
- @graph: Media graph structure
@@ -340,14 +339,16 @@ media_entity_graph_walk_next(struct media_entity_graph *graph) * top of the stack until no more entities on the level can be * found. */
- while (link_top(graph) < stack_top(graph)->num_links) {
- while (link_top(graph) != &(stack_top(graph)->links)) {
No need for parentheses around the second operand of !=.
Ok, I'll remove it.
Javier will handle this one too.
struct media_entity *entity = stack_top(graph);
struct media_link *link = &entity->links[link_top(graph)];
struct media_link *link;
struct media_entity *next;
link = list_entry(link_top(graph), typeof(*link), list);
/* The link is not enabled so we do not follow. */ if (!(link->flags & MEDIA_LNK_FL_ENABLED)) {
link_top(graph)++;
}link_top(graph) = link_top(graph)->next; continue;
[snip]
@@ -395,6 +396,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, struct media_device *mdev = entity->graph_obj.mdev; struct media_entity_graph graph; struct media_entity *entity_err = entity;
- struct media_link *link;
Nitpicking, I would have placed the variable declaration inside of the while loop, where i was declared.
OK.
In this specific case, I would keep it here, as link is used by the entire routine. Doing it inside the loop would cause it to be realocated every time. Also, it would conflict with Sakari changes to this part of the code.
So, better to keep it as-is, IMHO.
int ret;
mutex_lock(&mdev->graph_mutex); @@ -404,7 +406,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, while ((entity = media_entity_graph_walk_next(&graph))) { DECLARE_BITMAP(active, entity->num_pads); DECLARE_BITMAP(has_no_links, entity->num_pads);
unsigned int i;
entity->stream_count++; WARN_ON(entity->pipe && entity->pipe != pipe);
@@ -420,8 +421,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, bitmap_zero(active, entity->num_pads); bitmap_fill(has_no_links, entity->num_pads);
for (i = 0; i < entity->num_links; i++) {
struct media_link *link = &entity->links[i];
list_for_each_entry(link, &entity->links, list) { struct media_pad *pad = link->sink->entity == entity ? link->sink : link->source;
[snip]
+static void __media_entity_remove_link(struct media_entity *entity,
struct media_link *link);
No forward declaration please, let's reorder functions instead.
OK.
Done.
int media_create_pad_link(struct media_entity *source, u16 source_pad, struct media_entity *sink, u16 sink_pad, u32 flags)
[snip]
-void __media_entity_remove_links(struct media_entity *entity) +static void __media_entity_remove_link(struct media_entity *entity,
struct media_link *link)
No need for a __ in the function name, there's not media_entity_remove_link() function.
The idea is to distinguish the non-lock protected function from the protected ones: all non-locked functions are called with the __ prefix.
In this specific case, at least for now, we don't have, for now, any usage for a lock-protected media_entity_remove_link() because we only remove links when either removing entities or calling the lock-protected media_entity_remove_links().
Yet, as soon as we start using dynamic link removal, we'll need a locked version of it. So, I think we should keep it named as-is.
We ended by needing this at media_device_entity_unregister(). So, I actually exported this function.
{
- unsigned int i;
- struct media_link *rlink, *tmp;
- struct media_entity *remote;
- unsigned int r = 0;
- if (link->source->entity == entity)
remote = link->sink->entity;
- else
remote = link->source->entity;
- for (i = 0; i < entity->num_links; i++) {
struct media_link *link = &entity->links[i];
struct media_entity *remote;
unsigned int r = 0;
- list_for_each_entry_safe(rlink, tmp, &remote->links, list) {
if (rlink != link->reverse) {
r++;
The variable is incremented here but otherwise never used, you can remove it.
OK.
Done.
continue;
}
if (link->source->entity == entity)
remote = link->sink->entity;
else
remote = link->source->entity;
remote->num_backlinks--;
while (r < remote->num_links) {
struct media_link *rlink = &remote->links[r];
if (rlink != link->reverse) {
r++;
continue;
}
if (--remote->num_links == 0)
break;
if (link->source->entity == entity)
remote->num_backlinks--;
/* Remove the remote link */
Shouldn't you call media_gobj_remove() ?
Yes, and I'm pretty sure it was called on some earlier version... My guess is that some rebase was badly solved and the call for media_gobj_remove() got lot on some rebase.
I'll re-add it.
This got fixed during the final rebase.
list_del(&rlink->list);
kfree(rlink);
- }
And here too ?
Yes. I'll call it.
And here too.
- list_del(&link->list);
- kfree(link);
+}
if (--remote->num_links == 0)
break;
+void __media_entity_remove_links(struct media_entity *entity) +{
- struct media_link *link, *tmp;
/* Insert last entry in place of the dropped link. */
*rlink = remote->links[remote->num_links];
}
- }
list_for_each_entry_safe(link, tmp, &entity->links, list)
__media_entity_remove_link(entity, link);
entity->num_links = 0; entity->num_backlinks = 0;
Thanks, Mauro