[PATCH v2 0/3] ASoC: SOF: Intel: power optimizations with HDaudio SPIB register

[PATCH v2 0/3] ASoC: SOF: Intel: power optimizations with HDaudio SPIB register

[Pierre-Louis Bossart]

This patchset was initially provided in a larger series that was split
in two [1]. This part only provides support for the SPIB register
support, added on Intel platforms since Skylake (2015).

The use of the SPIB register helps reduce power consumption - though
to a smaller degree than DMI_L1. This hardware capability is however
incompatible with userspace-initiated rewinds typically used by
PulseAudio.

In the past (2015..2017) Intel suggested an API extension to let
applications disable rewinds. At the time the feedback was that such a
capability was too Intel-specific and SPIB remained unused except for
loading DSP code. We now see devices with smaller batteries being
released, and it's time to revisit Linux support for SPIB to extend
battery life.

In this update the rewinds are disabled via an opt-in kernel
parameter. In the previous reviews, there was consensus that a Kconfig
option was too complicated for distributions to set, and we are
missing a TBD API to expose such capabilities to user-space.

The debate on whether or not to use rewinds, and the impact of
disabling rewinds, will likely be closed when Intel releases the
'deep-buffer' support, currently under development [2][3]. With this
solution, rewinds will not be needed, ever. When an application deals
with content that is not latency-sensitive (e.g. music playback), it
will be able to reduce power consumption by selecting a different PCM
device with increased buffering capabilities.  Low-latency streams
will be handled by the 'regular' path. In other words, the impossible
compromise between power and latency will be handled with different
PCM devices/profiles for the same endpoint, and we can push the design
of capability negotiation to a later time when all the building blocks
(firmware topology, kernel, userspace) are ready - we still have
firmware xruns, DPCM race conditions to solve, and a need to describe
these alternate PCM devices with UCM using 'modifiers'.

[1] https://lore.kernel.org/alsa-devel/20210610205326.1176400-1-pierre-louis.bossart@linux.intel.com/
[2] https://github.com/thesofproject/linux/pull/3146
[3] https://github.com/thesofproject/sof/pull/4611

Pierre-Louis Bossart (1):
  ALSA: pcm: introduce INFO_NO_REWINDS flag

Ranjani Sridharan (2):
  ASOC: SOF: pcm: add .ack callback support
  ASoC: SOF: Intel: add .ack support for HDaudio platforms

 include/uapi/sound/asound.h      |  2 +-
 sound/core/pcm_native.c          |  2 ++
 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/cnl.c        |  1 +
 sound/soc/sof/intel/hda-pcm.c    | 41 ++++++++++++++++++++++++++++++--
 sound/soc/sof/intel/hda-stream.c |  2 ++
 sound/soc/sof/intel/hda.h        |  1 +
 sound/soc/sof/intel/tgl.c        |  1 +
 sound/soc/sof/ops.h              | 10 ++++++++
 sound/soc/sof/pcm.c              |  9 +++++++
 sound/soc/sof/sof-priv.h         |  3 +++
 11 files changed, 70 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

When the hardware can only deal with a monotonically increasing
appl_ptr, this flag can be set. In case the application requests a
rewind, snd_pcm_rewind() will not return an error code but signal that
the appl_ptr was not modified.

Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/uapi/sound/asound.h | 2 +-
 sound/core/pcm_native.c     | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1d84ec9db93b..bdb6f3c9db6c 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -300,7 +300,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
 #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
 #define SNDRV_PCM_INFO_EXPLICIT_SYNC	0x10000000	/* needs explicit sync of pointers and data */
-
+#define SNDRV_PCM_INFO_NO_REWINDS	0x20000000	/* hardware can only support monotonic changes of appl_ptr */
 #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index d233cb3b41d8..e3da2cc31a34 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2905,6 +2905,8 @@ static snd_pcm_sframes_t snd_pcm_rewind(struct snd_pcm_substream *substream,
 
 	if (frames == 0)
 		return 0;
