[PATCH] drm/amdgpu: Fix format character cut-off issues in amdgpu_vcn_early_init()

[Lazar, Lijo]

On 3/20/2024 8:28 PM, SRINIVASAN SHANMUGAM wrote:
> 
> On 3/20/2024 3:12 PM, Lazar, Lijo wrote:
>>
>> On 3/20/2024 2:15 PM, Srinivasan Shanmugam wrote:
>>> The issue was present in the lines where 'fw_name' was being formatted.
>>> This fix ensures that the output is not truncated
>>>
>>> Fixes the below with gcc W=1:
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c: In function
>>> ‘amdgpu_vcn_early_init’:
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:66: warning: ‘snprintf’
>>> output may be truncated before the last format character
>>> [-Wformat-truncation=]
>>>    102 |                 snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s.bin", ucode_prefix);
>>>       
>>> |                                                                  ^
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:17: note: ‘snprintf’
>>> output between 12 and 41 bytes into a destination of size 40
>>>    102 |                 snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s.bin", ucode_prefix);
>>>        |                
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:66: warning: ‘snprintf’
>>> output may be truncated before the last format character
>>> [-Wformat-truncation=]
>>>    102 |                 snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s.bin", ucode_prefix);
>>>       
>>> |                                                                  ^
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:17: note: ‘snprintf’
>>> output between 12 and 41 bytes into a destination of size 40
>>>    102 |                 snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s.bin", ucode_prefix);
>>>        |                
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:73: warning: ‘.bin’
>>> directive output may be truncated writing 4 bytes into a region of
>>> size between 2 and 31 [-Wformat-truncation=]
>>>    105 |                         snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s_%d.bin", ucode_prefix, i);
>>>       
>>> |                                                                         ^~~~
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:25: note: ‘snprintf’
>>> output between 14 and 43 bytes into a destination of size 40
>>>    105 |                         snprintf(fw_name, sizeof(fw_name),
>>> "amdgpu/%s_%d.bin", ucode_prefix, i);
>>>        |                        
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Suggested-by: Lijo Lazar <lijo.lazar at amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> index 9c514a606a2f..f994ee6c548d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>> @@ -94,7 +94,7 @@ static void amdgpu_vcn_idle_work_handler(struct
>>> work_struct *work);
>>>   int amdgpu_vcn_early_init(struct amdgpu_device *adev)
>>>   {
>>>       char ucode_prefix[30];
>> Hi Srini,
>>
>> Sorry, if I miscommunicated. Suggestion was to reduce prefix size to 25
>> as the max prefix length is possibly length of dimgrey_cavefish_vcn.
> Hi Lijo,
> 
> my mistake, the fw_name size must have been 53.
> 
> How 53? -> The size of ucode_prefix is 30, so the maximum length of
> ucode_prefix is 29 characters (since one character is needed for the
> null terminator). The maximum number of digits in an integer is 10.
> Therefore, the maximum possible length of the string written into
> fw_name is 14 + 29 + 10 = 53 characters.
> 
> On the other hand reducing ucode_prefix to 25 from 30:
> 
> 1. The length of the string "amdgpu/%s.bin" is 12 characters plus the
> length of ucode_prefix. The length of the string "amdgpu/%s_%d.bin" is
> 14 characters plus the length of ucode_prefix and the number of digits
> in i.
> 

> If ucode_prefix is 25 characters long, the maximum length of the string
> written into fw_name is 14 + 25 + 1 (for a single digit i) = 40
> characters, which is exactly the size of fw_name.
> 
> Is that this solution assumes that i will not be more than 9 (a single
> digit)?. If i can be a number with more than one digit, should we need
> to increase the size of fw_name accordingly.?
> 

With 25, fw_name size required is 24 (no null char) + 13 (rest of string
including null char) + 1 (digit). 'i' is assumed to be a single digit.
(We don't need to calculate this, that warning already gives this
calculation).

Assumption is we won't be having more than 10 (again, no random number
usage for suffix, only single digit usage) variants of VCN FW that needs
to be loaded for a single SOC.

> 2. If you reduce the size of ucode_prefix to 25, it means that it can
> store a version string of up to 24 characters (since one character is
> needed for the null terminator).
> 
> For example: if tomorrow, if we get something like
> dimgrey_cavefish_smc_xxxyyyzz - then in this case yyyzz would be lost?
> is that in "amdgpu_ucode_ip_version_decode " &
> "amdgpu_ucode_legacy_naming" , is that are we always ensuring that it
> will never be longer than 24 characters?
> 

Newer FW naming follow the syntax vcn_xx.yy.zz (IP version), and that is
not more than 25 chars.

Anyway, you may use 50 for fw_name or adjust prefix size - both are fine.

Thanks,
Lijo

> Thanks,
> Srini
>>
>> With fw_name[42] also, you may run into 43 bytes (30 prefix + 13 for
>> others) warning.
>>
>> Thanks,
>> Lijo
>>
>>> -    char fw_name[40];
>>> +    char fw_name[42];
>>>       int r, i;
>>>         for (i = 0; i < adev->vcn.num_vcn_inst; i++) {