[PATCH 0/3] ACPI/processor: trivial fixes

[PATCH 0/3] ACPI/processor: trivial fixes

[Sudeep Holla]

Hi Rafael,

While trying to understand the existing ACPI cpuidle driver, I found
few trivial issues. I am not sure if they were left intentionally or
unnoticed as they might not trigger any issues.

Let me know if these makes sense.

Regards,
Sudeep

Sudeep Holla (3):
  ACPI / processor: remove incorrect comparison of phys_id
  ACPI / processor: remove unused variabled from acpi_processor_power
    structure
  ACPI / cpuidle: avoid assigning signed errno to acpi_status

 drivers/acpi/acpi_processor.c |  3 ---
 drivers/acpi/processor_idle.c | 14 +++++++-------
 include/acpi/processor.h      |  3 ---
 3 files changed, 7 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

phys_id in acpi_processor structure is unsigned 32-bit integer,
comparing it with signed value is incorrect.

This patch removes that incorrect comparision in acpi_processor_hotadd_init
as the lone user of this function is already checking for correctness
of phys_id before calling it.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/acpi_processor.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index c4a8a5666298..eaf56f6ce1eb 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -170,9 +170,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	acpi_status status;
 	int ret;
 
-	if (pr->phys_id == -1)
-		return -ENODEV;
-
 	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
 	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
 		return -ENODEV;
-- 
1.9.1

[PATCH 2/3] ACPI / processor: remove unused variabled from acpi_processor_power structure

[Sudeep Holla]

Few elements in the acpi_processor_power structure are unused. It could
be remnant in the header missed while the code got removed from the
corresponding driver file.

This patch removes those unused variables in the structure declaration.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/acpi/processor.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 8e34af9e1c84..b95dc32a6e6b 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -67,9 +67,6 @@ struct acpi_processor_cx {
 };
 
 struct acpi_processor_power {
-	struct acpi_processor_cx *state;
-	unsigned long bm_check_timestamp;
-	u32 default_state;
 	int count;
 	struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
 	int timer_broadcast_on_state;
-- 
1.9.1

[PATCH 3/3] ACPI / cpuidle: avoid assigning signed errno to acpi_status

[Sudeep Holla]

It's incorrect to assign a signed value to an unsigned acpi_status
variable. This patch fixes it by using a signed integer to return
error values.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/processor_idle.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 380b4b43f361..499536504698 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -334,10 +334,10 @@ static int acpi_processor_get_power_info_default(struct acpi_processor *pr)
 
 static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 {
-	acpi_status status = 0;
+	acpi_status status;
 	u64 count;
 	int current_count;
-	int i;
+	int i, ret = 0;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *cst;
 
@@ -358,7 +358,7 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 	/* There must be at least 2 elements */
 	if (!cst || (cst->type != ACPI_TYPE_PACKAGE) || cst->package.count < 2) {
 		printk(KERN_ERR PREFIX "not enough elements in _CST\n");
-		status = -EFAULT;
+		ret = -EFAULT;
 		goto end;
 	}
 
@@ -367,7 +367,7 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 	/* Validate number of power states. */
 	if (count < 1 || count != cst->package.count - 1) {
 		printk(KERN_ERR PREFIX "count given by _CST is not valid\n");
-		status = -EFAULT;
+		ret = -EFAULT;
 		goto end;
 	}
 
@@ -489,12 +489,12 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 
 	/* Validate number of power states discovered */
 	if (current_count < 2)
-		status = -EFAULT;
+		ret = -EFAULT;
 
       end:
 	kfree(buffer.pointer);
 
-	return status;
+	return ret;
 }
 
 static void acpi_processor_power_verify_c3(struct acpi_processor *pr,
@@ -1109,7 +1109,7 @@ static int acpi_processor_registered;
 
 int acpi_processor_power_init(struct acpi_processor *pr)
 {
-	acpi_status status = 0;
+	acpi_status status;
 	int retval;
 	struct cpuidle_device *dev;
 	static int first_run;
-- 
1.9.1

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Rafael J. Wysocki]

On Tuesday, November 25, 2014 02:48:48 PM Sudeep Holla wrote:
> phys_id in acpi_processor structure is unsigned 32-bit integer,
> comparing it with signed value is incorrect.
> 
> This patch removes that incorrect comparision in acpi_processor_hotadd_init
> as the lone user of this function is already checking for correctness
> of phys_id before calling it.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

This one depens on commits I've dropped due to conflicts with the tip tree.

They'll need to be rebased on top of current linux-next and resent.

I can take patches [2-3/3], though.

> ---
>  drivers/acpi/acpi_processor.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index c4a8a5666298..eaf56f6ce1eb 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -170,9 +170,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  	acpi_status status;
>  	int ret;
>  
> -	if (pr->phys_id == -1)
> -		return -ENODEV;
> -
>  	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
>  	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
>  		return -ENODEV;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Hanjun Guo]

Hi Sudeep,

On 2014-11-25 22:48, Sudeep Holla wrote:
> phys_id in acpi_processor structure is unsigned 32-bit integer,
> comparing it with signed value is incorrect.

Yes, this is a bug :)