+	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS)
+		return 0;
 
 	snd_pcm_stream_lock_irq(substream);
 	ret = do_pcm_hwsync(substream);
-- 
2.25.1

[PATCH v2 2/3] ASOC: SOF: pcm: add .ack callback support

[Pierre-Louis Bossart]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add the indirections required at the core level for platform-specific
operations on ack.

Note that on errors in the .ack the ALSA core will restore the
previous appl_ptr.

Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/ops.h      | 10 ++++++++++
 sound/soc/sof/pcm.c      |  9 +++++++++
 sound/soc/sof/sof-priv.h |  3 +++
 3 files changed, 22 insertions(+)

diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index a93aa5b943b2..ca63efce493b 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -449,6 +449,16 @@ snd_sof_pcm_platform_pointer(struct snd_sof_dev *sdev,
 	return 0;
 }
 
+/* pcm ack */
+static inline int snd_sof_pcm_platform_ack(struct snd_sof_dev *sdev,
+					   struct snd_pcm_substream *substream)
+{
+	if (sof_ops(sdev) && sof_ops(sdev)->pcm_ack)
+		return sof_ops(sdev)->pcm_ack(sdev, substream);
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
 static inline int
 snd_sof_probe_compr_assign(struct snd_sof_dev *sdev,
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 374df2dfa816..172a2737eaac 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -864,6 +864,14 @@ static void sof_pcm_remove(struct snd_soc_component *component)
 	snd_soc_tplg_component_remove(component);
 }
 
+static int sof_pcm_ack(struct snd_soc_component *component,
+		       struct snd_pcm_substream *substream)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+
+	return snd_sof_pcm_platform_ack(sdev, substream);
+}
+
 void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 {
 	struct snd_soc_component_driver *pd = &sdev->plat_drv;
@@ -882,6 +890,7 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 	pd->hw_free = sof_pcm_hw_free;
 	pd->trigger = sof_pcm_trigger;
 	pd->pointer = sof_pcm_pointer;
+	pd->ack = sof_pcm_ack;
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
 	pd->compress_ops = &sof_probe_compressed_ops;
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 1289e2efeb62..753b6ef27f40 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -189,6 +189,9 @@ struct snd_sof_dsp_ops {
 	snd_pcm_uframes_t (*pcm_pointer)(struct snd_sof_dev *sdev,
 					 struct snd_pcm_substream *substream); /* optional */
 
+	/* pcm ack */
+	int (*pcm_ack)(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream); /* optional */
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
 	/* Except for probe_pointer, all probe ops are mandatory */
 	int (*probe_assign)(struct snd_sof_dev *sdev,
-- 
2.25.1

[PATCH v2 3/3] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Pierre-Louis Bossart]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

When we disable rewinds, then the .ack can be used to program SPIB
with the application pointer, which allows the HDaudio DMA to save
power by opportunistically bursting data transfers when the path to
memory is enabled (and conversely to shut it down when there are no
transfer requests).

The SPIB register can only be programmed with incremental values with
wrap-around after the DMA RUN bits are set. For simplicity, we set the
INFO_NO_REWINDS flag in the .open callback when we already need to
program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.

Rewinds are not used by many applications. One notable application
using rewinds is PulseAudio. Practical experiments with
Ubuntu/PulseAudio default settings did not show any audible issues,
but the user may hear volume changes and notification with a delay,
depending on the size of the ring buffer and latency constraints.

The choice of disabling rewinds is exposed as a kernel parameter and
not a Kconfig option to avoid any undesirable side-effects.

Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/cnl.c        |  1 +
 sound/soc/sof/intel/hda-pcm.c    | 41 ++++++++++++++++++++++++++++++--
 sound/soc/sof/intel/hda-stream.c |  2 ++
 sound/soc/sof/intel/hda.h        |  1 +
 sound/soc/sof/intel/tgl.c        |  1 +
 6 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
index e6a1f6532547..cb457f46fca5 100644
--- a/sound/soc/sof/intel/apl.c
+++ b/sound/soc/sof/intel/apl.c
@@ -74,6 +74,7 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
 	.pcm_hw_free	= hda_dsp_stream_hw_free,
 	.pcm_trigger	= hda_dsp_pcm_trigger,
 	.pcm_pointer	= hda_dsp_pcm_pointer,
+	.pcm_ack	= hda_dsp_pcm_ack,
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
 	/* probe callbacks */
diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c
index 430a268e6b26..1c619f3241ac 100644
--- a/sound/soc/sof/intel/cnl.c
+++ b/sound/soc/sof/intel/cnl.c
@@ -279,6 +279,7 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
 	.pcm_hw_free	= hda_dsp_stream_hw_free,
 	.pcm_trigger	= hda_dsp_pcm_trigger,
 	.pcm_pointer	= hda_dsp_pcm_pointer,
+	.pcm_ack	= hda_dsp_pcm_ack,
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
 	/* probe callbacks */
diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
index cc8ddef37f37..875350283eac 100644
--- a/sound/soc/sof/intel/hda-pcm.c
+++ b/sound/soc/sof/intel/hda-pcm.c
@@ -32,6 +32,10 @@ static bool hda_always_enable_dmi_l1;
 module_param_named(always_enable_dmi_l1, hda_always_enable_dmi_l1, bool, 0444);
 MODULE_PARM_DESC(always_enable_dmi_l1, "SOF HDA always enable DMI l1");
 
+static bool hda_disable_rewinds = IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_DISABLE_REWINDS);
+module_param_named(disable_rewinds, hda_disable_rewinds, bool, 0444);
+MODULE_PARM_DESC(disable_rewinds, "SOF HDA disable rewinds");
+
 u32 hda_dsp_get_mult_div(struct snd_sof_dev *sdev, int rate)
 {
 	switch (rate) {
@@ -120,8 +124,11 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
 		return ret;
 	}
 
-	/* disable SPIB, to enable buffer wrap for stream */
-	hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
+	/* enable SPIB when rewinds are disabled */
+	if (hda_disable_rewinds)
+		hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_ENABLE, 0);
+	else
+		hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
 
 	/* update no_stream_position flag for ipc params */
 	if (hda && hda->no_ipc_position) {
@@ -140,6 +147,29 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
 	return 0;
 }
 
+/* update SPIB register with appl position */
+int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream)
+{
+	struct hdac_stream *hstream = substream->runtime->private_data;
+	struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	ssize_t appl_pos, buf_size;
+	u32 spib;
+
+	appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
+	buf_size = frames_to_bytes(runtime, runtime->buffer_size);
+
+	spib = appl_pos % buf_size;
+
+	/* Allowable value for SPIB is 1 byte to max buffer size */
+	if (!spib)
+		spib = buf_size;
+
+	sof_io_write(sdev, stream->spib_addr, spib);
+
+	return 0;
+}
+
 int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev,
 			struct snd_pcm_substream *substream, int cmd)
 {
@@ -234,6 +264,13 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev,
 		return -EINVAL;
 	}
 
