# Upgrading the FreeBSD `speaker` driver

## Summary of the proposal

**Context:** the `speaker` driver (added 1991) is currently marked as
"to be removed" because it uses the global kernel mutex:

``` c
/* sys/dev/speaker/spkr.c, function tone() */

        disable_intr();  // <- acquire GIANT lock
        timer_spkr_setfreq(thz);
        enable_intr();   // <- release GIANT lock
```

**Proposal:** remove the calls to `disable_intr()` and `enable_intr()` to
ensure this driver does not get removed for this specific reason.

As explained below, these two calls are not required for correctness.

## Driver origin

`speaker(4)` provides a character-based interface to play tunes on the
PC speaker. For example:

``` shell
$ echo abc >/dev/speaker
```

This driver was originally written for 386BSD around 1990 by [Eric S.
Raymond](https://en.wikipedia.org/wiki/Eric_S._Raymond) (ESR), and imported into FreeBSD in 1991.

The core part of the driver is the logic to output a tone given a
frequency and duration. In the original version, we see this:

``` c
    // enable tick generation and configure frequency.
    sps = spltty();
    outb(PIT_CTRL, PIT_MODE);          /* prepare timer */
    outb(PIT_COUNT, (unsigned char) divisor);  /* send lo byte */
    outb(PIT_COUNT, (divisor >> 8));   /* send hi byte */
    splx(sps);

    // turn speaker on.
    outb(PPI, inb(PPI) | PPI_SPKR);

    // wait for duration of tone.
    timeout((caddr_t)endtone, (caddr_t)NULL, ticks);
    sleep((caddr_t)endtone, PZERO - 1); // calls endtone() at end of wait.
```

With `endtone()` defined as a function:
``` c
static endtone() {
 wakeup((caddr_t)endtone);
 outb(PPI, inb(PPI) & ~PPI_SPKR);
}
```

The calls to `spltty()` and `splx()` at the time were responsible for
defining a critical section around the two `outb()` calls that
configure the frequency.

In 1993 (commit `1e064bd4be79c7e4bbe6465ea4227df36dee83f2`), Andrey A.
Chernov (ache@FreeBSD.org) replaced the timeout/sleep pair by tsleep,
without changes to the rest of the logic:

```
    // configure frequency.
    sps = spltty();
    outb(...); outb(...); outb(...);
    splx(sps);
	// turn speaker on.
	outb(...)

    // wait duration of note.
	// New: endtone becomes a global variable.
	while ((error = tsleep((caddr_t)&endtone,
         SPKRPRI | PCATCH, "spkrtone", ticks)) == ERESTART)
         ;

    // turn speaker off.
	outb(...);
```

## Historical milestone: first guard API

The first major relevant change happened in in 1994 when Søren Schmidt
(sos@freebsd.org) factored the functions that program the i8254 chip
and extracted them away into `clock.c`.

There are three relevant commits:

- `5194771d254260e2e8aebb3ac800a37c08acd8ed` - New support for sharing the timers, `acquire_timer` / `release_timer`
- `f1106e3848541849b83146760b1d110f94313580` - Extract timer functions into `clock.c`
- `57d26a14e90f3cf2acea4e815876962a917e822b` /
  `bc4ff6e376e34c3ec3eda821a13191e859bd31d1` - Use new timer API from
  `clock.c`

The code above then became:

``` c
    sps = spltty();
	
	// set up the i8254 to generate COUNTER2 ticks.
    if (acquire_timer2(PIT_MODE)) {
       return;
    }

    // configure the frequency.
    outb(TIMER_CNTR2, (divisor & 0xff));       /* send lo byte */
    outb(TIMER_CNTR2, (divisor >> 8)); /* send hi byte */

    splx(sps);

    // turn speaker on.
    outb(IO_PPI, inb(IO_PPI) | PPI_SPKR);

    // wait duration of note.
    (void) tsleep((caddr_t)&endtone, SPKRPRI | PCATCH, "spkrtn", ticks);
	
	// turn speaker off.
    outb(IO_PPI, inb(IO_PPI) & ~PPI_SPKR);

    // reset the chip configuration.
    release_timer2();
```

This API change introduced acquire/release semantics for the `PIT_CTRL`
I/O register; so that two drivers (eg `syscons` and `speaker` that
simultaneously wanted to use the hardware speaker would not interfere
with each other.

Already here we can sense that the critical section (`spltty`/`splsx` at
the time) started to become less important: the `TIMER_CNTR2` register
is never used by other drivers without first configuring `PIT_CTRL`; so
if all drivers were using the new acquire/release interface, the
remaining critical section was already not necessary any more.

However, asserting this property and simplifying the code at the time
would have required to audit the kernel for all uses of `TIMER_CNTR2`,
as well as blocking other ways to access the I/O register. Given that
`dosemu` was popular at the time, it may not have been easy.

## Historical milestone: narrowing the critical section

In 1996, Joerg Wunsch (joerg@FreeBSD.org) found that the
acquire/release pair and spltty/splsx pair were not interleaved
properly, and corrected them as follows (commit
`f4de22acc79d85693c25cd479cdda94bf7d33401`):

``` c
    // acquire / configure COUNTER2.
    sps = splclock();
    if (acquire_timer2(PIT_MODE)) {
       splx(sps);
	   return;
    }
    splx(sps);

	// configure the frequency.
	disable_intr();
    outb(TIMER_CNTR2, (divisor & 0xff));
    outb(TIMER_CNTR2, (divisor >> 8));
    enable_intr();

    // turn speaker on.
    outb(IO_PPI, inb(IO_PPI) | PPI_SPKR);

    // wait for duration of note.
     if (ticks > 0)
        tsleep((caddr_t)&endtone, SPKRPRI | PCATCH, "spkrtn", ticks);

    // turn off speaker.
    outb(IO_PPI, inb(IO_PPI) & ~PPI_SPKR);

    // release COUNTER2.
    sps = splclock();
    release_timer2();
    splx(sps);
```

The main shift here is the move from `spltty()`, which defines a
critical section for the entire TTY subsystem, to `splclock()` that
only defines it for the clock subsystem.

The salient part of this change here is that now we have two separate
critical sections; one coordinated with `splclock`/`splx` for the
acquire/release API calls; and a separate one with the new
`disable_intr`/`enable_intr` pair to keep the global atomicity on
accesses to `TIMER_CNTR2`. This ways, only two `outb()` calls remain
under the global critical section.

## Historical milestone and Glitch: API lifts and simplifications

The first API change was in 2005, when Yoshihiro Takahashi
(nyan@FreeBSD.org) extracted the raw I/O operations into macros
defined in a header:

``` c
    // acquire / configure COUNTER2.
    sps = splclock();
    if (timer_spkr_acquire()) { // NEW
       splx(sps);
	   return;
    }
    splx(sps);
    ...

	// configure the frequency.
	disable_intr();
    spkr_set_pitch(divisor); // NEW
    enable_intr();

    // turn speaker on.
    ppi_speaker_on(); // NEW

    // wait for duration of note.
    ...

    // turn off speaker.
    ppi_speaker_off(); // NEW

    // release COUNTER2.
    sps = splclock();
    timer_spkr_release(); // NEW
    splx(sps);
```

In 2008, **the main historical glitch happened**.

Poul-Henning Kamp worked to sanitize the APIs around the i8254 chip;
this happened in commits `e46598588587b4897f6604489364f83fffd4d033` &
`93f5134aaf829826dbcbea457bfeb27389761854`.

On the one hand, phk noticed that the COUNTER2 logic is only ever used
with the speaker on. So he moved the `ppi_speaker_on()` and
`ppi_speaker_off()` calls to the acquire/release functions and
simplified `spkr.c` accordingly:

```
    // acquire / configure COUNTER2 *and* turn speaker on.
    sps = splclock();
    if (timer_spkr_acquire()) {
       splx(sps);
	   return;
    }
    splx(sps);

	// configure the frequency
	disable_intr();
	timer_spkr_setfreq(thz); // new name/interface for spkr_set_pitch.
	enable_intr();
	...
    // wait for duration of note.
    ...

    // release COUNTER2 *and* turn speaker off.
    sps = splclock();
    timer_spkr_release();
    splx(sps);
```


However, in that change, phk also moved the definition of `spkr_set_pitch()` from `timmerreg`, where it was defined as

``` c
#define spkr_set_pitch(pitch) \
       do { \
               outb(TIMER_CNTR2, (pitch) & 0xff); \
               outb(TIMER_CNTR2, (pitch) >> 8); \
       } while(0)
```

To `clock.c` as follows:

``` c
void
timer_spkr_setfreq(int freq)
{

       freq = i8254_freq / freq;
       mtx_lock_spin(&clock_lock);
       outb(TIMER_CNTR2, freq & 0xff);
       outb(TIMER_CNTR2, freq >> 8);
       mtx_unlock_spin(&clock_lock);
}
```

What do we see here?

- **phk introduced a mutex around the two outb calls where none was used previously.**
- **however, he did not remove the disable_intr / enable_intr pair in spkr.c.**

The locking in `spkr.c` had become redundant after this commit. Was
this an oversight? This is unclear. The commit message for
`e46598588587b4897f6604489364f83fffd4d033` suggests that phk was not
focused on `spkr.c` specifically and so may not have been on a mission
to simplify it further.

## Further simplifications - but not enough

In 2020, Warner Losh (imp@FreeBSD.org) then pointed out that there's
no other uses of the `timer_spkr_acquire`/`release` functions and so
the `spl...()` calls were not necessary, and then simplified the code further:

```c
    // acquire / configure COUNTER2 *and* turn speaker on.
    if (timer_spkr_acquire()) {
	   return;
    }

	// configure the frequency.
	disable_intr();
	timer_spkr_setfreq(thz);
	enable_intr();

    // wait for duration of note.
    ...

    // release COUNTER2 *and* turn speaker off.
    timer_spkr_release();
```

(commit `1f7705606e8286960a2cea3d514a1deb5339047f`)

However, Warner Losh did not notice either that the `disable_intr` /
`enable_intr` pairs had become unnecessary.

## Conclusion

Since phk's 2008 change, the `disable_intr()` / `enable_intr()` pair
in `spkr.c` have become redundant with other locking mechanisms
elsewhere and can be removed. The `speaker(4)` driver does not need to
be GIANT-locked.