Re: [PATCH RFC v2 08/16] media: convert links from array to list
On 08/07/15 16:20, Mauro Carvalho Chehab wrote:
Using memory realloc to increase the size of an array
is complex and makes harder to remove links. Also, by
embedding the link inside an array at the entity makes harder
to change the code to add interfaces, as interfaces will
also need to use links.
So, convert the links from arrays to lists.
Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9fb3f8958265..a95ca981aabb 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
snip
@@ -452,27 +445,21 @@ EXPORT_SYMBOL_GPL(media_entity_put);
static struct media_link *media_entity_add_link(struct media_entity *entity)
{
- if (entity-num_links = entity-max_links) {
- struct media_link *links = entity-links;
- unsigned int max_links = entity-max_links + 2;
- unsigned int i;
+ struct media_link *link;
- links = krealloc(links, max_links * sizeof(*links), GFP_KERNEL);
- if (links == NULL)
- return NULL;
+ link = kzalloc(sizeof(*link), GFP_KERNEL);
+ if (link == NULL)
+ return NULL;
- for (i = 0; i entity-num_links; i++)
- links[i].reverse-reverse = links[i];
-
- entity-max_links = max_links;
- entity-links = links;
- }
+ link-reverse-reverse = link;
Huh? link points to a zeroed struct, so link-reverse will be NULL.
This can't work.
Are you sure this line should be here? The original code doesn't set it
either for the new link, it just updates the reverse links for the
realloced links.
+ INIT_LIST_HEAD(link-list);
+ list_add(entity-links, link-list);
/* Initialize graph object embedded at the new link */
graph_obj_init(entity-parent, MEDIA_GRAPH_LINK,
- entity-links[entity-num_links].graph_obj);
+ link-graph_obj);
- return entity-links[entity-num_links++];
+ return link;
}
int
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 08/16] media: convert links from array to list
Em Tue, 11 Aug 2015 12:49:40 +0200
Hans Verkuil hverk...@xs4all.nl escreveu:
On 08/07/15 16:20, Mauro Carvalho Chehab wrote:
Using memory realloc to increase the size of an array
is complex and makes harder to remove links. Also, by
embedding the link inside an array at the entity makes harder
to change the code to add interfaces, as interfaces will
also need to use links.
So, convert the links from arrays to lists.
Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9fb3f8958265..a95ca981aabb 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
snip
@@ -452,27 +445,21 @@ EXPORT_SYMBOL_GPL(media_entity_put);
static struct media_link *media_entity_add_link(struct media_entity
*entity)
{
- if (entity-num_links = entity-max_links) {
- struct media_link *links = entity-links;
- unsigned int max_links = entity-max_links + 2;
- unsigned int i;
+ struct media_link *link;
- links = krealloc(links, max_links * sizeof(*links), GFP_KERNEL);
- if (links == NULL)
- return NULL;
+ link = kzalloc(sizeof(*link), GFP_KERNEL);
+ if (link == NULL)
+ return NULL;
- for (i = 0; i entity-num_links; i++)
- links[i].reverse-reverse = links[i];
-
- entity-max_links = max_links;
- entity-links = links;
- }
+ link-reverse-reverse = link;
Huh? link points to a zeroed struct, so link-reverse will be NULL.
This can't work.
Are you sure this line should be here? The original code doesn't set it
either for the new link, it just updates the reverse links for the
realloced links.
You're right. I think I can just remove that line.
I had to confess that I didn't understand well that the reverse links
stuff. I mean, IMHO, the entire concept of storing a second copy of
the link, calling it as reverse on some places and as backlink
on others is messy, and I guess we should get rid of duplicating the
number of links for no good reason.
It is easy to just add the same link to a list at the other entity,
and keep the link just once in the memory.
+ INIT_LIST_HEAD(link-list);
+ list_add(entity-links, link-list);
/* Initialize graph object embedded at the new link */
graph_obj_init(entity-parent, MEDIA_GRAPH_LINK,
- entity-links[entity-num_links].graph_obj);
+ link-graph_obj);
- return entity-links[entity-num_links++];
+ return link;
}
int
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC v2 08/16] media: convert links from array to list
Using memory realloc to increase the size of an array
is complex and makes harder to remove links. Also, by
embedding the link inside an array at the entity makes harder
to change the code to add interfaces, as interfaces will
also need to use links.
So, convert the links from arrays to lists.
Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9fb3f8958265..a95ca981aabb 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
#include media/media-device.h
#include media/media-devnode.h
@@ -148,22 +149,22 @@ 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;
- 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;
/* 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++;
@@ -471,6 +472,7 @@ void media_device_unregister_entity(struct media_entity
*entity)
{
int i;
struct media_device *mdev = entity-parent;
+ struct media_link *link, *tmp;
if (mdev == NULL)
return;
@@ -479,8 +481,11 @@ void media_device_unregister_entity(struct media_entity
*entity)
graph_obj_remove(entity-graph_obj);
for (i = 0; entity-num_pads; i++)
graph_obj_remove(entity-pads[i].graph_obj);
- for (i = 0; entity-num_links; i++)
- graph_obj_remove(entity-links[i].graph_obj);
+ list_for_each_entry_safe(link, tmp, entity-links, list) {
+ graph_obj_remove(link-graph_obj);
+ list_del(link-list);
+ kfree(link);
+ }
list_del(entity-list);
spin_unlock(mdev-lock);
entity-parent = NULL;
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e8451a4a403b..4a42ccfeffab 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -76,14 +76,6 @@ void graph_obj_remove(struct media_graph_obj *gobj)
* use cases the entity driver can guess the number of links which can safely
* be assumed to be equal to or larger than the number of pads.
*
- * For those reasons the links array can be preallocated based on the entity
- * driver guess and will be reallocated later if extra links need to be
- * created.
- *
- * This function allocates a links array with enough space to hold at least
- * 'num_pads' + 'extra_links' elements. The media_entity::max_links field will
- * be set to the number of allocated elements.
- *
* The pads array is managed by the entity driver and passed to
* media_entity_init() where its pointer will be stored in the entity
structure.
*/
@@ -91,21 +83,14 @@ 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;
-
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;
+ INIT_LIST_HEAD(entity-links);
for (i = 0; i num_pads; i++) {
pads[i].entity = entity;
@@ -119,7 +104,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) {
+
