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

[Sunil V L]

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