ASoC: core: Invoke pcm_new() for all DAI-link

PATCH In review Rohit kumar 4 participants 9 messages Nov 1, 2018 – Dec 13, 2018

Subsystems (2)

Sound ASoC

lore

Share & tools

lore

Rohit kumar

author

2018-11-01 12:38 UTC #0 lore raw

PATCH 1/1 ASoC: core: Invoke pcm_new() for all DAI-link

Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.

Below is the crash stack:

[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21
..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164

Signed-off-by Rohit kumar <[email protected]>

---
 sound/soc/soc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/soc-core.c +1 -1

index 6ddcf12..abdc460 100644
@@ -1467,7 +1467,7 @@ static int soc_link_dai_pcm_new(struct snd_soc_dai **dais, int num_dais,
 	for (i = 0; i < num_dais; ++i) {
 		struct snd_soc_dai_driver *drv = dais[i]->driver;
 
-		if (!rtd->dai_link->no_pcm && drv->pcm_new)
+		if (drv->pcm_new)
 			ret = drv->pcm_new(rtd, dais[i]);
 		if (ret < 0) {
 			dev_err(dais[i]->dev,

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Takashi Iwai

2018-11-02 07:42 UTC #1 lore raw

Quoted 20 lines · Rohit kumar

Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.
Below is the crash stack:
[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21
..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164

Quoted 1 line

Signed-off-by Rohit kumar <[email protected]>

Did you check whether all drivers have no side-effect by this change? The hdmi-codec isn't the only driver that has pcm_new ops, so we have to make sure that such a fundamental change wouldn't bring any regressions.

thanks,

Takashi

Rohit Kumar

author

2018-11-02 12:06 UTC #2 lore raw

Quoted 21 lines · Takashi Iwai

Quoted 19 lines · Rohit kumar
Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.
Below is the crash stack:
[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21
..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164

Quoted 6 lines

Quoted 1 line
Signed-off-by Rohit kumar <[email protected]>
Did you check whether all drivers have no side-effect by this change? The hdmi-codec isn't the only driver that has pcm_new ops, so we have to make sure that such a fundamental change wouldn't bring any regressions.

Below are the drivers calling pcm_new() other than hdmi codec driver. sound/soc/meson/axg-frddr.c
sound/soc/meson/axg-toddr.c
These two drivers are frontend DAI drivers and should not be impacted because of this.

Other than this, pcm_new() is called from sound/soc/stm/stm32_sai_sub.c I could not get much info about this driver. However, it is just adding kcontrols in pcm_new() which uses internal private structs in get()/put(). Olivier Moysan can too confirm on this.

Thanks,
Rohit

Quoted 3 lines

thanks,
Takashi

Signature · 2 lines

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.

Arnaud Pouliquen

2018-11-05 11:13 UTC #3 lore raw

Hello Rohit,

Quoted 38 lines · Rohit Kumar

Quoted 24 lines · Takashi Iwai
Quoted 22 lines · Rohit kumar
Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.
Below is the crash stack:
[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP [   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21 ..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164
Quoted 6 lines
Quoted 1 line
Signed-off-by Rohit kumar <[email protected]>
Did you check whether all drivers have no side-effect by this change? The hdmi-codec isn't the only driver that has pcm_new ops, so we have to make sure that such a fundamental change wouldn't bring any regressions.
Below are the drivers calling pcm_new() other than hdmi codec driver. sound/soc/meson/axg-frddr.c
sound/soc/meson/axg-toddr.c
These two drivers are frontend DAI drivers and should not be impacted because of this.

Quoted 4 lines

Other than this, pcm_new() is called from sound/soc/stm/stm32_sai_sub.c I could not get much info about this driver. However, it is just adding kcontrols in pcm_new() which uses internal private structs in get()/put(). Olivier Moysan can too confirm on this.

First, i'm answering for Olivier: no regression identified for the SAI driver, it is not a DPCM driver.

Then i have a concern about the call of pcm_new for a no-PCM backend. Does it make sense? In DPCM concept, the backend is not linked to the PCM device...

Instead, I would suggest that you add protection in HDMI_codec on chmap_info pointer.

The drawback would be that the control is no more available...do you need it?

Regards
Arnaud

Quoted 7 lines

Thanks,
Rohit
Quoted 3 lines
thanks,
Takashi

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Rohit Kumar

author

2018-11-05 18:14 UTC #4 lore raw

Hello Arnaud,

Quoted 51 lines · Arnaud Pouliquen

Hello Rohit,
Quoted 37 lines · Rohit Kumar
Quoted 24 lines · Takashi Iwai
Quoted 22 lines · Rohit kumar
Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.
Below is the crash stack:
[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP [   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21 ..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164
Quoted 6 lines
Quoted 1 line
Signed-off-by Rohit kumar <[email protected]>
Did you check whether all drivers have no side-effect by this change? The hdmi-codec isn't the only driver that has pcm_new ops, so we have to make sure that such a fundamental change wouldn't bring any regressions.
Below are the drivers calling pcm_new() other than hdmi codec driver. sound/soc/meson/axg-frddr.c
sound/soc/meson/axg-toddr.c
These two drivers are frontend DAI drivers and should not be impacted because of this.
Quoted 4 lines
Other than this, pcm_new() is called from sound/soc/stm/stm32_sai_sub.c I could not get much info about this driver. However, it is just adding kcontrols in pcm_new() which uses internal private structs in get()/put(). Olivier Moysan can too confirm on this.
First, i'm answering for Olivier: no regression identified for the SAI driver, it is not a DPCM driver.
Then i have a concern about the call of pcm_new for a no-PCM backend. Does it make sense? In DPCM concept, the backend is not linked to the PCM device...

Quoted 5 lines

Instead, I would suggest that you add protection in HDMI_codec on chmap_info pointer.
The drawback would be that the control is no more available...do you need it?

I don't need chmap_info, but ELD kcontrol is also defined in pcm_new() which we need. We should probably update the driver to make it compatible with DPCM. Any suggestions?

Quoted 8 lines

Regards
Arnaud
Quoted 5 lines
Thanks,
Rohit
Quoted 3 lines
thanks,
Takashi

Thanks,
Rohit

Signature · 2 lines

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.

Arnaud Pouliquen

2018-11-06 15:41 UTC #5 lore raw

Hello Rohit

Quoted 59 lines · Rohit Kumar

Hello Arnaud,
Quoted 55 lines · Arnaud Pouliquen
Hello Rohit,
Quoted 37 lines · Rohit Kumar
Quoted 24 lines · Takashi Iwai
Quoted 22 lines · Rohit kumar
Remove no_pcm check to invoke pcm_new() for backend dai-links too. This fixes crash in hdmi codec driver during hdmi_codec_startup() while accessing chmap_info struct. chmap_info struct memory is allocated in pcm_new() of hdmi codec driver which is not invoked in case of DPCM when hdmi codec driver is part of backend dai-link.
Below is the crash stack:
[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP [   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21 ..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164
Quoted 6 lines
Quoted 1 line
Signed-off-by Rohit kumar <[email protected]>
Did you check whether all drivers have no side-effect by this change? The hdmi-codec isn't the only driver that has pcm_new ops, so we have to make sure that such a fundamental change wouldn't bring any regressions.
Below are the drivers calling pcm_new() other than hdmi codec driver. sound/soc/meson/axg-frddr.c
sound/soc/meson/axg-toddr.c
These two drivers are frontend DAI drivers and should not be impacted because of this.
Quoted 5 lines
Other than this, pcm_new() is called from sound/soc/stm/stm32_sai_sub.c I could not get much info about this driver. However, it is just adding kcontrols in pcm_new() which uses internal private structs in get()/put().
Olivier Moysan can too confirm on this.
First, i'm answering for Olivier: no regression identified for the SAI driver, it is not a DPCM driver.
Then i have a concern about the call of pcm_new for a no-PCM backend. Does it make sense? In DPCM concept, the backend is not linked to the PCM device...
Instead, I would suggest that you add protection in HDMI_codec on chmap_info pointer.

Quoted 6 lines

Quoted 2 lines
The drawback would be that the control is no more available...do you need it?
I don't need chmap_info, but ELD kcontrol is also defined in pcm_new() which
we need. We should probably update the driver to make it compatible with DPCM. Any suggestions?

I would say force device to 0 if no_pcm (need probably to create the control in hdmi_of_xlate_dai_id instead of hdmi_codec_pcm_new). But keep in mind that solution has to work in case of multi HDMI codec instances, perhaps using prefix...
Anyway, i'm not a DPCM expert so perhaps more elegant solutions exist.

Regards
Arnaud

Quoted 11 lines

Quoted 8 lines
Regards
Arnaud
Quoted 5 lines
Thanks,
Rohit
Quoted 3 lines
thanks,
Takashi
Thanks,
Rohit

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Mark Brown

maintainer

2018-11-07 16:14 UTC #6 lore raw

Quoted 5 lines · Arnaud Pouliquen

I would say force device to 0 if no_pcm (need probably to create the control in hdmi_of_xlate_dai_id instead of hdmi_codec_pcm_new). But keep in mind that solution has to work in case of multi HDMI codec instances, perhaps using prefix...
Anyway, i'm not a DPCM expert so perhaps more elegant solutions exist.

Liam, any thoughts on how to handle this? I've still not really worked with DPCM systems!

Rohit Kumar

author

2018-11-21 07:38 UTC #7 lore raw

Quoted 10 lines · Mark Brown

Quoted 5 lines · Arnaud Pouliquen
I would say force device to 0 if no_pcm (need probably to create the control in hdmi_of_xlate_dai_id instead of hdmi_codec_pcm_new). But keep in mind that solution has to work in case of multi HDMI codec instances, perhaps using prefix...
Anyway, i'm not a DPCM expert so perhaps more elegant solutions exist.
Liam, any thoughts on how to handle this? I've still not really worked with DPCM systems!

Liam/Mark, Do you have any suggestion on this?

Quoted 5 lines

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Thanks,
Rohit

Signature · 2 lines

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.

Mark Brown

maintainer

2018-12-13 18:10 UTC #8 lore raw

Applied "ASoC: core: Invoke pcm_new() for all DAI-link" to the asoc tree

In reply to Rohit kumar

The patch

ASoC: core: Invoke pcm_new() for all DAI-link

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

Quoted 1 line

From de17f14ea576d8a0f2932404467fa916542da94d Mon Sep 17 00:00:00 2001

From: Rohit kumar <[email protected]>
Date: Thu, 1 Nov 2018 18:08:49 +0530
Subject: [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

Below is the crash stack:

[   61.635493] Unable to handle kernel NULL pointer dereference at virtual address 00000018
..
[   61.666696]   CM = 0, WnR = 1
[   61.669778] user pgtable: 4k pages, 39-bit VAs, pgd = ffffffc0d6633000
[   61.676526] [0000000000000018] *pgd=0000000153fc8003, *pud=0000000153fc8003, *pmd=0000000000000000
[   61.685793] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   61.722955] CPU: 7 PID: 2238 Comm: aplay Not tainted 4.14.72 #21
..
[   61.740269] PC is at hdmi_codec_startup+0x124/0x164
[   61.745308] LR is at hdmi_codec_startup+0xe4/0x164

Signed-off-by Rohit kumar <[email protected]>

Signed-off-by Mark Brown <[email protected]>

---
 sound/soc/soc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/soc-core.c +1 -1

index b0db59e6339d..0462b3ec977a 100644
@@ -1467,7 +1467,7 @@ static int soc_link_dai_pcm_new(struct snd_soc_dai **dais, int num_dais,
 	for (i = 0; i < num_dais; ++i) {
 		struct snd_soc_dai_driver *drv = dais[i]->driver;
 
-		if (!rtd->dai_link->no_pcm && drv->pcm_new)
+		if (drv->pcm_new)
 			ret = drv->pcm_new(rtd, dais[i]);
 		if (ret < 0) {
 			dev_err(dais[i]->dev,

Signature · 1 line

2.19.0.rc2