This patch set are some minor cleanups for ACPI processor driver
to address the comments which raised by Rafael in ARM64 ACPI core
patches. I rebased this patch set on top of current Linus's tree.
v2:
- rebased on top of 4.1-rc2
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(-)
--
1.9.1
Just use pr->id instead of cpu_index to simplify the code.
Signed-off-by: Hanjun Guo <[email protected]>
---
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
--
1.9.1
Use pr->phys_id to replace phys_id to simplify the code.
Signed-off-by: Hanjun Guo <[email protected]>
---
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()) {
--
1.9.1
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 <[email protected]>
---
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 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);
--
1.9.1
Use invalid_logical_cpuid(pr->id) instead of direct comparison
of -1.
Signed-off-by: Hanjun Guo <[email protected]>
CC: Boris Ostrovsky <[email protected]>
CC: Stefano Stabellini <[email protected]>
---
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 <[email protected]>
CC: Boris Ostrovsky <[email protected]>
CC: Stefano Stabellini <[email protected]>
---
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
Since invalid_logical_cpuid() can check error values, so
return specific error instead of -1 for acpi_map_cpuid().
Signed-off-by: Hanjun Guo <[email protected]>
---
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)
--
1.9.1
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 <[email protected]>
---
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);
--
1.9.1
CC'ing Konrad and David.
On Tue, 5 May 2015, 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 <[email protected]>
> CC: Boris Ostrovsky <[email protected]>
> CC: Stefano Stabellini <[email protected]>
> ---
> 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
>
On 05/05/15 03:46, Hanjun Guo wrote:
> Just use pr->id instead of cpu_index to simplify the code.
>
IIRC pr->id is u32, has it changed in any other patches that's not in
mainline ? Otherwise, comparing it with -1 makes no sense here.
Regards,
Sudeep
> Signed-off-by: Hanjun Guo <[email protected]>
> ---
> 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
>
On 05/05/15 03:46, Hanjun Guo wrote:
> 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.
>
Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think
you need to reorder this and 1/7 patch IMO.
Regards,
Sudeep
On 05/05/15 03:46, Hanjun Guo wrote:
> 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 <[email protected]>
> ---
> 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
[...]
> 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;
Should this be phys_id == PHYS_CPUID_INVALID ? else I don't see why we
need to even define PHYS_CPUID_INVALID
Regards,
Sudeep
On Tuesday, May 05, 2015 10:46:34 AM Hanjun Guo wrote:
> 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 <[email protected]>
> ---
> 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;
What about
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);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Tuesday, May 05, 2015 12:15:13 PM Sudeep Holla wrote:
>
> On 05/05/15 03:46, Hanjun Guo wrote:
> > 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.
> >
>
> Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think
> you need to reorder this and 1/7 patch IMO.
Well, comparing an unsigned int with -1 is not technically invalid (although it
involves an implicit type conversion), but yes, Hanjun, please reorder the
patches.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 2015年05月05日 19:25, Sudeep Holla wrote:
>
>
> On 05/05/15 03:46, Hanjun Guo wrote:
>> 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 <[email protected]>
>> ---
>> 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
>
> [...]
>
>> 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;
>
> Should this be phys_id == PHYS_CPUID_INVALID ? else I don't see why we
> need to even define PHYS_CPUID_INVALID
I'm OK with this. For now, CPU phys_id will be valid value or
PHYS_CPUID_INVALID in all cases for ACPI processor driver, but
I want ask Rafael's opinion on this, is it OK to you too, Rafael?
Thanks
Hanjun
On 2015年05月05日 20:01, Rafael J. Wysocki wrote:
> On Tuesday, May 05, 2015 10:46:34 AM Hanjun Guo wrote:
>> 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 <[email protected]>
>> ---
>> 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;
>
> What about
>
> return !invalid_logical_cpuid(cpuid);
yes, cleaner, will update my patch.
Thanks
Hanjun
On 2015年05月05日 20:04, Rafael J. Wysocki wrote:
> On Tuesday, May 05, 2015 12:15:13 PM Sudeep Holla wrote:
>>
>> On 05/05/15 03:46, Hanjun Guo wrote:
>>> 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.
>>>
>>
>> Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think
>> you need to reorder this and 1/7 patch IMO.
>
> Well, comparing an unsigned int with -1 is not technically invalid (although it
> involves an implicit type conversion), but yes, Hanjun, please reorder the
> patches.
Sure, I will.
Thanks
Hanjun
On 05/04/2015 10:46 PM, Hanjun Guo wrote:
> 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.
Which is exactly what Xen code does (xen_pcpu_id() and
xen_hotadd_cpu()). And patch 4/7 fixes this.
-boris
>
> 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.
On 2015年05月05日 19:25, Sudeep Holla wrote:
>
>
> On 05/05/15 03:46, Hanjun Guo wrote:
>> 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 <[email protected]>
>> ---
>> 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
>
> [...]
>
>> 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;
>
> Should this be phys_id == PHYS_CPUID_INVALID ? else I don't see why we
> need to even define PHYS_CPUID_INVALID
Oh, we need PHYS_CPUID_INVALID because we defined
+#ifndef PHYS_CPUID_INVALID
+typedef u32 phys_cpuid_t;
+#define PHYS_CPUID_INVALID (phys_cpuid_t)(-1)
+#endif
in the common head file linux/acpi.h, as it is needed
for phys_cpuid_t for ARM64. this is already discussed
here:
https://lkml.org/lkml/2015/3/30/311
Thanks
Hanjun
On Tue, May 05, 2015 at 11:29:05AM +0100, Stefano Stabellini wrote:
> CC'ing Konrad and David.
>
> On Tue, 5 May 2015, 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.
Sounds right.
> >
> > Signed-off-by: Hanjun Guo <[email protected]>
> > CC: Boris Ostrovsky <[email protected]>
> > CC: Stefano Stabellini <[email protected]>
> > ---
> > 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
> >
On Tue, May 05, 2015 at 02:14:01PM +0100, Hanjun Guo wrote:
> On 2015???05???05??? 19:25, Sudeep Holla wrote:
> >
> >
> > On 05/05/15 03:46, Hanjun Guo wrote:
> >> 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 <[email protected]>
> >> ---
> >> 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
> >
> > [...]
> >
> >> 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;
> >
> > Should this be phys_id == PHYS_CPUID_INVALID ? else I don't see why we
> > need to even define PHYS_CPUID_INVALID
>
> I'm OK with this. For now, CPU phys_id will be valid value or
> PHYS_CPUID_INVALID in all cases for ACPI processor driver, but
> I want ask Rafael's opinion on this, is it OK to you too, Rafael?
Is your worry related to functions returning error values
other than PHYS_CPUID_INVALID (when they are expected to return a
physical id) ? Is there any in the current kernel ?
static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
{
return phys_id == PHYS_CPUID_INVALID;
}
This should do, and if we need more mapping functions that are supposed
to return physical ids they should return PHYS_CPUID_INVALID on failure.
Lorenzo
On 2015年05月12日 00:35, Lorenzo Pieralisi wrote:
> On Tue, May 05, 2015 at 02:14:01PM +0100, Hanjun Guo wrote:
>> On 2015???05???05??? 19:25, Sudeep Holla wrote:
>>>
>>>
>>> On 05/05/15 03:46, Hanjun Guo wrote:
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
>>>
>>> [...]
>>>
>>>> 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;
>>>
>>> Should this be phys_id == PHYS_CPUID_INVALID ? else I don't see why we
>>> need to even define PHYS_CPUID_INVALID
>>
>> I'm OK with this. For now, CPU phys_id will be valid value or
>> PHYS_CPUID_INVALID in all cases for ACPI processor driver, but
>> I want ask Rafael's opinion on this, is it OK to you too, Rafael?
>
> Is your worry related to functions returning error values
> other than PHYS_CPUID_INVALID (when they are expected to return a
> physical id) ?
Yes.
> Is there any in the current kernel ?
No such returns as far as I know.
>
> static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
> {
> return phys_id == PHYS_CPUID_INVALID;
> }
>
> This should do, and if we need more mapping functions that are supposed
> to return physical ids they should return PHYS_CPUID_INVALID on failure.
OK, I will send a update version for this patch only.
Thanks
Hanjun