public inbox for linux-acpi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
@ 2024-07-26 15:03 Miao Wang via B4 Relay
  2024-07-26 16:05 ` Sudeep Holla
  0 siblings, 1 reply; 6+ messages in thread
From: Miao Wang via B4 Relay @ 2024-07-26 15:03 UTC (permalink / raw)
  To: linux-acpi, linux-arm-kernel; +Cc: Rafael J. Wysocki, Len Brown, Hanjun Guo

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.

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
+
 #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)
 {
 	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();
 	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);
 #else
-static inline void acpi_arm_init(void) { }
+static inline void acpi_arch_init(void) { }
 #endif
 
 #ifdef CONFIG_ACPI_PCC
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
  2024-07-26 15:03 [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init Miao Wang via B4 Relay
@ 2024-07-26 16:05 ` Sudeep Holla
  2024-07-26 16:39   ` Miao Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Sudeep Holla @ 2024-07-26 16:05 UTC (permalink / raw)
  To: shankerwangmiao
  Cc: linux-acpi, linux-arm-kernel, Rafael J. Wysocki, Sudeep Holla,
	Len Brown, Hanjun Guo

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.

> 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

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
  2024-07-26 16:05 ` Sudeep Holla
@ 2024-07-26 16:39   ` Miao Wang
  2024-07-26 17:55     ` Sunil V L
  0 siblings, 1 reply; 6+ messages in thread
From: Miao Wang @ 2024-07-26 16:39 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-acpi, linux-arm-kernel, Rafael J. Wysocki, Len Brown,
	Hanjun Guo

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.

> 
>> 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.

> --
> Regards,
> Sudeep


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
  2024-07-26 16:39   ` Miao Wang
@ 2024-07-26 17:55     ` Sunil V L
  2024-07-26 18:23       ` Miao Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Sunil V L @ 2024-07-26 17:55 UTC (permalink / raw)
  To: Miao Wang
  Cc: Sudeep Holla, linux-acpi, linux-arm-kernel, Rafael J. Wysocki,
	Len Brown, Hanjun Guo

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?

Thanks
Sunil

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
  2024-07-26 17:55     ` Sunil V L
@ 2024-07-26 18:23       ` Miao Wang
  2024-07-27  6:52         ` Hanjun Guo
  0 siblings, 1 reply; 6+ messages in thread
From: Miao Wang @ 2024-07-26 18:23 UTC (permalink / raw)
  To: Sunil V L
  Cc: Sudeep Holla, linux-acpi, linux-arm-kernel, Rafael J. Wysocki,
	Len Brown, Hanjun Guo


> 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init
  2024-07-26 18:23       ` Miao Wang
@ 2024-07-27  6:52         ` Hanjun Guo
  0 siblings, 0 replies; 6+ messages in thread
From: Hanjun Guo @ 2024-07-27  6:52 UTC (permalink / raw)
  To: Miao Wang, Sunil V L
  Cc: Sudeep Holla, linux-acpi, linux-arm-kernel, Rafael J. Wysocki,
	Len Brown

On 2024/7/27 2:23, Miao Wang wrote:
> 
>> 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:
[...]
>>>>>
>>>>> -#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.

ACPI init is not in the hot code path, no worries for the performance
loss.

Weak function is my preference too :)

Thanks
Hanjun

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-07-27  6:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-26 15:03 [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init Miao Wang via B4 Relay
2024-07-26 16:05 ` Sudeep Holla
2024-07-26 16:39   ` Miao Wang
2024-07-26 17:55     ` Sunil V L
2024-07-26 18:23       ` Miao Wang
2024-07-27  6:52         ` Hanjun Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox