Re: clk: clock rates can overflow 32-bit fields

[Michael Turquette]

Quoting Brian Norris (2015-04-12 21:24:22)
> Hi,
> 
> I've recently been looking at using the common clock framework to
> handle my CPU clocks for use by the cpufreq-dt driver, and I ran
> across a few problems with integer overflow. On a 32-bit system,
> 'unsigned long' (the type used in clk_set_rate() and similar APIs) is
> often a 32-bit integer. This constrains the maximum clock frequency to
> ~4.3 GHz, which is sufficient for most CPUs these days. However, I've
> run into problems with high clock rates in the common clock framework
> when

Morbid curiosity: what cpu/soc are you running that hits this problem?

> 
> (1) using clk-divider.c; and/or
> (2) using intermediate clocks that run faster than 4.3 GHz
> 
> With clk-divider.c, we can run into problems at lower clock rates due
> to the usage of DIV_ROUND_UP (see, e.g., commit b11d282dbea2 "clk:
> divider: fix rate calculation for fractional rates"), since this might
> create overflows when doing the addition -- e.g., DIV_ROUND_UP(3 G,
> 1.5 G) = (3 G + 1.5 G - 1) / 1.5 G = (OVERFLOW) / 1.5 G
> 
> I could probably fix up the clk-divider.c issue locally, if necessary.
> But problem (2) seems to suggest a larger change may be required
> throughout the framework, and I'd like to solicit opinions before
> hacking away.

Yes, we've seen this problem coming up on us for a while. A related
problem is fractional rates where we simply lop off the fractional Hz.
Eg. 2.5Hz will be either 2 or 3 Hz depending on how your clock driver
rounds things.

> 
> So, any thoughts on how to best tackle this problem? Should we upgrade
> the clock framework to use a guaranteed 64-bit type for clock rates
> (e.g., u64)?

Well here is the bat shit crazy idea I've had for a while:

1) make rates 64-bit so that we're future proof. Max rate: 18 exahertz.

2) make all clk api functions return s64 so that error codes can be
baked into rates (that is a LOT of unused error codes). Likely all rate
representation inside the clock framework will be s64. Max rate: 9
exahertz.

3) instead of hertz as the base unit, use millihertz (or something else
sub-Hz?) to capture the fractional hertz stuff. Max rate: 9 petahertz.

> I'm not sure if this will yield problems on certain
> 32-bit architectures when we start doing 64-bit integer division. But
> I don't have many other great ideas at the moment...

It will yield problems. There has been resistance on this topic before.
I'm not sure the best way forward. Compile time options to change the
definition of a yet-uncreated clk_rate_t could be one way, but that
seems like the path to madness.

Maybe once most of the upstream clock framework users upgrade to 64-bit
machines then we can gain a consensus? I dunno, probably not :-/

Regards,
Mike

> 
> Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/