[PATCH] media: mediatek: vcodec: Add scp_put() to free the scp

[PATCH] media: mediatek: vcodec: Add scp_put() to free the scp

[Jiasheng Jiang]

Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory
leak.

Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP")
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
 .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c      | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
index ff23b225db70..1b0bc47355c0 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
@@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use
 	}
 
 	fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL);
-	if (!fw)
+	if (!fw) {
+		scp_put(scp);
 		return ERR_PTR(-ENOMEM);
+	}
+
 	fw->type = SCP;
 	fw->ops = &mtk_vcodec_rproc_msg;
 	fw->scp = scp;
-- 
2.25.1

Re: [PATCH] media: mediatek: vcodec: Add scp_put() to free the scp

[Sebastian Fricke]

Hey Jiasheng,

 ---- On Fri, 31 Jan 2025 02:28:30 +0100  Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote --- 
 > Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory
 > leak.

Your commit message sounds a bit like you copy-pasted your code into the commit message.
What kind of memory is leaking here? Are we talking about SRAM memory?
I'd reword the commit message to something like this to give a bit more context:

On Mediatek devices with a system companion processor (SCP) the mtk_scp structure has to be removed explicitly to avoid a memory leak.
Free the structure in case the allocation of the firmware structure fails during the firmware initialization.

---

Additionally, the commit title says close to nothing to the reader as well.
How about: Fix memory leak in FW initialization

But as I stated above you should clarify what kind of memory we are talking about here.

Also just out of interest have you ever actually experienced issues with this?
It seems to me that the situation where you run out of Kernel memory should be quite rare.

Regards,
Sebastian

 > 
 > Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP")
 > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
 > ---
 >  .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c      | 5 ++++-
 >  1 file changed, 4 insertions(+), 1 deletion(-)
 > 
 > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
 > index ff23b225db70..1b0bc47355c0 100644
 > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
 > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
 > @@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use
 >      }
 >  
 >      fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL);
 > -    if (!fw)
 > +    if (!fw) {
 > +        scp_put(scp);
 >          return ERR_PTR(-ENOMEM);
 > +    }
 > +
 >      fw->type = SCP;
 >      fw->ops = &mtk_vcodec_rproc_msg;
 >      fw->scp = scp;
 > -- 
 > 2.25.1
 > 
 > 
 > 

Re: [PATCH] media: mediatek: vcodec: Add scp_put() to free the scp

[Jiasheng Jiang]

Hi Sebastian,

On Fri, Feb 14, 2025 at 3:20 PM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Hey Jiasheng,
>
>  ---- On Fri, 31 Jan 2025 02:28:30 +0100  Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote ---
>  > Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory
>  > leak.
>
> Your commit message sounds a bit like you copy-pasted your code into the commit message.
> What kind of memory is leaking here? Are we talking about SRAM memory?
> I'd reword the commit message to something like this to give a bit more context:
>
> On Mediatek devices with a system companion processor (SCP) the mtk_scp structure has to be removed explicitly to avoid a memory leak.
> Free the structure in case the allocation of the firmware structure fails during the firmware initialization.
>
> ---
>
> Additionally, the commit title says close to nothing to the reader as well.
> How about: Fix memory leak in FW initialization
>
> But as I stated above you should clarify what kind of memory we are talking about here.
>
> Also just out of interest have you ever actually experienced issues with this?
> It seems to me that the situation where you run out of Kernel memory should be quite rare.

Yes, I just detected such a bug with a static analysis tool and I have
no idea how to trigger it.
I will submit a v2 to reword the commit message.

Thanks,
Jiasheng

>
> Regards,
> Sebastian
>
>  >
>  > Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP")
>  > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
>  > ---
>  >  .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c      | 5 ++++-
>  >  1 file changed, 4 insertions(+), 1 deletion(-)
>  >
>  > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
>  > index ff23b225db70..1b0bc47355c0 100644
>  > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
>  > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
>  > @@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use
>  >      }
>  >
>  >      fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL);
>  > -    if (!fw)
>  > +    if (!fw) {
>  > +        scp_put(scp);
>  >          return ERR_PTR(-ENOMEM);
>  > +    }
>  > +
>  >      fw->type = SCP;
>  >      fw->ops = &mtk_vcodec_rproc_msg;
>  >      fw->scp = scp;
>  > --
>  > 2.25.1
>  >
>  >
>  >
>

[PATCH v2] media: mediatek: vcodec: Fix a resource leak related to the scp device in FW initialization

[Jiasheng Jiang]

On Mediatek devices with a system companion processor (SCP) the mtk_scp
structure has to be removed explicitly to avoid a resource leak.
Free the structure in case the allocation of the firmware structure fails
during the firmware initialization.

Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP")
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
Changelog:

v1 -> v2:

1. Reword the commit message to give more context.
---
 .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c      | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
index ff23b225db70..1b0bc47355c0 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c
@@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use
 	}
 
 	fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL);
-	if (!fw)
+	if (!fw) {
+		scp_put(scp);
 		return ERR_PTR(-ENOMEM);
+	}
+
 	fw->type = SCP;
 	fw->ops = &mtk_vcodec_rproc_msg;
 	fw->scp = scp;
-- 
2.25.1