Skip to content

Cppcheck fixes#4818

Merged
lgirdwood merged 9 commits into
thesofproject:mainfrom
plbossart:fix/cppcheck-again
Sep 30, 2021
Merged

Cppcheck fixes#4818
lgirdwood merged 9 commits into
thesofproject:mainfrom
plbossart:fix/cppcheck-again

Conversation

@plbossart
Copy link
Copy Markdown
Member

@plbossart plbossart commented Sep 28, 2021 •
edited
Loading

Somehow developers don't seem to like static analysis. Come on people, use tools!

cppcheck --platform=unix64 --force --max-configs=1024 --inconclusive --enable=all --suppress=variableScope --suppress=shiftTooManyBitsSigned --suppress=arithOperationsOnVoidPointer --suppress=bitwiseOnBoolean src

The findings are rather spectacular this time! I also filed issues #4817 and #4816 when I didn't know how to fix the code.

@plbossart
ipc4: copier: fix return value on comp_trigger
0ab79fe 
cppcheck warning:

src/audio/copier.c:410:7: style: Variable 'ret' is assigned a value
that is never used. [unreadVariable]
  ret = cd->endpoint->drv->ops.trigger(cd->endpoint, cmd);
      ^

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
ipc4: dai: fix NULL pointer dereference in error message
82d039c 
cppcheck warning:

Checking src/ipc/ipc4/dai.c ...
src/ipc/ipc4/dai.c:73:5: warning: Either the condition '!dai' is
redundant or there is possible null pointer dereference:
dai. [nullPointerRedundantCheck]
    dai->dai_index, dai->type);
    ^

src/ipc/ipc4/dai.c:71:6: note: Assuming that condition '!dai' is not redundant
 if (!dai) {
     ^
src/ipc/ipc4/dai.c:73:5: note: Null pointer dereference
    dai->dai_index, dai->type);
    ^

simplify error message to avoid dereferences.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
ipc4: dai: fix return values
5e9ad77 
src/ipc/ipc4/dai.c:142:10: style: Variable 'ret' is assigned a value
that is never used. [unreadVariable]

 int ret = 0;
         ^

The init is not necessary, and in addition there's a missing return
when memcpy_s fails.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
ipc3: dai: avoid variable/function shadowing
f130e36 
cppcheck throws the following warning:

src/ipc/ipc3/dai.c:93:29: style: Local variable 'dai_config' shadows
outer function [shadowFunction]
 struct sof_ipc_dai_config *dai_config =
 ipc_from_dai_config(dd->dai_spec_config);
                            ^

