Runtime trace filtration

[ktrzcinx]

Add possibility to filter received trace messages (from sof-logger) in runtime. It improve debugging experience, allow to easily skip uninteresting messages.
Kernel part is to receive new trace settings in strictly formatted text from sof-logger and send it to firmware in IPC message.
This feature will need component alignment between shown topology representation and logger output, but it may be done separately.

FW part: thesofproject/sof#3329
logger part: thesofproject/sof#2965
issue: thesofproject/sof#2172

[plbossart]

Sounds like a good idea but needs a bit of cleanup, e.g. memory allocation doesn't seem quite right

[lyakh]

@plbossart @lgirdwood I'm not sure I fully appreciate the usefulness of this. We're adding a new user ABI that we'll have to maintain, version, secure against abuse etc. Besides we'll start getting partial logs, which will have us wonder where messages X and Y are gone. Wouldn't it be better to have simple 2-3 verbosity levels and otherwise use grep for any further filtering?

[kv2019i]

Mostly covered by other reviewers but a few more comments.

I wonder if we'd need to document the interface given this is more than just a readonly debugfs node. OTOH, sof-logger is open-source, so maybe that is sufficient in this case.

[ranj063]

@plbossart @lgirdwood I'm not sure I fully appreciate the usefulness of this. We're adding a new user ABI that we'll have to maintain, version, secure against abuse etc. Besides we'll start getting partial logs, which will have us wonder where messages X and Y are gone. Wouldn't it be better to have simple 2-3 verbosity levels and otherwise use grep for any further filtering?

@lyakh while I understand your concern, I think this will be quite a useful feature, at least for developers. In the past, I've struggled while trying to debug one particular component and enabling verbose trace for just that one component was not possible.

[plbossart]

@plbossart @lgirdwood I'm not sure I fully appreciate the usefulness of this. We're adding a new user ABI that we'll have to maintain, version, secure against abuse etc. Besides we'll start getting partial logs, which will have us wonder where messages X and Y are gone. Wouldn't it be better to have simple 2-3 verbosity levels and otherwise use grep for any further filtering?

@lyakh while I understand your concern, I think this will be quite a useful feature, at least for developers. In the past, I've struggled while trying to debug one particular component and enabling verbose trace for just that one component was not possible.

Agree with @ranj063. See e.g. how graphics and ACPI added 2 sets of flags for debug_layer and debug_level. if you really want verbose stuff on a specific component and squelch other issues, there's no real alternative.

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/method-tracing.html

[lyakh]

@lyakh while I understand your concern, I think this will be quite a useful feature, at least for developers. In the past, I've struggled while trying to debug one particular component and enabling verbose trace for just that one component was not possible.

Agree with @ranj063. See e.g. how graphics and ACPI added 2 sets of flags for debug_layer and debug_level. if you really want verbose stuff on a specific component and squelch other issues, there's no real alternative.

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/method-tracing.html

@ranj063 @plbossart if it is for developers, wouldn't compile-time configuration flags be enough? Do we need it at run-time?

[plbossart]

@lyakh while I understand your concern, I think this will be quite a useful feature, at least for developers. In the past, I've struggled while trying to debug one particular component and enabling verbose trace for just that one component was not possible.

Agree with @ranj063. See e.g. how graphics and ACPI added 2 sets of flags for debug_layer and debug_level. if you really want verbose stuff on a specific component and squelch other issues, there's no real alternative.
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/method-tracing.html

@ranj063 @plbossart if it is for developers, wouldn't compile-time configuration flags be enough? Do we need it at run-time?

You're forgetting about devices with the Intel production key, we don't necessarily have the freedom to recompile firmware in all cases.

[kv2019i]

Good discussion in the review so far. Most issues are already covered, but I did have a few points that I think still need attention.

[kv2019i]

@ktrzcinx Hmm, I think declaring "FIN" in the same enumeration and same style name is confusing many people (including me). You are essential defining bitfields within the 32bit key, with 7bit for element type and 1bit for the FIN end marker, and rest of the 24bits reserved.

There is some unavoidable abstraction overload, but how about:

bit6..0:
SOF_IPC_TRACE_FILTER_ELEM_TYPE_LEVEL
SOF_IPC_TRACE_FILTER_ELEM_TYPE_UUID
SOF_IPC_TRACE_FILTER_ELEM_TYPE_COMP
SOF_IPC_TRACE_FILTER_ELEM_TYPE_PIPE
SOF_IPC_TRACE_FILTER_ELEM_TYPE_MASK

bit7:
SOF_IPC_TRACE_FILTER_ELEM_FIN

This seems easier to follow for at least me. The TYPE is still a bit confusing as this is "type of filter action" and not type of a DSP element, but yeah, best I could come up.

[plbossart]

LGTM now, thanks @ktrzcinx

[lyakh]

if we return an error code from trace_debugfs_create(), snd_sof_init_trace() will fail too and leave the system with no SOF tracing. This isn't fatal for SOF probing / functionality, but maybe it's better to fail filter initialisation "silently." In fact I'd rather change snd_sof_init_trace() to not fail if trace_debugfs_create() fails, but that's something for a different PR. And anyway, if the system fails such functions, which in this case means it's really low on memory, many other things will fail too...

[ktrzcinx]

@lyakh as you have spotted, possibility of failure trace_debugfs_filter_create() in fully functional system in quite low, and even if it will be 'silent failure', then following line is going to fail too (dfse = devm_kzalloc(sdev->dev, sizeof(*dfse), GFP_KERNEL)), so system will be without SOF tracing at all. In my opinion it's not an argument to drop return code check and relay on other things fails. I can replace return ret with dev_err, but because of low fail probability, it shouldn't change much for end-user.

[kv2019i]

@ktrzcinx Two checkpatch warnings (u32 usage is ok), can you look at these:

WARNING: line length of 103 exceeds 100 columns
#52: FILE: include/sound/sof/trace.h:49:
+#define SOF_IPC_TRACE_FILTER_ELEM_SET_LEVEL 0x01 /**< new trace level for selected components */

WARNING: Prefer using '"%s...", func' to using 'sof_dfsentry_trace_filter_write', this function's name, in a string
#247: FILE: sound/soc/sof/trace.c:175:

•   dev_err(sdev->dev, "sof_dfsentry_trace_filter_write too long input, %zu > %d\n",
  

[ktrzcinx]

ping, FW part is already merged

[kv2019i]

Thanks @ktrzcinx , looks good now.