linux-kernel - Re: [ibm-acpi-devel] [PATCH] thinkpad-acpi: disable alsa volume mixer for SL410/SL510

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 6 Oct 2011 11:50:49 +0800
From:	Keng-Yü Lin <kengyu@...onical.com>
To:	Henrique de Moraes Holschuh <hmh@....eng.br>
Cc:	Henrique de Moraes Holschuh <ibm-acpi@....eng.br>,
	Matthew Garrett <mjg@...hat.com>,
	ibm-acpi-devel@...ts.sourceforge.net,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [ibm-acpi-devel] [PATCH] thinkpad-acpi: disable alsa volume mixer
 for SL410/SL510

On Thu, Mar 17, 2011 at 2:57 AM, Henrique de Moraes Holschuh
<hmh@....eng.br> wrote:
> On Tue, 15 Mar 2011, Keng-Yu Lin wrote:
>> The mute key on SL410/SL510 only works when the alsa volume mixer
>> is not enabled. This patch makes the alsa volume mixer disabled
>> on the matched SL410/SL510 EC versions.
>
> I'd like more data on this.  Is this some sort of weird interaction with
> userspace, or does the firmware actually changes behaviour when we
> enable the MUTE HKEY event in the event mask?
>

The firmware does not change its behaviour with the key enabled in the keymask.
The mute key (and the LED on it) of the two models works good without
thinkpad_acpi loaded. So in the patch I like to exclude the volume
control part for the two models.

>>  #define TPACPI_VOL_Q_MUTEONLY        0x0001  /* Mute-only control available */
>>  #define TPACPI_VOL_Q_LEVEL   0x0002  /* Volume control available */
>> +#define TPACPI_VOL_Q_IGNORE  0x0003  /* No Volume control available */
>
> Change the comment to /* Blacklist volume control */, please. And name
> it TPACPI_VOL_Q_BLACKLIST.
>
>> @@ -6921,20 +6924,21 @@ static int __init volume_init(struct ibm_init_struct *iibm)
>>       if (volume_capabilities >= TPACPI_VOL_CAP_MAX)
>>               return -EINVAL;
>>
>> +     quirks = tpacpi_check_quirks(volume_quirk_table,
>> +                                  ARRAY_SIZE(volume_quirk_table));
>> +
>>       /*
>>        * The ALSA mixer is our primary interface.
>>        * When disabled, don't install the subdriver at all
>>        */
>> -     if (!alsa_enable) {
>> +     if (!alsa_enable || quirks & TPACPI_VOL_Q_IGNORE) {
>>               vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
>>                           "ALSA mixer disabled by parameter, "
>>                           "not loading volume subdriver...\n");
>> +             alsa_enable = 0; /* reflect the quirk status in sysfs */
>>               return 1;
>>       }
>>
>> -     quirks = tpacpi_check_quirks(volume_quirk_table,
>> -                                  ARRAY_SIZE(volume_quirk_table));
>> -
>>       switch (volume_capabilities) {
>>       case TPACPI_VOL_CAP_AUTO:
>>               if (quirks & TPACPI_VOL_Q_MUTEONLY)
>
> Do not overload error messages like that.  It will now spill an
> incorrect message when the quirk list blacklists the volume control.
>
> Unfortunately, we're constrained by the unextensible alsa module command
> line ABI, so something more sensible (implementing "auto") is not
> available.
>
> Instead, please do it like this:
>
> 1.  Do not move the quirks test or error message up.
>
> 2.  After the quirk check, do this:
>
> if (quirks & TPACPI_VOL_Q_IGNORE) {
>        dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
>                   "ALSA mixer blacklisted for this model, not loading
> volume subdriver...\n");
>        return 1;
> }
>
> Note that it is a "dbg_printk", and NOT a "vdbg_printk".
>
> I'd much rather have something that allows the user to override the
> blacklisting, though.  But it cannot be done through alsa_enable.  Maybe
> you could be persuaded to add support for a "force_volume" parameter?
>
> If you do add support for "force_volume", please make it an unsigned int
> (not bool), use 0 for no, and 1 for yes, and leave the other values
> undefined.  And add it to the documentation in
> Documentation/laptops/thinkpad-acpi.txt.
>

Thanks for the suggestion. I will be sending a new version of patches.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/