| Message ID | 5475A307.8040605@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- 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
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) ...
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
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 -- 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
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.
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
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
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
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
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;