ipc4: ace: Add dynamic log level control via IPC with runtime filtering #10536
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,14 +140,42 @@ int ipc4_logging_enable_logs(bool first_block, | |
| log_state = (const struct ipc4_log_state_info *)data; | ||
|
|
||
| if (log_state->enable) { | ||
| uint32_t log_level = LOG_LEVEL_NONE; /* Default if no bits set */ | ||
| uint32_t mask = log_state->logs_mask[0]; | ||
|
|
||
| /* TODO: Improve mask handling for better code maintainability | ||
| * The logs_mask bits should be defined using proper macros or a struct | ||
| * to improve readability and maintainability. Current hardcoded bit | ||
| * positions are sufficient for now but should be refactored in the future. | ||
| * Possible improvements: | ||
| * - Define IPC4_LOG_MASK_* macros for each bit position | ||
| * - Create a struct with bitfields for each priority level | ||
| * - Add proper documentation in IPC4 specification headers | ||
| */ | ||
|
|
||
| /* Determine log level from mask bits 0-4 (priority levels) | ||
| * bit 0: critical & error -> LOG_LEVEL_ERR | ||
| * bit 1: high & warning -> LOG_LEVEL_WRN | ||
| * bit 2: medium -> LOG_LEVEL_INF | ||
| * bit 3: low & info -> LOG_LEVEL_INF | ||
| * bit 4: verbose & debug -> LOG_LEVEL_DBG | ||
| * Check highest bit set to determine maximum log level | ||
| */ | ||
| if (mask & BIT(4)) | ||
| log_level = LOG_LEVEL_DBG; | ||
| else if (mask & (BIT(3) | BIT(2))) | ||
| log_level = LOG_LEVEL_INF; | ||
| else if (mask & BIT(1)) | ||
| log_level = LOG_LEVEL_WRN; | ||
| else if (mask & BIT(0)) | ||
| log_level = LOG_LEVEL_ERR; | ||
|
Collaborator
There was a problem hiding this comment. We need to think about how to introduce this. Current shipping Linux kernels do not set log_mask and we cannot retroactively change shipped kernels (update of FW must not break user systems with older kernel), so this would effectively break mtrace for all existing systems as log level is forced to LOG_LEVEL_NONE. I think this needs to be a a build option (so we can keep the old behaviour in Linux overlay) and/or the default needs to be LOG_LEVEL_INF (like it is now). Looping in our kernel folks @bardliao @ujfalusi @ranj063
Contributor
Author
There was a problem hiding this comment. Why does FW that doesn't unnecessarily spam with logs mean breaking the user's system?
Collaborator
There was a problem hiding this comment. @tmleman I wasn't claiming that. Just saying this effectively breaks mtrace for existing systems. By default logs are disabled (no enable sent by host), but we have documented flow to set a kernel module parameter. If set, kernel will enable mtrace (no log mask) and mtrace is expected to work (also in release firmware files). As I understand this change, after this PR, there's no way to get mtrace working with current Linux kernel and to get FW logs out, one needs a new kernel that can set the log_mask to some non-zero value. Or did I miss something?
Member
There was a problem hiding this comment. Its probably best to have a Kconfig=level for Linux based OS to boot with current level and then have kernel tune after FW boot if required. This can be a debugFS file @ujfalusi |
||
|
|
||
| adsp_mtrace_log_init(mtrace_log_hook); | ||
| /* Initialize work queue if not already initialized */ | ||
| if (!log_work.work.handler) | ||
| k_work_init_delayable(&log_work, log_work_handler); | ||
|
|
||
tmleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| log_backend_enable(log_backend, mtrace_log_hook, log_level); | ||
|
Contributor
There was a problem hiding this comment. Does it make sense to enable backend for LOG_LEVEL_NONE?
Contributor
Author
There was a problem hiding this comment. First of all, this doesn't seem to be illegal (pointless, but not illegal). This IPC properly implemented using what Zephyr logger offers will allow, for example, to silence all logs for a specified backend and then set them to a higher level only for a specified source. And in such a scenario, enabling with level set to NONE makes perfect sense. |
||
|
|
||
| mtrace_aging_timer = log_state->aging_timer_period; | ||
| if (mtrace_aging_timer < IPC4_MTRACE_AGING_TIMER_MIN_MS) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either I don't understand something or something isn't working. This means, that only ERR level logs should appear in mtrace, right? But if you check this PR logs it's still the usual INFO. If indeed this is trying to set log level to ERR, is this a good idea? We need INF level logs in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment, according to my knowledge, everything works as I intended. The default log filtering level is set to ERROR at startup. This doesn't change the log level that gets compiled in, it still remains INFO. The fact that CI assumes that logs will appear in the memory window without sending IPC which is supposed to enable them, I've known for some time (I once wanted to disable logger backend autostart). The FW used in CI is built with the
app/debug_overlay.confand there runtime filtering is disabled to maintain the existing behavior expected by CI.