* [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
@ 2021-05-14 14:05 Arnd Bergmann
2021-05-14 14:21 ` Maximilian Luz
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2021-05-14 14:05 UTC (permalink / raw)
To: Maximilian Luz, Nathan Chancellor, Nick Desaulniers, Hans de Goede
Cc: Arnd Bergmann, linux-kernel, clang-built-linux
From: Arnd Bergmann <arnd@arndb.de>
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.
Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
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
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
2021-05-14 14:05 [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning Arnd Bergmann
@ 2021-05-14 14:21 ` Maximilian Luz
2021-05-14 19:38 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Maximilian Luz @ 2021-05-14 14:21 UTC (permalink / raw)
To: Arnd Bergmann, Nathan Chancellor, Nick Desaulniers, Hans de Goede
Cc: Arnd Bergmann, linux-kernel, clang-built-linux
On 5/14/21 4:05 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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.
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@gmail.com>
Thanks,
Max
> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
2021-05-14 14:21 ` Maximilian Luz
@ 2021-05-14 19:38 ` Hans de Goede
2021-05-14 19:41 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2021-05-14 19:38 UTC (permalink / raw)
To: Maximilian Luz, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers
Cc: Arnd Bergmann, linux-kernel, clang-built-linux
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@arndb.de>
>>
>> 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@gmail.com>
>
> Thanks,
> Max
>
>> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> 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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
2021-05-14 19:38 ` Hans de Goede
@ 2021-05-14 19:41 ` Hans de Goede
2021-05-14 20:04 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2021-05-14 19:41 UTC (permalink / raw)
To: Maximilian Luz, Arnd Bergmann, Nathan Chancellor, Nick Desaulniers
Cc: Arnd Bergmann, linux-kernel, clang-built-linux
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@arndb.de>
>>>
>>> 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@vger.kernel.org
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@gmail.com>
>>
>> Thanks,
>> Max
>>
>>> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>> 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
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
2021-05-14 19:41 ` Hans de Goede
@ 2021-05-14 20:04 ` Arnd Bergmann
2021-05-14 22:07 ` Maximilian Luz
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2021-05-14 20:04 UTC (permalink / raw)
To: Hans de Goede
Cc: Maximilian Luz, Nathan Chancellor, Nick Desaulniers,
Linux Kernel Mailing List, clang-built-linux
On Fri, May 14, 2021 at 9:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/14/21 9:38 PM, Hans de Goede wrote:
> > On 5/14/21 4:21 PM, Maximilian Luz wrote:
> >> On 5/14/21 4:05 PM, Arnd Bergmann wrote:
> >>> From: Arnd Bergmann <arnd@arndb.de>
> >>>
> >>> 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,
Thanks for taking a careful look. After a little experimentation I managed to
come up with a different workaround that avoids the cast.
> 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@vger.kernel.org
> was missing from the Cc even though get_maintainer.pl lists it.
I checked this as well now: the entries for the various surface drivers all
contain a reference to platform-driver-x86@vger.kernel.org, but (at least
in v5.13-rc1) the entry for that subsystem that lists the include file
does not list this email.
Sending v2 to the list now.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning
2021-05-14 20:04 ` Arnd Bergmann
@ 2021-05-14 22:07 ` Maximilian Luz
0 siblings, 0 replies; 6+ messages in thread
From: Maximilian Luz @ 2021-05-14 22:07 UTC (permalink / raw)
To: Arnd Bergmann, Hans de Goede
Cc: Nathan Chancellor, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux
On 5/14/21 10:04 PM, Arnd Bergmann wrote:
> On Fri, May 14, 2021 at 9:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> 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@vger.kernel.org
>> was missing from the Cc even though get_maintainer.pl lists it.
>
> I checked this as well now: the entries for the various surface drivers all
> contain a reference to platform-driver-x86@vger.kernel.org, but (at least
> in v5.13-rc1) the entry for that subsystem that lists the include file
> does not list this email.
Right, I must have forgotten to add that, sorry. I'll send in a patch to
fix that in a second.
Regards,
Max
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-14 22:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 14:05 [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning Arnd Bergmann
2021-05-14 14:21 ` Maximilian Luz
2021-05-14 19:38 ` Hans de Goede
2021-05-14 19:41 ` Hans de Goede
2021-05-14 20:04 ` Arnd Bergmann
2021-05-14 22:07 ` Maximilian Luz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).