diff mbox series

[RFC,v2,20/35] ACPI: Rename acpi_processor_hotadd_init and remove pre-processor guards

Message ID 20230913163823.7880-21-james.morse@arm.com (mailing list archive)
State Handled Elsewhere
Headers show
Series ACPI/arm64: add support for virtual cpuhotplug | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 0bb80ecc33a8
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 5 and now 5
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 284 this patch: 284
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 320 this patch: 320
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 25 this patch: 25
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 49 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

James Morse Sept. 13, 2023, 4:38 p.m. UTC
acpi_processor_hotadd_init() will make a CPU present by mapping it
based on its hardware id.

'hotadd_init' is ambiguous once there are two different behaviours
for cpu hotplug. This is for toggling the _STA present bit. Subsequent
patches will add support for toggling the _STA enabled bit, named
acpi_processor_make_enabled().

Rename it acpi_processor_make_present() to make it clear this is
for CPUs that were not previously present.

Expose the function prototypes it uses to allow the preprocessor
guards to be removed. The IS_ENABLED() check will let the compiler
dead-code elimination pass remove this if it isn't going to be
used.

Signed-off-by: James Morse <james.morse@arm.com>
---
 drivers/acpi/acpi_processor.c | 14 +++++---------
 include/linux/acpi.h          |  2 --
 2 files changed, 5 insertions(+), 11 deletions(-)

Comments

Jonathan Cameron Sept. 14, 2023, 2:17 p.m. UTC | #1
On Wed, 13 Sep 2023 16:38:08 +0000
James Morse <james.morse@arm.com> wrote:

> acpi_processor_hotadd_init() will make a CPU present by mapping it
> based on its hardware id.
> 
> 'hotadd_init' is ambiguous once there are two different behaviours
> for cpu hotplug. This is for toggling the _STA present bit. Subsequent
> patches will add support for toggling the _STA enabled bit, named
> acpi_processor_make_enabled().
> 
> Rename it acpi_processor_make_present() to make it clear this is
> for CPUs that were not previously present.
> 
> Expose the function prototypes it uses to allow the preprocessor
> guards to be removed. The IS_ENABLED() check will let the compiler
> dead-code elimination pass remove this if it isn't going to be
> used.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/acpi/acpi_processor.c | 14 +++++---------
>  include/linux/acpi.h          |  2 --
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 75257fae10e7..22a15a614f95 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -182,13 +182,15 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>  #endif /* CONFIG_X86 */
>  
>  /* Initialization */
> -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
> -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> +static int acpi_processor_make_present(struct acpi_processor *pr)
>  {
>  	unsigned long long sta;
>  	acpi_status status;
>  	int ret;
>  
> +	if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))
> +		return -ENODEV;
> +
>  	if (invalid_phys_cpuid(pr->phys_id))
>  		return -ENODEV;
>  
> @@ -222,12 +224,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  	cpu_maps_update_done();
>  	return ret;
>  }
> -#else
> -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> -{
> -	return -ENODEV;
> -}
> -#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
>  
>  static int acpi_processor_get_info(struct acpi_device *device)
>  {
> @@ -335,7 +331,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>  	 *  because cpuid <-> apicid mapping is persistent now.
>  	 */
>  	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> -		int ret = acpi_processor_hotadd_init(pr);
> +		int ret = acpi_processor_make_present(pr);
>  
>  		if (ret)
>  			return ret;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 651dd43976a9..b7ab85857bb7 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -316,12 +316,10 @@ static inline int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu,
>  }
>  #endif
>  
> -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
>  /* Arch dependent functions for cpu hotplug support */
>  int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
>  		 int *pcpu);
>  int acpi_unmap_cpu(int cpu);

I've lost track somewhat but I think the definitions of these are still under ifdefs
which is messy if nothing else and might cause build issues.

