As you may have noticed, I've been doing a significant amount of tidying up in the i2c code, specifically getting rid of polled mode access as much as possible, and auditing everything for "accesses i2c in hard interrupt context" (which is not safe). I am mostly complete with this task, but there is a notable exception: RTC (Real Time Clock) drivers.
The RTC drivers provide back-ends for the TODR (Time Of Day Register) subsystem. The TODR subsystem has 3 basic functions: -- inittodr(): Initializes the in-memory copy of the system time from one of 2 sources: the last-modified time of the root file system, or the value from the back-end RTC. It is called at boot time, and when the system wakes up after a sleep / hibernate. -- resettodr(): Writes the current in-memory copy of the system time to the back-end RTC. Called from cpu_reboot(), from the Xen "push our time to hypervisor" callout, and from settime1() (a back-end for clock_settime(), etc.) -- Wrappers around the back-end RTC drivers (todr_gettime(), todr_settime()) that take care of translating the in-memory representation to/from one of a couple of different formats that hardware prefers. None of these modify any global state worth worrying about (inittodr() sets "timeset" to true, and calls tc_setclock(), which has real synchronization internally), other than the back-end hardware. todr_gettime() and todr_settime() are ONLY called by inittodr() and resettodr(), and so they could (should probably?) be made static. It seems reasonable that todr_gettime() and todr_settime() should have some serialization here -- for example, when suspending (where wake-up would later call inittodr()), you want to make sure that any in-progress clock_settime() calls, etc. have completed before proceeding. Without doing so, you run the risk of deadlocking on e.g. an i2c bus lock (polled-mode operation must die). I'm thinking that I'd do the following: -- todr_lock() -- acquires a lock on the TODR subsystem. External users would be suspend code that grabs it on the way down. -- todr_unlock() -- releases the TODR lock. -- inittodr_locked() -- External users that have previously called todr_lock() would use inittodr_locked() instead of inittodr() to re-initialize the system time from the RTC hardware (which has presumably been ticking away powered by its own private CR2032). -- Internal to resettodr(), if shutting down, acquiring the internal TODR mutex would be a trylock to avoid getting stuck. I think that's the only serialization needed for the actual RTC hardware, and I think it's reasonable that sleeping should be allowed when accessing it. I would like to add a "shutting_down" global (to go along with "cold") that would automatically transition the i2c layer to polled-operation for the shutdown sequence (to avoid getting stuck writing the RTC ... worst case you just don't get to update it with the kernel's adjusted notion of UTC). Ultimately, I would like to rename inittodr() and resettodr() to something else (todr_copyin() and todr_copyout() maybe? The names today -- init and reset -- aren't really reflective of what they actually do). But I'm not going to die on that hill, so am not proposing any such radical extremist changes at this time :-) Now let's talk about settime1() in kern_time.c. It does a few things that seem dicey vis a vis modifying global state without proper serialization, and just about the entire function is wrapped in an splclock()/splx() pair. First of all, it's not clear to me at all that splclock()/splx() is really needed here... *maybe* across the call to tc_setclock()? But the mutex it takes internally is an IPL_HIGH spin mutex, so splclock() seems superfluous, just a legacy hold-over. ANYWAY... computing "now" and "delta" can be done completely outside a lock. The delta is used to perform authorization and adjust "boottime". The call to tc_setclock() can be done outside a lock. And I think the resettodr() can be done bare, as well (taking into account the TODR synchronization mentioned above). The only thing that requires some synchronization is modifying "boottime", but maybe it's not super critical (all of the consumers of this variable would need significant cleanup. Anyone have any other thoughts on this? -- thorpej
