[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.Kuehling at amd.com>
> Sent: Friday, May 31, 2024 4:37 AM
> To: Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; Kim, Jonathan <Jonathan.Kim at amd.com>; Huang, Tim <Tim.Huang at 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.Zhang at 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.kuehling at 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));
>>