This patch set are some minor cleanups for ACPI processor driver to address the comments which raised by Rafael in ARM64 ACPI core patches, so this patch set is on top of ARM64 ACPI core patches the git tree is
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git branch for-next/acpi.
Rafael, I assume this patchset will be taken with your tree if it makes sense, any rebase work needed please let me know.
the last patch - ACPI / processor: Introduce invalid_phys_cpuid() will cut u64 mpidr to int, but I think it is ok for error values, correct me if I'm wrong.
Comments are welcomed.
Hanjun Guo (7): ACPI / processor: remove cpu_index in acpi_processor_get_info() ACPI / processor: remove phys_id in acpi_processor_get_info() ACPI / processor: Introduce invalid_logical_cpuid() Xen / ACPI / processor: use invalid_logical_cpuid() Xen / ACPI / processor: Remove unneeded NULL check in xen_acpi_processor_enable() 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(-)
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 | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 58f335c..d6ac918 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,17 +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 ((cpu_index == -1) && (num_online_cpus() == 1)) - cpu_index = 0; + if ((pr->id == -1) && (num_online_cpus() == 1)) + pr->id = 0; } - pr->id = cpu_index;
/* * Extra Processor objects may be enumerated on MP systems with
MADT table scannig will stopped once it gets the errors returned by the handler, which is acpi_map_gic_cpu_interface() in for ARM64, so Ignore the return error value to search for all enabled CPUs for SMP init.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- arch/arm64/kernel/acpi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 07649e4..c263cba 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -181,7 +181,8 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, return -EINVAL;
acpi_table_print_madt_entry(header); - return acpi_map_gic_cpu_interface(processor); + acpi_map_gic_cpu_interface(processor); + return 0; }
/* Parse GIC cpu interface entries in MADT for SMP init */
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 d6ac918..95492d0 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 the only caller of acpi_parse_gic_cpu_interface() doesn't need the return value, make it have a void return type to avoid introducing subtle bugs, and update the comments of the function accordingly.
Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- arch/arm64/kernel/acpi.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index c263cba..8b83955 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -98,12 +98,8 @@ void __init __acpi_unmap_table(char *map, unsigned long size) /** * acpi_map_gic_cpu_interface - generates a logical cpu number * and map to MPIDR represented by GICC structure - * @mpidr: CPU's hardware id to register, MPIDR represented in MADT - * @enabled: this cpu is enabled or not - * - * Returns the logical cpu number which maps to MPIDR */ -static int __init +static void __init acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) { int i; @@ -112,17 +108,17 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
if (mpidr == INVALID_HWID) { pr_info("Skip MADT cpu entry with invalid MPIDR\n"); - return -EINVAL; + return; }
total_cpus++; if (!enabled) - return -EINVAL; + return;
if (enabled_cpus >= NR_CPUS) { pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n", NR_CPUS, total_cpus, mpidr); - return -EINVAL; + return; }
/* Check if GICC structure of boot CPU is available in the MADT */ @@ -130,7 +126,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) if (bootcpu_valid) { pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", mpidr); - return -EINVAL; + return; }
bootcpu_valid = true; @@ -145,28 +141,27 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) if (cpu_logical_map(i) == mpidr) { pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", mpidr); - return -EINVAL; + return; } }
if (!acpi_psci_present()) - return -EOPNOTSUPP; + return;
cpu_ops[enabled_cpus] = cpu_get_ops("psci"); /* CPU 0 was already initialized */ if (enabled_cpus) { if (!cpu_ops[enabled_cpus]) - return -EINVAL; + return;
if (cpu_ops[enabled_cpus]->cpu_init(NULL, enabled_cpus)) - return -EOPNOTSUPP; + return;
/* map the logical cpu id to cpu MPIDR */ cpu_logical_map(enabled_cpus) = mpidr; }
enabled_cpus++; - return enabled_cpus; }
static int __init
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 | 4 ++-- drivers/acpi/processor_pdc.c | 5 +---- include/linux/acpi.h | 5 +++++ 3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 95492d0..62c846b 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -274,7 +274,7 @@ static int acpi_processor_get_info(struct acpi_device *device) * Handle UP system running SMP kernel, with no CPU * entry in MADT */ - if ((pr->id == -1) && (num_online_cpus() == 1)) + if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1)) pr->id = 0; }
@@ -283,7 +283,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..6dd98d0 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) ? false : true; }
static void acpi_set_pdc_bits(u32 *buf) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index de4e86f..fd8a2a4 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -152,6 +152,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 of -1.
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;
/*
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 */
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 fd8a2a4..9a1f467 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -157,6 +157,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);
Oops, some side effects for staying up all night...
Please ignore this patchset, will resend, sorry for the noise.
On 2015年03月27日 21:55, Hanjun Guo wrote:
This patch set are some minor cleanups for ACPI processor driver to address the comments which raised by Rafael in ARM64 ACPI core patches, so this patch set is on top of ARM64 ACPI core patches the git tree is
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git branch for-next/acpi.
Rafael, I assume this patchset will be taken with your tree if it makes sense, any rebase work needed please let me know.
the last patch - ACPI / processor: Introduce invalid_phys_cpuid() will cut u64 mpidr to int, but I think it is ok for error values, correct me if I'm wrong.
Comments are welcomed.
Hanjun Guo (7): ACPI / processor: remove cpu_index in acpi_processor_get_info() ACPI / processor: remove phys_id in acpi_processor_get_info() ACPI / processor: Introduce invalid_logical_cpuid() Xen / ACPI / processor: use invalid_logical_cpuid() Xen / ACPI / processor: Remove unneeded NULL check in xen_acpi_processor_enable() 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(-)
Hanjun,
On Fri, Mar 27, 2015 at 01:55:04PM +0000, Hanjun Guo wrote:
This patch set are some minor cleanups for ACPI processor driver to address the comments which raised by Rafael in ARM64 ACPI core patches, so this patch set is on top of ARM64 ACPI core patches the git tree is
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git branch for-next/acpi.
Rafael, I assume this patchset will be taken with your tree if it makes sense, any rebase work needed please let me know.
the last patch - ACPI / processor: Introduce invalid_phys_cpuid() will cut u64 mpidr to int, but I think it is ok for error values, correct me if I'm wrong.
I appreciate that you probably have a bunch of code building on top of the series that's in linux-next, but sending it out at the moment is hugely confusing -- particularly when this is targetting an independent tree. If you have patches for 4.2, please can you wait until after the merge window before posting for review?
Right now most maintainers are probably trying to stabilise their current queues, so the most helpful thing you can do is test linux-next and send fixes for any issues you find there.
It doesn't help that the series you posted seems to include two patch series (n/7 and n/2).
Will
Hi Will,
On 2015年03月27日 22:03, Will Deacon wrote:
Hanjun,
On Fri, Mar 27, 2015 at 01:55:04PM +0000, Hanjun Guo wrote:
This patch set are some minor cleanups for ACPI processor driver to address the comments which raised by Rafael in ARM64 ACPI core patches, so this patch set is on top of ARM64 ACPI core patches the git tree is
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git branch for-next/acpi.
Rafael, I assume this patchset will be taken with your tree if it makes sense, any rebase work needed please let me know.
the last patch - ACPI / processor: Introduce invalid_phys_cpuid() will cut u64 mpidr to int, but I think it is ok for error values, correct me if I'm wrong.
I appreciate that you probably have a bunch of code building on top of the series that's in linux-next, but sending it out at the moment is hugely confusing -- particularly when this is targetting an independent tree. If you have patches for 4.2, please can you wait until after the merge window before posting for review?
OK, thanks for the reminding, I didn't mean any confusion but actually I did, sorry for that.
Right now most maintainers are probably trying to stabilise their current queues, so the most helpful thing you can do is test linux-next and send fixes for any issues you find there.
Sure, I will test linux-next on ARM64 platforms and x86 too.
It doesn't help that the series you posted seems to include two patch series (n/7 and n/2).
oh, side effects of staying up all night for the firmware mini-summit, please take those two patches for ARM64 ACPI if it is OK to you.
Thanks Hanjun
On Fri, Mar 27, 2015 at 02:37:28PM +0000, Hanjun Guo wrote:
On 2015年03月27日 22:03, Will Deacon wrote:
Right now most maintainers are probably trying to stabilise their current queues, so the most helpful thing you can do is test linux-next and send fixes for any issues you find there.
Sure, I will test linux-next on ARM64 platforms and x86 too.
Thanks.
It doesn't help that the series you posted seems to include two patch series (n/7 and n/2).
oh, side effects of staying up all night for the firmware mini-summit, please take those two patches for ARM64 ACPI if it is OK to you.
I already pushed them out :)
Have a good weekend,
Will
-
Hanjun Guo -
Will Deacon