| Message ID | 20230404182037.863533-23-sunilvl@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
On 4 Apr 2023, at 19:20, Sunil V L <sunilvl@ventanamicro.com> 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. Why not? Compilers and libatomic do, so surely the Linux kernel should too. > 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. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > drivers/platform/surface/aggregator/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig > index c114f9dd5fe1..88afc38ffdc5 100644 > --- a/drivers/platform/surface/aggregator/Kconfig > +++ b/drivers/platform/surface/aggregator/Kconfig > @@ -4,7 +4,7 @@ > menuconfig SURFACE_AGGREGATOR > tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers" > depends on SERIAL_DEV_BUS > - depends on ACPI > + depends on ACPI && !RISCV If you insist on doing this, at least make it some new config variable that’s self-documenting and means this automatically gets re-enabled when arch/riscv fixes this deficiency? Hard-coding arch lists like this seems like a terrible anti-pattern. Jess > select CRC_CCITT > help > The Surface System Aggregator Module (Surface SAM or SSAM) is an > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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 <luzmaximilian@gmail.com> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > drivers/platform/surface/aggregator/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig > index c114f9dd5fe1..88afc38ffdc5 100644 > --- a/drivers/platform/surface/aggregator/Kconfig > +++ b/drivers/platform/surface/aggregator/Kconfig > @@ -4,7 +4,7 @@ > menuconfig SURFACE_AGGREGATOR > tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers" > depends on SERIAL_DEV_BUS > - depends on ACPI > + depends on ACPI && !RISCV > select CRC_CCITT > help > The Surface System Aggregator Module (Surface SAM or SSAM) is an
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 <luzmaximilian@gmail.com> > 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
Hi Jess, On Wed, Apr 05, 2023 at 05:19:35AM +0100, Jessica Clarke wrote: > On 4 Apr 2023, at 19:20, Sunil V L <sunilvl@ventanamicro.com> 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. > > Why not? Compilers and libatomic do, so surely the Linux kernel should > too. > I think you are probably right. But I don't want to combine that activity with this series. IMO, that should be separate activity. > > 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. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > --- > > drivers/platform/surface/aggregator/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig > > index c114f9dd5fe1..88afc38ffdc5 100644 > > --- a/drivers/platform/surface/aggregator/Kconfig > > +++ b/drivers/platform/surface/aggregator/Kconfig > > @@ -4,7 +4,7 @@ > > menuconfig SURFACE_AGGREGATOR > > tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers" > > depends on SERIAL_DEV_BUS > > - depends on ACPI > > + depends on ACPI && !RISCV > > If you insist on doing this, at least make it some new config variable > that’s self-documenting and means this automatically gets re-enabled > when arch/riscv fixes this deficiency? Hard-coding arch lists like this > seems like a terrible anti-pattern. > I understand your point. But given that this is currently only issue with a single driver from Microsoft and that too only in COMPILE_TEST builds, I think introducing a new config variable is overkill. If we support sub-word atomics in kernel, the option may not be useful much anyway. There are patterns to disable an architecture for COMPILE_TEST builds. Thanks, Sunil
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 <luzmaximilian@gmail.com> >> > 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
diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig index c114f9dd5fe1..88afc38ffdc5 100644 --- a/drivers/platform/surface/aggregator/Kconfig +++ b/drivers/platform/surface/aggregator/Kconfig @@ -4,7 +4,7 @@ menuconfig SURFACE_AGGREGATOR tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers" depends on SERIAL_DEV_BUS - depends on ACPI + depends on ACPI && !RISCV select CRC_CCITT help The Surface System Aggregator Module (Surface SAM or SSAM) is an
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. Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- drivers/platform/surface/aggregator/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)