[TEST] Fix Cyan module load/unload issues by retrying the firmware boot by plbossart · Pull Request #2271 · thesofproject/linux

plbossart · 2020-07-13T21:52:12Z

This PR addresses issues that were missed early by CI and me/myself/ and I.
Somehow the module load/unload test fails randomly about 20-30% of the time. There's no obvious root cause, the FW_READY IPC is not provided and the boot times out. Adding a retry loop fixes the issue reliably.

it's quite ugly but aligned with other issues encountered on other platforms, so not something to be completely ashamed of - or just as bad as other platforms.

dmesg log excerpt

[  104.497229] sof-audio-acpi 808622A8:00: error: firmware boot failure
[  104.497296] sof-audio-acpi 808622A8:00: invalid header size 0xffffffff. FW oops is bogus
[  104.497314] sof-audio-acpi 808622A8:00: error: unexpected fault 0x90020000 trace 0x00003100
[  104.497329] sof-audio-acpi 808622A8:00: error: ipc host -> DSP: pending no complete yes raw 0x4000000000003100
[  104.497344] sof-audio-acpi 808622A8:00: error: mask host: pending no complete yes raw 0x1
[  104.497358] sof-audio-acpi 808622A8:00: error: ipc DSP -> host: pending no complete no raw 0x90020000
[  104.497372] sof-audio-acpi 808622A8:00: error: mask DSP: pending yes complete yes raw 0x3
[  104.497386] sof-audio-acpi 808622A8:00: error: failed to boot DSP firmware -5
[  104.497399] sof-audio-acpi 808622A8:00: retrying DSP boot, iteration 1
[  104.507939] sof-audio-acpi 808622A8:00: unexpected ipc interrupt raised!
[  104.507994] sof-audio-acpi 808622A8:00: Firmware info: version 1:5:1-88707
[  104.508005] sof-audio-acpi 808622A8:00: Firmware: ABI 3:16:0 Kernel ABI 3:17:0
[  104.508141] sof-audio-acpi 808622A8:00: warning: unknown ext header type 3 size 0x1c
[  104.508157] sof-audio-acpi 808622A8:00: warning: unknown ext header type 4 size 0x10
[  104.522576] sof-audio-acpi 808622A8:00: Topology: ABI 3:17:0 Kernel ABI 3:17:0
[  104.522589] sof-audio-acpi 808622A8:00: warning: widget type 1 name codec_out0 not handled
[  104.522595] sof-audio-acpi 808622A8:00: warning: widget type 7 name ssp2 Rx not handled
[  104.524952] sof-audio-acpi 808622A8:00: warning: widget type 1 name codec_out1 not handled
[  104.524957] sof-audio-acpi 808622A8:00: warning: widget type 7 name ssp2 Tx not handled
[  104.525424] sof-audio-acpi 808622A8:00: warning: widget type 0 name codec_in0 not handled
[  104.526014] sof-audio-acpi 808622A8:00: warning: widget type 0 name codec_in1 not handled
[  104.526405] sof-audio-acpi 808622A8:00: warning: widget type 1 name ssp0 Tx not handled
[  104.526409] sof-audio-acpi 808622A8:00: warning: widget type 0 name ssp0 Rx not handled
[  104.526412] sof-audio-acpi 808622A8:00: warning: widget type 1 name modem_out not handled
[  104.526415] sof-audio-acpi 808622A8:00: warning: widget type 0 name modem_in not handled
[  104.527771] sof-audio-acpi 808622A8:00: ASoC: Parent card not yet available, widget card binding deferred
[  104.550943] max98090 i2c-193C9890:00: MAX98090 REVID=0x43
[  104.553441] cht-bsw-max98090 cht-bsw-max98090: ASoC: no DMI vendor name!
[  104.555632] input: sof-bytcht max98090 Headset Jack as /devices/platform/808622A8:00/cht-bsw-max98090/sound/card0/input7

While I was at it, fix a number of IPC-related issues to clean-up the code and debug traces.

On some CherryTrail devices, the DSP sometimes fails to boot during load/unload tests, likely due to dependencies with the platform firmware or timing issues. Re-loading the firmware twice makes the problems go away. This is far from perfect but it's nearly impossible to solve the issue cleanly on 5-yr old devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

if the FW_READY mechanism fails, the platform may allow for a boot retry. In that case, debugfs complains about duplicate entries. Move the creation after boot success. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing code identifies unexpected IPC messages by testing if sdev->msg is NULL. That pointer is initialized but never reset to NULL, so the warning will only be thrown before the first IPC transfer happens. Fix by using the ipc_complete boolean used in all other places. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use the relevant register names to avoid confusion and use the correct bit offsets to avoid showing bad debug information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

s/regsisters/registers Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The Atom/sst driver configures all Tensilica-initiated traffic to be snooped. FIXME: Not sure if/why this this needed? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

bardliao · 2020-07-14T01:42:09Z

I tried 20 times module reload test, but I can't reproduce this issue with or without this PR on CML-H laptop

lyakh

seem to be no new CI failures, apart from those, also frequently present in other PRs

lyakh · 2020-07-14T09:15:05Z

 	snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_CSR,
-				  SHIM_BYT_CSR_STALL, 0x0);
+				  SHIM_BYT_CSR_XT_SNOOP | SHIM_BYT_CSR_STALL,
+				  SHIM_BYT_CSR_XT_SNOOP);


does this enable PCIe snooping of the DSP traffic? And without it it's disabled? I'm not an expert in that but in general that seems like a good thing to maintain cache coherency? I don't know why it would be possible to disable it, but maybe my guess is wrong and it's something different entirely.

I think it's wrong, will remove this.

would that leave snooping disabled? Or does that flag disable snooping when set?

WIP patch that works on Windows devices but not on Chromebook. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-07-14T23:46:15Z

Update known to fail on Cyan but want to check PCI base on MinnowBoard.

plbossart · 2020-07-15T00:26:47Z

I tried 20 times module reload test, but I can't reproduce this issue with or without this PR on CML-H laptop

yes, it's only trying to fix issues on the Cyan Chromebook

lyakh

a couple more comments

lyakh · 2020-07-15T07:59:01Z

+		/* when the DSP boots but FW_READY fails, tentatively retry */
+		if (ret == -EIO && ++tries < sof_ops(sdev)->firmware_boot_retries) {
+			dev_warn(sdev->dev, "retrying DSP boot, iteration %d\n", tries);
+			goto fw_load_retry;


don't we have to reset the sdev->fw_state to SOF_FW_BOOT_PREPARE when retrying? You could also extract snd_sof_load_firmware() and snd_sof_run_firmware() into a separate function and call it in a loop, thus avoiding a goto, but I'll probably do that anyway when my VirtIO patches reach that state... since they're touching this code too.

lyakh · 2020-07-15T08:02:20Z

+		/* errors are only due to memory allocation, not debugfs */
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: snd_sof_debugfs_buf_item failed\n");
+			return ret;


This also changes the error processing semantics: before this patch if debugfs file creation failed, we wouldn't boot the firmware, now the firmware is already running but we still fail probing... Maybe make this a soft failure now?

@plbossart I think we shoudl keep the debugfs creationg before attempting to boot. The sdev->fw_version member is populated when the FW_READY IPC message is received and if you move this to after boot is complete, I think this would never get populated

@ranj063 this was to work around the debugfs error warnings that the item is already created if you try and reboot multiple times when FW_READY never happens.

@plbossart ahh I see. Then I think this should be moved to ipc.c where we handle the FW_READY IPC. That would for sure indicate that the FW boot is complete and so its good to create the debugfs entry and copy the fw_version from the IPC reply.

Well we don't want to have a memory allocation failure in ipc.c, we should maybe create this earlier instead of in the firmware_run function.

then maybe we should just create this during dbg init?

lyakh · 2020-07-15T08:03:50Z

 	snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_CSR,
-				  SHIM_BYT_CSR_STALL, 0x0);
+				  SHIM_BYT_CSR_XT_SNOOP | SHIM_BYT_CSR_STALL,
+				  SHIM_BYT_CSR_XT_SNOOP);


would that leave snooping disabled? Or does that flag disable snooping when set?

ranj063 · 2020-07-15T19:11:35Z

+
+	pmc_status = snd_sof_dsp_read(sdev, BYT_PCI_BAR, PCI_PMCS);
+	if (pmc_status & 0x3) {
+		dev_err(sdev->dev, "error: PMC status does not indicated D0 state %x\n",


typo "indicate"

plbossart · 2020-07-23T21:53:57Z

Additional investigations show this is a Coreboot issue with BAR1 not set. Will be checking once a coreboot image is available.

plbossart added 6 commits July 13, 2020 16:43

ASoC: SOF: Intel: byt: fix IPC register dump

ea1e8ba

Use the relevant register names to avoid confusion and use the correct bit offsets to avoid showing bad debug information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: byt: fix typo in comment

2458e65

s/regsisters/registers Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: byt: snoop XT-initiated traffic

5130057

The Atom/sst driver configures all Tensilica-initiated traffic to be snooped. FIXME: Not sure if/why this this needed? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh and ranj063 as code owners July 13, 2020 21:52

lyakh previously approved these changes Jul 14, 2020

View reviewed changes

ASoC: SOF: Intel: byt: check if device is in D0 on probe

be9940e

WIP patch that works on Windows devices but not on Chromebook. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart dismissed lyakh’s stale review via be9940e July 14, 2020 23:45

lyakh requested changes Jul 15, 2020

View reviewed changes

ranj063 reviewed Jul 15, 2020

View reviewed changes

plbossart closed this Jul 23, 2020

plbossart mentioned this pull request Aug 13, 2020

[FEATURE] Add diagnostic dump for reset DSP failure with status = 0xffffffff thesofproject/sof#3267

Closed

Conversation

plbossart commented Jul 13, 2020

Uh oh!

bardliao commented Jul 14, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jul 14, 2020

Uh oh!

plbossart commented Jul 15, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jul 23, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants