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

[Andrzej Pietrasiewicz]

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.hirschf...@collabora.com>
---
  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?

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

Andrzej