On Tue, Sep 13, 2016 at 10:48:26AM -0600, Theo de Raadt wrote:
> > Anton Lindqvist wrote:
> > > I'm trying to fix a minor annoyance on my x240: the speaker mute key
> > > LED-state is not respected at boot. Pressing the mute key will mute the
> > > speaker while the expected behavior is to unmute. The LED-state will
> > > remain out-of-sync until I run `mixerctl -t outputs.master.mute`.
> > >
> > > I've managed to determine if the speaker is muted in the acpithinkpad(4)
> > > attach function by reading from the embedded controller. However,
> > > calling wskbd_set_mixervolume at this stage returns ENODEV. I assume the
> > > audio device has not been attached yet. I'm new to kernel development
> > > and therefore wonder if this approach makes sense. If true, is it
> > > possible to postpone a task to run once a certain device has attached?
> >
> > The function you're looking for is startuphook_establish.
> >
> > But this requires crazy amounts of testing, since many thinkpads will be
> > different. It's well known that the mute button, hardware state, and
> > software
> > state can became desynced, but the specifics vary by model.
> >
>
> Actually, I think Anton might be onto something here. The desyncronization
> may be due to the audio driver + BIOS not recognizing the other's state.
By using startuphook_establish, as proposed by tedu@, I managed to get
it working. Booting with the volume muted before reboot and the volume
not muted before reboot both behaves correctly.
A few comments regarding the patch:
- At first, I tried sticking all code in the startuphook function
because it felt like a good approach to keep all the logic in one
place and reduce the number of ifdef-conditionals. However, that
caused my kernel to halt since the EC is busy at this point and the
assertion in acpiec.c:337 failed. The only other usage of acpiec_read
I can find is related to polling the sensors. I therefore kept volume
reading from the EC inside the attached function and moved it prior
attaching the sensors. The kernel then continued successfully, but I'm
still worried a that potential race still can occur. Or is the action
taken enough to make the volume EC read atomic?
- I didn't bother checking the return value of startuphook_establish
since I couldn't find any other invocation in the kernel doing so.
- The VOLUME_MUTE_MASK is taken from the FreeBSD ThinkPad ACPI driver.
If the patch looks good, then I would like to add some logging prior
asking people to try it out.
Index: acpithinkpad.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
retrieving revision 1.52
diff -u -p -r1.52 acpithinkpad.c
--- acpithinkpad.c 5 May 2016 05:12:49 -0000 1.52
+++ acpithinkpad.c 14 Sep 2016 07:38:08 -0000
@@ -16,6 +16,7 @@
*/
#include <sys/param.h>
+#include <sys/types.h>
#include <sys/systm.h>
#include <dev/acpi/acpireg.h>
@@ -105,6 +106,7 @@
#define THINKPAD_NSENSORS 9
#define THINKPAD_NTEMPSENSORS 8
+#define THINKPAD_ECOFFSET_VOLUME 0x30
#define THINKPAD_ECOFFSET_FANLO 0x84
#define THINKPAD_ECOFFSET_FANHI 0x85
@@ -163,6 +165,7 @@ void thinkpad_sensor_attach(struct ac
void thinkpad_sensor_refresh(void *);
#if NAUDIO > 0 && NWSKBD > 0
+void thinkpad_attach_deferred(void *);
extern int wskbd_set_mixervolume(long, long);
#endif
@@ -252,12 +255,24 @@ thinkpad_attach(struct device *parent, s
{
struct acpithinkpad_softc *sc = (struct acpithinkpad_softc *)self;
struct acpi_attach_args *aa = aux;
+ uint8_t vol = 0;
sc->sc_acpi = (struct acpi_softc *)parent;
sc->sc_devnode = aa->aaa_node;
printf("\n");
+#if NAUDIO > 0 && NWSKBD > 0
+ if (sc->sc_acpi->sc_ec != NULL) {
+ acpiec_read(sc->sc_acpi->sc_ec, THINKPAD_ECOFFSET_VOLUME,
+ 1, &vol);
+#define VOLUME_MUTE_MASK 0x40
+ if ((vol & VOLUME_MUTE_MASK) == VOLUME_MUTE_MASK)
+ /* Defer speaker mute */
+ startuphook_establish(thinkpad_attach_deferred, sc);
+ }
+#endif
+
/* Set event mask to receive everything */
thinkpad_enable_events(sc);
thinkpad_sensor_attach(sc);
@@ -722,3 +737,11 @@ thinkpad_set_param(struct wsdisplay_para
return -1;
}
}
+
+#if NAUDIO > 0 && NWSKBD > 0
+void
+thinkpad_attach_deferred(void *v __unused)
+{
+ wskbd_set_mixervolume(0, 1);
+}
+#endif
