[PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

Not all platform drivers have pcm_{new,free} callbacks. Seen with a
"snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.

Resolves an OOPS seen on v4.11-rc1 with Google Kevin (Samsung Chromebook
Plus):

[    2.863304] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
[    2.886930] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok
[    2.912496] rk3399-gru-sound sound: da7219-hifi <-> ff880000.i2s mapping ok
[    2.936157] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    2.945221] pgd = ffffff8008f65000
[    2.949040] [00000000] *pgd=00000000f7dfe003, *pud=00000000f7dfe003, *pmd=0000000000000000
[    2.958307] Internal error: Oops: 86000005 [#1] PREEMPT SMP
[    2.964532] Modules linked in:
[    2.967945] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       4.11.0-rc1+ #941
[    2.976597] Hardware name: Google Kevin (DT)
[    2.981364] task: ffffffc0f1cd0000 task.stack: ffffffc0f1ccc000
[    2.987978] PC is at 0x0
[    2.990806] LR is at snd_soc_platform_drv_pcm_new+0x74/0x84
[    2.997030] pc : [<0000000000000000>] lr : [<ffffff80086298f4>] pstate: 60000145
[    3.005293] sp : ffffffc0f1ccf9a0
...
[    3.657268] [<          (null)>]           (null)
[    3.662523] [<ffffff8008638a48>] soc_new_pcm+0x3b8/0x458
[    3.668457] [<ffffff800862c1f4>] snd_soc_register_card+0x900/0xd40
[    3.675361] [<ffffff8008639288>] devm_snd_soc_register_card+0x5c/0x98
[    3.682559] [<ffffff8008645a7c>] rockchip_sound_probe+0x1b0/0x1f4
[    3.689369] [<ffffff800846c55c>] platform_drv_probe+0x60/0xac
[    3.695789] [<ffffff800846a2cc>] driver_probe_device+0x14c/0x2d0
[    3.702499] [<ffffff800846a4d4>] __driver_attach+0x84/0xb0
[    3.708628] [<ffffff8008469158>] bus_for_each_dev+0x9c/0xcc
[    3.714853] [<ffffff8008469ca0>] driver_attach+0x2c/0x34
[    3.720786] [<ffffff8008469800>] bus_add_driver+0xf0/0x1f4
[    3.726914] [<ffffff800846b230>] driver_register+0x9c/0xe8
[    3.733042] [<ffffff800846c4a0>] __platform_driver_register+0x60/0x6c
[    3.740241] [<ffffff8008c3a720>] rockchip_sound_driver_init+0x18/0x20
[    3.747438] [<ffffff8008083204>] do_one_initcall+0xa0/0x138
[    3.753666] [<ffffff8008c00d70>] kernel_init_freeable+0x1ac/0x264
[    3.760474] [<ffffff8008783914>] kernel_init+0x18/0x100
[    3.766310] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
[    3.772246] Code: bad PC value
[    3.775704] ---[ end trace f68728a0d3053b53 ]---
[    3.787286] Kernel panic - not syncing: Fatal exception
[    3.793126] SMP: stopping secondary CPUs
[    3.797508] Kernel Offset: disabled
[    3.801400] Memory Limit: none

Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I'm really not that familiar with this subsystem... but this does fix the
crash seen here.

 sound/soc/soc-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6dca408faae3..2722bb0c5573 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_platform *platform = rtd->platform;
 
-	return platform->driver->pcm_new(rtd);
+	if (platform->driver->pcm_new)
+		return platform->driver->pcm_new(rtd);
+	else
+		return 0;
 }
 
 static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
@@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
 	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
 
-	platform->driver->pcm_free(pcm);
+	if (platform->driver->pcm_free)
+		platform->driver->pcm_free(pcm);
 }
 
 /**
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

Not all platform drivers have pcm_{new,free} callbacks. Seen with a
"snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.

Resolves an OOPS seen on v4.11-rc1 with Google Kevin (Samsung Chromebook
Plus):

[    2.863304] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
[    2.886930] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok
[    2.912496] rk3399-gru-sound sound: da7219-hifi <-> ff880000.i2s mapping ok
[    2.936157] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    2.945221] pgd = ffffff8008f65000
[    2.949040] [00000000] *pgd=00000000f7dfe003, *pud=00000000f7dfe003, *pmd=0000000000000000
[    2.958307] Internal error: Oops: 86000005 [#1] PREEMPT SMP
[    2.964532] Modules linked in:
[    2.967945] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       4.11.0-rc1+ #941
[    2.976597] Hardware name: Google Kevin (DT)
[    2.981364] task: ffffffc0f1cd0000 task.stack: ffffffc0f1ccc000
[    2.987978] PC is at 0x0
[    2.990806] LR is at snd_soc_platform_drv_pcm_new+0x74/0x84
[    2.997030] pc : [<0000000000000000>] lr : [<ffffff80086298f4>] pstate: 60000145
[    3.005293] sp : ffffffc0f1ccf9a0
...
[    3.657268] [<          (null)>]           (null)
[    3.662523] [<ffffff8008638a48>] soc_new_pcm+0x3b8/0x458
[    3.668457] [<ffffff800862c1f4>] snd_soc_register_card+0x900/0xd40
[    3.675361] [<ffffff8008639288>] devm_snd_soc_register_card+0x5c/0x98
[    3.682559] [<ffffff8008645a7c>] rockchip_sound_probe+0x1b0/0x1f4
[    3.689369] [<ffffff800846c55c>] platform_drv_probe+0x60/0xac
[    3.695789] [<ffffff800846a2cc>] driver_probe_device+0x14c/0x2d0
[    3.702499] [<ffffff800846a4d4>] __driver_attach+0x84/0xb0
[    3.708628] [<ffffff8008469158>] bus_for_each_dev+0x9c/0xcc
[    3.714853] [<ffffff8008469ca0>] driver_attach+0x2c/0x34
[    3.720786] [<ffffff8008469800>] bus_add_driver+0xf0/0x1f4
[    3.726914] [<ffffff800846b230>] driver_register+0x9c/0xe8
[    3.733042] [<ffffff800846c4a0>] __platform_driver_register+0x60/0x6c
[    3.740241] [<ffffff8008c3a720>] rockchip_sound_driver_init+0x18/0x20
[    3.747438] [<ffffff8008083204>] do_one_initcall+0xa0/0x138
[    3.753666] [<ffffff8008c00d70>] kernel_init_freeable+0x1ac/0x264
[    3.760474] [<ffffff8008783914>] kernel_init+0x18/0x100
[    3.766310] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
[    3.772246] Code: bad PC value
[    3.775704] ---[ end trace f68728a0d3053b53 ]---
[    3.787286] Kernel panic - not syncing: Fatal exception
[    3.793126] SMP: stopping secondary CPUs
[    3.797508] Kernel Offset: disabled
[    3.801400] Memory Limit: none

Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I'm really not that familiar with this subsystem... but this does fix the
crash seen here.

 sound/soc/soc-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6dca408faae3..2722bb0c5573 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_platform *platform = rtd->platform;
 
-	return platform->driver->pcm_new(rtd);
+	if (platform->driver->pcm_new)
+		return platform->driver->pcm_new(rtd);
+	else
+		return 0;
 }
 
 static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
@@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
 	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
 
-	platform->driver->pcm_free(pcm);
+	if (platform->driver->pcm_free)
+		platform->driver->pcm_free(pcm);
 }
 
 /**
-- 
2.12.0.246.ga2ecc84866-goog

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Kuninori Morimoto]

Hi Brian

Thank you for your patch

> Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
(snip)
> Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> I'm really not that familiar with this subsystem... but this does fix the
> crash seen here.
> 
>  sound/soc/soc-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 6dca408faae3..2722bb0c5573 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
>  {
>  	struct snd_soc_platform *platform = rtd->platform;
>  
> -	return platform->driver->pcm_new(rtd);
> +	if (platform->driver->pcm_new)
> +		return platform->driver->pcm_new(rtd);
> +	else
> +		return 0;
>  }
>  
>  static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> @@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
>  	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
>  
> -	platform->driver->pcm_free(pcm);
> +	if (platform->driver->pcm_free)
> +		platform->driver->pcm_free(pcm);
>  }

It is a littlle bit strange for me.
commit 99b04f4c4051 has below code. This means, if platform doesn't have pcm_new/free callback,
component doesn't have snd_soc_platform_drv_pcm_new/free.
But your case, platform doesn't have pcm_new/free, but component had it ?

...
@@ -3181,6 +3198,10 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
 		platform->component.probe = snd_soc_platform_drv_probe;
 	if (platform_drv->remove)
 		platform->component.remove = snd_soc_platform_drv_remove;
+	if (platform_drv->pcm_new)
+		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
+	if (platform_drv->pcm_free)
+		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
 
 #ifdef CONFIG_DEBUG_FS
 	platform->component.debugfs_prefix = "platform";

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Kuninori Morimoto]

Hi Brian

Thank you for your patch

> Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
(snip)
> Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> I'm really not that familiar with this subsystem... but this does fix the
> crash seen here.
> 
>  sound/soc/soc-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 6dca408faae3..2722bb0c5573 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
>  {
>  	struct snd_soc_platform *platform = rtd->platform;
>  
> -	return platform->driver->pcm_new(rtd);
> +	if (platform->driver->pcm_new)
> +		return platform->driver->pcm_new(rtd);
> +	else
> +		return 0;
>  }
>  
>  static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> @@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
>  	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
>  
> -	platform->driver->pcm_free(pcm);
> +	if (platform->driver->pcm_free)
> +		platform->driver->pcm_free(pcm);
>  }

It is a littlle bit strange for me.
commit 99b04f4c4051 has below code. This means, if platform doesn't have pcm_new/free callback,
component doesn't have snd_soc_platform_drv_pcm_new/free.
But your case, platform doesn't have pcm_new/free, but component had it ?

...
@@ -3181,6 +3198,10 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
 		platform->component.probe = snd_soc_platform_drv_probe;
 	if (platform_drv->remove)
 		platform->component.remove = snd_soc_platform_drv_remove;
+	if (platform_drv->pcm_new)
+		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
+	if (platform_drv->pcm_free)
+		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
 
 #ifdef CONFIG_DEBUG_FS
 	platform->component.debugfs_prefix = "platform";

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

Hi Kuninori,

On Thu, Mar 09, 2017 at 12:17:41AM +0000, Kuninori Morimoto wrote:
> > Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> > "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
> (snip)
> > Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > I'm really not that familiar with this subsystem... but this does fix the
> > crash seen here.
> > 
> >  sound/soc/soc-core.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index 6dca408faae3..2722bb0c5573 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
> >  {
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  
> > -	return platform->driver->pcm_new(rtd);
> > +	if (platform->driver->pcm_new)
> > +		return platform->driver->pcm_new(rtd);
> > +	else
> > +		return 0;
> >  }
> >  
> >  static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> > @@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> >  	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  
> > -	platform->driver->pcm_free(pcm);
> > +	if (platform->driver->pcm_free)
> > +		platform->driver->pcm_free(pcm);
> >  }
> 
> It is a littlle bit strange for me.

Yes, and honestly I'm a little confused by the inheritance in this
framework.

> commit 99b04f4c4051 has below code. This means, if platform doesn't have pcm_new/free callback,
> component doesn't have snd_soc_platform_drv_pcm_new/free.
> But your case, platform doesn't have pcm_new/free, but component had it ?

I have a feeling you're checking the wrong thing below for this case.
All I know is that I'm definitely hitting a NULL
platform->driver->pcm_new callback, and that either reverting your patch
or applying the patch I just sent fixes it.

Brian

> ...
> @@ -3181,6 +3198,10 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
>  		platform->component.probe = snd_soc_platform_drv_probe;
>  	if (platform_drv->remove)
>  		platform->component.remove = snd_soc_platform_drv_remove;
> +	if (platform_drv->pcm_new)
> +		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
> +	if (platform_drv->pcm_free)
> +		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
>  
>  #ifdef CONFIG_DEBUG_FS
>  	platform->component.debugfs_prefix = "platform";

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Kuninori Morimoto]

Hi Brian

> > It is a littlle bit strange for me.
> 
> Yes, and honestly I'm a little confused by the inheritance in this
> framework.

Yes, I agree :)
This is 1st prepare for future ALSA SoC framework cleanup
It is Lars-Peter's idea

> I have a feeling you're checking the wrong thing below for this case.

If so, we should fix this "wrong thing" ?

> All I know is that I'm definitely hitting a NULL
> platform->driver->pcm_new callback, and that either reverting your patch
> or applying the patch I just sent fixes it.

I want to know why this happen.
Can you show me which driver is calling snd_soc_add_platform() in your case ?

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Kuninori Morimoto]

Hi Brian

> > It is a littlle bit strange for me.
> 
> Yes, and honestly I'm a little confused by the inheritance in this
> framework.

Yes, I agree :)
This is 1st prepare for future ALSA SoC framework cleanup
It is Lars-Peter's idea

> I have a feeling you're checking the wrong thing below for this case.

If so, we should fix this "wrong thing" ?

> All I know is that I'm definitely hitting a NULL
> platform->driver->pcm_new callback, and that either reverting your patch
> or applying the patch I just sent fixes it.

I want to know why this happen.
Can you show me which driver is calling snd_soc_add_platform() in your case ?

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Mark Brown]

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

On Wed, Mar 08, 2017 at 03:18:54PM -0800, Brian Norris wrote:
> Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
> 
> Resolves an OOPS seen on v4.11-rc1 with Google Kevin (Samsung Chromebook
> Plus):
> 
> [    2.863304] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
> [    2.886930] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

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

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 786 bytes --]

On Wed, Mar 08, 2017 at 03:18:54PM -0800, Brian Norris wrote:
> Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
> 
> Resolves an OOPS seen on v4.11-rc1 with Google Kevin (Samsung Chromebook
> Plus):
> 
> [    2.863304] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
> [    2.886930] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

On Thu, Mar 09, 2017 at 12:09:18PM +0100, Mark Brown wrote:
> On Wed, Mar 08, 2017 at 03:18:54PM -0800, Brian Norris wrote:
> > Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> > "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
> > 
> > Resolves an OOPS seen on v4.11-rc1 with Google Kevin (Samsung Chromebook
> > Plus):
> > 
> > [    2.863304] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
> > [    2.886930] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok
> 
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative then it's
> usually better to pull out the relevant sections.

I did think, and I did trim the trace significantly. I believe (or
believed; maybe incorrectly) that much of the remaining context *was*
useful, and that it could have answered some of Kuninori's questions, if
he chose to dig himself. As admitted in the commit message, I don't
really understand all of the relationships here, so it's hard to
highlight "only the relevant sections".

I'm sorry that my post did not meet your standards though. Next time
I'll just post a revert with little context and no attempt to understand
what actually went wrong.

Regards,
Brian

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

Hi Kuninori,

On Thu, Mar 09, 2017 at 12:53:50AM +0000, Kuninori Morimoto wrote:
> > All I know is that I'm definitely hitting a NULL
> > platform->driver->pcm_new callback, and that either reverting your patch
> > or applying the patch I just sent fixes it.
> 
> I want to know why this happen.
> Can you show me which driver is calling snd_soc_add_platform() in your case ?

There are 4 drivers calling that:

  snd_soc_dummy_probe
  rt5514_spi_probe
  2 instances of snd_dmaengine_pcm_register, via rockchip_i2s_probe

Only the latter two seem to run the assignment here:

	if (platform_drv->pcm_new)
		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;

Both snd_soc_dummy_probe and rt5514_spi_probe find ->pcm_new NULL here.

I'm running with the sound/soc/rockchip/rk3399_gru_sound.c platform
driver. If it helps, I'm using the DT posted at [1]. IIUC, the "RT5514
DSP" link is trying to link up a "snd-soc-dummy" instance.

I'm fairly thoroughly confused. Any better ideas?

Brian

[1] https://www.spinics.net/lists/arm-kernel/msg561964.html
    [PATCH v2 4/6] arm64: dts: rockchip: add Gru/Kevin DTS

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Kuninori Morimoto]

Hi Brian

Thank you for your feedback

> There are 4 drivers calling that:
> 
>   snd_soc_dummy_probe
>   rt5514_spi_probe
>   2 instances of snd_dmaengine_pcm_register, via rockchip_i2s_probe
> 
> Only the latter two seem to run the assignment here:
> 
> 	if (platform_drv->pcm_new)
> 		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
> 
> Both snd_soc_dummy_probe and rt5514_spi_probe find ->pcm_new NULL here.

Hmm...

The crasher was snd_dmaengine_pcm_register's platform ?
This means, in your current kernel, dmaengine platform dosn't call
its .pcm_new (= dmaengine_pcm_new) somehow ?

I'm wondering why ->pcm_new became NULL which exists on probe timing ?
Can you check component and driver by this patch ?
This is very rough but enough for debug

---------------------
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 5933851..43da1ec 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3322,6 +3322,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_platform *platform = rtd->platform;
 
+	printk("-------use name: %s, %p\n",
+	       platform->component.name,
+	       platform->driver);
+
 	return platform->driver->pcm_new(rtd);
 }
 
@@ -3356,8 +3360,12 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
 		platform->component.probe = snd_soc_platform_drv_probe;
 	if (platform_drv->remove)
 		platform->component.remove = snd_soc_platform_drv_remove;
-	if (platform_drv->pcm_new)
+	if (platform_drv->pcm_new) {
+		printk("-------add name: %s, %p\n",
+		       platform->component.name,
+		       platform->driver);
 		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
+	}
 	if (platform_drv->pcm_free)
 		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
 
---------------------

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Kuninori Morimoto]

Hi Brian

Thank you for your feedback

> There are 4 drivers calling that:
> 
>   snd_soc_dummy_probe
>   rt5514_spi_probe
>   2 instances of snd_dmaengine_pcm_register, via rockchip_i2s_probe
> 
> Only the latter two seem to run the assignment here:
> 
> 	if (platform_drv->pcm_new)
> 		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
> 
> Both snd_soc_dummy_probe and rt5514_spi_probe find ->pcm_new NULL here.

Hmm...

The crasher was snd_dmaengine_pcm_register's platform ?
This means, in your current kernel, dmaengine platform dosn't call
its .pcm_new (= dmaengine_pcm_new) somehow ?

I'm wondering why ->pcm_new became NULL which exists on probe timing ?
Can you check component and driver by this patch ?
This is very rough but enough for debug

---------------------
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 5933851..43da1ec 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3322,6 +3322,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_platform *platform = rtd->platform;
 
+	printk("-------use name: %s, %p\n",
+	       platform->component.name,
+	       platform->driver);
+
 	return platform->driver->pcm_new(rtd);
 }
 
@@ -3356,8 +3360,12 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
 		platform->component.probe = snd_soc_platform_drv_probe;
 	if (platform_drv->remove)
 		platform->component.remove = snd_soc_platform_drv_remove;
-	if (platform_drv->pcm_new)
+	if (platform_drv->pcm_new) {
+		printk("-------add name: %s, %p\n",
+		       platform->component.name,
+		       platform->driver);
 		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
+	}
 	if (platform_drv->pcm_free)
 		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
 
---------------------

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Mark Brown]

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

On Fri, Mar 10, 2017 at 04:24:07PM -0800, Brian Norris wrote:
> On Thu, Mar 09, 2017 at 12:09:18PM +0100, Mark Brown wrote:

> > Please think hard before including complete backtraces in upstream
> > reports, they are very large and contain almost no useful information
> > relative to their size so often obscure the relevant content in your
> > message. If part of the backtrace is usefully illustrative then it's
> > usually better to pull out the relevant sections.

> I did think, and I did trim the trace significantly. I believe (or
> believed; maybe incorrectly) that much of the remaining context *was*
> useful, and that it could have answered some of Kuninori's questions, if
> he chose to dig himself. As admitted in the commit message, I don't
> really understand all of the relationships here, so it's hard to
> highlight "only the relevant sections".

Any editing you'd done is really not obvious looking at the trace (the
inclusion of the top of the trace with the pgd and so on which really
need the image to be useful is a bit of a flag here, as is the bit at
the end after the end of the trace).  It's certainly not mentioned or
referenced.  At the very least for probe errors it's generally safe to
cut down to just the driver probe function is being called rather than
showing the entire device model call stack, it's vanishingly rare for
that to be adding anything and it's generally several times deeper than
what the driver is doing.

> I'm sorry that my post did not meet your standards though. Next time
> I'll just post a revert with little context and no attempt to understand
> what actually went wrong.

Providing changes with little context and no conveyed understanding of
exactly what went wrong is a very big part of the reason for complaining
about people just splatting backtraces in and certainly not what I was
asking for, it's sadly common to see a backtrace provided in lieu of any
analysis and it does make the whole thing harder to read.  That would be
even more problematic than what you sent, I wouldn't have applied such a
change at all unless I somehow reverse engineered the analysis.

You do have a reasonable analysis of your change in the first couple of
lines ("Not all platform drivers have pcm_{new,free} callbacks...")
which is why I applied it, it's at least not going to do any harm to
check that there's a function being provided.

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

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2388 bytes --]

On Fri, Mar 10, 2017 at 04:24:07PM -0800, Brian Norris wrote:
> On Thu, Mar 09, 2017 at 12:09:18PM +0100, Mark Brown wrote:

> > Please think hard before including complete backtraces in upstream
> > reports, they are very large and contain almost no useful information
> > relative to their size so often obscure the relevant content in your
> > message. If part of the backtrace is usefully illustrative then it's
> > usually better to pull out the relevant sections.

> I did think, and I did trim the trace significantly. I believe (or
> believed; maybe incorrectly) that much of the remaining context *was*
> useful, and that it could have answered some of Kuninori's questions, if
> he chose to dig himself. As admitted in the commit message, I don't
> really understand all of the relationships here, so it's hard to
> highlight "only the relevant sections".

Any editing you'd done is really not obvious looking at the trace (the
inclusion of the top of the trace with the pgd and so on which really
need the image to be useful is a bit of a flag here, as is the bit at
the end after the end of the trace).  It's certainly not mentioned or
referenced.  At the very least for probe errors it's generally safe to
cut down to just the driver probe function is being called rather than
showing the entire device model call stack, it's vanishingly rare for
that to be adding anything and it's generally several times deeper than
what the driver is doing.

> I'm sorry that my post did not meet your standards though. Next time
> I'll just post a revert with little context and no attempt to understand
> what actually went wrong.

Providing changes with little context and no conveyed understanding of
exactly what went wrong is a very big part of the reason for complaining
about people just splatting backtraces in and certainly not what I was
asking for, it's sadly common to see a backtrace provided in lieu of any
analysis and it does make the whole thing harder to read.  That would be
even more problematic than what you sent, I wouldn't have applied such a
change at all unless I somehow reverse engineered the analysis.

You do have a reasonable analysis of your change in the first couple of
lines ("Not all platform drivers have pcm_{new,free} callbacks...")
which is why I applied it, it's at least not going to do any harm to
check that there's a function being provided.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

On Mon, Mar 13, 2017 at 03:46:00AM +0000, Kuninori Morimoto wrote:
> > There are 4 drivers calling that:
> > 
> >   snd_soc_dummy_probe
> >   rt5514_spi_probe
> >   2 instances of snd_dmaengine_pcm_register, via rockchip_i2s_probe
> > 
> > Only the latter two seem to run the assignment here:
> > 
> > 	if (platform_drv->pcm_new)
> > 		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
> > 
> > Both snd_soc_dummy_probe and rt5514_spi_probe find ->pcm_new NULL here.
> 
> Hmm...
> 
> The crasher was snd_dmaengine_pcm_register's platform ?

No, actually it wasn't. It was spi2.0, which was a dummy, from
snd_soc_dummy_probe(). But somehow snd_soc_platform_drv_pcm_new() got
called for it...

> This means, in your current kernel, dmaengine platform dosn't call
> its .pcm_new (= dmaengine_pcm_new) somehow ?

I believe not. I'm still thoroughly confused though :)

> I'm wondering why ->pcm_new became NULL which exists on probe timing ?
> Can you check component and driver by this patch ?
> This is very rough but enough for debug

I added this (along with a bunch of debugging, including a form of my
current patch, to avoid still crashing on the NULL pointer). Trimmed
log (with some of the framework's dev_dbg() enabled):

[    2.521638] snd-soc-dummy snd-soc-dummy: codec register snd-soc-dummy
[    2.523532] da7219 8-001a: codec register 8-001a
[    2.523850] max98357a max98357a: codec register max98357a
[    2.530256] rt5514 1-0057: codec register 1-0057
[    2.531615] -------add name: ff880000.i2s, ffffff800888a598
[    2.531976] -------add name: ff8a0000.i2s, ffffff800888a598
[    2.532706] rk3399-gru-sound sound: ASoC: binding MAX98357A
[    2.532721] rk3399-gru-sound sound: ASoC: binding RT5514
[    2.532736] rk3399-gru-sound sound: ASoC: binding DA7219
[    2.532745] rk3399-gru-sound sound: ASoC: binding RT5514 DSP
[    2.537327] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 0 late -2
[    2.537332] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 1 late -2
[    2.537336] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 2 late -2
[    2.537340] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 3 late -2
[    2.537344] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 0 late -1
[    2.537347] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 1 late -1
[    2.537351] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 2 late -1
[    2.537354] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 3 late -1
[    2.537358] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 0 late 0
[    2.537362] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 1 late 0
[    2.537365] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 2 late 0
[    2.537369] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 3 late 0
[    2.537373] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 0 late 1
[    2.537376] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 1 late 1
[    2.537380] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 2 late 1
[    2.537383] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 3 late 1
[    2.537387] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 0 late 2
[    2.537569] -------use name: ff880000.i2s, ffffff800888a598
[    3.543003] rk3399-gru-sound sound: HiFi <-> ff880000.i2s mapping ok
[    3.550150] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 1 late 2
[    3.558828] -------use name: ff880000.i2s, ffffff800888a598
[    3.746799] rk3399-gru-sound sound: rt5514-aif1 <-> ff880000.i2s mapping ok
[    3.754635] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 2 late 2
[    3.764970] -------use name: ff880000.i2s, ffffff800888a598
[    3.976496] rk3399-gru-sound sound: da7219-hifi <-> ff880000.i2s mapping ok
[    3.984292] rk3399-gru-sound sound: ASoC: probe rk3399-gru-sound dai link 3 late 2
[    3.992927] -------use name: spi2.0, ffffff80090aeb90
[    4.170426] *** pcm_new was NULL ***
[    4.174426] rk3399-gru-sound sound: snd-soc-dummy-dai <-> spi2.0 mapping ok
[    4.186804] input: rk3399-gru-sound Headset Jack as /devices/platform/sound/sound/card0/input5

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Brian Norris]

On Mon, Mar 13, 2017 at 12:50:04PM +0000, Mark Brown wrote:
> On Fri, Mar 10, 2017 at 04:24:07PM -0800, Brian Norris wrote:
> > On Thu, Mar 09, 2017 at 12:09:18PM +0100, Mark Brown wrote:
> 
> > > Please think hard before including complete backtraces in upstream
> > > reports, they are very large and contain almost no useful information
> > > relative to their size so often obscure the relevant content in your
> > > message. If part of the backtrace is usefully illustrative then it's
> > > usually better to pull out the relevant sections.
> 
> > I did think, and I did trim the trace significantly. I believe (or
> > believed; maybe incorrectly) that much of the remaining context *was*
> > useful, and that it could have answered some of Kuninori's questions, if
> > he chose to dig himself. As admitted in the commit message, I don't
> > really understand all of the relationships here, so it's hard to
> > highlight "only the relevant sections".
> 
> Any editing you'd done is really not obvious looking at the trace (the

There's a "..." in there why I trimmed a huge amount of junk that arm64
likes to dump, like surrounding memory context near the $pc and $sp.
Probably more.

> inclusion of the top of the trace with the pgd and so on which really
> need the image to be useful is a bit of a flag here, as is the bit at
> the end after the end of the trace).  It's certainly not mentioned or

The PC (0) and LR (which is not reflected in the backtrace) are useful.

> referenced.  At the very least for probe errors it's generally safe to
> cut down to just the driver probe function is being called rather than
> showing the entire device model call stack, it's vanishingly rare for
> that to be adding anything and it's generally several times deeper than
> what the driver is doing.

Sure, this could have been trimmed about 50% without losing information.
I can try to be more precise next time, but the 50% useful info included
was quite intentional. Particularly, I'm not very happy with my patch,
so alternatives are welcome (esp. from those who might be able to glean
more from the report than I could).

> > I'm sorry that my post did not meet your standards though. Next time
> > I'll just post a revert with little context and no attempt to understand
> > what actually went wrong.
> 
> Providing changes with little context and no conveyed understanding of
> exactly what went wrong is a very big part of the reason for complaining
> about people just splatting backtraces in and certainly not what I was
> asking for, it's sadly common to see a backtrace provided in lieu of any
> analysis and it does make the whole thing harder to read.  That would be
> even more problematic than what you sent, I wouldn't have applied such a
> change at all unless I somehow reverse engineered the analysis.

Sure, I can understand frustration with essentially contentless "bug
reports." And I got frustrated by a seemingly pat response that was
(IMO) a bit mistargeted.

> You do have a reasonable analysis of your change in the first couple of
> lines ("Not all platform drivers have pcm_{new,free} callbacks...")
> which is why I applied it, it's at least not going to do any harm to
> check that there's a function being provided.

I suppose.

BTW, I don't see the patch applied anywhere (and I have no reply -
either manual or automated/patchwork - other than the above quote to say
it was).

Brian

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Kuninori Morimoto]

Hi Brian

> > The crasher was snd_dmaengine_pcm_register's platform ?
> 
> No, actually it wasn't. It was spi2.0, which was a dummy, from
> snd_soc_dummy_probe(). But somehow snd_soc_platform_drv_pcm_new() got
> called for it...

Hmm.. I couldn't reproduce your issue on my system with dummy platform.
And your log is indicating something is happening which shouldn't happen.

Best regards
---
Kuninori Morimoto

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Kuninori Morimoto]

Hi Brian

> > The crasher was snd_dmaengine_pcm_register's platform ?
> 
> No, actually it wasn't. It was spi2.0, which was a dummy, from
> snd_soc_dummy_probe(). But somehow snd_soc_platform_drv_pcm_new() got
> called for it...

Hmm.. I couldn't reproduce your issue on my system with dummy platform.
And your log is indicating something is happening which shouldn't happen.

Best regards
---
Kuninori Morimoto

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

[Mark Brown]

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

On Mon, Mar 13, 2017 at 02:52:56PM -0700, Brian Norris wrote:

> Sure, I can understand frustration with essentially contentless "bug
> reports." And I got frustrated by a seemingly pat response that was
> (IMO) a bit mistargeted.

It *is* a pat response, it's one of the things I find I have to send
often enough that I've got a canned response for it - I find that if I
don't do things like that then after a while what gets sent becomes so
brief that unless you're really familiar with what's being said it can
be hard to understand the feedback.  It's tricky though because as you
say it's fairly obvious it's a form letter.

> BTW, I don't see the patch applied anywhere (and I have no reply -
> either manual or automated/patchwork - other than the above quote to say
> it was).

20170309102819.bx5paxtffvmvmuov@sirena.org.uk, fix/core  Didn't seem to
have made -next today, guess it'll hit tomorrow.

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

Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new, free}

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 907 bytes --]

On Mon, Mar 13, 2017 at 02:52:56PM -0700, Brian Norris wrote:

> Sure, I can understand frustration with essentially contentless "bug
> reports." And I got frustrated by a seemingly pat response that was
> (IMO) a bit mistargeted.

It *is* a pat response, it's one of the things I find I have to send
often enough that I've got a canned response for it - I find that if I
don't do things like that then after a while what gets sent becomes so
brief that unless you're really familiar with what's being said it can
be hard to understand the feedback.  It's tricky though because as you
say it's fairly obvious it's a form letter.

> BTW, I don't see the patch applied anywhere (and I have no reply -
> either manual or automated/patchwork - other than the above quote to say
> it was).

20170309102819.bx5paxtffvmvmuov@sirena.org.uk, fix/core  Didn't seem to
have made -next today, guess it'll hit tomorrow.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]