A compiler warning and cosmetic improvements #4216
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Several code paths use the existing clock_ms_to_ticks() function in some not ideal ways, potentially leading to computation overhead or to precision loss. The function is often called to calculate ticks for 1 millisecond, which then is recalculated to the required time interval. It's better to let the function calculate the number of ticks for the required time interval directly. E.g. instead of clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1) * milliseconds it's better to call clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, milliseconds) directly. For microseconds however replacing clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1) * microseconds / 1000 with clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, microseconds / 1000) can lead to a loss of precision. To avoid that a new function clock_us_to_ticks() is added. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
If one of the "if" branches contains a jump instruction like "goto," "return," "break" etc. usually making that the only "if" branch simplifies the code. Fix one such case in schedule_ll_task(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When built under Zephyr the wrong format generates a compiler warning: modules/audio/sof/zephyr/include/sof/trace/trace.h:35:11: warning: \ format '%d' expects argument of type 'int', but argument 3 has \ type 'uint64_t' Since native SOF tracing cannot print 64-bit data, only print the low 32 bits of the time interval. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The hdr variable in ipc_compact_read_msg() initially points to data on stack, and then that variable is returned from the function, creating an impression of a bug. However, that isn't a bug because it is only returned from the function if it is overwritten to point to a different data block, this time not on stack. Trivially simplify the function to eliminate the confusion. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Remove redundant ret initialisation in two versions of schedule_task_init_edf() and in schedule_task_init_ll(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The list isn't modified in schedule_ll_clients_reschedule() when scanning it for the earliest task, replace list_for_item_safe() with the usual list_for_item(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
schedule_ll_task_cancel() looks for a task on the list to cancel it. However, if it doesn't find the task on the list it still sets its status and deletes it from the list. Currently this doesn't cause any problems, but it's potentially dangerous. This function can be called for completed tasks, which are no longer on the list. So list_item_del() will be called for the second time for it. Currently this isn't a problem because SOF implementation of that function also initialises the list head, but not all doubly-linked list implementations do that. E.g. Zephyr's sys_dlist_remove() nullifies both .next and .prev, so, a repeated call to it would cause a NULL dereference. In general it's safer to only modify an element when it has been verified to be valid. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
In scheduler_free_ll() sch->tasks is a list head, not a list item. Use list_init() instead of list_item_del() to initialise it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| comp_info(dev, "kpb_init_draining(): sync_draining_mode selected with interval %u [uS].", | ||
| (unsigned int)(drain_interval * 1000 / | ||
| clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1))); | ||
| } else { |
Contributor
There was a problem hiding this comment.
@lyakh I'm also having some warnings, on trace, when building SOF with Zephyr:
In file included from sof/sof/zephyr/../src/include/sof/lib/dai.h:27,
from sof/sof/src/lib/dai.c:7:
sof/sof/src/lib/dai.c: In function 'dai_group_get':
sof/sof/zephyr/include/sof/trace/trace.h:35:4: warning: implicit declaration of function 'printk'; did you mean 'cbprintf'? [-Wimplicit-function-declaration]
35 | printk("%llu: " format "\n", platform_timer_get(NULL), \
| ^~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:287:2: note: in expansion of macro '_log_message'
287 | _log_message(trace_log_filtered, true, LOG_LEVEL_CRITICAL, class, ctx, id_1, \
| ^~~~~~~~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:290:2: note: in expansion of macro '_trace_error_with_ids'
290 | _trace_error_with_ids(class, ctx, id_1, id_2, format, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:374:2: note: in expansion of macro 'trace_error_with_ids'
374 | trace_error_with_ids(_TRACE_INV_CLASS, ctx, \
| ^~~~~~~~~~~~~~~~~~~~
sof/sof/src/lib/dai.c:85:3: note: in expansion of macro 'tr_err'
85 | tr_err(&dai_tr, "dai_group_get(): invalid group_id %u",
| ^~~~~~
Any idea how can I get rid of them?
Do I need to do some LOGGING configuration for Zephyr?
Thanks!
Collaborator
Author
There was a problem hiding this comment.
@iuliana-prodan in fact that looks like a bug in zephyr/include/sof/trace/trace.h to me. It should include sys/printk.h under #ifdef __ZEPHYR__, would you like to submit a fix?
Contributor
There was a problem hiding this comment.
@lyakh ok, sure, I can do it.
Let me check it and I'll submit the fix.
Thanks!
Contributor
lgirdwood
reviewed
May 20, 2021
lgirdwood
approved these changes
May 20, 2021
Collaborator
Author
|
SOFCI TEST |
iuliana-prodan
approved these changes
May 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The compiler warning only triggers in Zephyr builds, others are all cosmetic improvements