[v3,02/19] ASoC: soc-core: tidyup soc_init_dai_link()
diff mbox series

Message ID 87d0e63joh.wl-kuninori.morimoto.gx@renesas.com
State Accepted
Commit bfce78a559655c5c4512a898a7e5d3a796fbb473
Headers show
Series
  • ASoC: soc-core cleanup - step 4
Related show

Commit Message

Kuninori Morimoto Nov. 5, 2019, 6:45 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc_init_dai_link() is needed to be called before soc_bind_dai_link().

	int snd_soc_instantiate_card()
	{
		for_each_card_prelinks(...) {
(1)			ret = soc_init_dai_link(...);
			...
		}
		...
		for_each_card_prelinks(...) {
(2)			ret = soc_bind_dai_link(...);
			...
		}
		...
		for_each_card_links(...) {
			...
(A)			ret = soc_init_dai_link(...);
			...
(B)			ret = soc_bind_dai_link(...);
		}
		...

(1) is for (2), and (A) is for (B)
(1) and (2) are for card prelink   dai_link.
(A) and (B) are for topology added dai_link.

soc_init_dai_link() is sanity check for dai_link, not initializing today.
Therefore, it is confusable naming. We can rename it as sanity_check.

And this check is for soc_bind_dai_link().
It can be more simple code if we can call it from soc_bind_dai_link().

This patch renames it to soc_dai_link_sanity_check(), and
call it from soc_bind_dai_link().

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
v2 -> v3
	- add Reviewed-by
	- call soc_dai_link_sanity_check() after soc_is_dai_link_bound()

 sound/soc/soc-core.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

Comments

Jon Hunter Nov. 11, 2019, 2:43 p.m. UTC | #1
On 05/11/2019 06:45, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> soc_init_dai_link() is needed to be called before soc_bind_dai_link().
> 
> 	int snd_soc_instantiate_card()
> 	{
> 		for_each_card_prelinks(...) {
> (1)			ret = soc_init_dai_link(...);
> 			...
> 		}
> 		...
> 		for_each_card_prelinks(...) {
> (2)			ret = soc_bind_dai_link(...);
> 			...
> 		}
> 		...
> 		for_each_card_links(...) {
> 			...
> (A)			ret = soc_init_dai_link(...);
> 			...
> (B)			ret = soc_bind_dai_link(...);
> 		}
> 		...
> 
> (1) is for (2), and (A) is for (B)
> (1) and (2) are for card prelink   dai_link.
> (A) and (B) are for topology added dai_link.
> 
> soc_init_dai_link() is sanity check for dai_link, not initializing today.
> Therefore, it is confusable naming. We can rename it as sanity_check.
> 
> And this check is for soc_bind_dai_link().
> It can be more simple code if we can call it from soc_bind_dai_link().
> 
> This patch renames it to soc_dai_link_sanity_check(), and
> call it from soc_bind_dai_link().
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> v2 -> v3
> 	- add Reviewed-by
> 	- call soc_dai_link_sanity_check() after soc_is_dai_link_bound()
> 
>  sound/soc/soc-core.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)

I am seeing an audio regression on -next and bisect is pointing to
this commit. I am seeing the following crash on boot during probe
deferral of the soundcard ...

[   13.520888] 8<--- cut here ---
[   13.531044] Unable to handle kernel NULL pointer dereference at virtual address 00000558
[   13.546555] pgd = 5c53a857
[   13.556672] [00000558] *pgd=00000000
[   13.574504] Internal error: Oops: 5 [#1] SMP ARM
[   13.585865] Modules linked in: snd_soc_tegra_wm8903(+) snd_soc_tegra_utils snd_soc_wm8903 brcmutil snd_soc_core ac97_bus snd_pcm_dmaengine snd_pcm snd_timer snd soundcore snd_soc_tegra30_ahub tegra30_devfreq tegra_wdt
[   13.608218] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[   13.617591] CPU: 1 PID: 178 Comm: systemd-udevd Not tainted 5.4.0-rc1-00276-gbfce78a55965 #11
[   13.617598] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[   13.617638] PC is at tegra_wm8903_remove+0x10/0x30 [snd_soc_tegra_wm8903]
[   13.617661] LR is at tegra_wm8903_remove+0x10/0x30 [snd_soc_tegra_wm8903]
[   13.685077] pc : [<bf09f254>]    lr : [<bf09f254>]    psr: 60000013
[   13.697095] sp : e9231c60  ip : 00000100  fp : c1704c48
[   13.710229] r10: 00000001  r9 : bf082ca8  r8 : bf087024
[   13.722152] r7 : 00000078  r6 : 00000000  r5 : bf0a1090  r4 : fffffdfb
[   13.734811] r3 : bf0a1280  r2 : 00000000  r1 : bf0a068c  r0 : 00000000
[   13.748859] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   13.762711] Control: 10c5387d  Table: a908004a  DAC: 00000051
[   13.775069] Process systemd-udevd (pid: 178, stack limit = 0x97e0dae2)
[   13.789094] Stack: (0xe9231c60 to 0xe9232000)
[   13.802489] 1c60: fffffdfb bf06e6a0 ffffffff 21a54800 bf0a1118 c1704c48 bf0a1228 bf087000
[   13.817940] 1c80: bf082c74 bf082c94 21a54800 3342fbf9 e9268c54 bf0a1090 00000000 bf0a1000
[   13.834685] 1ca0: ea9ee010 00000000 bf0a1664 00000000 c1704c48 bf06ef68 e9268c54 eb7d7880
[   13.851011] 1cc0: bf0a1000 bf09f518 ea9ee010 00000000 bf0a130c 00000000 bf0a130c 00000018
[   13.867641] 1ce0: 00000000 c095d8dc c18e52a4 ea9ee010 c18e52a8 c095b980 ea9ee010 bf0a130c
[   13.884376] 1d00: bf0a130c c1704c48 00000000 e91a4a64 bf0a1380 c095bc10 bf0a1380 c0cdeed0
[   13.899398] 1d20: bf0a0024 ea9ee010 00000000 bf0a130c c1704c48 00000000 e91a4a64 bf0a1380
[   13.915997] 1d40: c1704c48 c095bec0 00000000 bf0a130c ea9ee010 c095bf48 00000000 bf0a130c
[   13.932785] 1d60: c095bec8 c0959cbc e91a4a64 ea891858 ea9da3b4 3342fbf9 c184b908 bf0a130c
[   13.949018] 1d80: e9268c80 c184b908 00000000 c095acb0 bf0a0668 00210d00 bf0a130c bf0a130c
[   13.964549] 1da0: c18934e0 ffffe000 bf02f000 c095c9cc c1704c48 c18934e0 ffffe000 c0302ec8
[   13.981361] 1dc0: 8040003f e9266840 c1704c48 e93dcc00 f0821fff ebcffb80 e93dcc40 ea801e40
[   13.997018] 1de0: ebcffb80 e93dcc40 8040003e c046e908 00000000 3342fbf9 00000001 ebcffb80
[   14.013462] 1e00: e93dcc00 ea801e40 bf0a1380 3342fbf9 bf0a1380 00000001 e91a4b00 00000001
[   14.030635] 1e20: 00000000 c03dbf44 e9231f38 00000001 e9231f38 00000001 e91a4a40 c03db0a0
[   14.045868] 1e40: bf0a138c 00007fff bf0a1380 c03d7e54 00000000 bf0a13c8 b6df81bc bf0a1494
[   14.061143] 1e60: c1254fb4 bf0a1578 bf030f7d c1704c48 c1254f00 c1254f0c c1254f64 c1704c48
[   14.076164] 1e80: 00000000 00000000 ffffe000 bf000000 00003020 00000000 00000000 00000000
[   14.091152] 1ea0: 00000000 00000000 00000000 00000000 5f646e73 5f636f73 65726f63 00000000
[   14.105966] 1ec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   14.121024] 1ee0: 00000000 00000000 00000000 3342fbf9 7fffffff c1704c48 00000000 00000010
[   14.136351] 1f00: b6df81bc c0301204 e9230000 0000017b 00539720 c03db76c 7fffffff 00000000
[   14.151307] 1f20: 00000003 40000028 b6ea0f42 f081d000 00003020 00000000 f081db14 f081e1c0
[   14.166606] 1f40: f081d000 00003020 f081faa8 f081f94c f081ee00 00003000 00003100 00001f14
[   14.181789] 1f60: 000031de 00000000 00000000 00000000 00001f04 00000020 00000021 00000015
[   14.197068] 1f80: 00000000 00000010 00000000 3342fbf9 b6df8a8c b6df8a8c 00000000 b5caa400
[   14.212561] 1fa0: 0000017b c03011e0 b6df8a8c 00000000 00000010 b6df81bc 00000000 b6df8cd0
[   14.227942] 1fc0: b6df8a8c 00000000 b5caa400 0000017b 005383d0 bea569ac 00000000 00539720
[   14.243189] 1fe0: bea568a8 bea56898 b6df2951 b6ea0f42 400f0030 00000010 00000000 00000000
[   14.258485] [<bf09f254>] (tegra_wm8903_remove [snd_soc_tegra_wm8903]) from [<bf06e6a0>] (snd_soc_instantiate_card+0x2a4/0xa80 [snd_soc_core])
[   14.278509] [<bf06e6a0>] (snd_soc_instantiate_card [snd_soc_core]) from [<bf06ef68>] (snd_soc_register_card+0xec/0x110 [snd_soc_core])
[   14.298197] [<bf06ef68>] (snd_soc_register_card [snd_soc_core]) from [<bf09f518>] (tegra_wm8903_driver_probe+0x2a4/0x310 [snd_soc_tegra_wm8903])
[   14.307225] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[   14.318550] [<bf09f518>] (tegra_wm8903_driver_probe [snd_soc_tegra_wm8903]) from [<c095d8dc>] (platform_drv_probe+0x48/0x98)
[   14.318582] [<c095d8dc>] (platform_drv_probe) from [<c095b980>] (really_probe+0x234/0x34c)
[   14.366860] [<c095b980>] (really_probe) from [<c095bc10>] (driver_probe_device+0x60/0x168)
[   14.384433] [<c095bc10>] (driver_probe_device) from [<c095bec0>] (device_driver_attach+0x58/0x60)
[   14.400631] [<c095bec0>] (device_driver_attach) from [<c095bf48>] (__driver_attach+0x80/0xbc)
[   14.410962] brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip BCM4329/3
[   14.416550] [<c095bf48>] (__driver_attach) from [<c0959cbc>] (bus_for_each_dev+0x74/0xb4)
[   14.447878] [<c0959cbc>] (bus_for_each_dev) from [<c095acb0>] (bus_add_driver+0x164/0x1e8)
[   14.464924] brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac4329-sdio.nvidia,cardhu-a04.txt failed with error -2
[   14.465849] [<c095acb0>] (bus_add_driver) from [<c095c9cc>] (driver_register+0x7c/0x114)
[   14.487509] brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac4329-sdio.txt failed with error -2
[   14.500541] [<c095c9cc>] (driver_register) from [<c0302ec8>] (do_one_initcall+0x54/0x22c)
[   14.500574] [<c0302ec8>] (do_one_initcall) from [<c03dbf44>] (do_init_module+0x60/0x220)
[   14.549972] [<c03dbf44>] (do_init_module) from [<c03db0a0>] (load_module+0x1ffc/0x2494)
[   14.565845] [<c03db0a0>] (load_module) from [<c03db76c>] (sys_finit_module+0xac/0xd8)
[   14.581476] [<c03db76c>] (sys_finit_module) from [<c03011e0>] (__sys_trace_return+0x0/0x20)
[   14.597680] Exception stack(0xe9231fa8 to 0xe9231ff0)
[   14.610535] 1fa0:                   b6df8a8c 00000000 00000010 b6df81bc 00000000 b6df8cd0
[   14.626607] 1fc0: b6df8a8c 00000000 b5caa400 0000017b 005383d0 bea569ac 00000000 00539720
[   14.642640] 1fe0: bea568a8 bea56898 b6df2951 b6ea0f42
[   14.655529] Code: e92d4010 e5903100 e5931000 ebff2fa9 (e5900558) 
[   14.669547] ---[ end trace 6c5c1edf43b78cdc ]---

