[alsa-devel] [PATCH 0/6] Small fixes
Series of small fixes: * fixes few issues found during checking code with static checkers * fix few prints * define function in header, like it should be * release topology when done with it
Amadeusz Sławiński (6): ASoC: Intel: Skylake: Use correct function to access iomem space ASoC: Intel: Fix use of potentially uninitialized variable ASoC: dapm: Expose snd_soc_dapm_new_control_unlocked properly ASoC: Intel: Skylake: Print module type instead of id ASoC: Intel: Skylake: Release topology when we are done with it ASoC: Intel: NHLT: Fix debug print format
include/sound/soc-dapm.h | 3 +++ sound/soc/intel/common/sst-ipc.c | 2 ++ sound/soc/intel/skylake/skl-debug.c | 2 +- sound/soc/intel/skylake/skl-messages.c | 5 +++-- sound/soc/intel/skylake/skl-nhlt.c | 2 +- sound/soc/intel/skylake/skl-topology.c | 20 ++++++++++---------- sound/soc/intel/skylake/skl.h | 1 - sound/soc/soc-topology.c | 6 ------ 8 files changed, 20 insertions(+), 21 deletions(-)
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
For copying from __iomem, we should use __ioread32_copy.
reported by sparse: sound/soc/intel/skylake/skl-debug.c:437:34: warning: incorrect type in argument 1 (different address spaces) sound/soc/intel/skylake/skl-debug.c:437:34: expected void [noderef] asn:2 *to sound/soc/intel/skylake/skl-debug.c:437:34: got unsigned char * sound/soc/intel/skylake/skl-debug.c:437:51: warning: incorrect type in argument 2 (different address spaces) sound/soc/intel/skylake/skl-debug.c:437:51: expected void const *from sound/soc/intel/skylake/skl-debug.c:437:51: got void [noderef] asn:2 *[assigned] fw_reg_addr
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- sound/soc/intel/skylake/skl-debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c index 212370bf704c..3466675f2678 100644 --- a/sound/soc/intel/skylake/skl-debug.c +++ b/sound/soc/intel/skylake/skl-debug.c @@ -188,7 +188,7 @@ static ssize_t fw_softreg_read(struct file *file, char __user *user_buf, memset(d->fw_read_buff, 0, FW_REG_BUF);
if (w0_stat_sz > 0) - __iowrite32_copy(d->fw_read_buff, fw_reg_addr, w0_stat_sz >> 2); + __ioread32_copy(d->fw_read_buff, fw_reg_addr, w0_stat_sz >> 2);
for (offset = 0; offset < FW_REG_SIZE; offset += 16) { ret += snprintf(tmp + ret, FW_REG_BUF - ret, "%#.4x: ", offset);
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
If ipc->ops.reply_msg_match is NULL, we may end up using uninitialized mask value.
reported by smatch: sound/soc/intel/common/sst-ipc.c:266 sst_ipc_reply_find_msg() error: uninitialized symbol 'mask'.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- sound/soc/intel/common/sst-ipc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c index 1186a03a88d6..6068bb697e22 100644 --- a/sound/soc/intel/common/sst-ipc.c +++ b/sound/soc/intel/common/sst-ipc.c @@ -223,6 +223,8 @@ struct ipc_message *sst_ipc_reply_find_msg(struct sst_generic_ipc *ipc,
if (ipc->ops.reply_msg_match != NULL) header = ipc->ops.reply_msg_match(header, &mask); + else + mask = (u64)-1;
if (list_empty(&ipc->rx_list)) { dev_err(ipc->dev, "error: rx list empty but received 0x%llx\n",
On 2019-08-27 16:17, Amadeusz Sławiński wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
If ipc->ops.reply_msg_match is NULL, we may end up using uninitialized mask value.
reported by smatch: sound/soc/intel/common/sst-ipc.c:266 sst_ipc_reply_find_msg() error: uninitialized symbol 'mask'.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com
sound/soc/intel/common/sst-ipc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c index 1186a03a88d6..6068bb697e22 100644 --- a/sound/soc/intel/common/sst-ipc.c +++ b/sound/soc/intel/common/sst-ipc.c @@ -223,6 +223,8 @@ struct ipc_message *sst_ipc_reply_find_msg(struct sst_generic_ipc *ipc,
if (ipc->ops.reply_msg_match != NULL) header = ipc->ops.reply_msg_match(header, &mask);
- else
mask = (u64)-1;
Please see linux/limits.h and check if this can't be replaced by an equivalent found there.
if (list_empty(&ipc->rx_list)) { dev_err(ipc->dev, "error: rx list empty but received 0x%llx\n",
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
We use snd_soc_dapm_new_control_unlocked for topology and have local declaration, instead declare it properly in header like already declared snd_soc_dapm_new_control.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- include/sound/soc-dapm.h | 3 +++ sound/soc/soc-topology.c | 6 ------ 2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 2aa73d6dd7be..30d8d0410952 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -404,6 +404,9 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, struct snd_soc_dapm_widget *snd_soc_dapm_new_control( struct snd_soc_dapm_context *dapm, const struct snd_soc_dapm_widget *widget); +struct snd_soc_dapm_widget *snd_soc_dapm_new_control_unlocked( + struct snd_soc_dapm_context *dapm, + const struct snd_soc_dapm_widget *widget); int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai); int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card); diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index b8690715abb5..aa9a1fca46fa 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -80,12 +80,6 @@ struct soc_tplg {
static int soc_tplg_process_headers(struct soc_tplg *tplg); static void soc_tplg_complete(struct soc_tplg *tplg); -struct snd_soc_dapm_widget * -snd_soc_dapm_new_control_unlocked(struct snd_soc_dapm_context *dapm, - const struct snd_soc_dapm_widget *widget); -struct snd_soc_dapm_widget * -snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm, - const struct snd_soc_dapm_widget *widget); static void soc_tplg_denum_remove_texts(struct soc_enum *se); static void soc_tplg_denum_remove_values(struct soc_enum *se);
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
When we are printing module params, we were actually printing module id instead of type, but debug message was saying that number we get is type. So print module type, as it is useful when debugging paths, but also keep printing module id, as it is used in all other logs.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- sound/soc/intel/skylake/skl-messages.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index d43496c5f29e..476ef1897961 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -867,8 +867,9 @@ static int skl_set_module_format(struct skl_dev *skl,
}
- dev_dbg(skl->dev, "Module type=%d config size: %d bytes\n", - module_config->id.module_id, param_size); + dev_dbg(skl->dev, "Module type=%d id=%d config size: %d bytes\n", + module_config->m_type, module_config->id.module_id, + param_size); print_hex_dump_debug("Module params:", DUMP_PREFIX_OFFSET, 8, 4, *param_data, param_size, false); return 0;
Currently topology is kept in memory while driver is running. It's unnecessary, as it's only needed during parsing.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/intel/skylake/skl-topology.c | 20 ++++++++++---------- sound/soc/intel/skylake/skl.h | 1 - 2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index ae5c75d03fdc..69cd7a81bf2a 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -3579,23 +3579,25 @@ int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus) * The complete tplg for SKL is loaded as index 0, we don't use * any other index */ - ret = snd_soc_tplg_component_load(component, - &skl_tplg_ops, fw, 0); + ret = snd_soc_tplg_component_load(component, &skl_tplg_ops, fw, 0); if (ret < 0) { dev_err(bus->dev, "tplg component load failed%d\n", ret); - release_firmware(fw); - return -EINVAL; + goto err; }
- skl->tplg = fw; ret = skl_tplg_create_pipe_widget_list(component); - if (ret < 0) - return ret; + if (ret < 0) { + dev_err(bus->dev, "tplg create pipe widget list failed%d\n", + ret); + goto err; + }
list_for_each_entry(ppl, &skl->ppl_list, node) skl_tplg_set_pipe_type(skl, ppl->pipe);
- return 0; +err: + release_firmware(fw); + return ret; }
void skl_tplg_exit(struct snd_soc_component *component, struct hdac_bus *bus) @@ -3609,6 +3611,4 @@ void skl_tplg_exit(struct snd_soc_component *component, struct hdac_bus *bus)
/* clean up topology */ snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); - - release_firmware(skl->tplg); } diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index f8c714153610..2bfbf59277c4 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -75,7 +75,6 @@ struct skl_dev { const char *fw_name; char tplg_name[64]; unsigned short pci_id; - const struct firmware *tplg;
int supend_active;
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
oem_table_id is 8 chars long, so we need to limit it, otherwise it may print some unprintable characters into dmesg.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- sound/soc/intel/skylake/skl-nhlt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index ab3d23c7bd65..19f328d71f24 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -136,7 +136,7 @@ int skl_nhlt_update_topology_bin(struct skl_dev *skl) struct hdac_bus *bus = skl_to_bus(skl); struct device *dev = bus->dev;
- dev_dbg(dev, "oem_id %.6s, oem_table_id %8s oem_revision %d\n", + dev_dbg(dev, "oem_id %.6s, oem_table_id %.8s oem_revision %d\n", nhlt->header.oem_id, nhlt->header.oem_table_id, nhlt->header.oem_revision);
On 8/27/19 9:17 AM, Amadeusz Sławiński wrote:
Series of small fixes:
- fixes few issues found during checking code with static checkers
- fix few prints
- define function in header, like it should be
- release topology when done with it
Amadeusz Sławiński (6): ASoC: Intel: Skylake: Use correct function to access iomem space ASoC: Intel: Fix use of potentially uninitialized variable ASoC: dapm: Expose snd_soc_dapm_new_control_unlocked properly ASoC: Intel: Skylake: Print module type instead of id ASoC: Intel: Skylake: Release topology when we are done with it ASoC: Intel: NHLT: Fix debug print format
LGTM
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
include/sound/soc-dapm.h | 3 +++ sound/soc/intel/common/sst-ipc.c | 2 ++ sound/soc/intel/skylake/skl-debug.c | 2 +- sound/soc/intel/skylake/skl-messages.c | 5 +++-- sound/soc/intel/skylake/skl-nhlt.c | 2 +- sound/soc/intel/skylake/skl-topology.c | 20 ++++++++++---------- sound/soc/intel/skylake/skl.h | 1 - sound/soc/soc-topology.c | 6 ------ 8 files changed, 20 insertions(+), 21 deletions(-)
participants (3)
-
Amadeusz Sławiński -
Cezary Rojewski -
Pierre-Louis Bossart