From owner-svn-src-all@FreeBSD.ORG Tue May 6 16:50:13 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 37A50EFA; Tue, 6 May 2014 16:50:13 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id D51C292C; Tue, 6 May 2014 16:50:12 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id E39D8781369; Wed, 7 May 2014 02:26:21 +1000 (EST) Date: Wed, 7 May 2014 02:26:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Matthew Fleming Subject: Re: svn commit: r265442 - head/sys/dev/vt In-Reply-To: Message-ID: <20140507001943.F3578@besplex.bde.org> References: <201405061352.s46DqE9a025250@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=U6SrU4bu c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=SRubDjJBntYA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=6I5d2MoRAAAA:8 a=H5CCPXp9d4Vo8zz979kA:9 a=gKYRMQd8X2P9cKUT:21 a=e3IfHBkTZ2E1auAg:21 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 Cc: "svn-src-head@freebsd.org" , 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: Tue, 06 May 2014 16:50:13 -0000 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