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

[Sakari Ailus <sakari.ailus@iki.fi>]

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