Re: ALSA: hda: Make proper use of timecounter

[Takashi Iwai]

On Mon, 29 Nov 2021 17:06:40 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 11/24/21 4:40 PM, Thomas Gleixner wrote:
> > HDA uses a timecounter to read a hardware clock running at 24 MHz. The
> > conversion factor is set with a mult value of 125 and a shift value of 0,
> > which is not converting the hardware clock to nanoseconds, it is converting
> > to 1/3 nanoseconds because the conversion factor from 24Mhz to nanoseconds
> > is 125/3. The usage sites divide the "nanoseconds" value returned by
> > timecounter_read() by 3 to get a real nanoseconds value.
> > 
> > There is a lengthy comment in azx_timecounter_init() explaining this
> > choice. That comment makes blatantly wrong assumptions about how
> > timecounters work and what can overflow.
> > 
> > The comment says:
> > 
> >      * Applying the 1/3 factor as part of the multiplication
> >      * requires at least 20 bits for a decent precision, however
> >      * overflows occur after about 4 hours or less, not a option.
> > 
> > timecounters operate on time deltas between two readouts of a clock and use
> > the mult/shift pair to calculate a precise nanoseconds value:
> > 
> >     delta_nsec = (delta_clock * mult) >> shift;
> > 
> > The fractional part is also taken into account and preserved to prevent
> > accumulated rounding errors. For details see cyclecounter_cyc2ns().
> > 
> > The mult/shift pair has to be chosen so that the multiplication of the
> > maximum expected delta value does not result in a 64bit overflow. As the
> > counter wraps around on 32bit, the maximum observable delta between two
> > reads is (1 << 32) - 1 which is about 178.9 seconds.
> > 
> > That in turn means the maximum multiplication factor which fits into an u32
> > will not cause a 64bit overflow ever because it's guaranteed that:
> > 
> >      ((1 << 32) - 1) ^ 2 < (1 << 64)
> > 
> > The resulting correct multiplication factor is 2796202667 and the shift
> > value is 26, i.e. 26 bit precision. The overflow of the multiplication
> > would happen exactly at a clock readout delta of 6597069765 which is way
> > after the wrap around of the hardware clock at around 274.8 seconds which
> > is off from the claimed 4 hours by more than an order of magnitude.
> > 
> > If the counter ever wraps around the last read value then the calculation
> > is off by the number of wrap arounds times 178.9 seconds because the
> > overflow cannot be observed.
> > 
> > Use clocks_calc_mult_shift(), which calculates the most accurate mult/shift
> > pair based on the given clock frequency, and remove the bogus comment along
> > with the divisions at the readout sites.
> > 
> > Fixes: 5d890f591d15 ("ALSA: hda: support for wallclock timestamps")
> > Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> I don't recall the reason of why I added separate steps for
> multiplication by 125 and division by 3 back in 2012, but obviously they
> weren't aligned with my own comment "Max buffer time is limited to 178
> seconds to make sure wall clock counter does not overflow".
> 
> Thanks for the patch, much appreciated.
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>

Now queued to for-next branch.  Thanks.


Takashi