From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> To: Will Deacon <will.deacon@arm.com> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "hanjun.guo@linaro.org" <hanjun.guo@linaro.org>, Catalin Marinas <Catalin.Marinas@arm.com>, "Rafael J. Wysocki" <rjw@rjwysocki.net> Subject: Re: [PATCH 4/5] ARM64: kernel: acpi: refactor ACPI tables init and checks Date: Wed, 25 Mar 2015 15:13:56 +0000 [thread overview] Message-ID: <20150325151356.GA21478@red-moon> (raw) In-Reply-To: <20150325142623.GK24636@arm.com> 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 <lorenzo.pieralisi@arm.com> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Hanjun Guo <hanjun.guo@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> --- 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"); } } -- 2.2.1
WARNING: multiple messages have this Message-ID (diff)
From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 4/5] ARM64: kernel: acpi: refactor ACPI tables init and checks Date: Wed, 25 Mar 2015 15:13:56 +0000 [thread overview] Message-ID: <20150325151356.GA21478@red-moon> (raw) In-Reply-To: <20150325142623.GK24636@arm.com> 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 <lorenzo.pieralisi@arm.com> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Hanjun Guo <hanjun.guo@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> --- 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"); } } -- 2.2.1
next prev parent reply other threads:[~2015-03-25 15:13 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-03-24 17:58 [PATCH 0/5] ARM64: ACPI core updates Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-24 17:58 ` [PATCH 1/5] ACPI: move arm64 GSI IRQ model to generic GSI IRQ layer Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-25 13:20 ` Marc Zyngier 2015-03-25 13:20 ` Marc Zyngier 2015-03-25 13:20 ` Marc Zyngier 2015-03-25 15:41 ` Lorenzo Pieralisi 2015-03-25 15:41 ` Lorenzo Pieralisi 2015-03-25 15:41 ` Lorenzo Pieralisi 2015-03-25 13:23 ` Hanjun Guo 2015-03-25 13:23 ` Hanjun Guo 2015-03-25 13:23 ` Hanjun Guo 2015-03-24 17:58 ` [PATCH 2/5] ARM64: kernel: psci: factor out probe function Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-25 13:29 ` Hanjun Guo 2015-03-25 13:29 ` Hanjun Guo 2015-03-25 13:29 ` Hanjun Guo 2015-03-24 17:58 ` [PATCH 3/5] ARM64: kernel: psci: let ACPI probe PSCI version Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-25 13:35 ` Hanjun Guo 2015-03-25 13:35 ` Hanjun Guo 2015-03-25 13:35 ` Hanjun Guo 2015-03-24 17:58 ` [PATCH 4/5] ARM64: kernel: acpi: refactor ACPI tables init and checks Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-25 13:45 ` Hanjun Guo 2015-03-25 13:45 ` Hanjun Guo 2015-03-25 13:45 ` Hanjun Guo 2015-03-25 14:26 ` Will Deacon 2015-03-25 14:26 ` Will Deacon 2015-03-25 14:26 ` Will Deacon 2015-03-25 15:13 ` Lorenzo Pieralisi [this message] 2015-03-25 15:13 ` Lorenzo Pieralisi 2015-03-25 15:13 ` Lorenzo Pieralisi 2015-03-24 17:58 ` [PATCH 5/5] ARM64: kernel: acpi: honour acpi=force command line parameter Lorenzo Pieralisi 2015-03-24 17:58 ` Lorenzo Pieralisi 2015-03-25 6:20 ` Ard Biesheuvel 2015-03-25 6:20 ` Ard Biesheuvel 2015-03-25 6:20 ` Ard Biesheuvel 2015-03-25 13:56 ` Hanjun Guo 2015-03-25 13:56 ` Hanjun Guo 2015-03-25 13:56 ` Hanjun Guo 2015-03-25 15:22 ` Lorenzo Pieralisi 2015-03-25 15:22 ` Lorenzo Pieralisi 2015-03-25 15:22 ` Lorenzo Pieralisi 2015-03-25 16:25 ` Catalin Marinas 2015-03-25 16:25 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150325151356.GA21478@red-moon \ --to=lorenzo.pieralisi@arm.com \ --cc=Catalin.Marinas@arm.com \ --cc=hanjun.guo@linaro.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.