* [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL
2019-10-01 16:50 [PATCH 0/3] media: vimc: bug fixes related to memory management Dafna Hirschfeld
@ 2019-10-01 16:50 ` Dafna Hirschfeld
2019-10-01 17:19 ` Helen Koike
2019-10-01 16:50 ` [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization Dafna Hirschfeld
2019-10-01 16:50 ` [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks Dafna Hirschfeld
2 siblings, 1 reply; 10+ messages in thread
From: Dafna Hirschfeld @ 2019-10-01 16:50 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, andre.almeida, skhan,
hverkuil, kernel, dafna3
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL
2019-10-01 16:50 ` [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL Dafna Hirschfeld
@ 2019-10-01 17:19 ` Helen Koike
2019-10-01 17:25 ` Andrzej Pietrasiewicz
0 siblings, 1 reply; 10+ messages in thread
From: Helen Koike @ 2019-10-01 17:19 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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)
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL
2019-10-01 17:19 ` Helen Koike
@ 2019-10-01 17:25 ` Andrzej Pietrasiewicz
2019-10-01 17:35 ` Helen Koike
0 siblings, 1 reply; 10+ messages in thread
From: Andrzej Pietrasiewicz @ 2019-10-01 17:25 UTC (permalink / raw)
To: Helen Koike, Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL
2019-10-01 17:25 ` Andrzej Pietrasiewicz
@ 2019-10-01 17:35 ` Helen Koike
2019-10-01 17:38 ` Andrzej Pietrasiewicz
0 siblings, 1 reply; 10+ messages in thread
From: Helen Koike @ 2019-10-01 17:35 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL
2019-10-01 17:35 ` Helen Koike
@ 2019-10-01 17:38 ` Andrzej Pietrasiewicz
0 siblings, 0 replies; 10+ messages in thread
From: Andrzej Pietrasiewicz @ 2019-10-01 17:38 UTC (permalink / raw)
To: Helen Koike, Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization
2019-10-01 16:50 [PATCH 0/3] media: vimc: bug fixes related to memory management Dafna Hirschfeld
2019-10-01 16:50 ` [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL Dafna Hirschfeld
@ 2019-10-01 16:50 ` Dafna Hirschfeld
2019-10-01 17:22 ` Helen Koike
2019-10-01 16:50 ` [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks Dafna Hirschfeld
2 siblings, 1 reply; 10+ messages in thread
From: Dafna Hirschfeld @ 2019-10-01 16:50 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, andre.almeida, skhan,
hverkuil, kernel, dafna3
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization
2019-10-01 16:50 ` [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization Dafna Hirschfeld
@ 2019-10-01 17:22 ` Helen Koike
0 siblings, 0 replies; 10+ messages in thread
From: Helen Koike @ 2019-10-01 17:22 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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:
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks
2019-10-01 16:50 [PATCH 0/3] media: vimc: bug fixes related to memory management Dafna Hirschfeld
2019-10-01 16:50 ` [PATCH 1/3] media: vimc: initialize vim entity pointers to NULL Dafna Hirschfeld
2019-10-01 16:50 ` [PATCH 2/3] media: vimc: sensor: register subdevice only after initialization Dafna Hirschfeld
@ 2019-10-01 16:50 ` Dafna Hirschfeld
2019-10-01 17:33 ` Helen Koike
2 siblings, 1 reply; 10+ messages in thread
From: Dafna Hirschfeld @ 2019-10-01 16:50 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, andre.almeida, skhan,
hverkuil, kernel, dafna3
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks
2019-10-01 16:50 ` [PATCH 3/3] media: vimc: move media_entity_cleanup to release callbacks Dafna Hirschfeld
@ 2019-10-01 17:33 ` Helen Koike
0 siblings, 0 replies; 10+ messages in thread
From: Helen Koike @ 2019-10-01 17:33 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: ezequiel, andre.almeida, skhan, hverkuil, kernel, dafna3
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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread