[alsa-devel] [PATCH 0/4] ACPI: provide an override for _REV
As promised, here are a few patches which allow to override the _REV (supported ACPI revision) setting of the ACPI implementation. With this patch in place (and possibly with other quirks added to drivers/acpi/osl.c -- Matthew mentioned the Inspiron 7437), the patch b1ef29725865 can be applied again (or the revert of b1ef29725865 be reverted).
Also throw in a re-ordering of the kernel-parameters Documentation.
Dominik Brodowski (4): acpi: use same type for acpi_predefined_names values as in definition acpi: allow for an override to set _REV acpi: add _REV quirk for Dell XPS 13 (2015) acpi: fix kernel-parameters ordering in Documentation
Documentation/kernel-parameters.txt | 76 ++++++++++++++++++++++--------------- drivers/acpi/Kconfig | 15 ++++++++ drivers/acpi/bus.c | 2 + drivers/acpi/internal.h | 1 + drivers/acpi/osl.c | 60 ++++++++++++++++++++++++++++- include/acpi/acpiosxf.h | 2 +- 6 files changed, 123 insertions(+), 33 deletions(-)
In the definition of struct acpi_predefined_names, value is of type char *. Make the OSL override function also work with type char * (or, more precisely, with a pointer to it).
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net --- drivers/acpi/osl.c | 2 +- include/acpi/acpiosxf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 7ccba39..db14a66 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -540,7 +540,7 @@ static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN];
acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, - acpi_string * new_val) + char **new_val) { if (!init_val || !new_val) return AE_BAD_PARAMETER; diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h index 0bc78df..d02df0a 100644 --- a/include/acpi/acpiosxf.h +++ b/include/acpi/acpiosxf.h @@ -95,7 +95,7 @@ acpi_physical_address acpi_os_get_root_pointer(void); #ifndef ACPI_USE_ALTERNATE_PROTOTYPE_acpi_os_predefined_override acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, - acpi_string * new_val); + char **new_val); #endif
#ifndef ACPI_USE_ALTERNATE_PROTOTYPE_acpi_os_table_override
By using a module parameter named acpi.supported_rev=<value>, the BIOS may be told a different supported ACPI revision compared to the default (which currently is 5, but will be modified to 2 when the revert of b1ef29725865 is reverted).
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net --- Documentation/kernel-parameters.txt | 14 ++++++++++++++ drivers/acpi/osl.c | 16 ++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 61ab162..75f1f8e 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -335,6 +335,20 @@ bytes respectively. Such letter suffixes can also be entirely omitted. to assume that this machine's pmtimer latches its value and always returns good values.
+ acpi.rev= [HW,ACPI] + Tell ACPI BIOS the supported ACPI REV + Format: <int> in range 0..5 + Up to and including Linux v4.1, the BIOS was told which + ACPI revision the ACPICA subsystem in Linux actually + supports (which was 5 at the time); from v4.2 on, this + value will be set statically to 2 to match the behavior + of other ACPI implementations. As some BIOS may operate + differently depending on which value _REV is set to, this + parameter offers the capability to specify what to export + to the BIOS. Note that such changes in the behavior of + the BIOS may only be visible after cold booting the + system with this parameter _twice_. + acpi_sci= [HW,ACPI] ACPI System Control Interrupt trigger mode Format: { level | edge | high | low }
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index db14a66..caa52f7 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -538,6 +538,11 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN];
+/* acpi_supported_rev is 0 in case of no override; overrides are limited to + * values between 1 to 5. To simplify casting, use an unsigned long */ +static unsigned long acpi_supported_rev; +module_param_named(rev, acpi_supported_rev, ulong, 0); + acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, char **new_val) @@ -552,6 +557,17 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, *new_val = acpi_os_name; }
+ if (!memcmp(init_val->name, "_REV", 4) && (acpi_supported_rev > 0)) { + if (acpi_supported_rev <= 5) { + printk(KERN_INFO PREFIX + "Overriding _REV definition to %lu\n", + acpi_supported_rev); + *new_val = (char *) acpi_supported_rev; + } else + printk(KERN_INFO PREFIX + "_REV override must be between 1 to 5"); + } + return AE_OK; }
On Thu, May 14, 2015 at 03:31:26PM +0200, Dominik Brodowski wrote:
By using a module parameter named acpi.supported_rev=<value>, the BIOS
That should be acpi.rev=<value>. Feel free to edit the commit message accordingly. Thanks!
Best, Dominik
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny; and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override;
+int __init acpi_rev_override_setup(char *str) +{
- acpi_rev_override = true;
- return 1;
+} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif
Why __setup, and not module_param? Should mean a smaller diffstat...
So, here's what I'd do based on your modification:
Documentation/kernel-parameters.txt | 16 ++++++++++++++++ drivers/acpi/Kconfig | 20 ++++++++++++++++++++ drivers/acpi/blacklist.c | 26 ++++++++++++++++++++++++++ drivers/acpi/internal.h | 1 + drivers/acpi/osl.c | 8 ++++++++ 5 files changed, 71 insertions(+)
[PATCH] acpi: override to set _REV, especially on Dell XPS 13 (2015)
By using a module parameter named acpi.override_rev=1, the BIOS may be told a different supported ACPI revision compared to the default (which currently is 5, but will be modified to 2 when the revert of b1ef29725865 is reverted).
Such an override is needed, for example, on a Dell XPS 13 (2015): Based on what ACPI exports as is supported version (_REV), the firmware configures the audio device to either work in HDA mode or in I2S mode. As the latter only works on sufficiently new userspace, add a quirk and an associated config option to force sound to HDA mode.
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 61ab162..1edb048 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -335,6 +335,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted. to assume that this machine's pmtimer latches its value and always returns good values.
+ acpi.rev_override= [HW,ACPI] + Export 5 as the supported ACPI REV + Format: { "0" | "1" } (0 = DMI-based quirks, + 1 = force-enabled) + Up to and including Linux v4.1, the BIOS was told which + ACPI revision the ACPICA subsystem in Linux actually + supports (which was 5 at the time); from v4.2 on, this + value will be set statically to 2 to match the behavior + of other ACPI implementations. As some BIOS may operate + differently depending on which value _REV is set to, this + parameter offers the capability to specify that the + previously used value 5 is to be exported to the firmware. + Note that such changes in the behavior of the BIOS may + only be visible after cold booting the system with this + parameter _twice_. + acpi_sci= [HW,ACPI] ACPI System Control Interrupt trigger mode Format: { level | edge | high | low }
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index ab2cbb5..a5272c2 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -430,4 +430,24 @@ config XPOWER_PMIC_OPREGION
endif
+config ACPI_REV_OVERRIDE_DELL_XPS_13_2015 + bool "Dell XPS 13 (2015) quirk to force HDA sound" + depends on X86 && SND_HDA + default y + help + Based on what ACPI exports as the supported revision to the firmware, + Dell XPS 13 (2015) configures its audio device to either work in HDA + mode or in I2S mode, where the former is supposed to be used on Linux + until the latter is fully supported (in the kernel as well as in user + space). + + This option enables a DMI-based quirk for the above Dell machine (so + that HDA audio is exposed by the platform firmware to the kernel) and + makes it possible to force the kernel to return "5" as the supported + ACPI revision via the "acpi_rev_override" command line switch (when + using that switch it may be necessary to carry out a cold reboot + _twice_ in a row to make it take effect on the firmware). + + If in doubt, say Y. + endif # ACPI diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c index 1d17919..ed55ad7 100644 --- a/drivers/acpi/blacklist.c +++ b/drivers/acpi/blacklist.c @@ -162,6 +162,15 @@ static int __init dmi_disable_osi_win8(const struct dmi_system_id *d) acpi_osi_setup("!Windows 2012"); return 0; } +#ifdef CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 +static int __init dmi_enable_rev_override(const struct dmi_system_id *d) +{ + printk(KERN_NOTICE PREFIX "DMI detected: %s (force ACPI _REV to 5)\n", + d->ident); + acpi_rev_override = true; + return 0; +} +#endif
static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { { @@ -325,6 +334,23 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { DMI_MATCH(DMI_PRODUCT_NAME, "1015PX"), }, }, + +#ifdef CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 + /* + * DELL XPS 13 (2015) switches sound between HDA and I2S + * depending on the ACPI _REV callback. If userspace supports + * I2S sufficiently (or if you do not care about sound), you + * can safely disable this quirk. + */ + { + .callback = dmi_enable_rev_override, + .ident = "DELL XPS 13 (2015)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9343"), + }, + }, +#endif {} };
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index ba4a61e..fc8db23 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -23,6 +23,7 @@
#define PREFIX "ACPI: "
+extern bool acpi_rev_override; acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 7ccba39..4d020d0 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -534,6 +534,9 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys) } #endif
+bool acpi_rev_override; +module_param_named(rev_override, acpi_rev_override, bool, 0); + #define ACPI_MAX_OVERRIDE_LEN 100
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN]; @@ -552,6 +555,11 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, *new_val = acpi_os_name; }
+ if (!memcmp(init_val->name, "_REV", 4) && acpi_rev_override) { + printk(KERN_INFO PREFIX "Overriding _REV return value to 5\n"); + *new_val = (char *)5; + } + return AE_OK; }
On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny;
The idea is not about the code savings, but about having a simple way to disable the whole thing entirely at one point.
All of the workarounds under this option *including* the command line switch should be temporary.
and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
I'm not sure if we need a config option for Dell in particular. We can simply drop the quirk when it is not necessary any more.
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override;
+int __init acpi_rev_override_setup(char *str) +{
- acpi_rev_override = true;
- return 1;
+} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif
Why __setup, and not module_param? Should mean a smaller diffstat...
Because it is consistent with the other __setup things done in this file (and none of them is a module_param, mind you).
So, here's what I'd do based on your modification:
Documentation/kernel-parameters.txt | 16 ++++++++++++++++ drivers/acpi/Kconfig | 20 ++++++++++++++++++++ drivers/acpi/blacklist.c | 26 ++++++++++++++++++++++++++ drivers/acpi/internal.h | 1 + drivers/acpi/osl.c | 8 ++++++++ 5 files changed, 71 insertions(+)
[PATCH] acpi: override to set _REV, especially on Dell XPS 13 (2015)
By using a module parameter named acpi.override_rev=1, the BIOS may be told a different supported ACPI revision compared to the default (which currently is 5, but will be modified to 2 when the revert of b1ef29725865 is reverted).
Such an override is needed, for example, on a Dell XPS 13 (2015): Based on what ACPI exports as is supported version (_REV), the firmware configures the audio device to either work in HDA mode or in I2S mode. As the latter only works on sufficiently new userspace, add a quirk and an associated config option to force sound to HDA mode.
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 61ab162..1edb048 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -335,6 +335,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted. to assume that this machine's pmtimer latches its value and always returns good values.
- acpi.rev_override= [HW,ACPI]
Nope. And I said why above.
Export 5 as the supported ACPI REVFormat: { "0" | "1" } (0 = DMI-based quirks,1 = force-enabled)Up to and including Linux v4.1, the BIOS was told whichACPI revision the ACPICA subsystem in Linux actuallysupports (which was 5 at the time); from v4.2 on, thisvalue will be set statically to 2 to match the behaviorof other ACPI implementations. As some BIOS may operatedifferently depending on which value _REV is set to, thisparameter offers the capability to specify that thepreviously used value 5 is to be exported to the firmware.Note that such changes in the behavior of the BIOS mayonly be visible after cold booting the system with thisparameter _twice_.- acpi_sci= [HW,ACPI] ACPI System Control Interrupt trigger mode Format: { level | edge | high | low }
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index ab2cbb5..a5272c2 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -430,4 +430,24 @@ config XPOWER_PMIC_OPREGION
endif
+config ACPI_REV_OVERRIDE_DELL_XPS_13_2015
I'm still not seeing the point and this is just !@#$%^&*()_+ ugly.
- bool "Dell XPS 13 (2015) quirk to force HDA sound"
- depends on X86 && SND_HDA
- default y
- help
Based on what ACPI exports as the supported revision to the firmware,Dell XPS 13 (2015) configures its audio device to either work in HDAmode or in I2S mode, where the former is supposed to be used on Linuxuntil the latter is fully supported (in the kernel as well as in userspace).This option enables a DMI-based quirk for the above Dell machine (sothat HDA audio is exposed by the platform firmware to the kernel) andmakes it possible to force the kernel to return "5" as the supportedACPI revision via the "acpi_rev_override" command line switch (whenusing that switch it may be necessary to carry out a cold reboot_twice_ in a row to make it take effect on the firmware).If in doubt, say Y.endif # ACPI diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c index 1d17919..ed55ad7 100644 --- a/drivers/acpi/blacklist.c +++ b/drivers/acpi/blacklist.c @@ -162,6 +162,15 @@ static int __init dmi_disable_osi_win8(const struct dmi_system_id *d) acpi_osi_setup("!Windows 2012"); return 0; } +#ifdef CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 +static int __init dmi_enable_rev_override(const struct dmi_system_id *d) +{
- printk(KERN_NOTICE PREFIX "DMI detected: %s (force ACPI _REV to 5)\n",
d->ident);- acpi_rev_override = true;
- return 0;
+} +#endif
static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { { @@ -325,6 +334,23 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { DMI_MATCH(DMI_PRODUCT_NAME, "1015PX"), }, },
+#ifdef CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015
- /*
* DELL XPS 13 (2015) switches sound between HDA and I2S* depending on the ACPI _REV callback. If userspace supports* I2S sufficiently (or if you do not care about sound), you* can safely disable this quirk.*/- {
.callback = dmi_enable_rev_override,.ident = "DELL XPS 13 (2015)",.matches = {DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9343"),},- },
+#endif {} };
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index ba4a61e..fc8db23 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -23,6 +23,7 @@
#define PREFIX "ACPI: "
+extern bool acpi_rev_override; acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 7ccba39..4d020d0 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -534,6 +534,9 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys) } #endif
+bool acpi_rev_override; +module_param_named(rev_override, acpi_rev_override, bool, 0);
#define ACPI_MAX_OVERRIDE_LEN 100
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN]; @@ -552,6 +555,11 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, *new_val = acpi_os_name; }
- if (!memcmp(init_val->name, "_REV", 4) && acpi_rev_override) {
printk(KERN_INFO PREFIX "Overriding _REV return value to 5\n");*new_val = (char *)5;- }
- return AE_OK;
}
On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote:
On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny;
The idea is not about the code savings, but about having a simple way to disable the whole thing entirely at one point.
All of the workarounds under this option *including* the command line switch should be temporary.
Hopefully, yes. But I am not convinced about that yet (see below).
and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
I'm not sure if we need a config option for Dell in particular. We can simply drop the quirk when it is not necessary any more.
The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 queries _REV and uses it to modify its EC behaviour, and apparently breaks on Linux without that." If that is indeed the case, we will need a quirk for that Inspiron for much, much longer than for the Dell XPS 13 (2015).
Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), the quirk for the Inspiron 7437 and potential quirks for other systems: Some may depend on _sound_ userspace being up to date (XPS), some may depend on _other_ parts of userspace being up to date, and some may be needed for as long as these systems exist (Inspiron 7437?). And if the userspace for the XPS is up to date, the quirk for that particular issue may not be needed any more, while other quirks (such as potentially the one for the 7437) may still be needed -- that is why I see a need for different CONFIG options.
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override;
+int __init acpi_rev_override_setup(char *str) +{
- acpi_rev_override = true;
- return 1;
+} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif
Why __setup, and not module_param? Should mean a smaller diffstat...
Because it is consistent with the other __setup things done in this file (and none of them is a module_param, mind you).
module_param's aren't unusual in drivers/acpi/* and have substantial advantages IMO, but I also see the issue of consistency -- so if you really do prefer __setup, I'll use that approach in the next version of the patch.
+config ACPI_REV_OVERRIDE_DELL_XPS_13_2015
I'm still not seeing the point and this is just !@#$%^&*()_+ ugly.
I _know_ it's ugly. But this whole suggested change of _REV behavior after 3+ years is ugly, and its fallout may very well be considerably larger than is known at the moment: It all depends on whether there are other platforms out there which need such quirks, and whether these quirks are needed for different timeframes than the quirk for the Dell XPS 13 (2015).
Best, Dominik
On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote:
On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote:
On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny;
The idea is not about the code savings, but about having a simple way to disable the whole thing entirely at one point.
All of the workarounds under this option *including* the command line switch should be temporary.
Hopefully, yes. But I am not convinced about that yet (see below).
and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
I'm not sure if we need a config option for Dell in particular. We can simply drop the quirk when it is not necessary any more.
The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 queries _REV and uses it to modify its EC behaviour, and apparently breaks on Linux without that." If that is indeed the case, we will need a quirk for that Inspiron for much, much longer than for the Dell XPS 13 (2015).
Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), the quirk for the Inspiron 7437 and potential quirks for other systems: Some may depend on _sound_ userspace being up to date (XPS), some may depend on _other_ parts of userspace being up to date, and some may be needed for as long as these systems exist (Inspiron 7437?). And if the userspace for the XPS is up to date, the quirk for that particular issue may not be needed any more, while other quirks (such as potentially the one for the 7437) may still be needed -- that is why I see a need for different CONFIG options.
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override;
+int __init acpi_rev_override_setup(char *str) +{
- acpi_rev_override = true;
- return 1;
+} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif
Why __setup, and not module_param? Should mean a smaller diffstat...
Because it is consistent with the other __setup things done in this file (and none of them is a module_param, mind you).
module_param's aren't unusual in drivers/acpi/* and have substantial advantages IMO, but I also see the issue of consistency -- so if you really do prefer __setup, I'll use that approach in the next version of the patch.
In this file I prefer __setup.
+config ACPI_REV_OVERRIDE_DELL_XPS_13_2015
I'm still not seeing the point and this is just !@#$%^&*()_+ ugly.
I _know_ it's ugly. But this whole suggested change of _REV behavior after 3+ years is ugly, and its fallout may very well be considerably larger than is known at the moment: It all depends on whether there are other platforms out there which need such quirks, and whether these quirks are needed for different timeframes than the quirk for the Dell XPS 13 (2015).
It was done in response to active abuse and things would have broken anyway after adding ACPI 6 support to ACPICA (if the spec hadn't changed the interpretation of _REV return values).
On Thu, May 21, 2015 at 03:24:41AM +0200, Rafael J. Wysocki wrote:
On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote:
On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote:
On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny;
The idea is not about the code savings, but about having a simple way to disable the whole thing entirely at one point.
All of the workarounds under this option *including* the command line switch should be temporary.
Hopefully, yes. But I am not convinced about that yet (see below).
and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
I'm not sure if we need a config option for Dell in particular. We can simply drop the quirk when it is not necessary any more.
The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 queries _REV and uses it to modify its EC behaviour, and apparently breaks on Linux without that." If that is indeed the case, we will need a quirk for that Inspiron for much, much longer than for the Dell XPS 13 (2015).
Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), the quirk for the Inspiron 7437 and potential quirks for other systems: Some may depend on _sound_ userspace being up to date (XPS), some may depend on _other_ parts of userspace being up to date, and some may be needed for as long as these systems exist (Inspiron 7437?). And if the userspace for the XPS is up to date, the quirk for that particular issue may not be needed any more, while other quirks (such as potentially the one for the 7437) may still be needed -- that is why I see a need for different CONFIG options.
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
As long as you consider userspace and kernel moving along at a coordinated pace, yes -- where we should simply agree to disagree.
That leaves just the question on whether the quirk (and especially the kernel parameter) should be hidden behind a special config option (and not just CONFIG_X86) -- especially if "everyone will set them anyway" and if "removing the quirks from the source is trivial if needed".
Best, Dominik
On Thursday, May 21, 2015 12:24:13 PM Dominik Brodowski wrote:
On Thu, May 21, 2015 at 03:24:41AM +0200, Rafael J. Wysocki wrote:
On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote:
On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote:
On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote: > So the only value that would really make sense here is 5.
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
Well, this approach works as well -- limiting it to an override for just 5 seems reasonable; expanding blacklist.c to also cover this case (even though it's not a blacklisting per se) isn't worth any discussion.
Nonetheless, a few specifics:
+config ACPI_REV_OVERRIDE_POSSIBLE
Why should that be a config option at all? The code savings should be really, really tiny;
The idea is not about the code savings, but about having a simple way to disable the whole thing entirely at one point.
All of the workarounds under this option *including* the command line switch should be temporary.
Hopefully, yes. But I am not convinced about that yet (see below).
and especially in the beginning we might see a few machines where testing the override might seem to be a good idea. So I'd favour having the command line optional, and then only specific quirks behind a config option: For the Dell XPS 13 it makes sense to disable the quirk if userspace can manage i2s sound; for other systems, there may not be such hope. And as this is a machine-specific decision, I fear that we have to do CONFIG options for each and every such DMI entry.
I'm not sure if we need a config option for Dell in particular. We can simply drop the quirk when it is not necessary any more.
The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 queries _REV and uses it to modify its EC behaviour, and apparently breaks on Linux without that." If that is indeed the case, we will need a quirk for that Inspiron for much, much longer than for the Dell XPS 13 (2015).
Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), the quirk for the Inspiron 7437 and potential quirks for other systems: Some may depend on _sound_ userspace being up to date (XPS), some may depend on _other_ parts of userspace being up to date, and some may be needed for as long as these systems exist (Inspiron 7437?). And if the userspace for the XPS is up to date, the quirk for that particular issue may not be needed any more, while other quirks (such as potentially the one for the 7437) may still be needed -- that is why I see a need for different CONFIG options.
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
As long as you consider userspace and kernel moving along at a coordinated pace, yes -- where we should simply agree to disagree.
That leaves just the question on whether the quirk (and especially the kernel parameter) should be hidden behind a special config option (and not just CONFIG_X86) -- especially if "everyone will set them anyway" and if "removing the quirks from the source is trivial if needed".
If somebody (eg. me) doesn't want to build in the entire code associated with that stuff at all, the config option making it go away is actually useful. Conversely, if you know you need at least one of the quirks or the command line switch, building in it all is not a big deal.
I just want to avoid unnecessary proliferation of config options that's going to result from that if we start to add them per quirk.
I guess the Matthew's point is that distros would not need to patch their kernels to remove quirks if we added these config options to the mainline, but quite frankly I don't see much difference in this particular case.
On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki rjw@rjwysocki.net wrote:
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
We'd disable this quirk in Fedora the moment jack detection works, because we've got the userspace to handle it and using I2S is preferable to using HDA - but in doing so we might break battery detection on the other Dell that's playing _REV tricks. This seems like a suboptimal choice to have to make.
On 05/21/2015 01:10 PM, Matthew Garrett wrote:
On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki rjw@rjwysocki.net wrote:
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
We'd disable this quirk in Fedora the moment jack detection works, because we've got the userspace to handle it and using I2S is preferable to using HDA - but in doing so we might break battery detection on the other Dell that's playing _REV tricks. This seems like a suboptimal choice to have to make.
Having dug into this deeper with you on the other thread, battery detection already wasn't working. That machine did the _REV test and fix for Linux only on Windows 2009 _OSI and the _REV value of 5. The kernel currently responds to a later _OSI that the machine supports so that AML does not get run.
I believe battery detection may be fixed on that Inspiron by 75646e758a0ecbed5024454507d5be5b9ea9dcbf. if it's not, that's a tangential problem that should be fixed to match the Windows behavior for battery detection.
On Thursday, May 21, 2015 01:18:44 PM Mario Limonciello wrote:
On 05/21/2015 01:10 PM, Matthew Garrett wrote:
On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki rjw@rjwysocki.net wrote:
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
We'd disable this quirk in Fedora the moment jack detection works, because we've got the userspace to handle it and using I2S is preferable to using HDA - but in doing so we might break battery detection on the other Dell that's playing _REV tricks. This seems like a suboptimal choice to have to make.
Having dug into this deeper with you on the other thread, battery detection already wasn't working. That machine did the _REV test and fix for Linux only on Windows 2009 _OSI and the _REV value of 5. The kernel currently responds to a later _OSI that the machine supports so that AML does not get run.
I believe battery detection may be fixed on that Inspiron by 75646e758a0ecbed5024454507d5be5b9ea9dcbf. if it's not, that's a tangential problem that should be fixed to match the Windows behavior for battery detection.
OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV?
If not, then we're probably spending too much time discussing this.
OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV?
If not, then we're probably spending too much time discussing this.
From a Dell perspective I'm not aware of anything else breaking, but I'll be sure to raise its attention and any associated details if I become privy to them. I'm working with architecture to ensure that future products are not relying on _REV values as well.
I know Matthew had mentioned that another OEM's product was using this as well, but I don't know what for.
On Friday, May 22, 2015 04:19:05 PM Mario_Limonciello@Dell.com wrote:
OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV?
If not, then we're probably spending too much time discussing this.
From a Dell perspective I'm not aware of anything else breaking, but I'll be sure to raise its attention and any associated details if I become privy to them. I'm working with architecture to ensure that future products are not relying on _REV values as well. I know Matthew had mentioned that another OEM's product was using this as well, but I don't know what for.
OK, here's a deal.
I'll wait for the next few days for someone to tell me if there's any known system in addition to the XPS 13 (2015) which breaks as a result of the change to return 2 from _REV. If none are reported before the next Friday, I'll just apply the patch I posted some time ago. If any reports come in later, we'll still be able to rearrange things during the 4.2 cycle or even later.
If *something* is reported next week, I'll reconsider the approach depending on what that thing is.
On Saturday, May 23, 2015 12:15:54 AM Rafael J. Wysocki wrote:
On Friday, May 22, 2015 04:19:05 PM Mario_Limonciello@Dell.com wrote:
OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV?
If not, then we're probably spending too much time discussing this.
From a Dell perspective I'm not aware of anything else breaking, but I'll be sure to raise its attention and any associated details if I become privy to them. I'm working with architecture to ensure that future products are not relying on _REV values as well. I know Matthew had mentioned that another OEM's product was using this as well, but I don't know what for.
OK, here's a deal.
I'll wait for the next few days for someone to tell me if there's any known system in addition to the XPS 13 (2015) which breaks as a result of the change to return 2 from _REV. If none are reported before the next Friday, I'll just apply the patch I posted some time ago. If any reports come in later, we'll still be able to rearrange things during the 4.2 cycle or even later.
If *something* is reported next week, I'll reconsider the approach depending on what that thing is.
OK, so nobody has reported anything and below is the patch I'm planning to apply (before restoring _REV=2).
Please let me know if there are objections.
Thanks, Rafael
--- From: Rafael J. Wysocki rafael.j.wysocki@intel.com Subject: ACPI / init: Make it possible to override _REV
The platform firmware on some systems expects Linux to return "5" as the supported ACPI revision which makes it expose system configuration information in a special way.
For example, based on what ACPI exports as the supported revision, Dell XPS 13 (2015) configures its audio device to either work in HDA mode or in I2S mode, where the former is supposed to be used on Linux until the latter is fully supported (in the kernel as well as in user space).
Since ACPI 6 mandates that _REV should return "2" if ACPI 2 or later is supported by the OS, a subsequent change will make that happen, so make it possible to override that on systems where "5" is expected to be returned for Linux to work correctly one them (such as the Dell machine mentioned above).
Original-by: Dominik Brodowski linux@dominikbrodowski.net Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com --- Documentation/kernel-parameters.txt | 6 ++++++ drivers/acpi/Kconfig | 20 ++++++++++++++++++++ drivers/acpi/blacklist.c | 26 ++++++++++++++++++++++++++ drivers/acpi/internal.h | 1 + drivers/acpi/osl.c | 18 ++++++++++++++++++ 5 files changed, 71 insertions(+)
Index: linux-pm/drivers/acpi/blacklist.c =================================================================== --- linux-pm.orig/drivers/acpi/blacklist.c +++ linux-pm/drivers/acpi/blacklist.c @@ -162,6 +162,15 @@ static int __init dmi_disable_osi_win8(c acpi_osi_setup("!Windows 2012"); return 0; } +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static int __init dmi_enable_rev_override(const struct dmi_system_id *d) +{ + printk(KERN_NOTICE PREFIX "DMI detected: %s (force ACPI _REV to 5)\n", + d->ident); + acpi_rev_override_setup(NULL); + return 0; +} +#endif
static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { { @@ -325,6 +334,23 @@ static struct dmi_system_id acpi_osi_dmi DMI_MATCH(DMI_PRODUCT_NAME, "1015PX"), }, }, + +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE + /* + * DELL XPS 13 (2015) switches sound between HDA and I2S + * depending on the ACPI _REV callback. If userspace supports + * I2S sufficiently (or if you do not care about sound), you + * can safely disable this quirk. + */ + { + .callback = dmi_enable_rev_override, + .ident = "DELL XPS 13 (2015)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9343"), + }, + }, +#endif {} };
Index: linux-pm/drivers/acpi/internal.h =================================================================== --- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -58,6 +58,7 @@ void acpi_cmos_rtc_init(void); #else static inline void acpi_cmos_rtc_init(void) {} #endif +int acpi_rev_override_setup(char *str);
extern bool acpi_force_hot_remove;
Index: linux-pm/drivers/acpi/osl.c =================================================================== --- linux-pm.orig/drivers/acpi/osl.c +++ linux-pm/drivers/acpi/osl.c @@ -530,6 +530,19 @@ acpi_os_get_physical_address(void *virt, } #endif
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override; + +int __init acpi_rev_override_setup(char *str) +{ + acpi_rev_override = true; + return 1; +} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif + #define ACPI_MAX_OVERRIDE_LEN 100
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN]; @@ -548,6 +561,11 @@ acpi_os_predefined_override(const struct *new_val = acpi_os_name; }
+ if (!memcmp(init_val->name, "_REV", 4) && acpi_rev_override) { + printk(KERN_INFO PREFIX "Overriding _REV return value to 5\n"); + *new_val = (char *)5; + } + return AE_OK; }
Index: linux-pm/drivers/acpi/Kconfig =================================================================== --- linux-pm.orig/drivers/acpi/Kconfig +++ linux-pm/drivers/acpi/Kconfig @@ -431,6 +431,26 @@ config XPOWER_PMIC_OPREGION help This config adds ACPI operation region support for XPower AXP288 PMIC.
++config ACPI_REV_OVERRIDE_POSSIBLE + bool "Allow supported ACPI revision to be overriden" + depends on X86 + default y + help + The platform firmware on some systems expects Linux to return "5" as + the supported ACPI revision which makes it expose system configuration + information in a special way. + + For example, based on what ACPI exports as the supported revision, + Dell XPS 13 (2015) configures its audio device to either work in HDA + mode or in I2S mode, where the former is supposed to be used on Linux + until the latter is fully supported (in the kernel as well as in user + space). + + This option enables a DMI-based quirk for the above Dell machine (so + that HDA audio is exposed by the platform firmware to the kernel) and + makes it possible to force the kernel to return "5" as the supported + ACPI revision via the "acpi_rev_override" command line switch. + endif
endif # ACPI Index: linux-pm/Documentation/kernel-parameters.txt =================================================================== --- linux-pm.orig/Documentation/kernel-parameters.txt +++ linux-pm/Documentation/kernel-parameters.txt @@ -285,6 +285,12 @@ bytes respectively. Such letter suffixes dynamic table installation which will install SSDT tables to /sys/firmware/acpi/tables/dynamic.
+ acpi_rev_override [ACPI] Override the _REV object to return 5 (instead + of 2 which is mandated by ACPI 6) as the supported ACPI + specification revision (when using this switch, it may + be necessary to carry out a cold reboot _twice_ in a + row to make it take effect on the platform firmware). + acpi_rsdp= [ACPI,EFI,KEXEC] Pass the RSDP address to the kernel, mostly used on machines running EFI runtime service to boot the
On Thursday, May 21, 2015 11:10:20 AM Matthew Garrett wrote:
On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki rjw@rjwysocki.net wrote:
Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed.
We'd disable this quirk in Fedora the moment jack detection works, because we've got the userspace to handle it and using I2S is preferable to using HDA - but in doing so we might break battery detection on the other Dell that's playing _REV tricks. This seems like a suboptimal choice to have to make.
What about removing the quirk from the table in that case? Do we really need a special config option around it?
On Thursday, May 14, 2015 03:31:26 PM Dominik Brodowski wrote:
By using a module parameter named acpi.supported_rev=<value>, the BIOS may be told a different supported ACPI revision compared to the default (which currently is 5, but will be modified to 2 when the revert of b1ef29725865 is reverted).
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net
Documentation/kernel-parameters.txt | 14 ++++++++++++++ drivers/acpi/osl.c | 16 ++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 61ab162..75f1f8e 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -335,6 +335,20 @@ bytes respectively. Such letter suffixes can also be entirely omitted. to assume that this machine's pmtimer latches its value and always returns good values.
- acpi.rev= [HW,ACPI]
Tell ACPI BIOS the supported ACPI REVFormat: <int> in range 0..5Up to and including Linux v4.1, the BIOS was told whichACPI revision the ACPICA subsystem in Linux actuallysupports (which was 5 at the time); from v4.2 on, thisvalue will be set statically to 2 to match the behaviorof other ACPI implementations. As some BIOS may operatedifferently depending on which value _REV is set to, thisparameter offers the capability to specify what to exportto the BIOS. Note that such changes in the behavior ofthe BIOS may only be visible after cold booting thesystem with this parameter _twice_.- acpi_sci= [HW,ACPI] ACPI System Control Interrupt trigger mode Format: { level | edge | high | low }
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index db14a66..caa52f7 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -538,6 +538,11 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN];
+/* acpi_supported_rev is 0 in case of no override; overrides are limited to
- values between 1 to 5. To simplify casting, use an unsigned long */
+static unsigned long acpi_supported_rev; +module_param_named(rev, acpi_supported_rev, ulong, 0);
acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, char **new_val) @@ -552,6 +557,17 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, *new_val = acpi_os_name; }
- if (!memcmp(init_val->name, "_REV", 4) && (acpi_supported_rev > 0)) {
if (acpi_supported_rev <= 5) {
So the only value that would really make sense here is 5.
1 should never ever be used with Linux, 2 is the default, 3 is equivalent to 5 for all practical purposes and 4 has never been used in practice, so it is meaningless.
I'd be better to rename the command line switch to acpi.rev_override and simply do "acpi_supported_rev = 5" for it as well as in acpi_set_supported_rev() in [3/4].
printk(KERN_INFO PREFIX"Overriding _REV definition to %lu\n",acpi_supported_rev);*new_val = (char *) acpi_supported_rev;} elseprintk(KERN_INFO PREFIX"_REV override must be between 1 to 5");- }
- return AE_OK;
}
Rafael
On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 03:31:26 PM Dominik Brodowski wrote:
By using a module parameter named acpi.supported_rev=<value>, the BIOS may be told a different supported ACPI revision compared to the default (which currently is 5, but will be modified to 2 when the revert of b1ef29725865 is reverted).
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net
Documentation/kernel-parameters.txt | 14 ++++++++++++++ drivers/acpi/osl.c | 16 ++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 61ab162..75f1f8e 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -335,6 +335,20 @@ bytes respectively. Such letter suffixes can also be entirely omitted. to assume that this machine's pmtimer latches its value and always returns good values.
- acpi.rev= [HW,ACPI]
Tell ACPI BIOS the supported ACPI REVFormat: <int> in range 0..5Up to and including Linux v4.1, the BIOS was told whichACPI revision the ACPICA subsystem in Linux actuallysupports (which was 5 at the time); from v4.2 on, thisvalue will be set statically to 2 to match the behaviorof other ACPI implementations. As some BIOS may operatedifferently depending on which value _REV is set to, thisparameter offers the capability to specify what to exportto the BIOS. Note that such changes in the behavior ofthe BIOS may only be visible after cold booting thesystem with this parameter _twice_.- acpi_sci= [HW,ACPI] ACPI System Control Interrupt trigger mode Format: { level | edge | high | low }
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index db14a66..caa52f7 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -538,6 +538,11 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN];
+/* acpi_supported_rev is 0 in case of no override; overrides are limited to
- values between 1 to 5. To simplify casting, use an unsigned long */
+static unsigned long acpi_supported_rev; +module_param_named(rev, acpi_supported_rev, ulong, 0);
acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, char **new_val) @@ -552,6 +557,17 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, *new_val = acpi_os_name; }
- if (!memcmp(init_val->name, "_REV", 4) && (acpi_supported_rev > 0)) {
if (acpi_supported_rev <= 5) {So the only value that would really make sense here is 5.
1 should never ever be used with Linux, 2 is the default, 3 is equivalent to 5 for all practical purposes and 4 has never been used in practice, so it is meaningless.
I'd be better to rename the command line switch to acpi.rev_override and simply do "acpi_supported_rev = 5" for it as well as in acpi_set_supported_rev() in [3/4].
printk(KERN_INFO PREFIX"Overriding _REV definition to %lu\n",acpi_supported_rev);*new_val = (char *) acpi_supported_rev;} elseprintk(KERN_INFO PREFIX"_REV override must be between 1 to 5");- }
- return AE_OK;
}
Overall, what about the appended patch instead of your [2-3/4] (modulo the new command line parameter description which is missing here ATM)?
--- drivers/acpi/Kconfig | 22 ++++++++++++++++++++++ drivers/acpi/blacklist.c | 26 ++++++++++++++++++++++++++ drivers/acpi/internal.h | 4 ++++ drivers/acpi/osl.c | 18 ++++++++++++++++++ 4 files changed, 70 insertions(+)
Index: linux-pm/drivers/acpi/blacklist.c =================================================================== --- linux-pm.orig/drivers/acpi/blacklist.c +++ linux-pm/drivers/acpi/blacklist.c @@ -162,6 +162,15 @@ static int __init dmi_disable_osi_win8(c acpi_osi_setup("!Windows 2012"); return 0; } +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static int __init dmi_enable_rev_override(const struct dmi_system_id *d) +{ + printk(KERN_NOTICE PREFIX "DMI detected: %s (force ACPI _REV to 5)\n", + d->ident); + acpi_rev_override_setup(NULL); + return 0; +} +#endif
static struct dmi_system_id acpi_osi_dmi_table[] __initdata = { { @@ -325,6 +334,23 @@ static struct dmi_system_id acpi_osi_dmi DMI_MATCH(DMI_PRODUCT_NAME, "1015PX"), }, }, + +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE + /* + * DELL XPS 13 (2015) switches sound between HDA and I2S + * depending on the ACPI _REV callback. If userspace supports + * I2S sufficiently (or if you do not care about sound), you + * can safely disable this quirk. + */ + { + .callback = dmi_enable_rev_override, + .ident = "DELL XPS 13 (2015)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9343"), + }, + }, +#endif {} };
Index: linux-pm/drivers/acpi/internal.h =================================================================== --- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -23,6 +23,10 @@
#define PREFIX "ACPI: "
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +int __init acpi_rev_override_setup(char *str); +#endif + acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); Index: linux-pm/drivers/acpi/osl.c =================================================================== --- linux-pm.orig/drivers/acpi/osl.c +++ linux-pm/drivers/acpi/osl.c @@ -534,6 +534,19 @@ acpi_os_get_physical_address(void *virt, } #endif
+#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE +static bool acpi_rev_override; + +int __init acpi_rev_override_setup(char *str) +{ + acpi_rev_override = true; + return 1; +} +__setup("acpi_rev_override", acpi_rev_override_setup); +#else +#define acpi_rev_override false +#endif + #define ACPI_MAX_OVERRIDE_LEN 100
static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN]; @@ -552,6 +565,11 @@ acpi_os_predefined_override(const struct *new_val = acpi_os_name; }
+ if (!memcmp(init_val->name, "_REV", 4) && acpi_rev_override) { + printk(KERN_INFO PREFIX "Overriding _REV return value to 5\n"); + *new_val = (char *)5; + } + return AE_OK; }
Index: linux-pm/drivers/acpi/Kconfig =================================================================== --- linux-pm.orig/drivers/acpi/Kconfig +++ linux-pm/drivers/acpi/Kconfig @@ -428,6 +428,28 @@ config XPOWER_PMIC_OPREGION help This config adds ACPI operation region support for XPower AXP288 PMIC.
++config ACPI_REV_OVERRIDE_POSSIBLE + bool "Allow supported ACPI revision to be overriden" + depends on X86 + default y + help + The platform firmware on some systems expects Linux to return "5" as + the supported ACPI revision which makes it expose system configuration + information in a special way. + + For example, based on what ACPI exports as the supported revision, + Dell XPS 13 (2015) configures its audio device to either work in HDA + mode or in I2S mode, where the former is supposed to be used on Linux + until the latter is fully supported (in the kernel as well as in user + space). + + This option enables a DMI-based quirk for the above Dell machine (so + that HDA audio is exposed by the platform firmware to the kernel) and + makes it possible to force the kernel to return "5" as the supported + ACPI revision via the "acpi_rev_override" command line switch (when + using that switch it may be necessary to carry out a cold reboot + _twice_ in a row to make it take effect on the firmware). + endif
endif # ACPI
Based on what ACPI exports as is supported version (_REV), the Dell XPS 13 (2015) configures its audio device to either work in HDA mode or in I2S mode. As the latter only works on sufficiently new userspace, add a quirk and an associated config option to force sound to HDA mode.
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net --- drivers/acpi/Kconfig | 15 +++++++++++++++ drivers/acpi/bus.c | 2 ++ drivers/acpi/internal.h | 1 + drivers/acpi/osl.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 16da185..76e4fa7 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -430,4 +430,19 @@ config XPOWER_PMIC_OPREGION
endif
+config ACPI_REV_OVERRIDE_DELL_XPS_13_2015 + bool "Dell XPS 13 (2015) quirk to force HDA sound" + depends on X86 && SND_HDA + default y + help + Based on what ACPI exports as is supported version, the Dell XPS 13 + (2015) configures its audio device to either work in HDA mode or in + I2S mode. As the latter only works on sufficiently new userspace, + this config option allows to force sound to HDA mode. To switch + between I2S and HDA mode, either toggle this option or pass + acpi.rev=2 (for HDA) / acpi.rev=5 (for I2S) on the kernel command + line, and perform a cold reboot _twice_. + + If in doubt, say Y. + endif # ACPI diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index c412fdb..99c2e56 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -494,6 +494,8 @@ void __init acpi_early_init(void) */ dmi_check_system(dsdt_dmi_table);
+ acpi_os_quirks(); + status = acpi_reallocate_root_table(); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index ba4a61e..89566d7 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -23,6 +23,7 @@
#define PREFIX "ACPI: "
+void acpi_os_quirks(void); acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index caa52f7..6f41f66 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -44,6 +44,7 @@ #include <linux/list.h> #include <linux/jiffies.h> #include <linux/semaphore.h> +#include <linux/dmi.h>
#include <asm/io.h> #include <asm/uaccess.h> @@ -571,6 +572,47 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val, return AE_OK; }
+#ifdef CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 +static int acpi_set_supported_rev(const struct dmi_system_id *id) +{ + printk(KERN_NOTICE + "%s detected - force ACPI _REV to %lu\n", + id->ident, (unsigned long) id->driver_data); + acpi_supported_rev = (unsigned long) id->driver_data; + return 0; +} + +static struct dmi_system_id acpi_rev_quirk_table[] __initdata = { + /* + * DELL XPS 13 (2015) switches sound between HDA and I2S + * depending on the ACPI _REV callback. If userspace supports + * I2S sufficiently (or if you do not care about sound), you + * can safely disable this quirk. + */ + { + .callback = acpi_set_supported_rev, + .ident = "DELL XPS 13 (2015)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9343"), + }, + .driver_data = (void *) 5 + }, + {} +}; +#else /* !CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 */ +static struct dmi_system_id acpi_rev_quirk_table[] __initdata = { + {} +}; +#endif /* CONFIG_ACPI_REV_OVERRIDE_DELL_XPS_13_2015 */ + +void __init acpi_os_quirks(void) +{ + dmi_check_system(acpi_rev_quirk_table); + return; +} + + #ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE #include <linux/earlycpio.h> #include <linux/memblock.h>
Signed-off-by: Dominik Brodowski linux@dominikbrodowski.net --- Documentation/kernel-parameters.txt | 62 ++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 75f1f8e..d6db165 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -179,11 +179,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
See also Documentation/power/runtime_pm.txt, pci=noacpi
- acpi_rsdp= [ACPI,EFI,KEXEC] - Pass the RSDP address to the kernel, mostly used - on machines running EFI runtime service to boot the - second kernel for kdump. - acpi_apic_instance= [ACPI, IOAPIC] Format: <int> 2: use 2nd APIC table, if available @@ -197,6 +192,14 @@ bytes respectively. Such letter suffixes can also be entirely omitted. (e.g. thinkpad_acpi, sony_acpi, etc.) instead of the ACPI video.ko driver.
+ acpica_no_return_repair [HW, ACPI] + Disable AML predefined validation mechanism + This mechanism can repair the evaluation result to make + the return objects more ACPI specification compliant. + This option is useful for developers to identify the + root cause of an AML interpreter issue when the issue + has something to do with the repair mechanism. + acpi.debug_layer= [HW,ACPI,ACPI_DEBUG] acpi.debug_level= [HW,ACPI,ACPI_DEBUG] Format: <int> @@ -225,6 +228,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted. unusable. The "log_buf_len" parameter may be useful if you need to capture more output.
+ acpi_enforce_resources= [ACPI] + { strict | lax | no } + Check for resource conflicts between native drivers + and ACPI OperationRegions (SystemIO and SystemMemory + only). IO ports and memory declared in ACPI might be + used by the ACPI subsystem in arbitrary AML code and + can interfere with legacy drivers. + strict (default): access to resources claimed by ACPI + is denied; legacy drivers trying to access reserved + resources will fail to bind to device using them. + lax: access to resources claimed by ACPI is allowed; + legacy drivers trying to access reserved resources + will bind successfully but a warning message is logged. + no: ACPI OperationRegions are not marked as reserved, + no further checks are performed. + acpi_force_table_verification [HW,ACPI] Enable table checksum verification during early stage. By default, this is disabled due to x86 early mapping @@ -253,6 +272,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted. This feature is enabled by default. This option allows to turn off the feature.
+ acpi_no_memhotplug [ACPI] Disable memory hotplug. Useful for kdump + kernels. + acpi_no_static_ssdt [HW,ACPI] Disable installation of static SSDTs at early boot time By default, SSDTs contained in the RSDT/XSDT will be @@ -263,13 +285,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted. dynamic table installation which will install SSDT tables to /sys/firmware/acpi/tables/dynamic.
- acpica_no_return_repair [HW, ACPI] - Disable AML predefined validation mechanism - This mechanism can repair the evaluation result to make - the return objects more ACPI specification compliant. - This option is useful for developers to identify the - root cause of an AML interpreter issue when the issue - has something to do with the repair mechanism. + acpi_rsdp= [ACPI,EFI,KEXEC] + Pass the RSDP address to the kernel, mostly used + on machines running EFI runtime service to boot the + second kernel for kdump.
acpi_os_name= [HW,ACPI] Tell ACPI BIOS the name of the OS Format: To spoof as Windows 98: ="Microsoft Windows" @@ -379,25 +398,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted. Use timer override. For some broken Nvidia NF5 boards that require a timer override, but don't have HPET
- acpi_enforce_resources= [ACPI] - { strict | lax | no } - Check for resource conflicts between native drivers - and ACPI OperationRegions (SystemIO and SystemMemory - only). IO ports and memory declared in ACPI might be - used by the ACPI subsystem in arbitrary AML code and - can interfere with legacy drivers. - strict (default): access to resources claimed by ACPI - is denied; legacy drivers trying to access reserved - resources will fail to bind to device using them. - lax: access to resources claimed by ACPI is allowed; - legacy drivers trying to access reserved resources - will bind successfully but a warning message is logged. - no: ACPI OperationRegions are not marked as reserved, - no further checks are performed. - - acpi_no_memhotplug [ACPI] Disable memory hotplug. Useful for kdump - kernels. - add_efi_memmap [EFI; X86] Include EFI memory map in kernel's map of available physical RAM.
On Thursday, May 14, 2015 03:31:24 PM Dominik Brodowski wrote:
As promised, here are a few patches which allow to override the _REV (supported ACPI revision) setting of the ACPI implementation. With this patch in place (and possibly with other quirks added to drivers/acpi/osl.c -- Matthew mentioned the Inspiron 7437), the patch b1ef29725865 can be applied again (or the revert of b1ef29725865 be reverted).
Also throw in a re-ordering of the kernel-parameters Documentation.
Dominik Brodowski (4): acpi: use same type for acpi_predefined_names values as in definition acpi: allow for an override to set _REV acpi: add _REV quirk for Dell XPS 13 (2015) acpi: fix kernel-parameters ordering in Documentation
Thanks!
That's mostly OK, but I have a concern about the command line arg.
Please see comments on patch [2/4] for details.
Rafael
On Thursday, May 14, 2015 10:31:59 PM Rafael J. Wysocki wrote:
On Thursday, May 14, 2015 03:31:24 PM Dominik Brodowski wrote:
As promised, here are a few patches which allow to override the _REV (supported ACPI revision) setting of the ACPI implementation. With this patch in place (and possibly with other quirks added to drivers/acpi/osl.c -- Matthew mentioned the Inspiron 7437), the patch b1ef29725865 can be applied again (or the revert of b1ef29725865 be reverted).
Also throw in a re-ordering of the kernel-parameters Documentation.
Dominik Brodowski (4): acpi: use same type for acpi_predefined_names values as in definition acpi: allow for an override to set _REV acpi: add _REV quirk for Dell XPS 13 (2015) acpi: fix kernel-parameters ordering in Documentation
Thanks!
That's mostly OK, but I have a concern about the command line arg.
I've queued up patches [1/4] and [4/4] for 4.2.
Rafael
participants (5)
-
Dominik Brodowski -
Mario Limonciello -
Mario_Limonciello＠Dell.com
-
Matthew Garrett -
Rafael J. Wysocki