[v1,3/3] ASoC: soc-core: fix platform name vs. of_node assignement

Message ID 20181018111829.27056-4-marcel@ziswiler.com
State New
Headers show
Series
  • ASoC: last minute fixes
Related show

Commit Message

Marcel Ziswiler Oct. 18, 2018, 11:18 a.m.
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(-)

Comments

Jon Hunter Oct. 19, 2018, 10:22 a.m. | #1
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
Mark Brown Oct. 21, 2018, 11:23 a.m. | #2
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.
Matthias Reichl Dec. 18, 2018, 5:40 p.m. | #3
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
Jon Hunter Jan. 3, 2019, 4:42 p.m. | #4
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
Kuninori Morimoto Jan. 8, 2019, 2:25 a.m. | #5
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
Jon Hunter Jan. 8, 2019, 10:50 a.m. | #6
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
Jon Hunter Jan. 8, 2019, 12:03 p.m. | #7
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
Mark Brown Jan. 8, 2019, 3:33 p.m. | #8
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.
Jon Hunter Jan. 8, 2019, 3:48 p.m. | #9
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;
Mark Brown Jan. 8, 2019, 4:09 p.m. | #10
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...)?
Kuninori Morimoto Jan. 9, 2019, 1:51 a.m. | #11
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
Jon Hunter Jan. 9, 2019, 11:03 a.m. | #12
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
Mark Brown Jan. 9, 2019, 12:53 p.m. | #13
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 :/
Jon Hunter Jan. 9, 2019, 2:11 p.m. | #14
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
Mark Brown Jan. 9, 2019, 2:14 p.m. | #15
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.
Kuninori Morimoto Jan. 10, 2019, 1:16 a.m. | #16
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
Kuninori Morimoto Jan. 10, 2019, 3:46 a.m. | #17
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
Jon Hunter Jan. 10, 2019, 10:56 a.m. | #18
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
Kuninori Morimoto Jan. 11, 2019, 12:52 a.m. | #19
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
Jon Hunter Jan. 11, 2019, 8:41 a.m. | #20
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
Kuninori Morimoto Jan. 11, 2019, 8:51 a.m. | #21
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
Jon Hunter Jan. 11, 2019, 9:15 a.m. | #22
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
Mark Brown Jan. 14, 2019, 11:02 p.m. | #23
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.
Jon Hunter Jan. 15, 2019, 3:26 p.m. | #24
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
Matthias Reichl Jan. 15, 2019, 6:07 p.m. | #25
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

Patch

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);