Date: Wed, 7 May 2014 16:59:27 +0300 From: Aleksandr Rybalko <ray@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Matthew Fleming <mdf@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: <20140507165927.ede049ac7a79b0c19c6d95bf@freebsd.org> In-Reply-To: <20140507001943.F3578@besplex.bde.org> References: <201405061352.s46DqE9a025250@svn.freebsd.org> <CAMBSHm-z_yF5JLfF8U6r=z8xQGrLDVqwnAAjA3k7%2BB2X%2BZ=KSw@mail.gmail.com> <20140507001943.F3578@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 7 May 2014 02:26:20 +1000 (EST)
Bruce Evans <brde@optusnet.com.au> wrote:
> 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
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@freebsd.org>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140507165927.ede049ac7a79b0c19c6d95bf>