2023-04-05 11:15:07

by Sunil V L

[permalink] [raw]
Subject: Re: [PATCH V4 22/23] platform/surface: Disable for RISC-V

On Wed, Apr 05, 2023 at 11:33:00AM +0200, Maximilian Luz wrote:
> On 4/4/23 20:20, Sunil V L wrote:
> > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
> > in allmodconfig build. However, RISC-V doesn't support sub-word
> > atomics which is used by this driver. Due to this, the build fails
> > with below error.
> >
> > In function ‘ssh_seq_next’,
> > inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
> > ././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
> > 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > ./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
> > 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
> > | ^
> > ././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
> > 387 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^~~~~~~~~~~~~~~~~~~~
> > ././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
> > 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > | ^~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> > 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> > | ^~~~~~~~~~~~~~~~
> > ./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
> > 335 | BUILD_BUG(); \
> > | ^~~~~~~~~
> > ./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
> > 344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
> > | ^~~~~~~~~
> > ./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
> > 1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
> > | ^~~~~~~~~~~~
> > drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
> > 61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
> > | ^~~~~~~
> >
> > So, disable this driver for RISC-V even when ACPI is enabled for now.
>
> CONFIG_SURFACE_PLATFORMS should be enabled for ARM64 || X86 || COMPILE_TEST only,
> so I guess the issue only happens when compiling with the latter enabled?
>
> I'm not aware of any current plans of MS to release RISC-V-based Surface
> devices, so you could maybe also just explicitly disable CONFIG_SURFACE_PLATFORMS.
> In any case, I don't see any issues with disabling the whole platform/surface
> or only individual drivers for RISC-V, so for either solution:
>
> Acked-by: Maximilian Luz <[email protected]>
>
Hi Maximilian,

Thanks!. Yes, COMPILE_TEST gets enabled for allmodconfig builds. Since
the whole intention of COMPILE_TEST appears to be able to compile-test
drivers on a platform than they are supposed to be used, I think it is
better not to skip whole set of drivers but only that which can not build.
So, I prefer to keep this change as is.

Thanks,
Sunil


2023-04-05 11:43:21

by Maximilian Luz

[permalink] [raw]
Subject: Re: [PATCH V4 22/23] platform/surface: Disable for RISC-V

On 4/5/23 13:11, Sunil V L wrote:
> On Wed, Apr 05, 2023 at 11:33:00AM +0200, Maximilian Luz wrote:
>> On 4/4/23 20:20, Sunil V L wrote:
>>> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
>>> in allmodconfig build. However, RISC-V doesn't support sub-word
>>> atomics which is used by this driver. Due to this, the build fails
>>> with below error.
>>>
>>> In function ‘ssh_seq_next’,
>>> inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
>>> ././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
>>> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>> | ^
>>> ./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
>>> 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
>>> | ^
>>> ././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
>>> 387 | __compiletime_assert(condition, msg, prefix, suffix)
>>> | ^~~~~~~~~~~~~~~~~~~~
>>> ././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
>>> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>> | ^~~~~~~~~~~~~~~~~~~
>>> ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
>>> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>>> | ^~~~~~~~~~~~~~~~~~
>>> ./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>>> 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
>>> | ^~~~~~~~~~~~~~~~
>>> ./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
>>> 335 | BUILD_BUG(); \
>>> | ^~~~~~~~~
>>> ./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
>>> 344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
>>> | ^~~~~~~~~
>>> ./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
>>> 1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
>>> | ^~~~~~~~~~~~
>>> drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
>>> 61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
>>> | ^~~~~~~
>>>
>>> So, disable this driver for RISC-V even when ACPI is enabled for now.
>>
>> CONFIG_SURFACE_PLATFORMS should be enabled for ARM64 || X86 || COMPILE_TEST only,
>> so I guess the issue only happens when compiling with the latter enabled?
>>
>> I'm not aware of any current plans of MS to release RISC-V-based Surface
>> devices, so you could maybe also just explicitly disable CONFIG_SURFACE_PLATFORMS.
>> In any case, I don't see any issues with disabling the whole platform/surface
>> or only individual drivers for RISC-V, so for either solution:
>>
>> Acked-by: Maximilian Luz <[email protected]>
>>
> Hi Maximilian,
>
> Thanks!. Yes, COMPILE_TEST gets enabled for allmodconfig builds. Since
> the whole intention of COMPILE_TEST appears to be able to compile-test
> drivers on a platform than they are supposed to be used, I think it is
> better not to skip whole set of drivers but only that which can not build.
> So, I prefer to keep this change as is.

Hi Sunil,

What I wanted to say with my previous mail: I'm fairly confident that
platform/surface drivers will not be actively used on RISC-V hardware any
time soon (not sure if that came over in this way). But whatever you/others
prefer, I'm happy with either.

Best regards,
Max