From patchwork Wed Mar 25 15:13:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lorenzo Pieralisi X-Patchwork-Id: 6093251 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 790329F318 for ; Wed, 25 Mar 2015 15:16:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5AFD320377 for ; Wed, 25 Mar 2015 15:16:29 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2EAA02035B for ; Wed, 25 Mar 2015 15:16:28 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Yan0c-0008WK-R4; Wed, 25 Mar 2015 15:14:18 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Yan0Z-0008Pw-K5 for linux-arm-kernel@lists.infradead.org; Wed, 25 Mar 2015 15:14:16 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CE14E2A; Wed, 25 Mar 2015 08:13:56 -0700 (PDT) Received: from red-moon (red-moon.cambridge.arm.com [10.1.203.137]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F04AF3F51A; Wed, 25 Mar 2015 08:13:48 -0700 (PDT) Date: Wed, 25 Mar 2015 15:13:56 +0000 From: Lorenzo Pieralisi To: Will Deacon Subject: Re: [PATCH 4/5] ARM64: kernel: acpi: refactor ACPI tables init and checks Message-ID: <20150325151356.GA21478@red-moon> References: <1427219940-27201-1-git-send-email-lorenzo.pieralisi@arm.com> <1427219940-27201-5-git-send-email-lorenzo.pieralisi@arm.com> <20150325142623.GK24636@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150325142623.GK24636@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150325_081415_832858_E42AFB8E X-CRM114-Status: GOOD ( 31.14 ) X-Spam-Score: -5.0 (-----) Cc: Catalin Marinas , "Rafael J. Wysocki" , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "hanjun.guo@linaro.org" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Mar 25, 2015 at 02:26:23PM +0000, Will Deacon wrote: > On Tue, Mar 24, 2015 at 05:58:54PM +0000, Lorenzo Pieralisi wrote: [...] > > void __init acpi_boot_table_init(void) > > { > > + int err; > > + > > /* > > * Enable ACPI instead of device tree unless > > * - ACPI has been disabled explicitly (acpi=off), or > > @@ -278,19 +301,32 @@ void __init acpi_boot_table_init(void) > > (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) > > return; > > > > + /* > > + * ACPI is disabled at this point. Enable it in order to parse > > + * the ACPI tables and carry out sanity checks > > + */ > > enable_acpi(); > > > > /* Initialize the ACPI boot-time table parser. */ > > - if (acpi_table_init()) { > > - disable_acpi(); > > - return; > > + err = acpi_table_init(); > > + if (err) { > > + pr_err("Failed to init ACPI tables\n"); > > + goto out; > > } > > > > - if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) { > > - /* disable ACPI if no FADT is found */ > > + /* > > + * Check FADT presence and carry out FADT sanity checks > > + */ > > + err = acpi_fadt_sanity_check(); > > + > > +out: > > + /* > > + * If ACPI tables are initialized and FADT sanity checks passed, > > + * leave ACPI enabled and carry on booting; otherwise disable ACPI > > + * on initialization error. > > + */ > > + if (err) > > disable_acpi(); > > - pr_err("Can't find FADT\n"); > > - } > > Could you rewrite most of this as: > > enable_acpi(); > if (acpi_table_init() || acpi_fadt_sanity_check()) { > pr_err("Failed to init ACPI tables\n"); > disable_acpi(); > } > > I think it reads a bit better without the out: label. Yes it makes sense acpi_fadt_sanity_check() already spits some errors but there is not really a point in jumping around. Here is the updated patch (please note this requires updating patch 5 too, I will send it inline shortly), thanks. -- >8 -- Subject: [PATCH] ARM64: kernel: acpi: refactor ACPI tables init and checks Current ACPI init code on ARM64 relies on acpi_table_parse() API to check if the FADT is present and to carry out sanity checks on that. The handler passed to the acpi_table_parse() function and used to carry out the parsing on the requested table returns a value that is ignored by the acpi_table_parse() function, so it is not possible to propagate errors back to the acpi_table_parse() caller through the handler. This forces ARM64 ACPI init code to have disable_acpi() calls scattered all over the place that makes code unwieldy and not easy to follow. This patch refactors the ARM64 ACPI init code, by creating a self-contained function (ie acpi_fadt_sanity_check()) that carries out the required checks on FADT and returns an adequate return value to the caller. This allows creating a common error path that disables ACPI and makes code more readable and easy to parse and change were further checks FADT to be added in the future. Signed-off-by: Lorenzo Pieralisi Reviewed-by: Hanjun Guo Cc: Will Deacon Cc: Hanjun Guo Cc: Catalin Marinas Cc: Rafael J. Wysocki --- arch/arm64/kernel/acpi.c | 95 ++++++++++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index a70f714..172b7c9 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -218,43 +218,60 @@ void __init acpi_init_cpus(void) pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); } -static int __init acpi_parse_fadt(struct acpi_table_header *table) +/* + * acpi_fadt_sanity_check() - Check FADT presence and carry out sanity + * checks on it + * + * Return 0 on success, <0 on failure + */ +static int __init acpi_fadt_sanity_check(void) { - struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; + struct acpi_table_header *table; + struct acpi_table_fadt *fadt; + acpi_status status; + acpi_size tbl_size; + int ret = 0; + + /* + * FADT is required on arm64; retrieve it to check its presence + * and carry out revision and ACPI HW reduced compliancy tests + */ + status = acpi_get_table_with_size(ACPI_SIG_FADT, 0, &table, &tbl_size); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get FADT table, %s\n", msg); + return -ENODEV; + } + + fadt = (struct acpi_table_fadt *)table; /* * Revision in table header is the FADT Major revision, and there * is a minor revision of FADT which was introduced by ACPI 5.1, * we only deal with ACPI 5.1 or newer revision to get GIC and SMP - * boot protocol configuration data, or we will disable ACPI. + * boot protocol configuration data. */ - if (table->revision > 5 || - (table->revision == 5 && fadt->minor_revision >= 1)) { - if (!acpi_gbl_reduced_hardware) { - pr_err("Not hardware reduced ACPI mode, will not be supported\n"); - goto disable_acpi; - } - - /* - * ACPI 5.1 only has two explicit methods to boot up SMP, - * PSCI and Parking protocol, but the Parking protocol is - * only specified for ARMv7 now, so make PSCI as the only - * way for the SMP boot protocol before some updates for - * the Parking protocol spec. - */ - if (acpi_psci_present()) - return 0; - - pr_warn("No PSCI support, will not bring up secondary CPUs\n"); - return -EOPNOTSUPP; + if (table->revision < 5 || + (table->revision == 5 && fadt->minor_revision < 1)) { + pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", + table->revision, fadt->minor_revision); + ret = -EINVAL; + goto out; } - pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", - table->revision, fadt->minor_revision); + if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { + pr_err("FADT not ACPI hardware reduced compliant\n"); + ret = -EINVAL; + } -disable_acpi: - disable_acpi(); - return -EINVAL; +out: + /* + * acpi_get_table_with_size() creates FADT table mapping that + * should be released after parsing and before resuming boot + */ + early_acpi_os_unmap_memory(table, tbl_size); + return ret; } /* @@ -262,9 +279,13 @@ disable_acpi: * 1. find RSDP and get its address, and then find XSDT * 2. extract all tables and checksums them all * 3. check ACPI FADT revision + * 4. check ACPI FADT HW reduced flag * * We can parse ACPI boot-time tables such as MADT after * this function is called. + * + * ACPI is enabled on return if ACPI tables initialized and sanity checks + * passed, disabled otherwise */ void __init acpi_boot_table_init(void) { @@ -278,18 +299,20 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return; + /* + * ACPI is disabled at this point. Enable it in order to parse + * the ACPI tables and carry out sanity checks + */ enable_acpi(); - /* Initialize the ACPI boot-time table parser. */ - if (acpi_table_init()) { - disable_acpi(); - return; - } - - if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) { - /* disable ACPI if no FADT is found */ + /* + * If ACPI tables are initialized and FADT sanity checks passed, + * leave ACPI enabled and carry on booting; otherwise disable ACPI + * on initialization error. + */ + if (acpi_table_init() || acpi_fadt_sanity_check()) { + pr_err("Failed to init ACPI tables\n"); disable_acpi(); - pr_err("Can't find FADT\n"); } }