> -#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
>  
>  #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>  int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
Gavin Shan Sept. 18, 2023, 5:50 a.m. UTC | #2
On 9/15/23 00:17, Jonathan Cameron wrote:
> On Wed, 13 Sep 2023 16:38:08 +0000
> James Morse <james.morse@arm.com> wrote:
> 
>> acpi_processor_hotadd_init() will make a CPU present by mapping it
>> based on its hardware id.
>>
>> 'hotadd_init' is ambiguous once there are two different behaviours
>> for cpu hotplug. This is for toggling the _STA present bit. Subsequent
>> patches will add support for toggling the _STA enabled bit, named
>> acpi_processor_make_enabled().
>>
>> Rename it acpi_processor_make_present() to make it clear this is
>> for CPUs that were not previously present.
>>
>> Expose the function prototypes it uses to allow the preprocessor
>> guards to be removed. The IS_ENABLED() check will let the compiler
>> dead-code elimination pass remove this if it isn't going to be
>> used.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>>   drivers/acpi/acpi_processor.c | 14 +++++---------
>>   include/linux/acpi.h          |  2 --
>>   2 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 75257fae10e7..22a15a614f95 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -182,13 +182,15 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>>   #endif /* CONFIG_X86 */
>>   
>>   /* Initialization */
>> -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
>> -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> +static int acpi_processor_make_present(struct acpi_processor *pr)
>>   {
>>   	unsigned long long sta;
>>   	acpi_status status;
>>   	int ret;
>>   
>> +	if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))
>> +		return -ENODEV;
>> +
>>   	if (invalid_phys_cpuid(pr->phys_id))
>>   		return -ENODEV;
>>   
>> @@ -222,12 +224,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>>   	cpu_maps_update_done();
>>   	return ret;
>>   }
>> -#else
>> -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> -{
>> -	return -ENODEV;
>> -}
>> -#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
>>   
>>   static int acpi_processor_get_info(struct acpi_device *device)
>>   {
>> @@ -335,7 +331,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>>   	 *  because cpuid <-> apicid mapping is persistent now.
>>   	 */
>>   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
>> -		int ret = acpi_processor_hotadd_init(pr);
>> +		int ret = acpi_processor_make_present(pr);
>>   
>>   		if (ret)
>>   			return ret;
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 651dd43976a9..b7ab85857bb7 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -316,12 +316,10 @@ static inline int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu,
>>   }
>>   #endif
>>   
>> -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
>>   /* Arch dependent functions for cpu hotplug support */
>>   int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
>>   		 int *pcpu);
>>   int acpi_unmap_cpu(int cpu);
> 
> I've lost track somewhat but I think the definitions of these are still under ifdefs
> which is messy if nothing else and might cause build issues.
> 

Yup, it's not safe to use 'if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))' in
acpi_processor_make_present() until the ifdefs are removed for those two functions
in individual architectures.

>> -#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
>>   
>>   #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>>   int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);

Thanks,
Gavin
Russell King (Oracle) Oct. 23, 2023, 8:01 p.m. UTC | #3
On Mon, Sep 18, 2023 at 03:50:09PM +1000, Gavin Shan wrote:
> 
> On 9/15/23 00:17, Jonathan Cameron wrote:
> > On Wed, 13 Sep 2023 16:38:08 +0000
> > James Morse <james.morse@arm.com> wrote:
> > 
> > > acpi_processor_hotadd_init() will make a CPU present by mapping it
> > > based on its hardware id.
> > > 
> > > 'hotadd_init' is ambiguous once there are two different behaviours
> > > for cpu hotplug. This is for toggling the _STA present bit. Subsequent
> > > patches will add support for toggling the _STA enabled bit, named
> > > acpi_processor_make_enabled().
> > > 
> > > Rename it acpi_processor_make_present() to make it clear this is
> > > for CPUs that were not previously present.
> > > 
> > > Expose the function prototypes it uses to allow the preprocessor
> > > guards to be removed. The IS_ENABLED() check will let the compiler
> > > dead-code elimination pass remove this if it isn't going to be
> > > used.
> > > 
> > > Signed-off-by: James Morse <james.morse@arm.com>
> > > ---
> > >   drivers/acpi/acpi_processor.c | 14 +++++---------
> > >   include/linux/acpi.h          |  2 --
> > >   2 files changed, 5 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > > index 75257fae10e7..22a15a614f95 100644
> > > --- a/drivers/acpi/acpi_processor.c
> > > +++ b/drivers/acpi/acpi_processor.c
> > > @@ -182,13 +182,15 @@ static void __init acpi_pcc_cpufreq_init(void) {}
> > >   #endif /* CONFIG_X86 */
> > >   /* Initialization */
> > > -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
> > > -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > +static int acpi_processor_make_present(struct acpi_processor *pr)
> > >   {
> > >   	unsigned long long sta;
> > >   	acpi_status status;
> > >   	int ret;
> > > +	if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))
> > > +		return -ENODEV;
> > > +
> > >   	if (invalid_phys_cpuid(pr->phys_id))
> > >   		return -ENODEV;
> > > @@ -222,12 +224,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >   	cpu_maps_update_done();
> > >   	return ret;
> > >   }
> > > -#else
> > > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > > -{
> > > -	return -ENODEV;
> > > -}
> > > -#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
> > >   static int acpi_processor_get_info(struct acpi_device *device)
> > >   {
> > > @@ -335,7 +331,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
> > >   	 *  because cpuid <-> apicid mapping is persistent now.
> > >   	 */
> > >   	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> > > -		int ret = acpi_processor_hotadd_init(pr);
> > > +		int ret = acpi_processor_make_present(pr);
> > >   		if (ret)
> > >   			return ret;
> > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > > index 651dd43976a9..b7ab85857bb7 100644
> > > --- a/include/linux/acpi.h
> > > +++ b/include/linux/acpi.h
> > > @@ -316,12 +316,10 @@ static inline int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu,
> > >   }
> > >   #endif
> > > -#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
> > >   /* Arch dependent functions for cpu hotplug support */
> > >   int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
> > >   		 int *pcpu);
> > >   int acpi_unmap_cpu(int cpu);
> > 
> > I've lost track somewhat but I think the definitions of these are still under ifdefs
> > which is messy if nothing else and might cause build issues.
> > 
> 
> Yup, it's not safe to use 'if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))' in
> acpi_processor_make_present() until the ifdefs are removed for those two functions
> in individual architectures.

The same thing appears in a final patch that James seems to have added
to the repository:

ACPI: processor: Only call arch_unregister_cpu() if HOTPLUG_CPU is selected

in which acpi_processor_post_eject() has this change:

-       if (!device)
+       if (!IS_ENABLED(CONFIG_HOTPLUG_CPU) || !device)

I'm wondering if that's caused by a previous patch making the weak
definition of arch_unregister_cpu() dependent on HOTPLUG_CPU, and
whether dropping that ifdef around the function would be better. I
think I already asked that question, but this final patch seems to be
the confirmation that we need to provide a definition of it.

I think the reason James did it like that is because unregister_cpu()
is dependent upon CONFIG_HOTPLUG_CPU, but it's probably better to do:

#ifdef CONFIG_HOTPLUG_CPU
void __weak arch_unregister_cpu(int num)
{
	unregister_cpu(&per_cpu(cpu_devices, num));
}
#else
void __weak arch_unregister_cpu(int num)
{
}
#endif

Agreed?
diff mbox series

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 75257fae10e7..22a15a614f95 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -182,13 +182,15 @@  static void __init acpi_pcc_cpufreq_init(void) {}
 #endif /* CONFIG_X86 */
 
 /* Initialization */
-#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
-static int acpi_processor_hotadd_init(struct acpi_processor *pr)
+static int acpi_processor_make_present(struct acpi_processor *pr)
 {
 	unsigned long long sta;
 	acpi_status status;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_PRESENT_CPU))
+		return -ENODEV;
+
 	if (invalid_phys_cpuid(pr->phys_id))
 		return -ENODEV;
 
@@ -222,12 +224,6 @@  static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	cpu_maps_update_done();
 	return ret;
 }
-#else
-static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
-{
-	return -ENODEV;
-}
-#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
 
 static int acpi_processor_get_info(struct acpi_device *device)
 {
@@ -335,7 +331,7 @@  static int acpi_processor_get_info(struct acpi_device *device)
 	 *  because cpuid <-> apicid mapping is persistent now.
 	 */
 	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
-		int ret = acpi_processor_hotadd_init(pr);
+		int ret = acpi_processor_make_present(pr);
 
 		if (ret)
 			return ret;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 651dd43976a9..b7ab85857bb7 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -316,12 +316,10 @@  static inline int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu,
 }
 #endif
 
-#ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU
 /* Arch dependent functions for cpu hotplug support */
 int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
 		 int *pcpu);
 int acpi_unmap_cpu(int cpu);
-#endif /* CONFIG_ACPI_HOTPLUG_PRESENT_CPU */
 
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);