Re: Overflow in calculating audio timestamp

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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.


Regards

Takashi Sakamoto