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

[Hans de Goede]

Hi,

On 5/14/21 9:38 PM, Hans de Goede wrote:
> 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,

p.s.

I just went to patchwork to drop this patch from the queue:

https://patchwork.kernel.org/projecat/platform-driver-x86/list/

But it never got added there because platform-driver-x86@xxxxxxxxxxxxxxx
was missing from the Cc even though get_maintainer.pl lists it.

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
>>>
>>