Re: ThinkPad T480s & LED_MUTE, LED_MICMUTE

[Pali RohÃr]

On Monday 18 June 2018 12:36:31 Takashi Iwai wrote:
> On Mon, 18 Jun 2018 12:28:06 +0200,
> Pali RohÃr wrote:
> > 
> > On Saturday 16 June 2018 18:02:14 Takashi Iwai wrote:
> > > On Sat, 16 Jun 2018 17:43:09 +0200,
> > > Pali RohÃr wrote:
> > > > 
> > > > On Saturday 16 June 2018 09:05:41 Takashi Iwai wrote:
> > > > > On Fri, 15 Jun 2018 21:09:59 +0200,
> > > > > Pali RohÃr wrote:
> > > > > > 
> > > > > > On Friday 15 June 2018 14:51:47 Takashi Iwai wrote:
> > > > > > > On Fri, 08 Jun 2018 13:10:57 +0200,
> > > > > > > Pali RohÃr wrote:
> > > > > > > > 
> > > > > > > > Hi! With up-to-date thinkpad_acpi.ko driver on ThinkPad T480s I'm seeing
> > > > > > > > a strange behavior of LEDs which are integrated into mic mute (Fn+F4)
> > > > > > > > and mute (Fn+F1) keys.
> > > > > > > > 
> > > > > > > > When thinkpad_acpi.ko is not loaded, then mute key is working fine. When
> > > > > > > > pressed, it correctly generates KEY_MUTE on AT Translated Set 2 keyboard
> > > > > > > > input device and also turn on/of mute led. But when micmute key is
> > > > > > > > pressed then, nothing happen. No key event is reported and also led is
> > > > > > > > not turned on/off.
> > > > > > > > 
> > > > > > > > On the other hand, when thinkpad_acpi.ko is loaded, then both buttons
> > > > > > > > mute and micmute correctly generates input events; mute via AT keyboard
> > > > > > > > and micmute via ThinkPad Extra Buttons. But led is not changed. When
> > > > > > > > thinkpad_acpi.ko is loaded it turn off both leds (mute and micmute) and
> > > > > > > > leds after pressing any of those buttons, leds are not turned on again.
> > > > > > > > 
> > > > > > > > When thinkpad_acpi.ko is unloaded, then pressing mute button again start
> > > > > > > > switching led on/off.
> > > > > > > > 
> > > > > > > > So it seems that some init sequence of thinkpad_acpi.ko breaks mute led.
> > > > > > > > And fini sequence of thinkpad_acpi.ko makes mute led working again.
> > > > > > > 
> > > > > > > Usually the mute LED on Thinkpad is triggered from HD-audio driver
> > > > > > > (sound/pci/hda/thinkpad_helper.c), and it's a soft-bound via
> > > > > > > symbol_request(tpacpi_led_set).  I thought thinkpad_acpi is
> > > > > > > auto-loaded when the module gets bound.
> > > > > > > 
> > > > > > > A possible explanation would be that TPT480s has neither IBM0068,
> > > > > > > LEN0068 nor LEN0268 ACPI HIDs, hence the driver is not auto-loaded.
> > > > > > 
> > > > > > I have Debian Stretch kernel (4.9) which does not have LEN0268 alias for
> > > > > > thinkpad_acpi.ko. So thinkpad_acpi.ko is not loaded automatically. But I
> > > > > > have put thinkpad_acpi into /etc/modules and it is now automatically
> > > > > > loaded at boot.
> > > > > 
> > > > > That's odd.  It's exposed via
> > > > >   MODULE_DEVICE_TABLE(acpi, ibm_htk_device_ids);
> > > > > 
> > > > > It's been already in 4.9.  At this point, something is fishy.
> > > > 
> > > > $ /sbin/modinfo thinkpad_acpi | grep alias
> > > > alias:          dmi:bvnIBM:bvrI[MU]ET??WW*
> > > > alias:          tpacpi
> > > > alias:          acpi*:LEN0068:*
> > > > alias:          acpi*:IBM0068:*
> > > > 
> > > > No there is no LEN0268 on 4.9.
> > > 
> > > OK, that's the cause.  It's really old.
> > > 
> > > The commit a3c42a467a25 ("platform/x86: thinkpad_acpi: Adding new
> > > hotkey ID for Lenovo thinkpad") has to be backported.
> > 
> > This commit just autoloads thinkpad_acpi driver, right? So manual
> > modprobe (for now) is OK too?
> 
> Well, the manual modprobe is superfluous if it's properly bound with
> HD-audio driver.  The HD-audio driver calls symbol_request(), so it
> should do modprobe by itself.
> 
> > > Also, in the HD-audio side, the commit 2ecb704a1290 ("ALSA: hda - add
> > > a new condition to check if it is thinkpad") is needed, too.
> > 
> > This commit was introduced in 4.9 and Debian kernel has it. I looked
> > into Debian source code and there is check for LEN0268 in file
> > sound/pci/hda/thinkpad_helper.c
> 
> Then check whether the function gets really called.

Checked, no it is not called.

Now I see that snd_hda_codec_generic is visible in lsmod and thinkpad
helper is compiled only into snd-hda-codec-realtek and
snd-hda-codec-conexant... Therefore nobody is even trying to use that
function.

And yes, sound playback and microphone recording is working.

> 
> > > > > > I also compiled upstream version of thinkpad_acpi.ko, loaded it in
> > > > > > Stretch kernel, but it behaves in same way.
> > > > > >
> > > > > > Maybe... there could be a problem that thinkpad_acpi.ko must be already
> > > > > > loaded when sound subsystem is doing initialization? If yes, this could
> > > > > > explain it as /etc/modules is loaded at later stage and manually loading
> > > > > > of new version of thinkpad_acpi.ko at runtime does not help when sound
> > > > > > subsystem is already running.
> > > > > 
> > > > > Not really.  The HD-audio driver tries to bind with tpacpi_led_set()
> > > > > via symbol_request().  i.e. if it's not present, it tries to load a
> > > > > module.
> > > > > 
> > > > > Check whether hda_fixup_thinkpad_acpi() is called and the symbol gets
> > > > > loaded or not.
> > > > > 
> > > > > But, I don't think it's worth to debug such an old kernel primarily.
> > > > 
> > > > It is default one used by the last released Debian stable version.
> > > 
> > > Heh, that explains :)
> > > 
> > > And there was a recent regression in HD-audio that was addressed in
> > > 4.9.104.  If you're using some earlier 4.9.x, you might hit the
> > > problem regarding HD-audio thinkpad_acpi binding.
> > 
> > Debian has currently 4.9.88.
> > 
> > > (But I guess it doesn't work in anyway without the backport of the
> > >  commit above.)
> > 
> > Which change/regression it is? As 2ecb704a1290 is already part of 4.9 I
> > think this is a problem why it is not working...
> 
> It's 71bff398b0d4 that is in 4.9.104.
> 
> But you should really try the latest upstream kernel before spending
> too much time (not only yours but also others).

Now I see that in Debian backports is some 4.16.12 kernel version. I can
try this one (and later compile one from Linus tree).

> 
> Takashi
> 
> 
> > > > > Could you test the latest Linus tree or 4.17.x at least as a test
> > > > > basis?
> > > > 
> > > > Ok, will do that later.
> > > 
> > > If my analysis above is correct, everything should work with the
> > > recent upstream kernel as is.
> > > 
> > > Once after you confirm it, I can cook a patch to add the mixer enum to
> > > change LED behavior as you wanted.
> > > 
> > > 
> > > Takashi
> > 
> > -- 
> > Pali RohÃr
> > pali.rohar@xxxxxxxxx
> > 

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx