From 0e888a74e52db369e19aec908131cf171079b306 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Fri, 19 Nov 2021 17:08:49 -0600 Subject: [PATCH 1/4] ALSA: pcm: unconditionally check if appl_ptr is in 0..boundary range In some cases, the appl_ptr passed by userspace is not checked before being used. This patch adds an unconditional check and returns an error code should the appl_ptr exceed the ALSA 'boundary'. Suggested-by: Takashi Iwai Reviewed-by: Takashi Iwai Reviewed-by: Ranjani Sridharan Reviewed-by: Kai Vehmanen Signed-off-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20211119230852.206310-2-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- sound/core/pcm_lib.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 4f4b4739f987..fdd992772b20 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2133,6 +2133,9 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream, if (old_appl_ptr == appl_ptr) return 0; + if (appl_ptr >= runtime->boundary) + return -EINVAL; + runtime->control->appl_ptr = appl_ptr; if (substream->ops->ack) { ret = substream->ops->ack(substream); From b456abe63f60ad93c83a526d33b71574bc32656c Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Fri, 19 Nov 2021 17:08:50 -0600 Subject: [PATCH 2/4] ALSA: pcm: introduce INFO_NO_REWINDS flag When the hardware can only deal with a monotonically increasing appl_ptr, this flag can be set. In case the application requests a rewind, be it with a snd_pcm_rewind() or with a direct change of a mmap'ed pointer followed by a SNDRV_PCM_IOCTL_SYNC_PTR, this patch checks if a rewind occurred and returns an error. Credits to Takashi Iwai for identifying the path with SYNC_PTR and suggesting the pointer checks. Suggested-by: Takashi Iwai Reviewed-by: Takashi Iwai Reviewed-by: Ranjani Sridharan Reviewed-by: Kai Vehmanen Signed-off-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20211119230852.206310-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- include/uapi/sound/asound.h | 2 +- sound/core/pcm_lib.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 5fbb79e30819..ff7e638221c5 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_lib.c b/sound/core/pcm_lib.c index fdd992772b20..f2090025236b 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2128,6 +2128,7 @@ 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) @@ -2135,6 +2136,19 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream, 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 -EINVAL; + } else { + if (runtime->boundary + diff > runtime->buffer_size) + return -EINVAL; + } + } runtime->control->appl_ptr = appl_ptr; if (substream->ops->ack) { From 4a39ea3f07f14f21a6b97e78c972f71fc5761d3a Mon Sep 17 00:00:00 2001 From: Ranjani Sridharan Date: Fri, 19 Nov 2021 17:08:51 -0600 Subject: [PATCH 3/4] ASoC: SOF: pcm: add .ack callback support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Kai Vehmanen Signed-off-by: Ranjani Sridharan Signed-off-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20211119230852.206310-4-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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 a0648a13e3eb..0226a53148c9 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -487,6 +487,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 31dd79b794f1..98aa5a6579e3 100644 --- a/sound/soc/sof/pcm.c +++ b/sound/soc/sof/pcm.c @@ -917,6 +917,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; @@ -935,6 +943,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 a9c5197617f1..16caf5c74035 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -207,6 +207,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, From 6c26b5054ce2b822856e32f1840d13f777c6f295 Mon Sep 17 00:00:00 2001 From: Ranjani Sridharan Date: Fri, 19 Nov 2021 17:08:52 -0600 Subject: [PATCH 4/4] ASoC: SOF: Intel: add .ack support for HDaudio platforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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_SYNC_APPLPTR 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 Reviewed-by: Kai Vehmanen Co-developed-by: Pierre-Louis Bossart Signed-off-by: Pierre-Louis Bossart Signed-off-by: Ranjani Sridharan Link: https://lore.kernel.org/r/20211119230852.206310-5-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown --- 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/icl.c | 1 + sound/soc/sof/intel/tgl.c | 1 + 7 files changed, 46 insertions(+), 2 deletions(-) diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c index 1baf0fddeb3d..8778f46f1d37 100644 --- a/sound/soc/sof/intel/apl.c +++ b/sound/soc/sof/intel/apl.c @@ -78,6 +78,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 d455272bfc8e..04daaa6100f1 100644 --- a/sound/soc/sof/intel/cnl.c +++ b/sound/soc/sof/intel/cnl.c @@ -283,6 +283,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..974383cd0440 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 *hext_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, hext_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 control from being mapped. + * The status can still be mapped. + */ + if (hda_disable_rewinds) + runtime->hw.info |= SNDRV_PCM_INFO_NO_REWINDS | SNDRV_PCM_INFO_SYNC_APPLPTR; + /* * 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 646f5d4dc882..8ed4031ca007 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -534,6 +534,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/icl.c b/sound/soc/sof/intel/icl.c index 6c5422157ec8..343c1af7c453 100644 --- a/sound/soc/sof/intel/icl.c +++ b/sound/soc/sof/intel/icl.c @@ -77,6 +77,7 @@ const struct snd_sof_dsp_ops sof_icl_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/tgl.c b/sound/soc/sof/intel/tgl.c index 237e92e790b7..7f7929c5cb88 100644 --- a/sound/soc/sof/intel/tgl.c +++ b/sound/soc/sof/intel/tgl.c @@ -113,6 +113,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 */