Hi Sakari,
Thanks a lot for the review.
On 06/06/2013 09:41 PM, Sakari Ailus wrote:
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.
I would argue 'unsigned int' would be more optimal type for i in most
cases. Will move r 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.
I would go with 'unsigned int', as a more natural type for the CPU in
most cases. It's a minor issue, but I can't see how u16 would have been
more optimal than unsigned int for a local variable like this, while
this code is mostly used on 32-bit systems at least.
+ 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?
Yes, it is needed, remote->num_links is decremented inside the loop.
+ 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) ?
Hmm, perhaps squashing the above two lines to:
if (--remote->num_links == 0)
?
+ 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;
This WARN_ON_ONCE() is a bit problematic, I'm going to remove it in the
next
iteration. I found that tracking entity->parent without races is not so
straightforward in drivers currently, and this needs to be taken care of
before we can have something like asynchronous subdevice registration.
+ mutex_lock(&entity->parent->graph_mutex);
+ __media_entity_remove_links(entity);
+ mutex_unlock(&entity->parent->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_entity_remove_links);
Regards,
Sylwester
--
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
