Re: [PATCH v2] ASoC: core: Allow topology to override machine driver FE DAI link config.

[Guenter Roeck <linux@roeck-us.net>]

On Fri, May 03, 2019 at 10:09:28AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 5/3/19 8:29 AM, Guenter Roeck wrote:
> >On Mon, Jul 02, 2018 at 04:59:54PM +0100, Liam Girdwood wrote:
> >>Machine drivers statically define a number of DAI links that currently
> >>cannot be changed or removed by topology. This means PCMs and platform
> >>components cannot be changed by topology at runtime AND machine drivers
> >>are tightly coupled to topology.
> >>
> >>This patch allows topology to override the machine driver DAI link config
> >>in order to reuse machine drivers with different topologies and platform
> >>components. The patch supports :-
> >>
> >>1) create new FE PCMs with a topology defined PCM ID.
> >>2) destroy existing static FE PCMs
> >>3) change the platform component driver.
> >>4) assign any new HW params fixups.
> >>5) assign a new card name prefix to differentiate this topology to userspace.
> >>
> >>The patch requires no changes to the machine drivers, but does add some
> >>platform component flags that the platform component driver can assign
> >>before loading topologies.
> >>
> >
> >This patch causes most Kabylake systems, specifically those utilizing
> >kbl_rt5663_rt5514_max98927.c or kbl_da7219_max98927.c, to crash.
> 
> Guenter, can you confirm that Soraka is part of the list affected by this
> issue? It's the only device I have.
> 

I see the problem on Eve (Google Pixelbook). Both Eve and Soraka are based on
the same reference design (Poppy), so I am quite sure that Soraka is affected
as well.

> We are aware of issues on that platform with upstream code on the ssp fixup
> but were thinking it was topology related, so maybe we have a combination of
> issues... I recall Harsha (CC:ed) and team worked on this maybe around
> mid-November.
> 

It is easy to draw that conclusion, and I thought so as well initially.
The problem is the dereference sequence:

	struct snd_soc_dpcm *dpcm = container_of(
			params, struct snd_soc_dpcm, hw_params);
	struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
	struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;

dpcm->fe and dpcm->be typically point to a valid memory location
(presumably that is just what happens to be on the stack), and the
crash itself happens when fe_dai_link or be_dai_link are dereferenced.
One of those is often a NULL pointer, or at least does not point to a
valid memory location. That mislead me to believe that dpcm->fe and/or
dpcm->be is not initialized, which would indeed suggest a topology
issue.

Note that you may have to run ChromeOS userspace to see the problem;
AFAICS userspace has to send a SNDRV_PCM_IOCTL_HW_PARAMS ioctl to
the kernel to trigger it.

> That piece of code making use of dpcm internal structures has been around
> since the initial submission in mid 2017, I am not sure what the idea was
> behind all these constraints.
> 

No idea either ... which is one of the reasons why I am having a hard time
finding a fix. It is not just about finding references to fe and be - I am
not even sure if kabylake_ssp_fixup() is the appropriate function to
implement the adjustments it makes.

Is Harsha Priya still around, by any chance ?

Thanks,
Guenter

