From 7726e49837af634accaec317c8d246d1d90d8fc5 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 16 Dec 2020 11:25:12 +0000 Subject: ASoC: wm_adsp: Improve handling of raw byte streams As the register map is 16-bit or 32-bit big-endian, the 24-bit DSP words appear padded and with the bytes swapped. When reading a raw stream of bytes, the pad bytes must be removed and the data bytes swapped back to their original order. The previous implementation of this assumed that the be32_to_cpu() in wm_adsp_read_data_block() would swap back to little-endian. But this is obviously only true on a little-endian CPU. It also made two walks through the data, once to endian-swap and again to strip the pad bytes. This patch re-works the code so that the endian-swap and unpad are done together in a single walk, and it is not dependent on the endianness of the CPU. The data_word_size argument to wm_adsp_remove_padding() has been dropped because currently this is always 3. Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20201216112512.26503-1-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- sound/soc/codecs/wm_adsp.c | 55 +++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 25 deletions(-) (limited to 'sound/soc/codecs/wm_adsp.c') diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index dec8716aa8ef..5b7d81a91df3 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -3664,12 +3664,12 @@ int wm_adsp_compr_get_caps(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(wm_adsp_compr_get_caps); -static int wm_adsp_read_data_block(struct wm_adsp *dsp, int mem_type, - unsigned int mem_addr, - unsigned int num_words, u32 *data) +static int wm_adsp_read_raw_data_block(struct wm_adsp *dsp, int mem_type, + unsigned int mem_addr, + unsigned int num_words, __be32 *data) { struct wm_adsp_region const *mem = wm_adsp_find_region(dsp, mem_type); - unsigned int i, reg; + unsigned int reg; int ret; if (!mem) @@ -3682,16 +3682,22 @@ static int wm_adsp_read_data_block(struct wm_adsp *dsp, int mem_type, if (ret < 0) return ret; - for (i = 0; i < num_words; ++i) - data[i] = be32_to_cpu(data[i]) & 0x00ffffffu; - return 0; } static inline int wm_adsp_read_data_word(struct wm_adsp *dsp, int mem_type, unsigned int mem_addr, u32 *data) { - return wm_adsp_read_data_block(dsp, mem_type, mem_addr, 1, data); + __be32 raw; + int ret; + + ret = wm_adsp_read_raw_data_block(dsp, mem_type, mem_addr, 1, &raw); + if (ret) + return ret; + + *data = be32_to_cpu(raw) & 0x00ffffffu; + + return 0; } static int wm_adsp_write_data_word(struct wm_adsp *dsp, int mem_type, @@ -3724,18 +3730,22 @@ static inline int wm_adsp_buffer_write(struct wm_adsp_compr_buf *buf, buf->host_buf_ptr + field_offset, data); } -static void wm_adsp_remove_padding(u32 *buf, int nwords, int data_word_size) +static void wm_adsp_remove_padding(u32 *buf, int nwords) { - u8 *pack_in = (u8 *)buf; + const __be32 *pack_in = (__be32 *)buf; u8 *pack_out = (u8 *)buf; - int i, j; + int i; - /* Remove the padding bytes from the data read from the DSP */ + /* + * DSP words from the register map have pad bytes and the data bytes + * are in swapped order. This swaps back to the original little-endian + * order and strips the pad bytes. + */ for (i = 0; i < nwords; i++) { - for (j = 0; j < data_word_size; j++) - *pack_out++ = *pack_in++; - - pack_in += sizeof(*buf) - data_word_size; + u32 word = be32_to_cpu(*pack_in++); + *pack_out++ = (u8)word; + *pack_out++ = (u8)(word >> 8); + *pack_out++ = (u8)(word >> 16); } } @@ -3919,12 +3929,7 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) return -EINVAL; } - for (i = 0; i < ARRAY_SIZE(coeff_v1.name); i++) - coeff_v1.name[i] = be32_to_cpu(coeff_v1.name[i]); - - wm_adsp_remove_padding((u32 *)&coeff_v1.name, - ARRAY_SIZE(coeff_v1.name), - WM_ADSP_DATA_WORD_SIZE); + wm_adsp_remove_padding((u32 *)&coeff_v1.name, ARRAY_SIZE(coeff_v1.name)); buf->name = kasprintf(GFP_KERNEL, "%s-dsp-%s", ctl->dsp->part, (char *)&coeff_v1.name); @@ -4263,12 +4268,12 @@ static int wm_adsp_buffer_capture_block(struct wm_adsp_compr *compr, int target) return 0; /* Read data from DSP */ - ret = wm_adsp_read_data_block(buf->dsp, mem_type, adsp_addr, - nwords, compr->raw_buf); + ret = wm_adsp_read_raw_data_block(buf->dsp, mem_type, adsp_addr, + nwords, compr->raw_buf); if (ret < 0) return ret; - wm_adsp_remove_padding(compr->raw_buf, nwords, WM_ADSP_DATA_WORD_SIZE); + wm_adsp_remove_padding(compr->raw_buf, nwords); /* update read index to account for words read */ buf->read_index += nwords; -- cgit v1.2.3 From a0b653e89a3afd2a833f23589a83488fac842ddb Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 30 Dec 2020 17:24:26 +0000 Subject: ASoC: wm_adsp: Only use __be32 for big-endian data This fixes some minor cases where u32 or unsigned int types were used to store big-endian data, and __be32 types used to store both big-endian and cpu-endian data. This was producing sparse warnings. Most cases resulted from using the same variable to hold the big-endian value and its converted cpu-endian value. These can be simply fixed by introducing another local variable, or avoiding storing the intermediate value back into the original variable. One special case is the raw_buf used in the compressed streams to transfer data from DSP to user-side. The endian conversion happens in-place (as there's no point introducing another buffer) so a cast to (__be32 *) is added when passing it to wm_adsp_read_raw_data_block(). Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20201230172427.13865-1-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- sound/soc/codecs/wm_adsp.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) (limited to 'sound/soc/codecs/wm_adsp.c') diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 5b7d81a91df3..8cfa8ac1b8c4 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -980,7 +980,7 @@ static int wm_coeff_write_acked_control(struct wm_coeff_ctl *ctl, unsigned int event_id) { struct wm_adsp *dsp = ctl->dsp; - u32 val = cpu_to_be32(event_id); + __be32 val = cpu_to_be32(event_id); unsigned int reg; int i, ret; @@ -3704,6 +3704,7 @@ static int wm_adsp_write_data_word(struct wm_adsp *dsp, int mem_type, unsigned int mem_addr, u32 data) { struct wm_adsp_region const *mem = wm_adsp_find_region(dsp, mem_type); + __be32 val = cpu_to_be32(data & 0x00ffffffu); unsigned int reg; if (!mem) @@ -3711,9 +3712,7 @@ static int wm_adsp_write_data_word(struct wm_adsp *dsp, int mem_type, reg = dsp->ops->region_to_reg(mem, mem_addr); - data = cpu_to_be32(data & 0x00ffffffu); - - return regmap_raw_write(dsp->regmap, reg, &data, sizeof(data)); + return regmap_raw_write(dsp->regmap, reg, &val, sizeof(val)); } static inline int wm_adsp_buffer_read(struct wm_adsp_compr_buf *buf, @@ -3870,7 +3869,8 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) { struct wm_adsp_host_buf_coeff_v1 coeff_v1; struct wm_adsp_compr_buf *buf; - unsigned int val, reg; + unsigned int reg, version; + __be32 bufp; int ret, i; ret = wm_coeff_base_reg(ctl, ®); @@ -3878,17 +3878,17 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) return ret; for (i = 0; i < 5; ++i) { - ret = regmap_raw_read(ctl->dsp->regmap, reg, &val, sizeof(val)); + ret = regmap_raw_read(ctl->dsp->regmap, reg, &bufp, sizeof(bufp)); if (ret < 0) return ret; - if (val) + if (bufp) break; usleep_range(1000, 2000); } - if (!val) { + if (!bufp) { adsp_err(ctl->dsp, "Failed to acquire host buffer\n"); return -EIO; } @@ -3898,7 +3898,7 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) return -ENOMEM; buf->host_buf_mem_type = ctl->alg_region.type; - buf->host_buf_ptr = be32_to_cpu(val); + buf->host_buf_ptr = be32_to_cpu(bufp); ret = wm_adsp_buffer_populate(buf); if (ret < 0) @@ -3918,14 +3918,13 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) if (ret < 0) return ret; - coeff_v1.versions = be32_to_cpu(coeff_v1.versions); - val = coeff_v1.versions & HOST_BUF_COEFF_COMPAT_VER_MASK; - val >>= HOST_BUF_COEFF_COMPAT_VER_SHIFT; + version = be32_to_cpu(coeff_v1.versions) & HOST_BUF_COEFF_COMPAT_VER_MASK; + version >>= HOST_BUF_COEFF_COMPAT_VER_SHIFT; - if (val > HOST_BUF_COEFF_SUPPORTED_COMPAT_VER) { + if (version > HOST_BUF_COEFF_SUPPORTED_COMPAT_VER) { adsp_err(ctl->dsp, "Host buffer coeff ver %u > supported version %u\n", - val, HOST_BUF_COEFF_SUPPORTED_COMPAT_VER); + version, HOST_BUF_COEFF_SUPPORTED_COMPAT_VER); return -EINVAL; } @@ -3935,9 +3934,9 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl) (char *)&coeff_v1.name); compr_dbg(buf, "host_buf_ptr=%x coeff version %u\n", - buf->host_buf_ptr, val); + buf->host_buf_ptr, version); - return val; + return version; } static int wm_adsp_buffer_init(struct wm_adsp *dsp) @@ -4269,7 +4268,7 @@ static int wm_adsp_buffer_capture_block(struct wm_adsp_compr *compr, int target) /* Read data from DSP */ ret = wm_adsp_read_raw_data_block(buf->dsp, mem_type, adsp_addr, - nwords, compr->raw_buf); + nwords, (__be32 *)compr->raw_buf); if (ret < 0) return ret; -- cgit v1.2.3 From f6212e0ab3ff28d4e2e53a520496a052c241df39 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 30 Dec 2020 17:24:27 +0000 Subject: ASoC: wm_adsp: Use snd_ctl_elem_type_t for control types Sparse will complain about trying to convert between values declared as snd_ctl_elem_type_t and other types. This patch converts to consistently use snd_ctl_elem_type_t for control type values. A __force cast is needed in a couple of cases where the control type value is parsed out of a DSP data block. Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20201230172427.13865-2-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- sound/soc/codecs/wm_adsp.c | 12 +++++++----- sound/soc/codecs/wmfw.h | 6 +++--- 2 files changed, 10 insertions(+), 8 deletions(-) (limited to 'sound/soc/codecs/wm_adsp.c') diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 8cfa8ac1b8c4..f8ad768364c2 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -619,7 +619,7 @@ struct wm_coeff_ctl { unsigned int set:1; struct soc_bytes_ext bytes_ext; unsigned int flags; - unsigned int type; + snd_ctl_elem_type_t type; }; static const char *wm_adsp_mem_region_name(unsigned int type) @@ -1420,7 +1420,7 @@ static int wm_adsp_create_control(struct wm_adsp *dsp, const struct wm_adsp_alg_region *alg_region, unsigned int offset, unsigned int len, const char *subname, unsigned int subname_len, - unsigned int flags, unsigned int type) + unsigned int flags, snd_ctl_elem_type_t type) { struct wm_coeff_ctl *ctl; struct wmfw_ctl_work *ctl_work; @@ -1554,7 +1554,7 @@ struct wm_coeff_parsed_coeff { int mem_type; const u8 *name; int name_len; - int ctl_type; + snd_ctl_elem_type_t ctl_type; int flags; int len; }; @@ -1649,7 +1649,7 @@ static inline void wm_coeff_parse_coeff(struct wm_adsp *dsp, const u8 **data, blk->mem_type = le16_to_cpu(raw->hdr.type); blk->name = raw->name; blk->name_len = strlen(raw->name); - blk->ctl_type = le16_to_cpu(raw->ctl_type); + blk->ctl_type = (__force snd_ctl_elem_type_t)le16_to_cpu(raw->ctl_type); blk->flags = le16_to_cpu(raw->flags); blk->len = le32_to_cpu(raw->len); break; @@ -1662,7 +1662,9 @@ static inline void wm_coeff_parse_coeff(struct wm_adsp *dsp, const u8 **data, &blk->name); wm_coeff_parse_string(sizeof(u8), &tmp, NULL); wm_coeff_parse_string(sizeof(u16), &tmp, NULL); - blk->ctl_type = wm_coeff_parse_int(sizeof(raw->ctl_type), &tmp); + blk->ctl_type = + (__force snd_ctl_elem_type_t)wm_coeff_parse_int(sizeof(raw->ctl_type), + &tmp); blk->flags = wm_coeff_parse_int(sizeof(raw->flags), &tmp); blk->len = wm_coeff_parse_int(sizeof(raw->len), &tmp); diff --git a/sound/soc/codecs/wmfw.h b/sound/soc/codecs/wmfw.h index 7423272c30e9..f3d51602f85c 100644 --- a/sound/soc/codecs/wmfw.h +++ b/sound/soc/codecs/wmfw.h @@ -24,9 +24,9 @@ #define WMFW_CTL_FLAG_READABLE 0x0001 /* Non-ALSA coefficient types start at 0x1000 */ -#define WMFW_CTL_TYPE_ACKED 0x1000 /* acked control */ -#define WMFW_CTL_TYPE_HOSTEVENT 0x1001 /* event control */ -#define WMFW_CTL_TYPE_HOST_BUFFER 0x1002 /* host buffer pointer */ +#define WMFW_CTL_TYPE_ACKED ((__force snd_ctl_elem_type_t)0x1000) /* acked control */ +#define WMFW_CTL_TYPE_HOSTEVENT ((__force snd_ctl_elem_type_t)0x1001) /* event control */ +#define WMFW_CTL_TYPE_HOST_BUFFER ((__force snd_ctl_elem_type_t)0x1002) /* host buffer pointer */ struct wmfw_header { char magic[4]; -- cgit v1.2.3 From fe9989fb25b0cea6414e72e0514c70ed8b158c28 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 11 Jan 2021 13:38:25 +0000 Subject: ASoC: wm_adsp: Fix uninitialized variable warnings wm_adsp_read_data_word() used if (ret) to check for an error from wm_adsp_read_raw_data_block(). While this is perfectly valid, wm_adsp_read_raw_data_block() itself uses if (ret < 0) and three calls to wm_adsp_read_data_word() also use if (ret < 0). This creates an error check chain like this: 1st) if (ret < 0) return ret; 2nd) if (ret) return ret; 3rd) if (ret < 0) ... This can confuse the compiler into thinking that there are possible returns > 0 from the middle if() that are not handled by the final if(). If this was true it would lead to using uninitialized variables later in the outer function. Fix this by changing the test in wm_adsp_read_data_word() to be if (ret < 0). Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20210111133825.8758-1-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- sound/soc/codecs/wm_adsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sound/soc/codecs/wm_adsp.c') diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index f8ad768364c2..1fc7bc1970ea 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -3694,7 +3694,7 @@ static inline int wm_adsp_read_data_word(struct wm_adsp *dsp, int mem_type, int ret; ret = wm_adsp_read_raw_data_block(dsp, mem_type, mem_addr, 1, &raw); - if (ret) + if (ret < 0) return ret; *data = be32_to_cpu(raw) & 0x00ffffffu; -- cgit v1.2.3 From 6e9586361e145cd688e525880e1f84c0ccf57566 Mon Sep 17 00:00:00 2001 From: Charles Keepax Date: Thu, 11 Feb 2021 17:21:06 +0000 Subject: ASoC: wm_adsp: Remove unused control callback structure This callback structure has never been used and it is not clear why it was added in the first place. Remove it to clear up the code a little. Signed-off-by: Charles Keepax Link: https://lore.kernel.org/r/20210211172106.16258-1-ckeepax@opensource.cirrus.com Signed-off-by: Mark Brown --- sound/soc/codecs/wm_adsp.c | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'sound/soc/codecs/wm_adsp.c') diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 262c2db4f81c..070ca7d8c661 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -595,13 +595,6 @@ static const struct { [WM_ADSP_FW_MISC] = { .file = "misc" }, }; -struct wm_coeff_ctl_ops { - int (*xget)(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol); - int (*xput)(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol); -}; - struct wm_coeff_ctl { const char *name; const char *fw_name; @@ -609,7 +602,6 @@ struct wm_coeff_ctl { const char *subname; unsigned int subname_len; struct wm_adsp_alg_region alg_region; - struct wm_coeff_ctl_ops ops; struct wm_adsp *dsp; unsigned int enabled:1; struct list_head list; @@ -1497,8 +1489,6 @@ static int wm_adsp_create_control(struct wm_adsp *dsp, } ctl->enabled = 1; ctl->set = 0; - ctl->ops.xget = wm_coeff_get; - ctl->ops.xput = wm_coeff_put; ctl->dsp = dsp; ctl->flags = flags; -- cgit v1.2.3