Re: [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL

[Helen Koike <helen.koike@xxxxxxxxxxxxx>]

On 10/1/19 2:25 PM, Andrzej Pietrasiewicz wrote:
> Hi Dafna, hi Helen,
> 
> W dniu 01.10.2019 o 19:19, Helen Koike pisze:
>> Hi Dafna,
>>
>> On 10/1/19 1:50 PM, Dafna Hirschfeld wrote:
>>> since NULL value for vimc entity pointer indicates
>>> that entity creation failed and this is tested, the
>>> pointers should be initialized to NULL.
>>
>> Nice catch :)
>>
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
>>> ---
>>>   drivers/media/platform/vimc/vimc-core.c | 8 +++-----
>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>>> index 6e3e5c91ae39..32a79e578b78 100644
>>> --- a/drivers/media/platform/vimc/vimc-core.c
>>> +++ b/drivers/media/platform/vimc/vimc-core.c
>>> @@ -160,19 +160,17 @@ static int vimc_create_links(struct vimc_device *vimc)
>>>   static int vimc_add_subdevs(struct vimc_device *vimc)
>>>   {
>>>       unsigned int i;
>>> -    struct vimc_ent_device *ved;
>>>         for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
>>>           dev_dbg(&vimc->pdev.dev, "new entity for %s\n",
>>>               vimc->pipe_cfg->ents[i].name);
>>> -        ved = vimc->pipe_cfg->ents[i].add(vimc,
>>> +        vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
>>>                       vimc->pipe_cfg->ents[i].name);
>>> -        if (!ved) {
>>> +        if (!vimc->ent_devs[i]) {
>>>               dev_err(&vimc->pdev.dev, "add new entity for %s\n",
>>>                   vimc->pipe_cfg->ents[i].name);
>>>               return -EINVAL;
>>>           }
>>> -        vimc->ent_devs[i] = ved;
>>>       }
>>>       return 0;
>>>   }
>>
>> I believe just the kcalloc bellow should fix the issue.
>> But if you want to do this cleanup anyway, I would suggest sending a separate patch for it.
>>
>>> @@ -199,7 +197,7 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>>       }
>>>         /* allocate ent_devs */
>>> -    vimc->ent_devs = kmalloc_array(vimc->pipe_cfg->num_ents,
>>> +    vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents,
>>>                          sizeof(*vimc->ent_devs),
>>>                          GFP_KERNEL);
>>
>> Could you fix the alignment of the params here?
> 
> Isn't the above change (kmalloc_array() to kcalloc()) alone enough
> to ensure the promise from the patch title is fulfilled?

I fully agree. That is why I mentioned above in "I believe just the kcalloc bellow should fix the issue."
Sorry if I wasn't clear.

Thanks
Helen


> 
> In other words, why remove the "ved" local variable in vimc_add_subdevs()?
> 
> Andrzej