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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