+	/*
+	 * if we want the .ack to work, we need to prevent the status and
+	 * control from being mapped
+	 */
+	if (hda_disable_rewinds)
+		runtime->hw.info |= SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_EXPLICIT_SYNC;
+
 	/*
 	 * All playback streams are DMI L1 capable, capture streams need
 	 * pause push/release to be disabled
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 1d845c2cbc33..b6f037815344 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -655,6 +655,8 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev,
 					SOF_HDA_REG_PP_PPCTL, mask, 0);
 	spin_unlock_irq(&bus->reg_lock);
 
+	hda_dsp_stream_spib_config(sdev, link_dev, HDA_DSP_SPIB_DISABLE, 0);
+
 	stream->substream = NULL;
 
 	return 0;
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 1195018a1f4f..6829d36fbfe9 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -533,6 +533,7 @@ int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev,
 			struct snd_pcm_substream *substream, int cmd);
 snd_pcm_uframes_t hda_dsp_pcm_pointer(struct snd_sof_dev *sdev,
 				      struct snd_pcm_substream *substream);
+int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream);
 
 /*
  * DSP Stream Operations.
diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c
index 664a11aaada2..88491b97fde7 100644
--- a/sound/soc/sof/intel/tgl.c
+++ b/sound/soc/sof/intel/tgl.c
@@ -69,6 +69,7 @@ const struct snd_sof_dsp_ops sof_tgl_ops = {
 	.pcm_hw_free	= hda_dsp_stream_hw_free,
 	.pcm_trigger	= hda_dsp_pcm_trigger,
 	.pcm_pointer	= hda_dsp_pcm_pointer,
+	.pcm_ack	= hda_dsp_pcm_ack,
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
 	/* probe callbacks */
-- 
2.25.1

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Mon, 04 Oct 2021 18:24:21 +0200,
Pierre-Louis Bossart wrote:
> 
> When the hardware can only deal with a monotonically increasing
> appl_ptr, this flag can be set. In case the application requests a
> rewind, snd_pcm_rewind() will not return an error code but signal that
> the appl_ptr was not modified.

This modification itself is fine, but I guess that application may
still move the appl_ptr directly via SNDRV_PCM_IOCTL_SYNC_PTR ioctl.
We need to verify the backward move there, I suppose?


thanks,

Takashi

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

On 10/5/21 1:59 AM, Takashi Iwai wrote:
> On Mon, 04 Oct 2021 18:24:21 +0200,
> Pierre-Louis Bossart wrote:
>>
>> When the hardware can only deal with a monotonically increasing
>> appl_ptr, this flag can be set. In case the application requests a
>> rewind, snd_pcm_rewind() will not return an error code but signal that
>> the appl_ptr was not modified.
> 
> This modification itself is fine, but I guess that application may
> still move the appl_ptr directly via SNDRV_PCM_IOCTL_SYNC_PTR ioctl.
> We need to verify the backward move there, I suppose?

Sorry Takashi, I wasn't able to fully follow your question.

In the previous version, I had an explicit check to see if the appl_ptr
was modified by a rewind, but you mentioned this would be broken for
32-bit devices due to the use of the 'boundary'. I really have no idea
how we can detect a rewind in this configuration, so  if you are asking
to detect when the appl_ptr is modified through some other means (which
I didn't get) we will have the same problem, won't we?

see the initial thread here:

https://lore.kernel.org/alsa-devel/de5e91c6-5f0e-9866-a1c2-0943b4342359@linux.intel.com/

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 05 Oct 2021 15:10:40 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 10/5/21 1:59 AM, Takashi Iwai wrote:
> > On Mon, 04 Oct 2021 18:24:21 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >> When the hardware can only deal with a monotonically increasing
> >> appl_ptr, this flag can be set. In case the application requests a
> >> rewind, snd_pcm_rewind() will not return an error code but signal that
> >> the appl_ptr was not modified.
> > 
> > This modification itself is fine, but I guess that application may
> > still move the appl_ptr directly via SNDRV_PCM_IOCTL_SYNC_PTR ioctl.
> > We need to verify the backward move there, I suppose?
> 
> Sorry Takashi, I wasn't able to fully follow your question.
> 
> In the previous version, I had an explicit check to see if the appl_ptr
> was modified by a rewind, but you mentioned this would be broken for
> 32-bit devices due to the use of the 'boundary'. I really have no idea
> how we can detect a rewind in this configuration, so  if you are asking
> to detect when the appl_ptr is modified through some other means (which
> I didn't get) we will have the same problem, won't we?

Which same problem are you referring to?

What I suggested here is that there is still a way to modify the
appl_ptr from user-space even if you plug the way by disabling the
status/control mmap and disabling the rewind ioctl.  You have to cover
the sync_ptr ioctl call path as well, and it'd need a backward check.

> see the initial thread here:
> 
> https://lore.kernel.org/alsa-devel/de5e91c6-5f0e-9866-a1c2-0943b4342359@linux.intel.com/
 
And, what I meant in the previous thread was that the check in the
given patch wasn't "enough", i.e. it needs more careful checks
considering the boundary crossing.  That is, you can't simply compare
appl_ptr vs old_appl_ptr as a single condition for the backward move.

For example, check snd_pcm_playback_avail() and co.  That contains a
couple of more condition checks and corrections due to the possible
boundary crossing.  (Here, runtime->boundary may differ depending on
32 or 64bit context.)

The actual implementation of the backward move check would be slightly
different from those, but I hope you get my idea.


Takashi

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

> And, what I meant in the previous thread was that the check in the
> given patch wasn't "enough", i.e. it needs more careful checks
> considering the boundary crossing.  That is, you can't simply compare
> appl_ptr vs old_appl_ptr as a single condition for the backward move.

Indeed, that's why I tried to avoid any checks on pointers :-)

> For example, check snd_pcm_playback_avail() and co.  That contains a
> couple of more condition checks and corrections due to the possible
> boundary crossing.  (Here, runtime->boundary may differ depending on
> 32 or 64bit context.)
> 
> The actual implementation of the backward move check would be slightly
> different from those, but I hope you get my idea.

I think I do but not sure how to precisely deal with the boundary
wrap-around.

The only suggestion I have at this point would be to compare the 'avail'
space before and after the appl_ptr changes in pcm_lib_apply_appl_ptr().
If the 'avail' space grows as a result of user-space changes, that
indicates a rewind (both for capture and playback), doesn't it?

A tentative solution is shared here:
https://github.com/thesofproject/linux/pull/3207

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 02:19:16 +0200,
Pierre-Louis Bossart wrote:
> 
> > For example, check snd_pcm_playback_avail() and co.  That contains a
> > couple of more condition checks and corrections due to the possible
> > boundary crossing.  (Here, runtime->boundary may differ depending on
> > 32 or 64bit context.)
> > 
> > The actual implementation of the backward move check would be slightly
> > different from those, but I hope you get my idea.
> 
> I think I do but not sure how to precisely deal with the boundary
> wrap-around.
> 
> The only suggestion I have at this point would be to compare the 'avail'
> space before and after the appl_ptr changes in pcm_lib_apply_appl_ptr().
> If the 'avail' space grows as a result of user-space changes, that
> indicates a rewind (both for capture and playback), doesn't it?

There are a few different ways, and a simple one would be to treat as
a rewind if the change isn't 0..buffer_size.  e.g.

	snd_pcm_sframes_t diff = new_ptr - old_ptr;

	if (diff >= 0) {
		if (diff > buffer_size)
			return REWIND;
	} else {
		if (boundary + diff > buffer_size)
			return REWIND;
	}
	return OK;

Or, if a rewind is defined to be -buffer_size..-1, it'd be like:

	snd_pcm_sframes_t diff = new_ptr - old_ptr;

	if (diff >= 0) {
		if (boundary - diff <= buffer_size)
			return REWIND;
	} else {
		if (-diff <= buffer_size)
			return REWIND;
	}
	return OK;

In either way, the new_ptr has to be validated beforehand that it's
within 0..boundary-1.  (old_ptr is assumed to be valid.)

And don't miss that diff is a signed value, so it must be
snd_pcm_sframes_t, not *_uframes_t.


Takashi

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

>>> For example, check snd_pcm_playback_avail() and co.  That contains a
>>> couple of more condition checks and corrections due to the possible
>>> boundary crossing.  (Here, runtime->boundary may differ depending on
>>> 32 or 64bit context.)
>>>
>>> The actual implementation of the backward move check would be slightly
>>> different from those, but I hope you get my idea.
>>
>> I think I do but not sure how to precisely deal with the boundary
>> wrap-around.
>>
>> The only suggestion I have at this point would be to compare the 'avail'
>> space before and after the appl_ptr changes in pcm_lib_apply_appl_ptr().
>> If the 'avail' space grows as a result of user-space changes, that
>> indicates a rewind (both for capture and playback), doesn't it?
> 
> There are a few different ways, and a simple one would be to treat as
> a rewind if the change isn't 0..buffer_size.  e.g.
> 
> 	snd_pcm_sframes_t diff = new_ptr - old_ptr;
> 
> 	if (diff >= 0) {
> 		if (diff > buffer_size)
> 			return REWIND;
> 	} else {
> 		if (boundary + diff > buffer_size)
> 			return REWIND;
> 	}
> 	return OK;
> 
> Or, if a rewind is defined to be -buffer_size..-1, it'd be like:
> 
> 	snd_pcm_sframes_t diff = new_ptr - old_ptr;
> 
> 	if (diff >= 0) {
> 		if (boundary - diff <= buffer_size)
> 			return REWIND;
> 	} else {
> 		if (-diff <= buffer_size)
> 			return REWIND;
> 	}
> 	return OK;

ok, I'll trust your math :-)

> In either way, the new_ptr has to be validated beforehand that it's
> within 0..boundary-1.  (old_ptr is assumed to be valid.)

In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
already prior to calling that function
	if (appl_ptr >= runtime->boundary)
		appl_ptr -= runtime->boundary;
	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);


it's rather unclear to me why the same check is not done for sync_ptr, e.g.

if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
	err = pcm_lib_apply_appl_ptr(substream,	
			     sync_ptr.c.control.appl_ptr);

if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
	err = pcm_lib_apply_appl_ptr(substream,
				scontrol.appl_ptr);

Should I add a check there, or add a check inside of
pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
cases?

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 17:15:56 +0200,
Pierre-Louis Bossart wrote:
> 
> > In either way, the new_ptr has to be validated beforehand that it's
> > within 0..boundary-1.  (old_ptr is assumed to be valid.)
> 
> In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
> already prior to calling that function
> 	if (appl_ptr >= runtime->boundary)
> 		appl_ptr -= runtime->boundary;
> 	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);
> 
> 
> it's rather unclear to me why the same check is not done for sync_ptr, e.g.
> 
> if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> 	err = pcm_lib_apply_appl_ptr(substream,	
> 			     sync_ptr.c.control.appl_ptr);
> 
> if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
> 	err = pcm_lib_apply_appl_ptr(substream,
> 				scontrol.appl_ptr);
> 
> Should I add a check there, or add a check inside of
> pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
> cases?

I guess adding in pcm_lib_appl_appl_ptr() would be easier and safer.
There is even one more place that is calling pcm_lib_apply_appl_ptr()
in the very latest commit (a fix for a buggy 32bit compat ioctl).


thanks,

Takashi

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

On 10/12/21 10:27 AM, Takashi Iwai wrote:
> On Tue, 12 Oct 2021 17:15:56 +0200,
> Pierre-Louis Bossart wrote:
>>
>>> In either way, the new_ptr has to be validated beforehand that it's
>>> within 0..boundary-1.  (old_ptr is assumed to be valid.)
>>
>> In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
>> already prior to calling that function
>> 	if (appl_ptr >= runtime->boundary)
>> 		appl_ptr -= runtime->boundary;
>> 	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);
>>
>>
>> it's rather unclear to me why the same check is not done for sync_ptr, e.g.
>>
>> if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
>> 	err = pcm_lib_apply_appl_ptr(substream,	
>> 			     sync_ptr.c.control.appl_ptr);
>>
>> if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
>> 	err = pcm_lib_apply_appl_ptr(substream,
>> 				scontrol.appl_ptr);
>>
>> Should I add a check there, or add a check inside of
>> pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
>> cases?
> 
> I guess adding in pcm_lib_appl_appl_ptr() would be easier and safer.
> There is even one more place that is calling pcm_lib_apply_appl_ptr()
> in the very latest commit (a fix for a buggy 32bit compat ioctl).

ok, here's the code I'll start testing. Thanks a lot Takashi for your help.

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a144a3f68e9e..e839459916ca 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
snd_pcm_substream *substream,
 {
        struct snd_pcm_runtime *runtime = substream->runtime;
        snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+       snd_pcm_sframes_t diff;
        int ret;

        if (old_appl_ptr == appl_ptr)
                return 0;

+       /*
+        * check if a rewind is requested by the application, after
+        * verifying the new appl_ptr is in the 0..boundary range
+        */
+       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
+               if (appl_ptr >= runtime->boundary)
+                       appl_ptr -= runtime->boundary;
+
+               diff = appl_ptr - old_appl_ptr;
+               if (diff >= 0) {
+                       if (diff > runtime->buffer_size)
+                               return 0;
+               } else {
+                       if (runtime->boundary + diff > runtime->buffer_size)
+                               return 0;
+               }
+       }
+
        runtime->control->appl_ptr = appl_ptr;
        if (substream->ops->ack) {
                ret = substream->ops->ack(substream);

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 18:41:19 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 10/12/21 10:27 AM, Takashi Iwai wrote:
> > On Tue, 12 Oct 2021 17:15:56 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >>> In either way, the new_ptr has to be validated beforehand that it's
> >>> within 0..boundary-1.  (old_ptr is assumed to be valid.)
> >>
> >> In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
> >> already prior to calling that function
> >> 	if (appl_ptr >= runtime->boundary)
> >> 		appl_ptr -= runtime->boundary;
> >> 	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);
> >>
> >>
> >> it's rather unclear to me why the same check is not done for sync_ptr, e.g.
> >>
> >> if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> >> 	err = pcm_lib_apply_appl_ptr(substream,	
> >> 			     sync_ptr.c.control.appl_ptr);
> >>
> >> if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
> >> 	err = pcm_lib_apply_appl_ptr(substream,
> >> 				scontrol.appl_ptr);
> >>
> >> Should I add a check there, or add a check inside of
> >> pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
> >> cases?
> > 
> > I guess adding in pcm_lib_appl_appl_ptr() would be easier and safer.
> > There is even one more place that is calling pcm_lib_apply_appl_ptr()
> > in the very latest commit (a fix for a buggy 32bit compat ioctl).
> 
> ok, here's the code I'll start testing. Thanks a lot Takashi for your help.
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a144a3f68e9e..e839459916ca 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
> snd_pcm_substream *substream,
>  {
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> +       snd_pcm_sframes_t diff;
>         int ret;
> 
>         if (old_appl_ptr == appl_ptr)
>                 return 0;
> 
> +       /*
> +        * check if a rewind is requested by the application, after
> +        * verifying the new appl_ptr is in the 0..boundary range
> +        */
> +       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> +               if (appl_ptr >= runtime->boundary)
> +                       appl_ptr -= runtime->boundary;

The boundary check can (or should) be done unconditionally.
It was too naive to assume a sane appl_ptr passed always.
And, it can rather return an error.  So,

	if (appl_ptr >= runtime->boundary)
		return -EINVAL;

	/* check if a rewind is requested by the application */
	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
		diff = appl_ptr - old_appl_ptr;
		....

> +               if (diff >= 0) {
> +                       if (diff > runtime->buffer_size)
> +                               return 0;
> +               } else {
> +                       if (runtime->boundary + diff > runtime->buffer_size)
> +                               return 0;

I'm not sure whether we should return 0 here.  In snd_pcm_rewind() it
returns 0 due to application breakage, though.


thanks,

Takashi

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

>> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
>> index a144a3f68e9e..e839459916ca 100644
>> --- a/sound/core/pcm_lib.c
>> +++ b/sound/core/pcm_lib.c
>> @@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
>> snd_pcm_substream *substream,
>>  {
>>         struct snd_pcm_runtime *runtime = substream->runtime;
>>         snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
>> +       snd_pcm_sframes_t diff;
>>         int ret;
>>
>>         if (old_appl_ptr == appl_ptr)
>>                 return 0;
>>
>> +       /*
>> +        * check if a rewind is requested by the application, after
>> +        * verifying the new appl_ptr is in the 0..boundary range
>> +        */
>> +       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
>> +               if (appl_ptr >= runtime->boundary)
>> +                       appl_ptr -= runtime->boundary;
> 
> The boundary check can (or should) be done unconditionally.
> It was too naive to assume a sane appl_ptr passed always.
> And, it can rather return an error.  So,
> 
> 	if (appl_ptr >= runtime->boundary)
> 		return -EINVAL;

ok, but that would be a separate patch then since it impacts all users,
even without the NO_REWINDS.

> 
> 	/* check if a rewind is requested by the application */
> 	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> 		diff = appl_ptr - old_appl_ptr;
> 		....
> 
>> +               if (diff >= 0) {
>> +                       if (diff > runtime->buffer_size)
>> +                               return 0;
>> +               } else {
>> +                       if (runtime->boundary + diff > runtime->buffer_size)
>> +                               return 0;
> 
> I'm not sure whether we should return 0 here.  In snd_pcm_rewind() it
> returns 0 due to application breakage, though.

We could return -EINVAL indeed, that would keep the work-around in place
for PulseAudio.
Even for other uses, it's not so bad: the selection of NO_REWINDS is an
opt-in, and if a rewind still occurs a big fail would help detect a
configuration issue.

Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 20:02:07 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> >> index a144a3f68e9e..e839459916ca 100644
> >> --- a/sound/core/pcm_lib.c
> >> +++ b/sound/core/pcm_lib.c
> >> @@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
> >> snd_pcm_substream *substream,
> >>  {
> >>         struct snd_pcm_runtime *runtime = substream->runtime;
> >>         snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> >> +       snd_pcm_sframes_t diff;
> >>         int ret;
> >>
> >>         if (old_appl_ptr == appl_ptr)
> >>                 return 0;
> >>
> >> +       /*
> >> +        * check if a rewind is requested by the application, after
> >> +        * verifying the new appl_ptr is in the 0..boundary range
> >> +        */
> >> +       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> >> +               if (appl_ptr >= runtime->boundary)
> >> +                       appl_ptr -= runtime->boundary;
> > 
> > The boundary check can (or should) be done unconditionally.
> > It was too naive to assume a sane appl_ptr passed always.
> > And, it can rather return an error.  So,
> > 
> > 	if (appl_ptr >= runtime->boundary)
> > 		return -EINVAL;
> 
> ok, but that would be a separate patch then since it impacts all users,
> even without the NO_REWINDS.

Makes sense.

> > 	/* check if a rewind is requested by the application */
> > 	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> > 		diff = appl_ptr - old_appl_ptr;
> > 		....
> > 
> >> +               if (diff >= 0) {
> >> +                       if (diff > runtime->buffer_size)
> >> +                               return 0;
> >> +               } else {
> >> +                       if (runtime->boundary + diff > runtime->buffer_size)
> >> +                               return 0;
> > 
> > I'm not sure whether we should return 0 here.  In snd_pcm_rewind() it
> > returns 0 due to application breakage, though.
> 
> We could return -EINVAL indeed, that would keep the work-around in place
> for PulseAudio.
> Even for other uses, it's not so bad: the selection of NO_REWINDS is an
> opt-in, and if a rewind still occurs a big fail would help detect a
> configuration issue.

Yeah, that was my gut feeling ,too.


Takashi