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 <[email protected]>
>>>
>>> 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 [email protected]
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 <[email protected]>
>>
>> Thanks,
>> Max
>>
>>> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type")
>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>> ---
>>> 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
>>>
>>
On Fri, May 14, 2021 at 9:41 PM Hans de Goede <[email protected]> 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 <[email protected]>
> >>>
> >>> 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 [email protected]
> 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 [email protected], 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
On 5/14/21 10:04 PM, Arnd Bergmann wrote:
> On Fri, May 14, 2021 at 9:41 PM Hans de Goede <[email protected]> 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 [email protected]
>> 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 [email protected], 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