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

[Sudeep Holla <sudeep.holla@arm.com>]

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