[PATCH v2 0/4] Fix mediatek-drm coverity issues

[PATCH v2 0/4] Fix mediatek-drm coverity issues

[Jason-JH.Lin]

Add this patch series to fix some mediatek-drm coverity issues.

Change in v2:
1. remove kfree(pkt) in mtk_drm_crtc_create_pkt().
2. change the statement of cnt reach to MAX_CRTC.
3. drop the mtk_gem_obj initialized patch.
4. change casting from unsined long to __u64.
5. add 'int offset' for multiplier calculation.
6. drop the unrelavaent modification in dereference null check patch.

Jason-JH.Lin (4):
  drm/mediatek: Remove freeing not dynamic allocated memory
  drm/mediatek: Add cnt checking for coverity issue
  drm/mediatek: Add casting before assign
  drm/mediatek: Fix dereference before null check

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |  7 ++----
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  5 ++++-
 drivers/gpu/drm/mediatek/mtk_drm_gem.c   |  2 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 28 ++++++++++++------------
 4 files changed, 21 insertions(+), 21 deletions(-)

-- 
2.18.0

[PATCH v2 1/4] drm/mediatek: Remove freeing not dynamic allocated memory

[Jason-JH.Lin]

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 | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..8d44f3df116f 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -116,10 +116,9 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
 	dma_addr_t dma_addr;
 
 	pkt->va_base = kzalloc(size, GFP_KERNEL);
-	if (!pkt->va_base) {
-		kfree(pkt);
+	if (!pkt->va_base)
 		return -ENOMEM;
-	}
+
 	pkt->buf_size = size;
 	pkt->cl = (void *)client;
 
@@ -129,7 +128,6 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
 	if (dma_mapping_error(dev, dma_addr)) {
 		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
 		kfree(pkt->va_base);
-		kfree(pkt);
 		return -ENOMEM;
 	}
 
@@ -145,7 +143,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);
 }
 #endif
 
-- 
2.18.0

[PATCH v2 2/4] drm/mediatek: Add cnt checking for coverity issue

[Jason-JH.Lin]

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 6dcb4ba2466c..fc217e0acd45 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -354,7 +354,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) {
@@ -375,6 +375,9 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
 		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
 		if (all_drm_priv[cnt] && all_drm_priv[cnt]->mtk_drm_bound)
 			cnt++;
+
+		if (cnt == MAX_CRTC)
+			break;
 	}
 
 	if (drm_priv->data->mmsys_dev_num == cnt) {
-- 
2.18.0

[PATCH v2 3/4] drm/mediatek: Add casting before assign

[Jason-JH.Lin]

1. Add casting before assign to avoid the unintentional integer
   overflow or unintended sign extension.
2. Add a int varriable for multiplier calculation instead of calculating
   different types multiplier with dma_addr_t varriable directly.

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 | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index a25b28d3ee90..0c7878bc0b37 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 = (__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..1cd41454d545 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	dma_addr_t addr;
 	dma_addr_t hdr_addr = 0;
 	unsigned int hdr_pitch = 0;
+	int offset;
 
 	gem = fb->obj[0];
 	mtk_gem = to_mtk_gem_obj(gem);
@@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	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;
+		offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
+		addr += offset;
+		offset = (new_state->src.y1 >> 16) * pitch;
+		addr += offset;
 	} else {
 		int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
 				      / AFBC_DATA_BLOCK_WIDTH;
@@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 				       / AFBC_DATA_BLOCK_HEIGHT;
 		int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
 		int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
-		int hdr_size;
+		int hdr_size, hdr_offset;
 
 		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];
 
 		hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
+		hdr_offset = hdr_pitch * y_offset_in_blocks +
+			AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
+		hdr_addr = addr + hdr_offset;
 
-		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
-			   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);
+		offset = pitch * y_offset_in_blocks +
+			 AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
+			 fb->format->cpp[0] * (x_offset_in_blocks + 1);
+		addr = addr + hdr_size + offset;
 	}
 
 	mtk_plane_state->pending.enable = true;
-- 
2.18.0

[PATCH v2 4/4] drm/mediatek: Fix dereference before null check

[Jason-JH.Lin]

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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 1cd41454d545..4828ffa75467 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -122,11 +122,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_plane_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,
-- 
2.18.0

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

[AngeloGioacchino Del Regno]

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 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")

Fixes tag goes before your S-o-b tag.

Anyway,
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 6dcb4ba2466c..fc217e0acd45 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -354,7 +354,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) {
> @@ -375,6 +375,9 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
>   		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
>   		if (all_drm_priv[cnt] && all_drm_priv[cnt]->mtk_drm_bound)
>   			cnt++;
> +
> +		if (cnt == MAX_CRTC) > +			break;
>   	}
>   
>   	if (drm_priv->data->mmsys_dev_num == cnt) {

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

[AngeloGioacchino Del Regno]

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 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")

Fixes tag goes before your S-o-b tag.

Anyway,
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..8d44f3df116f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -116,10 +116,9 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
>   	dma_addr_t dma_addr;
>   
>   	pkt->va_base = kzalloc(size, GFP_KERNEL);
> -	if (!pkt->va_base) {
> -		kfree(pkt);
> +	if (!pkt->va_base)
>   		return -ENOMEM;
> -	}
> +
>   	pkt->buf_size = size;
>   	pkt->cl = (void *)client;
>   
> @@ -129,7 +128,6 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
>   	if (dma_mapping_error(dev, dma_addr)) {
>   		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
>   		kfree(pkt->va_base);
> -		kfree(pkt);
>   		return -ENOMEM;
>   	}
>   
> @@ -145,7 +143,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);
>   }
>   #endif
>   

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

[AngeloGioacchino Del Regno]

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 1. Add casting before assign to avoid the unintentional integer
>     overflow or unintended sign extension.
> 2. Add a int varriable for multiplier calculation instead of calculating
>     different types multiplier with dma_addr_t varriable directly.
> 
> 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 | 22 +++++++++++++---------
>   2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..0c7878bc0b37 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 = (__u64)args->pitch * args->height;

We could avoid explicit casting here if you do

	args->size = args->pitch;
	args->size *= 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..1cd41454d545 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	dma_addr_t addr;
>   	dma_addr_t hdr_addr = 0;
>   	unsigned int hdr_pitch = 0;
> +	int offset;

...but offset can never be negative, can it?
in that case, this should be unsigned int.

>   
>   	gem = fb->obj[0];
>   	mtk_gem = to_mtk_gem_obj(gem);
> @@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	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;
> +		offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
> +		addr += offset;
> +		offset = (new_state->src.y1 >> 16) * pitch;
> +		addr += offset;
>   	} else {
>   		int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
>   				      / AFBC_DATA_BLOCK_WIDTH;
> @@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   				       / AFBC_DATA_BLOCK_HEIGHT;
>   		int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
>   		int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
> -		int hdr_size;
> +		int hdr_size, hdr_offset;
>   
>   		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];
>   
>   		hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
> +		hdr_offset = hdr_pitch * y_offset_in_blocks +
> +			AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> +		hdr_addr = addr + hdr_offset;
>   
> -		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
> -			   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);
> +		offset = pitch * y_offset_in_blocks +
> +			 AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> +			 fb->format->cpp[0] * (x_offset_in_blocks + 1);
> +		addr = addr + hdr_size + offset;
>   	}
>   
>   	mtk_plane_state->pending.enable = true;

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

[AngeloGioacchino Del Regno]

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 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")

Fixes before S-o-b...

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

Hi Angelo,


On Wed, 2023-06-14 at 10:43 +0200, AngeloGioacchino Del Regno wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content.

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:

> 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")


Fixes before S-o-b...


Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


Thanks, I'll swap it at the next version.

Regards,
Jason-JH.Lin


[-- Attachment #2: Type: text/html, Size: 3226 bytes --]

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

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/plain, Size: 4672 bytes --]

Hi Angelo,

Thanks for the reviews.

On Wed, 2023-06-14 at 10:43 +0200, AngeloGioacchino Del Regno wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content.

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:

> 1. Add casting before assign to avoid the unintentional integer

>     overflow or unintended sign extension.

> 2. Add a int varriable for multiplier calculation instead of calculating

>     different types multiplier with dma_addr_t varriable directly.

>

> 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 | 22 +++++++++++++---------

>   2 files changed, 14 insertions(+), 10 deletions(-)

>

> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c

> index a25b28d3ee90..0c7878bc0b37 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 = (__u64)args->pitch * args->height;


We could avoid explicit casting here if you do


args->size = args->pitch;

args->size *= args->height;


OK, Thanks for your advice.

I'll take this.


>

>   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..1cd41454d545 100644

> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c

> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c

> @@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>   dma_addr_t addr;

>   dma_addr_t hdr_addr = 0;

>   unsigned int hdr_pitch = 0;

> +int offset;


...but offset can never be negative, can it?

in that case, this should be unsigned int.


src.x1 and src.y1 are 'int', so they can be negative.

But I'm not sure does anyone would set the negative to them.


I think using 'unsigned int' for offset would be very big after multiply with negative src.x1 or src.y1.

So I just use 'int' here to avoid that case.


Do you think I should use 'unsigned int' for offset and add the boundary checking for addr?


Regard,

Jason-JH.Lin


>

>   gem = fb->obj[0];

>   mtk_gem = to_mtk_gem_obj(gem);

> @@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>   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;

> +offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];

> +addr += offset;

> +offset = (new_state->src.y1 >> 16) * pitch;

> +addr += offset;

>   } else {

>   int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)

>         / AFBC_DATA_BLOCK_WIDTH;

> @@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>          / AFBC_DATA_BLOCK_HEIGHT;

>   int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;

>   int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;

> -int hdr_size;

> +int hdr_size, hdr_offset;

>

>   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];

>

>   hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);

> +hdr_offset = hdr_pitch * y_offset_in_blocks +

> +AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;

> +hdr_addr = addr + hdr_offset;

>

> -hdr_addr = addr + hdr_pitch * y_offset_in_blocks +

> -   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);

> +offset = pitch * y_offset_in_blocks +

> + AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *

> + fb->format->cpp[0] * (x_offset_in_blocks + 1);

> +addr = addr + hdr_size + offset;

>   }

>

>   mtk_plane_state->pending.enable = true;




[-- Attachment #2: Type: text/html, Size: 8368 bytes --]