| Message ID | 20181018111829.27056-4-marcel@ziswiler.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | ASoC: last minute fixes | expand |
On 18/10/2018 12:18, Marcel Ziswiler wrote: > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > This fixes the following error as seen post commit daecf46ee0e5 > ("ASoC: soc-core: use snd_soc_dai_link_component for platform") on > Apalis TK1 after initial probe deferral: > > tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for > sgtl5000 > tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 > tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) > tegra-snd-sgtl5000: probe of sound failed with error -22 > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > Changes in v1: > - Split from the Tegra series as suggested by Mark. > - Fix issue in soc-core rather than working around it in tegra_sgtl5000. > > sound/soc/soc-core.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 6ddcf12bc030..b97624005976 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -2733,7 +2733,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) > int snd_soc_register_card(struct snd_soc_card *card) > { > int i, ret; > - struct snd_soc_dai_link *link; > + struct snd_soc_dai_link *link = NULL; > > if (!card->name || !card->dev) > return -EINVAL; > @@ -2744,7 +2744,7 @@ int snd_soc_register_card(struct snd_soc_card *card) > if (ret) { > dev_err(card->dev, "ASoC: failed to init link %s\n", > link->name); > - return ret; > + goto err; > } > } > > @@ -2763,7 +2763,17 @@ int snd_soc_register_card(struct snd_soc_card *card) > mutex_init(&card->mutex); > mutex_init(&card->dapm_mutex); > > - return snd_soc_bind_card(card); > + ret = snd_soc_bind_card(card); > + if (ret) > + goto err; > + > + return 0; > + > +err: > + if (link && link->platform) > + link->platform = NULL; Looking at snd_soc_init_platform(), it seems that the platform pointer can be allocated by the machine driver and so if it is not allocated by the core, then I don't think we should clear it here. Seems we need a way to determine if this was allocated by the core. Furthermore, it seems that it is possible that there is more than one link that might be to be cleared. Cheers Jon
On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote: > Looking at snd_soc_init_platform(), it seems that the platform pointer > can be allocated by the machine driver and so if it is not allocated by > the core, then I don't think we should clear it here. Seems we need a > way to determine if this was allocated by the core. Indeed, this is a bit of a mess. We probably shouldn't be modifying the data that the drivers passed in, otherwise we get into trouble like this. That suggests that we should copy the data, probably all of it. I will try to have a proper look at this next week. > Furthermore, it seems that it is possible that there is more than one > link that might be to be cleared. Yes, that's an issue as well.
Hi Mark, On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote: > On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote: > > > Looking at snd_soc_init_platform(), it seems that the platform pointer > > can be allocated by the machine driver and so if it is not allocated by > > the core, then I don't think we should clear it here. Seems we need a > > way to determine if this was allocated by the core. > > Indeed, this is a bit of a mess. We probably shouldn't be modifying the > data that the drivers passed in, otherwise we get into trouble like > this. That suggests that we should copy the data, probably all of it. > I will try to have a proper look at this next week. did you find the time to look into this? The downstream Raspberry Pi kernel contains a bunch of machine drivers that are implemented in a similar way as the tegra_sgtl5000 driver (static card and dai link structs, dai_link->platform_of_node filled in from device tree) which are breaking in 4.20 on deferred probing. Switching these drivers to dynamically allocated dai link structs, like 76836fd35492 "ASoC: omap-abe-twl6040: Fix missing audio card caused by deferred probing" would be a possibility, but if there's some solution on the horizon that doesn't require changes to the driver code it'd be easier to wait for that. so long, Hias > > Furthermore, it seems that it is possible that there is more than one > > link that might be to be cleared. > > Yes, that's an issue as well. > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Mark, Kuninori On 18/12/2018 17:40, Matthias Reichl wrote: > Hi Mark, > > On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote: >> On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote: >> >>> Looking at snd_soc_init_platform(), it seems that the platform pointer >>> can be allocated by the machine driver and so if it is not allocated by >>> the core, then I don't think we should clear it here. Seems we need a >>> way to determine if this was allocated by the core. >> >> Indeed, this is a bit of a mess. We probably shouldn't be modifying the >> data that the drivers passed in, otherwise we get into trouble like >> this. That suggests that we should copy the data, probably all of it. >> I will try to have a proper look at this next week. > > did you find the time to look into this? > > The downstream Raspberry Pi kernel contains a bunch of machine drivers > that are implemented in a similar way as the tegra_sgtl5000 driver > (static card and dai link structs, dai_link->platform_of_node filled > in from device tree) which are breaking in 4.20 on deferred probing. > > Switching these drivers to dynamically allocated dai link structs, > like 76836fd35492 "ASoC: omap-abe-twl6040: Fix missing audio card > caused by deferred probing" would be a possibility, but if there's > some solution on the horizon that doesn't require changes to the > driver code it'd be easier to wait for that. > > so long, > > Hias > >>> Furthermore, it seems that it is possible that there is more than one >>> link that might be to be cleared. >> >> Yes, that's an issue as well. 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? Cheers Jon
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 think same issue happen on codec side too, so it cares it too. --------------- diff --git a/include/sound/soc.h b/include/sound/soc.h index 8ec1de8..49ac5a8 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -985,6 +985,10 @@ struct snd_soc_dai_link { /* Do not create a PCM for this DAI link (Backend link) */ unsigned int ignore:1; + /* allocated dai_link_comonent. These should be removed in the future */ + unsigned int allocated_platform:1; + unsigned int allocated_codecs:1; + struct list_head list; /* DAI link list of the soc card */ struct snd_soc_dobj dobj; /* For topology */ }; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0462b3e..49ccea3 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1023,6 +1023,25 @@ static void soc_remove_dai_links(struct snd_soc_card *card) } } +static void snd_soc_init_dai_link_component(struct snd_soc_card *card) +{ + struct snd_soc_dai_link *dai_link; + int i; + + /* + * FIXME + * + * this function should be removed in the future + */ + for_each_card_prelinks(card, i, dai_link) { + /* see snd_soc_init_platform */ + if (dai_link->allocated_platform) + dai_link->platform = NULL; + if (dai_link->allocated_codecs) + dai_link->codecs = NULL; + } +} + static int snd_soc_init_platform(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { @@ -1042,6 +1061,8 @@ static int snd_soc_init_platform(struct snd_soc_card *card, return -ENOMEM; dai_link->platform = platform; + dai_link->allocated_platform = 1; + platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL; @@ -1069,6 +1090,8 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, if (!dai_link->codecs) return -ENOMEM; + dai_link->allocated_codecs = 1; + dai_link->codecs[0].name = dai_link->codec_name; dai_link->codecs[0].of_node = dai_link->codec_of_node; dai_link->codecs[0].dai_name = dai_link->codec_dai_name; @@ -2739,6 +2762,8 @@ int snd_soc_register_card(struct snd_soc_card *card) if (!card->name || !card->dev) return -EINVAL; + snd_soc_init_dai_link_component(card); + for_each_card_prelinks(card, i, link) { ret = soc_init_dai_link(card, link); --------------- Best regards --- Kuninori Morimoto
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. > I think same issue happen on codec side too, so it cares it too. > > --------------- > diff --git a/include/sound/soc.h b/include/sound/soc.h > index 8ec1de8..49ac5a8 100644 > --- a/include/sound/soc.h > +++ b/include/sound/soc.h > @@ -985,6 +985,10 @@ struct snd_soc_dai_link { > /* Do not create a PCM for this DAI link (Backend link) */ > unsigned int ignore:1; > > + /* allocated dai_link_comonent. These should be removed in the future */ > + unsigned int allocated_platform:1; > + unsigned int allocated_codecs:1; > + You should add a comment here to state that these should not be modified by the machine driver and are private to the sound core. > struct list_head list; /* DAI link list of the soc card */ > struct snd_soc_dobj dobj; /* For topology */ > }; > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 0462b3e..49ccea3 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -1023,6 +1023,25 @@ static void soc_remove_dai_links(struct snd_soc_card *card) > } > } > > +static void snd_soc_init_dai_link_component(struct snd_soc_card *card) > +{ > + struct snd_soc_dai_link *dai_link; > + int i; > + > + /* > + * FIXME > + * > + * this function should be removed in the future > + */ > + for_each_card_prelinks(card, i, dai_link) { > + /* see snd_soc_init_platform */ > + if (dai_link->allocated_platform) > + dai_link->platform = NULL; > + if (dai_link->allocated_codecs) > + dai_link->codecs = NULL; > + } > +} > + It is still a little fragile because there is nothing to prevent a machine driver doing the wrong thing and setting these when they should not. > static int snd_soc_init_platform(struct snd_soc_card *card, > struct snd_soc_dai_link *dai_link) > { > @@ -1042,6 +1061,8 @@ static int snd_soc_init_platform(struct snd_soc_card *card, > return -ENOMEM; > > dai_link->platform = platform; > + dai_link->allocated_platform = 1; > + > platform->name = dai_link->platform_name; > platform->of_node = dai_link->platform_of_node; > platform->dai_name = NULL; > @@ -1069,6 +1090,8 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, > if (!dai_link->codecs) > return -ENOMEM; > > + dai_link->allocated_codecs = 1; > + > dai_link->codecs[0].name = dai_link->codec_name; > dai_link->codecs[0].of_node = dai_link->codec_of_node; > dai_link->codecs[0].dai_name = dai_link->codec_dai_name; > @@ -2739,6 +2762,8 @@ int snd_soc_register_card(struct snd_soc_card *card) > if (!card->name || !card->dev) > return -EINVAL; > > + snd_soc_init_dai_link_component(card); > + > for_each_card_prelinks(card, i, link) { > > ret = soc_init_dai_link(card, link); I still question if the platform link component needs to be allocated and why it cannot be in the DAI link structure? If it was static, then ... 1. If the dai_link->platform_name or dai_link->platform_of_node are populated and the platform->name/of_node are not populated then use platform_name/of_node as the platform->name/of_node. 2. If the dai_link->platform_name or dai_link->platform_of_node are not populated and the platform->name/of_node are populated then there is nothing to do, just use platform->name/of_node. 3. If the dai_link->platform_name or dai_link->platform_of_node are populated and the platform->name/of_node are populated then WARN and default to the platform_name/of_node as the platform->name/of_node. Could this work? Cheers Jon
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. Cheers Jon
On Tue, Dec 18, 2018 at 06:40:40PM +0100, Matthias Reichl wrote: > On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote: > > On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote: > > Indeed, this is a bit of a mess. We probably shouldn't be modifying the > > data that the drivers passed in, otherwise we get into trouble like > > this. That suggests that we should copy the data, probably all of it. > > I will try to have a proper look at this next week. > did you find the time to look into this? No, the end of the year turned out to be really busy unfortunately.
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;
On Tue, Jan 08, 2019 at 03:48:27PM +0000, Jon Hunter wrote: > > 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 ... Thanks for working on this, that looks nice and simple - can you send it as a real patch please (not reviewed properly yet but...)?
Hi Jon Thank you for your help > > 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. OK, thanks. It *will* be static, but not yet. Thank you for your patch. I guess you worry about allocated memory leak when failed case ? It is using devm_kzalloc(), so all allocated memory will be automatically freed when card was detached. But indeed if driver gots -EPROBE_DEFER many times, it will allocate many unused platform. Here is the background of snd_soc_init_platform. Legacy dai_link was xxx_name/xxx_of_node style, but multi codec support starts to use snd_soc_dai_link_component style. OTOH Lars-Petter is thinking that current ALSA SoC is not good match for modern sound device. I guess, we need "multi xxx" support as 1st step for modern sound device. "multi codec" is already supported, "multi cpu" patch was posted, but not yet accepted (or rejected ??). "multi platform" is no plan (?). These want to use snd_soc_dai_link_component style, because it is nice for multi xxx support style, I think. I think no one is planing for "multi platform" so far, thus, I posted snd_soc_dai_link_component style for it. # Maybe it should have num_platform, too. # all driver will have .num_platform = 1, thus I didn't added. # maybe it was my fault... The reason why platform/codec is allocating/copying by snd_soc_init_xxx so far is that it is glue for xxx_name/xxx_of_node (legacy style) <-> snd_soc_init_platform (modern style). I want to which to modern style immediately and remove legacy style. But as you know, we have too many ALSA SoC drivers now. So, if I posted "switch legacy style to modern style" patch for each (= for codec, for platform, for cpu), it will be patch-bomb, and Lars-Petter/Mark don't like it. Thus, I'm waiting "multi CPU" support patch. If CPU/Codec/Platform can be snd_soc_init_platform style, then, we can switch to modern style for all drivers. Then, all driver will have *static* platform. # So, I guess if your driver can switch to use # snd_soc_init_platform style directly, your problem can gone ? > Mark If there is no plan for "multi CPU" so far, I can post snd_soc_dai_link_component style for CPU. and post switch modern style for all drivers. Then, this issue can be solved ? Best regards --- Kuninori Morimoto
On 09/01/2019 01:51, Kuninori Morimoto wrote: > > Hi Jon > > Thank you for your help > >>> 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. > > OK, thanks. > It *will* be static, but not yet. > Thank you for your patch. > I guess you worry about allocated memory leak when failed case ? > It is using devm_kzalloc(), so all allocated memory will be automatically > freed when card was detached. > But indeed if driver gots -EPROBE_DEFER many times, > it will allocate many unused platform. No. Offline you had suggested using kmalloc and not devm_kzalloc and so I was worried in that case about a memory leak. Right now I am only concerned about an invalid pointer that is not being handled correctly. > Here is the background of snd_soc_init_platform. > > Legacy dai_link was xxx_name/xxx_of_node style, > but multi codec support starts to use snd_soc_dai_link_component style. > OTOH Lars-Petter is thinking that current ALSA SoC is not good match > for modern sound device. > I guess, we need "multi xxx" support as 1st step for modern sound device. > "multi codec" is already supported, > "multi cpu" patch was posted, but not yet accepted (or rejected ??). > "multi platform" is no plan (?). I would like someone to explain what multi-platform means? Even if a soundcard supports multiple platforms, there is only one platform you are using at any time so ... > These want to use snd_soc_dai_link_component style, > because it is nice for multi xxx support style, I think. > I think no one is planing for "multi platform" so far, thus, > I posted snd_soc_dai_link_component style for it. > # Maybe it should have num_platform, too. > # all driver will have .num_platform = 1, thus I didn't added. > # maybe it was my fault... ... I don't understand why you would ever need a 'num_platform' as the machine driver just needs to understand which platform is using it at any given time. Right? > The reason why platform/codec is allocating/copying by snd_soc_init_xxx > so far is that it is glue for > xxx_name/xxx_of_node (legacy style) <-> snd_soc_init_platform (modern style). > > I want to which to modern style immediately and remove legacy style. > But as you know, we have too many ALSA SoC drivers now. > So, if I posted "switch legacy style to modern style" patch > for each (= for codec, for platform, for cpu), it will be patch-bomb, > and Lars-Petter/Mark don't like it. > Thus, I'm waiting "multi CPU" support patch. Sorry, I still don't understand the dependency on the multi CPU and why we need to wait. > If CPU/Codec/Platform can be snd_soc_init_platform style, > then, we can switch to modern style for all drivers. > Then, all driver will have *static* platform. > > # So, I guess if your driver can switch to use > # snd_soc_init_platform style directly, your problem can gone ? Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this. Cheers Jon
On Wed, Jan 09, 2019 at 11:03:44AM +0000, Jon Hunter wrote: > On 09/01/2019 01:51, Kuninori Morimoto wrote: > > "multi platform" is no plan (?). > I would like someone to explain what multi-platform means? Even if a > soundcard supports multiple platforms, there is only one platform you > are using at any time so ... Platform in this case means DMA driver (for historical reasons I've never entirely understood the DMA drivers got called the platform drivers). It is possible that a system might have multiple DMA implementations in a single sound card but fortunately we don't seem to run into that. > > I want to which to modern style immediately and remove legacy style. > > But as you know, we have too many ALSA SoC drivers now. > > So, if I posted "switch legacy style to modern style" patch > > for each (= for codec, for platform, for cpu), it will be patch-bomb, > > and Lars-Petter/Mark don't like it. > > Thus, I'm waiting "multi CPU" support patch. > Sorry, I still don't understand the dependency on the multi CPU and why > we need to wait. I believe Morimoto-san's concern is to minimize the number of refactorings of the drivers as that gets disruptive. That's definitely a valid concern but we can't postpone fixing bugs over releases, we need things to work for people. > > If CPU/Codec/Platform can be snd_soc_init_platform style, > > then, we can switch to modern style for all drivers. > > Then, all driver will have *static* platform. > > # So, I guess if your driver can switch to use > > # snd_soc_init_platform style directly, your problem can gone ? > Yes that is an alternative and I can convert all the Tegra machine > drivers to use this now. However, that will not solve the problem for > non-Tegra devices and everyone will have to do this. We're going to have to go through another round of conversions that touch everything at some point no matter what :/
On 09/01/2019 12:53, Mark Brown wrote: ... >> Yes that is an alternative and I can convert all the Tegra machine >> drivers to use this now. However, that will not solve the problem for >> non-Tegra devices and everyone will have to do this. > > We're going to have to go through another round of conversions that > touch everything at some point no matter what :/ Do you have a preference here? Do you think that we can fix-up the soc-core or should I go ahead and migrate the Tegra machine driver to workaround this issue now? Cheers Jon
On Wed, Jan 09, 2019 at 02:11:58PM +0000, Jon Hunter wrote: > On 09/01/2019 12:53, Mark Brown wrote: > >> Yes that is an alternative and I can convert all the Tegra machine > >> drivers to use this now. However, that will not solve the problem for > >> non-Tegra devices and everyone will have to do this. > > We're going to have to go through another round of conversions that > > touch everything at some point no matter what :/ > Do you have a preference here? Do you think that we can fix-up the > soc-core or should I go ahead and migrate the Tegra machine driver to > workaround this issue now? We're going to need to migrate Tegra regardless so it'd be good to do that whatever happens, I'm intending to try to properly review the patch today.
Hi Mark, Jon > No. Offline you had suggested using kmalloc and not devm_kzalloc and so > I was worried in that case about a memory leak. Right now I am only > concerned about an invalid pointer that is not being handled correctly. I'm sorry I was confused/misunderstood, kmalloc idea was wrong. > I would like someone to explain what multi-platform means? Even if a > soundcard supports multiple platforms, there is only one platform you > are using at any time so ... (snip) > ... I don't understand why you would ever need a 'num_platform' as the > machine driver just needs to understand which platform is using it at > any given time. Right? As Mark explained, "platform" on ALSA SoC means "DMA", and we might have multiple DMA sound sysytem (= multi-platform) in the future. Currently, all driver/sysytem is using single DMA for 1 sound card. > > # So, I guess if your driver can switch to use > > # snd_soc_init_platform style directly, your problem can gone ? > > Yes that is an alternative and I can convert all the Tegra machine > drivers to use this now. However, that will not solve the problem for > non-Tegra devices and everyone will have to do this. Yeah I agree. But my concern is that the same problem happen on codec side too, by same logic, because snd_soc_init_multicodec() is overwriting dai_link too. Your posted patch solved platform side only, I think. > > >> Yes that is an alternative and I can convert all the Tegra machine > > >> drivers to use this now. However, that will not solve the problem for > > >> non-Tegra devices and everyone will have to do this. > > > > We're going to have to go through another round of conversions that > > > touch everything at some point no matter what :/ > > > Do you have a preference here? Do you think that we can fix-up the > > soc-core or should I go ahead and migrate the Tegra machine driver to > > workaround this issue now? > > We're going to need to migrate Tegra regardless so it'd be good to do > that whatever happens, I'm intending to try to properly review the patch > today. As I mentioned above, I think we have same issue on codec side too. exchanging *platform to platform doesn't solve all issues. And we need to exchange all driver again if we had multi-platform support in the future (I don't know when it can happen though...) My posted quick-patch can solve "dirty pointer" issue, but it can't solve "memory leak" issue. This issue will be solved if all driver can switch to modern style, but it needs more time. Are these correct ? So, how about this ? I will try to add snd_soc_dai_link_component support for CPU, and switch all driver to use modern style for v5.1 (or v5.2 ?). Until then, as temporary solution, we can use above quick-patch style. And to avoid "memory leak crash" attach, it temporary have bind dai_link limitation (max 5time?). If it goes to max limitation, ALSA SoC doesn't allow to try again. In such case, all related CPU/Codec driver need to rmmod/unbind, and insmod/bind again. Then, the limitation will be 0 cleared. You can try bind again. It can solve "dirty pointer" issue, "memory leak" issue, and "memory leak attack" issue. The problem is that code can be dirty temporary. But it will be removed if all driver can be swtich to modern style. Best regards --- Kuninori Morimoto
Hi Mark, Jon again > My posted quick-patch can solve "dirty pointer" issue, > but it can't solve "memory leak" issue. > This issue will be solved if all driver can switch to > modern style, but it needs more time. > Are these correct ? Sorry I was very confused. I think the issue is only "dirty pointer", there is no "memory leak" issue (= devm_kzalloc()). And the things Jon is worry about is that why we need to have platform pointer. And the answer is we need multi platform support in the future (pointer is prepare for it). Are these correct ? If so my posted patch can solve all issues ? Best regards --- Kuninori Morimoto
On 10/01/2019 01:16, Kuninori Morimoto wrote: ... > As I mentioned above, I think we have same issue on codec side too. Actually no. Looking at snd_soc_init_multicodec() it always allocates memory if any of the 'legacy' codec members (codec_name/codec_of_node/codec_dai_name) are populated. However, this looks quite fragile to me and is susceptible to leaking memory if the user/machine driver already incorrectly allocated the memory as well as populating these legacy codec members. My concern about all of this is it is not fool proof and hard to detect if a machine driver is doing something bad that it should not. > exchanging *platform to platform doesn't solve all issues. > And we need to exchange all driver again if we had multi-platform > support in the future (I don't know when it can happen though...) > > My posted quick-patch can solve "dirty pointer" issue, > but it can't solve "memory leak" issue. > This issue will be solved if all driver can switch to > modern style, but it needs more time. > Are these correct ? > > So, how about this ? It is still fragile. Again the machine driver could have incorrectly set these 'allocated_platform/codecs' members as they are exposed to the machine driver. You have no way to determine if the machine driver is doing the correct thing or not. The problem becomes more complex with probe deferral. Cheers Jon
Hi Jon > Actually no. Looking at snd_soc_init_multicodec() it always allocates > memory if any of the 'legacy' codec members > (codec_name/codec_of_node/codec_dai_name) are populated. However, this > looks quite fragile to me and is susceptible to leaking memory if the > user/machine driver already incorrectly allocated the memory as well as > populating these legacy codec members. Yeah, sorry I was 100% misunderstood. I thought there is a case that defered card connected to unbind_card_list, and try snd_soc_init_platform/codec again without freeing memory... very mess > My concern about all of this is it is not fool proof and hard to detect > if a machine driver is doing something bad that it should not. Yeah, agree. Best solution is removing legacy style, I think. > It is still fragile. Again the machine driver could have incorrectly set > these 'allocated_platform/codecs' members as they are exposed to the > machine driver. You have no way to determine if the machine driver is > doing the correct thing or not. The problem becomes more complex with > probe deferral. Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ? Best regards --- Kuninori Morimoto
On 11/01/2019 00:52, Kuninori Morimoto wrote: ... >> It is still fragile. Again the machine driver could have incorrectly set >> these 'allocated_platform/codecs' members as they are exposed to the >> machine driver. You have no way to determine if the machine driver is >> doing the correct thing or not. The problem becomes more complex with >> probe deferral. > > Indeed there is such case so far, but my understanding is that current > driver should select "legacy style" or "modern style". > If driver setup it as "legacy", but access to "modern" member, > it is driver side bug, right ? Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link structure today it is not clear what the driver should be setting and what is 'modern' and what is 'legacy'. You need to dig through the git history and code to figure this out. So you could say it is not very well documented/commented from a soc-core perspective and could be easy for a driver writer to get themselves in a pickle/mess. Anyway, that is easy to fix and we could add some comments to clear it up. Cheers Jon
Hi Jon > > Indeed there is such case so far, but my understanding is that current > > driver should select "legacy style" or "modern style". > > If driver setup it as "legacy", but access to "modern" member, > > it is driver side bug, right ? > > Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link > structure today it is not clear what the driver should be setting and > what is 'modern' and what is 'legacy'. You need to dig through the git > history and code to figure this out. So you could say it is not very > well documented/commented from a soc-core perspective and could be easy > for a driver writer to get themselves in a pickle/mess. Anyway, that is > easy to fix and we could add some comments to clear it up. Thank you for your feedback. Yes, indeed there is no enough information/documentation about legacy/modern style, and its plan (= all driver will be switched to modern, legacy will be removed, etc, etc..). So, can you agree about these ? 1) Add enough information/documentation about legacy/modern style and its plan. 2) Add dirty pointer fixup patch as workaround 3) switch to modern style as much as possible 1) and 2) are needed immediately. 3) needs more time, but we can try Best regards --- Kuninori Morimoto
On 11/01/2019 08:51, Kuninori Morimoto wrote: >>> Indeed there is such case so far, but my understanding is that current >>> driver should select "legacy style" or "modern style". >>> If driver setup it as "legacy", but access to "modern" member, >>> it is driver side bug, right ? >> >> Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link >> structure today it is not clear what the driver should be setting and >> what is 'modern' and what is 'legacy'. You need to dig through the git >> history and code to figure this out. So you could say it is not very >> well documented/commented from a soc-core perspective and could be easy >> for a driver writer to get themselves in a pickle/mess. Anyway, that is >> easy to fix and we could add some comments to clear it up. > > Thank you for your feedback. > Yes, indeed there is no enough information/documentation about > legacy/modern style, and its plan > (= all driver will be switched to modern, legacy will be removed, etc, etc..). > > So, can you agree about these ? > 1) Add enough information/documentation about legacy/modern style and its plan. > 2) Add dirty pointer fixup patch as workaround > 3) switch to modern style as much as possible I think that Mark needs to decided on whether use your 'dirty pointer' fix or not. Jon
On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote: > On 11/01/2019 08:51, Kuninori Morimoto wrote: > > So, can you agree about these ? > > 1) Add enough information/documentation about legacy/modern style and its plan. > > 2) Add dirty pointer fixup patch as workaround > > 3) switch to modern style as much as possible > I think that Mark needs to decided on whether use your 'dirty pointer' > fix or not. I've gone ahead with Curtis' version of Morimoto-san's patch just now - hopefully that's fine for v5.0. Like we've been saying neither approach is ideal, thanks Jon and Morimoto-san for your efforts analyzing this and your proposals.
On 14/01/2019 23:02, Mark Brown wrote: > On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote: >> On 11/01/2019 08:51, Kuninori Morimoto wrote: > >>> So, can you agree about these ? >>> 1) Add enough information/documentation about legacy/modern style and its plan. >>> 2) Add dirty pointer fixup patch as workaround >>> 3) switch to modern style as much as possible > >> I think that Mark needs to decided on whether use your 'dirty pointer' >> fix or not. > > I've gone ahead with Curtis' version of Morimoto-san's patch just now - > hopefully that's fine for v5.0. Like we've been saying neither approach > is ideal, thanks Jon and Morimoto-san for your efforts analyzing this > and your proposals. Thanks! Works for me. Jon
On Tue, Jan 15, 2019 at 03:26:42PM +0000, Jon Hunter wrote: > > On 14/01/2019 23:02, Mark Brown wrote: > > On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote: > >> On 11/01/2019 08:51, Kuninori Morimoto wrote: > > > >>> So, can you agree about these ? > >>> 1) Add enough information/documentation about legacy/modern style and its plan. > >>> 2) Add dirty pointer fixup patch as workaround > >>> 3) switch to modern style as much as possible > > > >> I think that Mark needs to decided on whether use your 'dirty pointer' > >> fix or not. > > > > I've gone ahead with Curtis' version of Morimoto-san's patch just now - > > hopefully that's fine for v5.0. Like we've been saying neither approach > > is ideal, thanks Jon and Morimoto-san for your efforts analyzing this > > and your proposals. > > Thanks! Works for me. Thanks alot, works here, too on 4.20 and 5.0-rc2! so long, Hias
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 6ddcf12bc030..b97624005976 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2733,7 +2733,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) int snd_soc_register_card(struct snd_soc_card *card) { int i, ret; - struct snd_soc_dai_link *link; + struct snd_soc_dai_link *link = NULL; if (!card->name || !card->dev) return -EINVAL; @@ -2744,7 +2744,7 @@ int snd_soc_register_card(struct snd_soc_card *card) if (ret) { dev_err(card->dev, "ASoC: failed to init link %s\n", link->name); - return ret; + goto err; } } @@ -2763,7 +2763,17 @@ int snd_soc_register_card(struct snd_soc_card *card) mutex_init(&card->mutex); mutex_init(&card->dapm_mutex); - return snd_soc_bind_card(card); + ret = snd_soc_bind_card(card); + if (ret) + goto err; + + return 0; + +err: + if (link && link->platform) + link->platform = NULL; + + return ret; } EXPORT_SYMBOL_GPL(snd_soc_register_card);