Protect access to the AT realtime clock with its own mutex.

[• ian]

Summary

The mutex protecting access to the registered realtime clock should not be overloaded to protect access to the atrtc hardware, which might not even be the registered rtc. More importantly, the resettodr mutex needs to be eliminated to remove locking/sleeping restrictions on clock drivers, and that can't happen if MD code for amd64 depends on it. This change moves the protection into what's really being protected: access to the atrtc date and time registers.

This change also adds protection when the clock is accessed from xentimer_settime(), which bypasses the resettodr locking.

[kib]

sys/x86/isa/atrtc.c

59 ↗

(On Diff #30417)

I think that this comment is somewhat inaccurate now. If atrtc_time_lock is properly cover every access to AT CMOS registers, then the clock_lock spinlock purpose is to disable interrupts to avoid interrupted CMOS registers accesses. I am not sure how to explain it better.

[marius]

sys/x86/isa/atrtc.c

59 ↗

(On Diff #30417)

Actually, atrtc_time_lock should be a spinlock so as hinted by the comment in atrtc_gettime(), the chances are that the time actually is obtained within the 244 us slot (see also dev/mc146818). The problem here is that if we don't manage to read the registers during that slot in which we are guaranteed that no updates occur, we might actually read them during a clock update in which case the output is undefined (unfortunately, unlike other RTC hardware, MC146818-compatible gear has no means of helping the OS in obtaining a consistent snapshot of the time registers). The best thing we can do in order to achieve that goal is to disable interrupts across the entire register read, i. e. use a spinlock for atrtc_time_lock; neither the critical section currently used in atrtc_gettime() nor disabling interrupts via clock_lock for the accesses to the individual registers comes close to that.

Maybe it's time to finally replace the MC146818-specific part of atrtc(4) with mc146818(4); IIRC, one of the obstacles in that regard, i. e. the x86-specific use of MC146818 hardware, was the support of MCA. However, at least that luckily went away in r313783.

[• ian]

sys/x86/isa/atrtc.c

59 ↗

(On Diff #30417)

I think the comment is accurate. Accessing the hardware requires multibyte sequences (select register then access selected register), and more than just the RTC fields are accessed via the same ports. The clock_lock ensures for example that if rtc_intr() runs while in the middle of atrtc_gettime() sequence, each thread's select-register-read-register sequences are uncorrupted.

As to using a spinlock instead of a critical section to fix pre-existing problems in atrtc_gettime(), I don't disagree with that, but it's beyond the scope of this change. My goal here was to keep the change to the code as minimal as possible, by just moving the existing mutex from subr_rtc.c and making it private within atrtc.c. I'd rather change the type of lock as a separate commit after this one, so that if it causes anyone problems it can easily be reverted without triggering a need to also revert other dependent changes in subr_rtc.