May 2020 - Alsa-devel - mailman.alsa-project.org

[Bug Report]Sound: sound/core/hwdep.c undefined result by left shifting 1 by 31
by Changming Liu 25 May '20

25 May '20

Hi Jaroslav, Takashi: Greetings, I'm a first year PhD student who is interested in using UBSan for linux. After some experiments, I found that in sound/core/hwdep.c function snd_hwdep_dsp_load there might be an undefined behavior that might cause unexpected result. More specifically, in this function,info was fetched from user space and, info.index was checked if it's greater than or equal to 32. If not then it's used as number of left shift bits to string literal 1. The problem is, since string literal 1 is by default signed int, so 1 << 31 cannot be represented as a valid integer and the result might be undefined across different platforms. So I guess change 1 to 1U might help? Due to the lack of knowledge of the interaction between this module and others, I'm not able to assess if this is critical or worth fixing. I'd appreciate if for your comment on this bug. This can help me understand UB a lot! Looking forward to your response. Best, Changming Liu

2 2

[PATCH v4 0/7] ASoC: add soc-link
by Kuninori Morimoto 25 May '20

25 May '20

Hi Mark Current ALSA SoC is handling dai_link related operation, but it is implmemented directly without using function/macro, and at random place. This v4 patch-set creates new snd_soc_link_xxx() functions which handles dai_link related operation, and implmement these at new soc-link.c. v3 -> v4 - add Reviewed-by from Ranjani and Pierre-Louis - fix bisection error at [2/7] v2 -> v3 - add missing #include <sound/soc-link.h> in soc-link.c v1 -> v2 - #include <sound/soc-link.h> is added on each c source file instead of soc.h - not have extra error message after snd_soc_link_xxx(), because it already indicate it via snc_link_ret() - snd_soc_link_compr_xxx() doesn't have rtd parameter, because it can be created from cstream Link: https://lore.kernel.org/r/87lflk4yk3.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/874ksa59wc.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/877dx868op.wl-kuninori.morimoto.gx@renesas.com Kuninori Morimoto (7): ASoC: add soc-link.c ASoC: soc-link: move soc_rtd_xxx() ASoC: soc-link: remove unneeded parameter from snd_soc_link_xxx() ASoC: soc-link: add snd_soc_link_be_hw_params_fixup() ASoC: soc-link: add snd_soc_link_compr_startup() ASoC: soc-link: add snd_soc_link_compr_shutdown() ASoC: soc-link: add snd_soc_link_compr_set_params() include/sound/soc-link.h | 27 +++++++ sound/soc/Makefile | 2 +- sound/soc/soc-compress.c | 46 ++++-------- sound/soc/soc-core.c | 18 ++--- sound/soc/soc-dai.c | 9 ++- sound/soc/soc-link.c | 150 +++++++++++++++++++++++++++++++++++++++ sound/soc/soc-pcm.c | 86 ++++------------------ 7 files changed, 219 insertions(+), 119 deletions(-) create mode 100644 include/sound/soc-link.h create mode 100644 sound/soc/soc-link.c -- 2.17.1 Thank you for your help !! Best regards --- Kuninori Morimoto

2 8

[PATCH] ASoC: fsl: imx-audmix: Fix unused assignment to variable 'ret'
by Tang Bin 25 May '20

25 May '20

Omit unused initialized value, because 'ret' will be assigined by the function snd_soc_component_read(). Signed-off-by: Zhang Shengju <zhangshengju(a)cmss.chinamobile.com> Signed-off-by: Tang Bin <tangbin(a)cmss.chinamobile.com> --- sound/soc/fsl/fsl_audmix.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c index 5faecbeb5..8b9027f76 100644 --- a/sound/soc/fsl/fsl_audmix.c +++ b/sound/soc/fsl/fsl_audmix.c @@ -116,7 +116,7 @@ static int fsl_audmix_put_mix_clk_src(struct snd_kcontrol *kcontrol, struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; unsigned int *item = ucontrol->value.enumerated.item; unsigned int reg_val, val, mix_clk; - int ret = 0; + int ret; /* Get current state */ ret = snd_soc_component_read(comp, FSL_AUDMIX_CTR, &reg_val); @@ -159,7 +159,7 @@ static int fsl_audmix_put_out_src(struct snd_kcontrol *kcontrol, unsigned int *item = ucontrol->value.enumerated.item; u32 out_src, mix_clk; unsigned int reg_val, val, mask = 0, ctr = 0; - int ret = 0; + int ret; /* Get current state */ ret = snd_soc_component_read(comp, FSL_AUDMIX_CTR, &reg_val); -- 2.20.1.windows.1

2 1

[PATCH] ASoC: fsl_asrc: Fix -Wmissing-prototypes warning
by Shengjiu Wang 25 May '20

25 May '20

From: shengjiu wang <shengjiu.wang(a)nxp.com> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 sound/soc/fsl/fsl_asrc.c:557:18: warning: no previous prototype for function 'fsl_asrc_get_dma_channel' [-Wmissing-prototypes] struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir) ^ sound/soc/fsl/fsl_asrc.c:557:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir) ^ static Fixes: be7bd03f0201 ("ASoC: fsl_asrc: Move common definition to fsl_asrc_common") Reported-by: kbuild test robot <lkp(a)intel.com> Signed-off-by: shengjiu wang <shengjiu.wang(a)nxp.com> --- sound/soc/fsl/fsl_asrc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 7e2d598ffb81..d79d2c7f8d36 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -554,7 +554,8 @@ static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair) /** * Get DMA channel according to the pair and direction. */ -struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir) +static struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, + bool dir) { struct fsl_asrc *asrc = pair->asrc; enum asrc_pair_index index = pair->index; @@ -564,7 +565,6 @@ struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir) return dma_request_slave_channel(&asrc->pdev->dev, name); } -EXPORT_SYMBOL_GPL(fsl_asrc_get_dma_channel); static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) -- 2.21.0

2 1

[PATCH] ASoC: fsl_asrc: Merge suspend/resume function to runtime_suspend/resume
by Shengjiu Wang 25 May '20

25 May '20

With dedicated power domain for asrc, power can be disabled after probe and pm runtime suspend, then the value of all registers need to be restored in pm runtime resume. So we can merge suspend/resume function to runtime_suspend/resume function and enable regcache only in end of probe. Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> --- sound/soc/fsl/fsl_asrc.c | 70 ++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 432936039de4..3ebbe15ac378 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -1100,6 +1100,7 @@ static int fsl_asrc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, asrc); pm_runtime_enable(&pdev->dev); spin_lock_init(&asrc->lock); + regcache_cache_only(asrc->regmap, true); ret = devm_snd_soc_register_component(&pdev->dev, &fsl_asrc_component, &fsl_asrc_dai, 1); @@ -1117,6 +1118,7 @@ static int fsl_asrc_runtime_resume(struct device *dev) struct fsl_asrc *asrc = dev_get_drvdata(dev); struct fsl_asrc_priv *asrc_priv = asrc->private; int i, ret; + u32 asrctr; ret = clk_prepare_enable(asrc->mem_clk); if (ret) @@ -1135,6 +1137,24 @@ static int fsl_asrc_runtime_resume(struct device *dev) goto disable_asrck_clk; } + /* Stop all pairs provisionally */ + regmap_read(asrc->regmap, REG_ASRCTR, &asrctr); + regmap_update_bits(asrc->regmap, REG_ASRCTR, + ASRCTR_ASRCEi_ALL_MASK, 0); + + /* Restore all registers */ + regcache_cache_only(asrc->regmap, false); + regcache_mark_dirty(asrc->regmap); + regcache_sync(asrc->regmap); + + regmap_update_bits(asrc->regmap, REG_ASRCFG, + ASRCFG_NDPRi_ALL_MASK | ASRCFG_POSTMODi_ALL_MASK | + ASRCFG_PREMODi_ALL_MASK, asrc_priv->regcache_cfg); + + /* Restart enabled pairs */ + regmap_update_bits(asrc->regmap, REG_ASRCTR, + ASRCTR_ASRCEi_ALL_MASK, asrctr); + return 0; disable_asrck_clk: @@ -1155,6 +1175,11 @@ static int fsl_asrc_runtime_suspend(struct device *dev) struct fsl_asrc_priv *asrc_priv = asrc->private; int i; + regmap_read(asrc->regmap, REG_ASRCFG, + &asrc_priv->regcache_cfg); + + regcache_cache_only(asrc->regmap, true); + for (i = 0; i < ASRC_CLK_MAX_NUM; i++) clk_disable_unprepare(asrc_priv->asrck_clk[i]); if (!IS_ERR(asrc->spba_clk)) @@ -1166,51 +1191,10 @@ static int fsl_asrc_runtime_suspend(struct device *dev) } #endif /* CONFIG_PM */ -#ifdef CONFIG_PM_SLEEP -static int fsl_asrc_suspend(struct device *dev) -{ - struct fsl_asrc *asrc = dev_get_drvdata(dev); - struct fsl_asrc_priv *asrc_priv = asrc->private; - - regmap_read(asrc->regmap, REG_ASRCFG, - &asrc_priv->regcache_cfg); - - regcache_cache_only(asrc->regmap, true); - regcache_mark_dirty(asrc->regmap); - - return 0; -} - -static int fsl_asrc_resume(struct device *dev) -{ - struct fsl_asrc *asrc = dev_get_drvdata(dev); - struct fsl_asrc_priv *asrc_priv = asrc->private; - u32 asrctr; - - /* Stop all pairs provisionally */ - regmap_read(asrc->regmap, REG_ASRCTR, &asrctr); - regmap_update_bits(asrc->regmap, REG_ASRCTR, - ASRCTR_ASRCEi_ALL_MASK, 0); - - /* Restore all registers */ - regcache_cache_only(asrc->regmap, false); - regcache_sync(asrc->regmap); - - regmap_update_bits(asrc->regmap, REG_ASRCFG, - ASRCFG_NDPRi_ALL_MASK | ASRCFG_POSTMODi_ALL_MASK | - ASRCFG_PREMODi_ALL_MASK, asrc_priv->regcache_cfg); - - /* Restart enabled pairs */ - regmap_update_bits(asrc->regmap, REG_ASRCTR, - ASRCTR_ASRCEi_ALL_MASK, asrctr); - - return 0; -} -#endif /* CONFIG_PM_SLEEP */ - static const struct dev_pm_ops fsl_asrc_pm = { SET_RUNTIME_PM_OPS(fsl_asrc_runtime_suspend, fsl_asrc_runtime_resume, NULL) - SET_SYSTEM_SLEEP_PM_OPS(fsl_asrc_suspend, fsl_asrc_resume) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) }; static const struct fsl_asrc_soc_data fsl_asrc_imx35_data = { -- 2.21.0

