| Message ID | 1541075929-29323-1-git-send-email-rohitkr@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | de17f14ea576d8a0f2932404467fa916542da94d |
| Headers | show |
| Series | ASoC: core: Invoke pcm_new() for all DAI-link | expand |
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
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,
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(-)