http://defect.opensolaris.org/bz/show_bug.cgi?id=6219
Bill Holler <bill.holler at sun.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |bill.holler at sun.com
--- Comment #1 from Bill Holler <bill.holler at sun.com> 2009-02-10 20:06:24
---
More feedback:
365 * The HPET Spec claims the Main Counter is initially halted and
zeroed.
366 * This is true on most test machines....
367 * A Dell 690 was observed to have an initially running Main
Counter.
368 * ENABLE_CNF bit was originally set and the main counter was
running.
seems unnecessary level of detail. Moderately interesting, but irrelevant.
377 /*
378 * Read main counter 10 times in a loop.
379 */
is one of those "obvious comments that don't help". The comment should
be *why* are we reading the counter ten times, and what happens next.
The code after that is quite complex, and really needs explication.
418 * The HPET spec allows for MSI. In the future MSI may be prefered.
preferred
hpet_allocate_proxy says that it sets up the timer before programming the
I/O APIC, but where is the I/O APIC programmed?...is it not before that,
in hpet_install_interrupt_handler->add_avintr? Moreover, "allocate" here
includes initing and adding an interrupt handler. It sounds more like "setup"
or "init" or something to me.
Do we need to do anything to protect against races in
hpet_uninstall_interrupt_handler? What if an interrupt is active when
it's called, or just after?
687 * Guaranty the high 64-bits do not increment after reading lower
"Guarantee". Guaranty is a noun.
On this: why not #ifdef amd64 for a true-64-bit read?
Similar comments for the one-line worker read/write routines; why all
the way into oneline functions?...(710-727, 739-744)
851 static int
852 hpet_timer_available(uint32_t allocated_timers, uint32_t n)
853 {
854 return (~(allocated_timers & (1 << n)));
855 }
That's not right; when allocated, you'll have a mask with one bit set,
and ~ also has bits set. You probably want '!', but I'd use "== 0" for
clarity anyway.
880 new_conf = hpet_read_timer_N_config(hip, timer_n);
881
882 /*
883 * Verify INT_ROUTE_CNF was set as expected:
884 */
885 return ((conf & new_conf) == conf);
Is there something specific to HPET that it won't accept config bits here,
or in general? If so, why is this the only verify?..
893 * guaranty the interrupt is handled after this function returns.
guarantee
949 * Timer registers including main counter may not be preserved through
ACPI
"including the main counter" needs () or ,, to set it off
955 * XXX: Future projects beware the HPET Main Counter is not saved/restored
for
956 * Deep C-state. That is not needed to use the HPET as an interval timer.
First, "beware: ", I think. Second: the HPET doesn't stop during deep c-state,
does it? If not, why is this something to beware of?
958 * Note that pause_cpus() called later by the CPR framework ensures all
CPUs
"called later by the CPR framework" needs either () or ,,
961 * It is safe to leave the HPET running as the system suspends.
but, 1), above, we say that HPET state isn't saved on s3-5, and, 2),
it sure looks like the routine is stopping the HPET...so ???
What makes sure the CPUs have processed their poke_cpus() before
continuing past hpet_expire_all(), or why isn't it necessary?
992 /*
993 * The ACPI and HPET specifications do not mention a need
to
994 * re-read ACPI tables nor re-map HPET physical memory
during
995 * resume.
996 */
Since mapping of the tables is only for the OS to be able to refer to
physical addresses, it wouldn't (unless you mean "the tables don't change
position in physical memory"), but there's plenty that depends on
the state of ACPI tables being consistent on S3 transition; mentioning it
here isn't the right place, I wouldn't think.
1029 cmn_err(CE_NOTE, "!hpet resume main counter failed");
1047 cmn_err(CE_WARN, "!hpet_acpi: timer setup failed.");
Both of these are in a routine named "hpet_resume", and should probably
say so.
1090 * The order of these opperations is important to avoid
operations
if we're going to use B_TRUE/B_FALSE, why not for proxy_installed as well?
1091 * lost wakeups: Set a flag to refuse all future LAPIC
Timer
1092 * proxy requests, then waking up all CPUs from deep
C-state,
1093 * and finally disable the HPET interrupt generating
timer.
tenses: "set, waking, disable". Probably "waking" should be "wake". Also
"interrupt-generating" needs a hyphen.
1098 etc.: what happens if we never get the lock? This CPU is screwed...
with no warning. counter and message?...
1124 * Interrupt Service Routine for HPET I/O APIC generated interrupts.
1125 * Used to wakeup CPUs from Deep C-state when there Local APIC Timer
stops.
"I/O-APIC-generated", and "their". And I wouldn't capitalize Service Routine
or Local [APIC] Timer".
1137 * We are using a level triggered interrupt.
level-triggered
1158 * CPUs in deep c-states do not enable interrupts until after
1159 * performing idle cleanup which includes descheduling itself from
number. Either "CPU...does" or "themselves"
So, I don't get this: the HPET interrupt *is* what moves us out of
deep c-state, right? What does the HPET do if not cause an interrupt to
awake from c-state?
--
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
