Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On 03/06/2015 06:49 AM, Ian Smith wrote:
> On Tue, 3 Mar 2015 11:45:49 -0500, Anthony Jenkins wrote:
> > On 03/01/2015 09:29 AM, Ian Smith wrote:
> [..]
> Regarding systems without ACPI loaded, or active: what happens when the
> below AcpiInstallAddressSpaceHandler() call fails, but returns 0? Would
> not that prevent rtc_start() from running atrtc_start() etc for non-ACPI
> clock initialisation and registration?
Good catch, there's technically no reason to bail on rtc_start() if I
fail to register the ACPI CMOS handler; it'll just never get called
(same as old behaviour). I'll change "Error" to "Warning" and remove
the return 0.
> I suppose there's a global kernel variable for acpi_is_active ono?
>
> > +static int
> > rtc_start(struct eventtimer *et, sbintime_t first, sbintime_t period)
> > {
> >
> > @@ -245,10 +323,17 @@
> > int i;
> >
> > sc = device_get_softc(dev);
> > +sc->acpi_handle = acpi_get_handle(dev);
> > sc->port_res = bus_alloc_resource(dev, SYS_RES_IOPORT, &sc->port_rid,
> > IO_RTC, IO_RTC + 1, 2, RF_ACTIVE);
> > if (sc->port_res == NULL)
> > device_printf(dev, "Warning: Couldn't map I/O.\n");
> > +if (ACPI_FAILURE(AcpiInstallAddressSpaceHandler(sc->acpi_handle,
> > +ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler, NULL, sc)))
> > +{
> > +device_printf(dev, "Error registering ACPI CMOS address space
> handler.\n");
> > +return 0;
> > +}
> > atrtc_start();
> > clock_register(dev, 100);
> > bzero(&sc->et, sizeof(struct eventtimer));
> > @@ -286,6 +371,15 @@
> > return(0);
> > }
>
> Might that not matter for detach, as you're not testing for return code?
Yeah I'd prefer to remember the failure to install the handler and
conditionally uninstall it in the detach method.
I'll fix up the logging and add these suggestions this weekend.
Thanks,
Anthony
> > +static int atrtc_detach(device_t dev)
> > +{
> > +struct atrtc_softc *sc;
> > +
> > +sc = device_get_softc(dev);
> > +AcpiRemoveAddressSpaceHandler(sc->acpi_handle,
> > ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler);
> > +return bus_generic_detach(dev);
> > +}
>
> cheers, Ian
> ___
> freebsd-acpi@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On Tue, 3 Mar 2015 11:45:49 -0500, Anthony Jenkins wrote:
> On 03/01/2015 09:29 AM, Ian Smith wrote:
[..]
> No problem adding logging. The bit I'm confused about (unless I'm
> misunderstanding the argument) is why
>
> # ugly pseudocode
> function original_cmos_xfer():
> lock();
> transfer byte to/from CMOS
> unlock();
>
> function acpi_cmos_xfer():
> foreach byte in num_bytes:
> call original_cmos_xfer(byte)
> end foreach
>
>
> is preferable to
>
> # ugly pseudocode
> function acpi_cmos_xfer():
> lock();
> foreachbyte in num_bytes:
> transfer byte to/from CMOS
> end foreach
> unlock();
>
> function original_cmos_xfer():
> call acpi_cmos_xfer(num_bytes = 1);
>
>
> There was no "extra locking", but I did introduce an extra function call
> which would slow down CMOS accesses to the RTC for some performance
> timers; I assume that's the main complaint.
And slow down all other callers of rtcin() and writertc(), as well as
any timer using the RTC periodic interrupt. Most callers are right here
in /sys/x86/isa/atrtc.c; I make it 37 plus another 9 #ifdef DDB. This
works for me on a 9.3-R system, not checked on 10.x or head ..
smithi@x200:~ % find /usr/src -type f -name "*.[ch]" \
-exec egrep -Hl 'rtcin\(|writertc\(|readrtc\(' {} + | xargs less
:/rtcin|writertc|readrtc
All in /sys/ of course. Ignoring mips/malta, most callers likely don't
care if these calls take twice(?) as long, but to me it seems suboptimal
to impede likely more frequently used calls for the sake of a much less
frequently used high-level function, that itself is not timing-critical.
I know nothing about how much xen/clock.c cares about timing. nvram.c
doesn't seem bothered, nor fetching memory size, fdc or vga parameters.
On the other hand, interrupt service needs to care a lot about timing.
Remember that FreeBSD is running on other than latest kit, including eg
266MHz 5x86 Soekris routers and the like, and smaller embedded systems
that may very well need not to have RTC ISRs time-pessimised, at up to
8kHz rates; such are also less likely to have HPETs to use as timers.
And for that matter, many older and smaller boards are unlikely to be
running ACPI BIOS at all - which brings up another question, below ..
> To me, the former
> pseudocode is "incorrect" because it allows the possibility of another
> thread of execution mucking with a multibyte CMOS access by ACPIBIOS. I
> agree it's /probably/ unlikely tho.
I've no idea whether that may be possible, or if or how it might matter.
But all that's just me, the Time Lords are likely having a good laugh :)
Regarding systems without ACPI loaded, or active: what happens when the
below AcpiInstallAddressSpaceHandler() call fails, but returns 0? Would
not that prevent rtc_start() from running atrtc_start() etc for non-ACPI
clock initialisation and registration?
I suppose there's a global kernel variable for acpi_is_active ono?
> +static int
> rtc_start(struct eventtimer *et, sbintime_t first, sbintime_t period)
> {
>
> @@ -245,10 +323,17 @@
> int i;
>
> sc = device_get_softc(dev);
> +sc->acpi_handle = acpi_get_handle(dev);
> sc->port_res = bus_alloc_resource(dev, SYS_RES_IOPORT, &sc->port_rid,
> IO_RTC, IO_RTC + 1, 2, RF_ACTIVE);
> if (sc->port_res == NULL)
> device_printf(dev, "Warning: Couldn't map I/O.\n");
> +if (ACPI_FAILURE(AcpiInstallAddressSpaceHandler(sc->acpi_handle,
> +ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler, NULL, sc)))
> +{
> +device_printf(dev, "Error registering ACPI CMOS address space
> handler.\n");
> +return 0;
> +}
> atrtc_start();
> clock_register(dev, 100);
> bzero(&sc->et, sizeof(struct eventtimer));
> @@ -286,6 +371,15 @@
> return(0);
> }
Might that not matter for detach, as you're not testing for return code?
> +static int atrtc_detach(device_t dev)
> +{
> +struct atrtc_softc *sc;
> +
> +sc = device_get_softc(dev);
> +AcpiRemoveAddressSpaceHandler(sc->acpi_handle,
> ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler);
> +return bus_generic_detach(dev);
> +}
cheers, Ian
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On 03/01/2015 09:29 AM, Ian Smith wrote:
> On Fri, 27 Feb 2015 23:26:16 -0500, Anthony Jenkins wrote:
> > On 02/27/15 21:56, Bruce Evans wrote:
> > > On Sat, 28 Feb 2015, Ian Smith wrote:
>
> I'm going to contract this down to a couple of points, ok fellas?
>
> > > > Don't worry about any extra locking; this is going to be used at
> boot
> > > > and/or resume we assume;
> > > Bleah... I hate assumptions like that.
>
> So let's log everything at least while this is experimental in head,
> get lots of people using all sorts of hardware to report any
> surprises, and find out whenever else such service might be requested?
No problem adding logging. The bit I'm confused about (unless I'm
misunderstanding the argument) is why
# ugly pseudocode
function original_cmos_xfer():
lock();
transfer byte to/from CMOS
unlock();
function acpi_cmos_xfer():
foreach byte in num_bytes:
call original_cmos_xfer(byte)
end foreach
is preferable to
# ugly pseudocode
function acpi_cmos_xfer():
lock();
foreachbyte in num_bytes:
transfer byte to/from CMOS
end foreach
unlock();
function original_cmos_xfer():
call acpi_cmos_xfer(num_bytes = 1);
There was no "extra locking", but I did introduce an extra function call
which would slow down CMOS accesses to the RTC for some performance
timers; I assume that's the main complaint. To me, the former
pseudocode is "incorrect" because it allows the possibility of another
thread of execution mucking with a multibyte CMOS access by ACPIBIOS. I
agree it's /probably/ unlikely tho.
> Sure, add a sysctl hw.acpi.cmosaccess.STFU for when we find benign
> stuff being logged at other times than boot and suspend/resume, but
> when Jo Blo reports here or in questions@ or laptop@ that her Hidden
> Dragon Kneetop won't boot / suspend / resume / whatever with these
> funny lines in /var/log/messages, we'll know what it's about without
> undertaling the sort of research you had to do to get your HP Envy
> going, eh? :)
Agreed; just saving logging tweaks for last.
>
> % sysctl smithi.STFU=1# enough already ..
>
> > +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 >= 64)
> > +return AE_BAD_PARAMETER;
>
> Width 0 passes, and address 61 width 32 passes. Maybe simpler:
> int bytes;/* or size, whatever, above */
> bytes = width >> 3;
> and substitute 'bytes' subsequently, and here, perhaps:
>
> if (bytes < 1 || bytes > 4 || (width & 0x07) || address > (64 -
> bytes))
>
> > +if (!acpi_check_rtc_byteaccess(function == ACPI_READ, address))
> > +return AE_BAD_PARAMETER;
>
> acpi_check_rtc_byteaccess() needs to be called per byte of 1, 2 or 4
> bytes - or pass it 'bytes' also, and loop over each of them within?
Thought I had done/fixed that... I'll check.
> Failing this _really_ needs a printf, as below + 'REJECTED' ono, IMO.
Agreed.
Thanks,
Anthony
>
> > +
> > +switch (function) {
> > +case ACPI_READ:
> > +acpi_cmos_read(address, (UINT8 *)value, width
> >> 3);
> > +break;
> > +case ACPI_WRITE:
> > +acpi_cmos_write(address, (const UINT8 *)value,
> > +width >> 3);
> > +break;
> > +default:
> > +return AE_BAD_PARAMETER;
> > +}
> > +printf("%s: %-5s%02u address=%04lx value=%08x\n",
> > +__FUNCTION__, function == ACPI_READ ? "READ" : "WRITE",
> width >> 3,
> > +address, *((UINT32 *)value));
> > +return AE_OK;
> > +}
>
> cheers, Ian
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On Fri, 27 Feb 2015 23:26:16 -0500, Anthony Jenkins wrote:
> On 02/27/15 21:56, Bruce Evans wrote:
> > On Sat, 28 Feb 2015, Ian Smith wrote:
I'm going to contract this down to a couple of points, ok fellas?
> > > Don't worry about any extra locking; this is going to be used at boot
> > > and/or resume we assume;
>
> Bleah... I hate assumptions like that.
So let's log everything at least while this is experimental in head, get
lots of people using all sorts of hardware to report any surprises, and
find out whenever else such service might be requested?
Sure, add a sysctl hw.acpi.cmosaccess.STFU for when we find benign stuff
being logged at other times than boot and suspend/resume, but when Jo
Blo reports here or in questions@ or laptop@ that her Hidden Dragon
Kneetop won't boot / suspend / resume / whatever with these funny lines
in /var/log/messages, we'll know what it's about without undertaling the
sort of research you had to do to get your HP Envy going, eh? :)
% sysctl smithi.STFU=1 # enough already ..
> +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 >= 64)
> +return AE_BAD_PARAMETER;
Width 0 passes, and address 61 width 32 passes. Maybe simpler:
int bytes; /* or size, whatever, above */
bytes = width >> 3;
and substitute 'bytes' subsequently, and here, perhaps:
if (bytes < 1 || bytes > 4 || (width & 0x07) || address > (64 - bytes))
> +if (!acpi_check_rtc_byteaccess(function == ACPI_READ, address))
> +return AE_BAD_PARAMETER;
acpi_check_rtc_byteaccess() needs to be called per byte of 1, 2 or 4
bytes - or pass it 'bytes' also, and loop over each of them within?
Failing this _really_ needs a printf, as below + 'REJECTED' ono, IMO.
> +
> +switch (function) {
> +case ACPI_READ:
> +acpi_cmos_read(address, (UINT8 *)value, width >> 3);
> +break;
> +case ACPI_WRITE:
> +acpi_cmos_write(address, (const UINT8 *)value,
> +width >> 3);
> +break;
> +default:
> +return AE_BAD_PARAMETER;
> +}
> +printf("%s: %-5s%02u address=%04lx value=%08x\n",
> +__FUNCTION__, function == ACPI_READ ? "READ" : "WRITE", width >> 3,
> +address, *((UINT32 *)value));
> +return AE_OK;
> +}
cheers, Ian
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On 02/27/15 21:56, Bruce Evans wrote: On Sat, 28 Feb 2015, Ian Smith wrote: On Thu, 26 Feb 2015 10:35:57 -0500, Anthony Jenkins wrote: > On 02/25/2015 02:53 PM, John Baldwin wrote: > > On Monday, February 23, 2015 03:22:57 PM Anthony Jenkins wrote: [.. omissions reflecting change of subject ..] > >> I'd like to make the acpi_wmi(4) interface easier to use, but my backlog > >> of contributions I'm sitting on is only growing. > > I've been waiting to see if you were going to post an update to rev 3 of your > > CMOS patch after Ian's last round of feedback FWIW. Much of his feedback > > seemed relevant (and I know you've already accepted some other rounds of > > feedback on that patch before then, hence 'v3') > > > I am... I've just been stalling, mostly because it "works for me" and I > didn't understand some of the critiques, particularly the > "pessimization" one (over my head I think). I'll toss what I have out > there for further review; I'll shoot for today or tomorrow. > > One of the things I felt I had to do in the CMOS handler was allow ACPI > to perform multibyte accesses to the CMOS region, but the existing CMOS > read/write functions were only byte accessors, and each byte access was > locked. A multibyte access would lock, read/write a byte, unlock, lock, > read/write a byte, unlock So I wrote multibyte accessors (which had > some issues I think I corrected) and had the original RTC CMOS accessor > functions call the multibyte ones. The multibyte accessors performed > the locking, so a multibyte access would lock, read/write a byte, > read/write a byte..., unlock. > > I believe one of the recommendations was to "put it back the way it > was", which I did, along with failing any attempt by ACPIBIOS to access > multiple bytes. You can safely ignore the deeper and rather sideways discussion Bruce and I engaged in; I was bewildered by the idea of 'good pessimisations' too, which is why I pursued it, arguably too far, but I learned things. "Good pessimization" = a good thing to do if you want to pessimize the software to sell new hardware. Meh... sounds like it's bad (TM), whatever it is... However the takeaway was agreement that for multiple reasons, multibyte acpi_cmos access should loop on using the system rtcin() and writertc() as is - modulo your useful macros esp. IO_DELAY() documenting inb(0x84) - and that setting bit 7 on IO_RTC_ADDR, _possibly_ disabling NMI on some older hardware, is inadvisable. As you pointed out, PNP0B00 only wants to access CMOS from 0x00-0x3f anyway, so why not just mask reg with 0x3f while calling rtcin()/writertc() ? I don't really like IO_NDELAY() even though it is clearer. 386BSD had a macro for this, at least in asm code. It was named cryptically as DUMMY_NOPS. It was sprinkled ad nauseum for 3 reasons: - the author didn't understand some i386 complications related to the 8259 interrupt controller, and delays reduced the bugs - some delays were actually needed - some delays were used due to FUD. All these inb(0x84) delays have gone away except for a few in the RTC routines and 1 in the i8254 timer part DELAY(). The delays in the RTC routines are probably now FUD even if they were needed in 1990. The delay in DELAY() really is still needed, to fake DELAY(1) when the i8254 is unavailable locked out. There the code is ifdefed for pc98 so as to use 0x5f instead of 0x84. The RTC routines never used 0x5f or even avoided using 0x84. The delays were more needed in 1990 than now. Now the ISA bus is behind a PCI bus or two, or possibly faked. Accessing it tends to take about twice as long as in a 1990's system configured with minimal ISA delays, and modern hardware shouldn't need such long delays anyway, or can be smarter and force them iff necessary. I don't mind pulling the inb(0x84) calls, if we're assuming FreeBSD won't be running on circa 1990 hardware. I don't really want to maintain atrtc(4), just stick an ACPI window on it. I can if I must though. Don't worry about any extra locking; this is going to be used at boot and/or resume we assume; Bleah... I hate assumptions like that. atrtc_settime() for one is called by default every 1800 seconds if running ntpd, and it doesn't bother holding the lock through multiple writertc() calls. Any (optional) user of the RTC as an interval timer source, particularly at high rates, will appreciate the existing pre-selection of last register used, as Bruce explained. atrtc_settime() is too buggy to use except in emergency. One of its bugs is that it is sloppy about setting times. atrtc_gettime() (spelling?) is also sloppy about getting times. The combined error is about 1-2 seconds. So it is useless to set the clock unless it it has changed by more than 1 second. But in 1800 seconds, the RTC should not have drifted more than a few milliseconds. The next of its bugs is that it has no locking. Races from this can result in writing its registers inconsist
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On Sat, 28 Feb 2015, Ian Smith wrote: On Thu, 26 Feb 2015 10:35:57 -0500, Anthony Jenkins wrote: > On 02/25/2015 02:53 PM, John Baldwin wrote: > > On Monday, February 23, 2015 03:22:57 PM Anthony Jenkins wrote: [.. omissions reflecting change of subject ..] > >> I'd like to make the acpi_wmi(4) interface easier to use, but my backlog > >> of contributions I'm sitting on is only growing. > > I've been waiting to see if you were going to post an update to rev 3 of your > > CMOS patch after Ian's last round of feedback FWIW. Much of his feedback > > seemed relevant (and I know you've already accepted some other rounds of > > feedback on that patch before then, hence 'v3') > > > I am... I've just been stalling, mostly because it "works for me" and I > didn't understand some of the critiques, particularly the > "pessimization" one (over my head I think). I'll toss what I have out > there for further review; I'll shoot for today or tomorrow. > > One of the things I felt I had to do in the CMOS handler was allow ACPI > to perform multibyte accesses to the CMOS region, but the existing CMOS > read/write functions were only byte accessors, and each byte access was > locked. A multibyte access would lock, read/write a byte, unlock, lock, > read/write a byte, unlock So I wrote multibyte accessors (which had > some issues I think I corrected) and had the original RTC CMOS accessor > functions call the multibyte ones. The multibyte accessors performed > the locking, so a multibyte access would lock, read/write a byte, > read/write a byte..., unlock. > > I believe one of the recommendations was to "put it back the way it > was", which I did, along with failing any attempt by ACPIBIOS to access > multiple bytes. You can safely ignore the deeper and rather sideways discussion Bruce and I engaged in; I was bewildered by the idea of 'good pessimisations' too, which is why I pursued it, arguably too far, but I learned things. "Good pessimization" = a good thing to do if you want to pessimize the software to sell new hardware. However the takeaway was agreement that for multiple reasons, multibyte acpi_cmos access should loop on using the system rtcin() and writertc() as is - modulo your useful macros esp. IO_DELAY() documenting inb(0x84) - and that setting bit 7 on IO_RTC_ADDR, _possibly_ disabling NMI on some older hardware, is inadvisable. As you pointed out, PNP0B00 only wants to access CMOS from 0x00-0x3f anyway, so why not just mask reg with 0x3f while calling rtcin()/writertc() ? I don't really like IO_NDELAY() even though it is clearer. 386BSD had a macro for this, at least in asm code. It was named cryptically as DUMMY_NOPS. It was sprinkled ad nauseum for 3 reasons: - the author didn't understand some i386 complications related to the 8259 interrupt controller, and delays reduced the bugs - some delays were actually needed - some delays were used due to FUD. All these inb(0x84) delays have gone away except for a few in the RTC routines and 1 in the i8254 timer part DELAY(). The delays in the RTC routines are probably now FUD even if they were needed in 1990. The delay in DELAY() really is still needed, to fake DELAY(1) when the i8254 is unavailable locked out. There the code is ifdefed for pc98 so as to use 0x5f instead of 0x84. The RTC routines never used 0x5f or even avoided using 0x84. The delays were more needed in 1990 than now. Now the ISA bus is behind a PCI bus or two, or possibly faked. Accessing it tends to take about twice as long as in a 1990's system configured with minimal ISA delays, and modern hardware shouldn't need such long delays anyway, or can be smarter and force them iff necessary. Don't worry about any extra locking; this is going to be used at boot and/or resume we assume; atrtc_settime() for one is called by default every 1800 seconds if running ntpd, and it doesn't bother holding the lock through multiple writertc() calls. Any (optional) user of the RTC as an interval timer source, particularly at high rates, will appreciate the existing pre-selection of last register used, as Bruce explained. atrtc_settime() is too buggy to use except in emergency. One of its bugs is that it is sloppy about setting times. atrtc_gettime() (spelling?) is also sloppy about getting times. The combined error is about 1-2 seconds. So it is useless to set the clock unless it it has changed by more than 1 second. But in 1800 seconds, the RTC should not have drifted more than a few milliseconds. The next of its bugs is that it has no locking. Races from this can result in writing its registers inconsistently. This makes witing to it unless it has changed by more than 1 second worse than useless. Except the next write is likely to fix it up after losing a race on the previous write. It would be a reasonable fix to read after write to check that the write worked. This only loses if the system crashes just after a write that doesn't work or if another process reads
Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]
On Thu, 26 Feb 2015 10:35:57 -0500, Anthony Jenkins wrote:
> On 02/25/2015 02:53 PM, John Baldwin wrote:
> > On Monday, February 23, 2015 03:22:57 PM Anthony Jenkins wrote:
[.. omissions reflecting change of subject ..]
> >> I'd like to make the acpi_wmi(4) interface easier to use, but my backlog
> >> of contributions I'm sitting on is only growing.
> > I've been waiting to see if you were going to post an update to rev 3 of
> > your
> > CMOS patch after Ian's last round of feedback FWIW. Much of his feedback
> > seemed relevant (and I know you've already accepted some other rounds of
> > feedback on that patch before then, hence 'v3')
> >
> I am... I've just been stalling, mostly because it "works for me" and I
> didn't understand some of the critiques, particularly the
> "pessimization" one (over my head I think). I'll toss what I have out
> there for further review; I'll shoot for today or tomorrow.
>
> One of the things I felt I had to do in the CMOS handler was allow ACPI
> to perform multibyte accesses to the CMOS region, but the existing CMOS
> read/write functions were only byte accessors, and each byte access was
> locked. A multibyte access would lock, read/write a byte, unlock, lock,
> read/write a byte, unlock So I wrote multibyte accessors (which had
> some issues I think I corrected) and had the original RTC CMOS accessor
> functions call the multibyte ones. The multibyte accessors performed
> the locking, so a multibyte access would lock, read/write a byte,
> read/write a byte..., unlock.
>
> I believe one of the recommendations was to "put it back the way it
> was", which I did, along with failing any attempt by ACPIBIOS to access
> multiple bytes.
You can safely ignore the deeper and rather sideways discussion Bruce
and I engaged in; I was bewildered by the idea of 'good pessimisations'
too, which is why I pursued it, arguably too far, but I learned things.
However the takeaway was agreement that for multiple reasons, multibyte
acpi_cmos access should loop on using the system rtcin() and writertc()
as is - modulo your useful macros esp. IO_DELAY() documenting inb(0x84)
- and that setting bit 7 on IO_RTC_ADDR, _possibly_ disabling NMI on
some older hardware, is inadvisable. As you pointed out, PNP0B00 only
wants to access CMOS from 0x00-0x3f anyway, so why not just mask reg
with 0x3f while calling rtcin()/writertc() ?
Don't worry about any extra locking; this is going to be used at boot
and/or resume we assume; atrtc_settime() for one is called by default
every 1800 seconds if running ntpd, and it doesn't bother holding the
lock through multiple writertc() calls. Any (optional) user of the RTC
as an interval timer source, particularly at high rates, will appreciate
the existing pre-selection of last register used, as Bruce explained.
I'm unsure whether any stats or profiling still uses the RTC at all?
kern.clockrate: { hz = 1000, tick = 1000, profhz = 8126, stathz = 127 }
So I would suggest:
. reading any bytes except 0x0c is safe (though in the case of time or
date bytes, possibly invalid if read while what the datasheet calls
the UIP bit is set):
#define RTC_STATUSA 0x0a/* status register A */
#define RTCSA_TUP 0x80 /* time update, don't look now */
. the only conceivable bytes permissable to allow writing are:
. below 0x10: bytes 1, 3 and 5 (alarm seconds, minute, hour). While
they're no use whilever we don't support wake on alarm and should
also be written during non-update periods, should be safe enough.
. 0x10 - 0x2f: none, unless you're prepared to copy the procedure in
/sys/dev/nvram/nvram.c to calculate and write the checksum to 0x2e
and 0x2f. I think we should just deny and log such access, unless
and until someone shows log messages demonstrating a perceived need.
. 0x30 through 0x3f: I guess you could allow write access, at least I
haven't heard of anything using that area for anything, but check ..
I recall reading that we don't use this, at least on x86:
#define RTC_CENTURY 0x32/* current century */
. in any case, log all access, accepted or denied, at kern.notice ono.
> acpi_rtc_cmos_handler: READ 01 address=0006 value=0006
> acpi_rtc_cmos_handler: READ 01 address=0004 value=0015
> acpi_rtc_cmos_handler: READ 01 address=0002 value=0006
Bruce called them ugly, but I'm happy such access is so obvious ..
> I think the reason behind having an ACPI CMOS handler is to give the OS
> a say when ACPIBIOS wants to access CMOS, to prevent it from stepping on
> the toes of an RTC CMOS driver who's also twiddling CMOS registers and
> (presumably) knows the state of the device.
Indeed. Virtually all of the state, apart from the default reset state,
is stored in RTC status/control registers, though different consumers
presumably know more. I haven't explored the code that may use the RTC
as an eve
Re: [PATCH] ACPI CMOS region support rev. 3
On Fri, 29 Aug 2014 11:30:49 -0400, Anthony Jenkins wrote:
> Okay here's another revision of the patch, hopefully I made good
> progress in cleaning up the style and addressing everyone's
> concerns. Comments/criticisms welcome as always!
>
> Patches:
> * Current: http://www.qtchat.org/docs/atrtc.c-20140829.patch
> * Previous: http://www.qtchat.org/docs/atrtc.c.patch
> Changes:
> * Removed 'U' suffix from integer literals.
> * Ensured all (my) lines were <= 40 columns
> * Changed continuation indent to 4 columns.
> * Replaced is_datetime_reg() with more generic acpi_check_rtc_byteaccess()
> to encapsulate (deeper) validation
> of ACPI CMOS accesses.
> * Replaced some ACPI Windows-like typenames with FreeBSD typenames where
> possible.
> Booting my HP Envy 14 laptop and suspend/resuming shows the only ACPI CMOS
> accesses are byte reads of registers
> 0x06, 0x04 and 0x02:
>
> uhub4: at usbus0, port 1, addr 1 (disconnected)
> ugen0.2: at usbus0 (disconnected)
> ums0: at uhub4, port 3, addr 1 (disconnected)
> uhid0: at uhub4, port 3, addr 1 (disconnected)
> uhub3: at usbus1, port 1, addr 1 (disconnected)
> uhub2: at usbus2, port 1, addr 1 (disconnected)
> ugen2.2: at usbus2 (disconnected)
> urtwn0: at uhub2, port 1, addr 2 (disconnected)
> uhub1: at usbus3, port 1, addr 1 (disconnected)
> ugen3.2: at usbus3 (disconnected)
> ubt0: at uhub1, port 4, addr 2 (disconnected)
> uhub0: at usbus4, port 1, addr 1 (disconnected)
> ugen4.2: at usbus4 (disconnected)
> acpi_rtc_cmos_handler: READ 01 address=0006 value=0006
> acpi_rtc_cmos_handler: READ 01 address=0004 value=0015
> acpi_rtc_cmos_handler: READ 01 address=0002 value=0006
> vgapci0: child drmn0 requested pci_set_powerstate
> info: [drm] PCIE GART of 512M enabled (table at 0x0004).
> drmn0: info: WB enabled
> drmn0: info: fence driver on ring 0 use gpu addr 0x2c00
> and cpu addr 0x0xf800b686cc00
> drmn0: info: fence driver on ring 1 use gpu addr 0x2c04
> and cpu addr 0x0xf800b686cc04
> drmn0: info: fence driver on ring 2 use gpu addr 0x2c08
> and cpu addr 0x0xf800b686cc08
> drmn0: info: fence driver on ring 3 use gpu addr 0x2c0c
> and cpu addr 0x0xf800b686cc0c
> drmn0: info: fence driver on ring 4 use gpu addr 0x2c10
> and cpu addr 0x0xf800b686cc10
> info: [drm] ring test on 0 succeeded in 3 usecs
> info: [drm] ring test on 3 succeeded in 2 usecs
> info: [drm] ring test on 4 succeeded in 2 usecs
> info: [drm] ib test on ring 0 succeeded in 0 usecs
> info: [drm] ib test on ring 3 succeeded in 0 usecs
> info: [drm] ib test on ring 4 succeeded in 0 usecs
> re0: link state changed to DOWN
> psm0: system resume hook called.
> psm0: current command byte: 0065 (reinitialize).
Ok, good knowing what your HP Envy 14 wants to know; minute, hour and
day of week. You'd said that without this it immediately resumed from
suspend; I suppose that it might want to calculate (within a week) how
long it had been suspended or when when it should awaken - perhaps to do
with the wake on alarm settings in BIOS setup (?) - and that failing to
retrieve those made it decide it was best (or due) to resume. Rather
odd, when it would also need to read day, hour and minute on resuming to
compare, but apparently doesn't do so - or at least, not via ACPI ..
> Backlight still doesn't resume, but can still ssh(1) into laptop.
I'd tend to treat and pursue this as a separate problem; it's been a
common issue on a number of laptops. Maybe try freebsd-mobile@ too?
> Thanks,
> Anthony Jenkins
Before quoting some issues with your patch, I'll say what I think:
0) It would be better to leave the system rtcin() and writertc() more
or less as is, including its present I/O timing and the optimisation for
repetitive access, allowing (eg) use of RTC periodic interrupts at high
rates, as Bruce discussed (albeit dismissively :) WRT profiling. Your
IODELAY() macro is useful documentation, I had to hunt that up myself.
I'm wondering if mav@ (cc'd) has anything to say about all this?
There are 18 calls to each of rtcin() and writertc() just within
atrtc.c itself, and other callers (in 9.3 sources) in:
smithi@x200:/sys % find . -type f -exec egrep -l 'rtcin\(|writertc\(' {} \; |
uniq
/sys/isa/rtc.h
/sys/mips/malta/malta_machdep.c
/sys/i386/xen/clock.c
/sys/i386/i386/machdep.c
/sys/dev/fb/vga.c
/sys/dev/acpi_support/acpi_ibm.c
/sys/dev/nvram/nvram.c
/sys/dev/fdc/fdc.c
/sys/x86/isa/atrtc.c
/sys/pc98/cbus/fdc.c
/sys/dev/fb/vga.c depends on 2 bits in RTC_EQUIPMENT which is defined in
that file - albeit for older VGA/EGA/CGA systems - as:
/* this should really be in `rtc.h' */
#define RTC_EQUIPMENT
