On 2/9/19 3:27 AM, Takashi Iwai wrote:
On Sat, 09 Feb 2019 00:29:53 +0100, Pierre-Louis Bossart wrote:
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com
BE dai links only have internal PCM's and their substream ops may not be set. Suspending these PCM's will result in their ops->trigger() being invoked and cause a kernel oops. So skip suspending PCM's if their ops are NULL.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/core/pcm_native.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..b6e158ce6650 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1506,6 +1506,14 @@ int snd_pcm_suspend_all(struct snd_pcm *pcm) /* FIXME: the open/close code should lock this as well */ if (substream->runtime == NULL) continue;
/** Skip BE dai link PCM's that are internal and may* not have their substream ops set.*/if (!substream->ops)continue;err = snd_pcm_suspend(substream); if (err < 0 && err != -EBUSY) return err;Basically it's OK and safe to apply this check. We may need to add such sanity checks in more places if this really hits.
But I still wonder how this can go through. Is substream->runtime set even if substream->ops is NULL? The substream->runtime is assigned dynamically at opening a substream via snd_pcm_attach_substream(), so without opening it, it must be NULL.
This error case was exposed when we tried to get rid of snd_pcm_suspend() per your recommendation, and use snd_soc_suspend() instead to do the work for us.
In the case of back-ends, all initializations are bypassed in soc_new_pcm() - see below a code snippet - and the ops aren't set before suspend is called.
The complete thread where we discussed this is at https://github.com/thesofproject/linux/pull/582
if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; //<<<< this bypasses all the ops initializations!! }
/* ASoC PCM operations */ if (rtd->dai_link->dynamic) { rtd->ops.open = dpcm_fe_dai_open; rtd->ops.hw_params = dpcm_fe_dai_hw_params; rtd->ops.prepare = dpcm_fe_dai_prepare; rtd->ops.trigger = dpcm_fe_dai_trigger; rtd->ops.hw_free = dpcm_fe_dai_hw_free; rtd->ops.close = dpcm_fe_dai_close; rtd->ops.pointer = soc_pcm_pointer; rtd->ops.ioctl = soc_pcm_ioctl; } else { rtd->ops.open = soc_pcm_open; rtd->ops.hw_params = soc_pcm_hw_params; rtd->ops.prepare = soc_pcm_prepare; rtd->ops.trigger = soc_pcm_trigger; rtd->ops.hw_free = soc_pcm_hw_free; rtd->ops.close = soc_pcm_close; rtd->ops.pointer = soc_pcm_pointer; rtd->ops.ioctl = soc_pcm_ioctl; }
for_each_rtdcom(rtd, rtdcom) { const struct snd_pcm_ops *ops = rtdcom->component->driver->ops;
if (!ops) continue;
if (ops->ack) rtd->ops.ack = soc_rtdcom_ack; if (ops->copy_user) rtd->ops.copy_user = soc_rtdcom_copy_user; if (ops->copy_kernel) rtd->ops.copy_kernel = soc_rtdcom_copy_kernel; if (ops->fill_silence) rtd->ops.fill_silence = soc_rtdcom_fill_silence; if (ops->page) rtd->ops.page = soc_rtdcom_page; if (ops->mmap) rtd->ops.mmap = soc_rtdcom_mmap; }
if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);
if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);
for_each_rtdcom(rtd, rtdcom) { component = rtdcom->component;
if (!component->driver->pcm_new) continue;
ret = component->driver->pcm_new(rtd); if (ret < 0) { dev_err(component->dev, "ASoC: pcm constructor failed: %d\n", ret); return ret; } }
pcm->private_free = soc_pcm_private_free; out: dev_info(rtd->card->dev, "%s <-> %s mapping ok\n", (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name, cpu_dai->name); return ret;