From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [PATCH 07/19] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map Date: Wed, 20 Aug 2014 10:14:37 -0500 Message-ID: <20140820151437.91964C41290@trevor.secretlab.ca> References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> Return-path: Received: from mail-qc0-f179.google.com ([209.85.216.179]:61673 "EHLO mail-qc0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751506AbaHTQQY (ORCPT ); Wed, 20 Aug 2014 12:16:24 -0400 Received: by mail-qc0-f179.google.com with SMTP id m20so7797785qcx.24 for ; Wed, 20 Aug 2014 09:16:19 -0700 (PDT) In-Reply-To: <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Catalin Marinas , "Rafael J. Wysocki" , Mark Rutland Cc: Graeme Gregory , Arnd Bergmann , Sudeep Holla , Will Deacon , Jason Cooper , Marc Zyngier , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Robert Richter , Lv Zheng , Robert Moore , Lorenzo Pieralisi , Liviu Dudau , Randy Dunlap , Charles.Garcia-Tobin@arm.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-acpi-private@linaro.org, Hanjun Guo , Tomasz Nowicki On Thu, 24 Jul 2014 21:00:13 +0800, Hanjun Guo wrote: > MADT contains the information for MPIDR which is essential for > SMP initialization, parse the GIC cpu interface structures to > get the MPIDR value and map it to cpu_logical_map(), and add > enabled cpu with valid MPIDR into cpu_possible_map and > cpu_present_map. > > Signed-off-by: Hanjun Guo > Signed-off-by: Tomasz Nowicki > --- > + /* If it is the first CPU, no need to check duplicate MPIDRs */ > + if (!enabled_cpus) > + goto skip_mpidr_check; > + > + /* > + * Duplicate MPIDRs are a recipe for disaster. Scan > + * all initialized entries and check for > + * duplicates. If any is found just ignore the CPU. > + */ > + for_each_present_cpu(cpu) { > + if (cpu_logical_map(cpu) == mpidr) { > + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > + mpidr); > + return -EINVAL; > + } > + } > + > +skip_mpidr_check: > + enabled_cpus++; So, this hunk is embarrasing. The goto is completely unnecessary and the whole for_each_present_cpu() block can be inside an "if (enabled_cpus) { ... }" block. A goto is just nasty. That said, is the if even necessary? Will for_each_present_cpu() iterate over anything if it is processing the first cpu? (I don't actually know the answer here, I did a quick look but not enough to get an authoritative answer). g. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751902AbaHTQQ0 (ORCPT ); Wed, 20 Aug 2014 12:16:26 -0400 Received: from mail-qa0-f42.google.com ([209.85.216.42]:39213 "EHLO mail-qa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751213AbaHTQQY (ORCPT ); Wed, 20 Aug 2014 12:16:24 -0400 From: Grant Likely Subject: Re: [PATCH 07/19] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map To: Hanjun Guo , Catalin Marinas , "Rafael J. Wysocki" , Mark Rutland Cc: Graeme Gregory , Arnd Bergmann , Sudeep Holla , Will Deacon , Jason Cooper , Marc Zyngier , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Robert Richter , Lv Zheng , Robert Moore , Lorenzo Pieralisi , Liviu Dudau , Randy Dunlap , Charles.Garcia-Tobin@arm.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-acpi-private@linaro.org, Hanjun Guo , Tomasz Nowicki In-Reply-To: <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> Date: Wed, 20 Aug 2014 10:14:37 -0500 Message-Id: <20140820151437.91964C41290@trevor.secretlab.ca> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 24 Jul 2014 21:00:13 +0800, Hanjun Guo wrote: > MADT contains the information for MPIDR which is essential for > SMP initialization, parse the GIC cpu interface structures to > get the MPIDR value and map it to cpu_logical_map(), and add > enabled cpu with valid MPIDR into cpu_possible_map and > cpu_present_map. > > Signed-off-by: Hanjun Guo > Signed-off-by: Tomasz Nowicki > --- > + /* If it is the first CPU, no need to check duplicate MPIDRs */ > + if (!enabled_cpus) > + goto skip_mpidr_check; > + > + /* > + * Duplicate MPIDRs are a recipe for disaster. Scan > + * all initialized entries and check for > + * duplicates. If any is found just ignore the CPU. > + */ > + for_each_present_cpu(cpu) { > + if (cpu_logical_map(cpu) == mpidr) { > + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > + mpidr); > + return -EINVAL; > + } > + } > + > +skip_mpidr_check: > + enabled_cpus++; So, this hunk is embarrasing. The goto is completely unnecessary and the whole for_each_present_cpu() block can be inside an "if (enabled_cpus) { ... }" block. A goto is just nasty. That said, is the if even necessary? Will for_each_present_cpu() iterate over anything if it is processing the first cpu? (I don't actually know the answer here, I did a quick look but not enough to get an authoritative answer). g. From mboxrd@z Thu Jan 1 00:00:00 1970 From: grant.likely@linaro.org (Grant Likely) Date: Wed, 20 Aug 2014 10:14:37 -0500 Subject: [PATCH 07/19] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map In-Reply-To: <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-8-git-send-email-hanjun.guo@linaro.org> Message-ID: <20140820151437.91964C41290@trevor.secretlab.ca> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 24 Jul 2014 21:00:13 +0800, Hanjun Guo wrote: > MADT contains the information for MPIDR which is essential for > SMP initialization, parse the GIC cpu interface structures to > get the MPIDR value and map it to cpu_logical_map(), and add > enabled cpu with valid MPIDR into cpu_possible_map and > cpu_present_map. > > Signed-off-by: Hanjun Guo > Signed-off-by: Tomasz Nowicki > --- > + /* If it is the first CPU, no need to check duplicate MPIDRs */ > + if (!enabled_cpus) > + goto skip_mpidr_check; > + > + /* > + * Duplicate MPIDRs are a recipe for disaster. Scan > + * all initialized entries and check for > + * duplicates. If any is found just ignore the CPU. > + */ > + for_each_present_cpu(cpu) { > + if (cpu_logical_map(cpu) == mpidr) { > + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > + mpidr); > + return -EINVAL; > + } > + } > + > +skip_mpidr_check: > + enabled_cpus++; So, this hunk is embarrasing. The goto is completely unnecessary and the whole for_each_present_cpu() block can be inside an "if (enabled_cpus) { ... }" block. A goto is just nasty. That said, is the if even necessary? Will for_each_present_cpu() iterate over anything if it is processing the first cpu? (I don't actually know the answer here, I did a quick look but not enough to get an authoritative answer). g.