[alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue
Hi Mark Cc Pierre-Louis, Takashi-san
Currently, I'm focusing to ASoC cleanup / balance-up. But, it found more unbalance issue, and Intel noticed about it. These patches fix dai_link remove issue on topology.
I want to get Acked-by or Reviewed-by from Takashi-san for 2) patch if possible.
These are already tested by Intel CI, and all issues were solved. (https://github.com/thesofproject/linux/pull/1504) Extra Tested-by / Reviewed-by are very welcome from Intel
Kuninori Morimoto (2): 1) ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter 2) ASoC: soc-pcm: remove soc_pcm_private_free()
include/sound/soc-component.h | 4 ++-- sound/soc/soc-component.c | 8 +++----- sound/soc/soc-core.c | 19 +++++++++++-------- sound/soc/soc-pcm.c | 12 +----------- 4 files changed, 17 insertions(+), 26 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch uses rtd instead of pcm at snd_soc_pcm_component_new/free() parameter. This is prepare for dai_link remove bug fix on topology.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc-component.h | 4 ++-- sound/soc/soc-component.c | 8 +++----- sound/soc/soc-pcm.c | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 6aa3ecb..f8fadf0 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -412,7 +412,7 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream, unsigned long offset); int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream, struct vm_area_struct *vma); -int snd_soc_pcm_component_new(struct snd_pcm *pcm); -void snd_soc_pcm_component_free(struct snd_pcm *pcm); +int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd); +void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd);
#endif /* __SOC_COMPONENT_H */ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 98ef066..1590e80 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -498,9 +498,8 @@ int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream, return -EINVAL; }
-int snd_soc_pcm_component_new(struct snd_pcm *pcm) +int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd) { - struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component; int ret; @@ -516,13 +515,12 @@ int snd_soc_pcm_component_new(struct snd_pcm *pcm) return 0; }
-void snd_soc_pcm_component_free(struct snd_pcm *pcm) +void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd) { - struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component;
for_each_rtd_components(rtd, rtdcom, component) if (component->driver->pcm_destruct) - component->driver->pcm_destruct(component, pcm); + component->driver->pcm_destruct(component, rtd->pcm); } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 4bf71e3..c624d30 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2898,7 +2898,7 @@ static void soc_pcm_private_free(struct snd_pcm *pcm)
/* need to sync the delayed work before releasing resources */ flush_delayed_work(&rtd->delayed_work); - snd_soc_pcm_component_free(pcm); + snd_soc_pcm_component_free(rtd); }
/* create a new pcm */ @@ -3036,7 +3036,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);
- ret = snd_soc_pcm_component_new(pcm); + ret = snd_soc_pcm_component_new(rtd); if (ret < 0) { dev_err(rtd->dev, "ASoC: pcm constructor failed: %d\n", ret); return ret;
The patch
ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5
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
From b2b2afbb48eac7215f951a8a462aa6837e0d495f Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 18 Nov 2019 10:50:32 +0900 Subject: [PATCH] ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter
This patch uses rtd instead of pcm at snd_soc_pcm_component_new/free() parameter. This is prepare for dai_link remove bug fix on topology.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87pnhqx89j.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- include/sound/soc-component.h | 4 ++-- sound/soc/soc-component.c | 8 +++----- sound/soc/soc-pcm.c | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 6aa3ecb7b6d3..f8fadf0e696a 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -412,7 +412,7 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream, unsigned long offset); int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream, struct vm_area_struct *vma); -int snd_soc_pcm_component_new(struct snd_pcm *pcm); -void snd_soc_pcm_component_free(struct snd_pcm *pcm); +int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd); +void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd);
#endif /* __SOC_COMPONENT_H */ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 98ef0666add2..1590e805d016 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -498,9 +498,8 @@ int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream, return -EINVAL; }
-int snd_soc_pcm_component_new(struct snd_pcm *pcm) +int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd) { - struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component; int ret; @@ -516,13 +515,12 @@ int snd_soc_pcm_component_new(struct snd_pcm *pcm) return 0; }
-void snd_soc_pcm_component_free(struct snd_pcm *pcm) +void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd) { - struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component;
for_each_rtd_components(rtd, rtdcom, component) if (component->driver->pcm_destruct) - component->driver->pcm_destruct(component, pcm); + component->driver->pcm_destruct(component, rtd->pcm); } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 4bf71e3211d8..c624d30bfa3c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2898,7 +2898,7 @@ static void soc_pcm_private_free(struct snd_pcm *pcm)
/* need to sync the delayed work before releasing resources */ flush_delayed_work(&rtd->delayed_work); - snd_soc_pcm_component_free(pcm); + snd_soc_pcm_component_free(rtd); }
/* create a new pcm */ @@ -3036,7 +3036,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);
- ret = snd_soc_pcm_component_new(pcm); + ret = snd_soc_pcm_component_new(rtd); if (ret < 0) { dev_err(rtd->dev, "ASoC: pcm constructor failed: %d\n", ret); return ret;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc-topology adds extra dai_link by using snd_soc_add_dai_link(), and removes it by snd_soc_romove_dai_link().
This snd_soc_add/remove_dai_link() and/or its related functions are unbalanced before, and now, these are balance-uped. But, it finds the random operation issue, and it is reported by Pierre-Louis.
When card was released, topology will call snd_soc_remove_dai_link() via (A).
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link;
/* This should be called before snd_card_free() */ (A) soc_remove_link_components(card);
/* free the ALSA card at first; this syncs with pending operations */ if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; }
/* remove and free each DAI */ (X) soc_remove_link_dais(card);
for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link);
... }
At (A), topology calls snd_soc_remove_dai_link(). Then topology rtd, and its related all data are freed.
Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free() is called.
static void soc_pcm_private_free(struct snd_pcm *pcm) { struct snd_soc_pcm_runtime *rtd = pcm->private_data;
/* need to sync the delayed work before releasing resources */ flush_delayed_work(&rtd->delayed_work); snd_soc_pcm_component_free(rtd); }
Here, it gets rtd via pcm->private_data. But, topology related rtd are already freed at (A). Normal sound card has no damage, becase it frees rtd at (C).
These are finalizing rtd related data. Thus, these should be called when rtd was freed, not sound card was freed. It is very natural and understandable.
In other words, pcm->private_free = soc_pcm_private_free() is no longer needed.
Extra issue is that there is zero chance to call soc_remove_dai() for topology related dai at (X). Because (A) removes rtd connection from card too, and, (X) is based on card connected rtd.
This means, (X) need to be called before (C) (= for normal sound) and (A) (= for topology).
Now, I want to focus this patch which is the reason why snd_card_free() = (B) is located there.
commit 4efda5f2130da033aeedc5b3205569893b910de2 ("ASoC: Fix use-after-free at card unregistration")
Original snd_card_free() was called last of this function. But moved to top to avoid use-after-free issue. The issue was happen at soc_pcm_free() which was pcm->private_free, today it is updated/renamed to soc_pcm_private_free().
In other words, (B) need to be called before (C) (= for normal sound) and (A) (= for topology), because it needs (not yet freed) rtd. But, (A) need to be called before (B), because it needs card->snd_card pointer.
If we call flush_delayed_work() and snd_soc_pcm_component_free() (= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)), there is no reason to call snd_card_free() at top of this function. It can be called end of this function, again.
But, in such case, it will likely break unbind again, as Takashi-san reported. When unbind is performed in a busy state, the code may release still-in-use resources. At least we need to call snd_card_disconnect_sync() at the first place.
The final code will be...
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link;
if (card->snd_card) (Z) snd_card_disconnect_sync(card->snd_card);
(X) soc_remove_link_dais(card); (A) soc_remove_link_components(card);
for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link);
... if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; } }
To avoid release still-in-use resources, call snd_card_disconnect_sync() at (Z).
(X) is needed for both non-topology and topology.
topology removes rtd via (A), and non topology removes rtd via (C).
snd_card_free() is no longer related to use-after-free issue. Thus, locating (B) is no problem.
Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal") Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()") Reported-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Tested-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-core.c | 19 +++++++++++-------- sound/soc/soc-pcm.c | 10 ---------- 2 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 977a7bf..e3a53ef 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
list_del(&rtd->list);
+ flush_delayed_work(&rtd->delayed_work); + snd_soc_pcm_component_free(rtd); + /* * we don't need to call kfree() for rtd->dev * see @@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, { struct snd_soc_dai_link *link, *_link;
- /* This should be called before snd_card_free() */ - soc_remove_link_components(card); - - /* free the ALSA card at first; this syncs with pending operations */ - if (card->snd_card) { - snd_card_free(card->snd_card); - card->snd_card = NULL; - } + if (card->snd_card) + snd_card_disconnect_sync(card->snd_card);
snd_soc_dapm_shutdown(card);
/* remove and free each DAI */ soc_remove_link_dais(card); + soc_remove_link_components(card);
for_each_card_links_safe(card, link, _link) snd_soc_remove_dai_link(card, link); @@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, /* remove the card */ if (card_probed && card->remove) card->remove(card); + + if (card->snd_card) { + snd_card_free(card->snd_card); + card->snd_card = NULL; + } }
static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c624d30..2c4f50c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) return ret; }
-static void soc_pcm_private_free(struct snd_pcm *pcm) -{ - struct snd_soc_pcm_runtime *rtd = pcm->private_data; - - /* need to sync the delayed work before releasing resources */ - flush_delayed_work(&rtd->delayed_work); - snd_soc_pcm_component_free(rtd); -} - /* create a new pcm */ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) { @@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) return ret; }
- pcm->private_free = soc_pcm_private_free; pcm->no_device_suspend = true; out: dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
The patch
ASoC: soc-pcm: remove soc_pcm_private_free()
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5
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
From 0ced7b050224b18ca73e38e7068f36be8e708c06 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 18 Nov 2019 10:51:11 +0900 Subject: [PATCH] ASoC: soc-pcm: remove soc_pcm_private_free()
soc-topology adds extra dai_link by using snd_soc_add_dai_link(), and removes it by snd_soc_romove_dai_link().
This snd_soc_add/remove_dai_link() and/or its related functions are unbalanced before, and now, these are balance-uped. But, it finds the random operation issue, and it is reported by Pierre-Louis.
When card was released, topology will call snd_soc_remove_dai_link() via (A).
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link;
/* This should be called before snd_card_free() */ (A) soc_remove_link_components(card);
/* free the ALSA card at first; this syncs with pending operations */ if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; }
/* remove and free each DAI */ (X) soc_remove_link_dais(card);
for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link);
... }
At (A), topology calls snd_soc_remove_dai_link(). Then topology rtd, and its related all data are freed.
Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free() is called.
static void soc_pcm_private_free(struct snd_pcm *pcm) { struct snd_soc_pcm_runtime *rtd = pcm->private_data;
/* need to sync the delayed work before releasing resources */ flush_delayed_work(&rtd->delayed_work); snd_soc_pcm_component_free(rtd); }
Here, it gets rtd via pcm->private_data. But, topology related rtd are already freed at (A). Normal sound card has no damage, becase it frees rtd at (C).
These are finalizing rtd related data. Thus, these should be called when rtd was freed, not sound card was freed. It is very natural and understandable.
In other words, pcm->private_free = soc_pcm_private_free() is no longer needed.
Extra issue is that there is zero chance to call soc_remove_dai() for topology related dai at (X). Because (A) removes rtd connection from card too, and, (X) is based on card connected rtd.
This means, (X) need to be called before (C) (= for normal sound) and (A) (= for topology).
Now, I want to focus this patch which is the reason why snd_card_free() = (B) is located there.
commit 4efda5f2130da033aeedc5b3205569893b910de2 ("ASoC: Fix use-after-free at card unregistration")
Original snd_card_free() was called last of this function. But moved to top to avoid use-after-free issue. The issue was happen at soc_pcm_free() which was pcm->private_free, today it is updated/renamed to soc_pcm_private_free().
In other words, (B) need to be called before (C) (= for normal sound) and (A) (= for topology), because it needs (not yet freed) rtd. But, (A) need to be called before (B), because it needs card->snd_card pointer.
If we call flush_delayed_work() and snd_soc_pcm_component_free() (= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)), there is no reason to call snd_card_free() at top of this function. It can be called end of this function, again.
But, in such case, it will likely break unbind again, as Takashi-san reported. When unbind is performed in a busy state, the code may release still-in-use resources. At least we need to call snd_card_disconnect_sync() at the first place.
The final code will be...
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link;
if (card->snd_card) (Z) snd_card_disconnect_sync(card->snd_card);
(X) soc_remove_link_dais(card); (A) soc_remove_link_components(card);
for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link);
... if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; } }
To avoid release still-in-use resources, call snd_card_disconnect_sync() at (Z).
(X) is needed for both non-topology and topology.
topology removes rtd via (A), and non topology removes rtd via (C).
snd_card_free() is no longer related to use-after-free issue. Thus, locating (B) is no problem.
Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal") Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()") Reported-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Tested-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/87o8xax88g.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-core.c | 19 +++++++++++-------- sound/soc/soc-pcm.c | 10 ---------- 2 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 977a7bfad519..e3a53ef1db04 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
list_del(&rtd->list);
+ flush_delayed_work(&rtd->delayed_work); + snd_soc_pcm_component_free(rtd); + /* * we don't need to call kfree() for rtd->dev * see @@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, { struct snd_soc_dai_link *link, *_link;
- /* This should be called before snd_card_free() */ - soc_remove_link_components(card); - - /* free the ALSA card at first; this syncs with pending operations */ - if (card->snd_card) { - snd_card_free(card->snd_card); - card->snd_card = NULL; - } + if (card->snd_card) + snd_card_disconnect_sync(card->snd_card);
snd_soc_dapm_shutdown(card);
/* remove and free each DAI */ soc_remove_link_dais(card); + soc_remove_link_components(card);
for_each_card_links_safe(card, link, _link) snd_soc_remove_dai_link(card, link); @@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, /* remove the card */ if (card_probed && card->remove) card->remove(card); + + if (card->snd_card) { + snd_card_free(card->snd_card); + card->snd_card = NULL; + } }
static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c624d30bfa3c..2c4f50c44591 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) return ret; }
-static void soc_pcm_private_free(struct snd_pcm *pcm) -{ - struct snd_soc_pcm_runtime *rtd = pcm->private_data; - - /* need to sync the delayed work before releasing resources */ - flush_delayed_work(&rtd->delayed_work); - snd_soc_pcm_component_free(rtd); -} - /* create a new pcm */ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) { @@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) return ret; }
- pcm->private_free = soc_pcm_private_free; pcm->no_device_suspend = true; out: dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
Dear all,
Missatge de Kuninori Morimoto kuninori.morimoto.gx@renesas.com del dia dl., 18 de nov. 2019 a les 2:52:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc-topology adds extra dai_link by using snd_soc_add_dai_link(), and removes it by snd_soc_romove_dai_link().
This snd_soc_add/remove_dai_link() and/or its related functions are unbalanced before, and now, these are balance-uped. But, it finds the random operation issue, and it is reported by Pierre-Louis.
When card was released, topology will call snd_soc_remove_dai_link() via (A).
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link; /* This should be called before snd_card_free() */ (A) soc_remove_link_components(card); /* free the ALSA card at first; this syncs with pending operations */ if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; } /* remove and free each DAI */ (X) soc_remove_link_dais(card); for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link); ... }At (A), topology calls snd_soc_remove_dai_link(). Then topology rtd, and its related all data are freed.
Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free() is called.
static void soc_pcm_private_free(struct snd_pcm *pcm) { struct snd_soc_pcm_runtime *rtd = pcm->private_data; /* need to sync the delayed work before releasing resources */ flush_delayed_work(&rtd->delayed_work); snd_soc_pcm_component_free(rtd); }Here, it gets rtd via pcm->private_data. But, topology related rtd are already freed at (A). Normal sound card has no damage, becase it frees rtd at (C).
These are finalizing rtd related data. Thus, these should be called when rtd was freed, not sound card was freed. It is very natural and understandable.
In other words, pcm->private_free = soc_pcm_private_free() is no longer needed.
Extra issue is that there is zero chance to call soc_remove_dai() for topology related dai at (X). Because (A) removes rtd connection from card too, and, (X) is based on card connected rtd.
This means, (X) need to be called before (C) (= for normal sound) and (A) (= for topology).
Now, I want to focus this patch which is the reason why snd_card_free() = (B) is located there.
commit 4efda5f2130da033aeedc5b3205569893b910de2 ("ASoC: Fix use-after-free at card unregistration")Original snd_card_free() was called last of this function. But moved to top to avoid use-after-free issue. The issue was happen at soc_pcm_free() which was pcm->private_free, today it is updated/renamed to soc_pcm_private_free().
In other words, (B) need to be called before (C) (= for normal sound) and (A) (= for topology), because it needs (not yet freed) rtd. But, (A) need to be called before (B), because it needs card->snd_card pointer.
If we call flush_delayed_work() and snd_soc_pcm_component_free() (= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)), there is no reason to call snd_card_free() at top of this function. It can be called end of this function, again.
But, in such case, it will likely break unbind again, as Takashi-san reported. When unbind is performed in a busy state, the code may release still-in-use resources. At least we need to call snd_card_disconnect_sync() at the first place.
The final code will be...
static void soc_cleanup_card_resources(struct snd_soc_card *card) { struct snd_soc_dai_link *link, *_link; if (card->snd_card) (Z) snd_card_disconnect_sync(card->snd_card); (X) soc_remove_link_dais(card); (A) soc_remove_link_components(card); for_each_card_links_safe(card, link, _link) (C) snd_soc_remove_dai_link(card, link); ... if (card->snd_card) { (B) snd_card_free(card->snd_card); card->snd_card = NULL; } }To avoid release still-in-use resources, call snd_card_disconnect_sync() at (Z).
(X) is needed for both non-topology and topology.
topology removes rtd via (A), andnon topology removes rtd via (C).
snd_card_free() is no longer related to use-after-free issue. Thus, locating (B) is no problem.
Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal") Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()") Reported-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Tested-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
I didn't look into detail yet, but after applying this patch my Samsung Chromebook Plus started to show different warnings like this, probably caused because in my case the driver is deferring?
I'll try to take a look, but if anyone already knows the cause, please let me know.
Thanks, Enric
[ 10.873336] ------------[ cut here ]------------ [ 10.878507] WARNING: CPU: 4 PID: 45 at kernel/workqueue.c:3032 __flush_work.isra.45+0x210/0x230 [ 10.888226] Modules linked in: snd_soc_rockchip_i2s(+) vctrl_regulator snd_soc_rk3399_gru_sound snd_soc_rt5514 cec gpu_sched snd_soc_max98357a kfifo_buf i2c_hid cros_e c_sensors_core snd_soc_da7219 snd_soc_rockchip_pcm phy_rockchip_pcie snd_soc_rt5514_spi rockchip_saradc sbs_battery pwm_bl cros_usbpd_charger cros_ec_chardev cros_usbpd_l ogger pcie_rockchip_host pwm_cros_ec rockchip_thermal snd_soc_rl6231 ip_tables x_tables ipv6 nf_defrag_ipv6 [ 10.931870] CPU: 4 PID: 45 Comm: kworker/4:1 Not tainted 5.4.0+ #322 [ 10.938967] Hardware name: Google Kevin (DT) [ 10.943738] Workqueue: events deferred_probe_work_func [ 10.949476] pstate: 00000005 (nzcv daif -PAN -UAO) [ 10.954825] pc : __flush_work.isra.45+0x210/0x230 [ 10.960068] lr : flush_delayed_work+0x34/0x58 [ 10.964929] sp : ffff800011d4b930 [ 10.968625] x29: ffff800011d4b930 x28: 00000000fffffdfb [ 10.974547] x27: ffff80001129b530 x26: ffff8000111a6a08 [ 10.980469] x25: 0000000000000003 x24: 0000000000000001 [ 10.986399] x23: ffff800011a7a980 x22: ffff8000118998c8 [ 10.992329] x21: 0000000000000000 x20: ffff800008df1000 [ 10.998260] x19: ffff0000ed60f698 x18: 0000000000000001 [ 11.004190] x17: 0000000000000001 x16: ffff800011e7f000 [ 11.010120] x15: ffffffffffffffff x14: 0000000000000000 [ 11.016051] x13: 0000000000000000 x12: 0000000000000020 [ 11.021981] x11: 0000000000000008 x10: 0101010101010101 [ 11.027912] x9 : 0000000000000000 x8 : 7f7f7f7f7f7f7f7f [ 11.033842] x7 : ffff0000f5557340 x6 : 0080808080808080 [ 11.039778] x5 : dead000000000100 x4 : 0000000000000000 [ 11.045711] x3 : 0000000000000000 x2 : 9def22a3228fd300 [ 11.051642] x1 : 0000000000000000 x0 : 0000000000000000 [ 11.057565] Call trace: [ 11.060294] __flush_work.isra.45+0x210/0x230 [ 11.065157] flush_delayed_work+0x34/0x58 [ 11.069623] soc_free_pcm_runtime.part.13+0x40/0x60 [ 11.075074] snd_soc_remove_dai_link+0x54/0x60 [ 11.075076] soc_cleanup_card_resources+0x160/0x2a8 [ 11.075078] snd_soc_bind_card+0x264/0xa10 [ 11.075080] snd_soc_register_card+0xf4/0x108 [ 11.075088] devm_snd_soc_register_card+0x40/0x90 [ 11.100189] rockchip_sound_probe+0x204/0x2e4 [snd_soc_rk3399_gru_sound] [ 11.107770] platform_drv_probe+0x50/0xa0 [ 11.112244] really_probe+0xd4/0x308 [ 11.116234] driver_probe_device+0x54/0xe8 [ 11.122457] __device_attach_driver+0x80/0xb8 [ 11.127312] bus_for_each_drv+0x78/0xc8 [ 11.131788] __device_attach+0xd4/0x130 [ 11.136070] device_initial_probe+0x10/0x18 [ 11.140738] bus_probe_device+0x90/0x98 [ 11.145020] deferred_probe_work_func+0x6c/0xa0 [ 11.155998] process_one_work+0x1e0/0x358 [ 11.160474] worker_thread+0x40/0x488 [ 11.164562] kthread+0x118/0x120 [ 11.168163] ret_from_fork+0x10/0x18 [ 11.172347] ---[ end trace dc22ca199fcfcf7c ]---
sound/soc/soc-core.c | 19 +++++++++++-------- sound/soc/soc-pcm.c | 10 ---------- 2 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 977a7bf..e3a53ef 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
list_del(&rtd->list);
flush_delayed_work(&rtd->delayed_work);snd_soc_pcm_component_free(rtd);/* * we don't need to call kfree() for rtd->dev * see@@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, { struct snd_soc_dai_link *link, *_link;
/* This should be called before snd_card_free() */soc_remove_link_components(card);/* free the ALSA card at first; this syncs with pending operations */if (card->snd_card) {snd_card_free(card->snd_card);card->snd_card = NULL;}
if (card->snd_card)snd_card_disconnect_sync(card->snd_card); snd_soc_dapm_shutdown(card); /* remove and free each DAI */ soc_remove_link_dais(card);soc_remove_link_components(card); for_each_card_links_safe(card, link, _link) snd_soc_remove_dai_link(card, link);@@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card, /* remove the card */ if (card_probed && card->remove) card->remove(card);
if (card->snd_card) {snd_card_free(card->snd_card);card->snd_card = NULL;}}
static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c624d30..2c4f50c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) return ret; }
-static void soc_pcm_private_free(struct snd_pcm *pcm) -{
struct snd_soc_pcm_runtime *rtd = pcm->private_data;/* need to sync the delayed work before releasing resources */flush_delayed_work(&rtd->delayed_work);snd_soc_pcm_component_free(rtd);-}
/* create a new pcm */ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) { @@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) return ret; }
pcm->private_free = soc_pcm_private_free; pcm->no_device_suspend = true;out: dev_info(rtd->card->dev, "%s <-> %s mapping ok\n", -- 2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
I didn't look into detail yet, but after applying this patch my Samsung Chromebook Plus started to show different warnings like this, probably caused because in my case the driver is deferring?
I'll try to take a look, but if anyone already knows the cause, please let me know.
Hi Enric,
Can you try:
https://patchwork.kernel.org/patch/11265061/
It should be already in Mark's tree.
Hi Daniel,
Missatge de Daniel Baluta daniel.baluta@nxp.com del dia dj., 5 de des. 2019 a les 13:54:
I didn't look into detail yet, but after applying this patch my Samsung Chromebook Plus started to show different warnings like this, probably caused because in my case the driver is deferring?
I'll try to take a look, but if anyone already knows the cause, please let me know.
Hi Enric,
Can you try:
Actually I picked
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=...
which is supposed to land in this release cycle, so all fine.
Thanks, Enric
It should be already in Mark's tree.
On 11/17/19 7:49 PM, Kuninori Morimoto wrote:
Hi Mark Cc Pierre-Louis, Takashi-san
Currently, I'm focusing to ASoC cleanup / balance-up. But, it found more unbalance issue, and Intel noticed about it. These patches fix dai_link remove issue on topology.
I want to get Acked-by or Reviewed-by from Takashi-san for 2) patch if possible.
These are already tested by Intel CI, and all issues were solved. (https://github.com/thesofproject/linux/pull/1504) Extra Tested-by / Reviewed-by are very welcome from Intel
if you don't mind I'd like to retest this new series, it's based on a different tip and is not exactly the same as before.
e.g. we tested this:
- /* free the ALSA card at first; this syncs with pending operations */ - if (card->snd_card) { - snd_card_free(card->snd_card); - card->snd_card = NULL; - } + if (card->snd_card) + snd_card_disconnect_sync(card->snd_card);
/* remove and free each DAI */ soc_remove_link_dais(card); + soc_remove_link_components(card);
and the new code shows this
- /* free the ALSA card at first; this syncs with pending operations */ - if (card->snd_card) { - snd_card_free(card->snd_card); - card->snd_card = NULL; - } + if (card->snd_card) + snd_card_disconnect_sync(card->snd_card);
snd_soc_dapm_shutdown(card); <<< not tested yet.
/* remove and free each DAI */ soc_remove_link_dais(card); + soc_remove_link_components(card);
Kuninori Morimoto (2):
- ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter
- ASoC: soc-pcm: remove soc_pcm_private_free()
include/sound/soc-component.h | 4 ++-- sound/soc/soc-component.c | 8 +++----- sound/soc/soc-core.c | 19 +++++++++++-------- sound/soc/soc-pcm.c | 12 +----------- 4 files changed, 17 insertions(+), 26 deletions(-)
Hi Pierre-Louis
These are already tested by Intel CI, and all issues were solved. (https://github.com/thesofproject/linux/pull/1504) Extra Tested-by / Reviewed-by are very welcome from Intel
if you don't mind I'd like to retest this new series, it's based on a different tip and is not exactly the same as before.
Yes, of course. Sorry I didn't mention about it.
- /* free the ALSA card at first; this syncs with pending operations */
- if (card->snd_card) {
snd_card_free(card->snd_card);card->snd_card = NULL;- }
if (card->snd_card)
snd_card_disconnect_sync(card->snd_card);snd_soc_dapm_shutdown(card); <<< not tested yet.
/* remove and free each DAI */ soc_remove_link_dais(card);
soc_remove_link_components(card);
Yes. It is from
2a6f0892bda954dc2688b002060093ee0fe38528 ("ASoC: soc-core: call snd_soc_dapm_shutdown() at soc_cleanup_card_resources()")
Thank you for your help !! Best regards --- Kuninori Morimoto
On 11/18/19 7:10 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis
These are already tested by Intel CI, and all issues were solved. (https://github.com/thesofproject/linux/pull/1504) Extra Tested-by / Reviewed-by are very welcome from Intel
if you don't mind I'd like to retest this new series, it's based on a different tip and is not exactly the same as before.
Yes, of course. Sorry I didn't mention about it.
- /* free the ALSA card at first; this syncs with pending operations */
- if (card->snd_card) {
snd_card_free(card->snd_card);card->snd_card = NULL;- }
if (card->snd_card)
snd_card_disconnect_sync(card->snd_card);snd_soc_dapm_shutdown(card); <<< not tested yet.
/* remove and free each DAI */ soc_remove_link_dais(card);
soc_remove_link_components(card);
Yes. It is from
2a6f0892bda954dc2688b002060093ee0fe38528 ("ASoC: soc-core: call snd_soc_dapm_shutdown() at soc_cleanup_card_resources()")
No regression detected so from the Intel side we're good with this patchset. Thanks Morimoto-san for this comprehensive analysis, really nice work!
participants (5)
-
Daniel Baluta -
Enric Balletbo Serra -
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart