Re: [alsa-devel] [PATCH v3 06/19] ASoC: soc-core: add soc_unbind_dai_link()

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Linux-ALSA <alsa-devel@alsa-project.org>,
	Mark Brown <broonie@kernel.org>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH v3 06/19] ASoC: soc-core: add soc_unbind_dai_link()
Date: 13 Nov 2019 13:37:46 +0900	[thread overview]
Message-ID: <87lfsk4cit.wl-kuninori.morimoto.gx@renesas.com> (raw)
In-Reply-To: <ce011d55-b1b2-7bd9-9de3-48fb616703f0@linux.intel.com>

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]


Hi Pierre-Louis

Thank you for your report

> >> +       for_each_rtd_components(rtd, rtdcom, component) {
> >> +               pr_err("plb: %s processing component\n", __func__);
> >> +               if (!component)
> >> +                       pr_err("plb: %s component is NULL\n", __func__);
> > 
> > Could you perhaps add traces of which components are being accessed at
> > each stage?  We might want to go through and just add something like
> > that in the code anyway to help figure things out.
> 
> I tried to add more traces but couldn't triangulate on a clear issue,
> and the traces have an Heisenbug effect.
> 
> So I switched to higher-level code analysis: it turns out that
> soc_dai_link_remove() routine is called from both topology and on card
> cleanup.
> 
> The patch 06/19 in this series essentially forces the pcm_runtimes to
> be freed in both cases, so possibly twice for topology-managed
> dailinks - or using information that's been freed already.
> 
> I 'fixed' this by adding an additional parameter to avoid doing the
> pcm runtime free from the topology (as was done before), and the
> kernel oops goes away. My tests have been running for 45mn now, when
> without change I get a kernel oops in less than 10-20 cycles (but
> still more than apparently our CI tracks, something to improve).
> 
> I pushed the code on GitHub to check if there are any negative points
> reported by the Intel CI, should be complete shortly:
> https://github.com/thesofproject/linux/pull/1469
> 
> I am not sure the suggested fix is correct, I don't fully get what the
> topology and card cleanups should do and how the work is split, if at
> all.

BTW, I guess your kernel is appling this patch,
but, is it correct ?

	df95a16d2a9626dcfc3f2b3671c9b91fa076c997
	("ASoC: soc-core: fix RIP warning on card removal")

If so, I think I could understand the issue.
But, before explaining detail,
I want to confirm that my solution is correct or not first.

Can you please check attached patch ?
Then, please remove above RIP warning patch.
It is not clean patch, but OK to confirming, so far.
I think
	- no kernel Oops
	- no RIP warning

Thank you for your help !!
Best regards
---
Kuninori Morimoto

[-- Attachment #2: 0001-topology-die-fixup-patch-candidate-1.patch --]
[-- Type: text/plain, Size: 3782 bytes --]

From 0d825237eea4baad0c489e1c43a58373f41a3632 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Wed, 13 Nov 2019 11:58:18 +0900
Subject: [PATCH] topology die fixup patch candidate 1

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h |  2 +-
 sound/soc/soc-component.c     |  5 ++---
 sound/soc/soc-core.c          | 14 ++++++++------
 sound/soc/soc-pcm.c           | 10 ----------
 4 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 6aa3ecb..cf726a5 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -413,6 +413,6 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream,
 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);
+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..195979f 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -516,13 +516,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-core.c b/sound/soc/soc-core.c
index 1e8dd19..8e7ff7d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -386,6 +386,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
@@ -1965,12 +1968,6 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card)
 {
 	struct snd_soc_dai_link *link, *_link;
 
-	/* 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;
-	}
-
 	/* remove and free each DAI */
 	soc_remove_link_dais(card);
 	soc_remove_link_components(card);
@@ -1988,6 +1985,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card)
 	/* remove the card */
 	if (card->remove)
 		card->remove(card);
+
+	if (card->snd_card) {
+		snd_card_free(card->snd_card);
+		card->snd_card = NULL;
+	}
 }
 
 static int snd_soc_instantiate_card(struct snd_soc_card *card)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 1c00119..a60e381 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2883,15 +2883,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(pcm);
-}
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
@@ -3033,7 +3024,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


[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
