Re: [PATCH] ALSA: PCM: check if ops are defined before suspending PCM

[Takashi Iwai <tiwai@suse.de>]

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.


thanks,

Takashi