diff mbox

[v3,04/17] ARM64 / ACPI: Introduce early_param for "acpi"

Message ID 1409583475-6978-5-git-send-email-hanjun.guo@linaro.org
State New
Headers show

Commit Message

Hanjun Guo Sept. 1, 2014, 2:57 p.m. UTC
From: Al Stone <al.stone@linaro.org>

Introduce one early parameters "off" for "acpi" to disable ACPI on
ARM64.

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 Documentation/kernel-parameters.txt |    3 ++-
 arch/arm64/kernel/acpi.c            |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Catalin Marinas Sept. 9, 2014, 4:37 p.m. UTC | #1
On Mon, Sep 01, 2014 at 03:57:42PM +0100, Hanjun Guo wrote:
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -74,3 +74,18 @@ void __init acpi_boot_table_init(void)
>   * TBD when ARM/ARM64 starts to support suspend...
>   */
>  int (*acpi_suspend_lowlevel)(void) = NULL;
> +
> +static int __init parse_acpi(char *arg)
> +{
> +	if (!arg)
> +		return -EINVAL;
> +
> +	/* "acpi=off" disables both ACPI table parsing and interpreter */
> +	if (strcmp(arg, "off") == 0)
> +		disable_acpi();
> +	else
> +		return -EINVAL;	/* Core will print when we return error */
> +
> +	return 0;
> +}
> +early_param("acpi", parse_acpi);

I forgot about early param, so there is a way to set acpi_disabled to 1
before populating the tables.
Bjorn Helgaas Sept. 9, 2014, 5:17 p.m. UTC | #2
On Mon, Sep 1, 2014 at 8:57 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> From: Al Stone <al.stone@linaro.org>
>
> Introduce one early parameters "off" for "acpi" to disable ACPI on
> ARM64.

It might be worth mentioning the purpose of this in the changelog (and
maybe the documentation).  I don't think this parameter is supported
on ia64, and on x86 it seems like it's mostly used as a "fix" by
Ubuntu users who consider the problem resolved when they find this
parameter.  That just means we don't get a chance to fix the real
underlying problem, so I'm not sure it's a real benefit to have the
parameter.

> Signed-off-by: Al Stone <al.stone@linaro.org>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  Documentation/kernel-parameters.txt |    3 ++-
>  arch/arm64/kernel/acpi.c            |   15 +++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 5ae8608..9dfb1d8 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -165,7 +165,7 @@ multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30
>  bytes respectively. Such letter suffixes can also be entirely omitted.
>
>
> -       acpi=           [HW,ACPI,X86]
> +       acpi=           [HW,ACPI,X86,ARM]
>                         Advanced Configuration and Power Interface
>                         Format: { force | off | strict | noirq | rsdt }
>                         force -- enable ACPI if default was off
> @@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>                                 strictly ACPI specification compliant.
>                         rsdt -- prefer RSDT over (default) XSDT
>                         copy_dsdt -- copy DSDT to memory
> +                       For ARM64, ONLY "acpi=off" is available.
>
>                         See also Documentation/power/runtime_pm.txt, pci=noacpi
>
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index b6940a0..9547275 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -74,3 +74,18 @@ void __init acpi_boot_table_init(void)
>   * TBD when ARM/ARM64 starts to support suspend...
>   */
>  int (*acpi_suspend_lowlevel)(void) = NULL;
> +
> +static int __init parse_acpi(char *arg)
> +{
> +       if (!arg)
> +               return -EINVAL;
> +
> +       /* "acpi=off" disables both ACPI table parsing and interpreter */
> +       if (strcmp(arg, "off") == 0)
> +               disable_acpi();
> +       else
> +               return -EINVAL; /* Core will print when we return error */
> +
> +       return 0;
> +}
> +early_param("acpi", parse_acpi);
> --
> 1.7.9.5
>
--
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
Jon Masters Sept. 9, 2014, 10:14 p.m. UTC | #3
On 09/09/2014 01:17 PM, Bjorn Helgaas wrote:
> On Mon, Sep 1, 2014 at 8:57 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Introduce one early parameters "off" for "acpi" to disable ACPI on
>> ARM64.
> 
> It might be worth mentioning the purpose of this in the changelog (and
> maybe the documentation).  I don't think this parameter is supported
> on ia64, and on x86 it seems like it's mostly used as a "fix" by
> Ubuntu users who consider the problem resolved when they find this
> parameter.  That just means we don't get a chance to fix the real
> underlying problem, so I'm not sure it's a real benefit to have the
> parameter.

