| Message ID | 5475A307.8040605@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 -- 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
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;