ASoC: Intel: Boards: cml_rt1011_rt5682: use statically define codec c… by fredoh9 · Pull Request #2173 · thesofproject/linux

fredoh9 · 2020-06-07T19:43:38Z

…onfig

When the machine driver probe is deferred, the memory allocated
during probe seems to be corrupted causing the card registration to fail.

Instead of dynamic allocation for 4-channel config, statically define
2spk and 4spk and update config and codec component accordingly.

KASAN issue fixed.
[ 23.301373] cml_rt1011_rt5682 cml_rt1011_rt5682: sof_rt1011_quirk = f
[ 23.301875] ==================================================================
[ 23.302018] BUG: KASAN: use-after-free in snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682]
[ 23.302178] Read of size 8 at addr ffff8881ec6acae0 by task kworker/0:2/105
[ 23.302320] CPU: 0 PID: 105 Comm: kworker/0:2 Not tainted 5.7.0-rc7-test+ #3
[ 23.302322] Hardware name: Google Helios/Helios, BIOS 01/21/2020
[ 23.302329] Workqueue: events deferred_probe_work_func
[ 23.302331] Call Trace:
[ 23.302339] dump_stack+0x76/0xa0
[ 23.302345] print_address_description.constprop.0.cold+0xd3/0x43e
[ 23.302351] ? _raw_spin_lock_irqsave+0x7b/0xd0
[ 23.302355] ? _raw_spin_trylock_bh+0xf0/0xf0
[ 23.302362] ? snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682]
[ 23.302365] __kasan_report.cold+0x37/0x86
[ 23.302371] ? snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682]
[ 23.302375] kasan_report+0x38/0x50
[ 23.302382] snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682]
[ 23.302389] platform_drv_probe+0x66/0xc0

Suggested-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
Signed-off-by: Fred Oh fred.oh@linux.intel.com

fixes: #2168

lyakh

"When the machine driver probe is deferred, the memory allocated during probe seems to be corrupted" - I think it would be nice to actually understand what's going on there. Do you mean that happens when the driver probing returns -EPROBE_DEFER? AFAICT from the PoV of the driver probing it should be treated just like a normal probing failure, e.g. all the devm_* resources should be freed. Is this what you're referring to?

bardliao · 2020-06-08T09:03:04Z

"When the machine driver probe is deferred, the memory allocated during probe seems to be corrupted" - I think it would be nice to actually understand what's going on there. Do you mean that happens when the driver probing returns -EPROBE_DEFER? AFAICT from the PoV of the driver probing it should be treated just like a normal probing failure, e.g. all the devm_* resources should be freed. Is this what you're referring to?

@lyakh The PR is for #2168. It do fix the issue, but it may not point to the right reason. See #2168 (comment). Alternative solution is #2174 However, I like this PR anyway. Statically define is always more readable.

plbossart

see comments below. we need to choose between this and @bardliao 's solution.

ranj063 · 2020-06-08T16:02:17Z

"When the machine driver probe is deferred, the memory allocated during probe seems to be corrupted" - I think it would be nice to actually understand what's going on there. Do you mean that happens when the driver probing returns -EPROBE_DEFER? AFAICT from the PoV of the driver probing it should be treated just like a normal probing failure, e.g. all the devm_* resources should be freed. Is this what you're referring to?
@lyakh The PR is for #2168. It do fix the issue, but it may not point to the right reason. See #2168 (comment). Alternative solution is #2174 However, I like this PR anyway. Statically define is always more readable.

@bardliao I think this solution is bit more straightforward as well and the risks of use-after-free are lower with this one and the code is a lot simpler. So if you agree maybe we can go with this one once @fredoh9 fixes the comments on the commit message and the separate patch to change the dev_info to dev_dbg

plbossart · 2020-06-08T16:10:51Z

"When the machine driver probe is deferred, the memory allocated during probe seems to be corrupted" - I think it would be nice to actually understand what's going on there. Do you mean that happens when the driver probing returns -EPROBE_DEFER? AFAICT from the PoV of the driver probing it should be treated just like a normal probing failure, e.g. all the devm_* resources should be freed. Is this what you're referring to?
@lyakh The PR is for #2168. It do fix the issue, but it may not point to the right reason. See #2168 (comment). Alternative solution is #2174 However, I like this PR anyway. Statically define is always more readable.
@bardliao I think this solution is bit more straightforward as well and the risks of use-after-free are lower with this one and the code is a lot simpler. So if you agree maybe we can go with this one once @fredoh9 fixes the comments on the commit message and the separate patch to change the dev_info to dev_dbg

we also want the last patch from @bardliao to walk through the dalinks

fredoh9 · 2020-06-08T17:05:41Z

I addressed @plbossart comments and cherry-pick @bardliao 2nd patch also. I need to more testing with it now. Will update my branch in shortly.

… quirk Change dev_info to dev_dbg to reduce noise during multiple deferred probes. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

plbossart

2 nit-picks below but it's looking good otherwise. Thanks @fredoh9

plbossart · 2020-06-08T20:16:06Z

- cml_rt1011_rt5682_dailink[i].codecs = ssp1_codec_4spk;
- cml_rt1011_rt5682_dailink[i].num_codecs =
- ARRAY_SIZE(ssp1_codec_4spk);
+ CML_RT1011_CODEC_DAI)) {


indentation seems odd here?

fredoh9 · 2020-06-08T21:25:59Z

From device test, one error is reported in CML_RVP. I will look into it and address @plbossart comments.
sh-cml-rvp-hda-01 kernel: [ 630.851303] sof-audio-pci 0000:00:1f.3: error: unknown DSP message 0xf0000000

fredoh9 · 2020-06-08T23:08:52Z

Addressed @plbossart 's two comments.

For KMOD test, I tested with both CML Helios and CML_RVP_HDA. I worked fine for 25 iterations. And I don't think this change to do with sh-cml-rvp-hda-01 kernel: [ 630.851303] sof-audio-pci 0000:00:1f.3: error: unknown DSP message 0xf0000000. Will monitor device test result again.

plbossart · 2020-06-08T23:30:36Z

the alignment is still off

diff --git a/sound/soc/intel/boards/cml_rt1011_rt5682.c b/sound/soc/intel/boards/cml_rt1011_rt5682.c
index bb9862ff23ae..d56c708a332b 100644
--- a/sound/soc/intel/boards/cml_rt1011_rt5682.c+++ b/sound/soc/intel/boards/cml_rt1011_rt5682.c@@ -548,7 +548,7 @@ static int snd_cml_rt1011_probe(struct platform_device *pdev)
SOF_RT1011_SPEAKER_TR)) {
for_each_card_prelinks(&snd_soc_card_cml, i, dai_link) {
if (!strcmp(cml_rt1011_rt5682_dailink[i].codecs->dai_name,
- CML_RT1011_CODEC_DAI)) {+ CML_RT1011_CODEC_DAI)) {
dai_link->codecs = ssp1_codec_4spk;
dai_link->num_codecs = ARRAY_SIZE(ssp1_codec_4spk);

…onfig When the cml_rt1011_rt5682_dailink[].codecs pointer is overridden by a quirk with a devm allocated structure and the probe is deferred, in the next probe we will see an use-after-free condition (verified with KASAN). This can be avoided by using statically allocated configurations - which simplifies the code quite a bit as well. KASAN issue fixed. [ 23.301373] cml_rt1011_rt5682 cml_rt1011_rt5682: sof_rt1011_quirk = f [ 23.301875] ================================================================== [ 23.302018] BUG: KASAN: use-after-free in snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682] [ 23.302178] Read of size 8 at addr ffff8881ec6acae0 by task kworker/0:2/105 [ 23.302320] CPU: 0 PID: 105 Comm: kworker/0:2 Not tainted 5.7.0-rc7-test+ #3 [ 23.302322] Hardware name: Google Helios/Helios, BIOS 01/21/2020 [ 23.302329] Workqueue: events deferred_probe_work_func [ 23.302331] Call Trace: [ 23.302339] dump_stack+0x76/0xa0 [ 23.302345] print_address_description.constprop.0.cold+0xd3/0x43e [ 23.302351] ? _raw_spin_lock_irqsave+0x7b/0xd0 [ 23.302355] ? _raw_spin_trylock_bh+0xf0/0xf0 [ 23.302362] ? snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682] [ 23.302365] __kasan_report.cold+0x37/0x86 [ 23.302371] ? snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682] [ 23.302375] kasan_report+0x38/0x50 [ 23.302382] snd_cml_rt1011_probe+0x23a/0x3d0 [snd_soc_cml_rt1011_rt5682] [ 23.302389] platform_drv_probe+0x66/0xc0 Fixes: 629ba12 ("ASoC: Intel: boards: split woofer and tweeter support") Suggested-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

for_each_card_prelinks() is a common API to walk through each prelink in the card. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

fredoh9 · 2020-06-08T23:44:14Z

@plbossart my bad, aligned again with strcmp() block.

plbossart

thanks @fredoh9

fredoh9 · 2020-06-09T01:04:42Z

From latest device test result, I don't see same error.
https://sof-ci.01.org/linuxpr/PR2173/build3907/devicetest/

ranj063 · 2020-06-09T01:11:52Z

@fredoh9 the device that this PR is meant for shows "Not Tested".

fredoh9 · 2020-06-09T01:26:46Z

SOFCI TEST

bardliao

It looks good, but I think using cml_rt1011_rt5682_dailink[i].codecs[0]->dai_name makes more sense since cml_rt1011_rt5682_dailink[i].codecs is an array. We can fix it in another commit.

bardliao · 2020-06-09T01:40:39Z

- snd_soc_card_cml.num_configs = SPK_CH;
-
 for (i = 0; i < ARRAY_SIZE(cml_rt1011_rt5682_dailink); i++) {
 if (!strcmp(cml_rt1011_rt5682_dailink[i].codecs->dai_name,


cml_rt1011_rt5682_dailink[i].codecs is an array, It is harmless, but I think using cml_rt1011_rt5682_dailink[i].codecs[0]->dai_name makes more sense.

yes, this will have to be a separate patch anyway though

bardliao

Ready to go. Thanks @fredoh9

fredoh9 · 2020-06-09T03:37:51Z

SOFCI TEST

fredoh9 · 2020-06-09T14:08:30Z

CI Test results are good now.
https://sof-ci.01.org/linuxpr/PR2173/build3914/devicetest/

fredoh9 requested review from dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners June 7, 2020 19:43

fredoh9 commented Jun 7, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

bardliao reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from 352da05 to 4b3ec69Compare June 8, 2020 03:54

YvonneYang2 mentioned this pull request Jun 8, 2020
[BUG][CML] "ASoC: Neither/both codec name/of_node are set for SSP1-Codec" shows when boot up system #2168
Closed

lyakh reviewed Jun 8, 2020
View reviewed changes

bardliao reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c

bardliao reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

plbossart requested changes Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c
Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from 4b3ec69 to c5ea25bCompare June 8, 2020 17:31

ranj063 reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

ranj063 reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

ASoC: Intel: Boards: cml_rt1011_rt5682: reduce log level for printing…
afa20e1
… quirk Change dev_info to dev_dbg to reduce noise during multiple deferred probes. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from c5ea25b to e33af32Compare June 8, 2020 18:34

ranj063 reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

ranj063 reviewed Jun 8, 2020
View reviewed changes

Comment threadsound/soc/intel/boards/cml_rt1011_rt5682.c Outdated

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from e33af32 to 2d85740Compare June 8, 2020 18:51

ranj063 previously approved these changes Jun 8, 2020
View reviewed changes

plbossart requested changes Jun 8, 2020
View reviewed changes

fredoh9 dismissed ranj063’s stale review via 025922dJune 8, 2020 23:06

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from 2d85740 to 025922dCompare June 8, 2020 23:06

fredoh9and others added 2 commits June 8, 2020 16:38

ASoC: intel: cml_rt1011_rt5682: use for_each_card_prelinks
062af53
for_each_card_prelinks() is a common API to walk through each prelink in the card. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

fredoh9 force-pushed the fix/cml_helios_rt5682_probe branch from 025922d to 062af53Compare June 8, 2020 23:42

plbossart approved these changes Jun 9, 2020
View reviewed changes

ranj063 approved these changes Jun 9, 2020
View reviewed changes

bardliao reviewed Jun 9, 2020
View reviewed changes

bardliao approved these changes Jun 9, 2020
View reviewed changes

plbossart merged commit a7bffa4 into thesofproject:topic/sof-devJun 9, 2020

fredoh9 mentioned this pull request Jun 9, 2020
ASoc: Intel: cml_rt1011_rt5682: explicitly access first codec #2181
Merged

Uh oh!

Conversation

fredoh9 commented Jun 7, 2020

Uh oh!

Uh oh!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

bardliao commented Jun 8, 2020

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Jun 8, 2020

Uh oh!

plbossart commented Jun 8, 2020

Uh oh!

fredoh9 commented Jun 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossartJun 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredoh9 commented Jun 8, 2020

Uh oh!

fredoh9 commented Jun 8, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Jun 8, 2020

Uh oh!

fredoh9 commented Jun 8, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

fredoh9 commented Jun 9, 2020

Uh oh!

ranj063 commented Jun 9, 2020

Uh oh!

fredoh9 commented Jun 9, 2020

Uh oh!

bardliao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliaoJun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ranj063Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

fredoh9 commented Jun 9, 2020

Uh oh!

fredoh9 commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fredoh9 commented Jun 8, 2020•
edited
Loading

bardliao left a comment •
edited
Loading