Re: [PATCH 8/8] drm/amdkfd: remove dead code in kfd_create_vcrat_image_gpu

[Felix Kuehling]

On 2024-05-30 21:44, Zhang, Jesse(Jie) wrote:
[AMD Official Use Only - AMD Internal Distribution Only]

Hi Felix,

-----Original Message-----
From: Kuehling, Felix <felix.kuehl...@amd.com>
Sent: Friday, May 31, 2024 4:37 AM
To: Zhang, Jesse(Jie) <jesse.zh...@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; Kim, Jonathan <jonathan....@amd.com>; Huang, Tim 
<tim.hu...@amd.com>
Subject: Re: [PATCH 8/8] drm/amdkfd: remove dead code in 
kfd_create_vcrat_image_gpu


On 2024-05-29 23:50, Jesse Zhang wrote:
Since the value of avail_size is at least VCRAT_SIZE_FOR_GPU(16384),
minus struct crat_header(40UL) and struct crat_subtype_compute(40UL) it cannot 
be less than 0.

Signed-off-by: Jesse Zhang <jesse.zh...@amd.com>
---
   drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 6 ------
   1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 71150d503dc7..ead43386a7ef 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -2213,9 +2213,6 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
        * Modify length and total_entries as subunits are added.
        */
       avail_size -= sizeof(struct crat_header);
-     if (avail_size < 0)
-             return -ENOMEM;
-
Avail_size is passed in from the caller through the *size parameter.
You're making an assumption about the caller here.

[Zhang, Jesse(Jie)]  avil_size is checked at the beginning of 
kfd_create_vcrat_image_gpu
and it cannot be smaller than VCRAT_SIZE_FOR_GPU (16384).

         if (!pcrat_image || avail_size < VCRAT_SIZE_FOR_GPU)
                 return -EINVAL;

Ok, I missed that. Makes sense. Maybe mention it in the commit 
description that kfd_create_vcrat_image_gpu itself checks the avail_size 
at the start. The patch is

Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com>




Regards
Jesse

Regards,
    Felix


       memset(crat_table, 0, sizeof(struct crat_header));

       memcpy(&crat_table->signature, CRAT_SIGNATURE, @@ -2229,9 +2226,6
@@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
        * First fill in the sub type header and then sub type data
        */
       avail_size -= sizeof(struct crat_subtype_computeunit);
-     if (avail_size < 0)
-             return -ENOMEM;
-
       sub_type_hdr = (struct crat_subtype_generic *)(crat_table + 1);
       memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit));