device speaker

[Bruce Evans]

On Thu, 9 Jun 2005, Ruslan Ermilov wrote:

> On Wed, Jun 08, 2005 at 10:59:22PM -0700, John Baldwin wrote:
>> On Jun 7, 2005, at 12:22 AM, Ruslan Ermilov wrote:
>>> Fine.  I'd like to repo-copy sys/i386/isa/spkr.c (and its header)
>>> somewhere to sys/x86/ once the latter is ready.  Do you have any
>>> estimation when one gets ready?
>>
>> The correct thing to do is to go ahead and make a sys/x86/x86 to stick
>> it in (I'm not sure it really belongs in an 'isa' subdirectory).  The
>> sys/x86 tree will probably grow as people start slowly moving things
>> that are identical between i386 and amd64 over to it.
>>
> It sure is an ISA device, so I was thinking about sys/x86/isa/ or
> sys/x86/dev/spkr/.  Which one do you like more?

He should like neither.  The only x86 dependency of the spkr device is
its implementation.  Its hardware implementation just needs an 8254 and
an 8255 wired up to a speaker in much the same way as the original IBM
PC, and its software implementation currently has PIO hard-coded.  Some
alphas apparently have identical hardware, since alpha/isa/clock.c uses
the same code (modulo bugs (*)) as i386/isa/clock.c to beep the speaker.
The spkr device should just work on these alphas.

(*) Bugs in sysbeep():

i386 version:
(1) Broken locking.
(2) Broken as designed interface:
     The "pitch" (frequency) is actually the period in cycles at the
     frequency of the i8253 on the original IBM PC.  Non-broken callers
     invert the pitch to give a period under the assumption that the
     i8254 freqency is the same as on the IBM PC's i8253 frequency.
     This interface is broken as designed, but some callers understand
     it.  sysbeep() should scale the period to match the actual frequency.

alpha version:
(2a) Gets the broken as designed interface backwards by inverting the
     "pitch"

Diff of the sysbeep() part of alpha/isa/clock.c with i386/isa/clock.c
(a full diff didn't produce useful context due to gratuitous differences
in the files):

% --- 0	Fri Jun 10 09:38:29 2005
% +++ 1	Fri Jun 10 09:38:58 2005
% @@ -2,24 +2,13 @@
%  sysbeep(int pitch, int period)
%  {
% -	/*
% -	 * XXX: TurboLaser doesn't have an i8254 counter.
% -	 * XXX: A replacement is needed, and another method
% -	 * XXX: of determining this would be nice.
% -	 */
% -	if (hwrpb->rpb_type == ST_DEC_21000) {
% -		return (0);
% -	}
% -
% -	mtx_lock_spin(&clock_lock);
% +	int x = splclock();

Here is the start of the broken locking for i386's.  splclock() used
to work but is now null.  clock_lock is acquired later for i386's, but
there is nothing except Giant to lock the call to acquire_timer2()
(this function requires callers to lock for it and the lock should
strictly be at least clock_lock since an access to the shared TIMER_MODE
register is made).  Locking everything by abusing clock_lock for the
PPI would work but isn't done in either version.  In practice, the PPI
is probably locked by Giant.

Locking is similarly broken in spkr.c.

% 
%  	if (acquire_timer2(TIMER_SQWAVE|TIMER_16BIT))
%  		if (!beeping) {
%  			/* Something else owns it. */
% -			mtx_unlock_spin(&clock_lock);
% +			splx(x);
%  			return (-1); /* XXX Should be EBUSY, but nobody cares anyway. */
%  		}
% -
% -	if (pitch) pitch = TIMER_DIV(pitch);
% -

Here is the inversion of the "pitch" on alpha's.  The TIMER_DIV() used to
cause a trap for division by zero when users asked for an impossibly
high frequency and callers correctly inverted the frequency to give a
not so correct period of 0.  Then the code was unimproved further by
only avoiding the division for the trapping case, complete with
formatting and logic style bugs.

% +	mtx_lock_spin(&clock_lock);
%  	outb(TIMER_CNTR2, pitch);
%  	outb(TIMER_CNTR2, (pitch>>8));

This is a perfectly illogical place to acquire clock_lock (on i386's).
We didn't acquire clock_lock for the call to acquire_timer2() which needs
some locking, but we acquire it here when no more locking is needed since
acquire_timer2() gave us a long-lived exlusive lock.  (It happens that
exclusivity gives us safe access to TIMER_CNTR2, since although the
i8254 has a common control/status register, it has separate counter
registers and we have exclusive access to the only register that we 
access diectly.

% @@ -27,8 +16,9 @@
%  	if (!beeping) {
%  		/* enable counter2 output to speaker */
% -		if (pitch) outb(IO_PPI, inb(IO_PPI) | 3);
% +		outb(IO_PPI, inb(IO_PPI) | 3);

This is the final part of the unimproved alpha code to avoid division
by zero.  Here "pitch" is the period in i8254 cycles and is not subject
to further bogus inversions.  A period of 0 is non-physical but is no
worse than a period of 1 -- even speakers of quality thousands of times
better than a PC speaker can't usefully oscillate at > 1MHz.  The
hardware doesn't seem to care whether we write 0, 1 or any other
preposterously small value for the period, so there is little reason
to handle the (pitch == 0) case specially (with 2 style bugs).  (There
is a tiny reason: a "pitch" of 0 can result in 2 ways: the original arg
may be 0, or we may convert an very large arg to 0 when we bogusly
invert the arg; if the inversion were non-bogus, then we should keep
track of the original arg so as to special-case only the case where
we didn't invert it.)

%  		beeping = period;
%  		timeout(sysbeepstop, (void *)NULL, period);
%  	}
% +	splx(x);
%  	return (0);
%  }

Bruce