[PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

[PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

[Stephan Gerhold]

At the moment, PCM devices for DPCM are only created based on the
dpcm_playback/capture parameters of the DAI link, without considering
if the CPU/FE DAI is actually capable of playback/capture.

Normally the dpcm_playback/capture parameter should match the
capabilities of the CPU DAI. However, there is no way to set that
parameter from the device tree (e.g. with simple-audio-card or
qcom sound cards). dpcm_playback/capture are always both set to 1.

This causes problems when the CPU DAI does only support playback
or capture. Attemting to open that PCM device with an unsupported
stream type then results in a null pointer dereference:

    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000128
    Internal error: Oops: 96000044 [#1] PREEMPT SMP
    CPU: 3 PID: 1582 Comm: arecord Not tainted 5.7.0-rc1
    pc : invalidate_paths_ep+0x30/0xe0
    lr : snd_soc_dapm_dai_get_connected_widgets+0x170/0x1a8
    Call trace:
     invalidate_paths_ep+0x30/0xe0
     snd_soc_dapm_dai_get_connected_widgets+0x170/0x1a8
     dpcm_path_get+0x38/0xd0
     dpcm_fe_dai_open+0x70/0x920
     snd_pcm_open_substream+0x564/0x840
     snd_pcm_open+0xfc/0x228
     snd_pcm_capture_open+0x4c/0x78
     snd_open+0xac/0x1a8
     ...

... because the DAI playback/capture_widget is not set in that case.

We could add checks there to fix the problem (maybe we should
anyway), but much easier is to not expose the device as
playback/capture in the first place. Attemting to use that
device would always fail later anyway.

Add checks for snd_soc_dai_stream_valid() to the DPCM case
to avoid exposing playback/capture if it is not supported.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
This can be reproduced with q6asm-dai with a device tree
configuration like:

&q6asmdai {
	dai@0 {
		reg = <0>;
		direction = <Q6ASM_DAI_RX>;
	};
};

Then a simple "arecord output.wav" will result in the crash
above, since it attempts to use that FE DAI for capture
(even though it is limited to playback).
---
 sound/soc/soc-pcm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 454735f8fa92..77a680da366f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2911,8 +2911,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	int i;
 
 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
-		playback = rtd->dai_link->dpcm_playback;
-		capture = rtd->dai_link->dpcm_capture;
+		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+		if (rtd->num_cpus > 1) {
+			dev_err(rtd->dev,
+				"DPCM doesn't support Multi CPU yet\n");
+			return -EINVAL;
+		}
+
+		playback = rtd->dai_link->dpcm_playback &&
+			   snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
+		capture = rtd->dai_link->dpcm_capture &&
+			  snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);
 	} else {
 		/* Adapt stream for codec2codec links */
 		int cpu_capture = rtd->dai_link->params ?
-- 
2.26.1

Applied "ASoC: soc-pcm: dpcm: Only allow playback/capture if supported" to the asoc tree

[Mark Brown]

The patch

   ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 9b5db059366ae2087e07892b5fc108f81f4ec189 Mon Sep 17 00:00:00 2001
From: Stephan Gerhold <stephan@gerhold.net>
Date: Wed, 15 Apr 2020 12:49:28 +0200
Subject: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

At the moment, PCM devices for DPCM are only created based on the
dpcm_playback/capture parameters of the DAI link, without considering
if the CPU/FE DAI is actually capable of playback/capture.

Normally the dpcm_playback/capture parameter should match the
capabilities of the CPU DAI. However, there is no way to set that
parameter from the device tree (e.g. with simple-audio-card or
qcom sound cards). dpcm_playback/capture are always both set to 1.

This causes problems when the CPU DAI does only support playback
or capture. Attemting to open that PCM device with an unsupported
stream type then results in a null pointer dereference:

    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000128
    Internal error: Oops: 96000044 [#1] PREEMPT SMP
    CPU: 3 PID: 1582 Comm: arecord Not tainted 5.7.0-rc1
    pc : invalidate_paths_ep+0x30/0xe0
    lr : snd_soc_dapm_dai_get_connected_widgets+0x170/0x1a8
    Call trace:
     invalidate_paths_ep+0x30/0xe0
     snd_soc_dapm_dai_get_connected_widgets+0x170/0x1a8
     dpcm_path_get+0x38/0xd0
     dpcm_fe_dai_open+0x70/0x920
     snd_pcm_open_substream+0x564/0x840
     snd_pcm_open+0xfc/0x228
     snd_pcm_capture_open+0x4c/0x78
     snd_open+0xac/0x1a8
     ...

... because the DAI playback/capture_widget is not set in that case.

We could add checks there to fix the problem (maybe we should
anyway), but much easier is to not expose the device as
playback/capture in the first place. Attemting to use that
device would always fail later anyway.

Add checks for snd_soc_dai_stream_valid() to the DPCM case
to avoid exposing playback/capture if it is not supported.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Link: https://lore.kernel.org/r/20200415104928.86091-1-stephan@gerhold.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-pcm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 289aebc15529..1f302de44052 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2911,8 +2911,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	int i;
 
 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
-		playback = rtd->dai_link->dpcm_playback;
-		capture = rtd->dai_link->dpcm_capture;
+		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+		if (rtd->num_cpus > 1) {
+			dev_err(rtd->dev,
+				"DPCM doesn't support Multi CPU yet\n");
+			return -EINVAL;
+		}
+
+		playback = rtd->dai_link->dpcm_playback &&
+			   snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
+		capture = rtd->dai_link->dpcm_capture &&
+			  snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);
 	} else {
 		/* Adapt stream for codec2codec links */
 		int cpu_capture = rtd->dai_link->params ?
-- 
2.20.1

Re: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

[Pierre-Louis Bossart]

> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 454735f8fa92..77a680da366f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2911,8 +2911,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   	int i;
>   
>   	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> -		playback = rtd->dai_link->dpcm_playback;
> -		capture = rtd->dai_link->dpcm_capture;
> +		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +		if (rtd->num_cpus > 1) {
> +			dev_err(rtd->dev,
> +				"DPCM doesn't support Multi CPU yet\n");
> +			return -EINVAL;
> +		}
> +
> +		playback = rtd->dai_link->dpcm_playback &&
> +			   snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
> +		capture = rtd->dai_link->dpcm_capture &&
> +			  snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);

This commit introduces major regressions with SOF on CherryTrail and 
Broadwell:

     [   25.705750]  SSP2-Codec: ASoC: no backend playback stream
     [   27.923378]  SSP2-Codec: ASoC: no users playback at close - state

it's likely due to the check for min_channels > 0 in 
snd_soc_dai_stream_valid(), which wasn't a requirement before.

We are testing a fix [1] but other users of DPCM might be impacted.

Mark, this commit is on your for-5.7 branch but not on for-next? Not 
sure which SHA1 to use for the Fixes: tag

[1] 
https://github.com/thesofproject/linux/pull/2018/commits/4fa10638dca8aad7a320e85cc3e00b179b8de410

Re: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

[Stephan Gerhold]

On Thu, Apr 16, 2020 at 03:51:23PM -0500, Pierre-Louis Bossart wrote:
> 
> > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> > index 454735f8fa92..77a680da366f 100644
> > --- a/sound/soc/soc-pcm.c
> > +++ b/sound/soc/soc-pcm.c
> > @@ -2911,8 +2911,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >   	int i;
> >   	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > -		playback = rtd->dai_link->dpcm_playback;
> > -		capture = rtd->dai_link->dpcm_capture;
> > +		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +		if (rtd->num_cpus > 1) {
> > +			dev_err(rtd->dev,
> > +				"DPCM doesn't support Multi CPU yet\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		playback = rtd->dai_link->dpcm_playback &&
> > +			   snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
> > +		capture = rtd->dai_link->dpcm_capture &&
> > +			  snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);
> 
> This commit introduces major regressions with SOF on CherryTrail and
> Broadwell:
> 
>     [   25.705750]  SSP2-Codec: ASoC: no backend playback stream
>     [   27.923378]  SSP2-Codec: ASoC: no users playback at close - state
> 
> it's likely due to the check for min_channels > 0 in
> snd_soc_dai_stream_valid(), which wasn't a requirement before.
> 
> We are testing a fix [1] but other users of DPCM might be impacted.
> 

Indeed, I actually ran into a similar problem myself for q6afe-dai:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=for-5.7&id=0c824ec094b5cda766c80d88c2036e28c24a4cb1

As mentioned in that commit message it was already broken on 5.7-rc1
for me, because of commit 0e9cf4c452ad ("ASoC: pcm: check if cpu-dai
supports a given stream"). [2]

With this commit it's more visible at least, you get a proper error
instead of silently not calling hw_params() for example. :)

[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e9cf4c452ad7e2776441cbac0b9983abaf17ff0

> Mark, this commit is on your for-5.7 branch but not on for-next? Not sure
> which SHA1 to use for the Fixes: tag
> 
> [1] https://github.com/thesofproject/linux/pull/2018/commits/4fa10638dca8aad7a320e85cc3e00b179b8de410

Re: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

On Thu, Apr 16, 2020 at 03:51:23PM -0500, Pierre-Louis Bossart wrote:

> Mark, this commit is on your for-5.7 branch but not on for-next? Not sure
> which SHA1 to use for the Fixes: tag

The commit you're fixing should be the commit you're fixing regardless
of which branch it is on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]