But the phys_id in acpi_processor structure should be signed value because
acpi_get_phys_id() will return -1 if there if no CPU entry found in MADT
table.

I found the id in acpi_processor structure should also be signed value by
unsigned now, we should fix that too.

> 
> This patch removes that incorrect comparision in acpi_processor_hotadd_init
> as the lone user of this function is already checking for correctness
> of phys_id before calling it.

	if (apic_id < 0)
		acpi_handle_debug(pr->handle, "failed to get CPU APIC ID.\n");

it only check the value and print debug message but no returns, so I think
the check in the following patch is still needed.

> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/acpi/acpi_processor.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index c4a8a5666298..eaf56f6ce1eb 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -170,9 +170,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  	acpi_status status;
>  	int ret;
>  
> -	if (pr->phys_id == -1)
> -		return -ENODEV;

I prepared a patch below:

diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 8e34af9..cfdab24 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -199,8 +199,8 @@ struct acpi_processor_flags {
 struct acpi_processor {
        acpi_handle handle;
        u32 acpi_id;
-       u32 phys_id;    /* CPU hardware ID such as APIC ID for x86 */
-       u32 id;         /* CPU logical ID allocated by OS */
+       int phys_id;    /* CPU hardware ID such as APIC ID for x86 */
+       int id;         /* CPU logical ID allocated by OS */
        u32 pblk;
        int performance_platform_limit;
        int throttling_platform_limit;

what do you think?

Thanks
Hanjun

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

Hi Hanjun,

On 26/11/14 09:53, Hanjun Guo wrote:
> Hi Sudeep,
>
> On 2014-11-25 22:48, Sudeep Holla wrote:
>> phys_id in acpi_processor structure is unsigned 32-bit integer,
>> comparing it with signed value is incorrect.
>
> Yes, this is a bug :)
>
> But the phys_id in acpi_processor structure should be signed value
> because acpi_get_phys_id() will return -1 if there if no CPU entry
> found in MADT table.
>
> I found the id in acpi_processor structure should also be signed
> value by unsigned now, we should fix that too.
>

I tend to disagree, since the physical(h/w) and logical identifiers are
unsigned integers, the structure elements must also be the same. Though
for checking the correctness of those identifiers, the functions can
manage through signed values or any other alternates IMO(i.e. more
implementation details)

Let's see what's Rafael's opinion on this.

>>
>> This patch removes that incorrect comparision in
>> acpi_processor_hotadd_init as the lone user of this function is
>> already checking for correctness of phys_id before calling it.
>
> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
> APIC ID.\n");
>
> it only check the value and print debug message but no returns, so I
> think the check in the following patch is still needed.
>

Agreed, but that's something we need to fix in the logic and not by
changing these identifiers to signed values in the structures.

Regards,
Sudeep

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

Hi Rafael,

On 25/11/14 23:11, Rafael J. Wysocki wrote:
> On Tuesday, November 25, 2014 02:48:48 PM Sudeep Holla wrote:
>> phys_id in acpi_processor structure is unsigned 32-bit integer,
>> comparing it with signed value is incorrect.
>>
>> This patch removes that incorrect comparision in
>> acpi_processor_hotadd_init as the lone user of this function is
>> already checking for correctness of phys_id before calling it.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> This one depens on commits I've dropped due to conflicts with the tip
> tree.
>
> They'll need to be rebased on top of current linux-next and resent.
>

I have rebased but will hold off until your response to Hanjun's concern.

> I can take patches [2-3/3], though.

Thanks.

Regards,
Sudeep

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Rafael J. Wysocki]

On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
> Hi Hanjun,
> 
> On 26/11/14 09:53, Hanjun Guo wrote:
> > Hi Sudeep,
> >
> > On 2014-11-25 22:48, Sudeep Holla wrote:
> >> phys_id in acpi_processor structure is unsigned 32-bit integer,
> >> comparing it with signed value is incorrect.
> >
> > Yes, this is a bug :)
> >
> > But the phys_id in acpi_processor structure should be signed value
> > because acpi_get_phys_id() will return -1 if there if no CPU entry
> > found in MADT table.
> >
> > I found the id in acpi_processor structure should also be signed
> > value by unsigned now, we should fix that too.
> >
> 
> I tend to disagree, since the physical(h/w) and logical identifiers are
> unsigned integers, the structure elements must also be the same. Though
> for checking the correctness of those identifiers, the functions can
> manage through signed values or any other alternates IMO(i.e. more
> implementation details)
> 
> Let's see what's Rafael's opinion on this.
> 
> >>
> >> This patch removes that incorrect comparision in
> >> acpi_processor_hotadd_init as the lone user of this function is
> >> already checking for correctness of phys_id before calling it.
> >
> > if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
> > APIC ID.\n");
> >
> > it only check the value and print debug message but no returns, so I
> > think the check in the following patch is still needed.
> >
> 
> Agreed, but that's something we need to fix in the logic and not by
> changing these identifiers to signed values in the structures.

I'd rather not change data structures just because of what one funtion returns.

Instead, I'd do something like

	#define CPU_PHYS_ID_INVALID	(u32)(-1)

change the function in question to return CPU_PHYS_ID_INVALID instead of -1
and change the check to

	if (phys_id == CPU_PHYS_ID_INVALID)
		...


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Hanjun Guo]

On 2014-11-27 6:26, Rafael J. Wysocki wrote:
> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>> Hi Hanjun,
>>
>> On 26/11/14 09:53, Hanjun Guo wrote:
>>> Hi Sudeep,
>>>
>>> On 2014-11-25 22:48, Sudeep Holla wrote:
>>>> phys_id in acpi_processor structure is unsigned 32-bit integer,
>>>> comparing it with signed value is incorrect.
>>>
>>> Yes, this is a bug :)
>>>
>>> But the phys_id in acpi_processor structure should be signed value
>>> because acpi_get_phys_id() will return -1 if there if no CPU entry
>>> found in MADT table.
>>>
>>> I found the id in acpi_processor structure should also be signed
>>> value by unsigned now, we should fix that too.
>>>
>>
>> I tend to disagree, since the physical(h/w) and logical identifiers are
>> unsigned integers, the structure elements must also be the same. Though
>> for checking the correctness of those identifiers, the functions can
>> manage through signed values or any other alternates IMO(i.e. more
>> implementation details)
>>
>> Let's see what's Rafael's opinion on this.
>>
>>>>
>>>> This patch removes that incorrect comparision in
>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>> already checking for correctness of phys_id before calling it.
>>>
>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>> APIC ID.\n");
>>>
>>> it only check the value and print debug message but no returns, so I
>>> think the check in the following patch is still needed.
>>>
>>
>> Agreed, but that's something we need to fix in the logic and not by
>> changing these identifiers to signed values in the structures.
> 
> I'd rather not change data structures just because of what one funtion returns.
> 
> Instead, I'd do something like
> 
> 	#define CPU_PHYS_ID_INVALID	(u32)(-1)
> 
> change the function in question to return CPU_PHYS_ID_INVALID instead of -1
> and change the check to
> 
> 	if (phys_id == CPU_PHYS_ID_INVALID)

I'm fine with this :)

Thanks
Hanjun

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

Hi Rafael,

On 26/11/14 22:26, Rafael J. Wysocki wrote:
> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>> Hi Hanjun,
>>
>> On 26/11/14 09:53, Hanjun Guo wrote:
>>> Hi Sudeep,
>>>
>>> On 2014-11-25 22:48, Sudeep Holla wrote:

[...]

>>>>
>>>> This patch removes that incorrect comparision in
>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>> already checking for correctness of phys_id before calling it.
>>>
>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>> APIC ID.\n");
>>>
>>> it only check the value and print debug message but no returns, so I
>>> think the check in the following patch is still needed.
>>>
>>
>> Agreed, but that's something we need to fix in the logic and not by
>> changing these identifiers to signed values in the structures.
>
> I'd rather not change data structures just because of what one funtion returns.
>
> Instead, I'd do something like
>
> 	#define CPU_PHYS_ID_INVALID	(u32)(-1)
>
> change the function in question to return CPU_PHYS_ID_INVALID instead of -1
> and change the check to
>
> 	if (phys_id == CPU_PHYS_ID_INVALID)
> 		...
>

Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
to phys_id ? Since the variable is getting renamed it will conflict.

Regards,
Sudeep

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Rafael J. Wysocki]

On Friday, November 28, 2014 07:05:09 PM Sudeep Holla wrote:
> Hi Rafael,
> 
> On 26/11/14 22:26, Rafael J. Wysocki wrote:
> > On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
> >> Hi Hanjun,
> >>
> >> On 26/11/14 09:53, Hanjun Guo wrote:
> >>> Hi Sudeep,
> >>>
> >>> On 2014-11-25 22:48, Sudeep Holla wrote:
> 
> [...]
> 
> >>>>
> >>>> This patch removes that incorrect comparision in
> >>>> acpi_processor_hotadd_init as the lone user of this function is
> >>>> already checking for correctness of phys_id before calling it.
> >>>
> >>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
> >>> APIC ID.\n");
> >>>
> >>> it only check the value and print debug message but no returns, so I
> >>> think the check in the following patch is still needed.
> >>>
> >>
> >> Agreed, but that's something we need to fix in the logic and not by
> >> changing these identifiers to signed values in the structures.
> >
> > I'd rather not change data structures just because of what one funtion returns.
> >
> > Instead, I'd do something like
> >
> > 	#define CPU_PHYS_ID_INVALID	(u32)(-1)
> >
> > change the function in question to return CPU_PHYS_ID_INVALID instead of -1
> > and change the check to
> >
> > 	if (phys_id == CPU_PHYS_ID_INVALID)
> > 		...
> >
> 
> Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
> to phys_id ? Since the variable is getting renamed it will conflict.

Yes, it's better to rebase IMO.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Hanjun Guo]

On 2014年11月29日 07:26, Rafael J. Wysocki wrote:
> On Friday, November 28, 2014 07:05:09 PM Sudeep Holla wrote:
>> Hi Rafael,
>>
>> On 26/11/14 22:26, Rafael J. Wysocki wrote:
>>> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>>>> Hi Hanjun,
>>>>
>>>> On 26/11/14 09:53, Hanjun Guo wrote:
>>>>> Hi Sudeep,
>>>>>
>>>>> On 2014-11-25 22:48, Sudeep Holla wrote:
>>
>> [...]
>>
>>>>>>
>>>>>> This patch removes that incorrect comparision in
>>>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>>>> already checking for correctness of phys_id before calling it.
>>>>>
>>>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>>>> APIC ID.\n");
>>>>>
>>>>> it only check the value and print debug message but no returns, so I
>>>>> think the check in the following patch is still needed.
>>>>>
>>>>
>>>> Agreed, but that's something we need to fix in the logic and not by
>>>> changing these identifiers to signed values in the structures.
>>>
>>> I'd rather not change data structures just because of what one funtion returns.
>>>
>>> Instead, I'd do something like
>>>
>>> 	#define CPU_PHYS_ID_INVALID	(u32)(-1)
>>>
>>> change the function in question to return CPU_PHYS_ID_INVALID instead of -1
>>> and change the check to
>>>
>>> 	if (phys_id == CPU_PHYS_ID_INVALID)
>>> 		...
>>>
>>
>> Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
>> to phys_id ? Since the variable is getting renamed it will conflict.
>
> Yes, it's better to rebase IMO.

