On Mon, 11 Feb 2019 16:41:31 +0100, Pierre-Louis Bossart wrote:
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
Thanks, now I took a look at the code. And, this surfaced that the another part of the problem is that DPCM does the substream open handling by itself in soc-pcm.c. Oh well. I'm afraid that we have some hidden bugs there that may lead to a crash easily. (Fortunately (or unfortunately) fuzzer isn't performed on ASoC because we have no virtual device driver :)
IMO, some of DPCM code should be raised to the upper level, to ALSA PCM core. The current code is still in a rough form of early plumbing.
In anyway, I merged the patch now with a bit more comments.
Thanks!
Takashi