From owner-freebsd-acpi@FreeBSD.ORG Sun Aug 17 12:31:59 2014 Return-Path: Delivered-To: freebsd-acpi@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 B5AE41C2 for ; Sun, 17 Aug 2014 12:31:59 +0000 (UTC) Received: from mail.metricspace.net (mail.metricspace.net [IPv6:2001:470:1f11:617::103]) by mx1.freebsd.org (Postfix) with ESMTP id 7F1F8224F for ; Sun, 17 Aug 2014 12:31:59 +0000 (UTC) Received: from [192.168.42.211] (unknown [172.16.2.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: eric) by mail.metricspace.net (Postfix) with ESMTPSA id 5435955FB2 for ; Sun, 17 Aug 2014 12:31:58 +0000 (UTC) Message-ID: <53F0A0BD.5060508@metricspace.net> Date: Sun, 17 Aug 2014 08:31:57 -0400 From: Eric McCorkle User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: freebsd-acpi@freebsd.org Subject: Re: ACPI error messages on Lenovo W540 References: <53A048B1.1080108@metricspace.net> <201406230953.16496.jhb@freebsd.org> <53A8AD54.8040908@metricspace.net> <201406241000.35812.jhb@freebsd.org> <53AA1D05.6000304@metricspace.net> In-Reply-To: <53AA1D05.6000304@metricspace.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Aug 2014 12:31:59 -0000 Finally got time to do some more poking around regarding this. I haven't read the entire ACPI spec, so bear with me... As John Baldwin said, the wrapper nvidia_acpi.c passes in a buffer instead of a package. In the definition for _DSM, you can see calls to NVOP, NVPS, and NBCI. I looked at all three, and they seem to treat Arg3 as a buffer as well (they call CreateField on it, which according to the ACPI spec, should take a buffer argument). So it looks like both the nvidia driver as well as the ALS code are pretty thoroughly convinced that Arg3 (the fourth arg) is a buffer instead of a package, despite what the spec says about what it "should" be. Could this possibly be fixed by adding some kind of quirk to the ACPI execution engine that says "_DSM's fourth argument is a buffer"? On 06/24/2014 20:51, Eric McCorkle wrote: > > It looks like this is the definition: > > Method (_DSM, 4, NotSerialized) // _DSM: > Device-Specific Method > { > If (\CMPB (Arg0, Buffer (0x10) > { > /* 0000 */ 0xF8, 0xD8, 0x86, > 0xA4, 0xDA, 0x0B, 0x1B, 0x47, > /* 0008 */ 0xA7, 0x2B, 0x60, > 0x42, 0xA6, 0xB5, 0xBE, 0xE0 > })) > { > Return (NVOP (Arg0, Arg1, Arg2, Arg3)) > } > > If (\CMPB (Arg0, Buffer (0x10) > { > /* 0000 */ 0x01, 0x2D, 0x13, > 0xA3, 0xDA, 0x8C, 0xBA, 0x49, > /* 0008 */ 0xA5, 0x2E, 0xBC, > 0x9D, 0x46, 0xDF, 0x6B, 0x81 > })) > { > Return (NVPS (Arg0, Arg1, Arg2, Arg3)) > } > > If (\WIN8) > { > If (\CMPB (Arg0, Buffer (0x10) > { > /* 0000 */ 0x75, 0x0B, 0xA5, > 0xD4, 0xC7, 0x65, 0xF7, 0x46, > /* 0008 */ 0xBF, 0xB7, 0x41, > 0x51, 0x4C, 0xEA, 0x02, 0x44 > })) > { > Return (NBCI (Arg0, Arg1, Arg2, Arg3)) > } > } > > Return (Buffer (0x04) > { > 0x01, 0x00, 0x00, 0x80 > }) > } > > Not sure if that's helpful at all... > >> Ah, the nvidia driver calls _DSM and it has the bug. In its >> nvidia_acpi.c file >> it uses a Buffer instead of a Package for the fourth argument to >> _DSM. OTOH, the >> warning doesn't prevent the method from running, so the warning may be >> harmless. >> You can try contacting the nvidia folks to tell them about the warning >> and point >> out the bug in their _DSM wrapper in nvidia_acpi.c to see what they say. > > Will do. Is there a preferred point of contact? From owner-freebsd-acpi@FreeBSD.ORG Sun Aug 17 15:17:23 2014 Return-Path: Delivered-To: freebsd-acpi@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 8BB90EC7 for ; Sun, 17 Aug 2014 15:17:23 +0000 (UTC) Received: from mail.metricspace.net (mail.metricspace.net [IPv6:2001:470:1f11:617::103]) by mx1.freebsd.org (Postfix) with ESMTP id 5509C228F for ; Sun, 17 Aug 2014 15:17:23 +0000 (UTC) Received: from [192.168.42.206] (unknown [172.16.2.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: eric) by mail.metricspace.net (Postfix) with ESMTPSA id 9066F43635 for ; Sun, 17 Aug 2014 15:17:15 +0000 (UTC) Message-ID: <53F0C77A.5050103@metricspace.net> Date: Sun, 17 Aug 2014 11:17:14 -0400 From: Eric McCorkle User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: freebsd-acpi@freebsd.org Subject: Re: ACPI error messages on Lenovo W540 References: <53A048B1.1080108@metricspace.net> <201406230953.16496.jhb@freebsd.org> <53A8AD54.8040908@metricspace.net> <201406241000.35812.jhb@freebsd.org> <53AA1D05.6000304@metricspace.net> <53F0A0BD.5060508@metricspace.net> In-Reply-To: <53F0A0BD.5060508@metricspace.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Aug 2014 15:17:23 -0000 Actually, I managed to get a kernel panic to happen, which provides some more information: NVRM: GPU at 0000:01:00.0 has fallen off the bus. NVRM: RmInitAdapter failed! (0x25:0x28:1169) nvidia0: NVRM: rm_init_adapter() failed! Fatal trap 12: page fault while in kernel mode cpuid = 2; apic id = 02 fault virtual address = 0x8 fault code = supervisor read data, page not present instruction pointer = 0x20:0xffffffff817eeeb5 stack pointer = 0x28:0xfffffe021d481420 frame pointer = 0x28:0xfffffe021d481500 code segment = base 0x0, limit 0xfffff, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags = interrupt enabled, resume, IOPL = 3 current process = 73254 (Xorg) trap number = 12 panic: page fault cpuid = 2 Uptime: 12h17m8s It looks like rm_init_adapter is contained in nvidia's .o file that comes with the driver, so I'm out of luck for trying to track it down. I can't tell whether this is caused by the ACPI errors or not... On 08/17/2014 08:31, Eric McCorkle wrote: > Finally got time to do some more poking around regarding this. I > haven't read the entire ACPI spec, so bear with me... > > As John Baldwin said, the wrapper nvidia_acpi.c passes in a buffer > instead of a package. In the definition for _DSM, you can see calls to > NVOP, NVPS, and NBCI. I looked at all three, and they seem to treat > Arg3 as a buffer as well (they call CreateField on it, which according > to the ACPI spec, should take a buffer argument). > > So it looks like both the nvidia driver as well as the ALS code are > pretty thoroughly convinced that Arg3 (the fourth arg) is a buffer > instead of a package, despite what the spec says about what it "should" be. > > Could this possibly be fixed by adding some kind of quirk to the ACPI > execution engine that says "_DSM's fourth argument is a buffer"? > > On 06/24/2014 20:51, Eric McCorkle wrote: >> >> It looks like this is the definition: >> >> Method (_DSM, 4, NotSerialized) // _DSM: >> Device-Specific Method >> { >> If (\CMPB (Arg0, Buffer (0x10) >> { >> /* 0000 */ 0xF8, 0xD8, 0x86, >> 0xA4, 0xDA, 0x0B, 0x1B, 0x47, >> /* 0008 */ 0xA7, 0x2B, 0x60, >> 0x42, 0xA6, 0xB5, 0xBE, 0xE0 >> })) >> { >> Return (NVOP (Arg0, Arg1, Arg2, Arg3)) >> } >> >> If (\CMPB (Arg0, Buffer (0x10) >> { >> /* 0000 */ 0x01, 0x2D, 0x13, >> 0xA3, 0xDA, 0x8C, 0xBA, 0x49, >> /* 0008 */ 0xA5, 0x2E, 0xBC, >> 0x9D, 0x46, 0xDF, 0x6B, 0x81 >> })) >> { >> Return (NVPS (Arg0, Arg1, Arg2, Arg3)) >> } >> >> If (\WIN8) >> { >> If (\CMPB (Arg0, Buffer (0x10) >> { >> /* 0000 */ 0x75, 0x0B, 0xA5, >> 0xD4, 0xC7, 0x65, 0xF7, 0x46, >> /* 0008 */ 0xBF, 0xB7, 0x41, >> 0x51, 0x4C, 0xEA, 0x02, 0x44 >> })) >> { >> Return (NBCI (Arg0, Arg1, Arg2, Arg3)) >> } >> } >> >> Return (Buffer (0x04) >> { >> 0x01, 0x00, 0x00, 0x80 >> }) >> } >> >> Not sure if that's helpful at all... >> >>> Ah, the nvidia driver calls _DSM and it has the bug. In its >>> nvidia_acpi.c file >>> it uses a Buffer instead of a Package for the fourth argument to >>> _DSM. OTOH, the >>> warning doesn't prevent the method from running, so the warning may be >>> harmless. >>> You can try contacting the nvidia folks to tell them about the warning >>> and point >>> out the bug in their _DSM wrapper in nvidia_acpi.c to see what they say. >> >> Will do. Is there a preferred point of contact? > _______________________________________________ > freebsd-acpi@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-acpi > To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@freebsd.org" From owner-freebsd-acpi@FreeBSD.ORG Sun Aug 17 15:29:51 2014 Return-Path: Delivered-To: freebsd-acpi@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 9FCC73F2 for ; Sun, 17 Aug 2014 15:29:51 +0000 (UTC) Received: from nm16-vm7.access.bullet.mail.gq1.yahoo.com (nm16-vm7.access.bullet.mail.gq1.yahoo.com [216.39.63.194]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5FF182460 for ; Sun, 17 Aug 2014 15:29:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s2048; t=1408289216; bh=Tp/3RTvrOA9rpYNJuXojgtFHA8q21ZithA6+wX/VgJE=; h=Received:Received:Received:DKIM-Signature:X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Content-Transfer-Encoding; b=Cu9JMiP2UBtuSQA32Iso6mtfSW1dpgzHDSTVZK1gqLrimVRmLXGYkc3Sj9yoY6Ne0v2rPvjqF6ERixgYW7mymvnUCD/TPDk1F2iGpRI1AeNOScVcm93wAIkHkjWjrAH1N5/+CQwasI1eTa6ozWj3aHoCez8Pzclf6k7RaK1xd1Cn51jYhniD7VJYlDaANFasPYuJSH06Zxa+mET2WYp7/oEG798pRHi+HpTxaUyFQBmCedA78H++csopuImi4D4gaKiM1QP4EXZxxLZTN13Ww4P0vwfFgE905pnp0My+RGSLZDW8xaAVwnWZHJLga9/YLlS7OHXMdTDtP7Hwx7mVUA== DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s2048; d=att.net; b=WAjfqrPfR0IU91IPosjrLqSDcZsafLCNcmCJxVHHj072uw4XPuI8+giIbxU21vfKEIVzMeop2yNrDsC4kYpSwlNkllOq26kM6wW1o8yFFYjpKTcp36Wo3s95NOmMiI0UwJHiStFJOIrZEtpqJOYhwCC/Rod29cETQYch6fow/t27rGE+jlk7S8wW/UB8aHDrnj5XC0ZA+XAmdDD+BbiYzRVU9rDgNfTNzy1D5kYuuce8DYxTp0cHrB0GI6RdE0S7vrAaXZmU/Gp4T+4iMWxSIT0ml91GLEIar4Jm+nohOI1nKFPUOrS39PJiq6ThopKqqO8swyEMmDh4ohDq78FNjQ==; Received: from [216.39.60.170] by nm16.access.bullet.mail.gq1.yahoo.com with NNFMP; 17 Aug 2014 15:26:56 -0000 Received: from [98.138.104.98] by tm6.access.bullet.mail.gq1.yahoo.com with NNFMP; 17 Aug 2014 15:26:56 -0000 Received: from [127.0.0.1] by smtp118.sbc.mail.ne1.yahoo.com with NNFMP; 17 Aug 2014 15:26:56 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1408289216; bh=Tp/3RTvrOA9rpYNJuXojgtFHA8q21ZithA6+wX/VgJE=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Content-Transfer-Encoding; b=XvzzulVnWSQ4WgTdGLPPnt/qeh6fWdV0k0/X90M5Vu1XXjZzVq7ax8JCP337SeTG72s+txd2qPKz9kn5D0z8YJ4Z16XbQr4JNf33clHhCXDnmELI4WxGPohbPEaZz0ll3ZDIBHJ8gag5LcUEWTWI1TAS9Vg0VhcrAKuX5L8mlNw= X-Yahoo-Newman-Id: 637722.38537.bm@smtp118.sbc.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 33sb8ywVM1kvefhCVPpJk4rGH8pGEqrCrejQNcWFh_ozk9M WdwZsKqOD8cC6dRhG.iD7z4MU_I4v9ajKoJNVV0.vh5V514kfkFPXMAGYt1P YO_EDE8w.e35gahmud3aXFQ4UquW2dPiZAQ7TSrNDCSX0rlrPj9qFAh9YTIu w8YUmFrjTcMcIo00deunIRRo7OeK28hzb6IFujQVzr6i5fR52TXXp_0WCPx6 BeJzQ6ddUWwk_HYLDF05xUqm_bXrSPZ.2bwh8XyLKQ193UC0TwBuiHQIfd.j iMT2IME0hVsv2QKl_scEXeK7yVM1F1jUsbioDFFayPININWKg6pFu1cSlsIs HOdPUYkOPZCYy0f1QDD.TFwf1jHg.L8rgcLMAyeWisp25_x9LJlu4hiAbNsn 1FQDCXKgIggxUFd.CEDO61_z_1dadKFbowHZ4YLNgErLFT0S6vw3PWAHPHUk w8bDa2V7N5U2OL1aU6fIbgZlXfhmh67v9TxZrBfONRwEJ.4k5xpeemc2C2ge uBDPtFQ1Fn1qCq0aPywC_.uyMs3wt9z47c0QngzcgeG6XISCH85NQ5rMEqz3 DGo4bn_Lh2lEsMyUM6T5kPDGSmgqWCqPs8JBPtA-- X-Yahoo-SMTP: OKD1keCswBBTAmAF1s00hLyKW3wE3YfSK0Eazl6b4VZG4LTqJxg- Message-ID: <53F0C9BF.9060405@att.net> Date: Sun, 17 Aug 2014 11:26:55 -0400 From: Anthony Jenkins User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Bruce Evans , Ian Smith Subject: Re: [PATCH] ACPI CMOS region support - submission? References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> <20140806040804.P849@besplex.bde.org> In-Reply-To: <20140806040804.P849@besplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Aug 2014 15:29:51 -0000 Thanks for the critique guys, comments inline. Sorry it took so long... On 08/05/2014 16:49, Bruce Evans wrote: > On Tue, 5 Aug 2014, Ian Smith wrote: > >> On Sat, 2 Aug 2014 18:10:05 -0400, Anthony Jenkins wrote: >> >> > I made a few minor changes since the last incarnation: >> > >> > - Defined the CMOS address/data register addresses as macros >> > - Defined the (apparent) I/O delay as a macro >> >> Looks good, Anthony. >> >> > I also verified the ACPI CMOS region code only accesses up to >> > register 63 (0x3F - in previous emails I mistakenly said 0x7F). >> >> Is 0x3f what the ACPI spec limits ACPI access to? >> >> This is mildly surprising, as all modern RTC chips are at least 128 >> bytes; noone's actually used a MC146818 in over ten years, I gather. > > BIOSes at least used to use much more. It is good for ACPI to not allow > clobbering the BIOS state. > >> > If/when this gets in, I'd like to add sysctl controls to e.g. allow >> > ACPI access to the date/time registers (I currently return failure >> > when attempting to write them via ACPI). I don't see anything in the >> > spec (after re-reading it) that disallows ACPI from touching those. >> >> I don't know about the spec, but I can't see allowing ACPI write access >> to time/alarm/date registers as being a sensible idea; this could allow >> completely out-of-band modification of time/alarm/date regs without the >> necessary precautions needed for setting these, namely use of the SET >> bit in status reg B (0x0b) to disable updates while setting time & date, >> and anyway why allow changing time/date without the OS knowing about it? >> Reading should be no problem, though without proper observance of UIP >> bit timing, data may not be consistent. > > They might need to be read on resume. I can't see how resume can possibly > work without reading them or some clock to reset the time. But this should > be done by calling the standard time reading functions, not at a low level. > >> I guess there may be a case for messing with the alarm bytes, though we >> don't currently allow use of the alarm for wake from poweroff (S5) or >> STR (S3) states, as doth Linux. I looked into this some years ago when >> there were a few requests for the feature, however it would involve some >> major reworking to always preserve the alarm interrupt bit through init >> and suspend/resume, and appropriate clearing of same after use, along >> with a utility to setup the alarm .. a potential to leave open, perhaps? > > I use the seconds alarm for pps, but don't use suspend. > >> Which leads to my other concern here: that you are allowing out-of-band >> access to especially status reg C (0x0c), which when read clears all >> enabled interrupts, and the other status regs whose settings are also >> too important to allow screwing with. > > Yes, reading it would break interrupt handling. > >> We used to use the RTC periodic interrupt as a clock source for a 128Hz >> interrupt (1024Hz while profiling) but since mav@'s reworking of all the >> clocks and timers sometime prior to 9.0-REL (IIRC), that's now rarely if >> ever used as such, but remains an available clock or timer source. >> >> Reg A (0x0a) is r/w and sets clock divider and rate selection bits. Do >> we want ACPI to be able to mess with these? I think not. Readonly, ok. >> >> Reg B (0x0b) is r/w and contains the aforementioned SET bit, the three >> interrupt enable bits (PIE, AIE, UIE), the SQWE, DM, 24/12 and DSE bits, >> again none of which should be allowed to be written to out-of-band. >> >> And reg C (0x0c) is read-only, and as mentioned clears interrupts; again >> not something that should be permitted without the OS knowing about it. >> >> I suppose you have the MC146818 spec to hand? >> >> Couple of things from your patch: >> >> -static int rtc_reg = -1; >> >> Indeed; I never could figure out the advantage in this, as how rarely >> would you want to read or write the same reg twice in a row anyway? > > About 99.9999% of accesses are to the status register for handling RTC > interrupts. The RTC is almost never used except for the 128Hz interrupt, > so the index register is normally constant at the single value used by > this interrupt (RTC_INTR). With mav's changes, it is now rarely used. > > Removing this is a good re-pessimization. rtcin() does 4 i/o's with the > pessimization. This takes about 5 usec. At 128 Hz this only wastes > 0.064% of a CPU. At 1024 Hz it wastes 0.512%. The non-pessimized > version only does 1 i/o so it wastes 1/4 as much. The wastage should > be much larger. 1024 Hz is far too slow for profiling a modern system. > It is about right for a 4.77 MHz 8086, except the RTC overhead is too > high for that (I used the i8254). Scaling this for a 2 GHz modern CPU > gives 429 kHz (or more, since instructions per cycle is more). The > overhead for that with the pessimization would be 214% of a CPU. > Without the pessimization, it would only be 53.7% of a CPU. Still too > much. The RTC is just unsuitable for profiling interrupts at a useful > rate. The i8254 is better and the lapic timer is much better, but > profhz has only been increased to 8192. Other things don't scale so > well. There are other i/o's for interrupts that make it difficult > to go much higher than 429 KHz though 429 Khz is certainly possible > using less than 100% of a CPU. > >> static u_char rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF; >> static u_char rtc_statusb = RTCSB_24HR; >> >> +static void acpi_cmos_read(ACPI_PHYSICAL_ADDRESS address, UINT8 *buf, UINT32 buflen) >> { >> + UINT32 offset; > > Please no Windows typedefs in non-Windows code. Ya need to complain to the folks at acpica.org, who define the types in the region access handler functions: sys/contrib/dev/acpica/include/actypes.h: /* Address Spaces (For Operation Regions) */ typedef ACPI_STATUS (*ACPI_ADR_SPACE_HANDLER) ( UINT32 Function, ACPI_PHYSICAL_ADDRESS Address, UINT32 BitWidth, UINT64 *Value, void *HandlerContext, void *RegionContext); > It is a minor layering violation to have acpi functions in non-acpi drivers. > At least use the driver's spellings and not the Windows ones in drivers. I suppose I could make an atrtc_acpi.c or something and put them there... >> RTC_LOCK; >> >> + for (offset = 0U; offset < buflen; ++offset) { >> + IO_DELAY(); >> + outb(IO_RTC_ADDR, ((address + offset) | 0x80) & 0xFF); >> + IO_DELAY(); >> + buf[offset] = inb(IO_RTC_DATA); > > This should just call rtcin() in a loop instead of breaking it. Then > there would be no need to put this in the driver. I had to refactor rtcin() and writertc() because the ACPI read/write functions may be multibyte and the original functions LOCK(), read/write 1 byte, UNLOCK(). It's ACPI's variable length region handler reads/writes that prompted this change. >> Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ? > > 0x80 disables NMI on some systems (mostly old ones?). Old books (Van > Gilluwe) say to use it. FreeBSD never bothered with it. Locking makes > it unecessary, modulo likely bugs in the NMI handler. > > It is also necessary disable ordinary interrupts and be careful about > reenabling NMIs. FreeBSD's locking disables ordinary interrupts as a > side effect of using spinlocks, except in my version where spinlocks > don't block fast interrupt handlers. The remaining care is just to > turn NMIs back on in the RTC at the end of the loop before unlocking > (normally) reenables ordinary interrupts. This releases any pending > NMIs. > > Spinlocks don't block NMIs, so the NMI handler must be careful not to > block endlessly on a spinlock or call any code that depends on spinlocks > for locking, like the code here. Fast interrupt handlers should be > similarly careful, but aren't except in my version. > > NMIs are rare except for pmc and SMP stopping, and the NMI handler never > bothered about correctness in FreeBSD. isa_nmi() for x86 normally just > prints things (using log()) and returns for the system to panic. > Printing is supposed to work when called in the "any" context, but it > is quite broken now. It has locks galore. When an NMI occurs while > one of these locks is held, then the NMI handler deadlocks on the lock > if it is a (non-recursive) spinlock held by the same CPU. If the lock > is held by another CPU, then there is some chance that the lock will > go away, and the bug is just that the printing is delayed. If the > lock is a recursive spinlock held by the same CPU, then the locking > just doesn't work. The buggy locking in printf() mostly uses non- > recursive spinlocks. > > The sloppiness in the NMI handler is not much of a problem for the RTC. > The NMI handler just doesn't normally go near the RTC code, so it won't > deadlock. Unless there is something like a resettodr() call for shutdown > and panic() stumbles into this. > >> >> +int >> +rtcin(int reg) >> +{ >> + u_char val; >> + >> + acpi_cmos_read(reg & 0xFF, &val, 1); > > rtcin() is pessimized too. > > The pessimization factor must also be increased from 4 to 5 or 6 (1 > more i/o to turn off the NMI disable bit at the end, and probably > another to delay after this). > >> ... >> static int >> +is_datetime_reg(ACPI_PHYSICAL_ADDRESS address) >> +{ >> + return address == 0x00 || >> + address == 0x02 || >> + address == 0x04 || >> + address == 0x04 || >> + (address >= 0x06 && address <= 0x09); >> +} > > Lots of style bugs. The KNF style is: > > > return (address == 0x00 || address == 0x02 || address == 0x04 || > address == 0x04 || (address >= 0x06 && address <= 0x09)); > > This includes: > - silly parentheses around return values > - no splitting of statements after every subexpression to make split > statements even more split > - continuation indent is 4 spaces. No problem... is there a FreeBSD style guide somewhere, or do I just follow "K&R style"? >> I guess that second 0x04 should be 0x06? But why are you limiting to >> even addresses, ie time regs but not the alarm regs? If anything, the >> alarm regs seem more likely something BIOS/AML may? want to know about? Ehhhh... just trying to encapsulate the request that "CMOS writes should not touch certain registers". Stuck that request in a function that would return true if such a register write request was made. It would be helpful to have a concise spec for which registers and what types of accesses should be disallowed by the CMOS region handler... >> +static ACPI_STATUS >> +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address, UINT32 width, >> + UINT64 *value, void *context, void *region_context) > > More style bugs. Now the line splitting doesn't even keep the line > short enough, and the continuation indentation isn't even 1 tab or gnu style > (-lp). > >> +{ >> + struct atrtc_softc *sc; >> + >> + sc = (struct atrtc_softc *)context; >> + if (!value || !sc) >> + return AE_BAD_PARAMETER; >> + if (width > 32 || (width & 0x07) || address >= 64U) >> + return AE_BAD_PARAMETER; > > Missing some Windows bad style (U suffix on 64 but not on 32). Yeah I'm not very consistent with those... >> Ok, address is limited to 0x3f here, though strictly address should be >> less than (64 - width>>3) for multibyte access? I guess non-ACPI access >> via rtcin() and writertc() will still be able to access registers, well, >> from 0 to 0xff .. >> >> + if (function == ACPI_WRITE && >> + (is_datetime_reg(address) || >> + (width > 8 && address <= 0x09))) >> + return AE_BAD_PARAMETER; > > Larger indentation errors than above. The missing silly parentheses on > return values seem to be consistent. > >> >> I'd exclude AT LEAST 0x0a - 0x0d for write, certainly 0x0c from read. >> >> + printf("%s: %-5s%02u address=%04lx value=%08x\n", + __FUNCTION__, >> function == ACPI_READ ? "READ" : "WRITE", + width >> 3, address, >> *((UINT32 *)value)); >> + return AE_OK; >> >> The printf is good to see. Maybe you'd eventually want to limit this to >> when verbose booting is on? It would be useful to see with suspend and >> resume messages, or anywhere else ACPI wanted R/W access to 'CMOS' RAM? > > It's so ugly it is obviously only for debugging :-). > >> I tried hunting down a list of what CMOS locations various systems use, >> and find some anecdotal info but nothing like 'manuf X uses bytes Y-Z'. > > Why do we have to care about acpi doing unsafe accesses? Only the > block access seems dangerous. With scattered accesses like > rtcin(RTC_FOO) using the system (non-acpi) spelling for 'FOO', it is > very easy to see any dangerous ones. rtcin() used to be only used for > memory sizing and in the fd driver (for dubious disk equipment byte > reads; the equipment bytes just tell you about equipment that systems > might have had in 1985, so it is almost useless, while other BIOS bytes > are even more useless since they are not standard). It is now used in: > - nvram driver. This seems to allow anyone with access to the device > to read and/or write the offsets 14-127. So they can manage or > corrupt some BIOS bytes but not the ones used by FreeBSD :-). acpia > should be even more trustable, but might need read access to the > clock bytes. > - fb driver. This reads the video equipment byte. This is so hackish > that RTC_EQUIPMENT is still not defined in rtc.h but is defined here. > EQUIPMENT is the standard name for this byte (see van Gilluwe), but > it is not the only equipment-related byte. There is also the diskette > equipment byte. Many other equipment-related bytes that worked in > 1985 are also not defined in rtc.h. They are also not used in FreeBSD. > - acpi_support/icpi_ibm.c uses a large subset of RTC space. It uses > rtcin(), but has its own defines for 6 RTC registers between 0x64 and > 0x6e (for things like brightness and volume). Obviously very > vendor-specific. > - i386/xen/clock.c is a copy of the old x86 clock.c. It duplicates > almost everything including the badly named readrtc() BCD translation > wrapper, but uses the systtem rtcin() and writertc(). > Write accesses used to be protected by writerc() being static, but this > was broken by exporting it for nvram and xen. > > Bruce > _______________________________________________ > freebsd-acpi@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-acpi > To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@freebsd.org" > From owner-freebsd-acpi@FreeBSD.ORG Fri Aug 22 22:01:05 2014 Return-Path: Delivered-To: freebsd-acpi@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 1F0F5DA7 for ; Fri, 22 Aug 2014 22:01:05 +0000 (UTC) Received: from kenobi.freebsd.org (kenobi.freebsd.org [IPv6:2001:1900:2254:206a::16:76]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 05683389F for ; Fri, 22 Aug 2014 22:01:05 +0000 (UTC) Received: from bugs.freebsd.org ([127.0.1.118]) by kenobi.freebsd.org (8.14.9/8.14.9) with ESMTP id s7MM14hu083934 for ; Fri, 22 Aug 2014 22:01:04 GMT (envelope-from bugzilla-noreply@freebsd.org) From: bugzilla-noreply@freebsd.org To: freebsd-acpi@FreeBSD.org Subject: [Bug 105537] [acpi] problems in acpi on HP Compaq nc6320 Date: Fri, 22 Aug 2014 22:01:04 +0000 X-Bugzilla-Reason: AssignedTo X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: Base System X-Bugzilla-Component: kern X-Bugzilla-Version: unspecified X-Bugzilla-Keywords: X-Bugzilla-Severity: Affects Only Me X-Bugzilla-Who: hiren@FreeBSD.org X-Bugzilla-Status: In Discussion X-Bugzilla-Priority: Normal X-Bugzilla-Assigned-To: freebsd-acpi@FreeBSD.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: assigned_to Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Bugzilla-URL: https://bugs.freebsd.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Aug 2014 22:01:05 -0000 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=105537 Hiren Panchasara changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|hiren@FreeBSD.org |freebsd-acpi@FreeBSD.org --- Comment #6 from Hiren Panchasara --- No time and no hardware. -- You are receiving this mail because: You are the assignee for the bug.