[PATCH v2 0/7] minor cleanups for ACPI processor driver

[PATCH v2 0/7] minor cleanups for ACPI processor driver

[Hanjun Guo]

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

[PATCH v2 1/7] ACPI / processor: remove cpu_index in acpi_processor_get_info()

[Hanjun Guo]

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
-- 
1.9.1

Re: [PATCH v2 1/7] ACPI / processor: remove cpu_index in acpi_processor_get_info()

[Sudeep Holla]

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 <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
>

[PATCH v2 2/7] ACPI / processor: remove phys_id in acpi_processor_get_info()

[Hanjun Guo]

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()) {
-- 
1.9.1

[PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Hanjun Guo]

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 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

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Sudeep Holla]

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

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Rafael J. Wysocki]

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.

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Hanjun Guo]

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

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Rafael J. Wysocki]

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 <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;

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.

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Hanjun Guo]

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 <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;
>
> What about
>
> 	return !invalid_logical_cpuid(cpuid);

yes, cleaner, will update my patch.

Thanks
Hanjun

Re: [PATCH v2 3/7] ACPI / processor: Introduce invalid_logical_cpuid()

[Boris Ostrovsky]

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.

[PATCH v2 4/7] Xen / ACPI / processor: use invalid_logical_cpuid()

[Hanjun Guo]

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;
 
 	/*
-- 
1.9.1

[PATCH v2 5/7] Xen / ACPI / processor: Remove unneeded NULL check in xen_acpi_processor_enable()

[Hanjun Guo]

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 */
-- 
1.9.1

Re: [PATCH v2 5/7] Xen / ACPI / processor: Remove unneeded NULL check in xen_acpi_processor_enable()

[Stefano Stabellini]

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 <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 */
> -- 
> 1.9.1
> 

Re: [PATCH v2 5/7] Xen / ACPI / processor: Remove unneeded NULL check in xen_acpi_processor_enable()

[Konrad Rzeszutek Wilk]

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 <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 */
> > -- 
> > 1.9.1
> > 

[PATCH v2 6/7] ACPI / processor: return specific error instead of -1

[Hanjun Guo]

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)
-- 
1.9.1

[PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Hanjun Guo]

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);
-- 
1.9.1

Re: [PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Sudeep Holla]

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 <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

[...]

> 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

Re: [PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Hanjun Guo]

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 <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
>
> [...]
>
>> 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

Re: [PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Lorenzo Pieralisi]

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 <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
> >
> > [...]
> >
> >> 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

Re: [PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Hanjun Guo]

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 <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
>>>
>>> [...]
>>>
>>>> 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

Re: [PATCH v2 7/7] ACPI / processor: Introduce invalid_phys_cpuid()

[Hanjun Guo]

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 <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
>
> [...]
>
>> 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