[PATCH 0/3] media: vimc: bug fixes related to memory management

[PATCH 0/3] media: vimc: bug fixes related to memory management

[Dafna Hirschfeld]

This patchset has 3 patches fixing 3 bugs in vimc.
The patchset is rebased on top of v5 of the patchset
'media: vimc: Collapse component structure into a single
monolithic driver' sent by 'Shuah Khan'





Dafna Hirschfeld (3):
  media: vimc: initialize vim entity pointers to NULL
  media: vimc: sensor: register subdevice only after initialization
  media: vimc: move media_entity_cleanup to release callbacks

 drivers/media/platform/vimc/vimc-capture.c |  2 +-
 drivers/media/platform/vimc/vimc-common.c  |  1 -
 drivers/media/platform/vimc/vimc-core.c    |  8 +++-----
 drivers/media/platform/vimc/vimc-debayer.c |  1 +
 drivers/media/platform/vimc/vimc-scaler.c  |  1 +
 drivers/media/platform/vimc/vimc-sensor.c  | 21 +++++++++++----------
 6 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.20.1

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

[Dafna Hirschfeld]

since NULL value for vimc entity pointer indicates
that entity creation failed and this is tested, the
pointers should be initialized to NULL.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@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;
 }
@@ -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);
 	if (!vimc->ent_devs)
-- 
2.20.1

[PATCH 2/3] media: vimc: sensor: register subdevice only after initialization

[Dafna Hirschfeld]

vimc_sen_add function first registers the subdevice and then
calls tpg_alloc. If tpg_alloc fails it unregisters the subdevice
and then frees vsen, this cause double free since the release
callback that follows subdevice unregistration also frees vsen.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-sensor.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 46dc6a535abe..ee2306c08569 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -358,6 +358,13 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 		goto err_free_vsen;
 	}
 
+	/* Initialize the test pattern generator */
+	tpg_init(&vsen->tpg, vsen->mbus_format.width,
+		 vsen->mbus_format.height);
+	ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
+	if (ret)
+		goto err_free_hdl;
+
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
 				   vcfg_name,
@@ -365,7 +372,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
 				   &vimc_sen_int_ops, &vimc_sen_ops);
 	if (ret)
-		goto err_free_hdl;
+		goto err_free_tpg;
 
 	vsen->ved.process_frame = vimc_sen_process_frame;
 	vsen->dev = &vimc->pdev.dev;
@@ -373,17 +380,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 	/* Initialize the frame format */
 	vsen->mbus_format = fmt_default;
 
-	/* Initialize the test pattern generator */
-	tpg_init(&vsen->tpg, vsen->mbus_format.width,
-		 vsen->mbus_format.height);
-	ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
-	if (ret)
-		goto err_unregister_ent_sd;
-
 	return &vsen->ved;
 
-err_unregister_ent_sd:
-	vimc_ent_sd_unregister(&vsen->ved,  &vsen->sd);
+err_free_tpg:
+	tpg_free(&vsen->tpg);
 err_free_hdl:
 	v4l2_ctrl_handler_free(&vsen->hdl);
 err_free_vsen:
-- 
2.20.1

[PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks

[Dafna Hirschfeld]

according to the docs, this function must be called during
the cleanup phase after unregistering the entity.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-capture.c | 2 +-
 drivers/media/platform/vimc/vimc-common.c  | 1 -
 drivers/media/platform/vimc/vimc-debayer.c | 1 +
 drivers/media/platform/vimc/vimc-scaler.c  | 1 +
 drivers/media/platform/vimc/vimc-sensor.c  | 1 +
 5 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 602f80323031..5f353c20e605 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -330,6 +330,7 @@ static void vimc_cap_release(struct video_device *vdev)
 	struct vimc_cap_device *vcap =
 		container_of(vdev, struct vimc_cap_device, vdev);
 
+	media_entity_cleanup(vcap->ved.ent);
 	vimc_pads_cleanup(vcap->ved.pads);
 	kfree(vcap);
 }
@@ -340,7 +341,6 @@ void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
 
 	vcap = container_of(ved, struct vimc_cap_device, ved);
 	vb2_queue_release(&vcap->queue);
-	media_entity_cleanup(ved->ent);
 	video_unregister_device(&vcap->vdev);
 }
 
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index a3120f4f7a90..999bc353fb10 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -423,7 +423,6 @@ EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
 
 void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
 {
-	media_entity_cleanup(ved->ent);
 	v4l2_device_unregister_subdev(sd);
 }
 EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index feac47d79449..e1bad6713cde 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -477,6 +477,7 @@ static void vimc_deb_release(struct v4l2_subdev *sd)
 	struct vimc_deb_device *vdeb =
 				container_of(sd, struct vimc_deb_device, sd);
 
+	media_entity_cleanup(vdeb->ved.ent);
 	vimc_pads_cleanup(vdeb->ved.pads);
 	kfree(vdeb);
 }
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index a6a3cc5be872..1982bc089af5 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -336,6 +336,7 @@ static void vimc_sca_release(struct v4l2_subdev *sd)
 	struct vimc_sca_device *vsca =
 				container_of(sd, struct vimc_sca_device, sd);
 
+	media_entity_cleanup(vsca->ved.ent);
 	vimc_pads_cleanup(vsca->ved.pads);
 	kfree(vsca);
 }
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index ee2306c08569..63fe024ccea5 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -291,6 +291,7 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
 
 	v4l2_ctrl_handler_free(&vsen->hdl);
 	tpg_free(&vsen->tpg);
