svn commit: r265442 - head/sys/dev/vt

[Aleksandr Rybalko]

On Wed, 7 May 2014 02:26:20 +1000 (EST)
Bruce Evans <brde at optusnet.com.au> wrote:

> On Tue, 6 May 2014, Matthew Fleming wrote:
> 
> > On Tue, May 6, 2014 at 6:52 AM, Aleksandr Rybalko <ray at 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

Hi Matthew! Hi Bruce!

Thank you very much for pointing that.
And sorry for my mistakes :)
It looks like fixed in r265546.

P.S. Bruce, I very like your explanations (all, not only this one), but
sometime I got tired before I done with reading. Can you please be
a bit less verbose sometime? :)

Thanks a lot!

WBW
-- 
Aleksandr Rybalko <ray at freebsd.org>