From: Jon Hunter <jonathanh@nvidia.com> To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: alsa-devel@alsa-project.org, Marcel Ziswiler <marcel.ziswiler@toradex.com>, linux-kernel@vger.kernel.org, Liam Girdwood <lgirdwood@gmail.com>, Takashi Iwai <tiwai@suse.com>, Matthias Reichl <hias@horus.com>, Mark Brown <broonie@kernel.org>, Marcel Ziswiler <marcel@ziswiler.com>, linux-tegra@vger.kernel.org Subject: Re: [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement Date: Tue, 8 Jan 2019 10:50:18 +0000 [thread overview] Message-ID: <b5164c93-8be8-1c3d-9415-c895da6ad622@nvidia.com> (raw) In-Reply-To: <87lg3vuc7p.wl-kuninori.morimoto.gx@renesas.com> 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 -- nvpublic
WARNING: multiple messages have this Message-ID (diff)
From: Jon Hunter <jonathanh@nvidia.com> To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Mark Brown <broonie@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, <linux-tegra@vger.kernel.org>, Matthias Reichl <hias@horus.com>, <alsa-devel@alsa-project.org>, Marcel Ziswiler <marcel.ziswiler@toradex.com>, Takashi Iwai <tiwai@suse.com>, <linux-kernel@vger.kernel.org>, Marcel Ziswiler <marcel@ziswiler.com> Subject: Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement Date: Tue, 8 Jan 2019 10:50:18 +0000 [thread overview] Message-ID: <b5164c93-8be8-1c3d-9415-c895da6ad622@nvidia.com> (raw) In-Reply-To: <87lg3vuc7p.wl-kuninori.morimoto.gx@renesas.com> 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 -- nvpublic
next prev parent reply other threads:[~2019-01-08 10:50 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-18 11:18 [PATCH v1 0/3] ASoC: last minute fixes Marcel Ziswiler 2018-10-18 11:18 ` Marcel Ziswiler 2018-10-18 11:18 ` [PATCH v1 1/3] ASoC: tegra_sgtl5000: fix device_node refcounting Marcel Ziswiler 2018-10-18 11:18 ` Marcel Ziswiler 2018-10-18 11:18 ` [PATCH v1 2/3] ASoC: soc-core: fix trivial checkpatch issues Marcel Ziswiler 2018-10-18 11:18 ` Marcel Ziswiler 2018-10-19 12:26 ` Applied "ASoC: soc-core: fix trivial checkpatch issues" to the asoc tree Mark Brown 2018-10-19 12:26 ` Mark Brown 2018-10-19 12:26 ` Mark Brown 2018-10-19 12:34 ` Mark Brown 2018-10-19 12:34 ` Mark Brown 2018-10-19 12:34 ` Mark Brown 2018-10-18 11:18 ` [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement Marcel Ziswiler 2018-10-18 11:18 ` Marcel Ziswiler 2018-10-19 10:22 ` Jon Hunter 2018-10-19 10:22 ` Jon Hunter 2018-10-21 11:23 ` Mark Brown 2018-10-21 11:23 ` Mark Brown 2018-12-18 17:40 ` Matthias Reichl 2018-12-18 17:40 ` [alsa-devel] " Matthias Reichl 2019-01-03 16:42 ` Jon Hunter 2019-01-03 16:42 ` Jon Hunter 2019-01-08 2:25 ` Kuninori Morimoto 2019-01-08 2:25 ` Kuninori Morimoto 2019-01-08 10:50 ` Jon Hunter [this message] 2019-01-08 10:50 ` Jon Hunter 2019-01-08 12:03 ` Jon Hunter 2019-01-08 12:03 ` Jon Hunter 2019-01-08 15:48 ` Jon Hunter 2019-01-08 15:48 ` Jon Hunter 2019-01-08 16:09 ` Mark Brown 2019-01-08 16:09 ` [alsa-devel] " Mark Brown 2019-01-09 1:51 ` Kuninori Morimoto 2019-01-09 1:51 ` Kuninori Morimoto 2019-01-09 11:03 ` Jon Hunter 2019-01-09 11:03 ` Jon Hunter 2019-01-09 12:53 ` Mark Brown 2019-01-09 12:53 ` [alsa-devel] " Mark Brown 2019-01-09 14:11 ` Jon Hunter 2019-01-09 14:11 ` Jon Hunter 2019-01-09 14:14 ` Mark Brown 2019-01-09 14:14 ` [alsa-devel] " Mark Brown 2019-01-10 1:16 ` Kuninori Morimoto 2019-01-10 3:46 ` Kuninori Morimoto 2019-01-10 10:56 ` Jon Hunter 2019-01-10 10:56 ` Jon Hunter 2019-01-11 0:52 ` Kuninori Morimoto 2019-01-11 0:52 ` [alsa-devel] " Kuninori Morimoto 2019-01-11 8:41 ` Jon Hunter 2019-01-11 8:41 ` Jon Hunter 2019-01-11 8:51 ` Kuninori Morimoto 2019-01-11 8:51 ` Kuninori Morimoto 2019-01-11 9:15 ` Jon Hunter 2019-01-11 9:15 ` Jon Hunter 2019-01-14 23:02 ` Mark Brown 2019-01-14 23:02 ` [alsa-devel] " Mark Brown 2019-01-15 15:26 ` Jon Hunter 2019-01-15 15:26 ` Jon Hunter 2019-01-15 18:07 ` Matthias Reichl 2019-01-08 15:33 ` Mark Brown 2019-01-08 15:33 ` [alsa-devel] " Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b5164c93-8be8-1c3d-9415-c895da6ad622@nvidia.com \ --to=jonathanh@nvidia.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=hias@horus.com \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=marcel.ziswiler@toradex.com \ --cc=marcel@ziswiler.com \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.