| Message ID | 20150517174144.GA17503@light.dominikbrodowski.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } >
On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote: > On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote: > > On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote: > > > On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote: > > > > So the only value that would really make sense here is 5. > > > > > Overall, what about the appended patch instead of your [2-3/4] (modulo the > > > new command line parameter description which is missing here ATM)? > > > > Well, this approach works as well -- limiting it to an override for just 5 > > seems reasonable; expanding blacklist.c to also cover this case (even though > > it's not a blacklisting per se) isn't worth any discussion. > > > > Nonetheless, a few specifics: > > > > > +config ACPI_REV_OVERRIDE_POSSIBLE > > > > Why should that be a config option at all? The code savings should be > > really, really tiny; > > The idea is not about the code savings, but about having a simple way to disable > the whole thing entirely at one point. > > All of the workarounds under this option *including* the command line switch > should be temporary. Hopefully, yes. But I am not convinced about that yet (see below). > > and especially in the beginning we might see a few > > machines where testing the override might seem to be a good idea. So I'd > > favour having the command line optional, and then only specific quirks > > behind a config option: For the Dell XPS 13 it makes sense to disable the > > quirk if userspace can manage i2s sound; for other systems, there may not be > > such hope. And as this is a machine-specific decision, I fear that we have > > to do CONFIG options for each and every such DMI entry. > > I'm not sure if we need a config option for Dell in particular. We can simply > drop the quirk when it is not necessary any more. The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 queries _REV and uses it to modify its EC behaviour, and apparently breaks on Linux without that." If that is indeed the case, we will need a quirk for that Inspiron for much, much longer than for the Dell XPS 13 (2015). Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), the quirk for the Inspiron 7437 and potential quirks for other systems: Some may depend on _sound_ userspace being up to date (XPS), some may depend on _other_ parts of userspace being up to date, and some may be needed for as long as these systems exist (Inspiron 7437?). And if the userspace for the XPS is up to date, the quirk for that particular issue may not be needed any more, while other quirks (such as potentially the one for the 7437) may still be needed -- that is why I see a need for different CONFIG options. > > > +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE > > > +static bool acpi_rev_override; > > > + > > > +int __init acpi_rev_override_setup(char *str) > > > +{ > > > + acpi_rev_override = true; > > > + return 1; > > > +} > > > +__setup("acpi_rev_override", acpi_rev_override_setup); > > > +#else > > > +#define acpi_rev_override false > > > +#endif > > > > Why __setup, and not module_param? Should mean a smaller diffstat... > > Because it is consistent with the other __setup things done in this file > (and none of them is a module_param, mind you). module_param's aren't unusual in drivers/acpi/* and have substantial advantages IMO, but I also see the issue of consistency -- so if you really do prefer __setup, I'll use that approach in the next version of the patch. > > +config ACPI_REV_OVERRIDE_DELL_XPS_13_2015 > > I'm still not seeing the point and this is just !@#$%^&*()_+ ugly. I _know_ it's ugly. But this whole suggested change of _REV behavior after 3+ years is ugly, and its fallout may very well be considerably larger than is known at the moment: It all depends on whether there are other platforms out there which need such quirks, and whether these quirks are needed for different timeframes than the quirk for the Dell XPS 13 (2015). Best, Dominik
On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote: > On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote: > > On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote: > > > On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote: > > > > On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote: > > > > > So the only value that would really make sense here is 5. > > > > > > > Overall, what about the appended patch instead of your [2-3/4] (modulo the > > > > new command line parameter description which is missing here ATM)? > > > > > > Well, this approach works as well -- limiting it to an override for just 5 > > > seems reasonable; expanding blacklist.c to also cover this case (even though > > > it's not a blacklisting per se) isn't worth any discussion. > > > > > > Nonetheless, a few specifics: > > > > > > > +config ACPI_REV_OVERRIDE_POSSIBLE > > > > > > Why should that be a config option at all? The code savings should be > > > really, really tiny; > > > > The idea is not about the code savings, but about having a simple way to disable > > the whole thing entirely at one point. > > > > All of the workarounds under this option *including* the command line switch > > should be temporary. > > Hopefully, yes. But I am not convinced about that yet (see below). > > > > and especially in the beginning we might see a few > > > machines where testing the override might seem to be a good idea. So I'd > > > favour having the command line optional, and then only specific quirks > > > behind a config option: For the Dell XPS 13 it makes sense to disable the > > > quirk if userspace can manage i2s sound; for other systems, there may not be > > > such hope. And as this is a machine-specific decision, I fear that we have > > > to do CONFIG options for each and every such DMI entry. > > > > I'm not sure if we need a config option for Dell in particular. We can simply > > drop the quirk when it is not necessary any more. > > The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which > may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 > queries _REV and uses it to modify its EC behaviour, and apparently breaks > on Linux without that." If that is indeed the case, we will need a quirk > for that Inspiron for much, much longer than for the Dell XPS 13 (2015). > > Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), > the quirk for the Inspiron 7437 and potential quirks for other systems: Some > may depend on _sound_ userspace being up to date (XPS), some may depend on > _other_ parts of userspace being up to date, and some may be needed for > as long as these systems exist (Inspiron 7437?). And if the userspace for > the XPS is up to date, the quirk for that particular issue may not be needed > any more, while other quirks (such as potentially the one for the 7437) may > still be needed -- that is why I see a need for different CONFIG options. Which doesn't explain why we need a config option per quirk. To me, such config options don't add any value, because (a) everyone will set them anyway and (b) removing the quirks from the source is trivial if needed. > > > > +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE > > > > +static bool acpi_rev_override; > > > > + > > > > +int __init acpi_rev_override_setup(char *str) > > > > +{ > > > > + acpi_rev_override = true; > > > > + return 1; > > > > +} > > > > +__setup("acpi_rev_override", acpi_rev_override_setup); > > > > +#else > > > > +#define acpi_rev_override false > > > > +#endif > > > > > > Why __setup, and not module_param? Should mean a smaller diffstat... > > > > Because it is consistent with the other __setup things done in this file > > (and none of them is a module_param, mind you). > > module_param's aren't unusual in drivers/acpi/* and have substantial > advantages IMO, but I also see the issue of consistency -- so if you really > do prefer __setup, I'll use that approach in the next version of the patch. In this file I prefer __setup. > > > +config ACPI_REV_OVERRIDE_DELL_XPS_13_2015 > > > > I'm still not seeing the point and this is just !@#$%^&*()_+ ugly. > > I _know_ it's ugly. But this whole suggested change of _REV behavior after > 3+ years is ugly, and its fallout may very well be considerably larger than > is known at the moment: It all depends on whether there are other platforms > out there which need such quirks, and whether these quirks are needed for > different timeframes than the quirk for the Dell XPS 13 (2015). It was done in response to active abuse and things would have broken anyway after adding ACPI 6 support to ACPICA (if the spec hadn't changed the interpretation of _REV return values).
On Thu, May 21, 2015 at 03:24:41AM +0200, Rafael J. Wysocki wrote: > On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote: > > On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote: > > > On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote: > > > > On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote: > > > > > On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote: > > > > > > So the only value that would really make sense here is 5. > > > > > > > > > Overall, what about the appended patch instead of your [2-3/4] (modulo the > > > > > new command line parameter description which is missing here ATM)? > > > > > > > > Well, this approach works as well -- limiting it to an override for just 5 > > > > seems reasonable; expanding blacklist.c to also cover this case (even though > > > > it's not a blacklisting per se) isn't worth any discussion. > > > > > > > > Nonetheless, a few specifics: > > > > > > > > > +config ACPI_REV_OVERRIDE_POSSIBLE > > > > > > > > Why should that be a config option at all? The code savings should be > > > > really, really tiny; > > > > > > The idea is not about the code savings, but about having a simple way to disable > > > the whole thing entirely at one point. > > > > > > All of the workarounds under this option *including* the command line switch > > > should be temporary. > > > > Hopefully, yes. But I am not convinced about that yet (see below). > > > > > > and especially in the beginning we might see a few > > > > machines where testing the override might seem to be a good idea. So I'd > > > > favour having the command line optional, and then only specific quirks > > > > behind a config option: For the Dell XPS 13 it makes sense to disable the > > > > quirk if userspace can manage i2s sound; for other systems, there may not be > > > > such hope. And as this is a machine-specific decision, I fear that we have > > > > to do CONFIG options for each and every such DMI entry. > > > > > > I'm not sure if we need a config option for Dell in particular. We can simply > > > drop the quirk when it is not necessary any more. > > > > The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which > > may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 > > queries _REV and uses it to modify its EC behaviour, and apparently breaks > > on Linux without that." If that is indeed the case, we will need a quirk > > for that Inspiron for much, much longer than for the Dell XPS 13 (2015). > > > > Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), > > the quirk for the Inspiron 7437 and potential quirks for other systems: Some > > may depend on _sound_ userspace being up to date (XPS), some may depend on > > _other_ parts of userspace being up to date, and some may be needed for > > as long as these systems exist (Inspiron 7437?). And if the userspace for > > the XPS is up to date, the quirk for that particular issue may not be needed > > any more, while other quirks (such as potentially the one for the 7437) may > > still be needed -- that is why I see a need for different CONFIG options. > > Which doesn't explain why we need a config option per quirk. To me, such config > options don't add any value, because (a) everyone will set them anyway and (b) > removing the quirks from the source is trivial if needed. As long as you consider userspace and kernel moving along at a coordinated pace, yes -- where we should simply agree to disagree. That leaves just the question on whether the quirk (and especially the kernel parameter) should be hidden behind a special config option (and not just CONFIG_X86) -- especially if "everyone will set them anyway" and if "removing the quirks from the source is trivial if needed". Best, Dominik
On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > Which doesn't explain why we need a config option per quirk. To me, such config > options don't add any value, because (a) everyone will set them anyway and (b) > removing the quirks from the source is trivial if needed. We'd disable this quirk in Fedora the moment jack detection works, because we've got the userspace to handle it and using I2S is preferable to using HDA - but in doing so we might break battery detection on the other Dell that's playing _REV tricks. This seems like a suboptimal choice to have to make.
On 05/21/2015 01:10 PM, Matthew Garrett wrote: > On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> Which doesn't explain why we need a config option per quirk. To me, such config >> options don't add any value, because (a) everyone will set them anyway and (b) >> removing the quirks from the source is trivial if needed. > We'd disable this quirk in Fedora the moment jack detection works, > because we've got the userspace to handle it and using I2S is > preferable to using HDA - but in doing so we might break battery > detection on the other Dell that's playing _REV tricks. This seems > like a suboptimal choice to have to make. Having dug into this deeper with you on the other thread, battery detection already wasn't working. That machine did the _REV test and fix for Linux only on Windows 2009 _OSI and the _REV value of 5. The kernel currently responds to a later _OSI that the machine supports so that AML does not get run. I believe battery detection may be fixed on that Inspiron by 75646e758a0ecbed5024454507d5be5b9ea9dcbf. if it's not, that's a tangential problem that should be fixed to match the Windows behavior for battery detection.
On Thursday, May 21, 2015 11:10:20 AM Matthew Garrett wrote: > On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > Which doesn't explain why we need a config option per quirk. To me, such config > > options don't add any value, because (a) everyone will set them anyway and (b) > > removing the quirks from the source is trivial if needed. > > We'd disable this quirk in Fedora the moment jack detection works, > because we've got the userspace to handle it and using I2S is > preferable to using HDA - but in doing so we might break battery > detection on the other Dell that's playing _REV tricks. This seems > like a suboptimal choice to have to make. What about removing the quirk from the table in that case? Do we really need a special config option around it?
On Thursday, May 21, 2015 12:24:13 PM Dominik Brodowski wrote: > On Thu, May 21, 2015 at 03:24:41AM +0200, Rafael J. Wysocki wrote: > > On Monday, May 18, 2015 06:47:11 AM Dominik Brodowski wrote: > > > On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote: > > > > On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote: > > > > > On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote: > > > > > > On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote: > > > > > > > So the only value that would really make sense here is 5. > > > > > > > > > > > Overall, what about the appended patch instead of your [2-3/4] (modulo the > > > > > > new command line parameter description which is missing here ATM)? > > > > > > > > > > Well, this approach works as well -- limiting it to an override for just 5 > > > > > seems reasonable; expanding blacklist.c to also cover this case (even though > > > > > it's not a blacklisting per se) isn't worth any discussion. > > > > > > > > > > Nonetheless, a few specifics: > > > > > > > > > > > +config ACPI_REV_OVERRIDE_POSSIBLE > > > > > > > > > > Why should that be a config option at all? The code savings should be > > > > > really, really tiny; > > > > > > > > The idea is not about the code savings, but about having a simple way to disable > > > > the whole thing entirely at one point. > > > > > > > > All of the workarounds under this option *including* the command line switch > > > > should be temporary. > > > > > > Hopefully, yes. But I am not convinced about that yet (see below). > > > > > > > > and especially in the beginning we might see a few > > > > > machines where testing the override might seem to be a good idea. So I'd > > > > > favour having the command line optional, and then only specific quirks > > > > > behind a config option: For the Dell XPS 13 it makes sense to disable the > > > > > quirk if userspace can manage i2s sound; for other systems, there may not be > > > > > such hope. And as this is a machine-specific decision, I fear that we have > > > > > to do CONFIG options for each and every such DMI entry. > > > > > > > > I'm not sure if we need a config option for Dell in particular. We can simply > > > > drop the quirk when it is not necessary any more. > > > > > > The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which > > > may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437 > > > queries _REV and uses it to modify its EC behaviour, and apparently breaks > > > on Linux without that." If that is indeed the case, we will need a quirk > > > for that Inspiron for much, much longer than for the Dell XPS 13 (2015). > > > > > > Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015), > > > the quirk for the Inspiron 7437 and potential quirks for other systems: Some > > > may depend on _sound_ userspace being up to date (XPS), some may depend on > > > _other_ parts of userspace being up to date, and some may be needed for > > > as long as these systems exist (Inspiron 7437?). And if the userspace for > > > the XPS is up to date, the quirk for that particular issue may not be needed > > > any more, while other quirks (such as potentially the one for the 7437) may > > > still be needed -- that is why I see a need for different CONFIG options. > > > > Which doesn't explain why we need a config option per quirk. To me, such config > > options don't add any value, because (a) everyone will set them anyway and (b) > > removing the quirks from the source is trivial if needed. > > As long as you consider userspace and kernel moving along at a coordinated pace, > yes -- where we should simply agree to disagree. > > That leaves just the question on whether the quirk (and especially the kernel > parameter) should be hidden behind a special config option (and not just > CONFIG_X86) -- especially if "everyone will set them anyway" and if "removing > the quirks from the source is trivial if needed". If somebody (eg. me) doesn't want to build in the entire code associated with that stuff at all, the config option making it go away is actually useful. Conversely, if you know you need at least one of the quirks or the command line switch, building in it all is not a big deal. I just want to avoid unnecessary proliferation of config options that's going to result from that if we start to add them per quirk. I guess the Matthew's point is that distros would not need to patch their kernels to remove quirks if we added these config options to the mainline, but quite frankly I don't see much difference in this particular case.
On Thursday, May 21, 2015 01:18:44 PM Mario Limonciello wrote: > > On 05/21/2015 01:10 PM, Matthew Garrett wrote: > > On Wed, May 20, 2015 at 6:24 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > >> Which doesn't explain why we need a config option per quirk. To me, such config > >> options don't add any value, because (a) everyone will set them anyway and (b) > >> removing the quirks from the source is trivial if needed. > > We'd disable this quirk in Fedora the moment jack detection works, > > because we've got the userspace to handle it and using I2S is > > preferable to using HDA - but in doing so we might break battery > > detection on the other Dell that's playing _REV tricks. This seems > > like a suboptimal choice to have to make. > > Having dug into this deeper with you on the other thread, battery > detection already wasn't working. > That machine did the _REV test and fix for Linux only on Windows 2009 > _OSI and the _REV value of 5. > The kernel currently responds to a later _OSI that the machine supports > so that AML does not get run. > > I believe battery detection may be fixed on that Inspiron by > 75646e758a0ecbed5024454507d5be5b9ea9dcbf. > if it's not, that's a tangential problem that should be fixed to match > the Windows behavior for battery detection. OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV? If not, then we're probably spending too much time discussing this.
> OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV? > > If not, then we're probably spending too much time discussing this. From a Dell perspective I'm not aware of anything else breaking, but I'll be sure to raise its attention and any associated details if I become privy to them. I'm working with architecture to ensure that future products are not relying on _REV values as well. I know Matthew had mentioned that another OEM's product was using this as well, but I don't know what for.
On Friday, May 22, 2015 04:19:05 PM Mario_Limonciello@Dell.com wrote: > > OK, so do we have any proof that anything in addition to the sound thing is broken by returning 2 from _REV? > > > > If not, then we're probably spending too much time discussing this. > > From a Dell perspective I'm not aware of anything else breaking, but I'll be sure to raise its attention and any associated details if I become privy to them. I'm working with architecture to ensure that future products are not relying on _REV values as well. > I know Matthew had mentioned that another OEM's product was using this as well, but I don't know what for. OK, here's a deal. I'll wait for the next few days for someone to tell me if there's any known system in addition to the XPS 13 (2015) which breaks as a result of the change to return 2 from _REV. If none are reported before the next Friday, I'll just apply the patch I posted some time ago. If any reports come in later, we'll still be able to rearrange things during the 4.2 cycle or even later. If *something* is reported next week, I'll reconsider the approach depending on what that thing is.
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; }