Hi Sudeep, any updates for this patch? I'm working on introducing
typedef u32 phys_cpuid_t for x86 and ia64 for phys_id, and need
this CPU_PHYS_ID_INVALID macro, if you are not working on that, I
will restart the work to address the comments from Lorenzo and
Catalin [1].

[1]: https://lkml.org/lkml/2015/2/3/635

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

On 13/02/15 07:56, Hanjun Guo wrote:
> On 2014年11月29日 07:26, Rafael J. Wysocki wrote:
>> On Friday, November 28, 2014 07:05:09 PM Sudeep Holla wrote:
>>> Hi Rafael,
>>>
>>> On 26/11/14 22:26, Rafael J. Wysocki wrote:
>>>> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>>>>> Hi Hanjun,
>>>>>
>>>>> On 26/11/14 09:53, Hanjun Guo wrote:
>>>>>> Hi Sudeep,
>>>>>>
>>>>>> On 2014-11-25 22:48, Sudeep Holla wrote:
>>>
>>> [...]
>>>
>>>>>>>
>>>>>>> This patch removes that incorrect comparision in
>>>>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>>>>> already checking for correctness of phys_id before calling it.
>>>>>>
>>>>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>>>>> APIC ID.\n");
>>>>>>
>>>>>> it only check the value and print debug message but no returns, so I
>>>>>> think the check in the following patch is still needed.
>>>>>>
>>>>>
>>>>> Agreed, but that's something we need to fix in the logic and not by
>>>>> changing these identifiers to signed values in the structures.
>>>>
>>>> I'd rather not change data structures just because of what one funtion returns.
>>>>
>>>> Instead, I'd do something like
>>>>
>>>> 	#define CPU_PHYS_ID_INVALID	(u32)(-1)
>>>>
>>>> change the function in question to return CPU_PHYS_ID_INVALID instead of -1
>>>> and change the check to
>>>>
>>>> 	if (phys_id == CPU_PHYS_ID_INVALID)
>>>> 		...
>>>>
>>>
>>> Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
>>> to phys_id ? Since the variable is getting renamed it will conflict.
>>
>> Yes, it's better to rebase IMO.
>
> Hi Sudeep, any updates for this patch? I'm working on introducing
> typedef u32 phys_cpuid_t for x86 and ia64 for phys_id, and need
> this CPU_PHYS_ID_INVALID macro, if you are not working on that, I
> will restart the work to address the comments from Lorenzo and
> Catalin [1].
>

IMO, it would be good if you work on that instead, so that there's no
dependency created as it's needed for your ARM64 port anyway.

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Hanjun Guo]

On 2015年02月16日 18:08, Sudeep Holla wrote:
>
>
> On 13/02/15 07:56, Hanjun Guo wrote:
>> On 2014年11月29日 07:26, Rafael J. Wysocki wrote:
>>> On Friday, November 28, 2014 07:05:09 PM Sudeep Holla wrote:
>>>> Hi Rafael,
>>>>
>>>> On 26/11/14 22:26, Rafael J. Wysocki wrote:
>>>>> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>>>>>> Hi Hanjun,
>>>>>>
>>>>>> On 26/11/14 09:53, Hanjun Guo wrote:
>>>>>>> Hi Sudeep,
>>>>>>>
>>>>>>> On 2014-11-25 22:48, Sudeep Holla wrote:
>>>>
>>>> [...]
>>>>
>>>>>>>>
>>>>>>>> This patch removes that incorrect comparision in
>>>>>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>>>>>> already checking for correctness of phys_id before calling it.
>>>>>>>
>>>>>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>>>>>> APIC ID.\n");
>>>>>>>
>>>>>>> it only check the value and print debug message but no returns, so I
>>>>>>> think the check in the following patch is still needed.
>>>>>>>
>>>>>>
>>>>>> Agreed, but that's something we need to fix in the logic and not by
>>>>>> changing these identifiers to signed values in the structures.
>>>>>
>>>>> I'd rather not change data structures just because of what one
>>>>> funtion returns.
>>>>>
>>>>> Instead, I'd do something like
>>>>>
>>>>>     #define CPU_PHYS_ID_INVALID    (u32)(-1)
>>>>>
>>>>> change the function in question to return CPU_PHYS_ID_INVALID
>>>>> instead of -1
>>>>> and change the check to
>>>>>
>>>>>     if (phys_id == CPU_PHYS_ID_INVALID)
>>>>>         ...
>>>>>
>>>>
>>>> Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
>>>> to phys_id ? Since the variable is getting renamed it will conflict.
>>>
>>> Yes, it's better to rebase IMO.
>>
>> Hi Sudeep, any updates for this patch? I'm working on introducing
>> typedef u32 phys_cpuid_t for x86 and ia64 for phys_id, and need
>> this CPU_PHYS_ID_INVALID macro, if you are not working on that, I
>> will restart the work to address the comments from Lorenzo and
>> Catalin [1].
>>
>
> IMO, it would be good if you work on that instead, so that there's no
> dependency created as it's needed for your ARM64 port anyway.

