[PATCH] 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 rf@opensource.cirrus.com --- sound/soc/codecs/wm_adsp.c | 55 +++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index bcf18bf15a02..7c52ada10af3 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -3662,12 +3662,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) @@ -3680,16 +3680,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, @@ -3722,18 +3728,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); } }
@@ -3917,12 +3927,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); @@ -4261,12 +4266,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;
Hi Richard,
I love your patch! Perhaps something to improve:
[auto build test WARNING on asoc/for-next] [also build test WARNING on v5.10 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Richard-Fitzgerald/ASoC-wm_adsp-Imp... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next config: arm64-randconfig-s032-20201223 (attached as .config) compiler: aarch64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.3-184-g1b896707-dirty # https://github.com/0day-ci/linux/commit/e68819993ab2e0f2870bf9ca578f6b371335... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Richard-Fitzgerald/ASoC-wm_adsp-Improve-handling-of-raw-byte-streams/20201216-193614 git checkout e68819993ab2e0f2870bf9ca578f6b3713358419 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
"sparse warnings: (new ones prefixed by >>)" sound/soc/codecs/wm_adsp.c:983:19: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned int [usertype] val @@ got restricted __be32 [usertype] @@ sound/soc/codecs/wm_adsp.c:983:19: sparse: expected unsigned int [usertype] val sound/soc/codecs/wm_adsp.c:983:19: sparse: got restricted __be32 [usertype] sound/soc/codecs/wm_adsp.c:1710:22: sparse: sparse: restricted snd_ctl_elem_type_t degrades to integer sound/soc/codecs/wm_adsp.c:2329:54: sparse: sparse: incorrect type in argument 8 (different base types) @@ expected unsigned int type @@ got restricted snd_ctl_elem_type_t [usertype] @@ sound/soc/codecs/wm_adsp.c:2329:54: sparse: expected unsigned int type sound/soc/codecs/wm_adsp.c:2329:54: sparse: got restricted snd_ctl_elem_type_t [usertype] sound/soc/codecs/wm_adsp.c:2350:54: sparse: sparse: incorrect type in argument 8 (different base types) @@ expected unsigned int type @@ got restricted snd_ctl_elem_type_t [usertype] @@ sound/soc/codecs/wm_adsp.c:2350:54: sparse: expected unsigned int type sound/soc/codecs/wm_adsp.c:2350:54: sparse: got restricted snd_ctl_elem_type_t [usertype] sound/soc/codecs/wm_adsp.c:2437:54: sparse: sparse: incorrect type in argument 8 (different base types) @@ expected unsigned int type @@ got restricted snd_ctl_elem_type_t [usertype] @@ sound/soc/codecs/wm_adsp.c:2437:54: sparse: expected unsigned int type sound/soc/codecs/wm_adsp.c:2437:54: sparse: got restricted snd_ctl_elem_type_t [usertype] sound/soc/codecs/wm_adsp.c:2458:54: sparse: sparse: incorrect type in argument 8 (different base types) @@ expected unsigned int type @@ got restricted snd_ctl_elem_type_t [usertype] @@ sound/soc/codecs/wm_adsp.c:2458:54: sparse: expected unsigned int type sound/soc/codecs/wm_adsp.c:2458:54: sparse: got restricted snd_ctl_elem_type_t [usertype] sound/soc/codecs/wm_adsp.c:2479:54: sparse: sparse: incorrect type in argument 8 (different base types) @@ expected unsigned int type @@ got restricted snd_ctl_elem_type_t [usertype] @@ sound/soc/codecs/wm_adsp.c:2479:54: sparse: expected unsigned int type sound/soc/codecs/wm_adsp.c:2479:54: sparse: got restricted snd_ctl_elem_type_t [usertype] sound/soc/codecs/wm_adsp.c:3714:14: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] data @@ got restricted __be32 [usertype] @@ sound/soc/codecs/wm_adsp.c:3714:14: sparse: expected unsigned int [usertype] data sound/soc/codecs/wm_adsp.c:3714:14: sparse: got restricted __be32 [usertype] sound/soc/codecs/wm_adsp.c:3901:29: sparse: sparse: cast to restricted __be32 sound/soc/codecs/wm_adsp.c:3921:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be32 [addressable] [usertype] versions @@ got unsigned int [usertype] @@ sound/soc/codecs/wm_adsp.c:3921:27: sparse: expected restricted __be32 [addressable] [usertype] versions sound/soc/codecs/wm_adsp.c:3921:27: sparse: got unsigned int [usertype] sound/soc/codecs/wm_adsp.c:3922:23: sparse: sparse: restricted __be32 degrades to integer
sound/soc/codecs/wm_adsp.c:4272:56: sparse: sparse: incorrect type in argument 5 (different base types) @@ expected restricted __be32 [usertype] *data @@ got unsigned int [usertype] *raw_buf @@
sound/soc/codecs/wm_adsp.c:4272:56: sparse: expected restricted __be32 [usertype] *data sound/soc/codecs/wm_adsp.c:4272:56: sparse: got unsigned int [usertype] *raw_buf
vim +4272 sound/soc/codecs/wm_adsp.c
565ace464105cb9 Charles Keepax 2016-01-06 4238 83a40ce993cda07 Charles Keepax 2016-01-06 4239 static int wm_adsp_buffer_capture_block(struct wm_adsp_compr *compr, int target) 83a40ce993cda07 Charles Keepax 2016-01-06 4240 { 83a40ce993cda07 Charles Keepax 2016-01-06 4241 struct wm_adsp_compr_buf *buf = compr->buf; 83a40ce993cda07 Charles Keepax 2016-01-06 4242 unsigned int adsp_addr; 83a40ce993cda07 Charles Keepax 2016-01-06 4243 int mem_type, nwords, max_read; cc7d6ce90216d10 Charles Keepax 2019-02-22 4244 int i, ret; 83a40ce993cda07 Charles Keepax 2016-01-06 4245 83a40ce993cda07 Charles Keepax 2016-01-06 4246 /* Calculate read parameters */ 83a40ce993cda07 Charles Keepax 2016-01-06 4247 for (i = 0; i < wm_adsp_fw[buf->dsp->fw].caps->num_regions; ++i) 83a40ce993cda07 Charles Keepax 2016-01-06 4248 if (buf->read_index < buf->regions[i].cumulative_size) 83a40ce993cda07 Charles Keepax 2016-01-06 4249 break; 83a40ce993cda07 Charles Keepax 2016-01-06 4250 83a40ce993cda07 Charles Keepax 2016-01-06 4251 if (i == wm_adsp_fw[buf->dsp->fw].caps->num_regions) 83a40ce993cda07 Charles Keepax 2016-01-06 4252 return -EINVAL; 83a40ce993cda07 Charles Keepax 2016-01-06 4253 83a40ce993cda07 Charles Keepax 2016-01-06 4254 mem_type = buf->regions[i].mem_type; 83a40ce993cda07 Charles Keepax 2016-01-06 4255 adsp_addr = buf->regions[i].base_addr + 83a40ce993cda07 Charles Keepax 2016-01-06 4256 (buf->read_index - buf->regions[i].offset); 83a40ce993cda07 Charles Keepax 2016-01-06 4257 83a40ce993cda07 Charles Keepax 2016-01-06 4258 max_read = wm_adsp_compr_frag_words(compr); 83a40ce993cda07 Charles Keepax 2016-01-06 4259 nwords = buf->regions[i].cumulative_size - buf->read_index; 83a40ce993cda07 Charles Keepax 2016-01-06 4260 83a40ce993cda07 Charles Keepax 2016-01-06 4261 if (nwords > target) 83a40ce993cda07 Charles Keepax 2016-01-06 4262 nwords = target; 83a40ce993cda07 Charles Keepax 2016-01-06 4263 if (nwords > buf->avail) 83a40ce993cda07 Charles Keepax 2016-01-06 4264 nwords = buf->avail; 83a40ce993cda07 Charles Keepax 2016-01-06 4265 if (nwords > max_read) 83a40ce993cda07 Charles Keepax 2016-01-06 4266 nwords = max_read; 83a40ce993cda07 Charles Keepax 2016-01-06 4267 if (!nwords) 83a40ce993cda07 Charles Keepax 2016-01-06 4268 return 0; 83a40ce993cda07 Charles Keepax 2016-01-06 4269 83a40ce993cda07 Charles Keepax 2016-01-06 4270 /* Read data from DSP */ e68819993ab2e0f Richard Fitzgerald 2020-12-16 4271 ret = wm_adsp_read_raw_data_block(buf->dsp, mem_type, adsp_addr, 83a40ce993cda07 Charles Keepax 2016-01-06 @4272 nwords, compr->raw_buf); 83a40ce993cda07 Charles Keepax 2016-01-06 4273 if (ret < 0) 83a40ce993cda07 Charles Keepax 2016-01-06 4274 return ret; 83a40ce993cda07 Charles Keepax 2016-01-06 4275 e68819993ab2e0f Richard Fitzgerald 2020-12-16 4276 wm_adsp_remove_padding(compr->raw_buf, nwords); 83a40ce993cda07 Charles Keepax 2016-01-06 4277 83a40ce993cda07 Charles Keepax 2016-01-06 4278 /* update read index to account for words read */ 83a40ce993cda07 Charles Keepax 2016-01-06 4279 buf->read_index += nwords; 83a40ce993cda07 Charles Keepax 2016-01-06 4280 if (buf->read_index == wm_adsp_buffer_size(buf)) 83a40ce993cda07 Charles Keepax 2016-01-06 4281 buf->read_index = 0; 83a40ce993cda07 Charles Keepax 2016-01-06 4282 83a40ce993cda07 Charles Keepax 2016-01-06 4283 ret = wm_adsp_buffer_write(buf, HOST_BUFFER_FIELD(next_read_index), 83a40ce993cda07 Charles Keepax 2016-01-06 4284 buf->read_index); 83a40ce993cda07 Charles Keepax 2016-01-06 4285 if (ret < 0) 83a40ce993cda07 Charles Keepax 2016-01-06 4286 return ret; 83a40ce993cda07 Charles Keepax 2016-01-06 4287 83a40ce993cda07 Charles Keepax 2016-01-06 4288 /* update avail to account for words read */ 83a40ce993cda07 Charles Keepax 2016-01-06 4289 buf->avail -= nwords; 83a40ce993cda07 Charles Keepax 2016-01-06 4290 83a40ce993cda07 Charles Keepax 2016-01-06 4291 return nwords; 83a40ce993cda07 Charles Keepax 2016-01-06 4292 } 83a40ce993cda07 Charles Keepax 2016-01-06 4293
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, 16 Dec 2020 11:25:12 +0000, Richard Fitzgerald wrote:
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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: wm_adsp: Improve handling of raw byte streams commit: 7726e49837af634accaec317c8d246d1d90d8fc5
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
kernel test robot
-
Mark Brown
-
Richard Fitzgerald