linux-kernel - Re: [PATCH] ALSA: core: Remove trigger_tstamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r0asbwpi.wl-tiwai@suse.de>
Date: Tue, 13 Aug 2024 11:26:01 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Zeno Endemann <zeno.endemann@...lbox.org>
Cc: linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	Cezary Rojewski <cezary.rojewski@...el.com>,
	Christian Brauner <brauner@...nel.org>,
	Mark Brown <broonie@...nel.org>,
	Pavel Hofman <pavel.hofman@...tera.com>,
	David Howells <dhowells@...hat.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
	Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
	Peter Ujfalusi <peter.ujfalusi@...ux.intel.com>,
	Bard Liao <yung-chuan.liao@...ux.intel.com>,
	Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
	Kai Vehmanen <kai.vehmanen@...ux.intel.com>
Subject: Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

On Mon, 12 Aug 2024 16:20:29 +0200,
Zeno Endemann wrote:
> 
> The trigger_tstamp_latched hook was introduced to allow drivers to
> provide their own trigger timestamp instead of the default generated
> one for higher accuracy. This makes sense in theory, but in practice
> the only place that uses this is the hda core, and:
> 
> * The custom timestamp there does not seem to be a meaningful
>   improvement over the default one; There is virtually no code in
>   between them, so I measured only a difference of around 300ns in a
>   KVM VM with ich9-intel-hda device.
> * It is also bugged as it does not set a timestamp when the stream
>   stops.
> * It creates a pitfall for hda driver writers; Calling
>   snd_hdac_stream_timecounter_init implicitly makes them responsible
>   for generating these timestamps.
> 
> Since there is no real good use of this facility, I propose to remove
> it.
> 
> I reported the bug initially on github (see below), there one can also
> find a reproducer userspace app, as well as some other potential ways
> to fix this issue, in case this removal is not accepted.
> 
> Cc'ing the Intel ASoC maintainers, as the skl-pcm.c is using the
> snd_hdac_stream_timecounter_init function this patch modifies.
> 
> Closes: https://github.com/alsa-project/alsa-lib/issues/387
> Signed-off-by: Zeno Endemann <zeno.endemann@...lbox.org>

As long as I read the thread, there seems still use cases for this
high resolution time counter, so I'm rather for fixing the bugs of
bogus trigger tstamp than dropping the feature.

I still wonder, though, why the trigger_tstamp becomes *earlier* when
runtime->trigger_tstamp_latched is set.  I thought the same value
would be kept, instead.  What am I overlooking?

In anyway, if it's only about the missing trigger_tstamp update at PCM
trigger actions other than START, it could be fixed simply by clearing
trigger_tstamp_latched flag, too.  Alternatively, move the conditional
call to snd_pcm_post_start() instead of snd_pcm_trigger_tstamp()
itself, something like below.


thanks,

Takashi

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1194,8 +1194,7 @@ static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
 	if (runtime->trigger_master == NULL)
 		return;
 	if (runtime->trigger_master == substream) {
-		if (!runtime->trigger_tstamp_latched)
-			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
+		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
 	} else {
 		snd_pcm_trigger_tstamp(runtime->trigger_master);
 		runtime->trigger_tstamp = runtime->trigger_master->runtime->trigger_tstamp;
@@ -1454,7 +1453,8 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream,
 			       snd_pcm_state_t state)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_trigger_tstamp(substream);
+	if (!runtime->trigger_tstamp_latched)
+		snd_pcm_trigger_tstamp(substream);
 	runtime->hw_ptr_jiffies = jiffies;
 	runtime->hw_ptr_buffer_jiffies = (runtime->buffer_size * HZ) / 
 							    runtime->rate;