src/ipc/ipc3/dai.c:250:5: note: Shadowed declaration
int dai_config(struct comp_dev *dev, struct
ipc_config_dai *common_config,
    ^
src/ipc/ipc3/dai.c:93:29: note: Shadow variable
 struct sof_ipc_dai_config *dai_config =
 ipc_from_dai_config(dd->dai_spec_config);

All other uses of 'struct sof_ipc_dai_config' use 'config', so let's
align the style.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
pipeline: simplify handling of first pipeline
2018ad7 
src/audio/pipeline/pipeline-schedule.c:186:6: style: Assignment of
function parameter has no effect outside the
function. [uselessAssignmentArg]
     cmd = -EINVAL;
     ^

The code isn't wrong but cppcheck points at a possible confusion. It's
better and more explicit to use a dedicated boolean flag than
modifying the command code.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
smart_amp: fix error handling
59f8629 
cppcheck throws the following warning:

Checking src/audio/smart_amp/smart_amp.c ...
src/audio/smart_amp/smart_amp.c:236:2: warning: Either the condition
'if(sad)' is redundant or there is possible null pointer dereference:
sad. [nullPointerRedundantCheck]
 sad->ipc_config = *ipc_sa;
 ^
src/audio/smart_amp/smart_amp.c:244:6: note: Assuming that condition
 'if(sad)' is not redundant
  if (sad)
     ^
src/audio/smart_amp/smart_amp.c:236:2: note: Null pointer dereference
 sad->ipc_config = *ipc_sa;

 ^

The following code is indeed completely wrong:

               if (sad) << already tested so always true
                       rfree(sad);
               rfree(sad); << wrong variable

Fix by adding the relevant rfree on the error path

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
mediatek: mt8195: afe-drv: remove redundant initialization
e4a420e 
cppcheck warning:

Checking src/drivers/mediatek/mt8195/afe-drv.c ...
src/drivers/mediatek/mt8195/afe-drv.c:245:6: style: Redundant
initialization for 'dai'. The initialized value is overwritten before
it is read. [redundantInitialization]
 dai = &afe->dais[id];
     ^
src/drivers/mediatek/mt8195/afe-drv.c:235:31: note: dai is initialized
 struct mtk_base_afe_dai *dai = &afe->dais[id];
                              ^
src/drivers/mediatek/mt8195/afe-drv.c:245:6: note: dai is overwritten
 dai = &afe->dais[id];
     ^
src/drivers/mediatek/mt8195/afe-drv.c:269:6: style: Redundant
initialization for 'dai'. The initialized value is overwritten before
it is read. [redundantInitialization]
 dai = &afe->dais[id];
     ^
src/drivers/mediatek/mt8195/afe-drv.c:259:31: note: dai is initialized
 struct mtk_base_afe_dai *dai = &afe->dais[id];
                              ^
src/drivers/mediatek/mt8195/afe-drv.c:269:6: note: dai is overwritten
 dai = &afe->dais[id];
     ^

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
amd: renoir: clk: fix infinite loop
edc4587 
cppcheck warning:

Checking src/platform/amd/renoir/lib/clk.c ...
src/platform/amd/renoir/lib/clk.c:79:19: style: Condition
'delay_cnt>0' is always true [knownConditionTrueFalse]
 while (delay_cnt > 0) {
                  ^
src/platform/amd/renoir/lib/clk.c:63:23: note: Assignment
'delay_cnt=10000', assigned value is 10000
 uint32_t delay_cnt = 10000;
                      ^
src/platform/amd/renoir/lib/clk.c:79:19: note: Condition 'delay_cnt>0'
is always true
 while (delay_cnt > 0) {
                  ^
src/platform/amd/renoir/lib/clk.c:43:13: style: Variable 'reg_value'
is assigned a value that is never used. [unreadVariable]
  reg_value = 0;
            ^
src/platform/amd/renoir/lib/clk.c:77:21: style: Variable
'acp_srbm_cycle_sts' is assigned a value that is never
used. [unreadVariable]
 acp_srbm_cycle_sts
 = (acp_srbm_cycle_sts_t)io_reg_read(PU_REGISTER_BASE +
 ACP_SRBM_CYCLE_STS);
                    ^
src/platform/amd/renoir/lib/clk.c:84:13: style: Variable 'delay_cnt' is assigned a value that is never used. [unreadVariable]
 } delay_cnt--;
            ^

the delay_cnt variable is obviously in the wrong position.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
amd: renoir: add error check
45d97cb 
cppcheck warning:

Checking src/platform/amd/renoir/platform.c ...
src/platform/amd/renoir/platform.c:97:6: style: Variable 'ret' is
reassigned a value before the old one has been
used. [redundantAssignment]
 ret = dai_init(sof);
     ^
src/platform/amd/renoir/platform.c:89:6: note: ret is assigned
 ret = acp_dma_init(sof);
     ^
src/platform/amd/renoir/platform.c:97:6: note: ret is overwritten
 ret = dai_init(sof);
     ^

Also fix two typos while we're at it.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
RanderWang
RanderWang approved these changes Sep 29, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lgirdwood
lgirdwood approved these changes Sep 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's possible to run cppcheck in CI, I would assume it cant be run on patches but on whole source files ?

@lgirdwood
Copy link
Copy Markdown
Member

lgirdwood commented Sep 29, 2021

CI has stalled. Restart.

@lgirdwood
Copy link
Copy Markdown
Member

lgirdwood commented Sep 29, 2021

SOFCI TEST

bhiregoudar
bhiregoudar reviewed Sep 29, 2021
View reviewed changes
Comment thread src/audio/pipeline/pipeline-schedule.c
if (pipeline_is_timer_driven(p)) {
/* Use the first of connected pipelines to trigger */
if (cmd >= 0) {
if (cmd >= 0 && !first_pipe) {
Copy link
Copy Markdown

@bhiregoudar bhiregoudar Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the cmd is being checked for >= 0 here? control won't come here if the switch case doesn't match

Copy link
Copy Markdown
Member Author

@plbossart plbossart Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm, yes, maybe we only need the !first _pipe. @lyakh would you concur?

Copy link
Copy Markdown
Collaborator

@lyakh lyakh Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't find this "comment by cppcheck" very useful... yes, cmd is used as a local variable here, it absolutely isn't expected to have any effect outside of this function. But ok, using a separate variable makes this more explicit. But otherwise - yes, after this change the cmd >= 0 test can be removed there. But first_pipe also seems inverted to me: it is false while the loop is handling the first pipeline and it becomes true for the second and any further pipelines. Shouldn't this be inverted?

Copy link
Copy Markdown
Member Author

@plbossart plbossart Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I still don't know my left from my right... will fix, thanks @lyakh

Copy link
Copy Markdown
Member Author

@plbossart plbossart Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged too quickly @lgirdwood, i'll send a follow-up to fix this.

Copy link
Copy Markdown
Member Author

@plbossart plbossart Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in #4832

@plbossart
Copy link
Copy Markdown
Member Author

plbossart commented Sep 29, 2021

Not sure if it's possible to run cppcheck in CI, I would assume it cant be run on patches but on whole source files ?

same as for everything else, you have to pull the entire tree and then run a bunch of commands. CI doesn't work on patches alone.

cppcheck is far from perfect, specifically it throws lots of false positives on unions. I don't think it's a candidate for CI, more of a maintainer tool run on a recurring schedule and before releases...

@lgirdwood
Copy link
Copy Markdown
Member

lgirdwood commented Sep 30, 2021

CI shows unrelated issue on BDW.

@lgirdwood lgirdwood merged commit b1d88a8 into thesofproject:main Sep 30, 2021
plbossart added a commit to plbossart/sof that referenced this pull request Sep 30, 2021
@plbossart
pipeline: fix handling of first pipeline
dcd3a93 
the logic was just wrong as discussed in
thesofproject#4818 (comment)

remove test on cmd and flip logic for first_pipe

Fixes: 107b60e ('pipeline: simplify handling of first pipeline')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart mentioned this pull request Sep 30, 2021
Merged
lgirdwood pushed a commit that referenced this pull request Oct 1, 2021
@plbossart @lgirdwood
pipeline: fix handling of first pipeline
75cfcd8 
the logic was just wrong as discussed in
#4818 (comment)

remove test on cmd and flip logic for first_pipe

Fixes: 107b60e ('pipeline: simplify handling of first pipeline')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulstelian97 paulstelian97 paulstelian97 approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@bhiregoudar bhiregoudar bhiregoudar approved these changes

@sunilkumardommati sunilkumardommati sunilkumardommati approved these changes

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lyakh lyakh Awaiting requested review from lyakh

@balakishorepati balakishorepati Awaiting requested review from balakishorepati

@kuanhsuncheng kuanhsuncheng Awaiting requested review from kuanhsuncheng kuanhsuncheng is a code owner

+1 more reviewer

@RanderWang RanderWang RanderWang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@plbossart @lgirdwood @lyakh @paulstelian97 @RanderWang @bhiregoudar @sunilkumardommati