[alsa-devel] [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 --- 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,
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
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
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
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
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
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!
On 11/7/2018 9:44 PM, Mark Brown wrote:
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!
Liam/Mark, Do you have any suggestion on this?
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks, Rohit
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,
participants (4)
-
Arnaud Pouliquen -
Mark Brown -
Rohit kumar -
Takashi Iwai