I think so, I just want to confirm that it will be no duplicate work
for us. I will prepare a patch.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ACPI / processor: remove incorrect comparison of phys_id

[Sudeep Holla]

On 17/02/15 15:01, Hanjun Guo wrote:
> On 2015年02月16日 18:08, Sudeep Holla wrote:
>>
>>
>> On 13/02/15 07:56, Hanjun Guo wrote:
>>> On 2014年11月29日 07:26, Rafael J. Wysocki wrote:
>>>> On Friday, November 28, 2014 07:05:09 PM Sudeep Holla wrote:
>>>>> Hi Rafael,
>>>>>
>>>>> On 26/11/14 22:26, Rafael J. Wysocki wrote:
>>>>>> On Wednesday, November 26, 2014 10:33:04 AM Sudeep Holla wrote:
>>>>>>> Hi Hanjun,
>>>>>>>
>>>>>>> On 26/11/14 09:53, Hanjun Guo wrote:
>>>>>>>> Hi Sudeep,
>>>>>>>>
>>>>>>>> On 2014-11-25 22:48, Sudeep Holla wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>>>
>>>>>>>>> This patch removes that incorrect comparision in
>>>>>>>>> acpi_processor_hotadd_init as the lone user of this function is
>>>>>>>>> already checking for correctness of phys_id before calling it.
>>>>>>>>
>>>>>>>> if (apic_id < 0) acpi_handle_debug(pr->handle, "failed to get CPU
>>>>>>>> APIC ID.\n");
>>>>>>>>
>>>>>>>> it only check the value and print debug message but no returns, so I
>>>>>>>> think the check in the following patch is still needed.
>>>>>>>>
>>>>>>>
>>>>>>> Agreed, but that's something we need to fix in the logic and not by
>>>>>>> changing these identifiers to signed values in the structures.
>>>>>>
>>>>>> I'd rather not change data structures just because of what one
>>>>>> funtion returns.
>>>>>>
>>>>>> Instead, I'd do something like
>>>>>>
>>>>>>      #define CPU_PHYS_ID_INVALID    (u32)(-1)
>>>>>>
>>>>>> change the function in question to return CPU_PHYS_ID_INVALID
>>>>>> instead of -1
>>>>>> and change the check to
>>>>>>
>>>>>>      if (phys_id == CPU_PHYS_ID_INVALID)
>>>>>>          ...
>>>>>>
>>>>>
>>>>> Do I need to rebase this on top of Hanjun's cleanups to convert apic_id
>>>>> to phys_id ? Since the variable is getting renamed it will conflict.
>>>>
>>>> Yes, it's better to rebase IMO.
>>>
>>> Hi Sudeep, any updates for this patch? I'm working on introducing
>>> typedef u32 phys_cpuid_t for x86 and ia64 for phys_id, and need
>>> this CPU_PHYS_ID_INVALID macro, if you are not working on that, I
>>> will restart the work to address the comments from Lorenzo and
>>> Catalin [1].
>>>
>>
>> IMO, it would be good if you work on that instead, so that there's no
>> dependency created as it's needed for your ARM64 port anyway.
>
> I think so, I just want to confirm that it will be no duplicate work
> for us. I will prepare a patch.

No issues, thanks for picking it up.

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html