4 5

[PATCH v3 0/7] ASoC: add soc-link
by Kuninori Morimoto 25 May '20

25 May '20

Hi Mark Current ALSA SoC is handling dai_link related operation, but it is implmemented directly without using function/macro, and at random place. This v3 patch-set creates new snd_soc_link_xxx() functions which handles dai_link related operation, and implmement these at new soc-link.c. v2 -> v3 - add missing #include <sound/soc-link.h> in soc-link.c v1 -> v2 - #include <sound/soc-link.h> is added on each c source file instead of soc.h - not have extra error message after snd_soc_link_xxx(), because it already indicate it via snc_link_ret() - snd_soc_link_compr_xxx() doesn't have rtd parameter, because it can be created from cstream Link: https://lore.kernel.org/r/874ksa59wc.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/877dx868op.wl-kuninori.morimoto.gx@renesas.com Kuninori Morimoto (7): ASoC: add soc-link.c ASoC: soc-link: move soc_rtd_xxx() ASoC: soc-link: remove unneeded parameter from snd_soc_link_xxx() ASoC: soc-link: add snd_soc_link_be_hw_params_fixup() ASoC: soc-link: add snd_soc_link_compr_startup() ASoC: soc-link: add snd_soc_link_compr_shutdown() ASoC: soc-link: add snd_soc_link_compr_set_params() include/sound/soc-link.h | 27 +++++++ sound/soc/Makefile | 2 +- sound/soc/soc-compress.c | 46 ++++-------- sound/soc/soc-core.c | 18 ++--- sound/soc/soc-dai.c | 9 ++- sound/soc/soc-link.c | 150 +++++++++++++++++++++++++++++++++++++++ sound/soc/soc-pcm.c | 86 ++++------------------ 7 files changed, 219 insertions(+), 119 deletions(-) create mode 100644 include/sound/soc-link.h create mode 100644 sound/soc/soc-link.c -- 2.17.1 Thank you for your help !! Best regards --- Kuninori Morimoto

