[PATCH v2 0/2] Add iterator for an entity's data links

[PATCH v2 0/2] Add iterator for an entity's data links

[Daniel Scally]

There are a bunch of places in the kernel where code iterates over an entity's
links to perform some action. Those almost invariably have the implicit
assumption that those links are data links, which might not be true following
the introduction of ancillary links. Add a dedicated iterator that skips any
non data links for use instead, which will allow that assumption to hold true.

Daniel Scally (2):
  media: entity: Add iterator for entity data links
  media: entity: Use dedicated data link iterator

 drivers/media/mc/mc-entity.c | 22 +++++++++++++++++++---
 include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] media: entity: Add iterator for entity data links

[Daniel Scally]

Iterating over the links for an entity is a somewhat common need
through the media subsystem, but generally the assumption is that
they will all be data links. To meet that assumption add a new macro
that iterates through an entity's links and skips non-data links.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes since v1 (suggested by Jacopo and Laurent):

	- Simplified __media_entity_next_link()
	- Added the link_type param to __media_entity_next_link()
	- Moved __media_entity_next_link() to mc-entity.c rather than
	  media-entity.h

 drivers/media/mc/mc-entity.c | 16 ++++++++++++++++
 include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 11f5207f73aa..a247d5679930 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bitmap.h>
+#include <linux/list.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <media/media-entity.h>
@@ -1051,3 +1052,18 @@ struct media_link *media_create_ancillary_link(struct media_entity *primary,
 	return link;
 }
 EXPORT_SYMBOL_GPL(media_create_ancillary_link);
+
+struct media_link *__media_entity_next_link(struct media_entity *entity,
+					    struct media_link *link,
+					    unsigned long link_type)
+{
+	link = link ? list_next_entry(link, list)
+		    : list_first_entry(&entity->links, typeof(*link), list);
+
+	list_for_each_entry_from(link, &entity->links, list)
+		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == link_type)
+			return link;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__media_entity_next_link);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index a9a1c0ec5d1c..903c9368c50d 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -1140,4 +1140,33 @@ struct media_link *
 media_create_ancillary_link(struct media_entity *primary,
 			    struct media_entity *ancillary);
 
+/**
+ * __media_entity_next_link() - iterate through a &media_entity's links,
+ *
+ * @entity:	pointer to the &media_entity
+ * @link:	pointer to a &media_link to hold the iterated values
+ * @link_type:	one of the MEDIA_LNK_FL_LINK_TYPE flags
+ *
+ * Return the next link against an entity matching a specific link type. This
+ * allows iteration through an entity's links whilst guaranteeing all of the
+ * returned links are of the given type.
+ */
+struct media_link *__media_entity_next_link(struct media_entity *entity,
+					    struct media_link *link,
+					    unsigned long link_type);
+
+/**
+ * for_each_media_entity_data_link() - Iterate through an entity's data links
+ *
+ * @entity:	pointer to the &media_entity
+ * @link:	pointer to a &media_link to hold the iterated values
+ *
+ * Iterate over a &media_entity's data links
+ */
+#define for_each_media_entity_data_link(entity, link)			\
+	for (link = __media_entity_next_link(entity, NULL,		\
+					     MEDIA_LNK_FL_DATA_LINK);	\
+	     link;							\
+	     link = __media_entity_next_link(entity, link,		\
+					     MEDIA_LNK_FL_DATA_LINK))
 #endif
-- 
2.25.1

[PATCH v2 2/2] media: entity: Use dedicated data link iterator

[Daniel Scally]

Where iteration over links for an entity is clearly assuming that
all of those links are data links, use the new iterator to guarantee
that that assumption is met.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/mc/mc-entity.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index a247d5679930..cebb905260d3 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -450,7 +450,7 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
 		bitmap_zero(active, entity->num_pads);
 		bitmap_fill(has_no_links, entity->num_pads);
 
-		list_for_each_entry(link, &entity->links, list) {
+		for_each_media_entity_data_link(entity, link) {
 			struct media_pad *pad = link->sink->entity == entity
 						? link->sink : link->source;
 
@@ -889,7 +889,7 @@ media_entity_find_link(struct media_pad *source, struct media_pad *sink)
 {
 	struct media_link *link;
 
-	list_for_each_entry(link, &source->entity->links, list) {
+	for_each_media_entity_data_link(source->entity, link) {
 		if (link->source->entity == source->entity &&
 		    link->source->index == source->index &&
 		    link->sink->entity == sink->entity &&
@@ -905,7 +905,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad)
 {
 	struct media_link *link;
 
-	list_for_each_entry(link, &pad->entity->links, list) {
+	for_each_media_entity_data_link(pad->entity, link) {
 		if (!(link->flags & MEDIA_LNK_FL_ENABLED))
 			continue;
 
-- 
2.25.1

Re: [PATCH v2 1/2] media: entity: Add iterator for entity data links

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thu, Jul 07, 2022 at 11:47:32PM +0100, Daniel Scally wrote:
> Iterating over the links for an entity is a somewhat common need
> through the media subsystem, but generally the assumption is that
> they will all be data links. To meet that assumption add a new macro
> that iterates through an entity's links and skips non-data links.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since v1 (suggested by Jacopo and Laurent):
> 
> 	- Simplified __media_entity_next_link()
> 	- Added the link_type param to __media_entity_next_link()
> 	- Moved __media_entity_next_link() to mc-entity.c rather than
> 	  media-entity.h
> 
>  drivers/media/mc/mc-entity.c | 16 ++++++++++++++++
>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 11f5207f73aa..a247d5679930 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/bitmap.h>
> +#include <linux/list.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
> @@ -1051,3 +1052,18 @@ struct media_link *media_create_ancillary_link(struct media_entity *primary,
>  	return link;
>  }
>  EXPORT_SYMBOL_GPL(media_create_ancillary_link);
> +
> +struct media_link *__media_entity_next_link(struct media_entity *entity,
> +					    struct media_link *link,
> +					    unsigned long link_type)
> +{
> +	link = link ? list_next_entry(link, list)
> +		    : list_first_entry(&entity->links, typeof(*link), list);
> +
> +	list_for_each_entry_from(link, &entity->links, list)
> +		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == link_type)
> +			return link;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_next_link);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index a9a1c0ec5d1c..903c9368c50d 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -1140,4 +1140,33 @@ struct media_link *
>  media_create_ancillary_link(struct media_entity *primary,
>  			    struct media_entity *ancillary);
>  
> +/**
> + * __media_entity_next_link() - iterate through a &media_entity's links,

s/iterate/Iterate/
s/,$//

> + *
> + * @entity:	pointer to the &media_entity
> + * @link:	pointer to a &media_link to hold the iterated values
> + * @link_type:	one of the MEDIA_LNK_FL_LINK_TYPE flags
> + *
> + * Return the next link against an entity matching a specific link type. This
> + * allows iteration through an entity's links whilst guaranteeing all of the
> + * returned links are of the given type.
> + */
> +struct media_link *__media_entity_next_link(struct media_entity *entity,
> +					    struct media_link *link,
> +					    unsigned long link_type);
> +
> +/**
> + * for_each_media_entity_data_link() - Iterate through an entity's data links
> + *
> + * @entity:	pointer to the &media_entity
> + * @link:	pointer to a &media_link to hold the iterated values
> + *
> + * Iterate over a &media_entity's data links
> + */
> +#define for_each_media_entity_data_link(entity, link)			\
> +	for (link = __media_entity_next_link(entity, NULL,		\
> +					     MEDIA_LNK_FL_DATA_LINK);	\
> +	     link;							\
> +	     link = __media_entity_next_link(entity, link,		\
> +					     MEDIA_LNK_FL_DATA_LINK))

Missing one blank line here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] media: entity: Add iterator for entity data links

[Sakari Ailus]

Hi Laurent, Daniel,

On Mon, Jul 11, 2022 at 07:17:34PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thu, Jul 07, 2022 at 11:47:32PM +0100, Daniel Scally wrote:
> > Iterating over the links for an entity is a somewhat common need
> > through the media subsystem, but generally the assumption is that
> > they will all be data links. To meet that assumption add a new macro
> > that iterates through an entity's links and skips non-data links.
> > 
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes since v1 (suggested by Jacopo and Laurent):
> > 
> > 	- Simplified __media_entity_next_link()
> > 	- Added the link_type param to __media_entity_next_link()
> > 	- Moved __media_entity_next_link() to mc-entity.c rather than
> > 	  media-entity.h
> > 
> >  drivers/media/mc/mc-entity.c | 16 ++++++++++++++++
> >  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 11f5207f73aa..a247d5679930 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -9,6 +9,7 @@
> >   */
> >  
> >  #include <linux/bitmap.h>
> > +#include <linux/list.h>
> >  #include <linux/property.h>
> >  #include <linux/slab.h>
> >  #include <media/media-entity.h>
> > @@ -1051,3 +1052,18 @@ struct media_link *media_create_ancillary_link(struct media_entity *primary,
> >  	return link;
> >  }
> >  EXPORT_SYMBOL_GPL(media_create_ancillary_link);
> > +
> > +struct media_link *__media_entity_next_link(struct media_entity *entity,
> > +					    struct media_link *link,
> > +					    unsigned long link_type)
> > +{
> > +	link = link ? list_next_entry(link, list)
> > +		    : list_first_entry(&entity->links, typeof(*link), list);
> > +
> > +	list_for_each_entry_from(link, &entity->links, list)
> > +		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == link_type)
> > +			return link;
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(__media_entity_next_link);
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index a9a1c0ec5d1c..903c9368c50d 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -1140,4 +1140,33 @@ struct media_link *
> >  media_create_ancillary_link(struct media_entity *primary,
> >  			    struct media_entity *ancillary);
> >  
> > +/**
> > + * __media_entity_next_link() - iterate through a &media_entity's links,
> 
> s/iterate/Iterate/
> s/,$//
> 
> > + *
> > + * @entity:	pointer to the &media_entity
> > + * @link:	pointer to a &media_link to hold the iterated values
> > + * @link_type:	one of the MEDIA_LNK_FL_LINK_TYPE flags
> > + *
> > + * Return the next link against an entity matching a specific link type. This
> > + * allows iteration through an entity's links whilst guaranteeing all of the
> > + * returned links are of the given type.
> > + */
> > +struct media_link *__media_entity_next_link(struct media_entity *entity,
> > +					    struct media_link *link,
> > +					    unsigned long link_type);
> > +
> > +/**
> > + * for_each_media_entity_data_link() - Iterate through an entity's data links
> > + *
> > + * @entity:	pointer to the &media_entity
> > + * @link:	pointer to a &media_link to hold the iterated values
> > + *
> > + * Iterate over a &media_entity's data links
> > + */
> > +#define for_each_media_entity_data_link(entity, link)			\
> > +	for (link = __media_entity_next_link(entity, NULL,		\
> > +					     MEDIA_LNK_FL_DATA_LINK);	\
> > +	     link;							\
> > +	     link = __media_entity_next_link(entity, link,		\
> > +					     MEDIA_LNK_FL_DATA_LINK))
> 
> Missing one blank line here.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Added with these addressed.

-- 
Sakari Ailus