linux-kernel - [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-next>] [day] [month] [year] [list]

Message-ID: <20240812142029.46608-1-zeno.endemann@mailbox.org>
Date: Mon, 12 Aug 2024 16:20:29 +0200
From: Zeno Endemann <zeno.endemann@...lbox.org>
To: linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc: 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>,
	Zeno Endemann <zeno.endemann@...lbox.org>
Subject: [PATCH] ALSA: core: Remove trigger_tstamp_latched

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>
---
 include/sound/pcm.h     | 1 -
 sound/core/pcm_native.c | 4 +---
 sound/hda/hdac_stream.c | 6 ------
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index ac8f3aef9205..3539af9f733e 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -361,7 +361,6 @@ struct snd_pcm_runtime {
 	snd_pcm_state_t suspended_state; /* suspended stream state */
 	struct snd_pcm_substream *trigger_master;
 	struct timespec64 trigger_tstamp;	/* trigger timestamp */
-	bool trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
 	int overrange;
 	snd_pcm_uframes_t avail_max;
 	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 4057f9f10aee..ced5bd2d7ebb 100644
--- 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;
@@ -1422,7 +1421,6 @@ static int snd_pcm_pre_start(struct snd_pcm_substream *substream,
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    !snd_pcm_playback_data(substream))
 		return -EPIPE;
-	runtime->trigger_tstamp_latched = false;
 	runtime->trigger_master = substream;
 	return 0;
 }
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index b53de020309f..271d42b765fc 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -660,14 +660,11 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
  *
  * Initializes the time counter of streams marked by the bit flags (each
  * bit corresponds to the stream index).
- * The trigger timestamp of PCM substream assigned to the given stream is
- * updated accordingly, too.
  */
 void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
 				      unsigned int streams)
 {
 	struct hdac_bus *bus = azx_dev->bus;
-	struct snd_pcm_runtime *runtime = azx_dev->substream->runtime;
 	struct hdac_stream *s;
 	bool inited = false;
 	u64 cycle_last = 0;
@@ -681,9 +678,6 @@ void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
 			}
 		}
 	}
-
-	snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
-	runtime->trigger_tstamp_latched = true;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_stream_timecounter_init);
 
-- 
2.46.0