Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-19 Thread Ian Smith
On Wed, 18 Mar 2015 15:30:23 -0600, Warner Losh wrote:
   On Mar 18, 2015, at 10:06 AM, Anthony Jenkins anthony.b.jenk...@att.net 
   wrote:
   
   On 03/18/2015 11:29 AM, Warner Losh wrote:
   On Mar 17, 2015, at 7:02 AM, Anthony Jenkins anthony.b.jenk...@att.net 
   wrote:
   \Where else might ATRTC_VERBOSE be set otherwise?
   I'm picturing a (future?) config(5) knob, e.g.
   
 device atrtc
 options ATRTC_VERBOSE=1
   
   
   so it can be set at compile time.
   Why not just boot verbose? history has shown too many options like
   this is hard to use.

You can blame this on me :)  I agree about the option not being needed; 
the way it is you can just set sysctl hw.acpi.atrtc_verbose=0 to quell 
reports of successful access, if it turns out these are routine on some 
machines, especially outside of boot/suspend/resume contexts.

However I'll still argue that, this being a new gadget and that we could 
use finding out which vendors want to read or write which locations in 
CMOS for whatever reason, at least while it's in head, we should log all 
access by default unless setting atrtc_verbose=0, and in _any_ case we 
should be logging attempts to R/W out-of-bounds CMOS locations.

   I think I understand what you're saying... I also prefer fewer config(5)
   knobs.  So you're suggesting I determine (at runtime) the boot verbose
   setting (kenv(2) or however it's properly done) and dump the
   compile-time verbosity setting?
  
  if (bootverbose)
   do verbose things;
  
  is how thatÿÿs done.

Sure, and maybe successful access could be limited to bootverbose, and 
we could ask people whose boxes fail to boot/suspend/resume/whatever to 
boot verbose to reveal such as why Anthony's HP Envy either failed to 
suspend or immediately resumed - which isn't entirely clear, even with 
the messages - unless its ACPI AML succeeded in reading minute, hour and 
weekday, but I have a feeling we may see more of this sort of thing.

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. 5

2015-03-19 Thread Ian Smith
On Thu, 19 Mar 2015 09:11:08 -0400, Anthony Jenkins wrote:
  On 03/19/2015 04:10 AM, Ian Smith wrote:
   On Wed, 18 Mar 2015 15:30:23 -0600, Warner Losh wrote:
  On Mar 18, 2015, at 10:06 AM, Anthony Jenkins 
   anthony.b.jenk...@att.net wrote:
  
  On 03/18/2015 11:29 AM, Warner Losh wrote:
  On Mar 17, 2015, at 7:02 AM, Anthony Jenkins 
   anthony.b.jenk...@att.net wrote:
  \Where else might ATRTC_VERBOSE be set otherwise?
  I'm picturing a (future?) config(5) knob, e.g.
  
device atrtc
options ATRTC_VERBOSE=1
  
  
  so it can be set at compile time.
  Why not just boot verbose? history has shown too many options like
  this is hard to use.
  
   You can blame this on me :)  I agree about the option not being needed; 
   the way it is you can just set sysctl hw.acpi.atrtc_verbose=0 to quell 
   reports of successful access, if it turns out these are routine on some 
   machines, especially outside of boot/suspend/resume contexts.
  
   However I'll still argue that, this being a new gadget and that we could 
   use finding out which vendors want to read or write which locations in 
   CMOS for whatever reason, at least while it's in head, we should log all 
   access by default unless setting atrtc_verbose=0,
  
  So the default verbosity of ACPI CMOS region accesses should be
  verbose?  I personally don't mind the default being silent and
  asking people triaging an ACPI problem to boot verbosely and send the
  logs (I think that's in the FreeBSD ACPI handbook anyway).

Assuming they know that their problem is ACPI related, sure.

   and in _any_ case we 
   should be logging attempts to R/W out-of-bounds CMOS locations.
  
  Error logs are always printed; they don't honor atrtc_verbose.

That would be comforting.  However I was referring to rev. 5:

+   if (bitwidth == 0 || bitwidth  32 || (bitwidth  0x07) ||
+   addr + bytewidth - 1  63) {
+   ATRTC_DBG_PRINTF(1,
+   Invalid bitwidth (%u) or addr (0x%08lx).\n,
+   bitwidth, addr);
+   return AE_BAD_PARAMETER;
+   }
+   if (!acpi_check_rtc_access(func == ACPI_READ, addr, bytewidth)) {
+   ATRTC_DBG_PRINTF(1, Bad CMOS %s access at addr 0x%08lx.\n,
+   func == ACPI_READ ? read : write, addr);
+   return AE_BAD_PARAMETER;
+   }

  I think I understand what you're saying... I also prefer fewer 
   config(5)
  knobs.  So you're suggesting I determine (at runtime) the boot verbose
  setting (kenv(2) or however it's properly done) and dump the
  compile-time verbosity setting?
 
 if (bootverbose)
 do verbose things;
 
 is how thatÿÿs done.
  
   Sure, and maybe successful access could be limited to bootverbose, and 
   we could ask people whose boxes fail to boot/suspend/resume/whatever to 
   boot verbose to reveal such as why Anthony's HP Envy either failed to 
   suspend or immediately resumed - which isn't entirely clear, even with 
   the messages - unless its ACPI AML succeeded in reading minute, hour and 
   weekday, but I have a feeling we may see more of this sort of thing.
  
  Now that I think about it, adding this ACPI CMOS region access should
  simply eliminate a class of failures where FreeBSD wasn't giving the
  BIOS access to CMOS.

Do we have other examples than your HP Envy of such a class of failures?

  Logging /successful/  R/W accesses to CMOS by the
  BIOS (AML) won't really provide any useful info (IMHO), but the user can
  flip on bootverbose if she's curious.  If a user's box fails to
  boot/suspend/resume/whatever, we'll see any ACPI CMOS region access errors.

Well I've made a case otherwise, likely too avidly; I'll leave it there.

Thanks for flushing out this issue and doing something about it!

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. 5

2015-03-19 Thread Anthony Jenkins
On 03/19/2015 09:49 AM, Ian Smith wrote:
 On Thu, 19 Mar 2015 09:11:08 -0400, Anthony Jenkins wrote:
   On 03/19/2015 04:10 AM, Ian Smith wrote:
On Wed, 18 Mar 2015 15:30:23 -0600, Warner Losh wrote:
   On Mar 18, 2015, at 10:06 AM, Anthony Jenkins 
 anthony.b.jenk...@att.net wrote:
   
   On 03/18/2015 11:29 AM, Warner Losh wrote:
   On Mar 17, 2015, at 7:02 AM, Anthony Jenkins 
 anthony.b.jenk...@att.net wrote:
   \Where else might ATRTC_VERBOSE be set otherwise?
   I'm picturing a (future?) config(5) knob, e.g.
   
 device atrtc
 options ATRTC_VERBOSE=1
   
   
   so it can be set at compile time.
   Why not just boot verbose? history has shown too many options like
   this is hard to use.
   
You can blame this on me :)  I agree about the option not being needed; 
the way it is you can just set sysctl hw.acpi.atrtc_verbose=0 to quell 
reports of successful access, if it turns out these are routine on some 
machines, especially outside of boot/suspend/resume contexts.
   
However I'll still argue that, this being a new gadget and that we could 
use finding out which vendors want to read or write which locations in 
CMOS for whatever reason, at least while it's in head, we should log all 
access by default unless setting atrtc_verbose=0,
   
   So the default verbosity of ACPI CMOS region accesses should be
   verbose?  I personally don't mind the default being silent and
   asking people triaging an ACPI problem to boot verbosely and send the
   logs (I think that's in the FreeBSD ACPI handbook anyway).

 Assuming they know that their problem is ACPI related, sure.

and in _any_ case we 
should be logging attempts to R/W out-of-bounds CMOS locations.
   
   Error logs are always printed; they don't honor atrtc_verbose.

 That would be comforting.  However I was referring to rev. 5:

 +   if (bitwidth == 0 || bitwidth  32 || (bitwidth  0x07) ||
 +   addr + bytewidth - 1  63) {
 +   ATRTC_DBG_PRINTF(1,
 +   Invalid bitwidth (%u) or addr (0x%08lx).\n,
 +   bitwidth, addr);
 +   return AE_BAD_PARAMETER;
 +   }
 +   if (!acpi_check_rtc_access(func == ACPI_READ, addr, bytewidth)) {
 +   ATRTC_DBG_PRINTF(1, Bad CMOS %s access at addr 0x%08lx.\n,
 +   func == ACPI_READ ? read : write, addr);
 +   return AE_BAD_PARAMETER;
 +   }

Ahh you're right, my bad.  These are /supposed/ to be simple printf()s
(or ATRTC_DBG_PRINTF(0, ...)s, but I think this would trigger a
unsigned int = 0 is always true warning).

   I think I understand what you're saying... I also prefer fewer 
 config(5)
   knobs.  So you're suggesting I determine (at runtime) the boot 
 verbose
   setting (kenv(2) or however it's properly done) and dump the
   compile-time verbosity setting?
  
  if (bootverbose)
do verbose things;
  
  is how thatÿÿs done.
   
Sure, and maybe successful access could be limited to bootverbose, and 
we could ask people whose boxes fail to boot/suspend/resume/whatever to 
boot verbose to reveal such as why Anthony's HP Envy either failed to 
suspend or immediately resumed - which isn't entirely clear, even with 
the messages - unless its ACPI AML succeeded in reading minute, hour and 
weekday, but I have a feeling we may see more of this sort of thing.
   
   Now that I think about it, adding this ACPI CMOS region access should
   simply eliminate a class of failures where FreeBSD wasn't giving the
   BIOS access to CMOS.

 Do we have other examples than your HP Envy of such a class of failures?

Oh definitely!  You should be able to search for my name in this
newsgroup and find instances where I've manually provided the patch to
someone having suspend/resume issues and they've been resolved.  One has
subject Impossible shutdown circa 2014-June-26.

Anthony

   Logging /successful/  R/W accesses to CMOS by the
   BIOS (AML) won't really provide any useful info (IMHO), but the user can
   flip on bootverbose if she's curious.  If a user's box fails to
   boot/suspend/resume/whatever, we'll see any ACPI CMOS region access errors.

 Well I've made a case otherwise, likely too avidly; I'll leave it there.

 Thanks for flushing out this issue and doing something about it!

 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. 5

2015-03-19 Thread Anthony Jenkins
On 03/19/2015 04:10 AM, Ian Smith wrote:
 On Wed, 18 Mar 2015 15:30:23 -0600, Warner Losh wrote:
On Mar 18, 2015, at 10:06 AM, Anthony Jenkins 
 anthony.b.jenk...@att.net wrote:

On 03/18/2015 11:29 AM, Warner Losh wrote:
On Mar 17, 2015, at 7:02 AM, Anthony Jenkins 
 anthony.b.jenk...@att.net wrote:
\Where else might ATRTC_VERBOSE be set otherwise?
I'm picturing a (future?) config(5) knob, e.g.

  device atrtc
  options ATRTC_VERBOSE=1


so it can be set at compile time.
Why not just boot verbose? history has shown too many options like
this is hard to use.

 You can blame this on me :)  I agree about the option not being needed; 
 the way it is you can just set sysctl hw.acpi.atrtc_verbose=0 to quell 
 reports of successful access, if it turns out these are routine on some 
 machines, especially outside of boot/suspend/resume contexts.

 However I'll still argue that, this being a new gadget and that we could 
 use finding out which vendors want to read or write which locations in 
 CMOS for whatever reason, at least while it's in head, we should log all 
 access by default unless setting atrtc_verbose=0,

So the default verbosity of ACPI CMOS region accesses should be
verbose?  I personally don't mind the default being silent and
asking people triaging an ACPI problem to boot verbosely and send the
logs (I think that's in the FreeBSD ACPI handbook anyway).

 and in _any_ case we 
 should be logging attempts to R/W out-of-bounds CMOS locations.

Error logs are always printed; they don't honor atrtc_verbose.

I think I understand what you're saying... I also prefer fewer config(5)
knobs.  So you're suggesting I determine (at runtime) the boot verbose
setting (kenv(2) or however it's properly done) and dump the
compile-time verbosity setting?
   
   if (bootverbose)
  do verbose things;
   
   is how thatÿÿs done.

 Sure, and maybe successful access could be limited to bootverbose, and 
 we could ask people whose boxes fail to boot/suspend/resume/whatever to 
 boot verbose to reveal such as why Anthony's HP Envy either failed to 
 suspend or immediately resumed - which isn't entirely clear, even with 
 the messages - unless its ACPI AML succeeded in reading minute, hour and 
 weekday, but I have a feeling we may see more of this sort of thing.

Now that I think about it, adding this ACPI CMOS region access should
simply eliminate a class of failures where FreeBSD wasn't giving the
BIOS access to CMOS.  Logging /successful/  R/W accesses to CMOS by the
BIOS (AML) won't really provide any useful info (IMHO), but the user can
flip on bootverbose if she's curious.  If a user's box fails to
boot/suspend/resume/whatever, we'll see any ACPI CMOS region access errors.

Anthony

 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. 5

2015-03-19 Thread Anthony Jenkins
On 03/18/2015 05:32 PM, Warner Losh wrote:
 On Mar 18, 2015, at 9:59 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:

 On 03/18/2015 11:26 AM, Warner Losh wrote:
 Looking at patch 5:

 You need to rework this so there’s an atrtc_acpi.c. Put all the ACPI 
 attachment in there.
 Okay, shouldn't be a problem.

 You should also split off the little bit that’s ISA-specific into atrtc_isa.
 You mean rtcin() and writertc()? ...but that's not my stuff, it was
 already in atrtc.c.  PNP0800 (the PC-AT RTC component) is (practically)
 an ISA-specific device.
 There will be a small “isa” specific driver. Here ISA means ‘FreeBSD ISA 
 attachment’ not
 necessarily what you’d find on an ISA bus. This means you’d have two separate 
 driver
 statement. You can then do the ACPI stuff in the ACPI attachment and not have 
 to worry
 about whether or not it is compiled into the kernel, since you’d only include 
 it if acpi is
 in the kernel.

Okay I'll look for a simple example in the existing drivers...any
suggestions?

I was just hoping to leave atrtc(4) as intact as possible, adding an
ACPI accessor for BIOSes that need it; this sounds like a reworking of
atrtc(4)...

Anthony


 Warner


___
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. 5

2015-03-18 Thread Anthony Jenkins
On 03/18/2015 11:29 AM, Warner Losh wrote:
 On Mar 17, 2015, at 7:02 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:
 \Where else might ATRTC_VERBOSE be set otherwise?
 I'm picturing a (future?) config(5) knob, e.g.

device atrtc
options ATRTC_VERBOSE=1


 so it can be set at compile time.
 Why not just boot verbose? history has shown too many options like
 this is hard to use.

I think I understand what you're saying... I also prefer fewer config(5)
knobs.  So you're suggesting I determine (at runtime) the boot verbose
setting (kenv(2) or however it's properly done) and dump the
compile-time verbosity setting?

Thanks,
Anthony

 I still wonder if there isn't a global acpi_loaded_and_running variable
 so you could avoid even attempting ACPI init calls, perhaps making this
 not so dependent on ACPI, at least at runtime.
 I haven't (yet) been able to find a compile-time flag that tells me if
 the kernel supports ACPI; I'm /pretty/ sure the ACPI headers I'm
 #include'ing will exist for every build of FreeBSD.
 I wouldn’t count on it. However, they may exist for all platforms that
 use atrtc.

 But it wouldn’t be an issue if you did a separate attachment.

 Warner


___
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. 5

2015-03-18 Thread Warner Losh

 On Mar 17, 2015, at 7:02 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:
 \Where else might ATRTC_VERBOSE be set otherwise?
 
 I'm picturing a (future?) config(5) knob, e.g.
 
device atrtc
options ATRTC_VERBOSE=1
 
 
 so it can be set at compile time.

Why not just boot verbose? history has shown too many options like
this is hard to use.

 I still wonder if there isn't a global acpi_loaded_and_running variable
 so you could avoid even attempting ACPI init calls, perhaps making this
 not so dependent on ACPI, at least at runtime.
 
 I haven't (yet) been able to find a compile-time flag that tells me if
 the kernel supports ACPI; I'm /pretty/ sure the ACPI headers I'm
 #include'ing will exist for every build of FreeBSD.

I wouldn’t count on it. However, they may exist for all platforms that
use atrtc.

But it wouldn’t be an issue if you did a separate attachment.

Warner



signature.asc
Description: Message signed with OpenPGP using GPGMail


Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-18 Thread Warner Losh
Looking at patch 5:

You need to rework this so there’s an atrtc_acpi.c. Put all the ACPI attachment 
in there. You should also split off the little bit that’s ISA-specific into 
atrtc_isa. Once you do that, we can talk.

Warner



signature.asc
Description: Message signed with OpenPGP using GPGMail


Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-18 Thread Anthony Jenkins
On 03/18/2015 11:26 AM, Warner Losh wrote:
 Looking at patch 5:

 You need to rework this so there’s an atrtc_acpi.c. Put all the ACPI 
 attachment in there.

Okay, shouldn't be a problem.

 You should also split off the little bit that’s ISA-specific into atrtc_isa.

You mean rtcin() and writertc()? ...but that's not my stuff, it was
already in atrtc.c.  PNP0800 (the PC-AT RTC component) is (practically)
an ISA-specific device.

Anthony

 Once you do that, we can talk.

 Warner


___
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. 5

2015-03-18 Thread Jung-uk Kim
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 03/17/2015 08:28, Ian Smith wrote:
 I still wonder if there isn't a global acpi_loaded_and_running
 variable so you could avoid even attempting ACPI init calls,
 perhaps making this not so dependent on ACPI, at least at runtime.

For runtime, power_pm_get_type() is what you looking for.  For example,

switch (power_pm_get_type()) {
case POWER_PM_TYPE_ACPI:/* Do something specific to ACPI. */
case POWER_PM_TYPE_APM: /* Do something specific to APM. */
default:/* Do something without PM. */
}

 But perhaps jkim's concern is more regarding building on platforms
 not supporting ACPI at all? Is that the (only?) issue with this on
 ARM?

sys/x86/isa/atrtc.c is only for x86 (excluding pc98).  I am only
concerned about ACPI-less i386 kernel at this point.

Jung-uk Kim
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQEcBAEBCAAGBQJVCbdVAAoJEHyflib82/FGK04H/2e/DVefzoorkEuW5sxgHqGg
XGFB21wLxP4bfnkkGlTfYrEPkdB53zW6qez2nUv+zA4aTy/BTpmRN0KAhwMRCkJj
QjM757IoQr+QyWQhU62NOsu7Ox86MI6RBrPssURuwib8HWJbIUPDKKYmK+sXI7Bq
UmlBJeiK0BhzCQ7l0tIaR6VFlQSxMQC/x/fwkHI9hKPyKwq8ACeqQ2ZI05v6ZQzo
IIfVU0LLz62kDoJDicaRNfJbGtRPOvx4Nnm1RE8wVtaqlwQYrffp6QpHaRfXHEos
QwWEWXrMFfjQtCH+KCrzfZsCQD1rTe+eDb0tFD315PbpvEs6yG6VlBxf4pUJRAU=
=YDkP
-END PGP SIGNATURE-
___
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. 5

2015-03-18 Thread Warner Losh

 On Mar 18, 2015, at 9:59 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:
 
 On 03/18/2015 11:26 AM, Warner Losh wrote:
 Looking at patch 5:
 
 You need to rework this so there’s an atrtc_acpi.c. Put all the ACPI 
 attachment in there.
 
 Okay, shouldn't be a problem.
 
 You should also split off the little bit that’s ISA-specific into atrtc_isa.
 
 You mean rtcin() and writertc()? ...but that's not my stuff, it was
 already in atrtc.c.  PNP0800 (the PC-AT RTC component) is (practically)
 an ISA-specific device.

There will be a small “isa” specific driver. Here ISA means ‘FreeBSD ISA 
attachment’ not
necessarily what you’d find on an ISA bus. This means you’d have two separate 
driver
statement. You can then do the ACPI stuff in the ACPI attachment and not have 
to worry
about whether or not it is compiled into the kernel, since you’d only include 
it if acpi is
in the kernel.

Warner



signature.asc
Description: Message signed with OpenPGP using GPGMail


Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-18 Thread Warner Losh

 On Mar 18, 2015, at 10:06 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:
 
 On 03/18/2015 11:29 AM, Warner Losh wrote:
 On Mar 17, 2015, at 7:02 AM, Anthony Jenkins anthony.b.jenk...@att.net 
 wrote:
 \Where else might ATRTC_VERBOSE be set otherwise?
 I'm picturing a (future?) config(5) knob, e.g.
 
   device atrtc
   options ATRTC_VERBOSE=1
 
 
 so it can be set at compile time.
 Why not just boot verbose? history has shown too many options like
 this is hard to use.
 
 I think I understand what you're saying... I also prefer fewer config(5)
 knobs.  So you're suggesting I determine (at runtime) the boot verbose
 setting (kenv(2) or however it's properly done) and dump the
 compile-time verbosity setting?

if (bootverbose)
do verbose things;

is how that’s done.

Warner



signature.asc
Description: Message signed with OpenPGP using GPGMail


Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-17 Thread Ian Smith
On Mon, 16 Mar 2015 15:51:30 -0400, Anthony Jenkins wrote:
  On 03/16/2015 01:49 PM, Ian Smith wrote:
   On Mon, 16 Mar 2015 11:50:59 -0400, Anthony Jenkins wrote:
 On 03/16/2015 11:00 AM, Anthony Jenkins wrote:
  On 03/16/2015 09:59 AM, Ian Smith wrote:
  On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote:
  +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?
  ===
 
  Otherwise (for example) a 2 byte read from 0x0b or 4 byte read from 
  0x09-0x0b will read 0x0c (clearing interrupts), or a 2 or 4 byte 
   write 
  to (say) 0x01 will also write to 0x02 and 0x04 (clobbering the time).
  Right, this is an (incorrect) hybrid of a few attempts, probably from
  around the time I lost my SSD and only had a single backup copy of my
  work to go from.  In one revision I had disallowed all multibyte
  accesses (width  8) since IMHO it was more consistent/correct with 
   the
  suggested locking.  I wasn't ignoring your suggestion, just making one
  or a few changes at a time (generally the simpler ones).
 
 Okay now I remember why I was reluctant to do this - suppose ACPIBIOS
 does a multibyte op on a set of bytes whose last byte fails
 acpi_check_rtc_byteaccess().  I will have already performed n-1
 accesses.  At one point I had a revision (acpi_check_rtc_access()?) that
 permitted/denied the entire request (it took the starting address and
 byte length), but I guess that got lost too.  I'll just recreate it...
  
   Yep, validating all access before doing any sounds like the way to go.
  
   Also, bytes = width  3 is ok, since you then affirm !(width  0x07), 
   so non-multiples of 8 bits are invalidated anyway.  You should still 
   check that width (or bytes)  0, even if 0 should never be passed.
  
  Oh yeah, forgot about that!
  
   I guess the Big Kids will start playing once this hits bugzilla? :)
  
  I'm just glad I get to learn how to commit stuff to FreeBSD.
  
  Here's another iteration...comments welcome.  Think I got (most)
  everything in there.  I need to unit test acpi_check_rtc_access() to
  make sure it DTRT?

You've fixed all my concerns, thanks Anthony.  A couple of questions:

+#ifndef ATRTC_VERBOSE
+#define ATRTC_VERBOSE 1
+#endif

Where else might ATRTC_VERBOSE be set otherwise?

+static unsigned int atrtc_verbose = ATRTC_VERBOSE;
+SYSCTL_UINT(_debug, OID_AUTO, atrtc_verbose, CTLFLAG_RWTUN,
+   atrtc_verbose, 0, AT-RTC Debug Level (0-2));
+#define ATRTC_DBG_PRINTF(level, format, ...) \
+   if (atrtc_verbose = level) printf(format, ##__VA_ARGS__)

Genuine question, I don't know .. but would not that 0 in SYSCTL_UINT 
initialise atrtc_verbose to 0?  Or does it keep its existing setting?
Or would replacing 0 there with ATRTC_VERBOSE work, or be clearer?

 static int
+acpi_check_rtc_access(int is_read, u_long addr, u_long len)
+{
+   int retval = 1; /* Success */
+
+   if (is_read) {
+   /* Reading 0x0C will muck with interrupts */
+   if (addr + len - 1 = 0x0C  addr = 0x0c)
+   retval = 0;

Looks alright to me, given my uncertainty with C operator precedence.

+   } else {
+   /* Allow single-byte writes to alarm registers and
+* addr = 0x30, else deny.
+*/
+   if (!((len == 1  (addr = 5  (addr  1))) || addr = 0x30))
+   retval = 0;
+   }
+   return retval;
+}

After a short struggle unwinding brackets, this looks right; but I will 
suggest clarifying the comment:

  +   /* Allow single-byte writes to alarm registers and
- +* addr = 0x30, else deny.
+ +* multi-byte writes to addr = 0x30, else deny.

I still wonder if there isn't a global acpi_loaded_and_running variable 
so you could avoid even attempting ACPI init calls, perhaps making this 
not so dependent on ACPI, at least at runtime.

But perhaps jkim's concern is more regarding building on platforms not 
supporting ACPI at all?  Is that the (only?) issue with this on ARM?

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. 5

2015-03-17 Thread Anthony Jenkins
On 03/17/2015 08:28 AM, Ian Smith wrote:
 On Mon, 16 Mar 2015 15:51:30 -0400, Anthony Jenkins wrote:
   On 03/16/2015 01:49 PM, Ian Smith wrote:
On Mon, 16 Mar 2015 11:50:59 -0400, Anthony Jenkins wrote:
  On 03/16/2015 11:00 AM, Anthony Jenkins wrote:
   On 03/16/2015 09:59 AM, Ian Smith wrote:
   On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote:
   +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?
   ===
  
   Otherwise (for example) a 2 byte read from 0x0b or 4 byte read 
 from 
   0x09-0x0b will read 0x0c (clearing interrupts), or a 2 or 4 byte 
 write 
   to (say) 0x01 will also write to 0x02 and 0x04 (clobbering the 
 time).
   Right, this is an (incorrect) hybrid of a few attempts, probably 
 from
   around the time I lost my SSD and only had a single backup copy of 
 my
   work to go from.  In one revision I had disallowed all multibyte
   accesses (width  8) since IMHO it was more consistent/correct with 
 the
   suggested locking.  I wasn't ignoring your suggestion, just making 
 one
   or a few changes at a time (generally the simpler ones).
  
  Okay now I remember why I was reluctant to do this - suppose ACPIBIOS
  does a multibyte op on a set of bytes whose last byte fails
  acpi_check_rtc_byteaccess().  I will have already performed n-1
  accesses.  At one point I had a revision (acpi_check_rtc_access()?) 
 that
  permitted/denied the entire request (it took the starting address and
  byte length), but I guess that got lost too.  I'll just recreate it...
   
Yep, validating all access before doing any sounds like the way to go.
   
Also, bytes = width  3 is ok, since you then affirm !(width  0x07), 
so non-multiples of 8 bits are invalidated anyway.  You should still 
check that width (or bytes)  0, even if 0 should never be passed.
   
   Oh yeah, forgot about that!
   
I guess the Big Kids will start playing once this hits bugzilla? :)
   
   I'm just glad I get to learn how to commit stuff to FreeBSD.
   
   Here's another iteration...comments welcome.  Think I got (most)
   everything in there.  I need to unit test acpi_check_rtc_access() to
   make sure it DTRT?

 You've fixed all my concerns, thanks Anthony.  A couple of questions:

 +#ifndef ATRTC_VERBOSE
 +#define ATRTC_VERBOSE 1
 +#endif

 Where else might ATRTC_VERBOSE be set otherwise?

I'm picturing a (future?) config(5) knob, e.g.

device atrtc
options ATRTC_VERBOSE=1


so it can be set at compile time.

 +static unsigned int atrtc_verbose = ATRTC_VERBOSE;
 +SYSCTL_UINT(_debug, OID_AUTO, atrtc_verbose, CTLFLAG_RWTUN,
 +   atrtc_verbose, 0, AT-RTC Debug Level (0-2));
 +#define ATRTC_DBG_PRINTF(level, format, ...) \
 +   if (atrtc_verbose = level) printf(format, ##__VA_ARGS__)

 Genuine question, I don't know .. but would not that 0 in SYSCTL_UINT 
 initialise atrtc_verbose to 0?  Or does it keep its existing setting?
 Or would replacing 0 there with ATRTC_VERBOSE work, or be clearer?

Ahh, good catch... guess I don't even need the variable initializer.

  static int
 +acpi_check_rtc_access(int is_read, u_long addr, u_long len)
 +{
 +   int retval = 1; /* Success */
 +
 +   if (is_read) {
 +   /* Reading 0x0C will muck with interrupts */
 +   if (addr + len - 1 = 0x0C  addr = 0x0c)
 +   retval = 0;

 Looks alright to me, given my uncertainty with C operator precedence.

 +   } else {
 +   /* Allow single-byte writes to alarm registers and
 +* addr = 0x30, else deny.
 +*/
 +   if (!((len == 1  (addr = 5  (addr  1))) || addr = 
 0x30))
 +   retval = 0;
 +   }
 +   return retval;
 +}

 After a short struggle unwinding brackets, this looks right; but I will 
 suggest clarifying the comment:

   +   /* Allow single-byte writes to alarm registers and
 - +* addr = 0x30, else deny.
 + +* multi-byte writes to addr = 0x30, else deny.

Okay.


 I still wonder if there isn't a global acpi_loaded_and_running variable 
 so you could avoid even attempting ACPI init calls, perhaps making this 
 not so dependent on ACPI, at least at runtime.

I haven't (yet) been able to find a compile-time flag that tells me if
the kernel supports ACPI; I'm /pretty/ sure the ACPI headers I'm
#include'ing will exist for every build of FreeBSD.

 But perhaps jkim's concern is more regarding building on platforms not 
 supporting ACPI at all?  Is that the (only?) issue with this on ARM?

Ehh... I'll wait for him to chime in.  I could try cross-compiling the
kernel for an ARM box, but I doubt sys/x86/isa/atrtc.c is even picked up

Re: [PATCH] ACPI CMOS region support rev. 5

2015-03-16 Thread Jung-uk Kim
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 03/16/2015 15:51, Anthony Jenkins wrote:
 On 03/16/2015 01:49 PM, Ian Smith wrote:
 On Mon, 16 Mar 2015 11:50:59 -0400, Anthony Jenkins wrote:
 On 03/16/2015 11:00 AM, Anthony Jenkins wrote:
 On 03/16/2015 09:59 AM, Ian Smith wrote:
 On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote:
 +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? ===
 
 Otherwise (for example) a 2 byte read from 0x0b or 4 byte
 read from 0x09-0x0b will read 0x0c (clearing interrupts),
 or a 2 or 4 byte write to (say) 0x01 will also write to
 0x02 and 0x04 (clobbering the time).
 Right, this is an (incorrect) hybrid of a few attempts,
 probably from around the time I lost my SSD and only had a
 single backup copy of my work to go from.  In one revision I
 had disallowed all multibyte accesses (width  8) since IMHO
 it was more consistent/correct with the suggested locking.  I
 wasn't ignoring your suggestion, just making one or a few
 changes at a time (generally the simpler ones).
 
 Okay now I remember why I was reluctant to do this - suppose
 ACPIBIOS does a multibyte op on a set of bytes whose last byte
 fails acpi_check_rtc_byteaccess().  I will have already
 performed n-1 accesses.  At one point I had a revision
 (acpi_check_rtc_access()?) that permitted/denied the entire
 request (it took the starting address and byte length), but I
 guess that got lost too.  I'll just recreate it...
 
 Yep, validating all access before doing any sounds like the way
 to go.
 
 Also, bytes = width  3 is ok, since you then affirm !(width 
 0x07), so non-multiples of 8 bits are invalidated anyway.  You
 should still check that width (or bytes)  0, even if 0 should
 never be passed.
 
 Oh yeah, forgot about that!
 
 I guess the Big Kids will start playing once this hits bugzilla?
 :)
 
 I'm just glad I get to learn how to commit stuff to FreeBSD.
 
 Here's another iteration...comments welcome.  Think I got (most) 
 everything in there.  I need to unit test acpi_check_rtc_access()
 to make sure it DTRT.

I see that there are several minor style(9) bugs but the most serious
problem is this patch makes atrtc.c dependent on ACPI and it
practically kills off APM support.  Please make it optional (hint:
sys/conf/files* and sys/conf/options*) although I don't mind killing
off APM support. ;-)

Jung-uk Kim
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQEcBAEBCAAGBQJVB2jgAAoJEHyflib82/FG2i4IAIVjf2fN6HcxVBzWbB5SWBGl
d4ZircYjOkq5ld8jqVBuZP+En5Jm94JABo1Hp3XV4z8GNzCT29jB8STh4WmpU8Zu
A6kURXJjAPyUZbbQKtRr1YzTOfzttUNBBnPPbFvyAZG0vLEjCwZnx/2t7yrO2A/I
7PgbW5Qtl1TTRYux/eF6zFEWo2nPrK70Rr8dKCYTUlJnYorz3YVuSkM1PrjJUjom
6C0t3N3X5BxziuW/NRwWWWCf2EOkAR3Mdefo/eFASm8+n4GTCpxflnWPuy6NjIYY
1NSmqGurTcFoyQ9igzl7J8gWteqwaPMjlpAp0GCU1ADpkhrBmcU7dFK9C7jcOaE=
=ZKAN
-END PGP SIGNATURE-
___
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. 5

2015-03-16 Thread Anthony Jenkins


On 03/16/15 19:36, Jung-uk Kim wrote:
 On 03/16/2015 15:51, Anthony Jenkins wrote:
  On 03/16/2015 01:49 PM, Ian Smith wrote:
  On Mon, 16 Mar 2015 11:50:59 -0400, Anthony Jenkins wrote:
  On 03/16/2015 11:00 AM, Anthony Jenkins wrote:
  On 03/16/2015 09:59 AM, Ian Smith wrote:
  On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote:
  +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? ===
 
  Otherwise (for example) a 2 byte read from 0x0b or 4 byte
  read from 0x09-0x0b will read 0x0c (clearing interrupts),
  or a 2 or 4 byte write to (say) 0x01 will also write to
  0x02 and 0x04 (clobbering the time).
  Right, this is an (incorrect) hybrid of a few attempts,
  probably from around the time I lost my SSD and only had a
  single backup copy of my work to go from.  In one revision I
  had disallowed all multibyte accesses (width  8) since IMHO
  it was more consistent/correct with the suggested locking.  I
  wasn't ignoring your suggestion, just making one or a few
  changes at a time (generally the simpler ones).
 
  Okay now I remember why I was reluctant to do this - suppose
  ACPIBIOS does a multibyte op on a set of bytes whose last byte
  fails acpi_check_rtc_byteaccess().  I will have already
  performed n-1 accesses.  At one point I had a revision
  (acpi_check_rtc_access()?) that permitted/denied the entire
  request (it took the starting address and byte length), but I
  guess that got lost too.  I'll just recreate it...
 
  Yep, validating all access before doing any sounds like the way
  to go.
 
  Also, bytes = width  3 is ok, since you then affirm !(width 
  0x07), so non-multiples of 8 bits are invalidated anyway.  You
  should still check that width (or bytes)  0, even if 0 should
  never be passed.

  Oh yeah, forgot about that!

  I guess the Big Kids will start playing once this hits bugzilla?
  :)

  I'm just glad I get to learn how to commit stuff to FreeBSD.

  Here's another iteration...comments welcome.  Think I got (most)
  everything in there.  I need to unit test acpi_check_rtc_access()
  to make sure it DTRT.

 I see that there are several minor style(9) bugs

Ahh... style(9) is what I was looking for, thanks.  Yeah I was hoping
someone would point me to FreeBSD's official coding style guide.

 but the most serious
 problem is this patch makes atrtc.c dependent on ACPI and it
 practically kills off APM support.

Not quite sure what you mean here... do you mean a non-ACPI build of the
kernel would fail to compile atrtc(4)?  Yeah I can fix that.

 Please make it optional (hint:
 sys/conf/files* and sys/conf/options*) although I don't mind killing
 off APM support. ;-)

Well I'd just make the ACPI CMOS handler code conditional on ACPI being
enabled in config(5)... is that what you're looking for (or would that
work)?

Thanks,
Anthony

 Jung-uk Kim
 ___
 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