From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuninori Morimoto Subject: Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement Date: 08 Jan 2019 11:25:41 +0900 Message-ID: <87lg3vuc7p.wl-kuninori.morimoto.gx@renesas.com> References: <20181018111829.27056-1-marcel@ziswiler.com> <20181018111829.27056-4-marcel@ziswiler.com> <20181021112301.GC8554@sirena.org.uk> <20181218174040.k7u26vnnoplllnwb@camel2.lan> <952471da-b355-6471-6c19-5120d6704f81@nvidia.com> Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <952471da-b355-6471-6c19-5120d6704f81@nvidia.com> Sender: linux-kernel-owner@vger.kernel.org To: Jon Hunter Cc: Mark Brown , Liam Girdwood , linux-tegra@vger.kernel.org, Matthias Reichl , alsa-devel@alsa-project.org, Marcel Ziswiler , Takashi Iwai , linux-kernel@vger.kernel.org, Marcel Ziswiler List-Id: linux-tegra@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B84AC43387 for ; Tue, 8 Jan 2019 02:25:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E6D321855 for ; Tue, 8 Jan 2019 02:25:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727368AbfAHCZo (ORCPT ); Mon, 7 Jan 2019 21:25:44 -0500 Received: from relmlor1.renesas.com ([210.160.252.171]:28494 "EHLO relmlie5.idc.renesas.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727030AbfAHCZn (ORCPT ); Mon, 7 Jan 2019 21:25:43 -0500 Date: 08 Jan 2019 11:25:41 +0900 X-IronPort-AV: E=Sophos;i="5.56,452,1539615600"; d="scan'208";a="4601244" Received: from unknown (HELO relmlir6.idc.renesas.com) ([10.200.68.152]) by relmlie5.idc.renesas.com with ESMTP; 08 Jan 2019 11:25:41 +0900 Received: from morimoto-PC.renesas.com (unknown [10.166.18.140]) by relmlir6.idc.renesas.com (Postfix) with ESMTP id 0A0AC419C221; Tue, 8 Jan 2019 11:25:41 +0900 (JST) Message-ID: <87lg3vuc7p.wl-kuninori.morimoto.gx@renesas.com> From: Kuninori Morimoto To: Jon Hunter Cc: Mark Brown , Liam Girdwood , , Matthias Reichl , , Marcel Ziswiler , Takashi Iwai , , Marcel Ziswiler Subject: Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement In-Reply-To: <952471da-b355-6471-6c19-5120d6704f81@nvidia.com> References: <20181018111829.27056-1-marcel@ziswiler.com> <20181018111829.27056-4-marcel@ziswiler.com> <20181021112301.GC8554@sirena.org.uk> <20181218174040.k7u26vnnoplllnwb@camel2.lan> <952471da-b355-6471-6c19-5120d6704f81@nvidia.com> User-Agent: Wanderlust/2.15.9 Emacs/24.5 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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