On 2/3/23 12:02, Jaroslav Kysela wrote: > On 03. 02. 23 17:11, Alan Young wrote: >> >> On 03/02/2023 00:34, Takashi Sakamoto wrote: >>> Hi, >>> >>> Thank you for the report. >>> >>> On Thu, Feb 02, 2023 at 01:55:24PM +0000, Alan Young wrote: >>>> sound/core/pcm_lib.c:update_audio_tstamp() contains the following >>>> calculation: >>>> >>>> audio_nsecs = div_u64(audio_frames * 1000000000LL, >>>> runtime->rate); >>>> >>>> This will result in a 64-bit overflow after 4.4 days at 48000 Hz, or >>>> 1.1 >>>> days at 192000. >>>> >>>> Are you interested in a patch to improve this? >>>> >>>> The same calculation occurs in a couple of other places. >>> I'm interested in your patch. Would you please post it C.C.ed to the >>> list and me? As you noted, we can see the issue in ALSA PCM core and >>> Intel HDA stuffs at least. >>> >>> * sound/core/pcm_lib.c >>> * sound/pci/hda/hda_controller.c >>> * sound/soc/intel/skylake/skl-pcm.c >>> >>> I note that 'NSEC_PER_SEC' macro is available once including >>> 'linux/time.h'. It is better to use instead of the literal. >>> The macro is defined in 'include/vdso/time64.h'. >>> >>> >>> As another issue, the value of 'audio_frames' comes from the value of >>> 'struct snd_pcm_runtime.hw_ptr_wrap'. In ALSA PCM core, the value is >>> increased by the size of PCM buffer every time hw_ptr cross the boundary >>> of PCM buffer, thus multiples of the size is expected. Nevertheless, >>> there is no check for overflow within 64 bit storage. In my opinion, the >>> committer had less care of it since user does not practically >>> playback or >>> capture PCM substream so long. But the additional check is preferable as >>> long as it does not break the fallback implementation of audio time >>> stamp. >> >> >> I have not yet finished testing various alternatives. I want to extend >> the overflow by "enough" and also am conscious of the need to keep the >> overhead down. >> >> I actually think, on reflection, that the only case that matters is the >> call from update_audio_tstamp(). The others only deal with codec delays >> which will be small (unless I misunderstand those drivers). >> >> This is what I have so far but I'll submit a proper patch when I have it >> refined. >> >> static u64 snd_pcm_lib_frames_to_nsecs(u64 frames, unsigned int rate) >> { >> /* >> * Avoid 64-bit calculation overflow after: >> * - 4.8 days @ 44100 >> * - 0.56 days @ 384000 >> * extending these intervals by a factor of 100. >> */ >> if (frames < 0xffffffffffffffffLLU / NSEC_PER_SEC) >> return div_u64(frames * NSEC_PER_SEC, rate); >> >> if (rate % 100 == 0) >> return div_u64(frames * (NSEC_PER_SEC/100), (rate/100)); >> >> /* Fallback: reduce precision to approximately >> deci-micro-seconds: 1.28e-7 */ >> return div_u64(frames * (NSEC_PER_SEC >> 7), rate) << 7; >> } > > Thank you for your suggestion, but I think that the *whole* code for > !get_time_info in update_audio_tstamp() should be recoded. The calling > of ns_to_timespec64() is not enough to handle the boundary wraps in a > decent range (tenths years for 24x7 operation) and the bellow code is > dangerous for 32-bit apps / system: > > if (crossed_boundary) { > snd_BUG_ON(crossed_boundary != 1); > runtime->hw_ptr_wrap += runtime->boundary; > } > > I would probably propose to have just hw_ptr_wrap +1 counter (we can > reconstruct the frame position back by multiplication and do range check > later), remove snd_BUG_ON and improve the timespec64 calculation. > > The calculation should be split to two parts (tv_sec / tv_nsec): > > 1) calculate seconds: (frames / rate) > 2) calculate the remainder (ns): ((frames % rate) * NSEC_PER_SEC) / rate > > With 64-bit integer range, we should go up to (for 384000Hz rate): > > 2**64 / 384000 / 3600 / 24 / 365 = ~1523287 years > > Maybe I did a mistake somewhere. I'm open for comments. I am not following how the boundary comes into play for cases where the timestamp comes directly from a link counter, and is not related to the DMA hw_ptr at all.