3 12

[PATCH 0/5] ASoC: SOF: extended manifest support for 5.8
by Kai Vehmanen 25 May '20

25 May '20

Hello, extended firmware manifest is a method to retrieve capabilities directly from the firmware file instead of routing the information via the DSP and reading it back via IPC (latter mechanism still supported but will be deprecated). This feature was briefly merged to 5.8 with the series sent on 2020-Apr-15, but due to a regression hit with exporting uapi headers, the patches got dropped. Here's an update with the uapi header issue fixed, rebased to latest 'for-5.8' and a few minor fixes. This has been sitting in sof-dev for some weeks and no further issues have been found. We also added a check for the uapi-export case to SOF CI, so such errors would not slip through again in the future. Tooling support to create firmware files with an extended header is available in SOF firmware repository (see the rimage tool) and this part is already merged. Karol Trzcinski (5): ASoC: SOF: loader: Adjust validation condition for fw_offset ASoC: SOF: Introduce extended manifest ASoC: SOF: ext_manifest: parse firmware version ASoC: SOF: ext_manifest: parse windows ASoC: SOF: ext_manifest: parse compiler version include/sound/sof/ext_manifest.h | 95 +++++++++++++++++ sound/soc/sof/intel/hda-loader.c | 2 +- sound/soc/sof/loader.c | 176 ++++++++++++++++++++++++++++++- 3 files changed, 269 insertions(+), 4 deletions(-) create mode 100644 include/sound/sof/ext_manifest.h -- 2.26.2

2 6

BUG: aplay (arecord) vu-meter overwrites /dev/null
by Clemens Koller 25 May '20

25 May '20

Hello! I am using the vu-meter of arecord v1.2.2 for monitoring remote computers audio input, which is usually (according to web search) done as: $ arecord -f S16_LE -r 44100 -c 2 --vumeter=stereo /dev/null This command was run as root, which is obviously not good, but still... every once in a while, I ran into some state, where /dev/null gets overwritten with (corrupted) audio data immediately upon start or after a long time (i.e. when max_filesize is reached). I would expect that this is not the desired behaviour. I was reading some code at: https://github.com/alsa-project/alsa-utils/blob/master/aplay/aplay.c#L3152 So I guess I could use: $ arecord -f S16_LE -r 44100 -c 2 --vumeter=stereo - /dev/null instead. But I am still not sure not to corrupt /dev/null when * max_filesize is reached or * we get a SIGUSR1 which will trigger recycle_capture_file Can somebody shed some light on how this is supposed to work? This code is very old and the documentation is still missing some bits and pieces there. There are also some commits trying to fix this behaviour - i.e. https://github.com/alsa-project/alsa-utils/commit/85827fbb642463ab724a9471a… I am able to assist with further debugging if necessary or dig deeper into the code. But I am not too familiar with this grown piece. Thank you very much! Clemens Koller

1 0

[PATCH] add I2C device and compatible ID
by Seven Lee 25 May '20

25 May '20

