[PATCH 0/3] ALSA: hda: Stream setup fixes
Follow up to the recent hdac_stream-related series of mine [1]. Addresses three topics:
- false-positive timeout (-110) messages that appear during firmware loading procedure - null-ptr-deref when assigning stub substream of COUPLED type. 'Stub' as precisely those kind of substreams are utilized for code-loading procedure - hstream->fifo_size initialization, superfluous '+1'
The two fixes lead the way, there is no dependency of patch 3/3 on either of them.
[1]: https://lore.kernel.org/alsa-devel/20230926080623.43927-1-cezary.rojewski@in...
Cezary Rojewski (3): ALSA: hda: Fix possible null-ptr-deref when assigning a stream ALSA: hda: Fix stream fifo_size initialization ALSA: hda: Add code_loading parameter to stream setup
include/sound/hdaudio.h | 2 +- include/sound/hdaudio_ext.h | 4 ++-- sound/hda/ext/hdac_ext_stream.c | 12 +++++++----- sound/hda/hdac_stream.c | 27 ++++++++++++++++----------- sound/pci/hda/hda_controller.c | 2 +- sound/pci/hda/hda_intel.c | 2 +- sound/soc/intel/avs/pcm.c | 2 +- sound/soc/intel/avs/probes.c | 2 +- sound/soc/intel/skylake/skl-pcm.c | 2 +- 9 files changed, 31 insertions(+), 24 deletions(-)
While AudioDSP drivers assign streams exclusively of HOST or LINK type, nothing blocks a user to attempt to assign a COUPLED stream. As supplied substream instance may be a stub, what is the case when code-loading, such scenario ends with null-ptr-deref.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/hda/hdac_stream.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index 5382894bebab..a132108fba40 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -362,8 +362,10 @@ struct hdac_stream *snd_hdac_stream_assign(struct hdac_bus *bus, struct hdac_stream *res = NULL;
/* make a non-zero unique key for the substream */ - int key = (substream->pcm->device << 16) | (substream->number << 2) | - (substream->stream + 1); + int key = (substream->number << 2) | (substream->stream + 1); + + if (substream->pcm) + key |= (substream->pcm->device << 16);
spin_lock_irq(&bus->reg_lock); list_for_each_entry(azx_dev, &bus->stream_list, list) {
SDxFIFOS register indicates the fifo size directly. There is no need to modify the value after reading the register.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/hda/hdac_stream.c | 2 +- sound/pci/hda/hda_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index a132108fba40..a784fd77cd4b 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -308,7 +308,7 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) if (ret) dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n", AZX_REG_SD_FIFOSIZE, ret); - azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE) + 1; + azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE);
/* when LPIB delay correction gives a small negative value, * we ignore it; currently set the threshold statically to diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index ca765ac4765f..e19274fd990d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -806,7 +806,7 @@ static unsigned int azx_via_get_position(struct azx *chip, mod_dma_pos = le32_to_cpu(*azx_dev->core.posbuf); mod_dma_pos %= azx_dev->core.period_bytes;
- fifo_size = azx_stream(azx_dev)->fifo_size - 1; + fifo_size = azx_stream(azx_dev)->fifo_size;
if (azx_dev->insufficient) { /* Link position never gather than FIFO size */
AudioDSP firmware is the one who kicks SDxFIFOS calculation when a stream is decoupled mode. During firmware bring up procedure, there is no firmware running and the code-loading stream is always a decoupled one. So, there is none to trigger the calculation and we end up with false-positive timeout (-110) messages.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hdaudio.h | 2 +- include/sound/hdaudio_ext.h | 4 ++-- sound/hda/ext/hdac_ext_stream.c | 12 +++++++----- sound/hda/hdac_stream.c | 21 ++++++++++++--------- sound/pci/hda/hda_controller.c | 2 +- sound/soc/intel/avs/pcm.c | 2 +- sound/soc/intel/avs/probes.c | 2 +- sound/soc/intel/skylake/skl-pcm.c | 2 +- 8 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 41d725babf53..dd7c87bbc613 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -573,7 +573,7 @@ void snd_hdac_stream_release(struct hdac_stream *azx_dev); struct hdac_stream *snd_hdac_get_stream(struct hdac_bus *bus, int dir, int stream_tag);
-int snd_hdac_stream_setup(struct hdac_stream *azx_dev); +int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading); void snd_hdac_stream_cleanup(struct hdac_stream *azx_dev); int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev); int snd_hdac_stream_set_params(struct hdac_stream *azx_dev, diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index d32959cb71d2..a8bebac1e4b2 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -60,7 +60,7 @@ struct hdac_ext_stream { bool link_locked:1; bool link_prepared;
- int (*host_setup)(struct hdac_stream *); + int (*host_setup)(struct hdac_stream *, bool);
struct snd_pcm_substream *link_substream; }; @@ -88,7 +88,7 @@ void snd_hdac_ext_stream_start(struct hdac_ext_stream *hext_stream); void snd_hdac_ext_stream_clear(struct hdac_ext_stream *hext_stream); void snd_hdac_ext_stream_reset(struct hdac_ext_stream *hext_stream); int snd_hdac_ext_stream_setup(struct hdac_ext_stream *hext_stream, int fmt); -int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream); +int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream, bool code_loading);
struct hdac_ext_link { struct hdac_bus *bus; diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 186e95bffb28..a3ac738f1130 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -21,12 +21,13 @@ /** * snd_hdac_ext_host_stream_setup - Setup a HOST stream. * @hext_stream: HDAudio stream to set up. + * @code_loading: Whether the stream is for PCM or code-loading. * * Return: Zero on success or negative error code. */ -int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream) +int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream, bool code_loading) { - return hext_stream->host_setup(hdac_stream(hext_stream)); + return hext_stream->host_setup(hdac_stream(hext_stream), code_loading); } EXPORT_SYMBOL_GPL(snd_hdac_ext_host_stream_setup);
@@ -34,16 +35,17 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_host_stream_setup); * snd_hdac_apl_host_stream_setup - Setup a HOST stream following procedure * recommended for ApolloLake devices. * @hstream: HDAudio stream to set up. + * @code_loading: Whether the stream is for PCM or code-loading. * * Return: Zero on success or negative error code. */ -static int snd_hdac_apl_host_stream_setup(struct hdac_stream *hstream) +static int snd_hdac_apl_host_stream_setup(struct hdac_stream *hstream, bool code_loading) { struct hdac_ext_stream *hext_stream = stream_to_hdac_ext_stream(hstream); int ret;
snd_hdac_ext_stream_decouple(hstream->bus, hext_stream, false); - ret = snd_hdac_stream_setup(hstream); + ret = snd_hdac_stream_setup(hstream, code_loading); snd_hdac_ext_stream_decouple(hstream->bus, hext_stream, true);
return ret; @@ -89,7 +91,7 @@ int snd_hdac_ext_stream_init_all(struct hdac_bus *bus, int start_idx, int num_stream, int dir) { struct pci_dev *pci = to_pci_dev(bus->dev); - int (*setup_op)(struct hdac_stream *); + int (*setup_op)(struct hdac_stream *, bool); int stream_tag = 0; int i, tag, idx = start_idx;
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index a784fd77cd4b..6ce24e248f8e 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -252,8 +252,9 @@ EXPORT_SYMBOL_GPL(snd_hdac_stream_reset); /** * snd_hdac_stream_setup - set up the SD for streaming * @azx_dev: HD-audio core stream to set up + * @code_loading: Whether the stream is for PCM or code-loading. */ -int snd_hdac_stream_setup(struct hdac_stream *azx_dev) +int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading) { struct hdac_bus *bus = azx_dev->bus; struct snd_pcm_runtime *runtime; @@ -302,13 +303,15 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) /* set the interrupt enable bits in the descriptor control register */ snd_hdac_stream_updatel(azx_dev, SD_CTL, 0, SD_INT_MASK);
- /* Once SDxFMT is set, the controller programs SDxFIFOS to non-zero value. */ - ret = snd_hdac_stream_readw_poll(azx_dev, SD_FIFOSIZE, reg, reg & AZX_SD_FIFOSIZE_MASK, - 3, 300); - if (ret) - dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n", - AZX_REG_SD_FIFOSIZE, ret); - azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE); + if (!code_loading) { + /* Once SDxFMT is set, the controller programs SDxFIFOS to non-zero value. */ + ret = snd_hdac_stream_readw_poll(azx_dev, SD_FIFOSIZE, reg, + reg & AZX_SD_FIFOSIZE_MASK, 3, 300); + if (ret) + dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n", + AZX_REG_SD_FIFOSIZE, ret); + azx_dev->fifo_size = reg; + }
/* when LPIB delay correction gives a small negative value, * we ignore it; currently set the threshold statically to @@ -953,7 +956,7 @@ int snd_hdac_dsp_prepare(struct hdac_stream *azx_dev, unsigned int format, if (err < 0) goto error;
- snd_hdac_stream_setup(azx_dev); + snd_hdac_stream_setup(azx_dev, true); snd_hdac_dsp_unlock(azx_dev); return azx_dev->stream_tag;
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index 406779625fb5..c42e9ffff9db 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -182,7 +182,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream) if (err < 0) goto unlock;
- snd_hdac_stream_setup(azx_stream(azx_dev)); + snd_hdac_stream_setup(azx_stream(azx_dev), false);
stream_tag = azx_dev->core.stream_tag; /* CA-IBG chips need the playback stream starting from 1 */ diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index e628fdfdc018..9b0b4d700675 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -625,7 +625,7 @@ static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_so if (ret < 0) return ret;
- ret = snd_hdac_ext_host_stream_setup(host_stream); + ret = snd_hdac_ext_host_stream_setup(host_stream, false); if (ret < 0) return ret;
diff --git a/sound/soc/intel/avs/probes.c b/sound/soc/intel/avs/probes.c index 4cab8c6c4576..bdc6b30dc009 100644 --- a/sound/soc/intel/avs/probes.c +++ b/sound/soc/intel/avs/probes.c @@ -145,7 +145,7 @@ static int avs_probe_compr_set_params(struct snd_compr_stream *cstream, ret = snd_hdac_stream_set_params(hdac_stream(host_stream), format_val); if (ret < 0) return ret; - ret = snd_hdac_stream_setup(hdac_stream(host_stream)); + ret = snd_hdac_stream_setup(hdac_stream(host_stream), false); if (ret < 0) return ret;
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 7502b2e70e46..2cbcba7c1dbc 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -148,7 +148,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) if (err < 0) return err;
- err = snd_hdac_ext_host_stream_setup(stream); + err = snd_hdac_ext_host_stream_setup(stream, false); if (err < 0) return err;
On Fri, Oct 06, 2023 at 12:28:57PM +0200, Cezary Rojewski wrote:
AudioDSP firmware is the one who kicks SDxFIFOS calculation when a stream is decoupled mode. During firmware bring up procedure, there is no firmware running and the code-loading stream is always a decoupled one. So, there is none to trigger the calculation and we end up with false-positive timeout (-110) messages.
Acked-by: Mark Brown broonie@kernel.org
On Fri, 06 Oct 2023 12:28:54 +0200, Cezary Rojewski wrote:
Follow up to the recent hdac_stream-related series of mine [1]. Addresses three topics:
- false-positive timeout (-110) messages that appear during firmware loading procedure
- null-ptr-deref when assigning stub substream of COUPLED type. 'Stub' as precisely those kind of substreams are utilized for code-loading procedure
- hstream->fifo_size initialization, superfluous '+1'
The two fixes lead the way, there is no dependency of patch 3/3 on either of them.
Cezary Rojewski (3): ALSA: hda: Fix possible null-ptr-deref when assigning a stream ALSA: hda: Fix stream fifo_size initialization ALSA: hda: Add code_loading parameter to stream setup
Applied all three patches now. Thanks.
Takashi
participants (3)
-
Cezary Rojewski
-
Mark Brown
-
Takashi Iwai