Re: [PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

Re: [PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Fixing the coverity issue of:
> mtk_drm_cmdq_pkt_destroy frees address of mtk_crtc->cmdq_handle
> 
> So remove the free function.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 7627122fd1c0 ("drm/mediatek: Add cmdq_handle in mtk_crtc")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 4bc45cdbddf1..c7b03e564095 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -148,7 +148,6 @@ static void mtk_drm_cmdq_pkt_destroy(struct
> cmdq_pkt *pkt)
>  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> >buf_size,
>  			 DMA_TO_DEVICE);
>  	kfree(pkt->va_base);
> -	kfree(pkt);


Also fix the same problem in mtk_drm_cmdq_pkt_create().

Regards,
CK

>  }
>  #endif
>  

Re: [PATCH 2/5] drm/mediatek: Add cnt checking for coverity issue

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> CERT-C Characters and Strings (CERT STR31-C)
> all_drm_priv[cnt] evaluates to an address that could be at negative
> offset of an array.
> 
> In mtk_drm_get_all_drm_priv():
> Guarantee that storage for strings has sufficient space for character
> data and the null terminator.
> 
> So change cnt to unsigned int and check its max value.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195
> multi mmsys support")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 86255a066faf..fcfa10332166 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -378,7 +378,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> device *dev)
>  	const struct of_device_id *of_id;
>  	struct device_node *node;
>  	struct device *drm_dev;
> -	int cnt = 0;
> +	unsigned int cnt = 0;
>  	int i, j;
>  
>  	for_each_child_of_node(phandle->parent, node) {
> @@ -397,7 +397,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> device *dev)
>  			continue;
>  
>  		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
> -		if (all_drm_priv[cnt] && all_drm_priv[cnt]-
> >mtk_drm_bound)
> +		if (cnt < MAX_CRTC && all_drm_priv[cnt] &&
> all_drm_priv[cnt]->mtk_drm_bound)
>  			cnt++;


I would like to add below statement here:

		if (cnt == MAX_CRTC)
			break;

Regards,
CK

>  	}
>  

Re: [PATCH 3/5] drm/mediatek: Add initialization for mtk_gem_obj

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Calling mtk_gem_obj = kzalloc() which returns uninitialized memory,
> because mtk_gem_obj is uninitialized.
> 
> It may cause using uninitialized value mtk_gem_obj->base.resv
> when calling drm_gem_object_init().
> 
> So add initialization for mtk_gem_obj.

So weird, nacked by me.

Regards,
CK

> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..9b8f72ed12e4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -33,7 +33,7 @@ static const struct drm_gem_object_funcs
> mtk_drm_gem_object_funcs = {
>  static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device
> *dev,
>  						unsigned long size)
>  {
> -	struct mtk_drm_gem_obj *mtk_gem_obj;
> +	struct mtk_drm_gem_obj *mtk_gem_obj = NULL;
>  	int ret;
>  
>  	size = round_up(size, PAGE_SIZE);

Re: [PATCH 4/5] drm/mediatek: Add casting before assign

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Add casting before assign to avoid the unintentional integer overflow
> or unintended sign extension.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c   |  2 +-
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 20 +++++++++++---------
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index 9b8f72ed12e4..537e83b95001 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file
> *file_priv, struct drm_device *dev,
>  	int ret;
>  
>  	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> -	args->size = args->pitch * args->height;
> +	args->size = (unsigned long)args->pitch * args->height;

The prototype is

struct drm_mode_create_dump {
	__u32 height;
	__u32 pitch;
	__u64 size;
};

Do you want to case to "__u64"?

>  
>  	mtk_gem = mtk_drm_gem_create(dev, args->size, false);
>  	if (IS_ERR(mtk_gem))
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 31f9420aff6f..a1337f386bbf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -140,7 +140,7 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_gem_object *gem;
>  	struct mtk_drm_gem_obj *mtk_gem;
> -	unsigned int pitch, format;
> +	unsigned int pitch, format, cpp;
>  	u64 modifier;
>  	dma_addr_t addr;
>  	dma_addr_t hdr_addr = 0;
> @@ -151,11 +151,12 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  	addr = mtk_gem->dma_addr;
>  	pitch = fb->pitches[0];
>  	format = fb->format->format;
> +	cpp = (unsigned int)fb->format->cpp[0];
>  	modifier = fb->modifier;
>  
>  	if (modifier == DRM_FORMAT_MOD_LINEAR) {
> -		addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
> -		addr += (new_state->src.y1 >> 16) * pitch;
> +		addr += (dma_addr_t)(new_state->src.x1 >> 16) * cpp;
> +		addr += (dma_addr_t)(new_state->src.y1 >> 16) * pitch;
>  	} else {
>  		int width_in_blocks = ALIGN(fb->width,
> AFBC_DATA_BLOCK_WIDTH)
>  				      / AFBC_DATA_BLOCK_WIDTH;
> @@ -167,17 +168,18 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  
>  		hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE;
>  		pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH *
> -			AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0];
> +			AFBC_DATA_BLOCK_HEIGHT * cpp;
>  
>  		hdr_size = ALIGN(hdr_pitch * height_in_blocks,
> AFBC_HEADER_ALIGNMENT);
>  
> -		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
> -			   AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> +		hdr_addr = addr +
> +			   (dma_addr_t)hdr_pitch * y_offset_in_blocks +
> +			   (dma_addr_t)AFBC_HEADER_BLOCK_SIZE *
> x_offset_in_blocks;
>  		/* The data plane is offset by 1 additional block. */
>  		addr = addr + hdr_size +
> -		       pitch * y_offset_in_blocks +
> -		       AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> -		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> +		       (dma_addr_t)pitch * y_offset_in_blocks +
> +		       (dma_addr_t)AFBC_DATA_BLOCK_WIDTH *
> AFBC_DATA_BLOCK_HEIGHT *
> +		       (dma_addr_t)cpp * (x_offset_in_blocks + 1);
>  	}

I would like you to add a variable 'u32 offset' for this calculating,
and I think u32 is enough for this calculating and it's not necessary
to do any casting.

Regards,
CK

>  
>  	mtk_plane_state->pending.enable = true;

Re: [PATCH 5/5] drm/mediatek: Fix dereference before null check

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Null-checking state suggests that it may be null, but it has already
> been dereferenced on drm_atomic_get_new_plane_state(state, plane).
> 
> The parameter state will never be NULL currently, so just remove the
> state is NULL flow in this function.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 5ddb0bd4ddc3 ("drm/atomic: Pass the full state to planes async
> atomic check and update")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index a1337f386bbf..e14b2920d242 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -103,8 +103,7 @@ static void mtk_drm_plane_destroy_state(struct
> drm_plane *plane,
>  static int mtk_plane_atomic_async_check(struct drm_plane *plane,
>  					struct drm_atomic_state *state)
>  {
> -	struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(state,
> -									
> 	 plane);
> +	struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(state, plane);

This is not related to this patch, so move to another patch.

Regards,
CK

>  	struct drm_crtc_state *crtc_state;
>  	int ret;
>  
> @@ -122,11 +121,7 @@ static int mtk_plane_atomic_async_check(struct
> drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (state)
> -		crtc_state = drm_atomic_get_existing_crtc_state(state,
> -								new_pla
> ne_state->crtc);
> -	else /* Special case for asynchronous cursor updates. */
> -		crtc_state = new_plane_state->crtc->state;
> +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> new_plane_state->crtc);
>  
>  	return drm_atomic_helper_check_plane_state(plane->state,
> crtc_state,
>  						   DRM_PLANE_NO_SCALING
> ,

Re: [PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:06 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Fixing the coverity issue of:
> > mtk_drm_cmdq_pkt_destroy frees address of mtk_crtc->cmdq_handle
> > 
> > So remove the free function.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 7627122fd1c0 ("drm/mediatek: Add cmdq_handle in mtk_crtc")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 4bc45cdbddf1..c7b03e564095 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -148,7 +148,6 @@ static void mtk_drm_cmdq_pkt_destroy(struct
> > cmdq_pkt *pkt)
> >  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> > > buf_size,
> > 
> >  			 DMA_TO_DEVICE);
> >  	kfree(pkt->va_base);
> > -	kfree(pkt);
> 
> 
> Also fix the same problem in mtk_drm_cmdq_pkt_create().
> 
> Regards,
> CK

OK, I'll also fix them. Thanks for the reminder.

Regards,
Jason-JH.Lin
> 
> >  }
> >  #endif
> >  

Re: [PATCH 2/5] drm/mediatek: Add cnt checking for coverity issue

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:21 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > CERT-C Characters and Strings (CERT STR31-C)
> > all_drm_priv[cnt] evaluates to an address that could be at negative
> > offset of an array.
> > 
> > In mtk_drm_get_all_drm_priv():
> > Guarantee that storage for strings has sufficient space for
> > character
> > data and the null terminator.
> > 
> > So change cnt to unsigned int and check its max value.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195
> > multi mmsys support")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 86255a066faf..fcfa10332166 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -378,7 +378,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> >  	const struct of_device_id *of_id;
> >  	struct device_node *node;
> >  	struct device *drm_dev;
> > -	int cnt = 0;
> > +	unsigned int cnt = 0;
> >  	int i, j;
> >  
> >  	for_each_child_of_node(phandle->parent, node) {
> > @@ -397,7 +397,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> >  			continue;
> >  
> >  		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
> > -		if (all_drm_priv[cnt] && all_drm_priv[cnt]-
> > > mtk_drm_bound)
> > 
> > +		if (cnt < MAX_CRTC && all_drm_priv[cnt] &&
> > all_drm_priv[cnt]->mtk_drm_bound)
> >  			cnt++;
> 
> 
> I would like to add below statement here:
> 
> 		if (cnt == MAX_CRTC)
> 			break;
> 
> Regards,
> CK

OK, I'll change to this statement.


Regards,
Jason-JH.Lin
> 
> >  	}
> >  

Re: [PATCH 3/5] drm/mediatek: Add initialization for mtk_gem_obj

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:27 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Calling mtk_gem_obj = kzalloc() which returns uninitialized memory,
> > because mtk_gem_obj is uninitialized.
> > 
> > It may cause using uninitialized value mtk_gem_obj->base.resv
> > when calling drm_gem_object_init().
> > 
> > So add initialization for mtk_gem_obj.
> 
> So weird, nacked by me.
> 
> Regards,
> CK
> 
I've checked the coverity issue:
1. alloc_fn: Calling kzalloc which returns uninitialized memory. 
2. assign: Assigning: mtk_gem_obj = kzalloc(424UL, 3264U), which points
to uninitialized data.
   mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL);

/include/linux/slab.h
733: static inline void *kzalloc(size_t size, gfp_t flags)
734: {
1. uninit_buffer: Call to kmalloc(size, flags | 0x100U) returns an
uninitialized buffer. [Note: The source code implementation of the
function has been overridden by a builtin model.]
2. return: Returning kmalloc(size, flags | 0x100U).


I think it may be a misjudgment to kzalloc returning an uninitialized
memory, so I'll drop this path.

Regards,
Jason-JH.Lin

> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > index a25b28d3ee90..9b8f72ed12e4 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > @@ -33,7 +33,7 @@ static const struct drm_gem_object_funcs
> > mtk_drm_gem_object_funcs = {
> >  static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device
> > *dev,
> >  						unsigned long size)
> >  {
> > -	struct mtk_drm_gem_obj *mtk_gem_obj;
> > +	struct mtk_drm_gem_obj *mtk_gem_obj = NULL;
> >  	int ret;
> >  
> >  	size = round_up(size, PAGE_SIZE);

Re: [PATCH 4/5] drm/mediatek: Add casting before assign

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 09:05 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Add casting before assign to avoid the unintentional integer
> > overflow
> > or unintended sign extension.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_gem.c   |  2 +-
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 20 +++++++++++---------
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > index 9b8f72ed12e4..537e83b95001 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file
> > *file_priv, struct drm_device *dev,
> >  	int ret;
> >  
> >  	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> > -	args->size = args->pitch * args->height;
> > +	args->size = (unsigned long)args->pitch * args->height;
> 
> The prototype is
> 
> struct drm_mode_create_dump {
> 	__u32 height;
> 	__u32 pitch;
> 	__u64 size;
> };
> 
> Do you want to case to "__u64"?
> 
Yes, it should be (unsigned long long), but I'll change it to:
args->size = (__u64)args->pitch * args->height;

> >  
> >  	mtk_gem = mtk_drm_gem_create(dev, args->size, false);
> >  	if (IS_ERR(mtk_gem))
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index 31f9420aff6f..a1337f386bbf 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -140,7 +140,7 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	struct drm_framebuffer *fb = new_state->fb;
> >  	struct drm_gem_object *gem;
> >  	struct mtk_drm_gem_obj *mtk_gem;
> > -	unsigned int pitch, format;
> > +	unsigned int pitch, format, cpp;
> >  	u64 modifier;
> >  	dma_addr_t addr;
> >  	dma_addr_t hdr_addr = 0;
> > @@ -151,11 +151,12 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	addr = mtk_gem->dma_addr;
> >  	pitch = fb->pitches[0];
> >  	format = fb->format->format;
> > +	cpp = (unsigned int)fb->format->cpp[0];
> >  	modifier = fb->modifier;
> >  
> >  	if (modifier == DRM_FORMAT_MOD_LINEAR) {
> > -		addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
> > -		addr += (new_state->src.y1 >> 16) * pitch;
> > +		addr += (dma_addr_t)(new_state->src.x1 >> 16) * cpp;
> > +		addr += (dma_addr_t)(new_state->src.y1 >> 16) * pitch;
> >  	} else {
> >  		int width_in_blocks = ALIGN(fb->width,
> > AFBC_DATA_BLOCK_WIDTH)
> >  				      / AFBC_DATA_BLOCK_WIDTH;
> > @@ -167,17 +168,18 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  
> >  		hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE;
> >  		pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH *
> > -			AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0];
> > +			AFBC_DATA_BLOCK_HEIGHT * cpp;
> >  
> >  		hdr_size = ALIGN(hdr_pitch * height_in_blocks,
> > AFBC_HEADER_ALIGNMENT);
> >  
> > -		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
> > -			   AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> > +		hdr_addr = addr +
> > +			   (dma_addr_t)hdr_pitch * y_offset_in_blocks +
> > +			   (dma_addr_t)AFBC_HEADER_BLOCK_SIZE *
> > x_offset_in_blocks;
> >  		/* The data plane is offset by 1 additional block. */
> >  		addr = addr + hdr_size +
> > -		       pitch * y_offset_in_blocks +
> > -		       AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> > -		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> > +		       (dma_addr_t)pitch * y_offset_in_blocks +
> > +		       (dma_addr_t)AFBC_DATA_BLOCK_WIDTH *
> > AFBC_DATA_BLOCK_HEIGHT *
> > +		       (dma_addr_t)cpp * (x_offset_in_blocks + 1);
> >  	}
> 
> I would like you to add a variable 'u32 offset' for this calculating,
> and I think u32 is enough for this calculating and it's not necessary
> to do any casting.
> 
> Regards,
> CK

I think x_offset_in_blocks and y_offset_in_blocks may be negative,
so I'll use 'int offset' for this calculating.

Regards,
Jason-JH.Lin

> 
> >  
> >  	mtk_plane_state->pending.enable = true;

Re: [PATCH 5/5] drm/mediatek: Fix dereference before null check

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 09:10 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Null-checking state suggests that it may be null, but it has
> > already
> > been dereferenced on drm_atomic_get_new_plane_state(state, plane).
> > 
> > The parameter state will never be NULL currently, so just remove
> > the
> > state is NULL flow in this function.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 5ddb0bd4ddc3 ("drm/atomic: Pass the full state to planes
> > async
> > atomic check and update")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index a1337f386bbf..e14b2920d242 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -103,8 +103,7 @@ static void mtk_drm_plane_destroy_state(struct
> > drm_plane *plane,
> >  static int mtk_plane_atomic_async_check(struct drm_plane *plane,
> >  					struct drm_atomic_state *state)
> >  {
> > -	struct drm_plane_state *new_plane_state =
> > drm_atomic_get_new_plane_state(state,
> > -									
> > 	 plane);
> > +	struct drm_plane_state *new_plane_state =
> > drm_atomic_get_new_plane_state(state, plane);
> 
> This is not related to this patch, so move to another patch.
> 
> Regards,
> CK

OK, I'll drop the modification here.

Regards,
Jason-JH.Lin
> 
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret;
> >  
> > @@ -122,11 +121,7 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (state)
> > -		crtc_state = drm_atomic_get_existing_crtc_state(state,
> > -								new_pla
> > ne_state->crtc);
> > -	else /* Special case for asynchronous cursor updates. */
> > -		crtc_state = new_plane_state->crtc->state;
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > new_plane_state->crtc);
> >  
> >  	return drm_atomic_helper_check_plane_state(plane->state,
> > crtc_state,
> >  						   DRM_PLANE_NO_SCALING
> > ,