The nau8810 driver can also compatible with nau8812 and nau8814. Signed-off-by: Seven Lee <wtli(a)nuvoton.com> --- .../devicetree/bindings/sound/nau8812.txt | 16 ++++++++++++++++ .../devicetree/bindings/sound/nau8814.txt | 16 ++++++++++++++++ sound/soc/codecs/nau8810.c | 4 ++++ 3 files changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/nau8812.txt create mode 100644 Documentation/devicetree/bindings/sound/nau8814.txt diff --git a/Documentation/devicetree/bindings/sound/nau8812.txt b/Documentation/devicetree/bindings/sound/nau8812.txt new file mode 100644 index 000000000000..7aab29586a1f --- /dev/null +++ b/Documentation/devicetree/bindings/sound/nau8812.txt @@ -0,0 +1,16 @@ +NAU8812 audio CODEC + +This device supports I2C only. + +Required properties: + + - compatible : "nuvoton,nau8812" + + - reg : the I2C address of the device. + +Example: + +codec: nau8812@1a { + compatible = "nuvoton,nau8812"; + reg = <0x1a>; +}; diff --git a/Documentation/devicetree/bindings/sound/nau8814.txt b/Documentation/devicetree/bindings/sound/nau8814.txt new file mode 100644 index 000000000000..24bf2ba3d23b --- /dev/null +++ b/Documentation/devicetree/bindings/sound/nau8814.txt @@ -0,0 +1,16 @@ +NAU8814 audio CODEC + +This device supports I2C only. + +Required properties: + + - compatible : "nuvoton,nau8814" + + - reg : the I2C address of the device. + +Example: + +codec: nau8814@1a { + compatible = "nuvoton,nau8814"; + reg = <0x1a>; +}; diff --git a/sound/soc/codecs/nau8810.c b/sound/soc/codecs/nau8810.c index efe9fb3fb225..33ebc6398426 100644 --- a/sound/soc/codecs/nau8810.c +++ b/sound/soc/codecs/nau8810.c @@ -895,6 +895,8 @@ static int nau8810_i2c_probe(struct i2c_client *i2c, static const struct i2c_device_id nau8810_i2c_id[] = { { "nau8810", 0 }, + { "nau8812", 0 }, + { "nau8814", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, nau8810_i2c_id); @@ -902,6 +904,8 @@ MODULE_DEVICE_TABLE(i2c, nau8810_i2c_id); #ifdef CONFIG_OF static const struct of_device_id nau8810_of_match[] = { { .compatible = "nuvoton,nau8810", }, + { .compatible = "nuvoton,nau8812", }, + { .compatible = "nuvoton,nau8814", }, { } }; MODULE_DEVICE_TABLE(of, nau8810_of_match); -- 2.26.2

2 2

XHCI vs PCM2903B/PCM2904 part 2
by Rik van Riel 25 May '20

25 May '20

After a few more weeks of digging, I have come to the tentative conclusion that either the XHCI driver, or the USB sound driver, or both, fail to handle USB errors correctly. I have some questions at the bottom, after a (brief-ish) explanation of exactly what seems to go wrong. TL;DR: arecord from a misbehaving device can hang forever after a USB error, due to poll on /dev/snd/timer never returning. The details: under some mysterious circumstances, the PCM290x family sound chips can send more data than expected during an isochronous transfer, leading to a babble error. Those circumstances seem to in part depend on the USB host controller and/or the electrical environment, since the chips work just fine for most people. Receiving data past the end of the isochronous transfer window scheduled for a device results in the XHCI controller throwing a babble error, which moves the endpoint into halted state. This is followed by the host controller software sending a reset endpoint command, and moving the endpoint into stopped state, as specified on pages 164-165 of the XHCI specification. However, the USB sound driver seems to have no idea that this error happened. The function retire_capture_urb looks at the status of each isochronous frame, but seems to be under the assumption that the sound device just keeps on running. The function snd_complete_urb seems to only detect that the device is not running if usb_submit_urb returns a failure. err = usb_submit_urb(urb, GFP_ATOMIC); if (err == 0) return; usb_audio_err(ep->chip, "cannot submit urb (err = %d)\n", err); if (ep->data_subs && ep->data_subs->pcm_substream) { substream = ep->data_subs->pcm_substream; snd_pcm_stop_xrun(substream); } However, the XHCI driver will happily submit an URB to a stopped device. Looking at the call trace usb_submit_urb -> xhci_urb_enqueue -> xhci_queue_isoc_tx_prepare -> prepare_ring, you can see this code: /* Make sure the endpoint has been added to xHC schedule */ switch (ep_state) { ... case EP_STATE_HALTED: xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n"); case EP_STATE_STOPPED: case EP_STATE_RUNNING: break; This leads me to a few questions: - should retire_capture_urb call snd_pcm_stop_xrun, or another function like it, if it sees certain errors in the iso frame in the URB? - should snd_complete_urb do something with these errors, too, in case they happen on the sync frames and not the data frames? - does the XHCI code need to ring the doorbell when submitting an URB to a stopped device, or is it always up to the higher-level driver to fully reset the device before it can do anything useful? - if a device in stopped state does not do anything useful, should usb_submit_urb return an error? - how should the USB sound driver recover from these occasional and/or one-off errors? stop the sound stream, or try to reinitialize the device and start recording again? I am willing to write patches and can test with my setup, but both the sound code and the USB code are new to me so I would like to know what direction I should go in :) -- All Rights Reversed.

4 8