From owner-svn-src-all@FreeBSD.ORG Wed May 7 13:59:29 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 6F541599; Wed, 7 May 2014 13:59:29 +0000 (UTC) Received: from smtp.dlink.ua (smtp.dlink.ua [193.138.187.146]) by mx1.freebsd.org (Postfix) with ESMTP id AD701BB0; Wed, 7 May 2014 13:59:28 +0000 (UTC) Received: from terran (unknown [192.168.99.1]) (Authenticated sender: ray) by smtp.dlink.ua (Postfix) with ESMTPA id 50454C4935; Wed, 7 May 2014 16:59:27 +0300 (EEST) Date: Wed, 7 May 2014 16:59:27 +0300 From: Aleksandr Rybalko To: Bruce Evans 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> <20140507001943.F3578@besplex.bde.org> X-Mailer: Sylpheed 3.3.1 (GTK+ 2.24.22; amd64-portbld-freebsd9.1) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: "svn-src-head@freebsd.org" , Matthew Fleming , Aleksandr Rybalko , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 May 2014 13:59:29 -0000 On Wed, 7 May 2014 02:26:20 +1000 (EST) Bruce Evans wrote: > On Tue, 6 May 2014, Matthew Fleming wrote: > > > On Tue, May 6, 2014 at 6:52 AM, Aleksandr Rybalko 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