Re: [PATCH 9/9] drivers/acpi: Use explicitly specified types

[Michal Orzel <michal.orzel@arm.com>]

On 22.06.2022 12:36, Jan Beulich wrote:
> On 20.06.2022 09:02, Michal Orzel wrote:
>> According to MISRA C 2012 Rule 8.1, types shall be explicitly
>> specified. Fix all the findings reported by cppcheck with misra addon
>> by substituting implicit type 'unsigned' to explicit 'unsigned int'.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> This patch may not be applicable as these files come from Linux.
> 
> We've diverged quite far, so the Linux origin isn't really a concern
> (anymore).
> 
Ok.
>> --- a/xen/drivers/acpi/tables/tbfadt.c
>> +++ b/xen/drivers/acpi/tables/tbfadt.c
>> @@ -235,7 +235,7 @@ void __init acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 lengt
>>  		ACPI_WARNING((AE_INFO,
>>  			      "FADT (revision %u) is longer than ACPI 5.0 version,"
>>  			      " truncating length %u to %zu",
>> -			      table->revision, (unsigned)length,
>> +			      table->revision, (unsigned int)length,
> 
> Since we generally try to avoid casts where not needed - did you consider
> dropping the cast here instead of fiddling with it? Aiui this wouldn't be
> a problem since we assume sizeof(int) >= 4 and since types more narrow
> than int would be converted to int (which in turn is generally printing
> okay with %u). Strictly speaking it would want to be PRIu32 ...
> 
The reason I did not consider it was to make the review process easier by performing only
the mechanical change being part of fixing 8.1 rule. However I'm fully ok to drop the cast.

Changing the format specifier to PRIu32 for length would require using PRIu8 for table->revision
to be coherent.

>> --- a/xen/drivers/acpi/tables/tbutils.c
>> +++ b/xen/drivers/acpi/tables/tbutils.c
>> @@ -481,7 +481,7 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
>>  			if (ACPI_FAILURE(status)) {
>>  				ACPI_WARNING((AE_INFO,
>>  					      "Truncating %u table entries!",
>> -					      (unsigned)
>> +					      (unsigned int)
>>  					      (acpi_gbl_root_table_list.size -
>>  					       acpi_gbl_root_table_list.
>>  					       count)));
> 
> Same here then, except PRIu32 wouldn't be correct to use in this case.
> 
Why is it so given that both size and count are of type u32?

> Jan

Cheers,
Michal