Re: [PATCH 1/3] media: imx-jpeg: Enhance error handling in buffer allocation

[Ming Qian(OSS)]

Hi Frank,

On 2025/3/27 22:35, Frank Li wrote:
> On Thu, Mar 27, 2025 at 10:37:05AM +0800, ming.qian@xxxxxxxxxxx wrote:
> > From: Ming Qian <ming.qian@xxxxxxxxxxx>
> >
> > In function mxc_jpeg_alloc_slot_data, driver will allocate some dma
> > buffer, but only return error if certain allocation failed.
> >
> > Without cleanup the allocation failure, the next time it will return
> > success directly, but let some buffer be uninitialized.
> > It may result in accessing a null pointer.
> >
> > Clean up if error occurs in the allocation.
> >
> > Signed-off-by: Ming Qian <ming.qian@xxxxxxxxxxx>
>
> Look like it is bug fix, add fixes tags

Yes, I missed it, I will add it in v2

Thanks,
Ming

> Frank
> > ---
> >   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 +++++++++++--------
> >   1 file changed, 27 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > index 0e6ee997284b..12661c177f5a 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > @@ -752,6 +752,32 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
> >   	return -1;
> >   }
> >
> > +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> > +{
> > +	/* free descriptor for decoding/encoding phase */
> > +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> > +			  jpeg->slot_data.desc,
> > +			  jpeg->slot_data.desc_handle);
> > +	jpeg->slot_data.desc = NULL;
> > +	jpeg->slot_data.desc_handle = 0;
> > +
> > +	/* free descriptor for encoder configuration phase / decoder DHT */
> > +	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> > +			  jpeg->slot_data.cfg_desc,
> > +			  jpeg->slot_data.cfg_desc_handle);
> > +	jpeg->slot_data.cfg_desc_handle = 0;
> > +	jpeg->slot_data.cfg_desc = NULL;
> > +
> > +	/* free configuration stream */
> > +	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> > +			  jpeg->slot_data.cfg_stream_vaddr,
> > +			  jpeg->slot_data.cfg_stream_handle);
> > +	jpeg->slot_data.cfg_stream_vaddr = NULL;
> > +	jpeg->slot_data.cfg_stream_handle = 0;
> > +
> > +	jpeg->slot_data.used = false;
> > +}
> > +
> >   static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> >   {
> >   	struct mxc_jpeg_desc *desc;
> > @@ -794,30 +820,11 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
> >   	return true;
> >   err:
> >   	dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", jpeg->slot_data.slot);
> > +	mxc_jpeg_free_slot_data(jpeg);
> >
> >   	return false;
> >   }
> >
> > -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
> > -{
> > -	/* free descriptor for decoding/encoding phase */
> > -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> > -			  jpeg->slot_data.desc,
> > -			  jpeg->slot_data.desc_handle);
> > -
> > -	/* free descriptor for encoder configuration phase / decoder DHT */
> > -	dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> > -			  jpeg->slot_data.cfg_desc,
> > -			  jpeg->slot_data.cfg_desc_handle);
> > -
> > -	/* free configuration stream */
> > -	dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> > -			  jpeg->slot_data.cfg_stream_vaddr,
> > -			  jpeg->slot_data.cfg_stream_handle);
> > -
> > -	jpeg->slot_data.used = false;
> > -}
> > -
> >   static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx *ctx,
> >   					       struct vb2_v4l2_buffer *src_buf,
> >   					       struct vb2_v4l2_buffer *dst_buf)
> > --
> > 2.43.0-rc1