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

[Rafael J. Wysocki]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150521/702f0408/attachment-0001.sig>