[PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

s5p_mfc_remove() fails to release encoder and decoder video devices.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

Changes since v1:
- Addressed comments from Javier Martinez Canillas and added
  his reviewed by:

 drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 274b4f1..f537b74 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1318,6 +1318,8 @@ static int s5p_mfc_remove(struct platform_device *pdev)
 
 	video_unregister_device(dev->vfd_enc);
 	video_unregister_device(dev->vfd_dec);
+	video_device_release(dev->vfd_enc);
+	video_device_release(dev->vfd_dec);
 	v4l2_device_unregister(&dev->v4l2_dev);
 	s5p_mfc_release_firmware(dev);
 	vb2_dma_contig_cleanup_ctx(dev->alloc_ctx[0]);
-- 
2.7.4

RE: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Kamil Debski]

HI Shuah,

Which branch do you base your patches on?

I have trouble applying this path
(https://patchwork.linuxtv.org/patch/34577/) and 
"s5p-mfc fix null pointer deference in clk_core_enable()"
(https://patchwork.linuxtv.org/patch/34751/) 
onto current linuxtv/master.

The top commit of linuxtv/master is :
"commit 0db5c79989de2c68d5abb7ba891bfdb3cd3b7e05
Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Date:   Thu Jun 16 08:04:40 2016 -0300

    [media] media-devnode.h: Fix documentation"

Could you please rebase the two patches mentioned above to the
linuxtv/master?

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland


> -----Original Message-----
> From: Shuah Khan [mailto:shuahkh@osg.samsung.com]
> Sent: Monday, June 13, 2016 9:45 PM
> To: kyungmin.park@samsung.com; k.debski@samsung.com;
> jtp.park@samsung.com; mchehab@osg.samsung.com
> Cc: Shuah Khan; linux-arm-kernel@lists.infradead.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()
> 
> s5p_mfc_remove() fails to release encoder and decoder video devices.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
> Changes since v1:
> - Addressed comments from Javier Martinez Canillas and added
>   his reviewed by:
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 274b4f1..f537b74 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1318,6 +1318,8 @@ static int s5p_mfc_remove(struct platform_device
> *pdev)
> 
>  	video_unregister_device(dev->vfd_enc);
>  	video_unregister_device(dev->vfd_dec);
> +	video_device_release(dev->vfd_enc);
> +	video_device_release(dev->vfd_dec);
>  	v4l2_device_unregister(&dev->v4l2_dev);
>  	s5p_mfc_release_firmware(dev);
>  	vb2_dma_contig_cleanup_ctx(dev->alloc_ctx[0]);
> --
> 2.7.4

Re: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

On 06/28/2016 03:57 AM, Kamil Debski wrote:
> HI Shuah,
> 
> Which branch do you base your patches on?
> 
> I have trouble applying this path
> (https://patchwork.linuxtv.org/patch/34577/) and 
> "s5p-mfc fix null pointer deference in clk_core_enable()"
> (https://patchwork.linuxtv.org/patch/34751/) 
> onto current linuxtv/master.
> 
> The top commit of linuxtv/master is :
> "commit 0db5c79989de2c68d5abb7ba891bfdb3cd3b7e05
> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Date:   Thu Jun 16 08:04:40 2016 -0300
> 
>     [media] media-devnode.h: Fix documentation"
> 
> Could you please rebase the two patches mentioned above to the
> linuxtv/master?
> 
> Best wishes,
> 

I based them on linux_next. I will rebase both patches on linuxtv/master
and resend.

thanks,
-- Shuah

Re: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

On 06/28/2016 07:39 AM, Shuah Khan wrote:
> On 06/28/2016 03:57 AM, Kamil Debski wrote:
>> HI Shuah,
>>
>> Which branch do you base your patches on?
>>
>> I have trouble applying this path
>> (https://patchwork.linuxtv.org/patch/34577/) and 
>> "s5p-mfc fix null pointer deference in clk_core_enable()"
>> (https://patchwork.linuxtv.org/patch/34751/) 
>> onto current linuxtv/master.
>>
>> The top commit of linuxtv/master is :
>> "commit 0db5c79989de2c68d5abb7ba891bfdb3cd3b7e05
>> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Date:   Thu Jun 16 08:04:40 2016 -0300
>>
>>     [media] media-devnode.h: Fix documentation"
>>
>> Could you please rebase the two patches mentioned above to the
>> linuxtv/master?
>>
>> Best wishes,
>>
> 
> I based them on linux_next. I will rebase both patches on linuxtv/master
> and resend.
> 

Hi Kamil,

Do you need me to rebase this one:

https://patchwork.kernel.org/patch/9166119/

I will do that anyway and send three patches. I don't see the above
in linuxtv/master

thanks,
-- Shuah

Re: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

On 06/28/2016 09:55 AM, Shuah Khan wrote:
> On 06/28/2016 07:39 AM, Shuah Khan wrote:
>> On 06/28/2016 03:57 AM, Kamil Debski wrote:
>>> HI Shuah,
>>>
>>> Which branch do you base your patches on?
>>>
>>> I have trouble applying this path
>>> (https://patchwork.linuxtv.org/patch/34577/) and 
>>> "s5p-mfc fix null pointer deference in clk_core_enable()"
>>> (https://patchwork.linuxtv.org/patch/34751/) 
>>> onto current linuxtv/master.
>>>
>>> The top commit of linuxtv/master is :
>>> "commit 0db5c79989de2c68d5abb7ba891bfdb3cd3b7e05
>>> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>> Date:   Thu Jun 16 08:04:40 2016 -0300
>>>
>>>     [media] media-devnode.h: Fix documentation"
>>>
>>> Could you please rebase the two patches mentioned above to the
>>> linuxtv/master?
>>>
>>> Best wishes,
>>>
>>
>> I based them on linux_next. I will rebase both patches on linuxtv/master
>> and resend.
>>
> 
> Hi Kamil,
> 
> Do you need me to rebase this one:
> 
> https://patchwork.kernel.org/patch/9166119/
> 
> I will do that anyway and send three patches. I don't see the above
> in linuxtv/master
> 

Hi Kamil,

https://patchwork.kernel.org/patch/9166119/
media: s5p-mfc fix video device release double release in probe error path

and 

(https://patchwork.linuxtv.org/patch/34577/)
media: s5p-mfc fix memory leak in s5p_mfc_remove()

applied as is on top of linuxtv/master latest with top
commit

The following failed and applied with fuzz.

"s5p-mfc fix null pointer deference in clk_core_enable()"
(https://patchwork.linuxtv.org/patch/34751/) 
onto current linuxtv/master.

I generated a rebasewd patch series with the above 3 patches
and sent it.

https://www.mail-archive.com/linux-media@vger.kernel.org/msg98784.html

thanks,
-- Shuah

Re: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

On 06/13/2016 07:42 AM, Javier Martinez Canillas wrote:
> Hello Shuah,
> 
> On Wed, Jun 8, 2016 at 9:35 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>> s5p_mfc_remove() fails to release encoder and decoder video devices.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> index 274b4f1..af61f54 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> @@ -1317,7 +1317,9 @@ static int s5p_mfc_remove(struct platform_device *pdev)
>>         destroy_workqueue(dev->watchdog_workqueue);
>>
>>         video_unregister_device(dev->vfd_enc);
>> +       video_device_release(dev->vfd_enc);
>>         video_unregister_device(dev->vfd_dec);
>> +       video_device_release(dev->vfd_dec);
>>         v4l2_device_unregister(&dev->v4l2_dev);
>>         s5p_mfc_release_firmware(dev);
>>         vb2_dma_contig_cleanup_ctx(dev->alloc_ctx[0]);
>> --
> 
> Can you please do the remove operations in the inverse order of their
> counterparts? IOW to do the release for both encoder and decoder after
> their unregistration.
> 

I considered that. No problem. I will move both release after the
video_unregister_device(dev->vfd_dec), releasing enc first and then
the dec

> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 

thanks,
-- Shuah

Re: [PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Javier Martinez Canillas]

Hello Shuah,

On Wed, Jun 8, 2016 at 9:35 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> s5p_mfc_remove() fails to release encoder and decoder video devices.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 274b4f1..af61f54 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1317,7 +1317,9 @@ static int s5p_mfc_remove(struct platform_device *pdev)
>         destroy_workqueue(dev->watchdog_workqueue);
>
>         video_unregister_device(dev->vfd_enc);
> +       video_device_release(dev->vfd_enc);
>         video_unregister_device(dev->vfd_dec);
> +       video_device_release(dev->vfd_dec);
>         v4l2_device_unregister(&dev->v4l2_dev);
>         s5p_mfc_release_firmware(dev);
>         vb2_dma_contig_cleanup_ctx(dev->alloc_ctx[0]);
> --

Can you please do the remove operations in the inverse order of their
counterparts? IOW to do the release for both encoder and decoder after
their unregistration.

After that change:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

[PATCH] media: s5p-mfc fix memory leak in s5p_mfc_remove()

[Shuah Khan]

s5p_mfc_remove() fails to release encoder and decoder video devices.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 274b4f1..af61f54 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1317,7 +1317,9 @@ static int s5p_mfc_remove(struct platform_device *pdev)
 	destroy_workqueue(dev->watchdog_workqueue);
 
 	video_unregister_device(dev->vfd_enc);
+	video_device_release(dev->vfd_enc);
 	video_unregister_device(dev->vfd_dec);
+	video_device_release(dev->vfd_dec);
 	v4l2_device_unregister(&dev->v4l2_dev);
 	s5p_mfc_release_firmware(dev);
 	vb2_dma_contig_cleanup_ctx(dev->alloc_ctx[0]);
-- 
2.7.4