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@xxxxxxxxxxxxxxx>
> >>---
> >>
> >>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@xxxxxxxxxxxxxxxx
> >https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
