Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init

[Miao Wang <shankerwangmiao@gmail.com>]

> 2024年7月27日 01:55，Sunil V L <sunilvl@ventanamicro.com> 写道：
> 
> On Sat, Jul 27, 2024 at 12:39:03AM +0800, Miao Wang wrote:
>> Hi,
>> 
>> Thanks for your quick reply.
>> 
>>> 2024年7月27日 00:05，Sudeep Holla <sudeep.holla@arm.com> 写道：
>>> 
>>> On Fri, Jul 26, 2024 at 11:03:01PM +0800, Miao Wang via B4 Relay wrote:
>>>> From: Miao Wang <shankerwangmiao@gmail.com>
>>>> 
>>>> So that we avoid arch-specific code in general ACPI initialization flow.
>>>> Other architectures can also have chance to define their own
>>>> arch-specific acpi initialization process if necessary.
>>>> 
>>> 
>>> Nice, but I assume you are adding something similar to another arch(riscv
>>> or loongarch ?). It would be nice to have those changes as well together to
>>> make it easy to understand the intention much quicker.
>> 
>> Yes, you are right about it. I'm trying to add some codes for loongarch,
>> after DSDT is loaded and namespace is created, before the devices are
>> enumerated, so I'll have chance to add a _DEP method to one of the device
>> using acpi_install_method to provide compatibility for some early loongarch
>> devices which are produced before the loongarch related ACPI standard is
>> finalized.
>> 
> I have arch-specific initialization need for RISC-V as well. So, good to
> see this patch!.
> 
>>> 
>>>> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
>>>> ---
>>>> arch/arm64/include/asm/acpi.h | 2 ++
>>>> drivers/acpi/arm64/init.c     | 2 +-
>>>> drivers/acpi/bus.c            | 2 +-
>>>> include/linux/acpi.h          | 6 +++---
>>>> 4 files changed, 7 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>>>> index a407f9cd549e..0d24e920e143 100644
>>>> --- a/arch/arm64/include/asm/acpi.h
>>>> +++ b/arch/arm64/include/asm/acpi.h
>>>> @@ -188,4 +188,6 @@ static inline void acpi_map_cpus_to_nodes(void) { }
>>>> 
>>>> #define ACPI_TABLE_UPGRADE_MAX_PHYS MEMBLOCK_ALLOC_ACCESSIBLE
>>>> 
>>>> +#define ACPI_HAVE_ARCH_INIT
>>>> +
>>> 
>>> There is nothing core arm66 arch specific in acpi_arm_init() and hence it
>>> is in drivers/acpi/arm64. I would like to avoid adding anything in arch/arm64
>>> if possible. Also I don't think we need to define this ACPI_HAVE_ARCH_INIT
>>> 
>>>> #endif /*_ASM_ACPI_H*/
>>>> diff --git a/drivers/acpi/arm64/init.c b/drivers/acpi/arm64/init.c
>>>> index d0c8aed90fd1..7a47d8095a7d 100644
>>>> --- a/drivers/acpi/arm64/init.c
>>>> +++ b/drivers/acpi/arm64/init.c
>>>> @@ -2,7 +2,7 @@
>>>> #include <linux/acpi.h>
>>>> #include "init.h"
>>>> 
>>>> -void __init acpi_arm_init(void)
>>>> +void __init acpi_arch_init(void)
>>> 
>>> Keep the name acpi_arm_init as is.
>>> 
>>>> {
>>>> if (IS_ENABLED(CONFIG_ACPI_AGDI))
>>>> acpi_agdi_init();
>>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>>> index 284bc2e03580..662f69e379ef 100644
>>>> --- a/drivers/acpi/bus.c
>>>> +++ b/drivers/acpi/bus.c
>>>> @@ -1458,7 +1458,7 @@ static int __init acpi_init(void)
>>>> acpi_viot_early_init();
>>>> acpi_hest_init();
>>>> acpi_ghes_init();
>>>> - acpi_arm_init();
>>>> + acpi_arch_init();
>>> 
>>> Here we need acpi_arch_init() like you have changed.
>>> 
>>>> acpi_scan_init();
>>>> acpi_ec_init();
>>>> acpi_debugfs_init();
>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>>> index f0b95c76c707..3c3a83499c2d 100644
>>>> --- a/include/linux/acpi.h
>>>> +++ b/include/linux/acpi.h
>>>> @@ -1517,10 +1517,10 @@ static inline int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>>>> }
>>>> #endif
>>>> 
>>>> -#ifdef CONFIG_ARM64
>>>> -void acpi_arm_init(void);
>>>> +#ifdef ACPI_HAVE_ARCH_INIT
>>>> +void acpi_arch_init(void);
>>> 
>>> This is bit inconsistent. The Makefile is still conditional on
>>> CONFIG_ARM64 while here you move to ACPI_HAVE_ARCH_INIT.
>>> So while not just undefine and redefine acpi_arch_init to acpi_arm_init.
>>> Something like this must work ?
>>> 
>>> #define acpi_arch_init() do { }while(0)
>>> 
>>> #ifdef CONFIG_ARM64
>>> #undef acpi_arch_init
>>> #define acpi_arch_init() acpi_arm_init()
>>> #endif
>> 
>> It will work. However I can see the pattern in other parts, where
>> the definition of a macro named HAVE_xxx is checked, and define an
>> inline static function with empty body if such macro is not defined
>> or define a function prototype with the same name otherwise, like
>> acpi_arch_set_root_pointer. I'm just trying to follow this pattern.
>> 
> I was thinking to make it weak function similar to cpc_read_ffh().
> Wouldn't it be better than ifdefery?

I believe there would be performance loss for those arches with a stub
function definition if a weak function is used (correct me if wrong).
So the approach with a static inline stub is more common in the kernel
code.

Cheers,

Miao Wang