[PATCH 0/4] ACPI: provide an override for _REV

[PATCH 0/4] ACPI: provide an override for _REV

[Dominik Brodowski]

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(-)

-- 
2.1.4

[PATCH 1/4] acpi: use same type for acpi_predefined_names values as in definition

[Dominik Brodowski]

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

[PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

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

[PATCH 3/4] acpi: add _REV quirk for Dell XPS 13 (2015)

[Dominik Brodowski]

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

[PATCH 4/4] acpi: fix kernel-parameters ordering in Documentation

[Dominik Brodowski]

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.
 
-- 
2.1.4

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] ACPI: provide an override for _REV

[Rafael J. Wysocki]

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

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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 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) {

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;
> +		} else
> +			printk(KERN_INFO PREFIX
> +				"_REV override must be between 1 to 5");
> +	}
> +
>  	return AE_OK;
>  }

Rafael

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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 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) {
> 
> 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;
> > +		} else
> > +			printk(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

Re: [PATCH 0/4] ACPI: provide an override for _REV

[Rafael J. Wysocki]

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

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

[-- Attachment #1: Type: text/plain, Size: 7596 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 8745 bytes --]

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

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

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 4932 bytes --]

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).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

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

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Matthew Garrett]

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.

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Mario Limonciello]

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.

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 2/4] acpi: allow for an override to set _REV

[Mario_Limonciello]

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

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/4] acpi: allow for an override to set _REV

[Rafael J. Wysocki]

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