[PATCH 1/3] ASoC: hdac_hda: call patch_ops.free() on probe error
Add error handling for patch_ops in hdac_hda_codec_probe().
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/codecs/hdac_hda.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 473efe9ef998..72bd779bf942 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -467,7 +467,7 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component) ret = snd_hda_codec_parse_pcms(hcodec); if (ret < 0) { dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret); - goto error_regmap; + goto error_patch; }
/* HDMI controls need to be created in machine drivers */ @@ -476,7 +476,7 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component) if (ret < 0) { dev_err(&hdev->dev, "unable to create controls %d\n", ret); - goto error_regmap; + goto error_patch; } }
@@ -496,6 +496,9 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
return 0;
+error_patch: + if (hcodec->patch_ops.free) + hcodec->patch_ops.free(hcodec); error_regmap: snd_hdac_regmap_exit(hdev); error_pm:
The hdac_hda remove implementation fails to free the hda codec resources, leading to memleaks at module unload. This gap has been there from the start, commit 6bae5ea94989 ("ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers").
Instead of duplicating the cleanup logic, use the common snd_hda_codec_cleanup_for_unbind() to free the resources. Remove existing code in hdac_hda to cleanup "codec.jackpoll_work" and call to snd_hdac_regmap_exit(), as these are already done in snd_hda_codec_cleanup_for_unbind().
The cleanup is done in ASoC component remove() callback and not in the HDAC bus hdev_detach(). This is done to ensure the codec specific cleanup routines are run before the parent card is freed.
Fixes: 6bae5ea94989 ("ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers") BugLink: https://github.com/thesofproject/linux/issues/2195 Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/codecs/hdac_hda.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 72bd779bf942..74574b9180a5 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -513,6 +513,7 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component) struct hdac_hda_priv *hda_pvt = snd_soc_component_get_drvdata(component); struct hdac_device *hdev = &hda_pvt->codec.core; + struct hda_codec *codec = &hda_pvt->codec; struct hdac_ext_link *hlink = NULL;
hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev)); @@ -524,7 +525,10 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component) pm_runtime_disable(&hdev->dev); snd_hdac_ext_bus_link_put(hdev->bus, hlink);
- snd_hdac_regmap_exit(hdev); + if (codec->patch_ops.free) + codec->patch_ops.free(codec); + + snd_hda_codec_cleanup_for_unbind(codec); }
static const struct snd_soc_dapm_route hdac_hda_dapm_routes[] = { @@ -608,12 +612,10 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
static int hdac_hda_dev_remove(struct hdac_device *hdev) { - struct hdac_hda_priv *hda_pvt; - - hda_pvt = dev_get_drvdata(&hdev->dev); - if (hda_pvt && hda_pvt->codec.registered) - cancel_delayed_work_sync(&hda_pvt->codec.jackpoll_work); - + /* + * Resources are freed in hdac_hda_codec_remove(). This + * function is kept to keep hda_codec_driver_remove() happy. + */ return 0; }
Commit 5bd70440cb0a ("ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence"), introduced a slight change of semantics to DAI startup/shutdown. If startup() returns an error, shutdown() is now called for the DAI.
This causes a deadlock in hdac_hda which issues a call to snd_hda_codec_pcm_put() in case open fails. Upon error, soc_pcm_open() will call shutdown(), and pcm_put() ends up getting called twice. Result is a deadlock on pcm->open_mutex, as snd_device_free() gets called from within snd_pcm_open(). Typical task backtrace looks like this:
[ 334.244627] snd_pcm_dev_disconnect+0x49/0x340 [snd_pcm] [ 334.244634] __snd_device_disconnect.part.0+0x2c/0x50 [snd] [ 334.244640] __snd_device_free+0x7f/0xc0 [snd] [ 334.244650] snd_hda_codec_pcm_put+0x87/0x120 [snd_hda_codec] [ 334.244660] soc_pcm_open+0x6a0/0xbe0 [snd_soc_core] [ 334.244676] ? dpcm_add_paths.isra.0+0x491/0x590 [snd_soc_core] [ 334.244679] ? kfree+0x9a/0x230 [ 334.244686] dpcm_be_dai_startup+0x255/0x300 [snd_soc_core] [ 334.244695] dpcm_fe_dai_open+0x20e/0xf30 [snd_soc_core] [ 334.244701] ? snd_pcm_hw_rule_muldivk+0x110/0x110 [snd_pcm] [ 334.244709] ? dpcm_be_dai_startup+0x300/0x300 [snd_soc_core] [ 334.244714] ? snd_pcm_attach_substream+0x3c4/0x540 [snd_pcm] [ 334.244719] snd_pcm_open_substream+0x69a/0xb60 [snd_pcm] [ 334.244729] ? snd_pcm_release_substream+0x30/0x30 [snd_pcm] [ 334.244732] ? __mutex_lock_slowpath+0x10/0x10 [ 334.244736] snd_pcm_open+0x1b3/0x3c0 [snd_pcm]
Fixes: 5bd70440cb0a ("ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence") BugLink: https://github.com/thesofproject/linux/issues/2159 Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com --- sound/soc/codecs/hdac_hda.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 74574b9180a5..49e6f23fc766 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -289,7 +289,6 @@ static int hdac_hda_dai_open(struct snd_pcm_substream *substream, struct hdac_hda_priv *hda_pvt; struct hda_pcm_stream *hda_stream; struct hda_pcm *pcm; - int ret;
hda_pvt = snd_soc_component_get_drvdata(component); pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai); @@ -300,11 +299,7 @@ static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
hda_stream = &pcm->stream[substream->stream];
- ret = hda_stream->ops.open(hda_stream, &hda_pvt->codec, substream); - if (ret < 0) - snd_hda_codec_pcm_put(pcm); - - return ret; + return hda_stream->ops.open(hda_stream, &hda_pvt->codec, substream); }
static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
On Fri, 17 Jul 2020 13:19:48 +0300, Kai Vehmanen wrote:
Add error handling for patch_ops in hdac_hda_codec_probe().
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: hdac_hda: call patch_ops.free() on probe error commit: 640f835cd052bba403f955db15130ff813be78d2 [2/3] ASoC: hdac_hda: fix memleak on module unload commit: c3ec8ac82105e9589dcd72636b6fd114db690d55 [3/3] ASoC: hdac_hda: fix deadlock after PCM open error commit: 06f07e2365378d51eddd0b5bf23506e1237662b0
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Kai Vehmanen -
Mark Brown