Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning

[Hans de Goede]

Hi,

On 5/14/21 4:21 PM, Maximilian Luz wrote:
> On 5/14/21 4:05 PM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>>
>> Clang complains about the assignment of SSAM_ANY_IID to
>> ssam_device_uid->instance:
>>
>> drivers/platform/surface/surface_aggregator_registry.c:478:25: error: implicit conversion from 'int' to '__u8' (aka 'unsigned char') changes value from 65535 to 255 [-Werror,-Wconstant-conversion]
>>          { SSAM_VDEV(HUB, 0x02, SSAM_ANY_IID, 0x00) },
>>          ~                      ^~~~~~~~~~~~
>> include/linux/surface_aggregator/device.h:71:23: note: expanded from macro 'SSAM_ANY_IID'
>>   #define SSAM_ANY_IID            0xffff
>>                                  ^~~~~~
>> include/linux/surface_aggregator/device.h:126:63: note: expanded from macro 'SSAM_VDEV'
>>          SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
>>                                                                       ^~~
>> include/linux/surface_aggregator/device.h:102:41: note: expanded from macro 'SSAM_DEVICE'
>>          .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,                        \
>>                                                 ^~~
>>
>> The assignment doesn't actually happen, but clang checks the type limits
>> before checking whether this assignment is reached. Shut up the warning
>> using an explicit type cast.
> 
> I'm not too happy about this fix as (I believe) it will also shut up any
> valid GCC error message in case those macros are used with non-u8 (and
> non-SSAM_ANY_xxx) values.

Since you're the maintainer of this code, I'll go with your judgement here,
esp. since as the commit msg states SSAM_ANY_IID is never actually
assigned to .instance, instead it gets set to 0.

So this is a false-positive compiler warning, which may be best fixed in
the compiler itself.

With that said I think this is the second time this comes up now, maybe
we should add a comment to the code about the clang warning ?

Regards,

Hans


> 
> I'll let others judge and decide what's preferred, however.
> 
> In case you're deciding to apply this, feel free to add:
> 
> Reviewed-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> 
> Thanks,
> Max
> 
>> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
>> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>> ---
>>   include/linux/surface_aggregator/device.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
>> index 4441ad667c3f..90df092ed565 100644
>> --- a/include/linux/surface_aggregator/device.h
>> +++ b/include/linux/surface_aggregator/device.h
>> @@ -98,9 +98,9 @@ struct ssam_device_uid {
>>                | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),    \
>>       .domain   = d,                                \
>>       .category = cat,                            \
>> -    .target   = ((tid) != SSAM_ANY_TID) ? (tid) : 0,            \
>> -    .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,            \
>> -    .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0                \
>> +    .target   = ((tid) != SSAM_ANY_TID) ? (u8)(tid) : 0,            \
>> +    .instance = ((iid) != SSAM_ANY_IID) ? (u8)(iid) : 0,            \
>> +    .function = ((fun) != SSAM_ANY_FUN) ? (u8)(fun) : 0            \
>>     /**
>>    * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
>>
>