| Message ID | 1430793998-21631-4-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 05/05/15 03:46, Hanjun Guo wrote: > In ACPI processor drivers, we use direct comparisons of cpu logical > id with -1 which are error prone in case logical cpuid is accidentally > assinged an error code and prevents us from returning an error-encoding > cpuid directly in some cases. > > So introduce invalid_logical_cpuid() to identify cpu with invalid > logical cpu num, then it will be used to replace the direct comparisons > with -1. > Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think you need to reorder this and 1/7 patch IMO. 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 Tuesday, May 05, 2015 10:46:34 AM Hanjun Guo wrote: > In ACPI processor drivers, we use direct comparisons of cpu logical > id with -1 which are error prone in case logical cpuid is accidentally > assinged an error code and prevents us from returning an error-encoding > cpuid directly in some cases. > > So introduce invalid_logical_cpuid() to identify cpu with invalid > logical cpu num, then it will be used to replace the direct comparisons > with -1. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > drivers/acpi/acpi_processor.c | 4 ++-- > drivers/acpi/processor_pdc.c | 5 +---- > include/linux/acpi.h | 5 +++++ > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index 95492d0..62c846b 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -274,7 +274,7 @@ static int acpi_processor_get_info(struct acpi_device *device) > * Handle UP system running SMP kernel, with no CPU > * entry in MADT > */ > - if ((pr->id == -1) && (num_online_cpus() == 1)) > + if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1)) > pr->id = 0; > } > > @@ -283,7 +283,7 @@ static int acpi_processor_get_info(struct acpi_device *device) > * less than the max # of CPUs. They should be ignored _iff > * they are physically not present. > */ > - if (pr->id == -1) { > + if (invalid_logical_cpuid(pr->id)) { > int ret = acpi_processor_hotadd_init(pr); > if (ret) > return ret; > diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c > index e5dd808..6dd98d0 100644 > --- a/drivers/acpi/processor_pdc.c > +++ b/drivers/acpi/processor_pdc.c > @@ -52,10 +52,7 @@ static bool __init processor_physically_present(acpi_handle handle) > type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0; > cpuid = acpi_get_cpuid(handle, type, acpi_id); > > - if (cpuid == -1) > - return false; > - > - return true; > + return invalid_logical_cpuid(cpuid) ? false : true; What about return !invalid_logical_cpuid(cpuid); > } > > static void acpi_set_pdc_bits(u32 *buf) > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index e4da5e3..913b49f 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -158,6 +158,11 @@ typedef u32 phys_cpuid_t; > #define PHYS_CPUID_INVALID (phys_cpuid_t)(-1) > #endif > > +static inline bool invalid_logical_cpuid(u32 cpuid) > +{ > + return (int)cpuid < 0; > +} > + > #ifdef CONFIG_ACPI_HOTPLUG_CPU > /* Arch dependent functions for cpu hotplug support */ > int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu); >
On Tuesday, May 05, 2015 12:15:13 PM Sudeep Holla wrote: > > On 05/05/15 03:46, Hanjun Guo wrote: > > In ACPI processor drivers, we use direct comparisons of cpu logical > > id with -1 which are error prone in case logical cpuid is accidentally > > assinged an error code and prevents us from returning an error-encoding > > cpuid directly in some cases. > > > > So introduce invalid_logical_cpuid() to identify cpu with invalid > > logical cpu num, then it will be used to replace the direct comparisons > > with -1. > > > > Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think > you need to reorder this and 1/7 patch IMO. Well, comparing an unsigned int with -1 is not technically invalid (although it involves an implicit type conversion), but yes, Hanjun, please reorder the patches.
On 2015?05?05? 20:01, Rafael J. Wysocki wrote: > On Tuesday, May 05, 2015 10:46:34 AM Hanjun Guo wrote: >> In ACPI processor drivers, we use direct comparisons of cpu logical >> id with -1 which are error prone in case logical cpuid is accidentally >> assinged an error code and prevents us from returning an error-encoding >> cpuid directly in some cases. >> >> So introduce invalid_logical_cpuid() to identify cpu with invalid >> logical cpu num, then it will be used to replace the direct comparisons >> with -1. >> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> drivers/acpi/acpi_processor.c | 4 ++-- >> drivers/acpi/processor_pdc.c | 5 +---- >> include/linux/acpi.h | 5 +++++ >> 3 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c >> index 95492d0..62c846b 100644 >> --- a/drivers/acpi/acpi_processor.c >> +++ b/drivers/acpi/acpi_processor.c >> @@ -274,7 +274,7 @@ static int acpi_processor_get_info(struct acpi_device *device) >> * Handle UP system running SMP kernel, with no CPU >> * entry in MADT >> */ >> - if ((pr->id == -1) && (num_online_cpus() == 1)) >> + if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1)) >> pr->id = 0; >> } >> >> @@ -283,7 +283,7 @@ static int acpi_processor_get_info(struct acpi_device *device) >> * less than the max # of CPUs. They should be ignored _iff >> * they are physically not present. >> */ >> - if (pr->id == -1) { >> + if (invalid_logical_cpuid(pr->id)) { >> int ret = acpi_processor_hotadd_init(pr); >> if (ret) >> return ret; >> diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c >> index e5dd808..6dd98d0 100644 >> --- a/drivers/acpi/processor_pdc.c >> +++ b/drivers/acpi/processor_pdc.c >> @@ -52,10 +52,7 @@ static bool __init processor_physically_present(acpi_handle handle) >> type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0; >> cpuid = acpi_get_cpuid(handle, type, acpi_id); >> >> - if (cpuid == -1) >> - return false; >> - >> - return true; >> + return invalid_logical_cpuid(cpuid) ? false : true; > > What about > > return !invalid_logical_cpuid(cpuid); yes, cleaner, will update my 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 2015?05?05? 20:04, Rafael J. Wysocki wrote: > On Tuesday, May 05, 2015 12:15:13 PM Sudeep Holla wrote: >> >> On 05/05/15 03:46, Hanjun Guo wrote: >>> In ACPI processor drivers, we use direct comparisons of cpu logical >>> id with -1 which are error prone in case logical cpuid is accidentally >>> assinged an error code and prevents us from returning an error-encoding >>> cpuid directly in some cases. >>> >>> So introduce invalid_logical_cpuid() to identify cpu with invalid >>> logical cpu num, then it will be used to replace the direct comparisons >>> with -1. >>> >> >> Ah, OK I see that this fixes the issue I raised in PATCH 1/7, so I think >> you need to reorder this and 1/7 patch IMO. > > Well, comparing an unsigned int with -1 is not technically invalid (although it > involves an implicit type conversion), but yes, Hanjun, please reorder the > patches. Sure, I will. 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 05/04/2015 10:46 PM, Hanjun Guo wrote: > In ACPI processor drivers, we use direct comparisons of cpu logical > id with -1 which are error prone in case logical cpuid is accidentally > assinged an error code and prevents us from returning an error-encoding > cpuid directly in some cases. Which is exactly what Xen code does (xen_pcpu_id() and xen_hotadd_cpu()). And patch 4/7 fixes this. -boris > > So introduce invalid_logical_cpuid() to identify cpu with invalid > logical cpu num, then it will be used to replace the direct comparisons > with -1. -- 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/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 95492d0..62c846b 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -274,7 +274,7 @@ static int acpi_processor_get_info(struct acpi_device *device) * Handle UP system running SMP kernel, with no CPU * entry in MADT */ - if ((pr->id == -1) && (num_online_cpus() == 1)) + if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1)) pr->id = 0; } @@ -283,7 +283,7 @@ static int acpi_processor_get_info(struct acpi_device *device) * less than the max # of CPUs. They should be ignored _iff * they are physically not present. */ - if (pr->id == -1) { + if (invalid_logical_cpuid(pr->id)) { int ret = acpi_processor_hotadd_init(pr); if (ret) return ret; diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c index e5dd808..6dd98d0 100644 --- a/drivers/acpi/processor_pdc.c +++ b/drivers/acpi/processor_pdc.c @@ -52,10 +52,7 @@ static bool __init processor_physically_present(acpi_handle handle) type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0; cpuid = acpi_get_cpuid(handle, type, acpi_id); - if (cpuid == -1) - return false; - - return true; + return invalid_logical_cpuid(cpuid) ? false : true; } static void acpi_set_pdc_bits(u32 *buf) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index e4da5e3..913b49f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -158,6 +158,11 @@ typedef u32 phys_cpuid_t; #define PHYS_CPUID_INVALID (phys_cpuid_t)(-1) #endif +static inline bool invalid_logical_cpuid(u32 cpuid) +{ + return (int)cpuid < 0; +} + #ifdef CONFIG_ACPI_HOTPLUG_CPU /* Arch dependent functions for cpu hotplug support */ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu);
In ACPI processor drivers, we use direct comparisons of cpu logical id with -1 which are error prone in case logical cpuid is accidentally assinged an error code and prevents us from returning an error-encoding cpuid directly in some cases. So introduce invalid_logical_cpuid() to identify cpu with invalid logical cpu num, then it will be used to replace the direct comparisons with -1. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- drivers/acpi/acpi_processor.c | 4 ++-- drivers/acpi/processor_pdc.c | 5 +---- include/linux/acpi.h | 5 +++++ 3 files changed, 8 insertions(+), 6 deletions(-)