On Mon, Dec 21, 2015 at 10:38:24PM +0100, Mark Kettenis wrote:
> The asmc(4) driver is a bit slow. On my MacBookPro12,1 it takes a
> couple of minutes to probe all the sensors. And changing the keyboard
> backlight takes a couple of seconds. This is especially annoying when
> you're writing a diff to bind that functionality to the keys reserved
> for this purpose on the keyboard.
>
> Turns out the chip used for the Apple SMC is my old friend the H8S
> microcontroller (see lom(4) on sparc64). With that knowledge in hand,
> it was fairly obvious what was going wrong. The driver didn't check
> whether the chip input buffer was full before writing to it. That
> caused commands to get lost. With that fixed, comunication with the
> chip seems to be reliable, but still somewhat slow.
Now, that really is an improvement. Thanks for looking into this and the
earlier bug fix x-mas present!
> There were two reasons for this slowness:
>
> 1. The asmc_command() function does additional reads as a "sanity
> flush". We had to wait for these reads to time out, which took a
> significant fraction of a second. On my system, I neversaw any of
> these flusing reads succeed. So I removed that bit of code.
I added this sanity flush, because Apple seems to keep changing the
returned number of bytes for some keys depending on the firmware
revision. An example for are the light sensors keys "ALVn": on newer
Macbooks they return 10 bytes, while on older they return only 6 Bytes.
The reason is that newer Macbooks provide the Lux value directly
(instead of ADC raw data) in the added four bytes (see asmc_light).
So from my understanding: if the buffer is not emptied/flushed you might
read the data from previous command with the next key? Maybe I got that
wrong?
> 2. Failing commands were retried up to 10 times. I assume this was
> necessary because writing to the chip was unreliable.
Yes.
> I reduce it to 3 retries.
Makes sense with your buf fix.
> 3. The timouts for individual reads and writes were quite high.
> Especially because the code used tsleep(), which means a context
> switch could happen and we'd sleep longer than intended.
Yes.
> Since I also noticed that the command to set the keyboard backlight
> succeeded initially (when a simple delay was used) but failed later
> (when tsleep was used), I concluded that the SMC times out as well if
> we didn't send the complete command within a certain time.
I think this conclusion is right. That is what I have seen in my tests
as well.
> Such a minimum time is hard to guarantee if we use tsleep. So this
> driverreally has to use an actual (spinning) delay, just like what I
> did in my lom(4) driver.
>
> That sounds worse than it is. Typically we spin only for a short time
> (500 us seems to be typical) for read and write operations that
> succeed. So I clamped it at 5 ms. With the diff below, the asmc
> taskq consumes about the same amount of CPU as the acpi thread. I
> think that is perfectly acceptable.
Agreed, that totally makes sense. I just added the tsleep() delays to
make things working... now, with your input buffer fix they are not
really needed anymore.
> However, since Apple's SMC implementation almost certainly changed
> over the years, it would be good to test this diff on a wider range of
> Apple hardware. Please check that all sensors are still present and
> are properly updated after applying this diff.
Works fine here.
ok jung@ (but we really should discuss the sanity flush removal)
> Thanks,
>
> Mark
>
>
> Index: asmc.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/isa/asmc.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 asmc.c
> --- asmc.c 15 Dec 2015 20:58:22 -0000 1.21
> +++ asmc.c 21 Dec 2015 21:35:05 -0000
> @@ -44,7 +44,11 @@
> #define ASMC_WRITE 0x11 /* SMC write command */
> #define ASMC_INFO 0x13 /* SMC info/type command */
>
> -#define ASMC_RETRY 10
> +#define ASMC_OBF 0x01 /* Output buffer full */
> +#define ASMC_IBF 0x02 /* Input buffer full */
> +#define ASMC_ACCEPT 0x04
> +
> +#define ASMC_RETRY 3
> #define ASMC_MAXLEN 32 /* SMC maximum data size len */
> #define ASMC_NOTFOUND 0x84 /* SMC status key not found */
>
> @@ -385,11 +389,8 @@ asmc_kbdled(void *arg)
> int r;
>
> if ((r = asmc_try(sc, ASMC_WRITE, "LKSB", buf, 2)))
> -#if 0 /* todo: write error, as no 0x04 wait status, but led is changed */
> printf("%s: keyboard backlight failed (0x%x)\n",
> sc->sc_dev.dv_xname, r);
> -#endif
> - ;
> }
>
> int
> @@ -436,44 +437,57 @@ asmc_status(struct asmc_softc *sc)
> }
>
> static int
> -asmc_wait(struct asmc_softc *sc, uint8_t m)
> +asmc_write(struct asmc_softc *sc, uint8_t off, uint8_t val)
> {
> - int us;
> + uint8_t str;
> + int i;
>
> - for (us = (2 << 4); us < (2 << 16); us *= 2) { /* wait up to 128 ms */
> - (!sc->sc_sensor_task) ? delay(us) :
> - tsleep(&sc->sc_taskq, 0, "asmc",
> - (us * hz + 999999) / 1000000 + 1);
> - if (bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND) & m)
> - return 1;
> + for (i = 500; i > 0; i--) {
> + str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
> + if ((str & ASMC_IBF) == 0)
> + break;
> + delay(10);
> }
> - return 0;
> -}
> + if (i == 0)
> + return ETIMEDOUT;
>
> -static int
> -asmc_write(struct asmc_softc *sc, uint8_t off, uint8_t val)
> -{
> bus_space_write_1(sc->sc_iot, sc->sc_ioh, off, val);
> - if (asmc_wait(sc, 0x04)) /* write accepted? */
> - return 0;
> - return 1;
> +
> + for (i = 500; i > 0; i--) {
> + str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
> + if (str & ASMC_ACCEPT)
> + break;
> + delay(10);
> + }
> + if (i == 0)
> + return ETIMEDOUT;
> +
> + return 0;
> }
>
> static int
> asmc_read(struct asmc_softc *sc, uint8_t off, uint8_t *buf)
> {
> - if (asmc_wait(sc, 0x01)) { /* ready for read? */
> - *buf = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
> - return 0;
> + uint8_t str;
> + int i;
> +
> + for (i = 500; i > 0; i--) {
> + str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
> + if (str & ASMC_OBF)
> + break;
> + delay(10);
> }
> - return 1;
> + if (i == 0)
> + return ETIMEDOUT;
> +
> + *buf = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
> + return 0;
> }
>
> static int
> asmc_command(struct asmc_softc *sc, int cmd, const char *key, uint8_t *buf,
> uint8_t len)
> {
> - uint8_t n;
> int i;
>
> if (len > ASMC_MAXLEN)
> @@ -489,9 +503,6 @@ asmc_command(struct asmc_softc *sc, int
> for (i = 0; i < len; i++)
> if (asmc_read(sc, ASMC_DATA, &buf[i]))
> return 1;
> - for (i = 0; i < ASMC_MAXLEN; i++) /* sanity flush */
> - if (asmc_read(sc, ASMC_DATA, &n))
> - break;
> } else if (cmd == ASMC_WRITE) {
> for (i = 0; i < len; i++)
> if (asmc_write(sc, ASMC_DATA, buf[i]))
> @@ -515,9 +526,7 @@ asmc_try(struct asmc_softc *sc, int cmd,
> if (r && (s = asmc_status(sc)))
> r = s;
> rw_exit_write(&sc->sc_lock);
> - if (sc->sc_sensor_task) /* give kbdled a chance to grab the lock */
> - tsleep(&sc->sc_taskq, 0, "asmc",
> - (1 * hz + 999999) / 1000000 + 1);
> +
> return r;
> }
>
>
>
>
>
>
