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

[Dominik Brodowski <linux@dominikbrodowski.net>]

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