On 08/01/2019 12:03, Jon Hunter wrote:
On 08/01/2019 10:50, Jon Hunter wrote:
Hi Kuninori,
On 08/01/2019 02:25, Kuninori Morimoto wrote:
Hi Jon
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Thank you for pointing it. Indeed it is mess. I think coping info is nice idea, but it is not easy so far, and it uses much memory...
I didn't test this, but can below patch solve your issue ?
I will give it a try and let you know.
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
To be specific, the following also works ...
--- include/sound/simple_card_utils.h | 2 +- include/sound/soc.h | 2 +- sound/soc/generic/audio-graph-card.c | 4 +++- sound/soc/generic/simple-card-utils.c | 4 ++-- sound/soc/generic/simple-card.c | 6 ++++-- sound/soc/soc-core.c | 18 +++++++----------- 6 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 6d69ed2bd7b1..78273b81ef82 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -75,7 +75,7 @@ void asoc_simple_card_clk_disable(struct asoc_simple_dai *dai); &dai_link->codec_dai_name, \ list_name, cells_name, NULL) #define asoc_simple_card_parse_platform(node, dai_link, list_name, cells_name) \ - asoc_simple_card_parse_dai(node, dai_link->platform, \ + asoc_simple_card_parse_dai(node, &dai_link->platform, \ &dai_link->platform_of_node, \ NULL, list_name, cells_name, NULL) int asoc_simple_card_parse_dai(struct device_node *node, diff --git a/include/sound/soc.h b/include/sound/soc.h index 8ec1de856ee7..8b7ffc60006a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -925,7 +925,7 @@ struct snd_soc_dai_link { */ const char *platform_name; struct device_node *platform_of_node; - struct snd_soc_dai_link_component *platform; + struct snd_soc_dai_link_component platform;
int id; /* optional ID for machine driver link identification */
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 3ec96cdc683b..e961d45ce141 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -687,7 +687,9 @@ static int graph_probe(struct platform_device *pdev) for (i = 0; i < li.link; i++) { dai_link[i].codecs = &dai_props[i].codecs; dai_link[i].num_codecs = 1; - dai_link[i].platform = &dai_props[i].platform; + dai_link[i].platform.name = dai_props[i].platform.name; + dai_link[i].platform.of_node = dai_props[i].platform.of_node; + dai_link[i].platform.dai_name = dai_props[i].platform.dai_name; }
priv->pa_gpio = devm_gpiod_get_optional(dev, "pa", GPIOD_OUT_LOW); diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 336895f7fd1e..74910c7841ec 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -397,8 +397,8 @@ EXPORT_SYMBOL_GPL(asoc_simple_card_init_dai); int asoc_simple_card_canonicalize_dailink(struct snd_soc_dai_link *dai_link) { /* Assumes platform == cpu */ - if (!dai_link->platform->of_node) - dai_link->platform->of_node = dai_link->cpu_of_node; + if (!dai_link->platform.of_node) + dai_link->platform.of_node = dai_link->cpu_of_node;
return 0;
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 479de236e694..b6402e09bba2 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -732,7 +732,9 @@ static int simple_probe(struct platform_device *pdev) for (i = 0; i < li.link; i++) { dai_link[i].codecs = &dai_props[i].codecs; dai_link[i].num_codecs = 1; - dai_link[i].platform = &dai_props[i].platform; + dai_link[i].platform.name = dai_props[i].platform.name; + dai_link[i].platform.of_node = dai_props[i].platform.of_node; + dai_link[i].platform.dai_name = dai_props[i].platform.dai_name; }
priv->dai_props = dai_props; @@ -782,7 +784,7 @@ static int simple_probe(struct platform_device *pdev) codecs->name = cinfo->codec; codecs->dai_name = cinfo->codec_dai.name;
- platform = dai_link->platform; + platform = &dai_link->platform; platform->name = cinfo->platform;
card->name = (cinfo->card) ? cinfo->card : cinfo->name; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0462b3ec977a..466099995e44 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -915,7 +915,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
/* find one from the set of registered platforms */ for_each_component(component) { - if (!snd_soc_is_matching_component(dai_link->platform, + if (!snd_soc_is_matching_component(&dai_link->platform, component)) continue;
@@ -1026,7 +1026,7 @@ static void soc_remove_dai_links(struct snd_soc_card *card) static int snd_soc_init_platform(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { - struct snd_soc_dai_link_component *platform = dai_link->platform; + struct snd_soc_dai_link_component *platform = &dai_link->platform;
/* * FIXME @@ -1034,14 +1034,10 @@ static int snd_soc_init_platform(struct snd_soc_card *card, * this function should be removed in the future */ /* convert Legacy platform link */ - if (!platform) { - platform = devm_kzalloc(card->dev, - sizeof(struct snd_soc_dai_link_component), - GFP_KERNEL); - if (!platform) - return -ENOMEM; + if (dai_link->platform_name || dai_link->platform_of_node) { + dev_dbg(card->dev, + "ASoC: Defaulting to legacy platform data!\n");
- dai_link->platform = platform; platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL; @@ -1123,7 +1119,7 @@ static int soc_init_dai_link(struct snd_soc_card *card, * Platform may be specified by either name or OF node, but * can be left unspecified, and a dummy platform will be used. */ - if (link->platform->name && link->platform->of_node) { + if (link->platform.name && link->platform.of_node) { dev_err(card->dev, "ASoC: Both platform name/of_node are set for %s\n", link->name); @@ -1921,7 +1917,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dev_err(card->dev, "init platform error"); continue; } - dai_link->platform->name = component->name; + dai_link->platform.name = component->name;
/* convert non BE into BE */ dai_link->no_pcm = 1;