> >
> >Reason is that the fixup functions in those drivers expect params to be
> >part of struct snd_soc_dpcm:
> >
> >static int kabylake_ssp_fixup(..)
> >{
> >	...
> >	struct snd_soc_dpcm *dpcm = container_of(
> >			params, struct snd_soc_dpcm, hw_params);
> >
> >That is, however, not always the case with the new call path.
> >
> >int soc_dai_hw_params(struct snd_pcm_substream *substream,
> >...
> >	/* perform any topology hw_params fixups before DAI  */
> >	if (rtd->dai_link->be_hw_params_fixup) {
> >		ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
> >
> >called from:
> >
> >static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> >...
> >	ret = soc_dai_hw_params(substream, &codec_params, codec_dai);
> >
> >codec_params is a local variable, and container_of() on it points to some
> >random location on the stack. Result is odd and random crashes in
> >kabylake_ssp_fixup(), nad even if it doesn't crash the fixup doesn't work.
> >
> >I have no idea how to fix the problem, unfortunately.
> >
> >Guenter
> >
> >>Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> >>---
> >>
> >>Changes since V1.
> >>   o Now iterate over component_list to fix crash with DT enumerated cards.
> >>   o Make sure name prefix is only added once with deferred probiing.
> >>
> >>  include/sound/soc.h  |  13 ++++++
> >>  sound/soc/soc-core.c | 101 +++++++++++++++++++++++++++++++++++++++++--
> >>  sound/soc/soc-pcm.c  |  12 +++++
> >>  3 files changed, 123 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/include/sound/soc.h b/include/sound/soc.h
> >>index f7579f82cc00..b1d65fcb8756 100644
> >>--- a/include/sound/soc.h
> >>+++ b/include/sound/soc.h
> >>@@ -806,6 +806,14 @@ struct snd_soc_component_driver {
> >>  	unsigned int use_pmdown_time:1; /* care pmdown_time at stop */
> >>  	unsigned int endianness:1;
> >>  	unsigned int non_legacy_dai_naming:1;
> >>+
> >>+	/* this component uses topology and ignore machine driver FEs */
> >>+	const char *ignore_machine;
> >>+	const char *topology_name_prefix;
> >>+	int (*be_hw_params_fixup)(struct snd_soc_pcm_runtime *rtd,
> >>+				  struct snd_pcm_hw_params *params);
> >>+	bool use_dai_pcm_id;	/* use the DAI link PCM ID as PCM device number */
> >>+	int be_pcm_base;	/* base device ID for all BE PCMs */
> >>  };
> >>  struct snd_soc_component {
> >>@@ -963,6 +971,9 @@ struct snd_soc_dai_link {
> >>  	/* pmdown_time is ignored at stop */
> >>  	unsigned int ignore_pmdown_time:1;
> >>+	/* Do not create a PCM for this DAI link (Backend link) */
> >>+	unsigned int ignore:1;
> >>+
> >>  	struct list_head list; /* DAI link list of the soc card */
> >>  	struct snd_soc_dobj dobj; /* For topology */
> >>  };
> >>@@ -1002,6 +1013,7 @@ struct snd_soc_card {
> >>  	const char *long_name;
> >>  	const char *driver_name;
> >>  	char dmi_longname[80];
> >>+	char topology_shortname[32];
> >>  	struct device *dev;
> >>  	struct snd_card *snd_card;
> >>@@ -1011,6 +1023,7 @@ struct snd_soc_card {
> >>  	struct mutex dapm_mutex;
> >>  	bool instantiated;
> >>+	bool topology_shortname_created;
> >>  	int (*probe)(struct snd_soc_card *card);
> >>  	int (*late_probe)(struct snd_soc_card *card);
> >>diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> >>index 4663de3cf495..38bf7d01894b 100644
> >>--- a/sound/soc/soc-core.c
> >>+++ b/sound/soc/soc-core.c
> >>@@ -852,6 +852,9 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
> >>  	const char *platform_name;
> >>  	int i;
> >>+	if (dai_link->ignore)
> >>+		return 0;
> >>+
> >>  	dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name);
> >>  	if (soc_is_dai_link_bound(card, dai_link)) {
> >>@@ -1461,7 +1464,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
> >>  {
> >>  	struct snd_soc_dai_link *dai_link = rtd->dai_link;
> >>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >>-	int i, ret;
> >>+	struct snd_soc_rtdcom_list *rtdcom;
> >>+	struct snd_soc_component *component;
> >>+	int i, ret, num;
> >>  	dev_dbg(card->dev, "ASoC: probe %s dai link %d late %d\n",
> >>  			card->name, rtd->num, order);
> >>@@ -1507,9 +1512,28 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
> >>  		soc_dpcm_debugfs_add(rtd);
> >>  #endif
> >>+	num = rtd->num;
> >>+
> >>+	/*
> >>+	 * most drivers will register their PCMs using DAI link ordering but
> >>+	 * topology based drivers can use the DAI link id field to set PCM
> >>+	 * device number and then use rtd + a base offset of the BEs.
> >>+	 */
> >>+	for_each_rtdcom(rtd, rtdcom) {
> >>+		component = rtdcom->component;
> >>+
> >>+		if (!component->driver->use_dai_pcm_id)
> >>+			continue;
> >>+
> >>+		if (rtd->dai_link->no_pcm)
> >>+			num += component->driver->be_pcm_base;
> >>+		else
> >>+			num = rtd->dai_link->id;
> >>+	}
> >>+
> >>  	if (cpu_dai->driver->compress_new) {
> >>  		/*create compress_device"*/
> >>-		ret = cpu_dai->driver->compress_new(rtd, rtd->num);
> >>+		ret = cpu_dai->driver->compress_new(rtd, num);
> >>  		if (ret < 0) {
> >>  			dev_err(card->dev, "ASoC: can't create compress %s\n",
> >>  					 dai_link->stream_name);
> >>@@ -1519,7 +1543,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
> >>  		if (!dai_link->params) {
> >>  			/* create the pcm */
> >>-			ret = soc_new_pcm(rtd, rtd->num);
> >>+			ret = soc_new_pcm(rtd, num);
> >>  			if (ret < 0) {
> >>  				dev_err(card->dev, "ASoC: can't create pcm %s :%d\n",
> >>  				       dai_link->stream_name, ret);
> >>@@ -1846,6 +1870,74 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour)
> >>  EXPORT_SYMBOL_GPL(snd_soc_set_dmi_name);
> >>  #endif /* CONFIG_DMI */
> >>+static void soc_check_tplg_fes(struct snd_soc_card *card)
> >>+{
> >>+	struct snd_soc_component *component;
> >>+	const struct snd_soc_component_driver *comp_drv;
> >>+	struct snd_soc_dai_link *dai_link;
> >>+	int i;
> >>+
> >>+	list_for_each_entry(component, &component_list, list) {
> >>+
> >>+		/* does this component override FEs ? */
> >>+		if (!component->driver->ignore_machine)
> >>+			continue;
> >>+
> >>+		/* for this machine ? */
> >>+		if (strcmp(component->driver->ignore_machine,
> >>+			   card->dev->driver->name))
> >>+			continue;
> >>+
> >>+		/* machine matches, so override the rtd data */
> >>+		for (i = 0; i < card->num_links; i++) {
> >>+
> >>+			dai_link = &card->dai_link[i];
> >>+
> >>+			/* ignore this FE */
> >>+			if (dai_link->dynamic) {
> >>+				dai_link->ignore = true;
> >>+				continue;
> >>+			}
> >>+
> >>+			dev_info(card->dev, "info: override FE DAI link %s\n",
> >>+				 card->dai_link[i].name);
> >>+
> >>+			/* override platform component */
> >>+			dai_link->platform_name = component->name;
> >>+
> >>+			/* convert non BE into BE */
> >>+			dai_link->no_pcm = 1;
> >>+
> >>+			/* override any BE fixups */
> >>+			dai_link->be_hw_params_fixup =
> >>+				component->driver->be_hw_params_fixup;
> >>+
> >>+			/* most BE links don't set stream name, so set it to
> >>+			 * dai link name if it's NULL to help bind widgets.
> >>+			 */
> >>+			if (!dai_link->stream_name)
> >>+				dai_link->stream_name = dai_link->name;
> >>+		}
> >>+
> >>+		/* Inform userspace we are using alternate topology */
> >>+		if (component->driver->topology_name_prefix) {
> >>+
> >>+			/* topology shortname created ? */
> >>+			if (!card->topology_shortname_created) {
> >>+				comp_drv = component->driver;
> >>+
> >>+				snprintf(card->topology_shortname, 32, "%s-%s",
> >>+					 comp_drv->topology_name_prefix,
> >>+					 card->name);
> >>+				card->topology_shortname_created = true;
> >>+			}
> >>+
> >>+			/* use topology shortname */
> >>+			card->name = card->topology_shortname;
> >>+		}
> >>+	}
> >>+}
> >>+
> >>  static int snd_soc_instantiate_card(struct snd_soc_card *card)
> >>  {
> >>  	struct snd_soc_pcm_runtime *rtd;
> >>@@ -1855,6 +1947,9 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
> >>  	mutex_lock(&client_mutex);
> >>  	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT);
> >>+	/* check whether any platform is ignore machine FE and using topology */
> >>+	soc_check_tplg_fes(card);
> >>+
> >>  	/* bind DAIs */
> >>  	for (i = 0; i < card->num_links; i++) {
> >>  		ret = soc_bind_dai_link(card, &card->dai_link[i]);
> >>diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> >>index 153218aa4909..db51849ae9bd 100644
> >>--- a/sound/soc/soc-pcm.c
> >>+++ b/sound/soc/soc-pcm.c
> >>@@ -865,8 +865,20 @@ int soc_dai_hw_params(struct snd_pcm_substream *substream,
> >>  		      struct snd_pcm_hw_params *params,
> >>  		      struct snd_soc_dai *dai)
> >>  {
> >>+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >>  	int ret;
> >>+	/* perform any topology hw_params fixups before DAI  */
> >>+	if (rtd->dai_link->be_hw_params_fixup) {
> >>+		ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
> >>+		if (ret < 0) {
> >>+			dev_err(rtd->dev,
> >>+				"ASoC: hw_params topology fixup failed %d\n",
> >>+				ret);
> >>+			return ret;
> >>+		}
> >>+	}
> >>+
> >>  	if (dai->driver->ops->hw_params) {
> >>  		ret = dai->driver->ops->hw_params(substream, params, dai);
> >>  		if (ret < 0) {
> >_______________________________________________
> >Alsa-devel mailing list
> >Alsa-devel@alsa-project.org
> >https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >