Date: Tue, 03 Mar 2015 11:45:49 -0500 From: Anthony Jenkins <Anthony.B.Jenkins@att.net> To: Ian Smith <smithi@nimnet.asn.au> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] Message-ID: <54F5E53D.1090601@att.net> In-Reply-To: <20150302002647.W42658@sola.nimnet.asn.au> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54F5E53D.1090601>