Date: Fri, 6 Mar 2015 22:49:19 +1100 (EST) From: Ian Smith <smithi@nimnet.asn.au> To: Anthony Jenkins <Anthony.B.Jenkins@att.net> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] Message-ID: <20150306025800.U46361@sola.nimnet.asn.au> In-Reply-To: <54F5E53D.1090601@att.net> References: <20150222180817.GD27984@strugglingcoder.info> <54EB8C21.2080600@att.net> <2401337.2oUs7iAbtB@ralph.baldwin.cx> <54EF3D5D.4010106@att.net> <20150227222203.P38620@sola.nimnet.asn.au> <20150228125857.D1277@besplex.bde.org> <54F14368.4020807@att.net> <20150302002647.W42658@sola.nimnet.asn.au> <54F5E53D.1090601@att.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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, 1000000);
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150306025800.U46361>