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

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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