Re: [PATCH] media: verisilicon: rockchip: Fix leaks in init

From: Frank Li <Frank.li@oss.nxp.com>
To: Michail Tatas <michail.tatas@gmail.com>
Cc: nicolas.dufresne@collabora.com, benjamin.gaignard@collabora.com,
	p.zabel@pengutronix.de, mchehab@kernel.org, heiko@sntech.de,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: verisilicon: rockchip: Fix leaks in init
Date: Fri, 21 Aug 2026 10:15:36 -0500	[thread overview]
Message-ID: <aohrmDgzhRxC6M_D@SMW015318> (raw)
In-Reply-To: <aodup3TleCfdxcF3@michalis-linux>

On Fri, Aug 21, 2026 at 12:16:23AM +0300, Michail Tatas wrote:
> On Wed, Aug 19, 2026 at 12:56:40PM -0400, Frank Li wrote:
> > On Wed, Aug 19, 2026 at 12:28:12PM +0300, Michail Tatas wrote:
> > > if one of the dma_alloc_coherent in the init fucntion fails then
> > > the previously allocated ones leak.
> > >
> > > Fix by freeing them in the error path.
> > >
> > > Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")
> > > Signed-off-by: Michail Tatas <michail.tatas@gmail.com>
> > > ---
> > >  .../verisilicon/rockchip_vpu981_hw_av1_dec.c  | 68 +++++++++++++++----
> > >  1 file changed, 55 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> > > index e4e21ad37323..fa77fd402412 100644
> > > --- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> > > +++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> > > @@ -369,6 +369,7 @@ void rockchip_vpu981_av1_dec_exit(struct hantro_ctx *ctx)
> > >
> > >  int rockchip_vpu981_av1_dec_init(struct hantro_ctx *ctx)
> > >  {
> > > +	int ret = 0;
> > >  	struct hantro_dev *vpu = ctx->dev;
> > >  	struct hantro_av1_dec_hw_ctx *av1_dec = &ctx->av1_dec;
> > >
> > > @@ -377,39 +378,54 @@ int rockchip_vpu981_av1_dec_init(struct hantro_ctx *ctx)
> > >  	av1_dec->global_model.cpu = dma_alloc_coherent(vpu->dev, GLOBAL_MODEL_SIZE,
> > >  						       &av1_dec->global_model.dma,
> > >  						       GFP_KERNEL);
> > > -	if (!av1_dec->global_model.cpu)
> > > -		return -ENOMEM;
> > > +	if (!av1_dec->global_model.cpu) {
> > > +		ret = -ENOMEM;
> > > +		goto global_model_cpu_err;
> > > +	}
> > > +
> > >  	av1_dec->global_model.size = GLOBAL_MODEL_SIZE;
> > >
> > >  	av1_dec->tile_info.cpu = dma_alloc_coherent(vpu->dev, AV1_TILE_INFO_SIZE,
> > >  						    &av1_dec->tile_info.dma,
> > >  						    GFP_KERNEL);
> > > -	if (!av1_dec->tile_info.cpu)
> > > -		return -ENOMEM;
> > > +	if (!av1_dec->tile_info.cpu) {
> > > +		ret = -ENOMEM;
> > > +		goto tile_info_cpu_err;
> > > +	}
> > > +
> >
> > This function is called by hantro_probe() if I am correct
> >
> > there are dmam_alloc_coherent(), use dmam_alloc_coherent() will simple
> > error handle and tear down.
> >
> > Frank
>
> Hello Frank,
>
> I did not know about the dmam* functions thanks for letting me know.
> I read some of the docs about them and they seem to not apply to
> this particular case since this code is not called from the probe
> function but rather the hantro_start_streaming function. Since
> dmam_alloc_coherent() only releases memory at device removal,
> it wouldn't free them when the stream ends and a new stream would
> re-allocate while the old buffers lingered until detach.
> So I think manual dma_free_coherent() error path is the correct
> fit here.

Okay, make sense.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

Frank

>
> Best regards,
> Michail

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip