On 2015/3/5 6:29, Rafael J. Wysocki wrote:
> On Wednesday, February 25, 2015 04:39:42 PM Hanjun Guo wrote:
>> CPU hardware ID (phys_id) is defined as u32 in structure acpi_processor,
>> but phys_id is used as int in acpi processor driver, so it will lead to
>> some inconsistence for the drivers.
>>
>> Furthermore, to cater for ACPI arch ports that implement 64 bits CPU
>> ids a generic CPU physical id type is required.
>>
>> So introduce typedef u32 phys_cpuid_t for x86 and ia64, and introduce
>> a macro CPU_PHYS_ID_INVALID as (u32)(-1), use phys_cpuid_t when phys_id
>> defined in acpi processor driver, and replace CPU_PHYS_ID_INVALID as -1
>> for phys_id, this will solve the inconsistence in acpi processor driver,
>> and will prepare for the ACPI on ARM64 too.
>>
>> CC: Rafael J Wysocki <rjw@xxxxxxxxxxxxx>
>> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>> Acked-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>> Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>> [hj: reworked cpu physid map return codes]
>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>> ---
>> arch/ia64/include/asm/acpi.h | 4 ++++
>> arch/ia64/kernel/acpi.c | 2 +-
>> arch/x86/include/asm/acpi.h | 4 ++++
>> arch/x86/kernel/acpi/boot.c | 2 +-
>> drivers/acpi/acpi_processor.c | 7 ++++---
>> drivers/acpi/processor_core.c | 30 +++++++++++++++---------------
>> include/acpi/processor.h | 6 +++---
>> include/linux/acpi.h | 2 +-
>> 8 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
>> index a1d91ab..ca1f0e4 100644
>> --- a/arch/ia64/include/asm/acpi.h
>> +++ b/arch/ia64/include/asm/acpi.h
>> @@ -34,6 +34,10 @@
>> #include <linux/numa.h>
>> #include <asm/numa.h>
>>
>> +typedef u32 phys_cpuid_t;
>> +
>> +#define CPU_PHYS_ID_INVALID (u32)(-1)
>> +
>> #ifdef CONFIG_ACPI
>> extern int acpi_lapic;
>> #define acpi_disabled 0 /* ACPI always enabled on IA64 */
>> diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
>> index 2c44989..067ef44 100644
>> --- a/arch/ia64/kernel/acpi.c
>> +++ b/arch/ia64/kernel/acpi.c
>> @@ -887,7 +887,7 @@ static int _acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu)
>> }
>>
>> /* wrapper to silence section mismatch warning */
>> -int __ref acpi_map_cpu(acpi_handle handle, int physid, int *pcpu)
>> +int __ref acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu)
>> {
>> return _acpi_map_lsapic(handle, physid, pcpu);
>> }
>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
>> index 3a45668..cd788dd 100644
>> --- a/arch/x86/include/asm/acpi.h
>> +++ b/arch/x86/include/asm/acpi.h
>> @@ -32,6 +32,10 @@
>> #include <asm/mpspec.h>
>> #include <asm/realmode.h>
>>
>> +typedef u32 phys_cpuid_t;
>> +
>> +#define CPU_PHYS_ID_INVALID (u32)(-1)
> Can we define those things in one common place instead of having to duplicate
> the definitions for every arch?
As in your reply in patch 14/21, ARM64 will use 64-bit for
phys_cpuid_t.
>
> Also, I'd like to have
>
> static inline bool invalid_cpu_phys_id(phys_cpuid_t phys_id)
> {
> return (int)phys_id < 0;
> }
>
> as that would cover error codes too.
>
> Moreover, why don't you use (phys_cpuid_t)(-1) in the #define? And call is
> PHYS_CPUID_INVALID for that matter (so that the name of the symbol corresponds
> to the name of the type)?
That's pretty fine to me too.
>
>
>> +
>> #ifdef CONFIG_ACPI
>> extern int acpi_lapic;
>> extern int acpi_ioapic;
>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>> index 3d525c6..e4f8582 100644
>> --- a/arch/x86/kernel/acpi/boot.c
>> +++ b/arch/x86/kernel/acpi/boot.c
>> @@ -757,7 +757,7 @@ static int _acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu)
>> }
>>
>> /* wrapper to silence section mismatch warning */
>> -int __ref acpi_map_cpu(acpi_handle handle, int physid, int *pcpu)
>> +int __ref acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu)
>> {
>> return _acpi_map_lsapic(handle, physid, pcpu);
>> }
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 1020b1b..e6c7c56 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -170,7 +170,7 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> acpi_status status;
>> int ret;
>>
>> - if (pr->phys_id == -1)
>> + if (pr->phys_id == CPU_PHYS_ID_INVALID)
>> return -ENODEV;
>>
>> status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
>> @@ -215,7 +215,8 @@ static int acpi_processor_get_info(struct acpi_device *device)
>> union acpi_object object = { 0 };
>> struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>> struct acpi_processor *pr = acpi_driver_data(device);
>> - int phys_id, cpu_index, device_declaration = 0;
>> + phys_cpuid_t phys_id;
>> + int cpu_index, device_declaration = 0;
>> acpi_status status = AE_OK;
>> static int cpu0_initialized;
>> unsigned long long value;
>> @@ -263,7 +264,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>> }
>>
>> phys_id = acpi_get_phys_id(pr->handle, device_declaration, pr->acpi_id);
>> - if (phys_id < 0)
>> + if (phys_id == CPU_PHYS_ID_INVALID)
>> acpi_handle_debug(pr->handle, "failed to get CPU physical ID.\n");
>> pr->phys_id = phys_id;
>>
>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>> index 7962651..a5547dc 100644
>> --- a/drivers/acpi/processor_core.c
>> +++ b/drivers/acpi/processor_core.c
>> @@ -32,7 +32,7 @@ static struct acpi_table_madt *get_madt_table(void)
>> }
>>
>> static int map_lapic_id(struct acpi_subtable_header *entry,
>> - u32 acpi_id, int *apic_id)
>> + u32 acpi_id, phys_cpuid_t *apic_id)
>> {
>> struct acpi_madt_local_apic *lapic =
>> container_of(entry, struct acpi_madt_local_apic, header);
>> @@ -48,7 +48,7 @@ static int map_lapic_id(struct acpi_subtable_header *entry,
>> }
>>
>> static int map_x2apic_id(struct acpi_subtable_header *entry,
>> - int device_declaration, u32 acpi_id, int *apic_id)
>> + int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
>> {
>> struct acpi_madt_local_x2apic *apic =
>> container_of(entry, struct acpi_madt_local_x2apic, header);
>> @@ -65,7 +65,7 @@ static int map_x2apic_id(struct acpi_subtable_header *entry,
>> }
>>
>> static int map_lsapic_id(struct acpi_subtable_header *entry,
>> - int device_declaration, u32 acpi_id, int *apic_id)
>> + int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
>> {
>> struct acpi_madt_local_sapic *lsapic =
>> container_of(entry, struct acpi_madt_local_sapic, header);
>> @@ -83,10 +83,10 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>> return 0;
>> }
>>
>> -static int map_madt_entry(int type, u32 acpi_id)
>> +static phys_cpuid_t map_madt_entry(int type, u32 acpi_id)
>> {
>> unsigned long madt_end, entry;
>> - int phys_id = -1; /* CPU hardware ID */
>> + phys_cpuid_t phys_id = CPU_PHYS_ID_INVALID; /* CPU hardware ID */
>> struct acpi_table_madt *madt;
>>
>> madt = get_madt_table();
>> @@ -117,12 +117,12 @@ static int map_madt_entry(int type, u32 acpi_id)
>> return phys_id;
>> }
>>
>> -static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
>> +static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
>> {
>> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> union acpi_object *obj;
>> struct acpi_subtable_header *header;
>> - int phys_id = -1;
>> + phys_cpuid_t phys_id = CPU_PHYS_ID_INVALID;
>>
>> if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer)))
>> goto exit;
>> @@ -149,27 +149,27 @@ exit:
>> return phys_id;
>> }
>>
>> -int acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id)
>> +phys_cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id)
>> {
>> - int phys_id;
>> + phys_cpuid_t phys_id;
>>
>> phys_id = map_mat_entry(handle, type, acpi_id);
>> - if (phys_id == -1)
>> + if (phys_id == CPU_PHYS_ID_INVALID)
>> phys_id = map_madt_entry(type, acpi_id);
>>
>> return phys_id;
>> }
>>
>> -int acpi_map_cpuid(int phys_id, u32 acpi_id)
>> +int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id)
>> {
>> #ifdef CONFIG_SMP
>> int i;
>> #endif
>>
>> - if (phys_id == -1) {
>> + if (phys_id == CPU_PHYS_ID_INVALID) {
>> /*
>> * On UP processor, there is no _MAT or MADT table.
>> - * So above phys_id is always set to -1.
>> + * So above phys_id is always set to CPU_PHYS_ID_INVALID.
>> *
>> * BIOS may define multiple CPU handles even for UP processor.
>> * For example,
>> @@ -190,7 +190,7 @@ int acpi_map_cpuid(int phys_id, u32 acpi_id)
>> if (nr_cpu_ids <= 1 && acpi_id == 0)
>> return acpi_id;
>> else
>> - return phys_id;
>> + return -1;
> Can we use a proper error code here?
I'm afraid not. In ACPI processor drivers, -1 will be deemed to
invalid cpu logical number, if we return error code here, we need
to modify multi places of "if (cpu_logical_num == -1)" to
"if (! (cpu_logical_num < 0))" too, so for me, I prefer to keep it as
-1, but I'm open for suggestions.
Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
