From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [patch v11 12/23] ARM64 / ACPI: Parse MADT for SMP initialization Date: Fri, 27 Mar 2015 03:48:50 +0800 Message-ID: <551462A2.4070308@linaro.org> References: <1427205776-5060-1-git-send-email-hanjun.guo@linaro.org> <1427205776-5060-13-git-send-email-hanjun.guo@linaro.org> <20150325171735.GI14585@localhost> <20150326151510.GI2805@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20150326151510.GI2805@arm.com> Sender: linux-kernel-owner@vger.kernel.org To: Will Deacon , Catalin Marinas Cc: "Rafael J. Wysocki" , Olof Johansson , "grant.likely@linaro.org" , Mark Rutland , Ashwin Chaugule , Lorenzo Pieralisi , Robert Richter , Arnd Bergmann , "graeme.gregory@linaro.org" , "linaro-acpi@lists.linaro.org" , Marc Zyngier , "jcm@redhat.com" , Timur Tabi , "msalter@redhat.com" , "linux-kernel@vger.kernel.org" , Tomasz Nowicki , "linux-acpi@vger.kernel.org" , Mark Brown , "suravee.suthikulpanit@amd.com" , Sudeep Holla List-Id: linux-acpi@vger.kernel.org On 2015=E5=B9=B403=E6=9C=8826=E6=97=A5 23:15, Will Deacon wrote: > On Wed, Mar 25, 2015 at 05:17:35PM +0000, Catalin Marinas wrote: >> On Tue, Mar 24, 2015 at 10:02:45PM +0800, Hanjun Guo wrote: >>> +/** >>> + * acpi_map_gic_cpu_interface - generates a logical cpu number >>> + * and map to MPIDR represented by GICC structure >>> + * @mpidr: CPU's hardware id to register, MPIDR represented in MAD= T >>> + * @enabled: this cpu is enabled or not >>> + * >>> + * Returns the logical cpu number which maps to MPIDR >>> + */ >>> +static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled= ) >> >> So here we have an u8 enabled. >> >>> +static int __init >>> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *processor; >>> + >>> + processor =3D (struct acpi_madt_generic_interrupt *)header; >>> + >>> + if (BAD_MADT_ENTRY(processor, end)) >>> + return -EINVAL; >>> + >>> + acpi_table_print_madt_entry(header); >>> + >>> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITM= ASK, >>> + processor->flags & ACPI_MADT_ENABLED); >> >> and here processor->flags is u32. Luckily, ACPI_MADT_ENABLED is 1 an= d we >> don't lose any information. So either make the enabled above a bool = or >> simply pass the flags with the check in acpi_map_gic_cpu_interface() >> (personal preference for the latter). > > I've applied the following patch on top of the series. > > Will > > --->8 > > commit 8ef320319592693f4a6286d80df210fd47b3e356 > Author: Will Deacon > Date: Thu Mar 26 15:09:20 2015 +0000 > > ARM64 / ACPI: fix usage of acpi_map_gic_cpu_interface > > acpi_parse_gic_cpu_interface calls acpi_map_gic_cpu_interface by= both > passing a 32-bit value in the u8 enabled parameter and then subs= equently > ignoring its return value. > > Sort it out. > > Reported-by: Catalin Marinas > Signed-off-by: Will Deacon > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index cd60329da8c4..07649e413244 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -103,9 +103,12 @@ void __init __acpi_unmap_table(char *map, unsign= ed long size) > * > * Returns the logical cpu number which maps to MPIDR > */ > -static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) > +static int __init > +acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *proce= ssor) How about just replace u8 with u32? This function has its purpose to be lived, on x86/ia64, ACPI core will get the physcal cpu ID via ACPI handle, then pass it to the arch specific mapping function to map the physcal cpu ID with logical cpu ID for the new added CPU, so when ACPI based CPU hot-plug is introduced on ARM64, we need to go back to that solution. > { > int i; > + u64 mpidr =3D processor->arm_mpidr & MPIDR_HWID_BITMASK; > + bool enabled =3D !!(processor->flags & ACPI_MADT_ENABLED); > > if (mpidr =3D=3D INVALID_HWID) { > pr_info("Skip MADT cpu entry with invalid MPIDR\n"); > @@ -178,11 +181,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtabl= e_header *header, > return -EINVAL; > > acpi_table_print_madt_entry(header); > - > - acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMAS= K, > - processor->flags & ACPI_MADT_ENABLED); > - > - return 0; > + return acpi_map_gic_cpu_interface(processor); I don't think we need to return the error value here, in ACPI core, it will stop the MADT scanning once it returned the error value, but actually we can skip some disabled GICC (cpu) entries and find all the enabled ones in MADT, for example, cpu0 entry, with flag enabled cpu1 entry, disabled - if we return the error value, table scanning will stop cpu2 entry, enabled - and this cpu will be ignored Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752420AbbCZTtE (ORCPT ); Thu, 26 Mar 2015 15:49:04 -0400 Received: from mail-pd0-f172.google.com ([209.85.192.172]:35417 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751595AbbCZTtB (ORCPT ); Thu, 26 Mar 2015 15:49:01 -0400 Message-ID: <551462A2.4070308@linaro.org> Date: Fri, 27 Mar 2015 03:48:50 +0800 From: Hanjun Guo User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Will Deacon , Catalin Marinas CC: "Rafael J. Wysocki" , Olof Johansson , "grant.likely@linaro.org" , Mark Rutland , Ashwin Chaugule , Lorenzo Pieralisi , Robert Richter , Arnd Bergmann , "graeme.gregory@linaro.org" , "linaro-acpi@lists.linaro.org" , Marc Zyngier , "jcm@redhat.com" , Timur Tabi , "msalter@redhat.com" , "linux-kernel@vger.kernel.org" , Tomasz Nowicki , "linux-acpi@vger.kernel.org" , Mark Brown , "suravee.suthikulpanit@amd.com" , Sudeep Holla , "linux-arm-kernel@lists.infradead.org" Subject: Re: [patch v11 12/23] ARM64 / ACPI: Parse MADT for SMP initialization References: <1427205776-5060-1-git-send-email-hanjun.guo@linaro.org> <1427205776-5060-13-git-send-email-hanjun.guo@linaro.org> <20150325171735.GI14585@localhost> <20150326151510.GI2805@arm.com> In-Reply-To: <20150326151510.GI2805@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015年03月26日 23:15, Will Deacon wrote: > On Wed, Mar 25, 2015 at 05:17:35PM +0000, Catalin Marinas wrote: >> On Tue, Mar 24, 2015 at 10:02:45PM +0800, Hanjun Guo wrote: >>> +/** >>> + * acpi_map_gic_cpu_interface - generates a logical cpu number >>> + * and map to MPIDR represented by GICC structure >>> + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT >>> + * @enabled: this cpu is enabled or not >>> + * >>> + * Returns the logical cpu number which maps to MPIDR >>> + */ >>> +static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) >> >> So here we have an u8 enabled. >> >>> +static int __init >>> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *processor; >>> + >>> + processor = (struct acpi_madt_generic_interrupt *)header; >>> + >>> + if (BAD_MADT_ENTRY(processor, end)) >>> + return -EINVAL; >>> + >>> + acpi_table_print_madt_entry(header); >>> + >>> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >>> + processor->flags & ACPI_MADT_ENABLED); >> >> and here processor->flags is u32. Luckily, ACPI_MADT_ENABLED is 1 and we >> don't lose any information. So either make the enabled above a bool or >> simply pass the flags with the check in acpi_map_gic_cpu_interface() >> (personal preference for the latter). > > I've applied the following patch on top of the series. > > Will > > --->8 > > commit 8ef320319592693f4a6286d80df210fd47b3e356 > Author: Will Deacon > Date: Thu Mar 26 15:09:20 2015 +0000 > > ARM64 / ACPI: fix usage of acpi_map_gic_cpu_interface > > acpi_parse_gic_cpu_interface calls acpi_map_gic_cpu_interface by both > passing a 32-bit value in the u8 enabled parameter and then subsequently > ignoring its return value. > > Sort it out. > > Reported-by: Catalin Marinas > Signed-off-by: Will Deacon > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index cd60329da8c4..07649e413244 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -103,9 +103,12 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > * > * Returns the logical cpu number which maps to MPIDR > */ > -static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) > +static int __init > +acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) How about just replace u8 with u32? This function has its purpose to be lived, on x86/ia64, ACPI core will get the physcal cpu ID via ACPI handle, then pass it to the arch specific mapping function to map the physcal cpu ID with logical cpu ID for the new added CPU, so when ACPI based CPU hot-plug is introduced on ARM64, we need to go back to that solution. > { > int i; > + u64 mpidr = processor->arm_mpidr & MPIDR_HWID_BITMASK; > + bool enabled = !!(processor->flags & ACPI_MADT_ENABLED); > > if (mpidr == INVALID_HWID) { > pr_info("Skip MADT cpu entry with invalid MPIDR\n"); > @@ -178,11 +181,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > return -EINVAL; > > acpi_table_print_madt_entry(header); > - > - acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, > - processor->flags & ACPI_MADT_ENABLED); > - > - return 0; > + return acpi_map_gic_cpu_interface(processor); I don't think we need to return the error value here, in ACPI core, it will stop the MADT scanning once it returned the error value, but actually we can skip some disabled GICC (cpu) entries and find all the enabled ones in MADT, for example, cpu0 entry, with flag enabled cpu1 entry, disabled - if we return the error value, table scanning will stop cpu2 entry, enabled - and this cpu will be ignored Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 From: hanjun.guo@linaro.org (Hanjun Guo) Date: Fri, 27 Mar 2015 03:48:50 +0800 Subject: [patch v11 12/23] ARM64 / ACPI: Parse MADT for SMP initialization In-Reply-To: <20150326151510.GI2805@arm.com> References: <1427205776-5060-1-git-send-email-hanjun.guo@linaro.org> <1427205776-5060-13-git-send-email-hanjun.guo@linaro.org> <20150325171735.GI14585@localhost> <20150326151510.GI2805@arm.com> Message-ID: <551462A2.4070308@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2015?03?26? 23:15, Will Deacon wrote: > On Wed, Mar 25, 2015 at 05:17:35PM +0000, Catalin Marinas wrote: >> On Tue, Mar 24, 2015 at 10:02:45PM +0800, Hanjun Guo wrote: >>> +/** >>> + * acpi_map_gic_cpu_interface - generates a logical cpu number >>> + * and map to MPIDR represented by GICC structure >>> + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT >>> + * @enabled: this cpu is enabled or not >>> + * >>> + * Returns the logical cpu number which maps to MPIDR >>> + */ >>> +static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) >> >> So here we have an u8 enabled. >> >>> +static int __init >>> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *processor; >>> + >>> + processor = (struct acpi_madt_generic_interrupt *)header; >>> + >>> + if (BAD_MADT_ENTRY(processor, end)) >>> + return -EINVAL; >>> + >>> + acpi_table_print_madt_entry(header); >>> + >>> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >>> + processor->flags & ACPI_MADT_ENABLED); >> >> and here processor->flags is u32. Luckily, ACPI_MADT_ENABLED is 1 and we >> don't lose any information. So either make the enabled above a bool or >> simply pass the flags with the check in acpi_map_gic_cpu_interface() >> (personal preference for the latter). > > I've applied the following patch on top of the series. > > Will > > --->8 > > commit 8ef320319592693f4a6286d80df210fd47b3e356 > Author: Will Deacon > Date: Thu Mar 26 15:09:20 2015 +0000 > > ARM64 / ACPI: fix usage of acpi_map_gic_cpu_interface > > acpi_parse_gic_cpu_interface calls acpi_map_gic_cpu_interface by both > passing a 32-bit value in the u8 enabled parameter and then subsequently > ignoring its return value. > > Sort it out. > > Reported-by: Catalin Marinas > Signed-off-by: Will Deacon > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index cd60329da8c4..07649e413244 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -103,9 +103,12 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > * > * Returns the logical cpu number which maps to MPIDR > */ > -static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) > +static int __init > +acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) How about just replace u8 with u32? This function has its purpose to be lived, on x86/ia64, ACPI core will get the physcal cpu ID via ACPI handle, then pass it to the arch specific mapping function to map the physcal cpu ID with logical cpu ID for the new added CPU, so when ACPI based CPU hot-plug is introduced on ARM64, we need to go back to that solution. > { > int i; > + u64 mpidr = processor->arm_mpidr & MPIDR_HWID_BITMASK; > + bool enabled = !!(processor->flags & ACPI_MADT_ENABLED); > > if (mpidr == INVALID_HWID) { > pr_info("Skip MADT cpu entry with invalid MPIDR\n"); > @@ -178,11 +181,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > return -EINVAL; > > acpi_table_print_madt_entry(header); > - > - acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, > - processor->flags & ACPI_MADT_ENABLED); > - > - return 0; > + return acpi_map_gic_cpu_interface(processor); I don't think we need to return the error value here, in ACPI core, it will stop the MADT scanning once it returned the error value, but actually we can skip some disabled GICC (cpu) entries and find all the enabled ones in MADT, for example, cpu0 entry, with flag enabled cpu1 entry, disabled - if we return the error value, table scanning will stop cpu2 entry, enabled - and this cpu will be ignored Thanks Hanjun