>> -       acpi=           [HW,ACPI,X86]
>> +       acpi=           [HW,ACPI,X86,ARM]
>>                         Advanced Configuration and Power Interface
>>                         Format: { force | off | strict | noirq | rsdt }
>>                         force -- enable ACPI if default was off
>> @@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>                                 strictly ACPI specification compliant.
>>                         rsdt -- prefer RSDT over (default) XSDT
>>                         copy_dsdt -- copy DSDT to memory
>> +                       For ARM64, ONLY "acpi=off" is available.

Something along the lines of:

"The purpose of disabling ACPI on 64-bit ARM server platforms is to
allow for an orderly adoption of ACPI on early systems that may also
provide a DeviceTree based boot option initially. The acpi= option
disables both parsing of any tables passed in through the EFI System
Table RSDP and also disables the runtime ACPI interpreter on arm64".

On our internal systems, we've both ACPI and DeviceTree working with a
simple switch (acpi=) to determine the behavior, which is facilitating
an orderly total migration to ACPI that is in currently in flight.

Jon.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Will Deacon Sept. 10, 2014, 1:04 p.m. UTC | #4
On Tue, Sep 09, 2014 at 11:14:58PM +0100, Jon Masters wrote:
> On 09/09/2014 01:17 PM, Bjorn Helgaas wrote:
> > On Mon, Sep 1, 2014 at 8:57 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Introduce one early parameters "off" for "acpi" to disable ACPI on
> >> ARM64.
> > 
> > It might be worth mentioning the purpose of this in the changelog (and
> > maybe the documentation).  I don't think this parameter is supported
> > on ia64, and on x86 it seems like it's mostly used as a "fix" by
> > Ubuntu users who consider the problem resolved when they find this
> > parameter.  That just means we don't get a chance to fix the real
> > underlying problem, so I'm not sure it's a real benefit to have the
> > parameter.
> 
> >> -       acpi=           [HW,ACPI,X86]
> >> +       acpi=           [HW,ACPI,X86,ARM]
> >>                         Advanced Configuration and Power Interface
> >>                         Format: { force | off | strict | noirq | rsdt }
> >>                         force -- enable ACPI if default was off
> >> @@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >>                                 strictly ACPI specification compliant.
> >>                         rsdt -- prefer RSDT over (default) XSDT
> >>                         copy_dsdt -- copy DSDT to memory
> >> +                       For ARM64, ONLY "acpi=off" is available.
> 
> Something along the lines of:
> 
> "The purpose of disabling ACPI on 64-bit ARM server platforms is to
> allow for an orderly adoption of ACPI on early systems that may also
> provide a DeviceTree based boot option initially. The acpi= option
> disables both parsing of any tables passed in through the EFI System
> Table RSDP and also disables the runtime ACPI interpreter on arm64".

Please, let's keep this polarised rhetoric out of the Linux kernel. If ACPI
support gets merged for arm64, then the community has a commitment to
support it alongside devicetree. There isn't a migration path because
nothing is being replaced and we shouldn't dictate to users in which
circumstances they are allowed to use one firmware interface over another.

Other entities (server vendors, distributions, firmware guys, ...) can make
up their own rules, but attempting to peddle their agenda in the upstream
kernel is completely inappropriate.

It's blindingly obvious that acpi=off is there to disable ACPI at boot.
We either support that option or we don't -- none of this `oh, well you
can use it in this specific case I suppose' rubbish. I'm not questioning
your use-case, but there's really no need to talk about an `orderly
adoption' when all you need to say is that your ACPI is busted and passing
acpi=off lets you boot with a devicetree.

Yes, I'm being pedantic, but it's really important not to get the upstream
messaging wrong about ACPI. Devicetree is absolutely *not* going away and
people can choose to use whatever they prefer, however they prefer to use
it.

Will
--
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
Bjorn Helgaas Sept. 10, 2014, 1:21 p.m. UTC | #5
On Wed, Sep 10, 2014 at 7:04 AM, Will Deacon <will.deacon@arm.com> wrote:

> It's blindingly obvious that acpi=off is there to disable ACPI at boot.
> We either support that option or we don't -- none of this `oh, well you
> can use it in this specific case I suppose' rubbish. I'm not questioning
> your use-case, but there's really no need to talk about an `orderly
> adoption' when all you need to say is that your ACPI is busted and passing
> acpi=off lets you boot with a devicetree.

Maybe we should set a taint bit or give some other indication that
we're using a flag to work around breakage.

There's a big difference between parameters like "root=", "console=",
"quiet", etc., and the ones like "pci=nocrs", "pci=realloc",
"acpi=off".  The latter are basically workarounds for deficiencies in
Linux or the platform, and we should try really hard to minimize their
use.  We might need some of them as interim workarounds, but I don't
think we should regard their use as acceptable standard practice for
end users.

Bjorn
--
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
Will Deacon Sept. 10, 2014, 6:30 p.m. UTC | #6
On Wed, Sep 10, 2014 at 02:21:59PM +0100, Bjorn Helgaas wrote:
> On Wed, Sep 10, 2014 at 7:04 AM, Will Deacon <will.deacon@arm.com> wrote:
> 
> > It's blindingly obvious that acpi=off is there to disable ACPI at boot.
> > We either support that option or we don't -- none of this `oh, well you
> > can use it in this specific case I suppose' rubbish. I'm not questioning
> > your use-case, but there's really no need to talk about an `orderly
> > adoption' when all you need to say is that your ACPI is busted and passing
> > acpi=off lets you boot with a devicetree.
> 
> Maybe we should set a taint bit or give some other indication that
> we're using a flag to work around breakage.

That sounds like a good idea. Working around breakage is a fact of life,
but the taint indicates that it's not the ideal behaviour.

Will
--
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
Grant Likely Sept. 10, 2014, 9:58 p.m. UTC | #7
On Wed, 10 Sep 2014 07:21:59 -0600, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Sep 10, 2014 at 7:04 AM, Will Deacon <will.deacon@arm.com> wrote:
> 
> > It's blindingly obvious that acpi=off is there to disable ACPI at boot.
> > We either support that option or we don't -- none of this `oh, well you
> > can use it in this specific case I suppose' rubbish. I'm not questioning
> > your use-case, but there's really no need to talk about an `orderly
> > adoption' when all you need to say is that your ACPI is busted and passing
> > acpi=off lets you boot with a devicetree.
> 
> Maybe we should set a taint bit or give some other indication that
> we're using a flag to work around breakage.

Nope. No taint. Maybe a log message, but there are perfectly valid
reasons to use acpi=off, such as the user has a DT for the hardware
that moves all the PM operations into the kernel-proper to tune for a
very specific use case. This moves outside the support envelope, but
that doesn't make it a bad thing or the wrong thing to do.

g.

--
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 mbox

Patch

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..9dfb1d8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -165,7 +165,7 @@  multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30
 bytes respectively. Such letter suffixes can also be entirely omitted.
 
 
-	acpi=		[HW,ACPI,X86]
+	acpi=		[HW,ACPI,X86,ARM]
 			Advanced Configuration and Power Interface
 			Format: { force | off | strict | noirq | rsdt }
 			force -- enable ACPI if default was off
@@ -175,6 +175,7 @@  bytes respectively. Such letter suffixes can also be entirely omitted.
 				strictly ACPI specification compliant.
 			rsdt -- prefer RSDT over (default) XSDT
 			copy_dsdt -- copy DSDT to memory
+			For ARM64, ONLY "acpi=off" is available.
 
 			See also Documentation/power/runtime_pm.txt, pci=noacpi
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index b6940a0..9547275 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -74,3 +74,18 @@  void __init acpi_boot_table_init(void)
  * TBD when ARM/ARM64 starts to support suspend...
  */
 int (*acpi_suspend_lowlevel)(void) = NULL;
+
+static int __init parse_acpi(char *arg)
+{
+	if (!arg)
+		return -EINVAL;
+
+	/* "acpi=off" disables both ACPI table parsing and interpreter */
+	if (strcmp(arg, "off") == 0)
+		disable_acpi();
+	else
+		return -EINVAL;	/* Core will print when we return error */
+
+	return 0;
+}
+early_param("acpi", parse_acpi);