Cheers
Jon
Kuninori Morimoto Nov. 12, 2019, 12:24 a.m. UTC | #2
Hi Jon

Thank you for reporting.

> I am seeing an audio regression on -next and bisect is pointing to
> this commit. I am seeing the following crash on boot during probe
> deferral of the soundcard ...

It seems timing bug.
I have a plan to post below patch if my current posting patch are accepted,
but it seems it is necessary immediately.
I believe your issue will be solved by this patch,
but can you please test it ?
I will formally post it with your tested-by if it was OK.

# It will be more cleanuped in the future,
# but it needs more other cleanup patches...

--------------------
Subject: [PATCH] ASoC: soc-core: care card_probed at soc_cleanup_card_resources()

soc_cleanup_card_resources() will call card->remove(), but it should be
called if card->probe() or card->late_probe() are called.
snd_soc_bind_card() might be error before calling
card->probe() / card->late_probe().
In that time, card->remove() will be called.
This patch adds card_probed parameter to judge it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-core.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 55014e7..b078227 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1980,7 +1980,8 @@ static void __soc_setup_card_name(char *name, int len,
 	}
 }
 
-static void soc_cleanup_card_resources(struct snd_soc_card *card)
+static void soc_cleanup_card_resources(struct snd_soc_card *card,
+				       int card_probed)
 {
 	struct snd_soc_dai_link *link, *_link;
 
@@ -2007,7 +2008,7 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card)
 	soc_cleanup_card_debugfs(card);
 
 	/* remove the card */