+	media_entity_cleanup(vsen->ved.ent);
 	vimc_pads_cleanup(vsen->ved.pads);
 	kfree(vsen);
 }
-- 
2.20.1

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

[Helen Koike]

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@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?

Thanks
Helen

>  	if (!vimc->ent_devs)
> 

Re: [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization

[Helen Koike]

On 10/1/19 1:50 PM, Dafna Hirschfeld wrote:
> vimc_sen_add function first registers the subdevice and then
> calls tpg_alloc. If tpg_alloc fails it unregisters the subdevice
> and then frees vsen, this cause double free since the release
> callback that follows subdevice unregistration also frees vsen.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
>  drivers/media/platform/vimc/vimc-sensor.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 46dc6a535abe..ee2306c08569 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -358,6 +358,13 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>  		goto err_free_vsen;
>  	}
>  
> +	/* Initialize the test pattern generator */
> +	tpg_init(&vsen->tpg, vsen->mbus_format.width,
> +		 vsen->mbus_format.height);
> +	ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
> +	if (ret)
> +		goto err_free_hdl;
> +
>  	/* Initialize ved and sd */
>  	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
>  				   vcfg_name,
> @@ -365,7 +372,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>  				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
>  				   &vimc_sen_int_ops, &vimc_sen_ops);
>  	if (ret)
> -		goto err_free_hdl;
> +		goto err_free_tpg;
>  
>  	vsen->ved.process_frame = vimc_sen_process_frame;
>  	vsen->dev = &vimc->pdev.dev;
> @@ -373,17 +380,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>  	/* Initialize the frame format */
>  	vsen->mbus_format = fmt_default;
>  
> -	/* Initialize the test pattern generator */
> -	tpg_init(&vsen->tpg, vsen->mbus_format.width,
> -		 vsen->mbus_format.height);
> -	ret = tpg_alloc(&vsen->tpg, VIMC_FRAME_MAX_WIDTH);
> -	if (ret)
> -		goto err_unregister_ent_sd;
> -
>  	return &vsen->ved;
>  
> -err_unregister_ent_sd:
> -	vimc_ent_sd_unregister(&vsen->ved,  &vsen->sd);
> +err_free_tpg:
> +	tpg_free(&vsen->tpg);
>  err_free_hdl:
>  	v4l2_ctrl_handler_free(&vsen->hdl);
>  err_free_vsen:
> 

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.hirschfeld@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

Re: [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks

[Helen Koike]

Hi Dafna,

Thanks for your patch

On 10/1/19 1:50 PM, Dafna Hirschfeld wrote:
> according to the docs, this function must be called during
> the cleanup phase after unregistering the entity.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

I just have one note below.

> ---
>  drivers/media/platform/vimc/vimc-capture.c | 2 +-
>  drivers/media/platform/vimc/vimc-common.c  | 1 -
>  drivers/media/platform/vimc/vimc-debayer.c | 1 +
>  drivers/media/platform/vimc/vimc-scaler.c  | 1 +
>  drivers/media/platform/vimc/vimc-sensor.c  | 1 +
>  5 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 602f80323031..5f353c20e605 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -330,6 +330,7 @@ static void vimc_cap_release(struct video_device *vdev)
>  	struct vimc_cap_device *vcap =
>  		container_of(vdev, struct vimc_cap_device, vdev);
>  
> +	media_entity_cleanup(vcap->ved.ent);
>  	vimc_pads_cleanup(vcap->ved.pads);
>  	kfree(vcap);
>  }
> @@ -340,7 +341,6 @@ void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>  
>  	vcap = container_of(ved, struct vimc_cap_device, ved);
>  	vb2_queue_release(&vcap->queue);
> -	media_entity_cleanup(ved->ent);
>  	video_unregister_device(&vcap->vdev);
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index a3120f4f7a90..999bc353fb10 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -423,7 +423,6 @@ EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
>  
>  void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)

Could you get rid of this function as well?
There is no point in keeping it anymore, feel free to send in another patch.

Thanks
Helen

>  {
> -	media_entity_cleanup(ved->ent);
>  	v4l2_device_unregister_subdev(sd);
>  }
>  EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index feac47d79449..e1bad6713cde 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -477,6 +477,7 @@ static void vimc_deb_release(struct v4l2_subdev *sd)
>  	struct vimc_deb_device *vdeb =
>  				container_of(sd, struct vimc_deb_device, sd);
>  
> +	media_entity_cleanup(vdeb->ved.ent);
>  	vimc_pads_cleanup(vdeb->ved.pads);
>  	kfree(vdeb);
>  }
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index a6a3cc5be872..1982bc089af5 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -336,6 +336,7 @@ static void vimc_sca_release(struct v4l2_subdev *sd)
>  	struct vimc_sca_device *vsca =
>  				container_of(sd, struct vimc_sca_device, sd);
>  
> +	media_entity_cleanup(vsca->ved.ent);
>  	vimc_pads_cleanup(vsca->ved.pads);
>  	kfree(vsca);
>  }
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index ee2306c08569..63fe024ccea5 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -291,6 +291,7 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
>  
>  	v4l2_ctrl_handler_free(&vsen->hdl);
>  	tpg_free(&vsen->tpg);
> +	media_entity_cleanup(vsen->ved.ent);
>  	vimc_pads_cleanup(vsen->ved.pads);
>  	kfree(vsen);
>  }
> 

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

[Helen Koike]

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@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?

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

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

[Andrzej Pietrasiewicz]

W dniu 01.10.2019 o 19:35, Helen Koike pisze:
> 

<snip>

>>>
>>> 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 for teaching me how to read :D

Andrzej