ASoC: SOF: trace: sdev->host_offset management changes by ujfalusi · Pull Request #3516 · thesofproject/linux

ujfalusi · 2022-03-14T11:02:43Z

Hi,

The READ_ONCE(sdev->host_offset) alone does nothing to prevent any race regarding to host_offset change race prevention.
Introduce a new helper to do this in a correct way and while at it, print out the new offset for us to see where we are.

Additional debug prints can be added to track what is copied to user space with a simple change:

diff--gita/sound/soc/sof/trace.cb/sound/soc/sof/trace.cindex76c763da5804..32c3c0d8a75e100644---a/sound/soc/sof/trace.c+++b/sound/soc/sof/trace.c
@@ -334,8+334,10 @@ staticssize_tsof_dfsentry_trace_read(structfile*file, char__user*buffer,
}
/* no new trace data */-if (!avail)
+if (!avail) {
+dev_dbg(sdev->dev, "trace: nothing to copy\n");
return0;
+	}
/* make sure count is <= avail */if (count>avail)
@@ -349,6+351,8 @@ staticssize_tsof_dfsentry_trace_read(structfile*file, char__user*buffer,
*/
snd_dma_buffer_sync(&sdev->dmatb, SNDRV_DMA_SYNC_CPU);
/* copy available trace data to debugfs */+dev_dbg(sdev->dev, "trace: copy to user from offset : %#llx, count: %#zx\n",
+lpos, count);
rem=copy_to_user(buffer, ((u8*)(dfse->buf) +lpos), count);
if (rem)
return-EFAULT;

plbossart · 2022-03-14T15:06:39Z


- if (ret == host_offset)
+ if (ret == host_offset) {
+ dev_dbg(sdev->dev, "trace: new host_offset: %#x\n", new_offset);


isn't this super-verbose? I wonder if this is going to fill the console with too many logs.

If the debug is enabled we are printing the IPC messages (two per message). With this change it will look like this:
ipc rx: 0x90020000: GLB_TRACE_MSG: DMA_POSITION trace: new host_offset: 0x7920 ipc rx done: 0x90020000: GLB_TRACE_MSG: DMA_POSITION
Instead of an empty indication that the position is updated we will know the value as well.
Then we have another print when the host_offset is reset to 0 (either because of closing the trace file or restart), but it is still one additional message.
It is not going to fill the console, but I always wondered what is the content behind the dtrace position update.
I can drop this print if you think we should not have it.

I will create a draft PR on top of this to print the trace file reads as well in hope that we can catch the empty trace with that and have some clue on what is going on.

ujfalusi · 2022-04-05T08:56:23Z

Changes since v1:

Rebased on sof-dev (ipc3-dtrace introduction)

ujfalusi · 2022-05-04T07:01:34Z

Changes since v2:

rebased on topic/sof-dev

plbossart · 2022-05-04T14:58:40Z

 	}

-	acpi_handle_info(handle, "Found %d idle states\n", last_index);
-


can we remove this? I don't know what problem this tries to fix.

certainly, this is needed for one of my DUTs, otherwise the dmesg is flooded with this message for some reason and I keep forgetting to drop this when I'm done testing before sending the PR.
I should have a hook to blow up when I try to push this patch to other branch than my wip...

plbossart · 2022-05-04T14:59:59Z


+static bool sof_dtrace_set_host_offset(struct snd_sof_dev *sdev, u32 new_offset)
+{
+	u32 host_offset = READ_ONCE(sdev->host_offset);


so this is going to conflict with your very own PR #3662 changes, specifically 024c2eb

Yes, I'm aware of that, they are of different topic and I don't want to mix them. I'm fine with fixing up either when the time comes.

plbossart · 2022-05-09T16:18:35Z

@ujfalusi rebase needed.

ujfalusi · 2022-05-10T11:53:24Z

Changes since v3:

dropped the unrelated patches
the acpi hack, the printing of the new offset value and the move of the dtrace status change to avoid missing position updates (PR ASoC: SOF: trace: Make sure we are not missing the first host_offset … #3523 is the proposed solution)

plbossart · 2022-05-17T21:46:40Z

Any other review? @dbaluta @bardliao @ranj063 @marc-hb

plbossart · 2022-05-19T12:40:09Z

@ujfalusi conflict?

ujfalusi · 2022-05-19T13:31:43Z

@ujfalusi conflict?

Yes, I know, I'll update it tomorrow.

…offset 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>

…ta available 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>

ujfalusi · 2022-05-19T18:58:04Z

Changes since v4:

rebased on sof-dev to resolve conflict.

ujfalusi requested review from jsarha and marc-hb March 14, 2022 11:02

ujfalusi requested review from juimonen, kv2019i and lyakh as code owners March 14, 2022 11:02

ujfalusi requested review from lgirdwood, plbossart and ranj063 March 14, 2022 11:04

plbossart reviewed Mar 14, 2022
View reviewed changes

ujfalusi force-pushed the peter/sof/pr/dtrace_print_offsets_01 branch from 07ccf0d to 6651821Compare April 5, 2022 08:02

ujfalusi requested review from ajitkupandey, bardliao, dbaluta, kuanhsuncheng and yaochunhung as code owners April 5, 2022 08:02

ujfalusi force-pushed the peter/sof/pr/dtrace_print_offsets_01 branch from 6651821 to 508e431Compare April 5, 2022 08:55

bardliao previously approved these changes Apr 6, 2022
View reviewed changes

ujfalusi mentioned this pull request May 4, 2022
ASoC/SoundWire: improve dmesg logs #3631
Merged

ujfalusi dismissed bardliao’s stale review via 75096dbMay 4, 2022 07:01

ujfalusi force-pushed the peter/sof/pr/dtrace_print_offsets_01 branch from 508e431 to 75096dbCompare May 4, 2022 07:01

plbossart requested changes May 4, 2022
View reviewed changes

ujfalusi force-pushed the peter/sof/pr/dtrace_print_offsets_01 branch from 75096db to debd209Compare May 10, 2022 11:51

plbossart previously approved these changes May 10, 2022
View reviewed changes

bardliao previously approved these changes May 19, 2022
View reviewed changes

ujfalusi added 2 commits May 19, 2022 21:56

ASoC: SOF: ipc3-dtrace: Return from dtrace_read if there is no new da…
9c037af
…ta available 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>

ujfalusi dismissed stale reviews from bardliao and plbossart via 9c037afMay 19, 2022 18:57

ujfalusi force-pushed the peter/sof/pr/dtrace_print_offsets_01 branch from debd209 to 9c037afCompare May 19, 2022 18:57

plbossart approved these changes May 19, 2022
View reviewed changes

bardliao approved these changes May 20, 2022
View reviewed changes

plbossart merged commit 8a9e909 into thesofproject:topic/sof-devMay 20, 2022

ujfalusi deleted the peter/sof/pr/dtrace_print_offsets_01 branch November 18, 2022 07:36

		}

		acpi_handle_info(handle, "Found %d idle states\n", last_index);

Uh oh!

Conversation

ujfalusi commented Mar 14, 2022

Uh oh!

plbossartMar 14, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusiMar 16, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusiMar 16, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Apr 5, 2022

Uh oh!

ujfalusi commented May 4, 2022

Uh oh!

plbossartMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusiMay 10, 2022

Choose a reason for hiding this comment

Uh oh!

plbossartMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusiMay 10, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 9, 2022

Uh oh!

ujfalusi commented May 10, 2022

Uh oh!

plbossart commented May 17, 2022

Uh oh!

plbossart commented May 19, 2022

Uh oh!

ujfalusi commented May 19, 2022

Uh oh!

ujfalusi commented May 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants