Page MenuHomeFreeBSD

Support multiple realtime clocks, and remove locking/sleeping restrictions on clock drivers.
ClosedPublic

Authored by ian on Jul 4 2017, 11:06 PM.
Tags
None
Referenced Files
F103202314: D11484.id30418.diff
Fri, Nov 22, 5:00 AM
F103200057: D11484.id30420.diff
Fri, Nov 22, 4:28 AM
F103193233: D11484.id30420.diff
Fri, Nov 22, 2:40 AM
Unknown Object (File)
Mon, Nov 18, 12:09 PM
Unknown Object (File)
Mon, Nov 18, 10:58 AM
Unknown Object (File)
Mon, Nov 18, 8:47 AM
Unknown Object (File)
Mon, Nov 18, 8:42 AM
Unknown Object (File)
Mon, Nov 18, 8:23 AM
Subscribers

Details

Summary

This tracks multiple concurrent realtime clock drivers in a list sorted by clock resolution. When system time changes (and periodically) the clock_settime() methods of all registered clocks are invoked. To initialize system time, each driver is tried in turn from best to worst resolution, until one succesfully returns a valid time.

The code no longer holds a mutex while calling the clock_settime() and clock_gettime() methods of the registered clocks. This allows clock drivers to do whatever kind of locking or sleeping is necessary (this is especially important for i2c clock chips since i2c drivers often need to sleep).

A new clock_register_flags() function allows the clock driver to pass flags. The flags currently defined help support drivers that use their own techniques to avoid roundoff errors (prevents the 4/5 rounding done by the subr_rtc code). A driver which may need to wait for resources (such as bus ownership) may pass a flag to indicate that it will obtain system time for itself after waiting for resources; this is merely an optimization to avoid the common code retrieving a timespec that will never get used.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/kern/subr_rtc.c
121 ↗(On Diff #30418)

rtc_list_acquire/release are just hand-rolled sleepable locks. Why not use sx in exclusive mode instead ?

You get the witness support for free, as well as the check that unlock is correctly paired with lock.

183 ↗(On Diff #30418)

Don't you need rtc_list_acquire() around the manipulations of rtc_list ?

214 ↗(On Diff #30418)

And there as well ?

Thanks for the input, new diff coming soon...

sys/kern/subr_rtc.c
121 ↗(On Diff #30418)

Hmm, good idea. I had ruled out sx locks in an earlier incarnation, then never reconsidered as things evolved.

183 ↗(On Diff #30418)

Doh! Of course. They were there at one time, they seem to have gotten lost during a round of code cleanup.

Replace homegrown locking with sx lock. Add locking around list maintenance code.

sys/kern/subr_rtc.c
147 ↗(On Diff #30420)

I do not see much point in existence of both clock_register() and clock_register_flags(). Change the interface of clock_register() to require flags. Consumers would be adjusted but it is cleaner than to add layers of API to keep unneeded backward compatibility there.

151 ↗(On Diff #30420)

Do you need M_ZERO ?

195 ↗(On Diff #30420)

Do you need to continue the loop after the match is found ? This is not a question about the missed break, but rather I do not understand is it legal to register same clockdev several times. If it is not correct, then might be the register() function should check it ?

sys/kern/subr_rtc.c
147 ↗(On Diff #30420)

Changing the existing function impacts anyone with out-of-tree drivers, and I hope to MFC this to 11 at least.

195 ↗(On Diff #30420)

During an earlier incarnation I was experimenting with letting a device register more than once, with different resolutions (there are clocks that can provide better resolution on warmstart than coldstart, for example). But that also required changes in clock_if and was all just too complicated for little benefit, so I discarded it all.

Preventing multiple registration would require another loop to check for duplicates in the registration code, I'm not sure the benefit is worth the extra code bloat. While the code is now able to handle things like hot-swappable clocks on an i2c bus (we actually have such a thing at $work), in reality almost all systems are going to have a single device, and a few systems might have a list with two devices on it, where both always attach once at boot time.

kib added inline comments.
sys/kern/subr_rtc.c
195 ↗(On Diff #30420)

You already do the pass over the list to find the position, and checking the dups would only require not exiting the loop earlier. Also, I do not expect the list to contain more that several clocks, so even such rare event as the registration is not affected.

Anyway, clicked the button.

This revision is now accepted and ready to land.Jul 6 2017, 7:14 PM
This revision was automatically updated to reflect the committed changes.