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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
