[PATCH 0/3] ASoC: SOF: Intel: fix to dsp state dump trace levels

[PATCH 0/3] ASoC: SOF: Intel: fix to dsp state dump trace levels

[Kai Vehmanen]

Small series that addresses a problem where DSP status dump
for a failure case, ends up being printed as as debug print. This
is important information for any bug report. While at it, the series
contains a few cleanups to related code.

Ranjani Sridharan (3):
  ASoC: SOF: Intel: hda: remove duplicated status dump
  ASoC: SOF: modify the SOF_DBG flags
  ASoC: SOF: Intel: hda: fix the condition passed to sof_dev_dbg_or_err

 sound/soc/sof/debug.c            |  2 +-
 sound/soc/sof/intel/byt.c        |  2 +-
 sound/soc/sof/intel/hda-loader.c | 19 +++++++++++++------
 sound/soc/sof/intel/hda.c        | 10 ++++------
 sound/soc/sof/loader.c           |  4 ++--
 sound/soc/sof/ops.c              |  2 +-
 sound/soc/sof/sof-priv.h         | 13 ++++++++-----
 7 files changed, 30 insertions(+), 22 deletions(-)

-- 
2.29.2

[PATCH 1/3] ASoC: SOF: Intel: hda: remove duplicated status dump

[Kai Vehmanen]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Remove the duplicate status dump in case DSP init fails. The core will
be powered down in this case and the status dump will be invalid anyway.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 02c3ff897274..d8fb4e277c19 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -408,10 +408,12 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	 * should be ready for code loading and firmware boot
 	 */
 	ret = cl_copy_fw(sdev, stream);
-	if (!ret)
+	if (!ret) {
 		dev_dbg(sdev->dev, "Firmware download successful, booting...\n");
-	else
+	} else {
+		hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
 		dev_err(sdev->dev, "error: load fw failed ret: %d\n", ret);
+	}
 
 cleanup:
 	/*
@@ -435,9 +437,6 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	if (!ret)
 		return chip_info->init_core_mask;
 
-	/* dump dsp registers and disable DSP upon error */
-	hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
-
 	/* disable DSP */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR,
 				SOF_HDA_REG_PP_PPCTL,
-- 
2.29.2

[PATCH 2/3] ASoC: SOF: modify the SOF_DBG flags

[Kai Vehmanen]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The SOF_DBG_* macros are used for dual purposes right now, for the
sof_core_debug module parameter and for the dbg_dump() ops. So, separate
these two types of flags into different types to avoid confusion.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/sof/debug.c            |  2 +-
 sound/soc/sof/intel/byt.c        |  2 +-
 sound/soc/sof/intel/hda-loader.c |  4 ++--
 sound/soc/sof/loader.c           |  4 ++--
 sound/soc/sof/ops.c              |  2 +-
 sound/soc/sof/sof-priv.h         | 11 ++++++-----
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index 143117334ae5..30213a1beaaa 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -817,7 +817,7 @@ void snd_sof_handle_fw_exception(struct snd_sof_dev *sdev)
 	}
 
 	/* dump vital information to the logs */
-	snd_sof_dsp_dbg_dump(sdev, SOF_DBG_REGS | SOF_DBG_MBOX);
+	snd_sof_dsp_dbg_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX);
 	snd_sof_ipc_dump(sdev);
 	snd_sof_trace_notify_for_error(sdev);
 }
diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
index 186736ee5fc2..19260dbecac5 100644
--- a/sound/soc/sof/intel/byt.c
+++ b/sound/soc/sof/intel/byt.c
@@ -336,7 +336,7 @@ static int byt_run(struct snd_sof_dev *sdev)
 	}
 	if (tries < 0) {
 		dev_err(sdev->dev, "error:  unable to run DSP firmware\n");
-		byt_dump(sdev, SOF_DBG_REGS | SOF_DBG_MBOX);
+		byt_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX);
 		return -ENODEV;
 	}
 
diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index d8fb4e277c19..c4dcbe500635 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -175,7 +175,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag)
 			__func__);
 
 err:
-	hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
+	hda_dsp_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX);
 	hda_dsp_core_reset_power_down(sdev, chip->host_managed_cores_mask);
 
 	return ret;
@@ -411,7 +411,7 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	if (!ret) {
 		dev_dbg(sdev->dev, "Firmware download successful, booting...\n");
 	} else {
-		hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
+		hda_dsp_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX);
 		dev_err(sdev->dev, "error: load fw failed ret: %d\n", ret);
 	}
 
diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c
index cbce484b6469..df39f477239a 100644
--- a/sound/soc/sof/loader.c
+++ b/sound/soc/sof/loader.c
@@ -856,8 +856,8 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)
 				 msecs_to_jiffies(sdev->boot_timeout));
 	if (ret == 0) {
 		dev_err(sdev->dev, "error: firmware boot failure\n");
-		snd_sof_dsp_dbg_dump(sdev, SOF_DBG_REGS | SOF_DBG_MBOX |
-			SOF_DBG_TEXT | SOF_DBG_PCI);
+		snd_sof_dsp_dbg_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX |
+			SOF_DBG_DUMP_TEXT | SOF_DBG_DUMP_PCI);
 		sdev->fw_state = SOF_FW_BOOT_FAILED;
 		return -EIO;
 	}
diff --git a/sound/soc/sof/ops.c b/sound/soc/sof/ops.c
index 1a394b4c6a2f..11ecebd07907 100644
--- a/sound/soc/sof/ops.c
+++ b/sound/soc/sof/ops.c
@@ -157,7 +157,7 @@ void snd_sof_dsp_panic(struct snd_sof_dev *sdev, u32 offset)
 		dev_dbg(sdev->dev, "panic: dsp_oops_offset %zu offset %d\n",
 			sdev->dsp_oops_offset, offset);
 
-	snd_sof_dsp_dbg_dump(sdev, SOF_DBG_REGS | SOF_DBG_MBOX);
+	snd_sof_dsp_dbg_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX);
 	snd_sof_trace_notify_for_error(sdev);
 }
 EXPORT_SYMBOL(snd_sof_dsp_panic);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 9ba3e3ceed01..ba317d3b8e53 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -22,11 +22,12 @@
 
 /* debug flags */
 #define SOF_DBG_ENABLE_TRACE	BIT(0)
-#define SOF_DBG_REGS		BIT(1)
-#define SOF_DBG_MBOX		BIT(2)
-#define SOF_DBG_TEXT		BIT(3)
-#define SOF_DBG_PCI		BIT(4)
-#define SOF_DBG_RETAIN_CTX	BIT(5)	/* prevent DSP D3 on FW exception */
+#define SOF_DBG_RETAIN_CTX	BIT(1)	/* prevent DSP D3 on FW exception */
+
+#define SOF_DBG_DUMP_REGS		BIT(0)
+#define SOF_DBG_DUMP_MBOX		BIT(1)
+#define SOF_DBG_DUMP_TEXT		BIT(2)
+#define SOF_DBG_DUMP_PCI		BIT(3)
 
 /* global debug state set by SOF_DBG_ flags */
 extern int sof_core_debug;
-- 
2.29.2

[PATCH 3/3] ASoC: SOF: Intel: hda: fix the condition passed to sof_dev_dbg_or_err

[Kai Vehmanen]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The condition boot_iteration == HDA_FW_BOOT_ATTEMPTS to determine
the log level for the DSP status dump would only work in the case of DSP
init failure after maximum number of attempts to initialize the DSP. If
DSP init succeeds in less than HDA_FW_BOOT_ATTEMPTS attempts and FW
loading fails, the ROM status dump would end up getting logged as debug
instead of an error.

So, add a new flag, SOF_DBG_DUMP_LOG_ERROR, to explicitly specify
the log level for DSP status dump.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 12 ++++++++++--
 sound/soc/sof/intel/hda.c        | 10 ++++------
 sound/soc/sof/loader.c           |  2 +-
 sound/soc/sof/sof-priv.h         |  2 ++
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index c4dcbe500635..ed773696b495 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -88,6 +88,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag)
 	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	const struct sof_intel_dsp_desc *chip = hda->desc;
 	unsigned int status;
+	u32 flags;
 	int ret;
 	int i;
 
@@ -175,7 +176,13 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag)
 			__func__);
 
 err:
-	hda_dsp_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX);
+	flags = SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX;
+
+	/* force error log level after max boot attempts */
+	if (hda->boot_iteration == HDA_FW_BOOT_ATTEMPTS)
+		flags |= SOF_DBG_DUMP_FORCE_ERR_LEVEL;
+
+	hda_dsp_dump(sdev, flags);
 	hda_dsp_core_reset_power_down(sdev, chip->host_managed_cores_mask);
 
 	return ret;
@@ -411,7 +418,8 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	if (!ret) {
 		dev_dbg(sdev->dev, "Firmware download successful, booting...\n");
 	} else {
-		hda_dsp_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX);
+		hda_dsp_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX |
+			     SOF_DBG_DUMP_FORCE_ERR_LEVEL);
 		dev_err(sdev->dev, "error: load fw failed ret: %d\n", ret);
 	}
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index bb4128a72a42..509a9b256423 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -416,9 +416,8 @@ void hda_dsp_dump_skl(struct snd_sof_dev *sdev, u32 flags)
 }
 
 /* dump the first 8 dwords representing the extended ROM status */
-static void hda_dsp_dump_ext_rom_status(struct snd_sof_dev *sdev)
+static void hda_dsp_dump_ext_rom_status(struct snd_sof_dev *sdev, u32 flags)
 {
-	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	char msg[128];
 	int len = 0;
 	u32 value;
@@ -429,14 +428,13 @@ static void hda_dsp_dump_ext_rom_status(struct snd_sof_dev *sdev)
 		len += snprintf(msg + len, sizeof(msg) - len, " 0x%x", value);
 	}
 
-	sof_dev_dbg_or_err(sdev->dev, hda->boot_iteration == HDA_FW_BOOT_ATTEMPTS,
+	sof_dev_dbg_or_err(sdev->dev, flags & SOF_DBG_DUMP_FORCE_ERR_LEVEL,
 			   "extended rom status: %s", msg);
 
 }
 
 void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags)
 {
-	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	struct sof_ipc_dsp_oops_xtensa xoops;
 	struct sof_ipc_panic_info panic_info;
 	u32 stack[HDA_DSP_STACK_DUMP_SIZE];
@@ -456,11 +454,11 @@ void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags)
 		snd_sof_get_status(sdev, status, panic, &xoops, &panic_info,
 				   stack, HDA_DSP_STACK_DUMP_SIZE);
 	} else {
-		sof_dev_dbg_or_err(sdev->dev, hda->boot_iteration == HDA_FW_BOOT_ATTEMPTS,
+		sof_dev_dbg_or_err(sdev->dev, flags & SOF_DBG_DUMP_FORCE_ERR_LEVEL,
 				   "status = 0x%8.8x panic = 0x%8.8x\n",
 				   status, panic);
 
-		hda_dsp_dump_ext_rom_status(sdev);
+		hda_dsp_dump_ext_rom_status(sdev, flags);
 		hda_dsp_get_status(sdev);
 	}
 }
diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c
index df39f477239a..08a17abb63ff 100644
--- a/sound/soc/sof/loader.c
+++ b/sound/soc/sof/loader.c
@@ -857,7 +857,7 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)
 	if (ret == 0) {
 		dev_err(sdev->dev, "error: firmware boot failure\n");
 		snd_sof_dsp_dbg_dump(sdev, SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX |
-			SOF_DBG_DUMP_TEXT | SOF_DBG_DUMP_PCI);
+			SOF_DBG_DUMP_TEXT | SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_FORCE_ERR_LEVEL);
 		sdev->fw_state = SOF_FW_BOOT_FAILED;
 		return -EIO;
 	}
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index ba317d3b8e53..68da8f797403 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -28,6 +28,8 @@
 #define SOF_DBG_DUMP_MBOX		BIT(1)
 #define SOF_DBG_DUMP_TEXT		BIT(2)
 #define SOF_DBG_DUMP_PCI		BIT(3)
+#define SOF_DBG_DUMP_FORCE_ERR_LEVEL	BIT(4) /* used to dump dsp status with error log level */
+
 
 /* global debug state set by SOF_DBG_ flags */
 extern int sof_core_debug;
-- 
2.29.2

Re: [PATCH 0/3] ASoC: SOF: Intel: fix to dsp state dump trace levels

[Mark Brown]

On Fri, 11 Dec 2020 12:07:40 +0200, Kai Vehmanen wrote:
> Small series that addresses a problem where DSP status dump
> for a failure case, ends up being printed as as debug print. This
> is important information for any bug report. While at it, the series
> contains a few cleanups to related code.
> 
> Ranjani Sridharan (3):
>   ASoC: SOF: Intel: hda: remove duplicated status dump
>   ASoC: SOF: modify the SOF_DBG flags
>   ASoC: SOF: Intel: hda: fix the condition passed to sof_dev_dbg_or_err
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: SOF: Intel: hda: remove duplicated status dump
      commit: b278fc55b47739da49ea7f95e6ad58d436091ba2
[2/3] ASoC: SOF: modify the SOF_DBG flags
      commit: fbfa22ec4b2b8a1bb1a52c56c376295c7b7e7849
[3/3] ASoC: SOF: Intel: hda: fix the condition passed to sof_dev_dbg_or_err
      commit: 8f7ef6fca0317fb217d1eef8f30010d7a9c6ae0e

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