Date: Wed, 7 May 2014 02:26:20 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Matthew Fleming <mdf@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Aleksandr Rybalko <ray@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org> Subject: Re: svn commit: r265442 - head/sys/dev/vt Message-ID: <20140507001943.F3578@besplex.bde.org> In-Reply-To: <CAMBSHm-z_yF5JLfF8U6r=z8xQGrLDVqwnAAjA3k7%2BB2X%2BZ=KSw@mail.gmail.com> References: <201405061352.s46DqE9a025250@svn.freebsd.org> <CAMBSHm-z_yF5JLfF8U6r=z8xQGrLDVqwnAAjA3k7%2BB2X%2BZ=KSw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 6 May 2014, Matthew Fleming wrote:
> On Tue, May 6, 2014 at 6:52 AM, Aleksandr Rybalko <ray@freebsd.org> wrote:
>> Log:
>> Implement KDMKTONE ioctl.
Does it have to have to be even worse than syscons?
>> Modified: head/sys/dev/vt/vt_core.c
>> ==============================================================================
>> --- head/sys/dev/vt/vt_core.c Tue May 6 13:46:36 2014 (r265441)
>> +++ head/sys/dev/vt/vt_core.c Tue May 6 13:52:13 2014 (r265442)
>> @@ -1732,9 +1732,17 @@ skip_thunk:
>> td->td_frame->tf_rflags &= ~PSL_IOPL;
>> #endif
>> return (0);
>> - case KDMKTONE: /* sound the bell */
>> - /* TODO */
>> + case KDMKTONE: { /* sound the bell */
>> + int freq, period;
Style bugs: nested declarations, and misindented braces to make the rest
not too ugly without using C++ declarations). Syscons doesn't use any
local variables here.
>> +
>> + freq = 1193182 / ((*(int*)data) & 0xffff);
This is a period in nominal x86 i8254 timer cycles, not a frequency.
1193182 would be a frequency. This is very confusing. The confusion
is reduced a little in kbdcontrol and syscons by spelling the variable
that is spelled 'period' here as 'duration'.
The current bugs caused by this confusion seem to be:
- the API has been broken. The original API (from SCO?) apparently has
units of x86 timer cycles at the nominal frequency for (*(int*)data).
- inverting the units fixed broken cases but broke working cases. Broken
cases included:
- the default "pitch" of 800 Hz was actually a period of 800 x86 timer
cycles. Its frequency was actually 1193182 / 800 = 1493 Hz. This
affected the default in kbdcontrol and the kernel.
Working cases included anything that conformed to the API:
- kbdcontrol -b 1.800 used to give 800 Hz. Now it gives 800 x86 timer
cycles or 1493 Hz after a double inversion.
The inversion bug is more obvious at frequencies far away from
sqrt(1193182) = 1092 Hz. 800 is only moderately far away.
- variable names are bad. 'pitch' is used for both frequencies and periods.
Variable names were not changed to match inversion of the API, though
sometimes the inversion made the variable name not so bad.
syscons is better layered here. It passes the undivided "pitch"
((*(int*)data) & 0xffff) to sc_bell(). sc_bell() implements visual
bell, which I use. Even KDMKTONE gets turned into visual bell. OTOH,
KDMKSOUND makes a "tone", which is actually always a sound.
Note that the hard-coded frequency 1193182 is almost correct, although
this is x86-specific and even x86 has a variable for the timer
frequency. It is part of the non-broken user API. It is sysbeep()'s
resposibility to convert from nominal x86 cycles to the actual hardware.
This is easier to fix with the magic number not exposed to userland.
Even x86 sysbeep() never bothered to do the conversion. Conversion
on x86 would only give a fix of a few ppm except on exotic hardware.
> This data comes from a user and can't be trusted. This is a potential
> divide-by-zero.
This bug has been implemented and executed before. It was in at least
the alpha version. The alpha sysbeep() did an extra inversion, so the
broken cases were reversed relative to the old i386 version, as above.
Better yet, the inversion implemented the divide-by-zero panic, as above.
I may misremember, but in unquoted parts of the diff I saw a check for
the zero-divisor case. This check is necessary to match the API. 0
is an out-of-band value that must be translated to a default value.
>
>> + period = (((*(int*)data)>>16) & 0xffff) * hz / 1000;
> This is signed shift which I can't recall if it's well-defined if the
> number is negative. Using u_int would definitely be defined.
Syscons does the same thing here. The behaviour is implementation-
defined IIRC. In practice, negative values for the duration give a
garbage result that is very far from negative. E.g., a duration of
-1 gives 0xffff after shifting. This is independent of whether the
sign bit gets shifted since the high bits are masked off. But the
result is garbage -- masking makes it positive. Then scaling by hz /
1000 makes 0xffff as large as possible for a duration instead of
negative. Not a problem.
You can get worse problems from physically impossible frequencies like
1 Hz and nearly-infinite Hz. Not 0 or infinite Hz since 0 is translated
or 1193182/0 is avoided in non-buggy versions. 1 Hz is physically
impossible and not a problem. Nearly-infinite Hz (a period of 0 or 1
timer cycles) might damage the speaker, but in practice the speaker
just can't keep up. I think it averages out to not doing anything.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140507001943.F3578>