Uh oh!
There was an error while loading. Please reload this page.
More cosmetic fixes and clean up - #1025
Conversation
There was a problem hiding this comment.
This one is trickier here, because depending on the CONFIG_TASK_HAVE_PRIORITY_XXX you can end up with memory leaks.
But looking at the code it looks that it doesn't really matter if those allocation failed.
There was a problem hiding this comment.
I think it was already mentioned somewhere, that it looks like KW problem, because fail to alloc from system heap always ends in panic.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
rmalloc_sys() panics if there is no free memory to allocate which practically makes the if(null) a dead code in every place that follows an allocation from RZONE_SYS, and what is more, it suggests the reader that the allocation may really fail at run-time being handled by the callers. If not sure why KW is unable to analyze that path and follow switch (zone & RZONE_TYPE_MASK) correctly.
I am also not sure why the _malloc() just traces error in the switch() default branch and returns NULL ptr since it seems to be a serious logic error, not a run-time problem to be handled in some way. This branch seems to confuse KW. I'd try another approach: panic in the default branch in _malloc() and mark this infinite loop as 'by design' piece to neutralize KW.
lyakh
commented
Feb 21, 2019
lgirdwood
commented
Feb 21, 2019
@lyakh looks like CI builds have failed. |
mmaka1
commented
Feb 25, 2019
@lyakh Could you plase rebase with master, KW should be OK now wrt sys heap allocations. |
Add a missing preamble in a recently created idc.c Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Interrupt registration can fail, check its return code in idc.c and propagate error to the caller. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The SMP version of init.c calls panic(), therefore it needs panic.h, OTOH the UP version doesn't call panic(), so the header isn't needed there. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When returning an error, it's usually better to propagate the error code, that caused the termination, than overriding it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
commented
Mar 13, 2019
I rebased and extended this series, also moved here cosmetic patches from my interrupt extension PR #1053 |
All trace_event() style macros resolve to _log_message() or __log_message() eventually. This message makes those macros usable as a function, e.g. within "if () else" clauses with or without curly braces. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Using macro arguments multiple times within the macro definition body can lead to repeated expressioon evaluation. To avoid that replace MIN() and MAX() macros with safer versions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Function-type macros are usually called as "M(...);" with a semicolon in the end. Therefore defining such macros with a semicolon isn't a good practice. This breaks constructs like if (...) M(...); else ...; Fix the dma_set_drvdata() macro to avoid such problems. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Some editors (e.g. emacs) do not take into account preprocessor
conditionals when performing syntax highlighting. I.e. patterns like
if (a) {
if (b) {
do_things();
}
cause those editors to miscalculate the number of opening and closing
braces and thus break code highlighting. Move the opening brace to
after the preprocessor conditional to avoid that.
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>Rearrange a conditional to reduce the number of branches and eliminate a goto. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
As long as the low bits of two numbers are 0, there's no need to right shift those numbers to compare them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Use a "for" loop instead of simulating it, using a "while" loop. Also avoid needlessly setting a variable inside that loop, where it can just be set once after the loop termination. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
"Formate" and "format" are two different words, in this case it's "format" that is meant. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| j = 0; | ||
| /* Loop until NULL */ | ||
| while (fir_list[j]) { | ||
| for (j = 0; fir_list[j]; j++) { |
There was a problem hiding this comment.
I suppose this works but what is the improvement?
There was a problem hiding this comment.
I find
for (i = 0; i < N; i++)
better than
i = 0;
while (i < N) {
...
i++;
}
| trace_dmic("dmic_set_config(), cfg mfir_a = %u, mfir_b = %u", | ||
| cfg.mfir_a, cfg.mfir_b); | ||
| trace_dmic("dmic_set_config(), cfg cic_shift = %u", cfg.cic_shift) | ||
| trace_dmic("dmic_set_config(), cfg cic_shift = %u", cfg.cic_shift); |
There was a problem hiding this comment.
No wonder indenting in editor has behaved strange. Thanks!
| #define MIN(a, b) (((a) < (b)) ? (a) : (b)) | ||
| #define MAX(a, b) (((a) > (b)) ? (a) : (b)) | ||
| #define MIN(a, b) ({ \ |
There was a problem hiding this comment.
I'm curious, why is this better?
There was a problem hiding this comment.
if you have MIN(x++, y), then with the first version you'd get
(x++ > y) ? x++ : y;
i.e. you'd have x incremented twice. With the second version you'd correctly have it incremented once. Same for any MIN(f(x), y) - f(x) would be called twice with the first version.
A couple of cosmetic fixes and improvements. They don't have anything to do with KW any more, since the only patch, that was dealing with a KW complaint, was fixing a false positive - thanks to all for pointing out! So, all what's left is just clean up and cosmetic stuff.