[media] media-entity: Don't use var length arrays
Commit Message
The graph traversal algorithm currently uses two variable-length
arrays that are dynamically allocated at the stack:
drivers/media/media-entity.c:238:17: warning: Variable length array is used.
drivers/media/media-entity.c:239:17: warning: Variable length array is used.
Those are bad, specially if the number of pads for some entity would
be bigger than a certain amount, as it would cause a Linux stack
overflow.
We could just replace entity->num_pads by a define in order to solve
the above sparse warnings, but that would still prevent having entities
with a big number of pads. So, let's do the right thing and use a
kalloced var to store those ancillary bitmaps, e. g. put them at
struct media_device, and use an arbitrary big number that will work
for all currently known usecases.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Comments
Hi Mauro,
Mauro Carvalho Chehab wrote:
> The graph traversal algorithm currently uses two variable-length
> arrays that are dynamically allocated at the stack:
>
> drivers/media/media-entity.c:238:17: warning: Variable length array is used.
> drivers/media/media-entity.c:239:17: warning: Variable length array is used.
>
> Those are bad, specially if the number of pads for some entity would
> be bigger than a certain amount, as it would cause a Linux stack
> overflow.
>
> We could just replace entity->num_pads by a define in order to solve
> the above sparse warnings, but that would still prevent having entities
> with a big number of pads. So, let's do the right thing and use a
> kalloced var to store those ancillary bitmaps, e. g. put them at
> struct media_device, and use an arbitrary big number that will work
> for all currently known usecases.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 153a46469814..02842d875809 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -235,8 +235,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> media_entity_graph_walk_start(&graph, 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++;
> @@ -250,8 +248,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> if (!entity->ops || !entity->ops->link_validate)
> continue;
>
> - bitmap_zero(active, entity->num_pads);
> - bitmap_fill(has_no_links, entity->num_pads);
> + bitmap_zero(mdev->active, entity->num_pads);
> + bitmap_fill(mdev->has_no_links, entity->num_pads);
>
> for (i = 0; i < entity->num_links; i++) {
> struct media_link *link = &entity->links[i];
> @@ -259,7 +257,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> ? link->sink : link->source;
>
> /* Mark that a pad is connected by a link. */
> - bitmap_clear(has_no_links, pad->index, 1);
> + bitmap_clear(mdev->has_no_links, pad->index, 1);
>
> /*
> * Pads that either do not need to connect or
> @@ -268,7 +266,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> */
> if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
> link->flags & MEDIA_LNK_FL_ENABLED)
> - bitmap_set(active, pad->index, 1);
> + bitmap_set(mdev->active, pad->index, 1);
>
> /*
> * Link validation will only take place for
> @@ -290,15 +288,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> }
>
> /* Either no links or validated links are fine. */
> - bitmap_or(active, active, has_no_links, entity->num_pads);
> + bitmap_or(mdev->active, mdev->active, mdev->has_no_links,
> + entity->num_pads);
>
> - if (!bitmap_full(active, entity->num_pads)) {
> + if (!bitmap_full(mdev->active, entity->num_pads)) {
> ret = -EPIPE;
> dev_dbg(entity->parent->dev,
> "\"%s\":%u must be connected by an enabled link\n",
> entity->name,
> (unsigned)find_first_zero_bit(
> - active, entity->num_pads));
> + mdev->active, entity->num_pads));
> goto error;
> }
> }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 6e6db78f1ee2..1ac132af031b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -32,6 +32,10 @@
>
> struct device;
>
> +/* Maximum number of pads per entity */
> +
> +#define MEDIA_MAX_PADS 512
> +
> /**
> * struct media_device - Media device
> * @dev: Parent device
> @@ -78,6 +82,11 @@ struct media_device {
>
> int (*link_notify)(struct media_link *link, u32 flags,
> unsigned int notification);
> +
> + /* private: ancillary bitmaps used by graph traversal algo */
> +
> + DECLARE_BITMAP(active, MEDIA_MAX_PADS);
> + DECLARE_BITMAP(has_no_links, MEDIA_MAX_PADS);
> };
>
> /* Supported link_notify @notification values. */
>
I think media_entity_init() should check the number of pads will not
exceed MEDIA_MAX_PADS.
With that change,
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
@@ -235,8 +235,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
media_entity_graph_walk_start(&graph, 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++;
@@ -250,8 +248,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
if (!entity->ops || !entity->ops->link_validate)
continue;
- bitmap_zero(active, entity->num_pads);
- bitmap_fill(has_no_links, entity->num_pads);
+ bitmap_zero(mdev->active, entity->num_pads);
+ bitmap_fill(mdev->has_no_links, entity->num_pads);
for (i = 0; i < entity->num_links; i++) {
struct media_link *link = &entity->links[i];
@@ -259,7 +257,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
? link->sink : link->source;
/* Mark that a pad is connected by a link. */
- bitmap_clear(has_no_links, pad->index, 1);
+ bitmap_clear(mdev->has_no_links, pad->index, 1);
/*
* Pads that either do not need to connect or
@@ -268,7 +266,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
*/
if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
link->flags & MEDIA_LNK_FL_ENABLED)
- bitmap_set(active, pad->index, 1);
+ bitmap_set(mdev->active, pad->index, 1);
/*
* Link validation will only take place for
@@ -290,15 +288,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
}
/* Either no links or validated links are fine. */
- bitmap_or(active, active, has_no_links, entity->num_pads);
+ bitmap_or(mdev->active, mdev->active, mdev->has_no_links,
+ entity->num_pads);
- if (!bitmap_full(active, entity->num_pads)) {
+ if (!bitmap_full(mdev->active, entity->num_pads)) {
ret = -EPIPE;
dev_dbg(entity->parent->dev,
"\"%s\":%u must be connected by an enabled link\n",
entity->name,
(unsigned)find_first_zero_bit(
- active, entity->num_pads));
+ mdev->active, entity->num_pads));
goto error;
}
}
@@ -32,6 +32,10 @@
struct device;
+/* Maximum number of pads per entity */
+
+#define MEDIA_MAX_PADS 512
+
/**
* struct media_device - Media device
* @dev: Parent device
@@ -78,6 +82,11 @@ struct media_device {
int (*link_notify)(struct media_link *link, u32 flags,
unsigned int notification);
+
+ /* private: ancillary bitmaps used by graph traversal algo */
+
+ DECLARE_BITMAP(active, MEDIA_MAX_PADS);
+ DECLARE_BITMAP(has_no_links, MEDIA_MAX_PADS);
};
/* Supported link_notify @notification values. */