[PATCH 0/3] ASoC: SOF: ipc3-dtrace: Handle race during initialization

[PATCH 0/3] ASoC: SOF: ipc3-dtrace: Handle race during initialization

[Peter Ujfalusi]

Hi,

This series handles the race which can result missing the first position update
after the trace is enabled.
In short: the firmware might send the position update (if we have enough
trace data generated) after the dma-trace is enabled by the TRACE_DMA_PARAMS_EXT
message. Depending on scheduling, load, preemption on Linux side we have seen
that occasionally this first position update got missed and we missed reading it
out.

A new state and more strict handling of host_offset can overcome this issue,
making the dtrace more reliable.

Regards,
Peter
---
Peter Ujfalusi (3):
  ASoC: SOF: ipc3-dtrace: Introduce SOF_DTRACE_INITIALIZING state
  ASoC: SOF: ipc3-dtrace: Add helper function to update the
    sdev->host_offset
  ASoC: SOF: ipc3-dtrace: Return from dtrace_read if there is no new
    data available

 sound/soc/sof/ipc3-dtrace.c | 47 ++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 9 deletions(-)

-- 
2.36.1

[PATCH 1/3] ASoC: SOF: ipc3-dtrace: Introduce SOF_DTRACE_INITIALIZING state

[Peter Ujfalusi]

With the new state we can make sure we are not missing the first
host_offset update.

In case the dtrace is small, the DMA copy will be fast and depending on
the moonphase it might be done before we set the sdev->dtrace_state  to
SOF_DTRACE_ENABLED.

The DMA will start the copy as soon as the host starts the DMA. Set the
dtrace to enabled before we let the DMA to run in order to avoid missing
the position update.

The new state is needed to cover architectures where the host side
snd_sof_dma_trace_trigger() is a NOP and the dtrace in the firmware is
ready as soon as the IPC message has been processed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/sof/ipc3-dtrace.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sof/ipc3-dtrace.c b/sound/soc/sof/ipc3-dtrace.c
index b4e1343f9138..9292ff7ce1e8 100644
--- a/sound/soc/sof/ipc3-dtrace.c
+++ b/sound/soc/sof/ipc3-dtrace.c
@@ -18,6 +18,7 @@
 enum sof_dtrace_state {
 	SOF_DTRACE_DISABLED,
 	SOF_DTRACE_STOPPED,
+	SOF_DTRACE_INITIALIZING,
 	SOF_DTRACE_ENABLED,
 };
 
@@ -32,6 +33,15 @@ struct sof_dtrace_priv {
 	enum sof_dtrace_state dtrace_state;
 };
 
+static bool trace_pos_update_expected(struct sof_dtrace_priv *priv)
+{
+	if (priv->dtrace_state == SOF_DTRACE_ENABLED ||
+	    priv->dtrace_state == SOF_DTRACE_INITIALIZING)
+		return true;
+
+	return false;
+}
+
 static int trace_filter_append_elem(struct snd_sof_dev *sdev, u32 key, u32 value,
 				    struct sof_ipc_trace_filter_elem *elem_list,
 				    int capacity, int *counter)
@@ -274,7 +284,7 @@ static size_t sof_wait_dtrace_avail(struct snd_sof_dev *sdev, loff_t pos,
 	if (ret)
 		return ret;
 
-	if (priv->dtrace_state != SOF_DTRACE_ENABLED && priv->dtrace_draining) {
+	if (priv->dtrace_draining && !trace_pos_update_expected(priv)) {
 		/*
 		 * tracing has ended and all traces have been
 		 * read by client, return EOF
@@ -445,6 +455,7 @@ static int ipc3_dtrace_enable(struct snd_sof_dev *sdev)
 	dev_dbg(sdev->dev, "%s: stream_tag: %d\n", __func__, params.stream_tag);
 
 	/* send IPC to the DSP */
+	priv->dtrace_state = SOF_DTRACE_INITIALIZING;
 	ret = sof_ipc_tx_message(sdev->ipc, &params, sizeof(params), &ipc_reply, sizeof(ipc_reply));
 	if (ret < 0) {
 		dev_err(sdev->dev, "can't set params for DMA for trace %d\n", ret);
@@ -452,17 +463,18 @@ static int ipc3_dtrace_enable(struct snd_sof_dev *sdev)
 	}
 
 start:
+	priv->dtrace_state = SOF_DTRACE_ENABLED;
+
 	ret = sof_dtrace_host_trigger(sdev, SNDRV_PCM_TRIGGER_START);
 	if (ret < 0) {
 		dev_err(sdev->dev, "Host dtrace trigger start failed: %d\n", ret);
 		goto trace_release;
 	}
 
-	priv->dtrace_state = SOF_DTRACE_ENABLED;
-
 	return 0;
 
 trace_release:
+	priv->dtrace_state = SOF_DTRACE_DISABLED;
 	sof_dtrace_host_release(sdev);
 	return ret;
 }
@@ -546,7 +558,7 @@ int ipc3_dtrace_posn_update(struct snd_sof_dev *sdev,
 	if (!sdev->fw_trace_is_supported)
 		return 0;
 
-	if (priv->dtrace_state == SOF_DTRACE_ENABLED &&
+	if (trace_pos_update_expected(priv) &&
 	    priv->host_offset != posn->host_offset) {
 		priv->host_offset = posn->host_offset;
 		wake_up(&priv->trace_sleep);
-- 
2.36.1

[PATCH 2/3] ASoC: SOF: ipc3-dtrace: Add helper function to update the sdev->host_offset

[Peter Ujfalusi]

We are using the READ_ONCE() on the debugfs read path for accessing
sdev->host_offset, but the set is not atomic or protected in any way.

Add a small helper to do the host_offset update and be really paranoid
about the a possible race in update

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/sof/ipc3-dtrace.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/ipc3-dtrace.c b/sound/soc/sof/ipc3-dtrace.c
index 9292ff7ce1e8..1f4d7a98c8fc 100644
--- a/sound/soc/sof/ipc3-dtrace.c
+++ b/sound/soc/sof/ipc3-dtrace.c
@@ -252,6 +252,21 @@ static int debugfs_create_trace_filter(struct snd_sof_dev *sdev)
 	return 0;
 }
 
+static bool sof_dtrace_set_host_offset(struct sof_dtrace_priv *priv, u32 new_offset)
+{
+	u32 host_offset = READ_ONCE(priv->host_offset);
+
+	if (host_offset != new_offset) {
+		/* This is a bit paranoid and unlikely that it is needed */
+		u32 ret = cmpxchg(&priv->host_offset, host_offset, new_offset);
+
+		if (ret == host_offset)
+			return true;
+	}
+
+	return false;
+}
+
 static size_t sof_dtrace_avail(struct snd_sof_dev *sdev,
 			       loff_t pos, size_t buffer_size)
 {
@@ -368,7 +383,7 @@ static int dfsentry_dtrace_release(struct inode *inode, struct file *file)
 
 	/* avoid duplicate traces at next open */
 	if (priv->dtrace_state != SOF_DTRACE_ENABLED)
-		priv->host_offset = 0;
+		sof_dtrace_set_host_offset(priv, 0);
 
 	return 0;
 }
@@ -444,7 +459,7 @@ static int ipc3_dtrace_enable(struct snd_sof_dev *sdev)
 	params.buffer.pages = priv->dma_trace_pages;
 	params.stream_tag = 0;
 
-	priv->host_offset = 0;
+	sof_dtrace_set_host_offset(priv, 0);
 	priv->dtrace_draining = false;
 
 	ret = sof_dtrace_host_init(sdev, &priv->dmatb, &params);
@@ -559,10 +574,8 @@ int ipc3_dtrace_posn_update(struct snd_sof_dev *sdev,
 		return 0;
 
 	if (trace_pos_update_expected(priv) &&
-	    priv->host_offset != posn->host_offset) {
-		priv->host_offset = posn->host_offset;
+	    sof_dtrace_set_host_offset(priv, posn->host_offset))
 		wake_up(&priv->trace_sleep);
-	}
 
 	if (posn->overflow != 0)
 		dev_err(sdev->dev,
-- 
2.36.1

[PATCH 3/3] ASoC: SOF: ipc3-dtrace: Return from dtrace_read if there is no new data available

[Peter Ujfalusi]

If no new trace data is available then return immediately, there is no
need to continue with the execution of the trace_read() function.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/sof/ipc3-dtrace.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/sof/ipc3-dtrace.c b/sound/soc/sof/ipc3-dtrace.c
index 1f4d7a98c8fc..f59931d818c1 100644
--- a/sound/soc/sof/ipc3-dtrace.c
+++ b/sound/soc/sof/ipc3-dtrace.c
@@ -353,6 +353,10 @@ static ssize_t dfsentry_dtrace_read(struct file *file, char __user *buffer,
 		return -EIO;
 	}
 
+	/* no new trace data */
+	if (!avail)
+		return 0;
+
 	/* make sure count is <= avail */
 	if (count > avail)
 		count = avail;
-- 
2.36.1

Re: [PATCH 0/3] ASoC: SOF: ipc3-dtrace: Handle race during initialization

[Mark Brown]

On Fri, 10 Jun 2022 11:01:16 +0300, Peter Ujfalusi wrote:
> This series handles the race which can result missing the first position update
> after the trace is enabled.
> In short: the firmware might send the position update (if we have enough
> trace data generated) after the dma-trace is enabled by the TRACE_DMA_PARAMS_EXT
> message. Depending on scheduling, load, preemption on Linux side we have seen
> that occasionally this first position update got missed and we missed reading it
> out.
> 
> [...]

Applied to

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

Thanks!

[1/3] ASoC: SOF: ipc3-dtrace: Introduce SOF_DTRACE_INITIALIZING state
      commit: 135786c32ed057068bec56f67a54064cfc845bde
[2/3] ASoC: SOF: ipc3-dtrace: Add helper function to update the sdev->host_offset
      commit: b66f9e703f0bee4e1aa7010299914b7b2009b4e0
[3/3] ASoC: SOF: ipc3-dtrace: Return from dtrace_read if there is no new data available
      commit: 1e90de2c9a40d7d0af5c7b0a6e2d362ffba94772

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