November 2019 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [linux-next:master 10267/11220] sound/soc/codecs/rt5677.c:5246:6: sparse: sparse: symbol 'rt5677_check_hotword' was not declared. Should it be static?
by kbuild test robot 18 Nov '19

18 Nov '19

tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master head: 4e8f108c3af2d6922a64df9f3d3d488c74f6009d commit: 21c00e5df4397870ee835c974bf50570f9d24253 [10267/11220] ASoC: rt5677: Enable jack detect while DSP is running reproduce: # apt-get install sparse # sparse version: v0.6.1-31-gfd3528a-dirty git checkout 21c00e5df4397870ee835c974bf50570f9d24253 make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp(a)intel.com> sparse warnings: (new ones prefixed by >>) sound/soc/codecs/rt5677.c:4700:17: sparse: sparse: dubious: x | !y >> sound/soc/codecs/rt5677.c:5246:6: sparse: sparse: symbol 'rt5677_check_hotword' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

3 3

[alsa-devel] [PATCH] ASoC: wm2200: add missed operations in remove and probe failure
by Chuhong Yuan 18 Nov '19

18 Nov '19

This driver misses calls to pm_runtime_disable and regulator_bulk_disable in remove and a call to free_irq in probe failure. Add the calls to fix it. Signed-off-by: Chuhong Yuan <hslester96(a)gmail.com> --- sound/soc/codecs/wm2200.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sound/soc/codecs/wm2200.c b/sound/soc/codecs/wm2200.c index cf64e109c658..7b087d94141b 100644 --- a/sound/soc/codecs/wm2200.c +++ b/sound/soc/codecs/wm2200.c @@ -2410,6 +2410,8 @@ static int wm2200_i2c_probe(struct i2c_client *i2c, err_pm_runtime: pm_runtime_disable(&i2c->dev); + if (i2c->irq) + free_irq(i2c->irq, wm2200); err_reset: if (wm2200->pdata.reset) gpio_set_value_cansleep(wm2200->pdata.reset, 0); @@ -2426,12 +2428,15 @@ static int wm2200_i2c_remove(struct i2c_client *i2c) { struct wm2200_priv *wm2200 = i2c_get_clientdata(i2c); + pm_runtime_disable(&i2c->dev); if (i2c->irq) free_irq(i2c->irq, wm2200); if (wm2200->pdata.reset) gpio_set_value_cansleep(wm2200->pdata.reset, 0); if (wm2200->pdata.ldo_ena) gpio_set_value_cansleep(wm2200->pdata.ldo_ena, 0); + regulator_bulk_disable(ARRAY_SIZE(wm2200->core_supplies), + wm2200->core_supplies); return 0; } -- 2.24.0

3 2

[alsa-devel] [PATCH] ASoC: wm5100: add missed pm_runtime_disable
by Chuhong Yuan 18 Nov '19

18 Nov '19

The driver forgets to call pm_runtime_disable in remove and probe failure. Add the calls to fix it. Signed-off-by: Chuhong Yuan <hslester96(a)gmail.com> --- sound/soc/codecs/wm5100.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c index 4af0e519e623..91cc63c5a51f 100644 --- a/sound/soc/codecs/wm5100.c +++ b/sound/soc/codecs/wm5100.c @@ -2617,6 +2617,7 @@ static int wm5100_i2c_probe(struct i2c_client *i2c, return ret; err_reset: + pm_runtime_disable(&i2c->dev); if (i2c->irq) free_irq(i2c->irq, wm5100); wm5100_free_gpio(i2c); @@ -2640,6 +2641,7 @@ static int wm5100_i2c_remove(struct i2c_client *i2c) { struct wm5100_priv *wm5100 = i2c_get_clientdata(i2c); + pm_runtime_disable(&i2c->dev); if (i2c->irq) free_irq(i2c->irq, wm5100); wm5100_free_gpio(i2c); -- 2.24.0

3 2

[alsa-devel] compression format of alsa-ucm-conf and alsa-topology-conf tarballs
by Lars Wendler 18 Nov '19

18 Nov '19

Hello lists, some shell output says more than any explanation: lars@abudhabi:~/temp/alsa> wget -q https://www.alsa-project.org/files/pub/lib/alsa-topology-conf-1.2.1.tar.bz2 lars@abudhabi:~/temp/alsa> wget -q https://www.alsa-project.org/files/pub/lib/alsa-ucm-conf-1.2.1.tar.bz2 lars@abudhabi:~/temp/alsa> tar -xjf alsa-topology-conf-1.2.1.tar.bz2 bzip2: (stdin) is not a bzip2 file. tar: Child returned status 2 tar: Error is not recoverable: exiting now lars@abudhabi:~/temp/alsa(2)> tar -xjf alsa-ucm-conf-1.2.1.tar.bz2 bzip2: (stdin) is not a bzip2 file. tar: Child returned status 2 tar: Error is not recoverable: exiting now lars@abudhabi:~/temp/alsa(2)> file alsa-topology-conf-1.2.1.tar.bz2 alsa-topology-conf-1.2.1.tar.bz2: XZ compressed data lars@abudhabi:~/temp/alsa> file alsa-ucm-conf-1.2.1.tar.bz2 alsa-ucm-conf-1.2.1.tar.bz2: XZ compressed data lars@abudhabi:~/temp/alsa> Please rename the tarballs if possible. Please CC me on replies. I'm not subscribed. Kind regards Lars -- Lars Wendler Gentoo package maintainer GPG: 21CC CF02 4586 0A07 ED93 9F68 498F E765 960E 9B39

3 3

[alsa-devel] [PATCH] ALSA: hda/hdmi - add a parameter to let users decide if checking the eld_valid
by Hui Wang 18 Nov '19

18 Nov '19

With the commit 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event"), the driver checks eld_valid before reporting Jack state, this fixes the 4 HDMI/DP audio devices issue. But recently some users complained that the hdmi audio on their machines couldn't work anymore with this commit. On their machines, the monitor_present is 1 while the eld_valid is 0 when plugging a monitor, and the hdmi audio could work even the eld_valid is 0. To make the hdmi audio work again on those machines, adding a module parameter, if usrs want to skip the checking eld_valid, they could set checking_eld_valid=0 when loading the module. And this parameter only applies to sense_via_verbs, for those getting eld via component, no need to apply this parameter since it is impossible that present is 1 while eld_valid is 0. BugLink: https://bugs.launchpad.net/bugs/1834771 Fixes: 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event") Cc: <stable(a)vger.kernel.org> Signed-off-by: Hui Wang <hui.wang(a)canonical.com> --- sound/pci/hda/patch_hdmi.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index be8a977fc684..d70fca4f4411 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,11 @@ static bool static_hdmi_pcm; module_param(static_hdmi_pcm, bool, 0644); MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info"); +static bool checking_eld_valid = true; +module_param(checking_eld_valid, bool, 0644); +MODULE_PARM_DESC(checking_eld_valid, "Checking eld_valid before reporting Jack " + "state (default = 1, using verbs only)"); + #define is_haswell(codec) ((codec)->core.vendor_id == 0x80862807) #define is_broadwell(codec) ((codec)->core.vendor_id == 0x80862808) #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809) @@ -1557,8 +1562,9 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, jack = snd_hda_jack_tbl_get(codec, pin_nid); if (jack) { jack->block_report = !ret; - jack->pin_sense = (eld->monitor_present && eld->eld_valid) ? - AC_PINSENSE_PRESENCE : 0; + if (checking_eld_valid) + jack->pin_sense = (eld->monitor_present && eld->eld_valid) ? + AC_PINSENSE_PRESENCE : 0; } mutex_unlock(&per_pin->lock); return ret; -- 2.17.1

2 6

[alsa-devel] [PATCH] ASoC: fsl_audmix: add missed pm_runtime_disable
by Chuhong Yuan 18 Nov '19

18 Nov '19

The driver forgets to call pm_runtime_disable in probe failure and remove. Add the missed calls to fix it. Signed-off-by: Chuhong Yuan <hslester96(a)gmail.com> --- sound/soc/fsl/fsl_audmix.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c index c7e4e9757dce..57cad4365e56 100644 --- a/sound/soc/fsl/fsl_audmix.c +++ b/sound/soc/fsl/fsl_audmix.c @@ -499,15 +499,20 @@ static int fsl_audmix_probe(struct platform_device *pdev) ARRAY_SIZE(fsl_audmix_dai)); if (ret) { dev_err(dev, "failed to register ASoC DAI\n"); - return ret; + goto err_disable_pm; } priv->pdev = platform_device_register_data(dev, mdrv, 0, NULL, 0); if (IS_ERR(priv->pdev)) { ret = PTR_ERR(priv->pdev); dev_err(dev, "failed to register platform %s: %d\n", mdrv, ret); + goto err_disable_pm; } + return 0; + +err_disable_pm: + pm_runtime_disable(dev); return ret; } @@ -515,6 +520,8 @@ static int fsl_audmix_remove(struct platform_device *pdev) { struct fsl_audmix *priv = dev_get_drvdata(&pdev->dev); + pm_runtime_disable(&pdev->dev); + if (priv->pdev) platform_device_unregister(priv->pdev); -- 2.24.0

1 0

Re: [alsa-devel] [RESEND PATCH v5 2/6] ASoC: amd: Refactoring of DAI from DMA driver (fwd)
by Julia Lawall 17 Nov '19

17 Nov '19

This code needs some work. You can't just kfree every pointer. YOu have to consider how the pointer was initialized. julia ---------- Forwarded message ---------- Date: Sun, 17 Nov 2019 12:48:14 +0800 From: kbuild test robot <lkp(a)intel.com> To: kbuild(a)lists.01.org Cc: Julia Lawall <julia.lawall(a)lip6.fr> Subject: Re: [alsa-devel] [RESEND PATCH v5 2/6] ASoC: amd: Refactoring of DAI from DMA driver CC: kbuild-all(a)lists.01.org In-Reply-To: <1573629249-13272-3-git-send-email-Vishnuvardhanrao.Ravulapati(a)amd.com> References: <1573629249-13272-3-git-send-email-Vishnuvardhanrao.Ravulapati(a)amd.com> CC: Hi Ravulapati, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on asoc/for-next] [cannot apply to v5.4-rc7 next-20191115] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Ravulapati-Vishnu-vardhan-rao/ASoC… base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next :::::: branch date: 4 days ago :::::: commit date: 4 days ago If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp(a)intel.com> Reported-by: Julia Lawall <julia.lawall(a)lip6.fr> >> sound/soc/amd/raven/acp3x-i2s.c:245:1-6: WARNING: invalid free of devm_ allocated data sound/soc/amd/raven/acp3x-i2s.c:248:1-6: WARNING: invalid free of devm_ allocated data sound/soc/amd/raven/acp3x-i2s.c:249:1-6: WARNING: invalid free of devm_ allocated data # https://github.com/0day-ci/linux/commit/74480eceed0f95f0b8d383d0882b918a335… git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 74480eceed0f95f0b8d383d0882b918a335ce0d4 vim +245 sound/soc/amd/raven/acp3x-i2s.c 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 207 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 208 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 209 static int acp3x_dai_probe(struct platform_device *pdev) 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 210 { 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 211 int status; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 212 struct resource *res; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 213 struct i2s_dev_data *adata; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 214 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 215 adata = devm_kzalloc(&pdev->dev, sizeof(struct i2s_dev_data), 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 216 GFP_KERNEL); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 217 if (!adata) 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 218 return -ENOMEM; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 219 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 220 res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 221 if (!res) { 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 222 dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n"); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 223 goto err; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 224 } 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 225 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 226 adata->acp3x_base = devm_ioremap(&pdev->dev, res->start, 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 227 resource_size(res)); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 228 if (IS_ERR(adata->acp3x_base)) 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 229 return PTR_ERR(adata->acp3x_base); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 230 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 231 adata->i2s_irq = res->start; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 232 dev_set_drvdata(&pdev->dev, adata); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 233 status = devm_snd_soc_register_component(&pdev->dev, 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 234 &acp3x_dai_component, 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 235 &acp3x_i2s_dai, 1); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 236 if (status) { 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 237 dev_err(&pdev->dev, "Fail to register acp i2s dai\n"); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 238 goto dev_err; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 239 } 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 240 pm_runtime_set_autosuspend_delay(&pdev->dev, 10000); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 241 pm_runtime_use_autosuspend(&pdev->dev); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 242 pm_runtime_enable(&pdev->dev); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 243 return 0; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 244 err: 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 @245 kfree(adata); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 246 return -ENOMEM; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 247 dev_err: 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 248 kfree(adata->acp3x_base); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 249 kfree(adata); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 250 kfree(res); 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 251 return -ENODEV; 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 252 } 74480eceed0f95 Ravulapati Vishnu vardhan rao 2019-11-13 253 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

1 0

[alsa-devel] [asoc:for-5.4 438/441] sound/soc/sof/sof-pci-dev.c:116:15: error: 'snd_soc_acpi_intel_cfl_machines' undeclared here (not in a function); did you mean 'snd_soc_acpi_intel_cnl_machines'?
by kbuild test robot 17 Nov '19

17 Nov '19

tree: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4 head: fe23be2d85b05f561431d75acddec726ea807d2a commit: 130d3e90777fe974e4b8fa100cec8faf19cac998 [438/441] ASoC: SOF: Intel: Fix CFL and CML FW nocodec binary names. config: x86_64-allyesconfig (attached as .config) compiler: gcc-7 (Debian 7.4.0-14) 7.4.0 reproduce: git checkout 130d3e90777fe974e4b8fa100cec8faf19cac998 # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp(a)intel.com> All errors (new ones prefixed by >>): >> sound/soc/sof/sof-pci-dev.c:116:15: error: 'snd_soc_acpi_intel_cfl_machines' undeclared here (not in a function); did you mean 'snd_soc_acpi_intel_cnl_machines'? .machines = snd_soc_acpi_intel_cfl_machines, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_soc_acpi_intel_cnl_machines >> sound/soc/sof/sof-pci-dev.c:136:15: error: 'snd_soc_acpi_intel_cml_machines' undeclared here (not in a function); did you mean 'snd_soc_acpi_intel_cfl_machines'? .machines = snd_soc_acpi_intel_cml_machines, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_soc_acpi_intel_cfl_machines vim +116 sound/soc/sof/sof-pci-dev.c 113 114 #if IS_ENABLED(CONFIG_SND_SOC_SOF_COFFEELAKE) 115 static const struct sof_dev_desc cfl_desc = { > 116 .machines = snd_soc_acpi_intel_cfl_machines, 117 .resindex_lpe_base = 0, 118 .resindex_pcicfg_base = -1, 119 .resindex_imr_base = -1, 120 .irqindex_host_ipc = -1, 121 .resindex_dma_base = -1, 122 .chip_info = &cnl_chip_info, 123 .default_fw_path = "intel/sof", 124 .default_tplg_path = "intel/sof-tplg", 125 .nocodec_fw_filename = "sof-cfl.ri", 126 .nocodec_tplg_filename = "sof-cnl-nocodec.tplg", 127 .ops = &sof_cnl_ops, 128 .arch_ops = &sof_xtensa_arch_ops 129 }; 130 #endif 131 132 #if IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_LP) || \ 133 IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_H) 134 135 static const struct sof_dev_desc cml_desc = { > 136 .machines = snd_soc_acpi_intel_cml_machines, 137 .resindex_lpe_base = 0, 138 .resindex_pcicfg_base = -1, 139 .resindex_imr_base = -1, 140 .irqindex_host_ipc = -1, 141 .resindex_dma_base = -1, 142 .chip_info = &cnl_chip_info, 143 .default_fw_path = "intel/sof", 144 .default_tplg_path = "intel/sof-tplg", 145 .nocodec_fw_filename = "sof-cml.ri", 146 .nocodec_tplg_filename = "sof-cnl-nocodec.tplg", 147 .ops = &sof_cnl_ops, 148 .arch_ops = &sof_xtensa_arch_ops 149 }; 150 #endif 151 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

1 0

[alsa-devel] parameter for pulse device?
by frederik＠ofb.net 16 Nov '19

16 Nov '19

Dear ALSA, In my ~/.asoundrc, I have lines like this: pcm.mic { type pulse; device "mic" } pcm.monitor { type pulse; device "monitor"; } pcm.music { type pulse; device "music"; } to make it easier for ALSA-aware programs to input and output via PulseAudio, e.g.: ecasound -i alsa,mic -o alsa,monitor -etd:... However, I would like to simplify this and not have to update ~/.asoundrc every time I create a new PulseAudio device. Since ALSA has the ability for PCMs to take a parameter, I thought this might work with the "pulse" PCM and the PulseAudio device name. But I get an "error: Invalid argument" when trying to pass the device name as an argument to the "pulse" PCM: $ ecasound -o alsa,pulse:music -i some.wav ... ALSA lib conf.c:5014:(snd_config_expand) Unknown parameters music ALSA lib pcm.c:2564:(snd_pcm_open_noupdate) Unknown PCM pulse:music ERROR: Connecting chainsetup failed: "Enabling chainsetup: AUDIOIO-ALSA: ... Unable to open ALSA-device for playback; error: Invalid argument" Is there some magic with macros that I can use to accomplish this syntax, or can we add the ability for the "pulse" PCM to take a parameter naming the device? Thanks, Frederick

4 15

Re: [alsa-devel] [PATCH 0/2] ALSA: pcm: Fix race condition in runtime access
by Chih-Yang Hsia 15 Nov '19

15 Nov '19

On Fri, Nov 15, 2019 at 11:49 PM Takashi Iwai <tiwai(a)suse.de> wrote: > > On Fri, 15 Nov 2019 16:35:19 +0100, > Chih-Yang Hsia wrote: > > > > On Fri, Nov 15, 2019 at 1:00 AM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > On Thu, 14 Nov 2019 17:37:54 +0100, > > > Chih-Yang Hsia wrote: > > > > > > > > On Thu, Nov 14, 2019 at 10:20 PM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > > > > > On Thu, 14 Nov 2019 15:16:04 +0100, > > > > > Chih-Yang Hsia wrote: > > > > > > > > > > > > On Wed, Nov 13, 2019 at 7:36 PM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > > > > > > > > > On Wed, 13 Nov 2019 10:47:51 +0100, > > > > > > > Takashi Iwai wrote: > > > > > > > > > > > > > > > > On Wed, 13 Nov 2019 08:24:41 +0100, > > > > > > > > Chih-Yang Hsia wrote: > > > > > > > > > > > > > > > > > > On Wed, Nov 13, 2019 at 2:16 AM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, 12 Nov 2019 18:17:13 +0100, > > > > > > > > > > paulhsia wrote: > > > > > > > > > > > > > > > > > > > > > > Since > > > > > > > > > > > - snd_pcm_detach_substream sets runtime to null without stream lock and > > > > > > > > > > > - snd_pcm_period_elapsed checks the nullity of the runtime outside of > > > > > > > > > > > stream lock. > > > > > > > > > > > > > > > > > > > > > > This will trigger null memory access in snd_pcm_running() call in > > > > > > > > > > > snd_pcm_period_elapsed. > > > > > > > > > > > > > > > > > > > > Well, if a stream is detached, it means that the stream must have been > > > > > > > > > > already closed; i.e. it's already a clear bug in the driver that > > > > > > > > > > snd_pcm_period_elapsed() is called against such a stream. > > > > > > > > > > > > > > > > > > > > Or am I missing other possible case? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > > > > > > > > > Takashi > > > > > > > > > > > > > > > > > > > > > > > > > > > > In multithreaded environment, it is possible to have to access both > > > > > > > > > `interrupt_handler` (from irq) and `substream close` (from > > > > > > > > > snd_pcm_release) at the same time. > > > > > > > > > Therefore, in driver implementation, if "substream close function" and > > > > > > > > > the "code section where snd_pcm_period_elapsed() in" do not hold the > > > > > > > > > same lock, then the following things can happen: > > > > > > > > > > > > > > > > > > 1. interrupt_handler -> goes into snd_pcm_period_elapsed with a valid > > > > > > > > > sustream pointer > > > > > > > > > 2. snd_pcm_release_substream: call close without blocking > > > > > > > > > 3. snd_pcm_release_substream: call snd_pcm_detache_substream and set > > > > > > > > > substream->runtime to NULL > > > > > > > > > 4. interrupt_handler -> call snd_pcm_runtime() and crash while > > > > > > > > > accessing fields in `substream->runtime` > > > > > > > > > > > > > > > > > > e.g. In intel8x0.c driver for ac97 device, > > > > > > > > > In driver intel8x0.c, `snd_pcm_period_elapsed` is called after > > > > > > > > > checking `ichdev->substream` in `snd_intel8x0_update`. > > > > > > > > > And if a `snd_pcm_release` call from alsa-lib and pass through close() > > > > > > > > > and run to snd_pcm_detach_substream() in another thread, it's possible > > > > > > > > > to trigger a crash. > > > > > > > > > I can reproduce the issue within a multithread VM easily. > > > > > > > > > > > > > > > > > > My patches are trying to provide a basic protection for this situation > > > > > > > > > (and internal pcm lock between detach and elapsed), since > > > > > > > > > - the usage of `snd_pcm_period_elapsed` does not warn callers about > > > > > > > > > the possible race if the driver does not force the order for `calling > > > > > > > > > snd_pcm_period_elapsed` and `close` by lock and > > > > > > > > > - lots of drivers already have this hidden issue and I can't fix them > > > > > > > > > one by one (You can check the "snd_pcm_period_elapsed usage" and the > > > > > > > > > "close implementation" within all the drivers). The most common > > > > > > > > > mistake is that > > > > > > > > > - Checking if the substream is null and call into snd_pcm_period_elapsed > > > > > > > > > - But `close` can happen anytime, pass without block and > > > > > > > > > snd_pcm_detach_substream will be trigger right after it > > > > > > > > > > > > > > > > Thanks, point taken. While this argument is valid and it's good to > > > > > > > > harden the PCM core side, the concurrent calls are basically a bug, > > > > > > > > and we'd need another fix in anyway. Also, the patch 2 makes little > > > > > > > > sense; there can't be multiple close calls racing with each other. So > > > > > > > > I'll go for taking your fix but only the first patch. > > > > > > > > > > > > > > > > Back to this race: the surfaced issue is, as you pointed out, the race > > > > > > > > between snd_pcm_period_elapsed() vs close call. However, the > > > > > > > > fundamental problem is the pending action after the PCM trigger-stop > > > > > > > > call. Since the PCM trigger doesn't block nor wait until the hardware > > > > > > > > actually stops the things, the driver may go to the other step even > > > > > > > > after this "supposed-to-be-stopped" point. In your case, it goes up > > > > > > > > to close, and crashes. If we had a sync-stop operation, the interrupt > > > > > > > > handler should have finished before moving to the close stage, hence > > > > > > > > such a race could be avoided. > > > > > > > > > > > > > > > > It's been a long known problem, and some drivers have the own > > > > > > > > implementation for stop-sync. I think it's time to investigate and > > > > > > > > start implementing the fundamental solution. > > > > > > > > > > > > > > BTW, what we need essentially for intel8x0 is to just call > > > > > > > synchronize_irq() before closing, at best in hw_free procedure: > > > > > > > > > > > > > > --- a/sound/pci/intel8x0.c > > > > > > > +++ b/sound/pci/intel8x0.c > > > > > > > @@ -923,8 +923,10 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream, > > > > > > > > > > > > > > static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream) > > > > > > > { > > > > > > > + struct intel8x0 *chip = snd_pcm_substream_chip(substream); > > > > > > > struct ichdev *ichdev = get_ichdev(substream); > > > > > > > > > > > > > > + synchronize_irq(chip->irq); > > > > > > > if (ichdev->pcm_open_flag) { > > > > > > > snd_ac97_pcm_close(ichdev->pcm); > > > > > > > ichdev->pcm_open_flag = 0; > > > > > > > > > > > > > > > > > > > > > The same would be needed also at the beginning of the prepare, as the > > > > > > > application may restart the stream without release. > > > > > > > > > > > > > > My idea is to add sync_stop PCM ops and call it from PCM core at > > > > > > > snd_pcm_prepare() and snd_pcm_hw_free(). > > > > > > > > > > > > > Will adding synchronize_irq() in snd_pcm_hw_free there fix the race issue? > > > > > > Is it possible to have sequence like the following steps ? > > > > > > - [Thread 1] snd_pcm_hw_free: just pass synchronize_irq() > > > > > > - [Thread 2] another interrupt come -> snd_intel8x0_update() -> goes > > > > > > into the lock region of snd_pcm_period_elapsed() and passes the > > > > > > PCM_RUNTIME_CHECK (right before snd_pcm_running()) > > > > > > > > > > This shouldn't happen because at the point snd_pcm_hw_free() the > > > > > stream has been already in the SETUP state, i.e. with trigger PCM > > > > > callback, the hardware has been programmed not to generate the PCM > > > > > stream IRQ. > > > > > > > > > Thanks for pointing that out. > > > > snd_pcm_drop() will be called right before accessing `opts->hw_free` > > > > and device dma will be stopped by SNDRV_PCM_TRIGGER_STOP. > > > > And snd_pcm_prepare() will be called when the device is not running. > > > > So synchronize_irq() should be enough for both of them. > > > > > > > > I have a patch like this now in intel8x0: > > > > > > > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c > > > > index 6ff94d8ad86e..728588937673 100644 > > > > --- a/sound/pci/intel8x0.c > > > > +++ b/sound/pci/intel8x0.c > > > > @@ -923,8 +923,10 @@ static int snd_intel8x0_hw_params(struct > > > > snd_pcm_substream *substream, > > > > > > > > static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream) > > > > { > > > > + struct intel8x0 *chip = snd_pcm_substream_chip(substream); > > > > struct ichdev *ichdev = get_ichdev(substream); > > > > > > > > + synchronize_irq(chip->irq); > > > > if (ichdev->pcm_open_flag) { > > > > snd_ac97_pcm_close(ichdev->pcm); > > > > ichdev->pcm_open_flag = 0; > > > > @@ -993,6 +995,7 @@ static int snd_intel8x0_pcm_prepare(struct > > > > snd_pcm_substream *substream) > > > > struct snd_pcm_runtime *runtime = substream->runtime; > > > > struct ichdev *ichdev = get_ichdev(substream); > > > > > > > > + synchronize_irq(chip->irq); > > > > ichdev->physbuf = runtime->dma_addr; > > > > ichdev->size = snd_pcm_lib_buffer_bytes(substream); > > > > ichdev->fragsize = snd_pcm_lib_period_bytes(substream); > > > > > > > > If that looks good to you, I can upload the patch to pw as well. > > > > Then we can upstream the intel8x0 patch and the first change I made in > > > > this series (the elapse one). > > > > Does that sound good to you? > > > > > > I already have a patch set that adds the irq-sync commonly, as this > > > problem is seen on various drivers as you already pointed. > > > > > > Below two patches add the support in PCM core side, and the rest need in > > > intel8x0.c is something like: > > > > > > --- a/sound/pci/intel8x0.c > > > +++ b/sound/pci/intel8x0.c > > > @@ -3092,6 +3092,7 @@ static int snd_intel8x0_create(struct snd_card *card, > > > return -EBUSY; > > > } > > > chip->irq = pci->irq; > > > + card->sync_irq = chip->irq; > > > > > Will this assignment or removement cause possible race if the driver > > is careless? > > Not really, it just influences on the possible synchronize_irq() call, > and the call itself can't be so racy. So it's basically safe to set > and clear at any time. Got it. I'm not that familiar with that function. > > > Maybe providing some helper functions or teaching driver writers when > > is the right time to change or remove the sync_irq will help. > > The assumption is to set this whenever an irq handler is requested or > freed. I don't mind introducing an API function > (e.g. snd_card_set_sync_irq(card, irq)), but OTOH I don't see much > benefit by that, either. This is no mandatory thing, you can > implement in the driver side in different ways, too... > Thanks for your clarification. I think both ways would be fine. Let me wait for your patches and add the fix for intel8x0 based on it later? CC me anytime when you're ready. Thanks! Paul > > thanks, > > Takashi > > > > > Best, > > Paul > > > > > if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { > > > snd_intel8x0_free(chip); > > > > > > > > > (The intel8x0 does re-acquire IRQ, so it'll need a bit more lines, but > > > you get the idea.) > > > > > > My plan is to merge the whole changes after 5.5-rc1, destined for > > > 5.6. > > > > > > > > > thanks, > > > > > > Takashi > > > > >

2 1