This patch set is some minor cleanups for ACPI processor driver to address the comments which raised by Rafael in ARM64 ACPI core patches.
Boris and Konrad already reviewed the XEN/ACPI part, they seems are ok with the changes for XEN [1] [2].
Sudeep, I didn't change the (int)phys_id < 0 to phys_id == PHYS_CPUID_INVALID as they have the same effects and Rafael prefer the previous one as he mentioned in ACPI core patch set comments, and PHYS_CPUID_INVALID is still needed for typedef phys_cpuid_t in ACPI core, so if you are not ok with it, please let me know.
v3: - Reorder the patches to let "Introduce invalid_logical_cpuid()" as the first one to avoid raising confusions. - Replace "return invalid_logical_cpuid(cpuid) ? false : true;" as "return !invalid_logical_cpuid(cpuid);" - Some updates to the patch subject and changelog - Rebase on top of 4.1-rc3
v2: - rebased on top of 4.1-rc2
[1]: https://lkml.org/lkml/2015/5/5/733 [2]: https://lkml.org/lkml/2015/5/9/222
Hanjun Guo (7): ACPI / processor: Introduce invalid_logical_cpuid() Xen / ACPI / processor: use invalid_logical_cpuid() Xen / ACPI / processor: Remove unneeded NULL check ACPI / processor: remove cpu_index in acpi_processor_get_info() ACPI / processor: remove phys_id in acpi_processor_get_info() ACPI / processor: return specific error instead of -1 ACPI / processor: Introduce invalid_phys_cpuid()
drivers/acpi/acpi_processor.c | 20 +++++++++----------- drivers/acpi/processor_core.c | 10 +++++----- drivers/acpi/processor_pdc.c | 5 +---- drivers/xen/xen-acpi-cpuhotplug.c | 12 +++--------- include/linux/acpi.h | 10 ++++++++++ 5 files changed, 28 insertions(+), 29 deletions(-)
In ACPI processor drivers, we use direct comparisons of cpu logical id with -1 which are error prone in case logical cpuid is accidentally assinged an error code and prevents us from returning an error-encoding cpuid directly in some cases.
So introduce invalid_logical_cpuid() to identify cpu with invalid logical cpu num, then it will be used to replace the direct comparisons with -1.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- drivers/acpi/acpi_processor.c | 5 +++-- drivers/acpi/processor_pdc.c | 5 +---- include/linux/acpi.h | 5 +++++ 3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 58f335c..ac6bda0 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -275,7 +275,8 @@ static int acpi_processor_get_info(struct acpi_device *device) * Handle UP system running SMP kernel, with no CPU * entry in MADT */ - if ((cpu_index == -1) && (num_online_cpus() == 1)) + if (invalid_logical_cpuid(cpu_index) + && (num_online_cpus() == 1)) cpu_index = 0; } pr->id = cpu_index; @@ -285,7 +286,7 @@ static int acpi_processor_get_info(struct acpi_device *device) * less than the max # of CPUs. They should be ignored _iff * they are physically not present. */ - if (pr->id == -1) { + if (invalid_logical_cpuid(pr->id)) { int ret = acpi_processor_hotadd_init(pr); if (ret) return ret; diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c index e5dd808..7cfbda4 100644 --- a/drivers/acpi/processor_pdc.c +++ b/drivers/acpi/processor_pdc.c @@ -52,10 +52,7 @@ static bool __init processor_physically_present(acpi_handle handle) type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0; cpuid = acpi_get_cpuid(handle, type, acpi_id);
- if (cpuid == -1) - return false; - - return true; + return !invalid_logical_cpuid(cpuid); }
static void acpi_set_pdc_bits(u32 *buf) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index e4da5e3..913b49f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -158,6 +158,11 @@ typedef u32 phys_cpuid_t; #define PHYS_CPUID_INVALID (phys_cpuid_t)(-1) #endif
+static inline bool invalid_logical_cpuid(u32 cpuid) +{ + return (int)cpuid < 0; +} + #ifdef CONFIG_ACPI_HOTPLUG_CPU /* Arch dependent functions for cpu hotplug support */ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu);
Use invalid_logical_cpuid(pr->id) instead of direct comparison.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Stefano Stabellini stefano.stabellini@eu.citrix.com --- drivers/xen/xen-acpi-cpuhotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xen-acpi-cpuhotplug.c b/drivers/xen/xen-acpi-cpuhotplug.c index 3e62ee4..5a62aa0 100644 --- a/drivers/xen/xen-acpi-cpuhotplug.c +++ b/drivers/xen/xen-acpi-cpuhotplug.c @@ -77,7 +77,7 @@ static int xen_acpi_processor_enable(struct acpi_device *device)
pr->id = xen_pcpu_id(pr->acpi_id);
- if ((int)pr->id < 0) + if (invalid_logical_cpuid(pr->id)) /* This cpu is not presented at hypervisor, try to hotadd it */ if (ACPI_FAILURE(xen_acpi_cpu_hotadd(pr))) { pr_err(PREFIX "Hotadd CPU (acpi_id = %d) failed.\n", @@ -226,7 +226,7 @@ static acpi_status xen_acpi_cpu_hotadd(struct acpi_processor *pr) return AE_ERROR;
pr->id = xen_hotadd_cpu(pr); - if ((int)pr->id < 0) + if (invalid_logical_cpuid(pr->id)) return AE_ERROR;
/*
On Mon, May 11, 2015 at 12:17:14PM +0800, Hanjun Guo wrote:
Use invalid_logical_cpuid(pr->id) instead of direct comparison.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Stefano Stabellini stefano.stabellini@eu.citrix.com
Reviewed-by: Konrad Rzeszutek Wilk konrad.wilk@oracle.com
drivers/xen/xen-acpi-cpuhotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xen-acpi-cpuhotplug.c b/drivers/xen/xen-acpi-cpuhotplug.c index 3e62ee4..5a62aa0 100644 --- a/drivers/xen/xen-acpi-cpuhotplug.c +++ b/drivers/xen/xen-acpi-cpuhotplug.c @@ -77,7 +77,7 @@ static int xen_acpi_processor_enable(struct acpi_device *device) pr->id = xen_pcpu_id(pr->acpi_id);
- if ((int)pr->id < 0)
- if (invalid_logical_cpuid(pr->id)) /* This cpu is not presented at hypervisor, try to hotadd it */ if (ACPI_FAILURE(xen_acpi_cpu_hotadd(pr))) { pr_err(PREFIX "Hotadd CPU (acpi_id = %d) failed.\n",
@@ -226,7 +226,7 @@ static acpi_status xen_acpi_cpu_hotadd(struct acpi_processor *pr) return AE_ERROR; pr->id = xen_hotadd_cpu(pr);
- if ((int)pr->id < 0)
- if (invalid_logical_cpuid(pr->id)) return AE_ERROR;
/* -- 1.9.1
Before xen_acpi_processor_enable() is called, struct acpi_processor *pr is allocated in xen_acpi_processor_add() and checked if it's NULL, so no need to check again when passed to xen_acpi_processor_enable(), just remove it.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Stefano Stabellini stefano.stabellini@eu.citrix.com --- drivers/xen/xen-acpi-cpuhotplug.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/xen/xen-acpi-cpuhotplug.c b/drivers/xen/xen-acpi-cpuhotplug.c index 5a62aa0..f4a3694 100644 --- a/drivers/xen/xen-acpi-cpuhotplug.c +++ b/drivers/xen/xen-acpi-cpuhotplug.c @@ -46,13 +46,7 @@ static int xen_acpi_processor_enable(struct acpi_device *device) unsigned long long value; union acpi_object object = { 0 }; struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; - struct acpi_processor *pr; - - pr = acpi_driver_data(device); - if (!pr) { - pr_err(PREFIX "Cannot find driver data\n"); - return -EINVAL; - } + struct acpi_processor *pr = acpi_driver_data(device);
if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) { /* Declared with "Processor" statement; match ProcessorID */
On Mon, May 11, 2015 at 12:17:15PM +0800, Hanjun Guo wrote:
Before xen_acpi_processor_enable() is called, struct acpi_processor *pr is allocated in xen_acpi_processor_add() and checked if it's NULL, so no need to check again when passed to xen_acpi_processor_enable(), just remove it.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Stefano Stabellini stefano.stabellini@eu.citrix.com
Reviewed-by: Konrad Rzeszutek Wilk konrad.wilk@oracle.com
drivers/xen/xen-acpi-cpuhotplug.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/xen/xen-acpi-cpuhotplug.c b/drivers/xen/xen-acpi-cpuhotplug.c index 5a62aa0..f4a3694 100644 --- a/drivers/xen/xen-acpi-cpuhotplug.c +++ b/drivers/xen/xen-acpi-cpuhotplug.c @@ -46,13 +46,7 @@ static int xen_acpi_processor_enable(struct acpi_device *device) unsigned long long value; union acpi_object object = { 0 }; struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
- struct acpi_processor *pr;
- pr = acpi_driver_data(device);
- if (!pr) {
pr_err(PREFIX "Cannot find driver data\n");return -EINVAL;- }
- struct acpi_processor *pr = acpi_driver_data(device);
if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) { /* Declared with "Processor" statement; match ProcessorID */ -- 1.9.1
Just use pr->id instead of cpu_index to simplify the code.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- drivers/acpi/acpi_processor.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index ac6bda0..0676b50 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -216,7 +216,7 @@ static int acpi_processor_get_info(struct acpi_device *device) struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; struct acpi_processor *pr = acpi_driver_data(device); phys_cpuid_t phys_id; - int cpu_index, device_declaration = 0; + int device_declaration = 0; acpi_status status = AE_OK; static int cpu0_initialized; unsigned long long value; @@ -268,18 +268,16 @@ static int acpi_processor_get_info(struct acpi_device *device) acpi_handle_debug(pr->handle, "failed to get CPU physical ID.\n"); pr->phys_id = phys_id;
- cpu_index = acpi_map_cpuid(pr->phys_id, pr->acpi_id); + pr->id = acpi_map_cpuid(pr->phys_id, pr->acpi_id); if (!cpu0_initialized && !acpi_has_cpu_in_madt()) { cpu0_initialized = 1; /* * Handle UP system running SMP kernel, with no CPU * entry in MADT */ - if (invalid_logical_cpuid(cpu_index) - && (num_online_cpus() == 1)) - cpu_index = 0; + if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1)) + pr->id = 0; } - pr->id = cpu_index;
/* * Extra Processor objects may be enumerated on MP systems with
Use pr->phys_id to replace phys_id to simplify the code.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- drivers/acpi/acpi_processor.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 0676b50..62c846b 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -215,7 +215,6 @@ 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); - phys_cpuid_t phys_id; int device_declaration = 0; acpi_status status = AE_OK; static int cpu0_initialized; @@ -263,10 +262,10 @@ static int acpi_processor_get_info(struct acpi_device *device) pr->acpi_id = value; }
- phys_id = acpi_get_phys_id(pr->handle, device_declaration, pr->acpi_id); - if (phys_id == PHYS_CPUID_INVALID) + pr->phys_id = acpi_get_phys_id(pr->handle, device_declaration, + pr->acpi_id); + if (pr->phys_id == PHYS_CPUID_INVALID) acpi_handle_debug(pr->handle, "failed to get CPU physical ID.\n"); - pr->phys_id = phys_id;
pr->id = acpi_map_cpuid(pr->phys_id, pr->acpi_id); if (!cpu0_initialized && !acpi_has_cpu_in_madt()) {
Since invalid_logical_cpuid() can check error values, so return specific error instead of -1 for acpi_map_cpuid().
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- drivers/acpi/processor_core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index b1ec78b..fd4140d 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -215,12 +215,12 @@ int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id) * Ignores phys_id and always returns 0 for the processor * handle with acpi id 0 if nr_cpu_ids is 1. * This should be the case if SMP tables are not found. - * Return -1 for other CPU's handle. + * Return -EINVAL for other CPU's handle. */ if (nr_cpu_ids <= 1 && acpi_id == 0) return acpi_id; else - return -1; + return -EINVAL; }
#ifdef CONFIG_SMP @@ -233,7 +233,7 @@ int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id) if (phys_id == 0) return phys_id; #endif - return -1; + return -ENODEV; }
int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id)
Introduce invalid_phys_cpuid() to identify cpu with invalid physical ID, then used it as replacement of the direct comparisons with PHYS_CPUID_INVALID.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- drivers/acpi/acpi_processor.c | 4 ++-- drivers/acpi/processor_core.c | 4 ++-- include/linux/acpi.h | 5 +++++ 3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 62c846b..92a5f73 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 == PHYS_CPUID_INVALID) + if (invalid_phys_cpuid(pr->phys_id)) return -ENODEV;
status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); @@ -264,7 +264,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
pr->phys_id = acpi_get_phys_id(pr->handle, device_declaration, pr->acpi_id); - if (pr->phys_id == PHYS_CPUID_INVALID) + if (invalid_phys_cpuid(pr->phys_id)) acpi_handle_debug(pr->handle, "failed to get CPU physical ID.\n");
pr->id = acpi_map_cpuid(pr->phys_id, pr->acpi_id); diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index fd4140d..33a38d6 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -184,7 +184,7 @@ phys_cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) phys_cpuid_t phys_id;
phys_id = map_mat_entry(handle, type, acpi_id); - if (phys_id == PHYS_CPUID_INVALID) + if (invalid_phys_cpuid(phys_id)) phys_id = map_madt_entry(type, acpi_id);
return phys_id; @@ -196,7 +196,7 @@ int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id) int i; #endif
- if (phys_id == PHYS_CPUID_INVALID) { + if (invalid_phys_cpuid(phys_id)) { /* * On UP processor, there is no _MAT or MADT table. * So above phys_id is always set to PHYS_CPUID_INVALID. diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 913b49f..cc82ff3 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -163,6 +163,11 @@ static inline bool invalid_logical_cpuid(u32 cpuid) return (int)cpuid < 0; }
+static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id) +{ + return (int)phys_id < 0; +} + #ifdef CONFIG_ACPI_HOTPLUG_CPU /* Arch dependent functions for cpu hotplug support */ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu);
-
Hanjun Guo -
Konrad Rzeszutek Wilk