From owner-freebsd-acpi@FreeBSD.ORG Mon Aug 4 13:00: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 28C6DB1D for ; Mon, 4 Aug 2014 13:00:59 +0000 (UTC) Received: from nm27-vm5.access.bullet.mail.bf1.yahoo.com (nm27-vm5.access.bullet.mail.bf1.yahoo.com [216.109.115.228]) (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 A27AA298A for ; Mon, 4 Aug 2014 13:00:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s2048; t=1407156862; bh=BH3T+moZ3m+1ZPJcseS9qa2cI4MkG0b/ST7tBzfy7MI=; 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=aK5QU425X4LxX6B1SBtWN4mjejCuPtm9KpWJqukYNX0KUQZeHlUoJcWeDfVMyAzFLPAhG/OhdeWzs9XoioLbED6A3LhAMtJNRMwOuYuXSgq25jLoQtjtmmkdAV2EAj1SWIM0sm/ywRLkcNREDyPS12fBOn0/X0Ow0c+7byzbALMkmhY2KKoRE68aKLskGFIIijt0gEXvdljjN/5IorSHerg4cHx76W5NyYZAJ43Kd6/zmOEmXm/6ogAVj9RpZFDmX1Om1EGT7JCTZ0RrbQbBu9Ng3jfEVlfmHutmdF8sm68TuFTnlfA6XJWlie/aIRJ6z9YMsYV0P0iaBh4pJ6UFBg== DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s2048; d=att.net; b=MJty9IqQfDuCBqgS+cyEThcU/tEYL2qIKNsBz1SpzUEUaHOj8Gu/SYjnMLz/+Z92Rd1ge7TikNb0nmMrnRG1P1GlSOOYp+bDTOaxctZPPg7ATHOsl2Dj7Fo/a+KgPtgSH9yxHefLpaVMb8Q+cKQjtVtOH1HE8ZCLc52b6+dFPSlPX0VDrQjoQXeKg2c705va5FRYNCr7eo3nLPLNsZ1vcVvzTWFqFj8ig2JNuG7KcXcOkT6u/otjJ74C2WL1DgBNUBgcKQ6xxMmmSfnOrexBpbDamIzJRIcxWBcmf8D3zpnnI7CqLSbhZ55nB7nJltV4psHJR/mcMJ7S1CoC0Prn2A==; Received: from [66.196.81.164] by nm27.access.bullet.mail.bf1.yahoo.com with NNFMP; 04 Aug 2014 12:54:22 -0000 Received: from [98.138.104.99] by tm10.access.bullet.mail.bf1.yahoo.com with NNFMP; 04 Aug 2014 12:54:22 -0000 Received: from [127.0.0.1] by smtp119.sbc.mail.ne1.yahoo.com with NNFMP; 04 Aug 2014 12:54:22 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1407156862; bh=BH3T+moZ3m+1ZPJcseS9qa2cI4MkG0b/ST7tBzfy7MI=; 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=WyRETFa0MEA67yl+efhuyJT27WGSMLaM3tLS0AdOC5G1v1ZqWhg2iYsaXWX/UVwFhhgWPQEfLI0OX3RO/lPxQZBVy2sh3kZODkC9hian2I/3qVBDlMoH1cvYXxicj4f2IfP6yRz2EfU4nMwgMBOYz1tiuP4m9HB/f8hdaLs5iKw= X-Yahoo-Newman-Id: 870907.99350.bm@smtp119.sbc.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: Rzt_JL8VM1m71Sq6gMwO3BhEI_VPvUsIqhPIoVm.77BZG3o .y6.SPFQB1XdAyejADR1I5XOCtOu6dRuZShlKofwUo0i0Hqt1nhPnYEPrEZ5 zYhOL6brp.5UrM1jLLbE5BZEHPhRGCv.CNE1it1rT3OB0LcmuPK8t_hpojjw 8nCBif9AWSjznz_r0kiAyt38_3uhwbL03MBAqti775c0I8QI0bGJcC1ZL90y tcs7.FW2WXHhHcsAdZPqv_kABf5xeoCusT_CkX3qNaTI0_.VUDRkt2vzTKnr kRJvrG0GOYwuvrxeNJYRbr2nkA90f0l0SCffDg5nf5fAu_F0eo8AdazSwTXH jsemcjzO9tTSUNOS6rI_S7IVr3230EuD996bHda45KkaV2dyL8k011FeLtY7 TnOj52XbIrYL766sVBexJE9GHSlJnYEqzepMT94cR.opj_5mRnJjkgvy_K87 j0BsSAnlr1mb2lTinTYbo538vDraQhlzSGrD3MeMV7Lx4tKGj8_bsAwSRCnB IIKLzL77a0RgXP9DjE2GTD2NBMYcf7lvCg3U8lPrGtCZ6_sR4eJlljAXDdw- - X-Yahoo-SMTP: OKD1keCswBBTAmAF1s00hLyKW3wE3YfSK0Eazl6b4VZG4LTqJxg- Message-ID: <53DF827D.5070208@att.net> Date: Mon, 04 Aug 2014 08:54:21 -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: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support - submission? References: <53DD61BD.7050508@att.net> In-Reply-To: <53DD61BD.7050508@att.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Ian Smith X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Aug 2014 13:00:59 -0000 On 08/02/2014 18:10, Anthony Jenkins wrote: > Okay how do I get this bad boy into -CURRENT? Do I need a sponsor to do the commit? Get my own FreeBSD developer status? I should probably open a bug first...commits should reference bugs. Anthony > 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 > > I also verified the ACPI CMOS region code only accesses up to register 63 (0x3F - in previous emails I mistakenly said 0x7F). > > 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. > > Thanks, > Anthony Jenkins From owner-freebsd-acpi@FreeBSD.ORG Tue Aug 5 10:47:09 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 1DCCC36D for ; Tue, 5 Aug 2014 10:47:09 +0000 (UTC) Received: from sola.nimnet.asn.au (paqi.nimnet.asn.au [115.70.110.159]) (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 616D224B6 for ; Tue, 5 Aug 2014 10:47:07 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by sola.nimnet.asn.au (8.14.2/8.14.2) with ESMTP id s75AksYL046263; Tue, 5 Aug 2014 20:46:54 +1000 (EST) (envelope-from smithi@nimnet.asn.au) Date: Tue, 5 Aug 2014 20:46:54 +1000 (EST) From: Ian Smith To: Anthony Jenkins Subject: Re: [PATCH] ACPI CMOS region support - submission? In-Reply-To: <53DD61BD.7050508@att.net> Message-ID: <20140805175541.G62404@sola.nimnet.asn.au> References: <53DD61BD.7050508@att.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; CHARSET=US-ASCII Content-ID: <20140805175541.A62404@sola.nimnet.asn.au> Cc: freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Aug 2014 10:47:09 -0000 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. > 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. 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? 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. 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? 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; 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); Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ? +int +rtcin(int reg) +{ + u_char val; + + acpi_cmos_read(reg & 0xFF, &val, 1); +void +writertc(int reg, u_char val) +{ + acpi_cmos_write(reg & 0xFF, &val, 1); +} Reg is already masked to 0xFF here anyway, and AFAICT every other access system-wide is via rtcin() and writertc(). static int +is_datetime_reg(ACPI_PHYSICAL_ADDRESS address) +{ + return address == 0x00 || + address == 0x02 || + address == 0x04 || + address == 0x04 || + (address >= 0x06 && address <= 0x09); +} 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? +static ACPI_STATUS +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address, UINT32 width, + UINT64 *value, void *context, void *region_context) +{ + 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; 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; 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? 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'. Probably I'm missing something really major here, on recent form :) cheers, Ian From owner-freebsd-acpi@FreeBSD.ORG Tue Aug 5 13:06:47 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 816DAB7B for ; Tue, 5 Aug 2014 13:06:47 +0000 (UTC) Received: from nm19-vm1.access.bullet.mail.bf1.yahoo.com (nm19-vm1.access.bullet.mail.bf1.yahoo.com [216.109.115.96]) (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 2F3C127DD for ; Tue, 5 Aug 2014 13:06:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s2048; t=1407243865; bh=PVSyLpcFl6mGGvkGaPiE5uyx1SzuddvSe7P7FHIOtzw=; 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=D+QeHEj05kF0iL/s/OnYl4Efj7SjeRr46FNN+tYT3pqiBR+LaFc2YuiX5pavJBlwcn1cD1AjjQjUa+uEDz8Ev8TzV5zY2xfnkijXYxOyXqHuRCc5K+iFEXFJNOz5tno6hlobzinxSX+ds3SDdf0/nW1AVG3trsIUStxAvplTs4bM8u5mOS3INDLzD73djcMfNXHXyqfePcyLQhifCRhSBu4yKxfKkatVOXblpKNqCan1uHle2WOG3auC0e6zBg0Us/2px/QPf2SU3B4catF3FxLd5cFxYYIYsHxH/KlTThnVhbvMZXoCeWptMihdKRK8hQeLW9AOqGGbqAtYOtigDw== DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s2048; d=att.net; b=TfoIgpiBVj+Hzy35p0iGcS7XNebpQB8YyWghKEuL4+cinFjiQflXGhFSqQt0HQWRMv/dgp8oh54IcOqe6ej8O/0FGe0tj2sORuHpYUfj175+F8ub0NAS8LmpSkDzamgpEHBMIDtsC1f+fiVvEN6LAdRO/TiBe2VBbjL4mk82z9qanxCDu1g+yMT4M4ugwLtVGiv9FxPDs3Ct6lt17E1GPaaQLoRPB8qkZxlDe4JacWaHszwSn54RgHkDuyx0EJuezVqMXbGgfEzw0kRBPLi+JWSpjBHbS5Gv7PRuZrJzqP1RpgY+WfMqDGNX4FouyCULkrchXm+2r2JMwa5dqSh/ow==; Received: from [66.196.81.159] by nm19.access.bullet.mail.bf1.yahoo.com with NNFMP; 05 Aug 2014 13:04:25 -0000 Received: from [98.138.104.98] by tm5.access.bullet.mail.bf1.yahoo.com with NNFMP; 05 Aug 2014 13:04:25 -0000 Received: from [127.0.0.1] by smtp118.sbc.mail.ne1.yahoo.com with NNFMP; 05 Aug 2014 13:04:22 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1407243862; bh=PVSyLpcFl6mGGvkGaPiE5uyx1SzuddvSe7P7FHIOtzw=; 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=1F3mjE/zl8eqCOufMQFLaC6WFYwOCzXskaGMfwgFHEMfSQLshJ4oiV/RwPX8ZHoBmBew8RjfLuEZmG/wSOcFPvIuj3gQy1NJv14cC9q0iotzDrEZQrhBJShAULMuuxAyQL9JXQ/iPmE+Xu4cP55qQlyHRIXHpEv4X2igwZ8qIBE= X-Yahoo-Newman-Id: 658238.75801.bm@smtp118.sbc.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: .BdMa5QVM1l2ty3o6CndySlYTfgyptQNXfh0upPuzxAfn0a iOvDwwcO74m3uJL87psSre9AMjFCHyFV2.TckdM4OizvRnWqTXLpEHgmuzUD fV748fVTFadf7pOpO_ESts9KpGNj3QMC0waHZ.EXSvui1qYli3WPzOWWVx4l N_LNV3MbXVe06iBBryZAQ._sH8A_oh4vKHRnob20pXgjwoOlaAM5a9ZeHr0W Ls.cnRfF50RIgJpLhS66L3RjbHEB_VjIGDWfywlwQi9FB31AJa.9fOvipLWK gr_k.YwDynh.GrNw4ZJCEGF.w5i1LdNVa6NYfibdPlTZI9CYiSjd5Qdrb4BO YeKtv14pW7G4nPxrBdfIXFR7P7s74FCBfJJC5DGevcFT3ZcIcXY9wrpCdvej .jSFhln0ZvyYZ49e5a_c7rC8urCptK8dEWl8nO88HrEu0ZeZtMJtNU5Cg8fC RQffyYZF1ghfuZJ0HINC35TmeBD8TArUSUm6XCa4g.hZunJYRJ5HoUkJfhel ckDH6N07QG0OQFVHUawp0Iz5sDfGHIg8HV4Bw.6QXH8HMf.2tERQ3nuw97g- - X-Yahoo-SMTP: OKD1keCswBBTAmAF1s00hLyKW3wE3YfSK0Eazl6b4VZG4LTqJxg- Message-ID: <53E0D655.6090701@att.net> Date: Tue, 05 Aug 2014 09:04:21 -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: Ian Smith Subject: Re: [PATCH] ACPI CMOS region support - submission? References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> In-Reply-To: <20140805175541.G62404@sola.nimnet.asn.au> 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 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Aug 2014 13:06:47 -0000 On 08/05/2014 06:46, 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? Yes, just for this specific device with PNP ID PNP0B00. > 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. True... the ACPI spec (3.0-5.0 at least) also added PNP0B01 and PNP0B02 to cover two (apparently common) CMOS implementations which are 128+ bytes. To cover these devices I'd have to sorta overhaul the atrtc.c driver (or add different drivers). I'm just focusing on the ubiquitous PNP0B00 for now... > > 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. That's fair, I also don't foresee any reason for ACPI to twiddle with the date/time CMOS regs (though there /is/ an open FreeBSD bug about ACPI wake from alarm not supported). As proposed, my code disallows write access to these regs; I just wanted to cover the case where somebody interpreted the spec as I did and designed their system to write to those registers. Also, to me the authors of the ACPI code are the same ones who designed the system and selected the south bridge part which provides the PNP0B00 device; personally I have no problem letting them stomp around in the date/time/alarm regs :-) Maybe it's sufficient to add a compile time flag like ALLOW_ACPI_CMOS_DATETIME_WRITES or something to enable the iffy behaviour. I need to find out where I got the idea that those regs were excluded from ACPI writes... > 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? > > 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. > > 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; Yeah I actually couldn't figure out the purpose of that variable until recently, then it just seemed an unnecessary optimization. > 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? > > 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; > > 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); > > Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ? Right, I just wanted to ensure bit 7 was always set (or was it clear?). Does that bit still control NMI? Maybe that should've been & (~0x80)... good catch! My system seems to not care about that bit... > +int > +rtcin(int reg) > +{ > + u_char val; > + > + acpi_cmos_read(reg & 0xFF, &val, 1); > > +void > +writertc(int reg, u_char val) > +{ > + acpi_cmos_write(reg & 0xFF, &val, 1); > +} > > Reg is already masked to 0xFF here anyway, and AFAICT every other access > system-wide is via rtcin() and writertc(). > > static int > +is_datetime_reg(ACPI_PHYSICAL_ADDRESS address) > +{ > + return address == 0x00 || > + address == 0x02 || > + address == 0x04 || > + address == 0x04 || > + (address >= 0x06 && address <= 0x09); > +} > > I guess that second 0x04 should be 0x06? Right, copy/paste error. > 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? That function just encapsulates identifying which registers are "read-only" (i.e. the date/time regs); I did not want to limit ACPI from writing the alarm regs. > +static ACPI_STATUS > +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address, UINT32 width, > + UINT64 *value, void *context, void *region_context) > +{ > + 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; > > Ok, address is limited to 0x3f here, though strictly address should be > less than (64 - width>>3) for multibyte access? Ahh yes, didn't catch that. > I guess non-ACPI access > via rtcin() and writertc() will still be able to access registers, well, > from 0 to 0xff .. Yup, tried to make sure not to change the behavior of rtcin() and writertc() (I did with the 0x80 bit masking, but it seemed to be a no-op). > + if (function == ACPI_WRITE && > + (is_datetime_reg(address) || > + (width > 8 && address <= 0x09))) > + return AE_BAD_PARAMETER; > > I'd exclude AT LEAST 0x0a - 0x0d for write, certainly 0x0c from read. I'll add those exclusions, possibly providing knobs for people to enable them if they so wish. > + 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? > > 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'. > > Probably I'm missing something really major here, on recent form :) Good comments, thanks! I'll try to make the changes tonight (but I'm slammed this week with a business trip tomorrow). Anthony > > cheers, Ian > From owner-freebsd-acpi@FreeBSD.ORG Tue Aug 5 20:49:23 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 CA93D1D5 for ; Tue, 5 Aug 2014 20:49:23 +0000 (UTC) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 510F724A2 for ; Tue, 5 Aug 2014 20:49:22 +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 mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 1C135D41C08; Wed, 6 Aug 2014 06:49:13 +1000 (EST) Date: Wed, 6 Aug 2014 06:49:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Smith Subject: Re: [PATCH] ACPI CMOS region support - submission? In-Reply-To: <20140805175541.G62404@sola.nimnet.asn.au> Message-ID: <20140806040804.P849@besplex.bde.org> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> 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=eojmkOZX c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=yaUwhfENFygA:10 a=qMAOWPgdKHIA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=QhA1Jl_5v47fETCu2T4A:9 a=yLazs6WoQpZzPl8J:21 a=vI0KxA2gom9bJNVb:21 a=CjuIK1q_8ugA:10 Cc: Anthony Jenkins , freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Aug 2014 20:49:23 -0000 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. 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. > > 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. > > 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. > 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? > > +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). > > 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 From owner-freebsd-acpi@FreeBSD.ORG Thu Aug 7 15:26:19 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 A5CD276D for ; Thu, 7 Aug 2014 15:26:19 +0000 (UTC) Received: from mail-lb0-x22a.google.com (mail-lb0-x22a.google.com [IPv6:2a00:1450:4010:c04::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 2C3CB2904 for ; Thu, 7 Aug 2014 15:26:18 +0000 (UTC) Received: by mail-lb0-f170.google.com with SMTP id l4so949579lbv.29 for ; Thu, 07 Aug 2014 08:26:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:subject:message-id:mime-version:content-type :content-disposition; bh=lXgViA7VbxR06PBgkIcYepa1Ikb2TQODCxS48Iloknk=; b=I2FSOH5/fKZO/+2HTVZnLUpDwsMqp76LYziHDCZskxoezvCteCY+firEalOp7Tvxrq abWoLK5PoPDsH4jMP9nhOl3XS0IgAh9liqfYWVzdG7q2vG817qqTLfSeGt2XkWum5Dl8 CsnOqj/7dAWIQfkTb2KcXHcD1HA+wDjq7r/nZHVhRCzo+zZOJyJEXlyQb4GMx07yMS5F iv1bVT6qCRh4Xt7W9l98bbf42I6RwXgkSSroZy/Noa8K5soXkudKBcgNo+AFidmjBddB aHOUsoDK3FrfLVdf7Ec/pIBnCPM6ctg95zM6G9Lrvz/bj3+GkDNWrvbLnPm7fHM5LiJH P/Fw== X-Received: by 10.112.28.8 with SMTP id x8mr2214781lbg.104.1407425176740; Thu, 07 Aug 2014 08:26:16 -0700 (PDT) Received: from hell.localdomain (b217-29-190-130.pppoe.mark-itt.net. [217.29.190.130]) by mx.google.com with ESMTPSA id o3sm245826lbh.24.2014.08.07.08.26.15 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Aug 2014 08:26:15 -0700 (PDT) Received: from hell (localhost [127.0.0.1]) by hell.localdomain (Postfix) with ESMTP id 79A7D13ABBA3 for ; Thu, 7 Aug 2014 19:26:13 +0400 (MSK) Received: (from bvv@localhost) by hell (8.14.7/8.14.7/Submit) id s77FQD8S010872 for freebsd-acpi@freebsd.org; Thu, 7 Aug 2014 19:26:13 +0400 (MSK) (envelope-from bvv) Date: Thu, 7 Aug 2014 19:26:13 +0400 From: Bykov Vladislav To: freebsd-acpi@freebsd.org Subject: Incorrect battery status Message-ID: <20140807152613.GA9936@hell> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Aug 2014 15:26:19 -0000 Hello, I am having a problem sensing battery charge. Yesterday my laptop had turned off halfway through capacity. When I turned it on again, I found only 30% remaining. Beside that, sometimes when charging capacity jumps as much as +-20%. I am not sure whether it is ACPI or my battery. Laptop model: HP Envy 4. Sincerely, Vladislav. From owner-freebsd-acpi@FreeBSD.ORG Fri Aug 8 13:15:47 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 24754CAD for ; Fri, 8 Aug 2014 13:15:47 +0000 (UTC) Received: from sola.nimnet.asn.au (paqi.nimnet.asn.au [115.70.110.159]) (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 7F3882188 for ; Fri, 8 Aug 2014 13:15:44 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by sola.nimnet.asn.au (8.14.2/8.14.2) with ESMTP id s78CvKhc001052; Fri, 8 Aug 2014 22:57:20 +1000 (EST) (envelope-from smithi@nimnet.asn.au) Date: Fri, 8 Aug 2014 22:57:20 +1000 (EST) From: Ian Smith To: Bruce Evans Subject: Re: [PATCH] ACPI CMOS region support - submission? In-Reply-To: <20140806040804.P849@besplex.bde.org> Message-ID: <20140808185539.V62404@sola.nimnet.asn.au> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> <20140806040804.P849@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Anthony Jenkins , freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Aug 2014 13:15:47 -0000 On Wed, 6 Aug 2014 06:49:12 +1000, 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. Well, we've long - not sure about 'always' - saved current timestamp to RTC clocktime (ts_to_ct) on suspend and recalled on resume (ct_to_ts), shown when bootverbose is enabled, x86 anyway; this has nothing to do with ACPI. Actually, 9.x and presumably 10.x needs debug.clocktime=1 to see the messages re these, previously (eg below from 8.2-R) you see these by default every 30 minutes, having booted verbose, which was annoying: Aug 8 00:41:32 t23 kernel: ts_to_ct(1407458492.000380514) = [2014-08-08 00:41:32] Aug 8 01:11:31 t23 kernel: ts_to_ct(1407460291.799401540) = [2014-08-08 01:11:31] Aug 8 01:41:31 t23 kernel: ts_to_ct(1407462091.598502848) = [2014-08-08 01:41:31] Aug 8 02:11:31 t23 kernel: ts_to_ct(1407463891.397341911) = [2014-08-08 02:11:31] Aug 8 02:41:31 t23 kernel: ts_to_ct(1407465691.196399111) = [2014-08-08 02:41:31] Aug 8 03:11:31 t23 kernel: ts_to_ct(1407467490.995395001) = [2014-08-08 03:11:30] Aug 8 03:41:30 t23 kernel: ts_to_ct(1407469290.794307971) = [2014-08-08 03:41:30] This reveals some warts; seconds are always rounded down, so that on resume you can expect, on average, to lose half a second; worst case almost a second. Thus the first ntpd correction after (even immediate) resume is often positive, maybe more than a second, sometimes two or so. Similarly when restoring time from RTC on resume, there's no synching with UIP so, on average, another half second will be dropped before ntpd picks up the pieces. Whether sync is worth the bother or time is moot. > > 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. Is it accurate enough for pps? Are you using ntpd to update your RTC? Ok, and others may want to use alarm or periodic interrupts for whatever purpose, possibly at much higher rates than once per second, so there's still a case for maintaining the reg select optimisation noted below .. > > 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. Right, I had completely missed its purpose. So perhaps did Anthony? > 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. I'm struggling to see why it's a 'good' re-pessimization? You suggest below that adding another one or two I/O delays would be even better? > > 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. I don't write in C at all, so I'll let style comments through to the keeper .. in this case Anthony :) > 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 originally thought the former also, but Anthony has been encouraged by someone here to add the ACPI CMOS Region methods here. Linux has a separate driver for this, acpi_cmos_rtc.c, on top of its usual rtc-cmos.c, which is itself way more complex than ours, much of that in supporting wake-on-alarm but also supporting 256 bytes of NVRAM, which uses a second MC146818-like chip - so still only using 0-0x7f byte access on each one. Linux also just ignores the issue of NMIs; quote: /* Most newer x86 systems have two register banks, the first used * for RTC and NVRAM and the second only for NVRAM. Caller must * own rtc_lock ... and we won't worry about access during NMI. */ > > 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. Yes, I think that'd be the right way to go; leave rtcin() and writertc() alone, warts and all allowing r/w of locations 0-0xff for its consumers, and as you say, calling rtcin() in the loop. This preserves POLA and further would allow moving the ACPI stuff to anywhere? more appropriate, I guess ACPICA when that's updated to ACPI 5 or wherever this came in. > > 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. Ok, but if we don't bother disabling NMI, no need to reenable it, right? And that bit 0x80 isn't 'in' the RTC, it must be separately decoded on the address bus to a latch or such .. > 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. Interesting, but I'm just going to take your word for all of that :) > > +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). But we really don't want to get into the NMI handling, unless getting right into it, yeah? I suspect right now it'd be added complication. > > ... > > 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. > > 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? Actually, I missed the negative connotation of is_datetime_reg; it does in fact attempt to allow only access to the alarm regs, though using multibyte access might defeat that. But further thoughts on what ACPI should or shouldn't have write access to, in a later message to Anthony. > > +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). > > > > > 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. You can't easily 'see' what BIOS AML code is doing without examining it specifically, and it's both vendor- and user-updateable. This is why I think it's important to print exactly which parts of RTC/NVRAM are being accessded, apart from developing some doc on what different vendors do. > 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. I don't think it makes sense. We already look after writing datetime to RTC on suspend, and on ntpd corrections. We read it back on boot, or resume, albeit both ways ignoring synching seconds properly. Why would we want to let AML code use OS ACPI services to write to those fields, without having actually requested a service, when the OS thinks it is in charge of managing time? Perhaps I just have a naive view on this? Other bytes in (so-called) NVRAM that a particular machine wants saved to and perhaps later reloaded from there across boot or suspend/resume - that this code is designed to deal with in the first place - is all very well, and _perhaps_ some BIOS might want to manage updating the alarm bytes this way (although normally configured in BIOS SETUP), but any potential to mess with date or time itself seems out-of-bounds to me. Further to /sys/dev/nvram/nvram.c .. it allows access to bytes 0x0e (EQUIPMENT) through 0x7f, but also checks then recalculates the checksum for bytes 0x10 through 0x2d, stored in 0x2e and 0x2f. So if ACPI wants to write anywhere in that area, it has to update the checksum too. We still have yet to see examples of reads or writes that this code allows. > - 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. Yes, which is why I went looking for what other vendors want to use, to no avail. None of our other acpi_support modules appear to use NVRAM, or at least, via rtcin() and writertc(). > - 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. At least nothing outside /sys/ accesses rtcin() or writertc(). One other thing; I don't really know where to go to check Linux code; google landed me at mit.edu a lot so I gathered bits there (Linux 3.x) but couldn't find a modern definition for tHeir CMOS_READ / CMOS_WRITE macros. This looks too old, maybe: http://members.xoom.virgilio.it/ozma/mulinux/Download/El-Torito_iso/usr.163 from 1993 .. #define CMOS_READ(addr) ({ \ outb_p((addr),RTC_PORT(0)); \ inb_p(RTC_PORT(1)); \ }) #define CMOS_WRITE(val, addr) ({ \ outb_p((addr),RTC_PORT(0)); \ outb_p((val),RTC_PORT(1)); \ }) .. but if that's all, why are we bothering with any I/O delay at all? Or do outb_p() and inb_p() take care of that? Yes, too many questions! Thanks for all the fish. cheers, Ian From owner-freebsd-acpi@FreeBSD.ORG Sat Aug 9 00:13:27 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 A8BEED4C for ; Sat, 9 Aug 2014 00:13:27 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 560C62136 for ; Sat, 9 Aug 2014 00:13:26 +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 mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 6E6411A097C; Sat, 9 Aug 2014 10:13:19 +1000 (EST) Date: Sat, 9 Aug 2014 10:13:18 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Smith Subject: Re: [PATCH] ACPI CMOS region support - submission? In-Reply-To: <20140808185539.V62404@sola.nimnet.asn.au> Message-ID: <20140809075607.F992@besplex.bde.org> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> <20140806040804.P849@besplex.bde.org> <20140808185539.V62404@sola.nimnet.asn.au> 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=BdjhjNd2 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=yaUwhfENFygA:10 a=qMAOWPgdKHIA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=iDx2INy_AAAA:8 a=_DmKWEar99B987Qc2u8A:9 a=FDMNH4t1IBWswFTX:21 a=RzCvpdMmAbLgQbJv:21 a=CjuIK1q_8ugA:10 Cc: Anthony Jenkins , freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Aug 2014 00:13:27 -0000 On Fri, 8 Aug 2014, Ian Smith wrote: > On Wed, 6 Aug 2014 06:49:12 +1000, Bruce Evans wrote: > > On Tue, 5 Aug 2014, Ian Smith wrote: > > > > > On Sat, 2 Aug 2014 18:10:05 -0400, Anthony Jenkins wrote: >* ... > > > 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. > > Well, we've long - not sure about 'always' - saved current timestamp to > RTC clocktime (ts_to_ct) on suspend and recalled on resume (ct_to_ts), > shown when bootverbose is enabled, x86 anyway; this has nothing to do > with ACPI. > > Actually, 9.x and presumably 10.x needs debug.clocktime=1 to see the > messages re these, previously (eg below from 8.2-R) you see these by > default every 30 minutes, having booted verbose, which was annoying: No, not long. This bug was intentionally left out originally, and wasn't committed until a few years ago. Only ntpd or the user can know if the time is correct enough to write to the RTC. Unfortunately, writes from ntpd don't happen automatically, since ntpd uses normally uses micro- adjustments on FreeBSD. Setting the clock using settimeofday() or clock_settime() works right for setting the RTC if and only if it is done ntpd has stabilized. (It has some bugs in its clobbering of the boot time, but works right for the hardware.) Unfortunately, ntpd usually only sets the clock like this for an initial step _before_ ntpd has stablized. At least with old versions of ntpd. I use an old version with ntpdate and set the clock at boot time using ntpdate from an accurate timeserver. This prevents ntpd doing a less accurate step. The writing routine is of low quality and has an error of +-1 second (by not synchronizing with seconds updates). The reading routine is not much better and also has an error of +-1 second. The latter is fixed in my version. Synchronization is easier for reading. For writing, I just log the write and return if it would make a less than useless change: % /* % * If the RTC time is already correct (to within -0 + 1 second), % * then do nothing, since writing would just clobber the hidden % * state which is more likely to be correct than the (truncated) % * time. We assume that the weekday is correct if everything % * else is. % */ % tv_sec = rtc_time(&s); % nanotime(&ts); % if (tv_sec == -1) % s = splhigh(); Here rtc_time() is like inittodr() except it reads the RTC accurately (to about 10 usec) and has the necessary locking to do this atomically enough with nanotime(). % change = (tv_sec == -1 || % (tv_sec != ts.tv_sec && tv_sec + 1 != ts.tv_sec)); % klocaltime(change ? ts.tv_sec : tv_sec, % &yyyy, &mm, &dd, &wday, &hh, &MM, &ss); Here klocaltime() is like clock_cs_to_ct() and localtime() except it has an easier to use API. % yy = yyyy % 100; % printf("resettodr: %schange\n", change ? "" : "no "); % printf("old: %02x/%02x/%02x %02x:%02x:%02x, cent = %02x, wday = %d\n", % rtcin(RTC_YEAR), rtcin(RTC_MONTH), rtcin(RTC_DAY), % rtcin(RTC_HRS), rtcin(RTC_MIN), rtcin(RTC_SEC), % rtcin(RTC_CENTURY), rtcin(RTC_WDAY)); % printf("new: %02x/%02x/%02x %02x:%02x:%02x, yyyy = %d, wday = %d\n", % bin2bcd(yy), bin2bcd(mm), bin2bcd(dd), % bin2bcd(hh), bin2bcd(MM), bin2bcd(ss), yyyy, wday); % if (change == 0) { % splx(s); % return; % } All this used to be protected by splhigh(), but splhigh() locking was broken by SMPng. It was used not just for exclusion, but also for timing. SMPng only gave the exclusion (by Giant locking, provided Giant locking is actually used for all callers). -current still has only Giant locking in clock_settime(), and non extra in inittodr() or resettodr(). This is most broken where inittodr() calls tc_setclock(). tc_setclock() is only locked by the calling it only from the hardclock interrupt handler, where it is locked by whatever locking that has (certainly not Giant). The above version benefits from some time-domain locking: rtc_time() synchronizes with the seconds rollover. After that the clock registers are stable for almost 1 second so it would take 1 seconds worth of interruption for the registers to change. Since this is a write, the current state of the registers doesn't matter so much, but 1 seconds of interruption would make the time written wrong by 1 second. The time written is certainly wrong by many microseconds, but since there is no synchronization the time written is always wrong by +-1 second. If the clock hardware were simple, then the error would usually be negative with an average of -0.5 seconds, but ISTR that there are complications that make the error less predictable. On read, the error is usually negative with an average error of -0.5 seconds, except the broken locking for timing allows huge errors when the seconds rollover also rolls over the minutes and/or hours. It only takes being interrupted for 244 usec to break the timing. > Aug 8 00:41:32 t23 kernel: ts_to_ct(1407458492.000380514) = [2014-08-08 00:41:32] > Aug 8 01:11:31 t23 kernel: ts_to_ct(1407460291.799401540) = [2014-08-08 01:11:31] > Aug 8 01:41:31 t23 kernel: ts_to_ct(1407462091.598502848) = [2014-08-08 01:41:31] > Aug 8 02:11:31 t23 kernel: ts_to_ct(1407463891.397341911) = [2014-08-08 02:11:31] > Aug 8 02:41:31 t23 kernel: ts_to_ct(1407465691.196399111) = [2014-08-08 02:41:31] > Aug 8 03:11:31 t23 kernel: ts_to_ct(1407467490.995395001) = [2014-08-08 03:11:30] > Aug 8 03:41:30 t23 kernel: ts_to_ct(1407469290.794307971) = [2014-08-08 03:41:30] Worse than I remembered (I don't run any kernels with this bug), so I never saw these messages before). Updating every 30 seconds is far too frequent, and spams the logs. My logs are spammed with the following messages instead :-): % Aug 8 05:46:49 besplex kernel: inittodr: delta = 0.000041737 This is the latency between the RTC read and using the result. ~41 usec. % Aug 8 05:46:56 besplex kernel: resettodr: change % Aug 8 05:46:56 besplex kernel: old: 14/08/08 05:46:54, cent = 20, wday = 6 % Aug 8 05:46:56 besplex kernel: new: 14/08/08 05:46:56, yyyy = 2014, wday = 6 This is the change initiated by netdate -b after booting. Nothing except rebooting calls resettodr(), and this change of 2 seconds is for 1 days of drift in the RTC. An error of 1-2 seconds per day means that it it is useless to sync to the RTC more often than once per day. Twice per hour is much more than once per day. % Aug 8 08:03:13 besplex kernel: sampled delta = 0.533057039, tv_sec = 1407448993 % Aug 8 10:19:45 besplex kernel: sampled delta = 0.615658287, tv_sec = 1407457185 % Aug 8 12:36:17 besplex kernel: sampled delta = 0.699694553, tv_sec = 1407465377 % Aug 8 14:52:49 besplex kernel: sampled delta = 0.785025899, tv_sec = 1407473569 % Aug 8 17:09:21 besplex kernel: sampled delta = 0.874881660, tv_sec = 1407481761 % Aug 8 19:25:54 besplex kernel: sampled delta = 0.963000188, tv_sec = 1407489953 % Aug 8 21:42:26 besplex kernel: sampled delta = 1.046254684, tv_sec = 1407498145 This tracks the drift of the RTC every 2048 seconds. Not much different to twice per hour. During development I used to log these messages even more often. These messages show the drift of the RTC almost exactly. It is about 82 milliseconds per 2048 seconds. My version tracks the drift every 16 seconds, and uses the delta for things like resetting the system time (not the RTC time) after stopping in ddb. Stopping in ddb is like a short suspension. Similar methods should be used to reset the system time after suspension. The new time is the current RTC time plus a compensation, where the compensation is the absolute difference of the times at the previous RTC read (done every 16 seconds or so while not suspended) plus the suspension time times the estimated RTC drift rate. > This reveals some warts; seconds are always rounded down, so that on > resume you can expect, on average, to lose half a second; worst case > almost a second. Thus the first ntpd correction after (even immediate) > resume is often positive, maybe more than a second, sometimes two or so. Indeed. It is half a second on average from upper layers not having any synchronization with seconds rollovers, then more from complications in the RTC. (I'm mostly writing about x86 RTCs. The details of the hardware complications are MD of course.) According to Linux sources, the complications are that settings must be made on half-second boundaries, not seconds boundaries. Apparently the rollover for a newly set value occurs after 500 milliseconds. You wouldn't want it to be unsynchronized, and having it occur after 500 milliseconds is less fragile than having it occur after 0 or 1000 milliseconds. In fact, 500 millseconds compensates on average for low quality setters like FreeBSD. These setters have an average error of -0.5 seconds, and the hardware behaviour gives a fixed adjustment of +0.5 seconds, so the average error is 0. Careful setting and reading doesn't matter much since the clock drift overnight is typically 1 second. It is not useful to adjust the clock by 1 second for the day's use and then have it drift by 1 second overnight. The error on rebooting is just reduced from 2 seconds to 1 second. Both are too large for ntpd, so you need ntpdate or worse in the boot sequence to fix up the clocks before they are used much. If the system is not turned off overnight or rebooted for other reasons, then the RTC time matters even less. > Similarly when restoring time from RTC on resume, there's no synching > with UIP so, on average, another half second will be dropped before ntpd > picks up the pieces. Whether sync is worth the bother or time is moot. Indeed. I get the sync during normal operation using the seconds alarm. No waiting to read the clock then. Otherwise, rtc_time() has to busy-wait for up to 1 second. Note good for an active system. Acceptable for booting and resuming. > > > 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. > > Is it accurate enough for pps? Are you using ntpd to update your RTC? No, but I once planned to use RTC to update ntpd (for systems not connected to the internet for long periods). It turned out that the RTC and the TSC drift by about the same rates, so they are equally good or bad as primary sources for ntpd. I had one system drifting by about 0.1 seconds/ day after drift compensations (the RTC was off by several seconds per day, but after compensating for this the drift from temperature variations was only 0.1 seconds/day. That is over a week or 2 when the average termperature doesn't change much. The compensation should depend on the season and on actual temperature measurements, but I never automated all that). So I now just use pps to detect any long-term drift or errors in the RTC. ntpdc -c kerninfo reports after 2 hours of uptime: % pll offset: -0.0011784 s2 hours of uptime: % pll frequency: -2.453 ppm % maximum error: 0.20712 s % estimated error: 0.004653 s % status: 2101 pll ppssignal nano % pll time constant: 6 % precision: 1e-09 s % frequency tolerance: 496 ppm % pps frequency: -14.001 ppm % pps stability: 0.076 ppm % pps jitter: 1.7625e-05 s % calibration interval: 256 s % calibration cycles: 48 % jitter exceeded: 2 % stability exceeded: 0 % calibration errors: 1 I forget how to interpet all of this. The pps frequency of -14.001 ppm is the average drift of the RTC relative to the (ntpd-disiplined) real time (or something). The pps stability of 0.076 ppm seems good. The pps jitter of 1.7525e-05 s seems like nonsense. IIRC, this measurement is not really jitter, but incorporates the average error of -14.001 ppm and some other error (1.7525e-05 translates to 17.525 ppm). > Ok, and others may want to use alarm or periodic interrupts for whatever > purpose, possibly at much higher rates than once per second, so there's > still a case for maintaining the reg select optimisation noted below .. Even once per second is too high. The lapic timer can do short, variable timeouts much better. The RTC has a very high overhead for reprogramming. > > > -static int rtc_reg = -1; > ... > > 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. > > I'm struggling to see why it's a 'good' re-pessimization? You suggest > below that adding another one or two I/O delays would be even better? Good pessimation == optimization for reduced performace. Even easy by wasting more time. > > 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 originally thought the former also, but Anthony has been encouraged by > someone here to add the ACPI CMOS Region methods here. The existence of the nvram driver and acpi_support/acpi_ibm.c show that rtcin() and writertc() can be used without any changes. The Region methods can just loop calling these functions. The only thing a special routine can do better is lock all the device accesses together. But locking is neither necessary nor sufficient for getting a coherent read -- the data can still change in a short time. Reads that need coherence should do something like looping until the whole region stabilizes. Writes are more complicated -- neither locking nor checking for stability gives atomicity for volatile registers. > > > 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. > > Yes, I think that'd be the right way to go; leave rtcin() and writertc() > alone, warts and all allowing r/w of locations 0-0xff for its consumers, > and as you say, calling rtcin() in the loop. This preserves POLA and > further would allow moving the ACPI stuff to anywhere? more appropriate, > I guess ACPICA when that's updated to ACPI 5 or wherever this came in. Except possibly for the bulk locking. I think ACPI mainly wants to access non-volatile registers so it doesn't need bulk locking. Move it somewhere near acpi_ibm.c? > > > Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ? > >,,, > Ok, but if we don't bother disabling NMI, no need to reenable it, right? > And that bit 0x80 isn't 'in' the RTC, it must be separately decoded on > the address bus to a latch or such .. Yes, and most hardware probably doesn't even have the latch. > > ... > > 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. > > You can't easily 'see' what BIOS AML code is doing without examining it > specifically, and it's both vendor- and user-updateable. This is why I > think it's important to print exactly which parts of RTC/NVRAM are being > accessded, apart from developing some doc on what different vendors do. If it uses my methods then I can stop it doing bad things by not allowing it to access most registers, and find what it is doing by loggin all accesses :-). Hard for the end user with unusual AML to debug though. I wouldn't allow it write access to any register that the system uses (mainly RTC) or read access to volatile registers that the system uses (mainly RTC statuses?). > > 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. > > I don't think it makes sense. We already look after writing datetime to > RTC on suspend, and on ntpd corrections. We read it back on boot, or > resume, albeit both ways ignoring synching seconds properly. Why would > we want to let AML code use OS ACPI services to write to those fields, > without having actually requested a service, when the OS thinks it is in > charge of managing time? Perhaps I just have a naive view on this? > > Other bytes in (so-called) NVRAM that a particular machine wants saved > to and perhaps later reloaded from there across boot or suspend/resume - > that this code is designed to deal with in the first place - is all very > well, and _perhaps_ some BIOS might want to manage updating the alarm > bytes this way (although normally configured in BIOS SETUP), but any > potential to mess with date or time itself seems out-of-bounds to me. I would allow it to write RTC registers after the system part of suspend finishes until after the system part of resume starts. Might be difficult to synchronize. > Further to /sys/dev/nvram/nvram.c .. it allows access to bytes 0x0e > (EQUIPMENT) through 0x7f, but also checks then recalculates the checksum > for bytes 0x10 through 0x2d, stored in 0x2e and 0x2f. So if ACPI wants > to write anywhere in that area, it has to update the checksum too. We > still have yet to see examples of reads or writes that this code allows. So it will need read access to access all the bytes. Actually, the volatile RTC bytes and some others must be off-limits for the checksum. > One other thing; I don't really know where to go to check Linux code; > google landed me at mit.edu a lot so I gathered bits there (Linux 3.x) > but couldn't find a modern definition for tHeir CMOS_READ / CMOS_WRITE > macros. This looks too old, maybe: Yes, it is obfuscated in a different way than FreeBSD (more at lower levels, less at higher). Try the include directory. There are squillions of definitions, some even in files named mc146818rtc.h. > http://members.xoom.virgilio.it/ozma/mulinux/Download/El-Torito_iso/usr.163 > from 1993 .. > > #define CMOS_READ(addr) ({ \ > outb_p((addr),RTC_PORT(0)); \ > inb_p(RTC_PORT(1)); \ > }) > #define CMOS_WRITE(val, addr) ({ \ > outb_p((addr),RTC_PORT(0)); \ > outb_p((val),RTC_PORT(1)); \ > }) The asm-x86_64 one in 2.6.10 (10 years old) is no different. > .. but if that's all, why are we bothering with any I/O delay at all? > Or do outb_p() and inb_p() take care of that? Yes, too many questions! Yes, they do. Linux used the extra-i/o pessimization much more than FreeBSD. outb_p() adds the pessimization if the pessimization is needed and/or configured. Bruce