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

[Hanjun Guo <hanjun.guo@linaro.org>]

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