From 0ab79fe0c9f52823d052b8aac70fcd5f3ec6819b Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 11:21:38 -0500 Subject: [PATCH 1/9] ipc4: copier: fix return value on comp_trigger 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 --- src/audio/copier.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/audio/copier.c b/src/audio/copier.c index 53dd5a2d897a..a30334762515 100644 --- a/src/audio/copier.c +++ b/src/audio/copier.c @@ -409,7 +409,7 @@ static int copier_comp_trigger(struct comp_dev *dev, int cmd) if (cd->endpoint) ret = cd->endpoint->drv->ops.trigger(cd->endpoint, cmd); - return 0; + return ret; } /* copy and process stream data from source to sink buffers */ From 82d039c77cdf0c775650c1d1a65618f98f5876e0 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 12:10:59 -0500 Subject: [PATCH 2/9] ipc4: dai: fix NULL pointer dereference in error message 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 --- src/ipc/ipc4/dai.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ipc/ipc4/dai.c b/src/ipc/ipc4/dai.c index 3d112c31dc75..733345d7580c 100644 --- a/src/ipc/ipc4/dai.c +++ b/src/ipc/ipc4/dai.c @@ -69,8 +69,7 @@ int ipc_dai_data_config(struct comp_dev *dev) struct alh_pdata *alh; if (!dai) { - comp_err(dev, "dai_data_config(): no config set for dai %d type %d", - dai->dai_index, dai->type); + comp_err(dev, "dai_data_config(): no dai!\n"); return -EINVAL; } From 5e9ad7707b8f109db566bdd3ea390aa4a263e7c1 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 12:18:38 -0500 Subject: [PATCH 3/9] ipc4: dai: fix return values 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 --- src/ipc/ipc4/dai.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ipc/ipc4/dai.c b/src/ipc/ipc4/dai.c index 733345d7580c..36afb098efd7 100644 --- a/src/ipc/ipc4/dai.c +++ b/src/ipc/ipc4/dai.c @@ -138,7 +138,7 @@ int dai_config(struct comp_dev *dev, struct ipc_config_dai *common_config, struct ipc4_copier_module_cfg *copier_cfg = spec_config; struct dai_data *dd = comp_get_drvdata(dev); int size; - int ret = 0; + int ret; /* ignore if message not for this DAI id/type */ if (dd->ipc_config.dai_index != common_config->dai_index || @@ -184,6 +184,7 @@ int dai_config(struct comp_dev *dev, struct ipc_config_dai *common_config, if (ret < 0) { rfree(dd->dai_spec_config); dd->dai_spec_config = NULL; + return -EINVAL; } } From f130e36b9aea082f1a661511f95ee71376def6b4 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 12:06:55 -0500 Subject: [PATCH 4/9] ipc3: dai: avoid variable/function shadowing 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 --- src/ipc/ipc3/dai.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ipc/ipc3/dai.c b/src/ipc/ipc3/dai.c index 7d1628f742ce..3f920e924e00 100644 --- a/src/ipc/ipc3/dai.c +++ b/src/ipc/ipc3/dai.c @@ -90,9 +90,9 @@ int ipc_dai_data_config(struct comp_dev *dev) { struct dai_data *dd = comp_get_drvdata(dev); struct ipc_config_dai *dai = &dd->ipc_config; - struct sof_ipc_dai_config *dai_config = ipc_from_dai_config(dd->dai_spec_config); + struct sof_ipc_dai_config *config = ipc_from_dai_config(dd->dai_spec_config); - if (!dai_config) { + if (!config) { comp_err(dev, "dai_data_config(): no config set for dai %d type %d", dai->dai_index, dai->type); return -EINVAL; @@ -115,18 +115,18 @@ int ipc_dai_data_config(struct comp_dev *dev) return -EINVAL; } - switch (dai_config->type) { + switch (config->type) { case SOF_DAI_INTEL_SSP: /* set dma burst elems to slot number */ - dd->config.burst_elems = dai_config->ssp.tdm_slots; + dd->config.burst_elems = config->ssp.tdm_slots; break; case SOF_DAI_INTEL_DMIC: /* We can use always the largest burst length. */ dd->config.burst_elems = 8; comp_info(dev, "config->dmic.fifo_bits = %u config->dmic.num_pdm_active = %u", - dai_config->dmic.fifo_bits, - dai_config->dmic.num_pdm_active); + config->dmic.fifo_bits, + config->dmic.num_pdm_active); break; case SOF_DAI_INTEL_HDA: break; @@ -143,7 +143,7 @@ int ipc_dai_data_config(struct comp_dev *dev) /* As with HDA, the DMA channel is assigned in runtime, * not during topology parsing. */ - dd->stream_id = dai_config->alh.stream_id; + dd->stream_id = config->alh.stream_id; break; case SOF_DAI_IMX_SAI: COMPILER_FALLTHROUGH; @@ -166,7 +166,7 @@ int ipc_dai_data_config(struct comp_dev *dev) default: /* other types of DAIs not handled for now */ comp_warn(dev, "dai_data_config(): Unknown dai type %d", - dai_config->type); + config->type); break; } From 2018ad73e59bba1e1d0861c5c58c9946b1a0d0fe Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 11:28:25 -0500 Subject: [PATCH 5/9] pipeline: simplify handling of first pipeline 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 --- src/audio/pipeline/pipeline-schedule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/audio/pipeline/pipeline-schedule.c b/src/audio/pipeline/pipeline-schedule.c index 2ba83cab70bb..664a050c3d4f 100644 --- a/src/audio/pipeline/pipeline-schedule.c +++ b/src/audio/pipeline/pipeline-schedule.c @@ -157,6 +157,7 @@ void pipeline_schedule_triggered(struct pipeline_walk_context *ctx, struct list_item *tlist; struct pipeline *p; uint32_t flags; + bool first_pipe = false; /* * Interrupts have to be disabled while adding tasks to or removing them @@ -180,10 +181,10 @@ void pipeline_schedule_triggered(struct pipeline_walk_context *ctx, p = container_of(tlist, struct pipeline, list); if (pipeline_is_timer_driven(p)) { /* Use the first of connected pipelines to trigger */ - if (cmd >= 0) { + if (cmd >= 0 && !first_pipe) { p->trigger.cmd = cmd; p->trigger.host = ppl_data->start; - cmd = -EINVAL; + first_pipe = true; } } else { p->xrun_bytes = 0; From 59f8629e3fcc62081e7eb81d48fd152965199bd8 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 11:36:04 -0500 Subject: [PATCH 6/9] smart_amp: fix error handling 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 --- src/audio/smart_amp/smart_amp.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/audio/smart_amp/smart_amp.c b/src/audio/smart_amp/smart_amp.c index 555cee24f02f..9749fcd41fe7 100644 --- a/src/audio/smart_amp/smart_amp.c +++ b/src/audio/smart_amp/smart_amp.c @@ -240,10 +240,6 @@ static struct comp_dev *smart_amp_new(const struct comp_driver *drv, if (bs > 0 && bs < sizeof(struct sof_smart_amp_config)) { comp_err(dev, "smart_amp_new(): failed to apply config"); - - if (sad) - rfree(sad); - rfree(sad); goto error; } @@ -282,6 +278,8 @@ static struct comp_dev *smart_amp_new(const struct comp_driver *drv, return dev; error: + rfree(sad); + rfree(dev); return NULL; } From e4a420e12a0a735f4d80415f937601eb66cd30af Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 11:45:11 -0500 Subject: [PATCH 7/9] mediatek: mt8195: afe-drv: remove redundant initialization 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 --- src/drivers/mediatek/mt8195/afe-drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/drivers/mediatek/mt8195/afe-drv.c b/src/drivers/mediatek/mt8195/afe-drv.c index 3ae30e718c1b..f9461f55a449 100644 --- a/src/drivers/mediatek/mt8195/afe-drv.c +++ b/src/drivers/mediatek/mt8195/afe-drv.c @@ -232,7 +232,7 @@ unsigned int afe_memif_get_cur_position(struct mtk_base_afe *afe, int id) int afe_dai_set_config(struct mtk_base_afe *afe, int id, unsigned int channel, unsigned int rate, unsigned int format) { - struct mtk_base_afe_dai *dai = &afe->dais[id]; + struct mtk_base_afe_dai *dai; /* TODO 1. if need use dai->id to search target dai */ /* TODO 1. if need a status to control the dai status */ @@ -256,7 +256,7 @@ int afe_dai_set_config(struct mtk_base_afe *afe, int id, unsigned int channel, u int afe_dai_get_config(struct mtk_base_afe *afe, int id, unsigned int *channel, unsigned int *rate, unsigned int *format) { - struct mtk_base_afe_dai *dai = &afe->dais[id]; + struct mtk_base_afe_dai *dai; /* TODO 1. if need use dai->id to search target dai */ /* TODO 1. if need a status to control the dai status */ From edc4587f5f05286cebbd4964fb52ea41d5db0332 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 12:24:03 -0500 Subject: [PATCH 8/9] amd: renoir: clk: fix infinite loop 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 --- src/platform/amd/renoir/lib/clk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform/amd/renoir/lib/clk.c b/src/platform/amd/renoir/lib/clk.c index d05c870b3ace..75e203084011 100644 --- a/src/platform/amd/renoir/lib/clk.c +++ b/src/platform/amd/renoir/lib/clk.c @@ -81,7 +81,8 @@ static void acp_reg_write_via_smn(uint32_t reg_offset, ACP_SRBM_CYCLE_STS); if (!acp_srbm_cycle_sts.bits.srbm_clients_sts) return; - } delay_cnt--; + delay_cnt--; + } } static void get_response_from_smu(void) From 45d97cb5372538a5a89ca3106394bd42fc3d3b14 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 28 Sep 2021 12:27:32 -0500 Subject: [PATCH 9/9] amd: renoir: add error check 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 --- src/platform/amd/renoir/platform.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/platform/amd/renoir/platform.c b/src/platform/amd/renoir/platform.c index 491401c1045f..cc5d0a567755 100644 --- a/src/platform/amd/renoir/platform.c +++ b/src/platform/amd/renoir/platform.c @@ -87,13 +87,15 @@ int platform_init(struct sof *sof) clock_set_freq(CLK_CPU(cpu_get_id()), CLK_MAX_CPU_HZ); /* init DMA */ ret = acp_dma_init(sof); + if (ret < 0) + return -ENODEV; /* Init DMA platform domain */ sof->platform_dma_domain = dma_multi_chan_domain_init(&sof->dma_info->dma_array[0], sizeof(sof->dma_info->dma_array), PLATFORM_DEFAULT_CLOCK, true); scheduler_init_ll(sof->platform_dma_domain); - /* initialize the host IPC mechanims */ + /* initialize the host IPC mechanisms */ ipc_init(sof); - /* initialize the DAI mechanims */ + /* initialize the DAI mechanisms */ ret = dai_init(sof); if (ret < 0) return -ENODEV;