[RFC PATCH 0/2] Media entity links handling

[RFC PATCH 0/2] Media entity links handling

[Sylwester Nawrocki]

Hi All,

This small patch set add a function for removing all links at a media
entity. I found out such a function is needed when media entites that
belong to single media device have drivers in different kernel modules.
This means virtually all camera drivers, since sensors are separate
modules from the host interface drivers.

More details can be found in the commits' description.

The links removal from a media entity is rather strightforward, but when
and where links should be created/removed is not immediately clear to me.

I assumed that links should be created/removed only when an entity is
registered to its media device, and with the graph mutex held.

I'm open to opinions whether it's good or not and possibly suggestions
on how those issues could be handled differently.

Thanks,
Sylwester

Sylwester Nawrocki (2):
  media: Add function removing all media entity links
  V4L: Remove all links of a media entity when unregistering subdev

 drivers/media/media-entity.c          |   51 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-device.c |    4 ++-
 include/media/media-entity.h          |    3 ++
 3 files changed, 57 insertions(+), 1 deletion(-)

--
1.7.9.5

[RFC PATCH 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki]

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@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 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..7c2b51c 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;
+
+	for (i = 0; i < entity->num_links; i++) {
+		struct media_link *link = &entity->links[i];
+		struct media_entity *remote;
+		int num_links;
+
+		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++) {
+			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)
+				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;
+
+	mutex_lock(&entity->parent->graph_mutex);
+	__media_entity_remove_links(entity);
+	mutex_lock(&entity->parent->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_entity_remove_links);
+
 static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
 {
 	int ret;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0c16f51..0d941d2 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -128,6 +128,9 @@ void media_entity_cleanup(struct media_entity *entity);
 
 int media_entity_create_link(struct media_entity *source, u16 source_pad,
 		struct media_entity *sink, u16 sink_pad, u32 flags);
+void __media_entity_remove_links(struct media_entity *entity);
+void media_entity_remove_links(struct media_entity *entity);
+
 int __media_entity_setup_link(struct media_link *link, u32 flags);
 int media_entity_setup_link(struct media_link *link, u32 flags);
 struct media_link *media_entity_find_link(struct media_pad *source,
-- 
1.7.9.5

[RFC PATCH 2/2] V4L: Remove all links of a media entity when unregistering subdev

[Sylwester Nawrocki]

Remove all links of the subdev's media entity after internal_ops
'unregistered' call and right before unregistering the entity from
a media device.

It is assumed here that an unregistered (orphan) media entity cannot
have links to other entities. All entities must belong to some media
device before they can be linked.

It is also assumed the media links should be created/removed with
the media graph's mutex held.

The above implies that the caller of v4l2_device_unregister_subdev()
must not hold the graph's mutex.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/v4l2-core/v4l2-device.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 8ed5da2..2dbfebc 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -269,8 +269,10 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	sd->v4l2_dev = NULL;
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev)
+	if (v4l2_dev->mdev) {
+		media_entity_remove_links(&sd->entity);
 		media_device_unregister_entity(&sd->entity);
+	}
 #endif
 	video_unregister_device(sd->devnode);
 	module_put(sd->owner);
-- 
1.7.9.5

[RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki]

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@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
[locking error in the initial patch version]
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
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;
+
+	for (i = 0; i < entity->num_links; i++) {
+		struct media_link *link = &entity->links[i];
+		struct media_entity *remote;
+		int num_links;
+
+		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++) {
+			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)
+				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;
+
+	mutex_lock(&entity->parent->graph_mutex);
+	__media_entity_remove_links(entity);
+	mutex_unlock(&entity->parent->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_entity_remove_links);
+
 static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
 {
 	int ret;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0c16f51..0d941d2 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -128,6 +128,9 @@ void media_entity_cleanup(struct media_entity *entity);
 
 int media_entity_create_link(struct media_entity *source, u16 source_pad,
 		struct media_entity *sink, u16 sink_pad, u32 flags);
+void __media_entity_remove_links(struct media_entity *entity);
+void media_entity_remove_links(struct media_entity *entity);
+
 int __media_entity_setup_link(struct media_link *link, u32 flags);
 int media_entity_setup_link(struct media_link *link, u32 flags);
 struct media_link *media_entity_find_link(struct media_pad *source,
-- 
1.7.9.5

Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sakari Ailus]

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@samsung.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> [locking error in the initial patch version]
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 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.

> +	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.

> +		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?

> +			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) ?

> +				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;
> +
> +	mutex_lock(&entity->parent->graph_mutex);
> +	__media_entity_remove_links(entity);
> +	mutex_unlock(&entity->parent->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_entity_remove_links);
> +
>  static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
>  {
>  	int ret;

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki]

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@samsung.com>
>> Reviewed-by: Andrzej Hajda<a.hajda@samsung.com>
>> [locking error in the initial patch version]
>> Reported-by: Dan Carpenter<dan.carpenter@oracle.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>> 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

Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sakari Ailus]

Hi Sylwester,

Sylwester Nawrocki wrote:
...
>>> 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.

Oh, forgot this one... yes, remote->num_links is decremented, but so is 
r it's compared to. So I don't think it's necessary to cache it, but 
it's neither an error to do so.

>>> +            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)
>
> ?

I'm not too fond of --/++ operators as part of more complex structures 
so I'd keep it separate. Fewer lines doesn't always equate to more 
readable code. :-)

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki]

Hi Sakari,

On 06/10/2013 12:15 AM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> ...
>>>> 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.
> 
> Oh, forgot this one... yes, remote->num_links is decremented, but so is 
> r it's compared to. So I don't think it's necessary to cache it, but 
> it's neither an error to do so.

Indeed, it seems more correct to not cache it, thanks.

>>>> +            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)
>>
>> ?
> 
> I'm not too fond of --/++ operators as part of more complex structures 
> so I'd keep it separate. Fewer lines doesn't always equate to more 
> readable code. :-)

In general I agree, however it's quite simple statement in this case -
only decrement and test, it's often only one instruction even in the
assembly language...

I'm going to squash following to this patch:

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f5ea82e..1fb619d 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity)
        for (i = 0; i < entity->num_links; i++) {
                struct media_link *link = &entity->links[i];
                struct media_entity *remote;
-               unsigned int r, num_links;
+               unsigned int r;

                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++) {
-                       if (remote->links[r] != link->reverse)
+               while (r < remote->num_links; r++) {
+                       if (remote->links[r] != link->reverse) {
+                               r++;
                                continue;
+                       }

                        if (link->source->entity == entity)
                                remote->num_backlinks--;
@@ -456,7 +456,7 @@ void __media_entity_remove_links(struct media_entity *entity)
                                break;

                        /* Insert last entry in place of the dropped link. */
-                       remote->links[r--] = remote->links[remote->num_links];
+                       remote->links[r] = remote->links[remote->num_links];
                }
        }

So the loop looks something like:


		while (r < remote->num_links) {
			if (remote->links[r] != link->reverse) {
				r++;
				continue;
			}

			if (link->source->entity == entity)
				remote->num_backlinks--;

			if (--remote->num_links == 0)
				break;

			/* Insert last entry in place of the dropped link. */
			remote->links[r] = remote->links[remote->num_links];
		}

Does it sound OK ?

Regards,
Sylwester

Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links

[Sylwester Nawrocki]

On 06/10/2013 12:26 PM, Sylwester Nawrocki wrote:
> Hi Sakari,
> 
> On 06/10/2013 12:15 AM, Sakari Ailus wrote:
>> Sylwester Nawrocki wrote:
>> ...
>>>>> 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.
>>
>> Oh, forgot this one... yes, remote->num_links is decremented, but so is 
>> r it's compared to. So I don't think it's necessary to cache it, but 
>> it's neither an error to do so.
> 
> Indeed, it seems more correct to not cache it, thanks.
> 
>>>>> +            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)
>>>
>>> ?
>>
>> I'm not too fond of --/++ operators as part of more complex structures 
>> so I'd keep it separate. Fewer lines doesn't always equate to more 
>> readable code. :-)
> 
> In general I agree, however it's quite simple statement in this case -
> only decrement and test, it's often only one instruction even in the
> assembly language...
> 
> I'm going to squash following to this patch:
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index f5ea82e..1fb619d 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity)
>         for (i = 0; i < entity->num_links; i++) {
>                 struct media_link *link = &entity->links[i];
>                 struct media_entity *remote;
> -               unsigned int r, num_links;
> +               unsigned int r;

Should have been:
	          unsigned int r = 0;


Thanks,
Sylwester