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;
}