Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Takashi Iwai <tiwai@suse.de>]

At Mon, 09 Feb 2015 12:26:02 +0100,
Lars-Peter Clausen wrote:
> 
> On 02/09/2015 11:48 AM, Takashi Iwai wrote:
> > At Mon, 9 Feb 2015 08:52:49 +0000,
> > Kuninori Morimoto wrote:
> >>
> >> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>
> >> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
> >> are based on component structure. Now, each CARD device knows
> >> connected component. But CARD doesn't notice if connected component
> >> was removed when user called rmmod or unbind in current implementation.
> >> Thus, CARD which lost some components still exist in system.
> >> And then, ALSA sound card will have some problem if user used this CARD
> >> in such timing. This patch temporarily removes CARD from system
> >> if connected component was removed, and re-add it if some component
> >> was added.
> >>
> >> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
> >> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
> >> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >> ---
> >>   include/sound/soc.h  |    1 +
> >>   sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
> >>   2 files changed, 27 insertions(+)
> >>
> >> diff --git a/include/sound/soc.h b/include/sound/soc.h
> >> index b4fca9a..a90eff4 100644
> >> --- a/include/sound/soc.h
> >> +++ b/include/sound/soc.h
> >> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
> >>   	struct list_head paths;
> >>   	struct list_head dapm_list;
> >>   	struct list_head dapm_dirty;
> >> +	struct list_head unbinded_list;
> >>
> >>   	/* Generic DAPM context for the card */
> >>   	struct snd_soc_dapm_context dapm;
> >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> >> index b7ab676..f8d5498 100644
> >> --- a/sound/soc/soc-core.c
> >> +++ b/sound/soc/soc-core.c
> >> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
> >>   static LIST_HEAD(platform_list);
> >>   static LIST_HEAD(codec_list);
> >>   static LIST_HEAD(component_list);
> >> +static LIST_HEAD(unbinded_card_list);
> >>
> >>   /*
> >>    * This is a timeout to do a DAPM powerdown after a stream is closed().
> >> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
> >>   		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
> >>   	}
> >>
> >> +	mutex_lock(&client_mutex);
> >> +	list_del(&card->unbinded_list);
> >> +	mutex_unlock(&client_mutex);
> >> +
> >>   	return 0;
> >>   }
> >>   EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
> >> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
> >>
> >>   static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>   {
> >> +	struct snd_soc_card *card, *_card;
> >> +	int ret;
> >> +
> >>   	if (!component->write && !component->read) {
> >>   		if (!component->regmap)
> >>   			component->regmap = dev_get_regmap(component->dev, NULL);
> >> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>   	}
> >>
> >>   	list_add(&component->list, &component_list);
> >> +
> >> +	/* re-add temporarily removed card if exist */
> >> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
> >> +				 unbinded_list) {
> >> +		ret = snd_soc_instantiate_card(card);
> >> +		if (ret < 0)
> >> +			continue;
> >> +
> >> +		list_del(&card->unbinded_list);
> >> +	}
> >
> > [Cc'ed Lars-Peter]
> >
> > This would instantiate a card even if it's irrelevant with the given
> > component?  If so, it looks fragile, and possibly racy, when there are
> > multiple cards.
> 
> That shouldn't be a problem. snd_soc_instantiate_card() does the proper 
> locking and will return an error if not all components are ready yet.

But there is no check of card->instantiated there, so the whole path
can be called again even if the card was already instantiated?


Takashi