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@xxxxxxxxxxxxxxxxxxxx>
>
>
> 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:
signature.asc
Description: This is a digitally signed message part.