-	if (card->remove)
+	if (card_probed && card->remove)
 		card->remove(card);
 }
 
@@ -2015,7 +2016,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 {
 	struct snd_soc_pcm_runtime *rtd;
 	struct snd_soc_dai_link *dai_link;
-	int ret, i;
+	int ret, i, card_probed = 0;
 
 	mutex_lock(&client_mutex);
 	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT);
@@ -2067,6 +2068,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		ret = card->probe(card);
 		if (ret < 0)
 			goto probe_end;
+		card_probed = 1;
 	}
 
 	/* probe all components used by DAI links on this card */
@@ -2129,6 +2131,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 			goto probe_end;
 		}
 	}
+	card_probed = 1;
 
 	snd_soc_dapm_new_widgets(card);
 
@@ -2145,7 +2148,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 
 probe_end:
 	if (ret < 0)
-		soc_cleanup_card_resources(card);
+		soc_cleanup_card_resources(card, card_probed);
 
 	mutex_unlock(&card->mutex);
 	mutex_unlock(&client_mutex);
@@ -2439,11 +2442,13 @@ EXPORT_SYMBOL_GPL(snd_soc_register_card);
 static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
 {
 	if (card->instantiated) {
+		int card_probed = 1;
+
 		card->instantiated = false;
 		snd_soc_dapm_shutdown(card);
 		snd_soc_flush_all_delayed_work(card);
 
-		soc_cleanup_card_resources(card);
+		soc_cleanup_card_resources(card, card_probed);
 		if (!unregister)
 			list_add(&card->list, &unbind_card_list);
 	} else {
Jon Hunter Nov. 12, 2019, 10:51 a.m. UTC | #3
On 12/11/2019 00:24, Kuninori Morimoto wrote:
> 
> Hi Jon
> 
> Thank you for reporting.
> 
>> I am seeing an audio regression on -next and bisect is pointing to
>> this commit. I am seeing the following crash on boot during probe
>> deferral of the soundcard ...
> 
> It seems timing bug.
> I have a plan to post below patch if my current posting patch are accepted,
> but it seems it is necessary immediately.
> I believe your issue will be solved by this patch,
> but can you please test it ?
> I will formally post it with your tested-by if it was OK.
> 
> # It will be more cleanuped in the future,
> # but it needs more other cleanup patches...
> 
> --------------------
> Subject: [PATCH] ASoC: soc-core: care card_probed at soc_cleanup_card_resources()
> 
> soc_cleanup_card_resources() will call card->remove(), but it should be
> called if card->probe() or card->late_probe() are called.
> snd_soc_bind_card() might be error before calling
> card->probe() / card->late_probe().
> In that time, card->remove() will be called.
> This patch adds card_probed parameter to judge it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-core.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Thanks! I can confirm that this works, so ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon
Kuninori Morimoto Nov. 13, 2019, 12:29 a.m. UTC | #4
Hi Jon

> >> I am seeing an audio regression on -next and bisect is pointing to
> >> this commit. I am seeing the following crash on boot during probe
> >> deferral of the soundcard ...
> > 
> > It seems timing bug.
> > I have a plan to post below patch if my current posting patch are accepted,
> > but it seems it is necessary immediately.
> > I believe your issue will be solved by this patch,
> > but can you please test it ?
> > I will formally post it with your tested-by if it was OK.
> > 
> > # It will be more cleanuped in the future,
> > # but it needs more other cleanup patches...
> > 
> > --------------------
> > Subject: [PATCH] ASoC: soc-core: care card_probed at soc_cleanup_card_resources()
> > 
> > soc_cleanup_card_resources() will call card->remove(), but it should be
> > called if card->probe() or card->late_probe() are called.
> > snd_soc_bind_card() might be error before calling
> > card->probe() / card->late_probe().
> > In that time, card->remove() will be called.
> > This patch adds card_probed parameter to judge it.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> >  sound/soc/soc-core.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Thanks! I can confirm that this works, so ...
> 
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks. I will post formal patch, soon

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

Patch
diff mbox series

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a141828..827625b 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -941,8 +941,8 @@  static bool soc_is_dai_link_bound(struct snd_soc_card *card,
 	return false;
 }
 
-static int soc_init_dai_link(struct snd_soc_card *card,
-			     struct snd_soc_dai_link *link)
+static int soc_dai_link_sanity_check(struct snd_soc_card *card,
+				     struct snd_soc_dai_link *link)
 {
 	int i;
 	struct snd_soc_dai_link_component *codec, *platform;
@@ -1043,7 +1043,7 @@  static int soc_bind_dai_link(struct snd_soc_card *card,
 	struct snd_soc_pcm_runtime *rtd;
 	struct snd_soc_dai_link_component *codec, *platform;
 	struct snd_soc_component *component;
-	int i;
+	int i, ret;
 
 	if (dai_link->ignore)
 		return 0;
@@ -1056,6 +1056,10 @@  static int soc_bind_dai_link(struct snd_soc_card *card,
 		return 0;
 	}
 
+	ret = soc_dai_link_sanity_check(card, dai_link);
+	if (ret < 0)
+		return ret;
+
 	rtd = soc_new_pcm_runtime(card, dai_link);
 	if (!rtd)
 		return -ENOMEM;
@@ -1985,15 +1989,6 @@  static int snd_soc_instantiate_card(struct snd_soc_card *card)
 	int ret, i;
 
 	mutex_lock(&client_mutex);
-	for_each_card_prelinks(card, i, dai_link) {
-		ret = soc_init_dai_link(card, dai_link);
-		if (ret) {
-			dev_err(card->dev, "ASoC: failed to init link %s: %d\n",
-				dai_link->name, ret);
-			mutex_unlock(&client_mutex);
-			return ret;
-		}
-	}
 	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT);
 
 	snd_soc_dapm_init(&card->dapm, card, NULL);
@@ -2073,9 +2068,6 @@  static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		if (soc_is_dai_link_bound(card, dai_link))
 			continue;
 
-		ret = soc_init_dai_link(card, dai_link);
-		if (ret)
-			goto probe_end;
 		ret = soc_bind_dai_link(card, dai_link);
 		if (ret)
 			goto probe_end;