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

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

[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

Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
---
 sound/soc/soc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6ddcf12..abdc460 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -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,
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Takashi Iwai]

On Thu, 01 Nov 2018 13:38:49 +0100,
Rohit kumar wrote:
> 
> 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 <rohitkr@codeaurora.org>

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

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Rohit Kumar]

On 11/2/2018 1:12 PM, Takashi Iwai wrote:
> On Thu, 01 Nov 2018 13:38:49 +0100,
> Rohit kumar wrote:
>> 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 <rohitkr@codeaurora.org>
> 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
> thanks,
>
> Takashi

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

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Arnaud Pouliquen]

Hello Rohit,

On 11/2/18 1:06 PM, Rohit Kumar wrote:
> 
> On 11/2/2018 1:12 PM, Takashi Iwai wrote:
>> On Thu, 01 Nov 2018 13:38:49 +0100,
>> Rohit kumar wrote:
>>> 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 <rohitkr@codeaurora.org>
>> 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.
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

> 
> Thanks,
> Rohit
>> thanks,
>>
>> Takashi
> 

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Rohit Kumar]

Hello Arnaud,


On 11/5/2018 4:43 PM, Arnaud Pouliquen wrote:
> Hello Rohit,
>
> On 11/2/18 1:06 PM, Rohit Kumar wrote:
>> On 11/2/2018 1:12 PM, Takashi Iwai wrote:
>>> On Thu, 01 Nov 2018 13:38:49 +0100,
>>> Rohit kumar wrote:
>>>> 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 <rohitkr@codeaurora.org>
>>> 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.
> 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?
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?
> Regards
> Arnaud
>
>> Thanks,
>> Rohit
>>> thanks,
>>>
>>> Takashi
Thanks,
Rohit

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

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Arnaud Pouliquen]

Hello Rohit

On 11/5/18 7:14 PM, Rohit Kumar wrote:
> Hello Arnaud,
> 
> 
> On 11/5/2018 4:43 PM, Arnaud Pouliquen wrote:
>> Hello Rohit,
>>
>> On 11/2/18 1:06 PM, Rohit Kumar wrote:
>>> On 11/2/2018 1:12 PM, Takashi Iwai wrote:
>>>> On Thu, 01 Nov 2018 13:38:49 +0100,
>>>> Rohit kumar wrote:
>>>>> 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 <rohitkr@codeaurora.org>
>>>> 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.
>> 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?
> 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

>> Regards
>> Arnaud
>>
>>> Thanks,
>>> Rohit
>>>> thanks,
>>>>
>>>> Takashi
> Thanks,
> Rohit
> 

Re: [alsa-devel] [PATCH] ASoC: core: Invoke pcm_new() for all DAI-link

[Mark Brown]

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

On Tue, Nov 06, 2018 at 04:41:23PM +0100, Arnaud Pouliquen wrote:

> 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!

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

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

[Mark Brown]

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

From de17f14ea576d8a0f2932404467fa916542da94d Mon Sep 17 00:00:00 2001
From: Rohit kumar <rohitkr@codeaurora.org>
Date: Thu, 1 Nov 2018 18:08:49 +0530
Subject: [PATCH] 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 <rohitkr@codeaurora.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b0db59e6339d..0462b3ec977a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -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,
-- 
2.19.0.rc2